Re: [Qemu-devel] [PATCH v3] scripts/checkpatch.pl: add check for `while` and `for`
On Wed, Feb 28, 2018 at 11:31:29AM +0800, Su Hang wrote:
Adding check for `while` and `for` statements, which condition has more than
one line.
The former checkpatch.pl can check `if` statement, which condition has more
than one line, whether block misses brace round, like this:
'''
if (cond1 ||
cond2)
statement;
'''
But it doesn't do the same check for `for` and `while` statements.
Using `(?:...)` instead of `(...)` in regex pattern catch.
Because `(?:...)` is faster and avoids unwanted side-effect.
Suggested-by: Stefan Hajnoczi
Suggested-by: Eric Blake
Suggested-by: Thomas Huth
Signed-off-by: Su Hang
---
scripts/checkpatch.pl | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 10c138344fa9..bed1dbbd54d1 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2352,9 +2352,9 @@ sub process {
}
}
-# check for missing bracing round if etc
- if ($line =~ /(^.*)\b(for|while|if)\b/ &&
- $line !~ /\#\s*(for|while|if)/) {
+# check for missing bracing around if etc
+ if ($line =~ /(^.*)\b(?:for|while|if)\b/ &&
+ $line !~ /\#\s*(?:for|while|if)/) {
It's a nit, but for readability I would suggest indenting the line
above an extra tab or two to make it clear that it is part of the
condition rather than the block. (You can see other instances of
this in the file).
Otherwise:
Reviewed-by: Darren Kenny
Thanks,
Darren.
my ($level, $endln, @chunks) =
ctx_statement_full($linenr, $realcnt, 1);
if ($dbg_adv_apw) {
--
2.7.4
Re: [Qemu-devel] [qemu-s390x] [PATCH] vfio-ccw: license text should indicate GPL v2 or later
On 02/27/2018 06:32 PM, Cornelia Huck wrote: > The license text currently specifies "any version" of the GPL. It > is unlikely that GPL v1 was ever intended; change this to the > standard "or any later version" text. > > Cc: Dong Jia Shi > Cc: Xiao Feng Ren > Cc: Pierre Morel > Signed-off-by: Cornelia Huck Acked-by: Christian Borntraeger > --- > hw/vfio/ccw.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c > index 16713f2c52..4e5855741a 100644 > --- a/hw/vfio/ccw.c > +++ b/hw/vfio/ccw.c > @@ -6,8 +6,8 @@ > *Xiao Feng Ren > *Pierre Morel > * > - * This work is licensed under the terms of the GNU GPL, version 2 or(at > - * your option) any version. See the COPYING file in the top-level > + * This work is licensed under the terms of the GNU GPL, version 2 or (at > + * your option) any later version. See the COPYING file in the top-level > * directory. > */ >
Re: [Qemu-devel] [RFC QEMU PATCH v4 05/10] xen-hvm: initialize fw_cfg interface
On 02/27/18 16:46 +, Anthony PERARD wrote:
> On Thu, Dec 07, 2017 at 06:18:07PM +0800, Haozhong Zhang wrote:
> > Xen is going to reuse QEMU to build ACPI of some devices (e.g., NFIT
> > and SSDT for NVDIMM) for HVM domains. The existing QEMU ACPI build
> > code requires a fw_cfg interface which will also be used to pass QEMU
> > built ACPI to Xen. Therefore, we need to initialize fw_cfg when any
> > ACPI is going to be built by QEMU.
> >
> > Signed-off-by: Haozhong Zhang
> > ---
> > Cc: Stefano Stabellini
> > Cc: Anthony Perard
> > Cc: "Michael S. Tsirkin"
> > Cc: Paolo Bonzini
> > Cc: Richard Henderson
> > Cc: Eduardo Habkost
> > ---
> > hw/i386/xen/xen-hvm.c | 12
> > 1 file changed, 12 insertions(+)
> >
> > diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
> > index fe01b7a025..4b29f4052b 100644
> > --- a/hw/i386/xen/xen-hvm.c
> > +++ b/hw/i386/xen/xen-hvm.c
> > @@ -14,6 +14,7 @@
> > #include "hw/pci/pci.h"
> > #include "hw/i386/pc.h"
> > #include "hw/i386/apic-msidef.h"
> > +#include "hw/loader.h"
> > #include "hw/xen/xen_common.h"
> > #include "hw/xen/xen_backend.h"
> > #include "qmp-commands.h"
> > @@ -1234,6 +1235,14 @@ static void xen_wakeup_notifier(Notifier *notifier,
> > void *data)
> > xc_set_hvm_param(xen_xc, xen_domid, HVM_PARAM_ACPI_S_STATE, 0);
> > }
> >
> > +static void xen_fw_cfg_init(PCMachineState *pcms)
> > +{
>
> The fw_cfg interface might already be initialized, it is used for
> "direct kernel boot" on hvm. It is initialized in xen_load_linux().
>
xen_hvm_init() --> xen_fw_cfg_init() are called before
xen_load_linux(). I'll add a check in xen_load_linux() to avoid
redoing fw_cfg_init_io and rom_set_fw.
Haozhong
> > +FWCfgState *fw_cfg = fw_cfg_init_io(FW_CFG_IO_BASE);
> > +
> > +rom_set_fw(fw_cfg);
> > +pcms->fw_cfg = fw_cfg;
> > +}
> > +
> > void xen_hvm_init(PCMachineState *pcms, MemoryRegion **ram_memory)
> > {
> > int i, rc;
> > @@ -1384,6 +1393,9 @@ void xen_hvm_init(PCMachineState *pcms, MemoryRegion
> > **ram_memory)
> >
> > /* Disable ACPI build because Xen handles it */
> > pcms->acpi_build_enabled = false;
> > +if (pcms->acpi_build_enabled) {
> > +xen_fw_cfg_init(pcms);
> > +}
> >
> > return;
> >
> > --
> > 2.15.1
> >
>
> --
> Anthony PERARD
Re: [Qemu-devel] [PATCH V6 3/4] tests/migration: Add migration-test header file
On Tue, Feb 27, 2018 at 04:27:33PM +, Dr. David Alan Gilbert wrote:
> * Wei Huang (w...@redhat.com) wrote:
> >
> >
> > On 02/27/2018 05:57 AM, Dr. David Alan Gilbert wrote:
> > > * Wei Huang (w...@redhat.com) wrote:
> > >> This patch moves the settings related migration-test from the
> > >> migration-test.c file to a seperate header file. It also renames the
> > >> x86-a-b-bootblock.s file extension from .s to .S, allowing gcc
> > >> pre-processor to include the C-style header file correctly.
> > >>
> > >> Signed-off-by: Wei Huang
> > >> Reviewed-by: Andrew Jones
> > >> ---
> > >> tests/migration-test.c | 28
> > >> +++---
> > >> tests/migration/Makefile | 4 ++--
> > >> tests/migration/migration-test.h | 18 ++
> > >> .../{x86-a-b-bootblock.s => x86-a-b-bootblock.S} | 7 +++---
> > >> tests/migration/x86-a-b-bootblock.h| 2 +-
> > >> 5 files changed, 39 insertions(+), 20 deletions(-)
> > >> create mode 100644 tests/migration/migration-test.h
> > >> rename tests/migration/{x86-a-b-bootblock.s => x86-a-b-bootblock.S}
> > >> (94%)
> > >>
> > >> diff --git a/tests/migration-test.c b/tests/migration-test.c
> > >> index 74f9361bdd..c88b7e7a19 100644
> > >> --- a/tests/migration-test.c
> > >> +++ b/tests/migration-test.c
> > >> @@ -21,10 +21,10 @@
> > >> #include "sysemu/sysemu.h"
> > >> #include "hw/nvram/chrp_nvram.h"
> > >>
> > >> -#define MIN_NVRAM_SIZE 8192 /* from spapr_nvram.c */
> > >> +#include "migration/migration-test.h"
> > >>
> > >> -const unsigned start_address = 1024 * 1024;
> > >> -const unsigned end_address = 100 * 1024 * 1024;
> > >> +const unsigned start_address = TEST_MEM_START;
> > >> +const unsigned end_address = TEST_MEM_END;
> > >> bool got_stop;
> > >>
> > >> #if defined(__linux__)
> > >> @@ -77,8 +77,8 @@ static bool ufd_version_check(void)
> > >>
> > >> static const char *tmpfs;
> > >>
> > >> -/* A simple PC boot sector that modifies memory (1-100MB) quickly
> > >> - * outputting a 'B' every so often if it's still running.
> > >> +/* The boot file modifies memory area in [start_address, end_address)
> > >> + * repeatedly. It outputs a 'B' at a fixed rate while it's still
> > >> running.
> > >> */
> > >> #include "tests/migration/x86-a-b-bootblock.h"
> > >>
> > >> @@ -104,9 +104,8 @@ static void init_bootfile_ppc(const char *bootpath)
> > >> memcpy(header->name, "common", 6);
> > >> chrp_nvram_finish_partition(header, MIN_NVRAM_SIZE);
> > >>
> > >> -/* FW_MAX_SIZE is 4MB, but slof.bin is only 900KB,
> > >> - * so let's modify memory between 1MB and 100MB
> > >> - * to do like PC bootsector
> > >> +/* FW_MAX_SIZE is 4MB, but slof.bin is only 900KB. So it is OK to
> > >> modify
> > >> + * memory between start_address and end_address like PC bootsector
> > >> does.
> > >> */
> > >>
> > >> sprintf(buf + 16,
> > >> @@ -263,11 +262,11 @@ static void wait_for_migration_pass(QTestState
> > >> *who)
> > >> static void check_guests_ram(QTestState *who)
> > >> {
> > >> /* Our ASM test will have been incrementing one byte from each page
> > >> from
> > >> - * 1MB to <100MB in order.
> > >> - * This gives us a constraint that any page's byte should be equal
> > >> or less
> > >> - * than the previous pages byte (mod 256); and they should all be
> > >> equal
> > >> - * except for one transition at the point where we meet the
> > >> incrementer.
> > >> - * (We're running this with the guest stopped).
> > >> + * start_address to < end_address in order. This gives us a
> > >> constraint
> > >> + * that any page's byte should be equal or less than the previous
> > >> pages
> > >> + * byte (mod 256); and they should all be equal except for one
> > >> transition
> > >> + * at the point where we meet the incrementer. (We're running this
> > >> with
> > >> + * the guest stopped).
> > >
> > > No, please don't modify the other architectures.
> > >
> > > It's OK for you to get the start/end value from the define in the header
> > > but we can't assume they're movable; they're architecture specific
> > > values that are chosen to fit where we know we have RAM.
> >
> > Drew didn't like the idea of having #define TEST_MEM_START/TEST_MEM_END
> > in headerfile, and in the meanwhile, we still use 1MB/100MB in comments.
> > Anyway we didn't modify the underneath semantic for other architectures.
>
> The problem is you have to be VERY careful; because while it might make
> sense to move the start for one architecture it might break on another.
We should make the addreses explicitly arch-specific. In the header create
arch-specific defines like below. In the code don't initialize
start/end_address to anything. Instead only set them to their arch-
specific defines in their arch-specific blocks of test_migrate_start().
#define X86_TEST_MEM_START (1024*1024)
#defi
Re: [Qemu-devel] [PATCH v3 09/29] postcopy: Allow registering of fd handler
On Fri, Feb 16, 2018 at 01:16:05PM +, Dr. David Alan Gilbert (git) wrote:
[...]
> diff --git a/migration/postcopy-ram.h b/migration/postcopy-ram.h
> index bee21d4401..4bda5aa509 100644
> --- a/migration/postcopy-ram.h
> +++ b/migration/postcopy-ram.h
> @@ -141,4 +141,25 @@ void postcopy_remove_notifier(NotifierWithReturn *n);
> /* Call the notifier list set by postcopy_add_start_notifier */
> int postcopy_notify(enum PostcopyNotifyReason reason, Error **errp);
>
> +struct PostCopyFD;
> +
> +/* ufd is a pointer to the struct uffd_msg *TODO: more Portable! */
> +typedef int (*pcfdhandler)(struct PostCopyFD *pcfd, void *ufd);
> +
> +struct PostCopyFD {
> +int fd;
> +/* Data to pass to handler */
> +void *data;
> +/* Handler to be called whenever we get a poll event */
> +pcfdhandler handler;
> +/* A string to use in error messages */
> +char *idstr;
This was changed to const char in next patch. We can move it here?
The patch is a big one, there are quite a lot of TODOs and I still
think there can be some helper functions shared between fd handling
for 0-1 and 2-N but it looks good to me for merging as a first version.
After we have confirmed the definition of PostCopyFD please add:
Reviewed-by: Peter Xu
Thanks,
--
Peter Xu
Re: [Qemu-devel] [PATCH v3 11/29] vhost+postcopy: Transmit 'listen' to client
On Fri, Feb 16, 2018 at 01:16:07PM +, Dr. David Alan Gilbert (git) wrote:
[...]
> typedef struct VuVirtqElement {
> diff --git a/docs/interop/vhost-user.txt b/docs/interop/vhost-user.txt
> index 621543e654..bdec9ec0e8 100644
> --- a/docs/interop/vhost-user.txt
> +++ b/docs/interop/vhost-user.txt
> @@ -682,6 +682,12 @@ Master message types
>the slave must open a userfaultfd for later use.
>Note that at this stage the migration is still in precopy mode.
>
> + * VHOST_USER_POSTCOPY_LISTEN
> + Id: 27
> + Master payload: N/A
> +
> + Master advises slave that a transition to postcopy mode has happened.
Could we add something to explain why this listen needs to be
broadcasted to clients? Since I failed to find it out quickly
myself. :(
--
Peter Xu
Re: [Qemu-devel] [PATCH v9 00/14] ARM SMMUv3 Emulation Support
Hi Peter, On 27/02/18 20:02, Peter Maydell wrote: > On 17 February 2018 at 18:46, Eric Auger wrote: >> This series implements the emulation code for ARM SMMUv3. >> >> SMMUv3 gets instantiated by adding ",iommu=smmuv3" to the virt >> machine option. >> >> VHOST integration will be handled in a separate series. VFIO >> integration is not targeted at the moment. Only stage 1 and >> AArch64 PTW are supported. >> >> Main changes since v8: >> - fix mingw compilation (qemu/log.h) >> - put gpl v2 license on all files to respect initial license >> - change proto of smmu_ptw* to clarify inputs/outputs and >> prepare for iotlb emulation >> - fix hash table lookup >> - cleanup access type handling during ptw >> - cleanup reset infra (parent_reset) >> - replace some inline functions by macros >> - fix some CMD fields >> - increment cmdq cons only after cmd execution >> - replace some remaining error_report by qemu_log_mask > > What state is this series in now? Is it "seems ready to > go, needs review"? Are you hoping it might get in for 2.12, > or targeting 2.13 ? Yes I think it is in a decent state and I would be happy to get some new reviews, for a tentative pull in 2.12. In any case I will be reactive on any comment in that prospect. Thanks Eric > > thanks > -- PMM >
Re: [Qemu-devel] [PATCH v3 10/29] vhost+postcopy: Register shared ufd with postcopy
On Fri, Feb 16, 2018 at 01:16:06PM +, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert"
>
> Register the UFD that comes in as the response to the 'advise' method
> with the postcopy code.
>
> Signed-off-by: Dr. David Alan Gilbert
> ---
> hw/virtio/vhost-user.c | 21 -
> migration/postcopy-ram.h | 2 +-
> 2 files changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 4f59993baa..dd4eb50668 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -24,6 +24,7 @@
> #include
> #include
> #include
> +#include
Why this line?
>
> #define VHOST_MEMORY_MAX_NREGIONS8
> #define VHOST_USER_F_PROTOCOL_FEATURES 30
> @@ -155,6 +156,7 @@ struct vhost_user {
> CharBackend *chr;
> int slave_fd;
> NotifierWithReturn postcopy_notifier;
> +struct PostCopyFD postcopy_fd;
> };
>
> static bool ioeventfd_enabled(void)
> @@ -780,6 +782,17 @@ out:
> return ret;
> }
>
> +/*
> + * Called back from the postcopy fault thread when a fault is received on our
> + * ufd.
> + * TODO: This is Linux specific
> + */
> +static int vhost_user_postcopy_fault_handler(struct PostCopyFD *pcfd,
> + void *ufd)
> +{
> +return 0;
> +}
> +
> /*
> * Called at the start of an inbound postcopy on reception of the
> * 'advise' command.
> @@ -819,8 +832,14 @@ static int vhost_user_postcopy_advise(struct vhost_dev
> *dev, Error **errp)
> error_setg(errp, "%s: Failed to get ufd", __func__);
> return -1;
> }
> +fcntl(ufd, F_SETFL, O_NONBLOCK);
Only curious: would it work even without this line?
>
> -/* TODO: register ufd with userfault thread */
> +/* register ufd with userfault thread */
> +u->postcopy_fd.fd = ufd;
> +u->postcopy_fd.data = dev;
> +u->postcopy_fd.handler = vhost_user_postcopy_fault_handler;
> +u->postcopy_fd.idstr = "vhost-user"; /* Need to find unique name */
> +postcopy_register_shared_ufd(&u->postcopy_fd);
> return 0;
> }
>
> diff --git a/migration/postcopy-ram.h b/migration/postcopy-ram.h
> index 4bda5aa509..23efbdf346 100644
> --- a/migration/postcopy-ram.h
> +++ b/migration/postcopy-ram.h
> @@ -153,7 +153,7 @@ struct PostCopyFD {
> /* Handler to be called whenever we get a poll event */
> pcfdhandler handler;
> /* A string to use in error messages */
> -char *idstr;
> +const char *idstr;
Move to previous patch?
> };
>
> /* Register a userfaultfd owned by an external process for
> --
> 2.14.3
>
--
Peter Xu
Re: [Qemu-devel] [PATCH v3 12/29] postcopy+vhost-user: Split set_mem_table for postcopy
On Fri, Feb 16, 2018 at 01:16:08PM +, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert"
>
> Split the set_mem_table routines in both qemu and libvhost-user
> because the postcopy versions are going to be quite different
> once changes in the later patches are added.
>
> Signed-off-by: Dr. David Alan Gilbert
> ---
> contrib/libvhost-user/libvhost-user.c | 53
> hw/virtio/vhost-user.c| 77
> ++-
> 2 files changed, 128 insertions(+), 2 deletions(-)
>
> diff --git a/contrib/libvhost-user/libvhost-user.c
> b/contrib/libvhost-user/libvhost-user.c
> index beec7695a8..4922b2c722 100644
> --- a/contrib/libvhost-user/libvhost-user.c
> +++ b/contrib/libvhost-user/libvhost-user.c
> @@ -448,6 +448,55 @@ vu_reset_device_exec(VuDev *dev, VhostUserMsg *vmsg)
> return false;
> }
>
> +static bool
> +vu_set_mem_table_exec_postcopy(VuDev *dev, VhostUserMsg *vmsg)
> +{
> +int i;
> +VhostUserMemory *memory = &vmsg->payload.memory;
> +dev->nregions = memory->nregions;
> +/* TODO: Postcopy specific code */
> +DPRINT("Nregions: %d\n", memory->nregions);
> +for (i = 0; i < dev->nregions; i++) {
> +void *mmap_addr;
> +VhostUserMemoryRegion *msg_region = &memory->regions[i];
> +VuDevRegion *dev_region = &dev->regions[i];
> +
> +DPRINT("Region %d\n", i);
> +DPRINT("guest_phys_addr: 0x%016"PRIx64"\n",
> + msg_region->guest_phys_addr);
> +DPRINT("memory_size: 0x%016"PRIx64"\n",
> + msg_region->memory_size);
> +DPRINT("userspace_addr 0x%016"PRIx64"\n",
> + msg_region->userspace_addr);
> +DPRINT("mmap_offset 0x%016"PRIx64"\n",
> + msg_region->mmap_offset);
> +
> +dev_region->gpa = msg_region->guest_phys_addr;
> +dev_region->size = msg_region->memory_size;
> +dev_region->qva = msg_region->userspace_addr;
> +dev_region->mmap_offset = msg_region->mmap_offset;
> +
> +/* We don't use offset argument of mmap() since the
> + * mapped address has to be page aligned, and we use huge
> + * pages. */
> +mmap_addr = mmap(0, dev_region->size + dev_region->mmap_offset,
> + PROT_READ | PROT_WRITE, MAP_SHARED,
> + vmsg->fds[i], 0);
> +
> +if (mmap_addr == MAP_FAILED) {
> +vu_panic(dev, "region mmap error: %s", strerror(errno));
> +} else {
> +dev_region->mmap_addr = (uint64_t)(uintptr_t)mmap_addr;
> +DPRINT("mmap_addr: 0x%016"PRIx64"\n",
> + dev_region->mmap_addr);
> +}
> +
> +close(vmsg->fds[i]);
> +}
> +
> +return false;
> +}
> +
> static bool
> vu_set_mem_table_exec(VuDev *dev, VhostUserMsg *vmsg)
> {
> @@ -464,6 +513,10 @@ vu_set_mem_table_exec(VuDev *dev, VhostUserMsg *vmsg)
> }
> dev->nregions = memory->nregions;
>
> +if (dev->postcopy_listening) {
> +return vu_set_mem_table_exec_postcopy(dev, vmsg);
> +}
> +
> DPRINT("Nregions: %d\n", memory->nregions);
> for (i = 0; i < dev->nregions; i++) {
> void *mmap_addr;
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index ec6a4a82fd..64f4b3b3f9 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -325,15 +325,86 @@ static int vhost_user_set_log_base(struct vhost_dev
> *dev, uint64_t base,
> return 0;
> }
>
> +static int vhost_user_set_mem_table_postcopy(struct vhost_dev *dev,
> + struct vhost_memory *mem)
> +{
> +int fds[VHOST_MEMORY_MAX_NREGIONS];
> +int i, fd;
> +size_t fd_num = 0;
> +bool reply_supported = virtio_has_feature(dev->protocol_features,
> +
> VHOST_USER_PROTOCOL_F_REPLY_ACK);
> +/* TODO: Add actual postcopy differences */
> +VhostUserMsg msg = {
> +.hdr.request = VHOST_USER_SET_MEM_TABLE,
> +.hdr.flags = VHOST_USER_VERSION,
> +};
> +
> +if (reply_supported) {
> +msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
> +}
> +
> +for (i = 0; i < dev->mem->nregions; ++i) {
> +struct vhost_memory_region *reg = dev->mem->regions + i;
> +ram_addr_t offset;
> +MemoryRegion *mr;
> +
> +assert((uintptr_t)reg->userspace_addr == reg->userspace_addr);
> +mr = memory_region_from_host((void *)(uintptr_t)reg->userspace_addr,
> + &offset);
> +fd = memory_region_get_fd(mr);
> +if (fd > 0) {
> +msg.payload.memory.regions[fd_num].userspace_addr =
> +reg->userspace_addr;
> +msg.payload.memory.regions[fd_num].memory_size =
> reg->memory_size;
> +msg.payload.memory.regions[fd_num].guest_phys_addr =
> +reg->guest_phys_addr;
> +
Re: [Qemu-devel] [PATCH v3 13/29] migration/ram: ramblock_recv_bitmap_test_byte_offset
On Fri, Feb 16, 2018 at 01:16:09PM +, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert"
>
> Utility for testing the map when you already know the offset
> in the RAMBlock.
>
> Signed-off-by: Dr. David Alan Gilbert
Reviewed-by: Peter Xu
> ---
> migration/ram.c | 5 +
> migration/ram.h | 1 +
> 2 files changed, 6 insertions(+)
>
> diff --git a/migration/ram.c b/migration/ram.c
> index 8333d8e35e..8db5e80500 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -169,6 +169,11 @@ int ramblock_recv_bitmap_test(RAMBlock *rb, void
> *host_addr)
> rb->receivedmap);
> }
>
> +bool ramblock_recv_bitmap_test_byte_offset(RAMBlock *rb, uint64_t
> byte_offset)
> +{
> +return test_bit(byte_offset >> TARGET_PAGE_BITS, rb->receivedmap);
> +}
> +
> void ramblock_recv_bitmap_set(RAMBlock *rb, void *host_addr)
> {
> set_bit_atomic(ramblock_recv_bitmap_offset(host_addr, rb),
> rb->receivedmap);
> diff --git a/migration/ram.h b/migration/ram.h
> index f3a227b4fc..63a37c4683 100644
> --- a/migration/ram.h
> +++ b/migration/ram.h
> @@ -60,6 +60,7 @@ int ram_postcopy_incoming_init(MigrationIncomingState *mis);
> void ram_handle_compressed(void *host, uint8_t ch, uint64_t size);
>
> int ramblock_recv_bitmap_test(RAMBlock *rb, void *host_addr);
> +bool ramblock_recv_bitmap_test_byte_offset(RAMBlock *rb, uint64_t
> byte_offset);
> void ramblock_recv_bitmap_set(RAMBlock *rb, void *host_addr);
> void ramblock_recv_bitmap_set_range(RAMBlock *rb, void *host_addr, size_t
> nr);
>
> --
> 2.14.3
>
--
Peter Xu
Re: [Qemu-devel] [PATCH 03/14] qio: introduce qio_channel_add_watch_full()
On Wed, Feb 28, 2018 at 01:06:22PM +0800, Peter Xu wrote:
> It's a more powerful version of qio_channel_add_watch(), which supports
> non-default gcontext. It's stripped from the old one, then we have
> g_source_get_id() to fetch the tag ID to keep the old interface.
>
> Note that the new API will return a gsource, meanwhile keep a reference
> of it so that callers need to unref them explicitly.
I don't really like this. Having qio_channel_add_watch and
qio_channel_add_watch_full with differing return values is
really very surprising. They should be functionally identical,
except for the extra context arg.
>
> Signed-off-by: Peter Xu
> ---
> include/io/channel.h | 31 ---
> io/channel.c | 24 +++-
> 2 files changed, 47 insertions(+), 8 deletions(-)
>
> diff --git a/include/io/channel.h b/include/io/channel.h
> index 3995e243a3..36af5e58ae 100644
> --- a/include/io/channel.h
> +++ b/include/io/channel.h
> @@ -620,20 +620,45 @@ GSource *qio_channel_create_watch(QIOChannel *ioc,
>GIOCondition condition);
>
> /**
> - * qio_channel_add_watch:
> + * qio_channel_add_watch_full:
> * @ioc: the channel object
> * @condition: the I/O condition to monitor
> * @func: callback to invoke when the source becomes ready
> * @user_data: opaque data to pass to @func
> * @notify: callback to free @user_data
> + * @context: gcontext to bind the source to
> *
> - * Create a new main loop source that is used to watch
> + * Create a new source that is used to watch
> * for the I/O condition @condition. The callback @func
> * will be registered against the source, to be invoked
> * when the source becomes ready. The optional @user_data
> * will be passed to @func when it is invoked. The @notify
> * callback will be used to free @user_data when the
> - * watch is deleted
> + * watch is deleted. The source will be bound to @context if
> + * provided, or main context if it is NULL.
> + *
> + * Note: if a valid source is returned, we need to explicitly unref
> + * the source to destroy it.
> + *
> + * Returns: the source pointer
> + */
> +GSource *qio_channel_add_watch_full(QIOChannel *ioc,
> +GIOCondition condition,
> +QIOChannelFunc func,
> +gpointer user_data,
> +GDestroyNotify notify,
> +GMainContext *context);
> +
> +/**
> + * qio_channel_add_watch:
> + * @ioc: the channel object
> + * @condition: the I/O condition to monitor
> + * @func: callback to invoke when the source becomes ready
> + * @user_data: opaque data to pass to @func
> + * @notify: callback to free @user_data
> + *
> + * Wrapper of qio_channel_add_watch_full(), but it'll only bind the
> + * source object to default main context.
> *
> * The returned source ID can be used with g_source_remove()
> * to remove and free the source when no longer required.
> diff --git a/io/channel.c b/io/channel.c
> index ec4b86de7c..3e734cc9a5 100644
> --- a/io/channel.c
> +++ b/io/channel.c
> @@ -299,6 +299,22 @@ void qio_channel_set_aio_fd_handler(QIOChannel *ioc,
> klass->io_set_aio_fd_handler(ioc, ctx, io_read, io_write, opaque);
> }
>
> +GSource *qio_channel_add_watch_full(QIOChannel *ioc,
> +GIOCondition condition,
> +QIOChannelFunc func,
> +gpointer user_data,
> +GDestroyNotify notify,
> +GMainContext *context)
> +{
> +GSource *source;
> +
> +source = qio_channel_create_watch(ioc, condition);
> +g_source_set_callback(source, (GSourceFunc)func, user_data, notify);
> +g_source_attach(source, context);
> +
> +return source;
> +}
> +
> guint qio_channel_add_watch(QIOChannel *ioc,
> GIOCondition condition,
> QIOChannelFunc func,
> @@ -308,11 +324,9 @@ guint qio_channel_add_watch(QIOChannel *ioc,
> GSource *source;
> guint id;
>
> -source = qio_channel_create_watch(ioc, condition);
> -
> -g_source_set_callback(source, (GSourceFunc)func, user_data, notify);
> -
> -id = g_source_attach(source, NULL);
> +source = qio_channel_add_watch_full(ioc, condition, func,
> +user_data, notify, NULL);
> +id = g_source_get_id(source);
> g_source_unref(source);
>
> return id;
> --
> 2.14.3
>
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH 04/14] migration: let incoming side use thread context
On Wed, Feb 28, 2018 at 01:06:23PM +0800, Peter Xu wrote:
> The old incoming migration is running in main thread and default
> gcontext. With the new qio_channel_add_watch_full() we can now let it
> run in the thread's own gcontext (if there is one).
>
> Currently this patch does nothing alone. But when any of the incoming
> migration is run in another iothread (e.g., the upcoming migrate-recover
> command), this patch will bind the incoming logic to the iothread
> instead of the main thread (which may already get page faulted and
> hanged).
>
> RDMA is not considered for now since it's not even using the QIO APIs at
> all.
Errm, yes, it is.
struct QIOChannelRDMA {
QIOChannel parent;
RDMAContext *rdma;
QEMUFile *file;
size_t len;
bool blocking; /* XXX we don't actually honour this yet */
};
>
> CC: Juan Quintela
> CC: Dr. David Alan Gilbert
> CC: Laurent Vivier
> Signed-off-by: Peter Xu
> ---
> migration/exec.c | 11 ++-
> migration/fd.c | 11 ++-
> migration/socket.c | 12 +++-
> 3 files changed, 19 insertions(+), 15 deletions(-)
>
> diff --git a/migration/exec.c b/migration/exec.c
> index 0bc5a427dd..f401fc005e 100644
> --- a/migration/exec.c
> +++ b/migration/exec.c
> @@ -55,6 +55,7 @@ void exec_start_incoming_migration(const char *command,
> Error **errp)
> {
> QIOChannel *ioc;
> const char *argv[] = { "/bin/sh", "-c", command, NULL };
> +GSource *source;
>
> trace_migration_exec_incoming(command);
> ioc = QIO_CHANNEL(qio_channel_command_new_spawn(argv,
> @@ -65,9 +66,9 @@ void exec_start_incoming_migration(const char *command,
> Error **errp)
> }
>
> qio_channel_set_name(ioc, "migration-exec-incoming");
> -qio_channel_add_watch(ioc,
> - G_IO_IN,
> - exec_accept_incoming_migration,
> - NULL,
> - NULL);
> +source = qio_channel_add_watch_full(ioc, G_IO_IN,
> +exec_accept_incoming_migration,
> +NULL, NULL,
> +g_main_context_get_thread_default());
> +g_source_unref(source);
> }
> diff --git a/migration/fd.c b/migration/fd.c
> index cd06182d1e..9c593eb3ff 100644
> --- a/migration/fd.c
> +++ b/migration/fd.c
> @@ -55,6 +55,7 @@ void fd_start_incoming_migration(const char *infd, Error
> **errp)
> {
> QIOChannel *ioc;
> int fd;
> +GSource *source;
>
> fd = strtol(infd, NULL, 0);
> trace_migration_fd_incoming(fd);
> @@ -66,9 +67,9 @@ void fd_start_incoming_migration(const char *infd, Error
> **errp)
> }
>
> qio_channel_set_name(QIO_CHANNEL(ioc), "migration-fd-incoming");
> -qio_channel_add_watch(ioc,
> - G_IO_IN,
> - fd_accept_incoming_migration,
> - NULL,
> - NULL);
> +source = qio_channel_add_watch_full(ioc, G_IO_IN,
> +fd_accept_incoming_migration,
> +NULL, NULL,
> +g_main_context_get_thread_default());
> +g_source_unref(source);
> }
> diff --git a/migration/socket.c b/migration/socket.c
> index e090097077..82c330083c 100644
> --- a/migration/socket.c
> +++ b/migration/socket.c
> @@ -164,6 +164,7 @@ static void socket_start_incoming_migration(SocketAddress
> *saddr,
> Error **errp)
> {
> QIOChannelSocket *listen_ioc = qio_channel_socket_new();
> +GSource *source;
>
> qio_channel_set_name(QIO_CHANNEL(listen_ioc),
> "migration-socket-listener");
> @@ -173,11 +174,12 @@ static void
> socket_start_incoming_migration(SocketAddress *saddr,
> return;
> }
>
> -qio_channel_add_watch(QIO_CHANNEL(listen_ioc),
> - G_IO_IN,
> - socket_accept_incoming_migration,
> - listen_ioc,
> - (GDestroyNotify)object_unref);
> +source = qio_channel_add_watch_full(QIO_CHANNEL(listen_ioc), G_IO_IN,
> +socket_accept_incoming_migration,
> +listen_ioc,
> +(GDestroyNotify)object_unref,
> +g_main_context_get_thread_default());
> +g_source_unref(source);
> }
>
> void tcp_start_incoming_migration(const char *host_port, Error **errp)
> --
> 2.14.3
>
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH 10/14] qio: refcount QIOTask
On Wed, Feb 28, 2018 at 01:06:29PM +0800, Peter Xu wrote:
> It will be used in multiple threads in follow-up patches. Let it start
> to have refcounts.
>
> Signed-off-by: Peter Xu
> ---
> include/io/task.h | 3 +++
> io/task.c | 23 ++-
> 2 files changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/include/io/task.h b/include/io/task.h
> index 9dbe3758d7..c6acd6489c 100644
> --- a/include/io/task.h
> +++ b/include/io/task.h
> @@ -322,4 +322,7 @@ gpointer qio_task_get_result_pointer(QIOTask *task);
> */
> Object *qio_task_get_source(QIOTask *task);
>
> +void qio_task_ref(QIOTask *task);
> +void qio_task_unref(QIOTask *task);
It should just be turned back into a QObject as it was originally,
so we get refcounting for free.
> diff --git a/io/task.c b/io/task.c
> index 204c0be286..00d3a5096a 100644
> --- a/io/task.c
> +++ b/io/task.c
> @@ -32,6 +32,7 @@ struct QIOTask {
> Error *err;
> gpointer result;
> GDestroyNotify destroyResult;
> +uint32_t refcount;
>
> /* Threaded QIO task specific fields */
> GSource *idle_source; /* The idle task to run complete routine */
> @@ -57,6 +58,8 @@ QIOTask *qio_task_new(Object *source,
>
> trace_qio_task_new(task, source, func, opaque);
>
> +qio_task_ref(task);
> +
> return task;
> }
>
> @@ -165,7 +168,7 @@ void qio_task_complete(QIOTask *task)
> {
> task->func(task, task->opaque);
> trace_qio_task_complete(task);
> -qio_task_free(task);
> +qio_task_unref(task);
> }
>
>
> @@ -208,3 +211,21 @@ Object *qio_task_get_source(QIOTask *task)
> {
> return task->source;
> }
> +
> +void qio_task_ref(QIOTask *task)
> +{
> +if (!task) {
> +return;
> +}
> +atomic_inc(&task->refcount);
> +}
> +
> +void qio_task_unref(QIOTask *task)
> +{
> +if (!task) {
> +return;
> +}
> +if (atomic_fetch_dec(&task->refcount) == 1) {
> +qio_task_free(task);
> +}
> +}
> --
> 2.14.3
>
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH 11/14] qio/chardev: return QIOTask when connect async
On Wed, Feb 28, 2018 at 01:06:30PM +0800, Peter Xu wrote: > Let qio_channel_socket_connect_async() return the created QIOTask object > for the async connection. In tcp chardev, cache that in SocketChardev > for further use. With the QIOTask refcount, this is pretty safe. Why do you want to return QIOTask ? This is going against the intended design pattern for QIOTask (that was based on that in GLib). The task supposed to be an internal use only helper that callers should never be touching until the completion callback is invoked. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH 13/14] qio: allow threaded qiotask to switch contexts
On Wed, Feb 28, 2018 at 01:06:32PM +0800, Peter Xu wrote: > This is the part of work to allow the QIOTask to use a different > gcontext rather than the default main gcontext, by providing > qio_task_context_set() API. > > We have done some work before on doing similar things to add non-default > gcontext support. The general idea is that we delete the old GSource > from the main context, then re-add a new one to the new context when > context changed to a non-default one. However this trick won't work > easily for threaded QIOTasks since we can't easily stop a real thread > and re-setup the whole thing from the very beginning. I think this entire usage pattern is really broken. We should not provide a way to change the GMainContext on an existing task. We should always just set the correct GMainContxt right from the start. This will avoid much of the complexity you're introducing into this patch series, and avoid having to expose GTasks to the callers of the async methods at all. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH 07/14] qio/chardev: update net listener gcontext
On Wed, Feb 28, 2018 at 01:06:26PM +0800, Peter Xu wrote:
> TCP chardevs can be using QIO network listeners working in the
> background when in listening mode. However the network listeners are
> always running in main context. This can race with chardevs that are
> running in non-main contexts.
>
> To solve this: firstly introduce qio_net_listener_set_context() to allow
> caller to set gcontext for network listeners. Then call it in
> tcp_chr_update_read_handler(), with the newly cached gcontext.
>
> It's fairly straightforward after we have introduced some net listener
> helper functions - basically we unregister the GSources and add them
> back with the correct context.
>
> Signed-off-by: Peter Xu
> ---
> chardev/char-socket.c | 9 +
> include/io/net-listener.h | 12
> io/net-listener.c | 7 +++
> 3 files changed, 28 insertions(+)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 43a2cc2c1c..8f0935cd15 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -559,6 +559,15 @@ static void tcp_chr_update_read_handler(Chardev *chr)
> {
> SocketChardev *s = SOCKET_CHARDEV(chr);
>
> +if (s->listener) {
> +/*
> + * It's possible that chardev context is changed in
> + * qemu_chr_be_update_read_handlers(). Reset it for QIO net
> + * listener if there is.
> + */
> +qio_net_listener_set_context(s->listener, chr->gcontext);
> +}
> +
> if (!s->connected) {
> return;
> }
> diff --git a/include/io/net-listener.h b/include/io/net-listener.h
> index 566be283b3..39dede9d6f 100644
> --- a/include/io/net-listener.h
> +++ b/include/io/net-listener.h
> @@ -106,6 +106,18 @@ int qio_net_listener_open_sync(QIONetListener *listener,
> SocketAddress *addr,
> Error **errp);
>
> +/**
> + * qio_net_listener_set_context:
> + * @listener: the net listener object
> + * @context: the context that we'd like to bind the sources to
> + *
> + * This helper does not do anything but moves existing net listener
> + * sources from the old one to the new one. It can be seen as a
> + * no-operation if there is no listening source at all.
> + */
> +void qio_net_listener_set_context(QIONetListener *listener,
> + GMainContext *context);
I don't think this is a good design. The GMainContext should be provided
to the qio_net_listener_set_client_func method immediately, not updated
after the fact
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH 01/14] chardev: fix leak in tcp_chr_telnet_init_io()
On Wed, Feb 28, 2018 at 01:06:20PM +0800, Peter Xu wrote: > Need to free TCPChardevTelnetInit when session established. > > Since at it, switch to use G_SOURCE_* macros. > > Signed-off-by: Peter Xu > --- > chardev/char-socket.c | 10 +++--- > 1 file changed, 7 insertions(+), 3 deletions(-) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH 02/14] qio: rename qio_task_thread_result
On Wed, Feb 28, 2018 at 01:06:21PM +0800, Peter Xu wrote:
> It is strange that it was called gio_task_thread_result. Rename it to
> follow the naming rule of the file.
>
> Signed-off-by: Peter Xu
> ---
> io/task.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Daniel P. Berrangé
>
> diff --git a/io/task.c b/io/task.c
> index 3ce556017c..1a0a1c7185 100644
> --- a/io/task.c
> +++ b/io/task.c
> @@ -80,7 +80,7 @@ struct QIOTaskThreadData {
> };
>
>
> -static gboolean gio_task_thread_result(gpointer opaque)
> +static gboolean qio_task_thread_result(gpointer opaque)
> {
> struct QIOTaskThreadData *data = opaque;
>
> @@ -110,7 +110,7 @@ static gpointer qio_task_thread_worker(gpointer opaque)
> * the worker results
> */
> trace_qio_task_thread_exit(data->task);
> -g_idle_add(gio_task_thread_result, data);
> +g_idle_add(qio_task_thread_result, data);
> return NULL;
> }
>
> --
> 2.14.3
>
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
[Qemu-devel] [PATCH] hw/i386: make IOMMUs configurable via default-configs/
Allow distributions to disable the Intel and/or AMD IOMMU devices. Signed-off-by: Paolo Bonzini --- default-configs/i386-softmmu.mak | 2 ++ default-configs/x86_64-softmmu.mak | 2 ++ hw/i386/Makefile.objs | 5 +++-- 3 files changed, 7 insertions(+), 2 deletions(-) diff --git a/default-configs/i386-softmmu.mak b/default-configs/i386-softmmu.mak index 3326e3e0bb..9e5a29fa4a 100644 --- a/default-configs/i386-softmmu.mak +++ b/default-configs/i386-softmmu.mak @@ -63,3 +63,5 @@ CONFIG_PXB=y CONFIG_ACPI_VMGENID=y CONFIG_FW_CFG_DMA=y CONFIG_I2C=y +CONFIG_VTD=y +CONFIG_AMD_IOMMU=y diff --git a/default-configs/x86_64-softmmu.mak b/default-configs/x86_64-softmmu.mak index 1c6cda1d9a..7baf91b921 100644 --- a/default-configs/x86_64-softmmu.mak +++ b/default-configs/x86_64-softmmu.mak @@ -63,3 +63,5 @@ CONFIG_PXB=y CONFIG_ACPI_VMGENID=y CONFIG_FW_CFG_DMA=y CONFIG_I2C=y +CONFIG_VTD=y +CONFIG_AMD_IOMMU=y diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs index fd279e7584..528b8dc431 100644 --- a/hw/i386/Makefile.objs +++ b/hw/i386/Makefile.objs @@ -2,8 +2,9 @@ obj-$(CONFIG_KVM) += kvm/ obj-y += multiboot.o obj-y += pc.o pc_piix.o pc_q35.o obj-y += pc_sysfw.o -obj-y += x86-iommu.o intel_iommu.o -obj-y += amd_iommu.o +obj-y += x86-iommu.o +obj-$(CONFIG_VTD) += x86-iommu.o intel_iommu.o +obj-$(CONFIG_AMD_IOMMU) += amd_iommu.o obj-$(CONFIG_XEN) += ../xenpv/ xen/ obj-$(CONFIG_VMPORT) += vmport.o obj-$(CONFIG_VMMOUSE) += vmmouse.o -- 2.14.3
Re: [Qemu-devel] [RFC QEMU PATCH v4 00/10] Implement vNVDIMM for Xen HVM guest
On 02/27/18 17:22 +, Anthony PERARD wrote: > On Thu, Dec 07, 2017 at 06:18:02PM +0800, Haozhong Zhang wrote: > > This is the QEMU part patches that works with the associated Xen > > patches to enable vNVDIMM support for Xen HVM domains. Xen relies on > > QEMU to build guest NFIT and NVDIMM namespace devices, and allocate > > guest address space for vNVDIMM devices. > > I've got other question, and maybe possible improvements. > > When QEMU build the ACPI tables, it also initialize some MemoryRegion, > which use more guest memory. Do you know if those regions are used with > your patch series on Xen? Yes, that's why dm_acpi_size is introduced. > Otherwise, we could try to avoid their > creation with this: > In xenfv_machine_options() > m->rom_file_has_mr = false; > (setting this in xen_hvm_init() would probably be better, but I havn't > try) If my memory is correct, simply setting rom_file_has_mr to false does not work (though I cannot remind the exact reason). I'll have a look as the code to refresh my memory. Haozhong > > If this is possible, libxl would not need to allocate more memory for > the guest (dm_acpi_size). > > -- > Anthony PERARD
Re: [Qemu-devel] [Qemu-block] [PATCH] vl: introduce vm_shutdown()
On Wed, Feb 28, 2018 at 09:58:13AM +0800, Fam Zheng wrote:
> On Tue, 02/27 15:30, Stefan Hajnoczi wrote:
> > On Fri, Feb 23, 2018 at 04:20:44PM +0800, Fam Zheng wrote:
> > > On Tue, 02/20 13:10, Stefan Hajnoczi wrote:
> > > > 1. virtio_scsi_handle_cmd_vq() racing with iothread_stop_all() hits the
> > > >virtio_scsi_ctx_check() assertion failure because the BDS AioContext
> > > >has been modified by iothread_stop_all().
> > >
> > > Does this patch fix the issue completely? IIUC virtio_scsi_handle_cmd can
> > > already be entered at the time of main thread calling
> > > virtio_scsi_clear_aio(),
> > > so this race condition still exists:
> > >
> > > main thread iothread
> > > -
> > > vm_shutdown
> > > ...
> > > virtio_bus_stop_ioeventfd
> > > virtio_scsi_dataplane_stop
> > > aio_poll()
> > > ...
> > >
> > > virtio_scsi_data_plane_handle_cmd()
> > > aio_context_acquire(s->ctx)
> > > virtio_scsi_acquire(s).enter
> > > virtio_scsi_clear_aio()
> > > aio_context_release(s->ctx)
> > >
> > > virtio_scsi_acquire(s).return
> > > virtio_scsi_handle_cmd_vq()
> > > ...
> > > virtqueue_pop()
> > >
> > > Is it possible that the above virtqueue_pop() still returns one element
> > > that was
> > > queued before vm_shutdown() was called?
> >
> > No, it can't because virtio_scsi_clear_aio() invokes
> > virtio_queue_host_notifier_aio_read(&vq->host_notifier) to process the
> > virtqueue. By the time we get back to iothread's
> > virtio_scsi_data_plane_handle_cmd() the virtqueue is already empty.
> >
> > Vcpus have been paused so no additional elements can slip into the
> > virtqueue.
>
> So there is:
>
> static void virtio_queue_host_notifier_aio_read(EventNotifier *n)
> {
> VirtQueue *vq = container_of(n, VirtQueue, host_notifier);
> if (event_notifier_test_and_clear(n)) {
> virtio_queue_notify_aio_vq(vq);
> }
> }
>
> Guest kicks after adding an element to VQ, but we check ioeventfd before
> trying
> virtqueue_pop(). Is that a problem? If VCPUs are paused after enqueuing but
> before kicking VQ, the ioeventfd is not set, the virtqueue is not processed
> here.
You are right.
This race condition also affects the existing 'stop' command, where
ioeventfd is disabled in the same way. I'll send a v2 with a patch to
fix this.
Stefan
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH v2 RFC] scripts/checkpatch.pl: add check for `while` and `for`
On Tue, Feb 27, 2018 at 05:42:50PM +, Peter Maydell wrote:
> On 27 February 2018 at 17:32, Stefan Hajnoczi wrote:
> > On Mon, Feb 26, 2018 at 10:53:18AM +0800, Su Hang wrote:
> >> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> >> index 1b4b812e28fa..10c138344fa9 100755
> >> --- a/scripts/checkpatch.pl
> >> +++ b/scripts/checkpatch.pl
> >> @@ -2353,7 +2353,8 @@ sub process {
> >> }
> >>
> >> # check for missing bracing round if etc
> >
> > Please fix the pre-existing typo in the comment:
> >
> > s/round/around/
>
> That's not really a typo IMHO -- both 'round' and 'around'
> seem fine to me here. (The OED entry for 'around' notes
> "There is variation with 'round' adv. and prep. in many of the
> senses below; in general 'around' is less usual in British usage",
> so some of this is US-vs-UK usage or personal idiolect.)
Thanks! I hear British people use it frequently but thought it was a
colloquialism that isn't used in written form, innit? :) I stand
corrected.
Stefan
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH v3 17/29] vhost+postcopy: Send requests to source for shared pages
On Fri, Feb 16, 2018 at 01:16:13PM +, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert"
>
> Send requests back to the source for shared page requests.
>
> Signed-off-by: Dr. David Alan Gilbert
> ---
> migration/migration.h| 2 ++
> migration/postcopy-ram.c | 31 ---
> migration/postcopy-ram.h | 3 +++
> migration/trace-events | 2 ++
> 4 files changed, 35 insertions(+), 3 deletions(-)
>
> diff --git a/migration/migration.h b/migration/migration.h
> index d158e62cf2..457bf37ec2 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -46,6 +46,8 @@ struct MigrationIncomingState {
> int userfault_quit_fd;
> QEMUFile *to_src_file;
> QemuMutex rp_mutex;/* We send replies from multiple threads */
> +/* RAMBlock of last request sent to source */
> +RAMBlock *last_rb;
> void *postcopy_tmp_page;
> void *postcopy_tmp_zero_page;
> /* PostCopyFD's for external userfaultfds & handlers of shared memory */
> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> index d118b78bf5..277ff749a0 100644
> --- a/migration/postcopy-ram.c
> +++ b/migration/postcopy-ram.c
> @@ -534,6 +534,31 @@ static int ram_block_enable_notify(const char
> *block_name, void *host_addr,
> return 0;
> }
>
> +/*
> + * Callback from shared fault handlers to ask for a page,
> + * the page must be specified by a RAMBlock and an offset in that rb
> + */
> +int postcopy_request_shared_page(struct PostCopyFD *pcfd, RAMBlock *rb,
> + uint64_t client_addr, uint64_t rb_offset)
> +{
> +size_t pagesize = qemu_ram_pagesize(rb);
> +uint64_t aligned_rbo = rb_offset & ~(pagesize - 1);
> +MigrationIncomingState *mis = migration_incoming_get_current();
> +
> +trace_postcopy_request_shared_page(pcfd->idstr, qemu_ram_get_idstr(rb),
> + rb_offset);
> +/* TODO: Check bitmap to see if we already have the page */
> +if (rb != mis->last_rb) {
> +mis->last_rb = rb;
> +migrate_send_rp_req_pages(mis, qemu_ram_get_idstr(rb),
> + aligned_rbo, pagesize);
> +} else {
> +/* Save some space */
> +migrate_send_rp_req_pages(mis, NULL, aligned_rbo, pagesize);
> +}
> +return 0;
> +}
> +
So IIUC this can only be called within the page fault thread or there
can be race. Is there a way to guarantee this? Or do we need a
comment for that?
> /*
> * Handle faults detected by the USERFAULT markings
> */
> @@ -544,9 +569,9 @@ static void *postcopy_ram_fault_thread(void *opaque)
> int ret;
> size_t index;
> RAMBlock *rb = NULL;
> -RAMBlock *last_rb = NULL; /* last RAMBlock we sent part of */
>
> trace_postcopy_ram_fault_thread_entry();
> +mis->last_rb = NULL; /* last RAMBlock we sent part of */
> qemu_sem_post(&mis->fault_thread_sem);
>
> struct pollfd *pfd;
> @@ -634,8 +659,8 @@ static void *postcopy_ram_fault_thread(void *opaque)
> * Send the request to the source - we want to request one
> * of our host page sizes (which is >= TPS)
> */
> -if (rb != last_rb) {
> -last_rb = rb;
> +if (rb != mis->last_rb) {
> +mis->last_rb = rb;
> migrate_send_rp_req_pages(mis, qemu_ram_get_idstr(rb),
> rb_offset, qemu_ram_pagesize(rb));
> } else {
> diff --git a/migration/postcopy-ram.h b/migration/postcopy-ram.h
> index dbc2ee1f2b..4c63f20df4 100644
> --- a/migration/postcopy-ram.h
> +++ b/migration/postcopy-ram.h
> @@ -162,5 +162,8 @@ struct PostCopyFD {
> */
> void postcopy_register_shared_ufd(struct PostCopyFD *pcfd);
> void postcopy_unregister_shared_ufd(struct PostCopyFD *pcfd);
> +/* Callback from shared fault handlers to ask for a page */
> +int postcopy_request_shared_page(struct PostCopyFD *pcfd, RAMBlock *rb,
> + uint64_t client_addr, uint64_t offset);
>
> #endif
> diff --git a/migration/trace-events b/migration/trace-events
> index 1e617ad7a6..7c910b5479 100644
> --- a/migration/trace-events
> +++ b/migration/trace-events
> @@ -198,6 +198,8 @@ postcopy_ram_incoming_cleanup_closeuf(void) ""
> postcopy_ram_incoming_cleanup_entry(void) ""
> postcopy_ram_incoming_cleanup_exit(void) ""
> postcopy_ram_incoming_cleanup_join(void) ""
> +postcopy_request_shared_page(const char *sharer, const char *rb, uint64_t
> rb_offset) "for %s in %s offset 0x%"PRIx64
> +
> save_xbzrle_page_skipping(void) ""
> save_xbzrle_page_overflow(void) ""
> ram_save_iterate_big_wait(uint64_t milliconds, int iterations) "big wait: %"
> PRIu64 " milliseconds, %d iterations"
> --
> 2.14.3
>
--
Peter Xu
Re: [Qemu-devel] [PATCH v4 2/5] qcow2: Document some maximum size constraints
On Tue 27 Feb 2018 05:29:41 PM CET, Eric Blake wrote: > +The refcount table has implications on the maximum host file size; a > +larger cluster size is required for the refcount table to cover > larger +offsets. Why is this? Because of the refcount_table_clusters field ? I think the maximum offset allowed by that is ridiculously high, exceeding any other limit imposed by the L1/L2 tables. If my numbers are right, with the default values that's 64 ZB. In addition to that, the size that can be covered by the refcount table also depends on the size of refcount entries (refcount_order). With 512 byte clusters and 64 bit refcount entries I still get 8 PB, way over what's limited by the L1/L2 tables (128 GB). Berto
Re: [Qemu-devel] [PATCH v2 5/5] s390x/cpumodel: Set up CPU model for AP device support
> KVM detects whether the AP instructions are installed on the host. If
> the instructions are installed, the feature is allowed (enabled) and
> can be turned on by userspace (QEMU).
As mentioned in the KVM thread, I'd like to verify if there is not a AP
interpretation facility.
>>
>> IOW: is this really a CPU model feature?
> I believe it is a necessary feature and came about due to review comments
> from Christian and Connie in v1.
Yes, I can see how this is guest ABI. But we always have to take care of
ever potentially wanting to emulate this in QEMU. But if we can turn
interpretation on/off using the feature flag, I am happy.
>>
>>>
>>> FEAT_INIT("plo-cl", S390_FEAT_TYPE_PLO, 0, "PLO Compare and load (32
>>> bit in general registers)"),
>>> FEAT_INIT("plo-clg", S390_FEAT_TYPE_PLO, 1, "PLO Compare and load (64
>>> bit in parameter list)"),
>>> diff --git a/target/s390x/cpu_features_def.h
>>> b/target/s390x/cpu_features_def.h
>>> index 7c5915c..8998b65 100644
>>> --- a/target/s390x/cpu_features_def.h
>>> +++ b/target/s390x/cpu_features_def.h
>>> @@ -27,8 +27,10 @@ typedef enum {
>>> S390_FEAT_SENSE_RUNNING_STATUS,
>>> S390_FEAT_CONDITIONAL_SSKE,
>>> S390_FEAT_CONFIGURATION_TOPOLOGY,
>>> +S390_FEAT_AP_QUERY_CONFIG_INFO,
>>> S390_FEAT_IPTE_RANGE,
>>> S390_FEAT_NONQ_KEY_SETTING,
>>> +S390_FEAT_AP_FACILITIES_TEST,
>>> S390_FEAT_EXTENDED_TRANSLATION_2,
>>> S390_FEAT_MSA,
>>> S390_FEAT_LONG_DISPLACEMENT,
>>> @@ -118,6 +120,7 @@ typedef enum {
>>> /* Misc */
>>> S390_FEAT_DAT_ENH_2,
>>> S390_FEAT_CMM,
>>> +S390_FEAT_AP,
>>>
>>> /* PLO */
>>> S390_FEAT_PLO_CL,
>>> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
>>> index 1d5f0da..35f91ea 100644
>>> --- a/target/s390x/cpu_models.c
>>> +++ b/target/s390x/cpu_models.c
>>> @@ -770,6 +770,8 @@ static void check_consistency(const S390CPUModel *model)
>>> { S390_FEAT_PRNO_TRNG_QRTCR, S390_FEAT_MSA_EXT_5 },
>>> { S390_FEAT_PRNO_TRNG, S390_FEAT_MSA_EXT_5 },
>>> { S390_FEAT_SIE_KSS, S390_FEAT_SIE_F2 },
>>> +{ S390_FEAT_AP_QUERY_CONFIG_INFO, S390_FEAT_AP },
>>> +{ S390_FEAT_AP_FACILITIES_TEST, S390_FEAT_AP },
Saw this way too late :)
>>
>>>
>>> static uint16_t full_GEN12_GA2[] = {
>>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>>> index e13c890..ae20ed8 100644
>>> --- a/target/s390x/kvm.c
>>> +++ b/target/s390x/kvm.c
>>> @@ -2105,6 +2105,7 @@ static int kvm_to_feat[][2] = {
>>> { KVM_S390_VM_CPU_FEAT_PFMFI, S390_FEAT_SIE_PFMFI},
>>> { KVM_S390_VM_CPU_FEAT_SIGPIF, S390_FEAT_SIE_SIGPIF},
>>> { KVM_S390_VM_CPU_FEAT_KSS, S390_FEAT_SIE_KSS},
>>> +{ KVM_S390_VM_CPU_FEAT_AP, S390_FEAT_AP},
>> Nothing speaks against the STFL bits, want to learn more about the
>> S390_FEAT_AP feature :)
> There are a couple of primary reasons for the addition of this feature.
>
> * Let's start with the fact that AP instructions absolutely must be
> installed on the host in order to virtualize
>AP devices for a guest using this patch series. There is a bit in the
> in the SIE block (ECA.28) that controls
>whether AP instructions executed on the guest are interpreted by SIE
> or intercepted by KVM. This patch series sets
>this bit so that AP instructions executed on the guest are
> interpreted by the firmware passed directly through
>to the AP devices configured for the guest in the CRYCB- a satellite
> control block of the SIE block to configure
>the AP facilities for the guest. If the AP instructions are not
> installed, the AP bus running in the guest will
>not initialize and the guest will not have access to any AP devices.
> So, the primary reason for the S390_FEAT_AP
>feature is to protect against this scenario.
Then I request the following change in KVM:
If KVM_S390_VM_CPU_FEAT_AP is enabled in KVM, ECA.28 is _always_ set
(not just if an AP device is configured). This especially makes things a
lot easier when it comes to handling hotplugged CPUs and avoiding race
conditions when enabling these bits as mentioned in the KVM series.
KVM_S390_VM_CPU_FEAT_AP == AP instructions available for the guest
(don't throw an operation exception).
So this feature then really is guest ABI. The instructions are
available. If there is no device configured, bad luck.
>
> * In the review of v1, Christian suggested creating a feature to prevent
> migration of a guest with AP devices
>to a system without AP support, or a system without AP instructions.
> In order to migrate to another system,
>the S390_FEAT_AP feature must be available on the target system.
I wonder if it would make sense to split this even further up.
E.g. to be able to differentiate between format0 and format2 crycb
format support. (which is necessary to properly migrate guests with
vSIE) But these would then be SIE specific CPU features in addition most
properly. But also depen
Re: [Qemu-devel] [PATCH v2 5/5] s390x/cpumodel: Set up CPU model for AP device support
> The ap_bus has a function for determining if the ap instructions are
> installed. I think it's basically trial execution.
>
Okay, just like CMM. Bad system design. But it is what it is.
>>
>
> I think it's best modeled with a CPU model feature. In the end
> it's about having or not having ap instructions in the guest, and
> making stable guest ABI is exactly the thing of cpu-models AFAIU.
Indeed, as mentioned in the other mail, the AP feature but then always
has to say "AP instructions available", not just if an AP device has
been defined.
>
>>>
>>> FEAT_INIT("plo-cl", S390_FEAT_TYPE_PLO, 0, "PLO Compare and load (32
>>> bit in general registers)"),
>>> FEAT_INIT("plo-clg", S390_FEAT_TYPE_PLO, 1, "PLO Compare and load (64
>>> bit in parameter list)"),
>>> diff --git a/target/s390x/cpu_features_def.h
>>> b/target/s390x/cpu_features_def.h
>>> index 7c5915c..8998b65 100644
>>> --- a/target/s390x/cpu_features_def.h
>>> +++ b/target/s390x/cpu_features_def.h
>>> @@ -27,8 +27,10 @@ typedef enum {
>>> S390_FEAT_SENSE_RUNNING_STATUS,
>>> S390_FEAT_CONDITIONAL_SSKE,
>>> S390_FEAT_CONFIGURATION_TOPOLOGY,
>>> +S390_FEAT_AP_QUERY_CONFIG_INFO,
>>> S390_FEAT_IPTE_RANGE,
>>> S390_FEAT_NONQ_KEY_SETTING,
>>> +S390_FEAT_AP_FACILITIES_TEST,
>>> S390_FEAT_EXTENDED_TRANSLATION_2,
>>> S390_FEAT_MSA,
>>> S390_FEAT_LONG_DISPLACEMENT,
>>> @@ -118,6 +120,7 @@ typedef enum {
>>> /* Misc */
>>> S390_FEAT_DAT_ENH_2,
>>> S390_FEAT_CMM,
>>> +S390_FEAT_AP,
>>>
>>> /* PLO */
>>> S390_FEAT_PLO_CL,
>>> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
>>> index 1d5f0da..35f91ea 100644
>>> --- a/target/s390x/cpu_models.c
>>> +++ b/target/s390x/cpu_models.c
>>> @@ -770,6 +770,8 @@ static void check_consistency(const S390CPUModel *model)
>>> { S390_FEAT_PRNO_TRNG_QRTCR, S390_FEAT_MSA_EXT_5 },
>>> { S390_FEAT_PRNO_TRNG, S390_FEAT_MSA_EXT_5 },
>>> { S390_FEAT_SIE_KSS, S390_FEAT_SIE_F2 },
>>> +{ S390_FEAT_AP_QUERY_CONFIG_INFO, S390_FEAT_AP },
>>> +{ S390_FEAT_AP_FACILITIES_TEST, S390_FEAT_AP },
>>> };
>>> int i;
>>>
>>> @@ -900,6 +902,16 @@ void s390_realize_cpu_model(CPUState *cs, Error **errp)
>>> cpu->model->cpu_id_format = max_model->cpu_id_format;
>>> cpu->model->cpu_ver = max_model->cpu_ver;
>>>
>>> +/*
>>> + * If the AP facilities are not installed on the guest, then it makes
>>> + * no sense to enable the QCI or APFT facilities because they are only
>>> + * needed by AP facilities.
>>> + */
>>> +if (!test_bit(S390_FEAT_AP, cpu->model->features)) {
>>> +clear_bit(S390_FEAT_AP_QUERY_CONFIG_INFO, cpu->model->features);
>>> +clear_bit(S390_FEAT_AP_FACILITIES_TEST, cpu->model->features);
>>> +}
>>
>> Please don't silently disable things. Instead
>>
>
> I agree, this has to go (already commented on this).
>
>> a) Add consistency checks (check_consistency())
>
> The consistency checks are already in place. As already stated
> before, one could make it produce an error.
>
>> b) Mask the bits out in the KVM CPU model sensing part
>> (kvm_s390_get_host_cpu_model()) - which you already have :)
>>
>
> Getting no ap but qci and apft indicated by KVM is unlikely to
> happen, ever.
I assume it would happen right now under vSIE (or I missed where we
enable the new ECA bit in the vSIE code). Having such simple masking
operations in the "sensing" part usually doesn't hurt.
We try to produce a consistent model even though the hardware/KVM might
be weird.
>
>>> +
>>> check_consistency(cpu->model);
>>> check_compatibility(max_model, cpu->model, errp);
>>> if (*errp) {
>>> diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
>>> index 0cdbc15..2d01b52 100644
>>> --- a/target/s390x/gen-features.c
>>> +++ b/target/s390x/gen-features.c
>>> @@ -447,6 +447,9 @@ static uint16_t full_GEN12_GA1[] = {
>>> S390_FEAT_ADAPTER_INT_SUPPRESSION,
>>> S390_FEAT_EDAT_2,
>>> S390_FEAT_SIDE_EFFECT_ACCESS_ESOP2,
>>> +S390_FEAT_AP,
>>> +S390_FEAT_AP_QUERY_CONFIG_INFO,
>>> +S390_FEAT_AP_FACILITIES_TEST,
>>> };
>>
>> Please keep the order as defined in target/s390x/cpu_features_def.h
>>
>>>
>>> static uint16_t full_GEN12_GA2[] = {
>>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>>> index e13c890..ae20ed8 100644
>>> --- a/target/s390x/kvm.c
>>> +++ b/target/s390x/kvm.c
>>> @@ -2105,6 +2105,7 @@ static int kvm_to_feat[][2] = {
>>> { KVM_S390_VM_CPU_FEAT_PFMFI, S390_FEAT_SIE_PFMFI},
>>> { KVM_S390_VM_CPU_FEAT_SIGPIF, S390_FEAT_SIE_SIGPIF},
>>> { KVM_S390_VM_CPU_FEAT_KSS, S390_FEAT_SIE_KSS},
>>> +{ KVM_S390_VM_CPU_FEAT_AP, S390_FEAT_AP},
>>
>> Nothing speaks against the STFL bits, want to learn more about the
>> S390_FEAT_AP feature :)
>>
>>
>
> Kernel series wise what you are looking for is
> '[PATCH v2 04/15] KVM: s390: CPU model support for AP virtualiza
Re: [Qemu-devel] [PATCH v2 3/3] virtio-balloon: add a timer to limit the free page report waiting time
On 02/27/2018 09:08 PM, Liang Li wrote: On Tue, Feb 27, 2018 at 06:10:47PM +0800, Wei Wang wrote: On 02/27/2018 08:50 AM, Michael S. Tsirkin wrote: On Mon, Feb 26, 2018 at 12:35:31PM +0800, Wei Wang wrote: On 02/09/2018 08:15 PM, Dr. David Alan Gilbert wrote: * Wei Wang (wei.w.w...@intel.com) wrote: I think all this is premature optimization. It is not at all clear that anything is gained by delaying migration. Just ask for hints and start sending pages immediately. If guest tells us a page is free before it's sent, we can skip sending it. OTOH if migration is taking less time to complete than it takes for guest to respond, then we are better off just ignoring the hint. OK, I'll try to create a thread for the free page optimization. We create the thread to poll for free pages at the beginning of the bulk stage, and stops at the end of bulk stage. There are also comments about postcopy support with this feature, I plan to leave that as the second step (that support seems not urgent for now). Best, Wei you can make use the current migration thread instead of creating a new one. This is what this version is doing - we make the optimization implementation be part of the migration thread. To make the optimization go in parallel with the the migration thread, we need another thread for the optimization. Best, Wei
Re: [Qemu-devel] [PATCH v2 3/3] virtio-balloon: add a timer to limit the free page report waiting time
On 02/27/2018 06:34 PM, Dr. David Alan Gilbert wrote: * Wei Wang (wei.w.w...@intel.com) wrote: On 02/09/2018 08:15 PM, Dr. David Alan Gilbert wrote: * Wei Wang (wei.w.w...@intel.com) wrote: This patch adds a timer to limit the time that host waits for the free page hints reported by the guest. Users can specify the time in ms via "free-page-wait-time" command line option. If a user doesn't specify a time, host waits till the guest finishes reporting all the free page hints. The policy (wait for all the free page hints to be reported or use a time limit) is determined by the orchestration layer. That's kind of a get-out; but there's at least two problems: a) With a timeout of 0 (the default) we might hang forever waiting for the guest; broken guests are just too common, we can't do that. b) Even if we were going to do that, you'd have to make sure that migrate_cancel provided a way out. c) How does that work during a savevm snapshot or when the guest is stopped? d) OK, the timer gives us some safety (except c); but how does the orchestration layer ever come up with a 'safe' value for it? Unless we can suggest a safe value that the orchestration layer can use, or a way they can work it out, then they just wont use it. Hi Dave, Sorry for my late response. Please see below: a) I think people would just kill the guest if it is broken. We can also change the default timeout value, for example 1 second, which is enough for the free page reporting. Remember that many VMs are automatically migrated without their being a human involved; those VMs might be in the BIOS or Grub or shutting down at the time of migration; there's no human to look at the VM. OK, thanks for the sharing. I plan to take Michael's suggestion to make the optimization run in parallel with the migration thread. The optimization will be in its own thread, and the migration thread runs as usual (not stuck by the optimization e.g. when the optimization part doesn't return promptly in any case). Best, Wei
Re: [Qemu-devel] Deprecate tilegx ?
On 28.02.2018 08:17, Paolo Bonzini wrote: > On 28/02/2018 07:11, Thomas Huth wrote: >> On 27.02.2018 12:51, Peter Maydell wrote: >>> I propose that we deprecate and plan to remove the unicore32 code: >> [...] >>> Essentially, it seems to be a largely-inactive university R&D project, >>> it's costing us in maintenance effort every time we have to touch it, >>> and I don't think it has any real users. >>> >>> Does anybody disagree? >>> >>> If we go ahead with deprecating then we should: >>> * add a note to Changelog that we're deprecating the target >>> * ditto qemu-doc.texi's deprecation section >>> * patch hw/unicore32/puv3.c to warn on startup that it's deprecated >>> * remove it entirely for the 2.14 release >>> >>> We could also remove linux-user/unicore32 immediately, since >>> the linux-user target has been disabled for some time. >> >> Sounds reasonable to me, but let's wait a week or two for feedback from >> Guan Xuetao. > > Sounds good---thought I would consider dropping unicore32 now with no > formal deprecation period... > >>> Possibly there are other target architectures we could reasonably >>> deprecate-and-remove (though none of the other ones Linux is dropping >>> in this round are ones we support)... >> >> I'd vote for marking tilegx as deprecated, too, since we even do not >> have an active maintainer for that CPU core (at least I did not spot one >> in our MAINTAINERS file). Opinions? > > Tilegx has been last modified in 2015, so it's a little more alive than > unicore32. > > Another one is moxie. Anthony? For moxie, we've got at least a maintainer in MAINTAINERS, and there is still a proper project page online (http://moxielogic.org/blog/pages/architecture.html). And since we've now also got a mini Moxie TCG test in tests/boot-serial-test.c, the CPU code is at least still basically working fine. So IMHO there's no urgent need to mark the moxie CPU as deprecated yet. Thomas
Re: [Qemu-devel] [patches] Re: [PULL] RISC-V QEMU Port Submission
On Wed, 28 Feb 2018 13:41:25 +1300 Michael Clark wrote: > On Wed, Feb 28, 2018 at 5:00 AM, Igor Mammedov wrote: > > > On Tue, 27 Feb 2018 14:01:05 + > > Peter Maydell wrote: > > > > > On 27 February 2018 at 00:15, Michael Clark wrote: > > > > -BEGIN PGP SIGNED MESSAGE- > > > > Hash: SHA1 > > > > > > > > The following changes since commit 0a773d55ac76c5aa89ed9187a3bc5a > > f8c5c2a6d0: > > > > > > > > maintainers: Add myself as a OpenBSD maintainer (2018-02-23 12:05:07 > > +) > > > > > > > > are available in the git repository at: > > > > > > > > https://github.com/riscv/riscv-qemu.git tags/riscv-qemu-upstream-v7 > > > > > > > > for you to fetch changes up to 170a9d412ca1eb3b7ae6f6c1ff86dc > > bdff0fd7a8: > > > > > > > > RISC-V Build Infrastructure (2018-02-27 11:09:43 +1300) > > > > > > > > - > > > > QEMU RISC-V Emulation Support (RV64GC, RV32GC) > > > > > > Hi; thanks for this pull request. Unfortunately it seems to > > > be missing Signed-off-by: tags. Every commit needs to have > > > the Signed-off-by: tags from the people who contributed code to > > > it, indicating that they're OK with the code going into QEMU. > > > (If the work was done by and copyright a company then you don't > > > need to provide signoffs from every person at the company who > > > worked on the code if you don't want to.) > > > > > > > The spike_v1.9 > > > > machine has been renamed to spike_v1.9.1 to match the privileged ISA > > > > version and spike_v1.10 has been made the default machine. > > > > > > I'm confused about this. Generally QEMU boards should model > > > hardware, and the board shouldn't care about the ISA versions. > > > Versioned board names in QEMU generally follow _QEMU_'s versioning, > > > and indicate that a board is identical to whatever we modelled > > > in that earlier QEMU version, for VM migration compatibility. > > > Board renames for minor ISA version bumps sounds like there's going > > > to be a lot of churn and breakage -- is this stuff really ready? > > > (Also, should we really have two different board source files > > > for two different ISA versions? I would have expected these to > > > share a source file to share code.) > > > > > > I did a test build and there are some compile errors: > > > > > > /home/pm215/qemu/linux-user/main.c:38:24: fatal error: target_elf.h: > > > No such file or directory > > > #include "target_elf.h" > > > ^ > > > compilation terminated. > > > > > > This is because your patchset has a clash with commit 542ca4349878a2e > > > which has just merged to master, and refactors out an ifdef ladder, > > > so now all targets supporting linux-user need to provide a > > > linux-user/$ARCH/target_elf.h file. Could you fix that up and rebase, > > > please? > > also '[PATCH v7 03/23] RISC-V CPU Core Definition' still hasn't addressed > > comment http://lists.gnu.org/archive/html/qemu-devel/2018-02/msg06412.html > > which isn't fixed since it was first pointed out (v4). > > > > I'd prefer that being fixed before merge so another people > > won't have to clean it up later after original authors, > > When they try to generalize cpu_type -> cpu_model conversion. > > > > I re-read the email and it doesn't seem clear what you want us to do. > I changed the CPU suffix to a prefix as you requested. The rest of the CPU > initialisation is "modelled" on arm not sh4. Not addressed comment is not related to CPU suffix, it's a separate issue. I haven't had time to clean up that part of ARM cpu initialization yet, so it still uses old approach, but you've been pointed to right approach rather early (v4) but ignored it. There is no point to use legacy approach with new patches that could block other people and would force them to cleanup before doing what they intended to do. I've pointed to sh4 as example that you should use for new CPU types internalization. Beside making RISC-V consistent with direction that part of code moves to, it will also simplify patch. Please ask if something still unclear. > If you want to make a pull request, please use this branch: > > - https://github.com/riscv/riscv-qemu/tree/qemu-upstream-v7
[Qemu-devel] [PATCH v3 2/4] target/arm: Add "_S" suffix to the secure version of a sysreg
This is a preparation for the coming feature of creating dynamically an XML
description for the ARM sysregs.
Add "_S" suffix to the secure version of sysregs that have both S and NS views
Replace (S) and (NS) by _S and _NS for the register that are manually defined,
so all the registers follow the same convention.
Signed-off-by: Abdallah Bouassida
---
target/arm/helper.c | 31 ---
1 file changed, 20 insertions(+), 11 deletions(-)
diff --git a/target/arm/helper.c b/target/arm/helper.c
index bdd212f..1594ec45 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -694,12 +694,12 @@ static const ARMCPRegInfo cp_reginfo[] = {
* the secure register to be properly reset and migrated. There is also no
* v8 EL1 version of the register so the non-secure instance stands alone.
*/
-{ .name = "FCSEIDR(NS)",
+{ .name = "FCSEIDR_NS",
.cp = 15, .opc1 = 0, .crn = 13, .crm = 0, .opc2 = 0,
.access = PL1_RW, .secure = ARM_CP_SECSTATE_NS,
.fieldoffset = offsetof(CPUARMState, cp15.fcseidr_ns),
.resetvalue = 0, .writefn = fcse_write, .raw_writefn = raw_write, },
-{ .name = "FCSEIDR(S)",
+{ .name = "FCSEIDR_S",
.cp = 15, .opc1 = 0, .crn = 13, .crm = 0, .opc2 = 0,
.access = PL1_RW, .secure = ARM_CP_SECSTATE_S,
.fieldoffset = offsetof(CPUARMState, cp15.fcseidr_s),
@@ -715,7 +715,7 @@ static const ARMCPRegInfo cp_reginfo[] = {
.access = PL1_RW, .secure = ARM_CP_SECSTATE_NS,
.fieldoffset = offsetof(CPUARMState, cp15.contextidr_el[1]),
.resetvalue = 0, .writefn = contextidr_write, .raw_writefn = raw_write,
},
-{ .name = "CONTEXTIDR(S)", .state = ARM_CP_STATE_AA32,
+{ .name = "CONTEXTIDR_S", .state = ARM_CP_STATE_AA32,
.cp = 15, .opc1 = 0, .crn = 13, .crm = 0, .opc2 = 1,
.access = PL1_RW, .secure = ARM_CP_SECSTATE_S,
.fieldoffset = offsetof(CPUARMState, cp15.contextidr_s),
@@ -1966,7 +1966,7 @@ static const ARMCPRegInfo generic_timer_cp_reginfo[] = {
cp15.c14_timer[GTIMER_PHYS].ctl),
.writefn = gt_phys_ctl_write, .raw_writefn = raw_write,
},
-{ .name = "CNTP_CTL(S)",
+{ .name = "CNTP_CTL_S",
.cp = 15, .crn = 14, .crm = 2, .opc1 = 0, .opc2 = 1,
.secure = ARM_CP_SECSTATE_S,
.type = ARM_CP_IO | ARM_CP_ALIAS, .access = PL1_RW | PL0_R,
@@ -2005,7 +2005,7 @@ static const ARMCPRegInfo generic_timer_cp_reginfo[] = {
.accessfn = gt_ptimer_access,
.readfn = gt_phys_tval_read, .writefn = gt_phys_tval_write,
},
-{ .name = "CNTP_TVAL(S)",
+{ .name = "CNTP_TVAL_S",
.cp = 15, .crn = 14, .crm = 2, .opc1 = 0, .opc2 = 0,
.secure = ARM_CP_SECSTATE_S,
.type = ARM_CP_NO_RAW | ARM_CP_IO, .access = PL1_RW | PL0_R,
@@ -2059,7 +2059,7 @@ static const ARMCPRegInfo generic_timer_cp_reginfo[] = {
.accessfn = gt_ptimer_access,
.writefn = gt_phys_cval_write, .raw_writefn = raw_write,
},
-{ .name = "CNTP_CVAL(S)", .cp = 15, .crm = 14, .opc1 = 2,
+{ .name = "CNTP_CVAL_S", .cp = 15, .crm = 14, .opc1 = 2,
.secure = ARM_CP_SECSTATE_S,
.access = PL1_RW | PL0_R,
.type = ARM_CP_64BIT | ARM_CP_IO | ARM_CP_ALIAS,
@@ -5562,7 +5562,8 @@ CpuDefinitionInfoList *arch_query_cpu_definitions(Error
**errp)
static void add_cpreg_to_hashtable(ARMCPU *cpu, const ARMCPRegInfo *r,
void *opaque, int state, int secstate,
- int crm, int opc1, int opc2)
+ int crm, int opc1, int opc2,
+ const char *name)
{
/* Private utility function for define_one_arm_cp_reg_with_opaque():
* add a single reginfo struct to the hash table.
@@ -5572,6 +5573,7 @@ static void add_cpreg_to_hashtable(ARMCPU *cpu, const
ARMCPRegInfo *r,
int is64 = (r->type & ARM_CP_64BIT) ? 1 : 0;
int ns = (secstate & ARM_CP_SECSTATE_NS) ? 1 : 0;
+r2->name = name;
/* Reset the secure state to the specific incoming state. This is
* necessary as the register may have been defined with both states.
*/
@@ -5803,19 +5805,26 @@ void define_one_arm_cp_reg_with_opaque(ARMCPU *cpu,
/* Under AArch32 CP registers can be common
* (same for secure and non-secure world) or banked.
*/
+const char *name;
+GString *s;
+
switch (r->secure) {
case ARM_CP_SECSTATE_S:
case ARM_CP_SECSTATE_NS:
add_cpreg_to_hashtable(cpu, r, opaque, state,
- r->secure, crm, opc1, opc2);
+ r->secure, crm, opc1, opc2,
+ r->name);
break;
[Qemu-devel] [PATCH v3 1/4] target/arm: Add "ARM_CP_NO_GDB" as a new bit field for ARMCPRegInfo type
This is a preparation for the coming feature of creating dynamically an XML
description for the ARM sysregs.
A register has ARM_CP_NO_GDB enabled will not be shown in the dynamic XML.
This bit is enabled automatically when creating CP_ANY wildcard aliases.
This bit could be enabled manually for any register we want to remove from the
dynamic XML description.
Signed-off-by: Abdallah Bouassida
---
target/arm/cpu.h| 3 ++-
target/arm/helper.c | 2 +-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 8c839fa..92cfe4c 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1776,10 +1776,11 @@ static inline uint64_t cpreg_to_kvm_id(uint32_t cpregid)
#define ARM_LAST_SPECIAL ARM_CP_DC_ZVA
#define ARM_CP_FPU 0x1000
#define ARM_CP_SVE 0x2000
+#define ARM_CP_NO_GDB0x4000
/* Used only as a terminator for ARMCPRegInfo lists */
#define ARM_CP_SENTINEL 0x
/* Mask of only the flag bits in a type field */
-#define ARM_CP_FLAG_MASK 0x30ff
+#define ARM_CP_FLAG_MASK 0x70ff
/* Valid values for ARMCPRegInfo state field, indicating which of
* the AArch32 and AArch64 execution states this register is visible in.
diff --git a/target/arm/helper.c b/target/arm/helper.c
index c5bc69b..bdd212f 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -5663,7 +5663,7 @@ static void add_cpreg_to_hashtable(ARMCPU *cpu, const
ARMCPRegInfo *r,
if (((r->crm == CP_ANY) && crm != 0) ||
((r->opc1 == CP_ANY) && opc1 != 0) ||
((r->opc2 == CP_ANY) && opc2 != 0)) {
-r2->type |= ARM_CP_ALIAS;
+r2->type |= ARM_CP_ALIAS | ARM_CP_NO_GDB;
}
/* Check that raw accesses are either forbidden or handled. Note that
--
2.7.4
[Qemu-devel] [PATCH v3 4/4] target/arm: Add arm_gdb_set_sysreg() callback
This is a callback to set the cp-regs registered by the dynamic XML.
Signed-off-by: Abdallah Bouassida
---
>> Some of our customers need to connect to Qemu using our tool TRACE32®
>> via GDB,
>> and for some use case they need to have write access to some particular
>> cpregs.
>> So, it will be nice to have this capability!
>> Usually, a user won't modify these registers unless he knows what he is
>> doing!
> I also still don't really like using write_raw_cp_reg() here --
> it will bypass some behaviour you want and in some cases will
> just break the emulation because invariants we assume will
> hold no longer hold. It would be a lot lot safer to not
> provide write access at all, only read access.
Adding to that our customers may need this write access, our tool TRACE32®
needs this also in some particular cases. For example: temporary disabling MMU
to do a physical memory access.
target/arm/cpu.h | 2 ++
target/arm/gdbstub.c | 21 -
target/arm/helper.c | 2 +-
3 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 0e35f64..f4fea98 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -2120,6 +2120,8 @@ static inline bool cp_access_ok(int current_el,
/* Raw read of a coprocessor register (as needed for migration, etc) */
uint64_t read_raw_cp_reg(CPUARMState *env, const ARMCPRegInfo *ri);
+void write_raw_cp_reg(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t v);
+
/**
* write_list_to_cpustate
* @cpu: ARMCPU
diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
index e08ad79..57bd418 100644
--- a/target/arm/gdbstub.c
+++ b/target/arm/gdbstub.c
@@ -183,12 +183,31 @@ static int arm_gdb_get_sysreg(CPUARMState *env, uint8_t
*buf, int reg)
return 0;
}
+static int arm_gdb_set_sysreg(CPUARMState *env, uint8_t *buf, int reg)
+{
+ARMCPU *cpu = arm_env_get_cpu(env);
+const ARMCPRegInfo *ri;
+uint32_t key;
+uint32_t tmp;
+
+tmp = ldl_p(buf);
+key = cpu->dyn_xml.cpregs_keys[reg];
+ri = get_arm_cp_reginfo(arm_env_get_cpu(env)->cp_regs, key);
+if (ri) {
+if (!(ri->type & ARM_CP_CONST)) {
+write_raw_cp_reg(env, ri, tmp);
+return cpreg_field_is_64bit(ri) ? 8 : 4;
+}
+}
+return 0;
+}
+
void arm_register_gdb_regs_for_features(CPUState *cs)
{
int n;
n = arm_gen_dynamic_xml(cs);
-gdb_register_coprocessor(cs, arm_gdb_get_sysreg, NULL,
+gdb_register_coprocessor(cs, arm_gdb_get_sysreg, arm_gdb_set_sysreg,
n, "system-registers.xml", 0);
}
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 1594ec45..4a4afbf 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -200,7 +200,7 @@ uint64_t read_raw_cp_reg(CPUARMState *env, const
ARMCPRegInfo *ri)
}
}
-static void write_raw_cp_reg(CPUARMState *env, const ARMCPRegInfo *ri,
+void write_raw_cp_reg(CPUARMState *env, const ARMCPRegInfo *ri,
uint64_t v)
{
/* Raw write of a coprocessor register (as needed for migration, etc).
--
2.7.4
[Qemu-devel] [PATCH v3 0/4] target/arm: Add a dynamic XML-description of the cp-registers to GDB
The last exchange: https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg03618.html - Add a new "ARM_CP_NO_GDB" bit field and enable it when creating CP_ANY wildcard aliases. - Add "_S" suffix to the secure version of a sysreg and fix the reg names that were manually containing (S) or (NS). - Add the XML dynamic generation and add a callback to read these sysregs. - Add a callback to set these sysregs (I put this as a separate patch as we still discussing if we'll give a write access for these sysregs from GDB). @Peter: Intresting advises from your last review, Thanks a lot ;) Abdallah Bouassida (4): target/arm: Add "ARM_CP_NO_GDB" as a new bit field for ARMCPRegInfo type target/arm: Add "_S" suffix to the secure version of a sysreg target/arm: Add the XML dynamic generation target/arm: Add arm_gdb_set_sysreg() callback gdbstub.c| 7 include/qom/cpu.h| 9 +++- target/arm/cpu.c | 3 ++ target/arm/cpu.h | 22 +- target/arm/gdbstub.c | 116 +++ target/arm/helper.c | 35 ++-- 6 files changed, 177 insertions(+), 15 deletions(-) -- 2.7.4
[Qemu-devel] [PATCH v3 3/4] target/arm: Add the XML dynamic generation
Generate an XML description for the cp-regs.
Register these regs with the gdb_register_coprocessor().
Add arm_gdb_get_sysreg() to use it as a callback to read those regs.
Signed-off-by: Abdallah Bouassida
---
gdbstub.c| 7
include/qom/cpu.h| 9 -
target/arm/cpu.c | 3 ++
target/arm/cpu.h | 17 +
target/arm/gdbstub.c | 97
5 files changed, 132 insertions(+), 1 deletion(-)
diff --git a/gdbstub.c b/gdbstub.c
index f1d5148..ffab30b 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -665,6 +665,9 @@ static const char *get_feature_xml(const char *p, const
char **newp,
pstrcat(target_xml, sizeof(target_xml), "gdb_core_xml_file);
pstrcat(target_xml, sizeof(target_xml), "\"/>");
+if (cc->gdb_has_dynamic_xml) {
+cc->register_gdb_regs_for_features(cpu);
+}
for (r = cpu->gdb_regs; r; r = r->next) {
pstrcat(target_xml, sizeof(target_xml), "xml);
@@ -674,6 +677,10 @@ static const char *get_feature_xml(const char *p, const
char **newp,
}
return target_xml;
}
+if (strncmp(p, "system-registers.xml", len) == 0) {
+CPUState *cpu = first_cpu;
+return cc->gdb_get_dynamic_xml(cpu);
+}
for (i = 0; ; i++) {
name = xml_builtin[i][0];
if (!name || (strncmp(name, p, len) == 0 && strlen(name) == len))
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index aff88fa..04771b3 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -131,6 +131,11 @@ struct TranslationBlock;
* before the insn which triggers a watchpoint rather than after it.
* @gdb_arch_name: Optional callback that returns the architecture name known
* to GDB. The caller must free the returned string with g_free.
+ * @gdb_has_dynamic_xml: Indicates if GDB supports generating a dynamic XML
+ * description for the sysregs of this CPU.
+ * @register_gdb_regs_for_features: Callback for letting GDB create a dynamic
+ * XML description for the sysregs and register those sysregs afterwards.
+ * @gdb_get_dynamic_xml: Callback for letting GDB get the dynamic XML.
* @cpu_exec_enter: Callback for cpu_exec preparation.
* @cpu_exec_exit: Callback for cpu_exec cleanup.
* @cpu_exec_interrupt: Callback for processing interrupts in cpu_exec.
@@ -197,7 +202,9 @@ typedef struct CPUClass {
const struct VMStateDescription *vmsd;
const char *gdb_core_xml_file;
gchar * (*gdb_arch_name)(CPUState *cpu);
-
+bool gdb_has_dynamic_xml;
+void (*register_gdb_regs_for_features)(CPUState *cpu);
+char *(*gdb_get_dynamic_xml)(CPUState *cpu);
void (*cpu_exec_enter)(CPUState *cpu);
void (*cpu_exec_exit)(CPUState *cpu);
bool (*cpu_exec_interrupt)(CPUState *cpu, int interrupt_request);
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 1b3ae62..04c4d04 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1781,6 +1781,9 @@ static void arm_cpu_class_init(ObjectClass *oc, void
*data)
cc->gdb_num_core_regs = 26;
cc->gdb_core_xml_file = "arm-core.xml";
cc->gdb_arch_name = arm_gdb_arch_name;
+cc->gdb_has_dynamic_xml = true;
+cc->register_gdb_regs_for_features = arm_register_gdb_regs_for_features;
+cc->gdb_get_dynamic_xml = arm_gdb_get_dynamic_xml;
cc->gdb_stop_before_watchpoint = true;
cc->debug_excp_handler = arm_debug_excp_handler;
cc->debug_check_watchpoint = arm_debug_check_watchpoint;
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 92cfe4c..0e35f64 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -133,6 +133,19 @@ enum {
s<2n+1> maps to the most significant half of d
*/
+/**
+ * DynamicGDBXMLInfo:
+ * @desc: Contains the XML descriptions.
+ * @num_cpregs: Number of the Coprocessor registers seen by GDB.
+ * @cpregs_keys: Array that contains the corresponding Key of
+ * a given cpreg with the same order of the cpreg in the XML description.
+ */
+typedef struct DynamicGDBXMLInfo {
+char *desc;
+int num_cpregs;
+uint32_t *cpregs_keys;
+} DynamicGDBXMLInfo;
+
/* CPU state for each instance of a generic timer (in cp15 c14) */
typedef struct ARMGenericTimer {
uint64_t cval; /* Timer CompareValue register */
@@ -671,6 +684,8 @@ struct ARMCPU {
uint64_t *cpreg_vmstate_values;
int32_t cpreg_vmstate_array_len;
+DynamicGDBXMLInfo dyn_xml;
+
/* Timers used by the generic (architected) timer */
QEMUTimer *gt_timer[NUM_GTIMERS];
/* GPIO outputs for generic timer */
@@ -835,6 +850,8 @@ hwaddr arm_cpu_get_phys_page_attrs_debug(CPUState *cpu,
vaddr addr,
int arm_cpu_gdb_read_register(CPUState *cpu, uint8_t *buf, int reg);
int arm_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
+void arm_register_gdb_regs_for_features(CPUState *cpu);
+char *arm_gdb_get_dynamic_xml(CPUState *cpu);
int arm_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState
Re: [Qemu-devel] [qemu-s390x] [PATCH] hw/s390x: Add the possibility to specify the netboot image on the command line
On 27.02.2018 12:32, Thomas Huth wrote:
> The file name of the netboot binary is currently hard-coded to
> "s390-netboot.img", without a possibility for the user to select
> an alternative firmware image here. That's unfortunate, especially
> since the basics are already there: The filename is a property of
> the s390-ipl device. So we just have to add a check whether the user
> already provided the property and only set the default if the string
> is still empty. Now it is possible to select a different firmware
> image with "-global s390-ipl.netboot_fw=/path/to/s390-netboot.img".
>
> Signed-off-by: Thomas Huth
> ---
> hw/s390x/s390-virtio-ccw.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 4abbe89..7b3fb5f 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -254,8 +254,10 @@ static void s390_init_ipl_dev(const char
> *kernel_filename,
> }
> qdev_prop_set_string(dev, "cmdline", kernel_cmdline);
> qdev_prop_set_string(dev, "firmware", firmware);
> -qdev_prop_set_string(dev, "netboot_fw", netboot_fw);
> qdev_prop_set_bit(dev, "enforce_bios", enforce_bios);
> +if (!strlen(object_property_get_str(new, "netboot_fw", &error_abort))) {
Isn't it the case that object_property_get_str() can return also NULL?
(looking at s390_ipl_set_loadparm())
> +qdev_prop_set_string(dev, "netboot_fw", netboot_fw);
> +}
> object_property_add_child(qdev_get_machine(), TYPE_S390_IPL,
>new, NULL);
> object_unref(new);
>
--
Thanks,
David / dhildenb
Re: [Qemu-devel] [PULL 00/12] Ui 20180227 patches
On 28 February 2018 at 06:26, Gerd Hoffmann wrote: > Hi, > >> Hi. This failed to build on my OpenBSD test box: >> >> CC qga/guest-agent-command-state.o >> In file included from /home/qemu/include/qemu/osdep.h:30:0, >> from /home/qemu/qga/guest-agent-command-state.c:12: >> ./config-host.h:29:0: warning: "CONFIG_SDL" redefined >> #define CONFIG_SDL m >> ^ >> ./config-host.h:17:0: note: this is the location of the previous definition >> #define CONFIG_SDL 1 >> ^ >> >> (warning repeated for pretty much every object file) >> >> and then linking of the final executables failed with >> >> ../audio/audio.o:(.data.rel.ro+0x0): undefined reference to >> `sdl_audio_driver' > > Oh, right, there is sdl audio, completely forgot about that ... > > (this probably triggers on openbsd because sdl audio is the default > there). > > I think modularizing SDL isn't that easy then. > Can you just drop the "sdl: build as ui module" patch? I can't drop patches from signed pull requests -- you need to respin it. thanks -- PMM
Re: [Qemu-devel] [PATCH v2 5/5] s390x/cpumodel: Set up CPU model for AP device support
On Wed, 28 Feb 2018 11:26:30 +0100 David Hildenbrand wrote: > Then I request the following change in KVM: > > If KVM_S390_VM_CPU_FEAT_AP is enabled in KVM, ECA.28 is _always_ set > (not just if an AP device is configured). This especially makes things a > lot easier when it comes to handling hotplugged CPUs and avoiding race > conditions when enabling these bits as mentioned in the KVM series. > > KVM_S390_VM_CPU_FEAT_AP == AP instructions available for the guest > (don't throw an operation exception). > > So this feature then really is guest ABI. The instructions are > available. If there is no device configured, bad luck. Sounds sensible from my POV.
Re: [Qemu-devel] [PATCH] i386: Allow monitor / mwait cpuid override
On 28/02/2018 00:55, Alexander Graf wrote: > > > On 27.02.18 10:52, Gonglei (Arei) wrote: >> Hi all, >> >> Guests could achive good performance in 'Message Passing Workloads' >> scenarios when knowing the X86_FEATURE_MWAIT feature which is presented by >> qemu. >> the reason is that after knowing that feature, >> the guest could use mwait method, which saves VMEXIT, >> to do idle, and achives high performace in latency-sensitive scenario. >> >> Is there any plan for this patch? >> >> Or May I send a updated version based on yours? @Alex? > > Oh, did I drop the ball on this one? If that's the case, sure, go ahead. Hi, it is probably best to implement this feature based on the HINT_DEDICATED cpuid bit that Wanpeng Li is adding. Paolo
Re: [Qemu-devel] [patches] Re: [PULL] RISC-V QEMU Port Submission
On 28 February 2018 at 00:09, Michael Clark wrote: > I've just talked to SiFive about this. They have agreed that we can remove > the sifive_e300 and sifive_u500 boards from the patch series that we are > going to submit upstream again later this week or early next week. These > machines and their devices are pretty easy for us to maintain in the riscv > or a sifive repo. This trims the number of machines from 5 to 3 and lets us > remove the SiFiveUART and SiFivePRCI from the next patch series we are > going to submit. e.g. v8 Models of boards which people actively want and are using are fine (though indeed you can save them for a later round of patches if you like). And it sounds like the 1.9.1 stuff is genuinely wanted, so thanks for the clarification there. What I want to avoid is boards going into QEMU just because you happened to have them already. Once a board model goes into QEMU it's a commitment to supporting it for the long term, and getting rid of it again is hard. > However I'm inclined to leave it as it is, at this point. It is not > something that we can't change in the future once the code is in-tree. With my 'upstream dev' hat on, I tend to be suspicious of this line of argument, because in a lot of cases what tends to happen is that the code for some new target or device goes in-tree, and then the people who worked on submitting it disappear, or never actually do get round to refactoring[*]. You get more leeway for making this argument the longer you've been around and participating in QEMU development, because then you have a track record of following up on things. [*] in fact we're currently discussing deleting support for a couple of target architectures that were basically "once the code went into mainline nothing further was ever done to it except global-refactorings and other tree wide maintenance by other QEMU developers". thanks -- PMM
Re: [Qemu-devel] [patches] Re: [PULL] RISC-V QEMU Port Submission
On 28 February 2018 at 11:53, Peter Maydell wrote: > With my 'upstream dev' hat on, I tend to be suspicious of this > line of argument, because in a lot of cases what tends to happen > is that the code for some new target or device goes in-tree, and > then the people who worked on submitting it disappear, or never > actually do get round to refactoring[*]. You get more leeway for > making this argument the longer you've been around and participating > in QEMU development, because then you have a track record of > following up on things. That said, I can probably live with it in this particular instance, given when the 2.12 codefreeze deadline is. thanks -- PMM
[Qemu-devel] [PULL 11/11] curses: build as ui module
Also drop curses libs from libs_softmmu. Add CURSES_{CFLAGS,LIBS}
variables so we can use them for linking the curses module.
Shared library dependencies dropped from qemu-system-*:
libncursesw.so.5 => /lib64/libncursesw.so.5
libtinfo.so.5 => /lib64/libtinfo.so.5
Signed-off-by: Gerd Hoffmann
Message-id: 20180221131537.31341-13-kra...@redhat.com
---
configure| 6 +++---
ui/Makefile.objs | 6 +-
2 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/configure b/configure
index 8074fc5001..5ecd538617 100755
--- a/configure
+++ b/configure
@@ -3269,8 +3269,6 @@ EOF
unset IFS
if compile_prog "$curses_inc" "$curses_lib" ; then
curses_found=yes
-QEMU_CFLAGS="$curses_inc $QEMU_CFLAGS"
-libs_softmmu="$curses_lib $libs_softmmu"
break
fi
done
@@ -6038,7 +6036,9 @@ if test "$cocoa" = "yes" ; then
echo "CONFIG_COCOA=y" >> $config_host_mak
fi
if test "$curses" = "yes" ; then
- echo "CONFIG_CURSES=y" >> $config_host_mak
+ echo "CONFIG_CURSES=m" >> $config_host_mak
+ echo "CURSES_CFLAGS=$curses_inc" >> $config_host_mak
+ echo "CURSES_LIBS=$curses_lib" >> $config_host_mak
fi
if test "$pipe2" = "yes" ; then
echo "CONFIG_PIPE2=y" >> $config_host_mak
diff --git a/ui/Makefile.objs b/ui/Makefile.objs
index 49223b0573..a37232762b 100644
--- a/ui/Makefile.objs
+++ b/ui/Makefile.objs
@@ -13,7 +13,6 @@ common-obj-$(CONFIG_LINUX) += input-linux.o
common-obj-$(CONFIG_SPICE) += spice-core.o spice-input.o spice-display.o
common-obj-$(CONFIG_SDL) += sdl.mo
common-obj-$(CONFIG_COCOA) += cocoa.o
-common-obj-$(CONFIG_CURSES) += curses.o
common-obj-$(CONFIG_VNC) += $(vnc-obj-y)
common-obj-$(call lnot,$(CONFIG_VNC)) += vnc-stubs.o
@@ -39,6 +38,11 @@ gtk.mo-objs := gtk.o
gtk.mo-cflags := $(GTK_CFLAGS) $(VTE_CFLAGS)
gtk.mo-libs := $(GTK_LIBS) $(VTE_LIBS)
+common-obj-$(CONFIG_CURSES) += curses.mo
+curses.mo-objs := curses.o
+curses.mo-cflags := $(CURSES_CFLAGS)
+curses.mo-libs := $(CURSES_LIBS)
+
ifeq ($(CONFIG_OPENGL),y)
common-obj-y += shader.o
common-obj-y += console-gl.o
--
2.9.3
[Qemu-devel] [PULL 06/11] console: add and use qemu_display_find_default
Using the new display registry instead of #ifdefs in vl.c.
Signed-off-by: Gerd Hoffmann
Message-id: 20180221131537.31341-7-kra...@redhat.com
---
include/ui/console.h | 1 +
ui/console.c | 19 +++
vl.c | 15 +--
3 files changed, 25 insertions(+), 10 deletions(-)
diff --git a/include/ui/console.h b/include/ui/console.h
index 94726cf190..3a53db9360 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -441,6 +441,7 @@ struct QemuDisplay {
};
void qemu_display_register(QemuDisplay *ui);
+bool qemu_display_find_default(DisplayOptions *opts);
void qemu_display_early_init(DisplayOptions *opts);
void qemu_display_init(DisplayState *ds, DisplayOptions *opts);
diff --git a/ui/console.c b/ui/console.c
index a11b120fc8..25d342cdcb 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -2188,6 +2188,25 @@ void qemu_display_register(QemuDisplay *ui)
dpys[ui->type] = ui;
}
+bool qemu_display_find_default(DisplayOptions *opts)
+{
+static DisplayType prio[] = {
+DISPLAY_TYPE_GTK,
+DISPLAY_TYPE_SDL,
+DISPLAY_TYPE_COCOA
+};
+int i;
+
+for (i = 0; i < ARRAY_SIZE(prio); i++) {
+if (dpys[prio[i]] == NULL) {
+continue;
+}
+opts->type = prio[i];
+return true;
+}
+return false;
+}
+
void qemu_display_early_init(DisplayOptions *opts)
{
assert(opts->type < DISPLAY_TYPE__MAX);
diff --git a/vl.c b/vl.c
index 47c953f8dc..59e56593f8 100644
--- a/vl.c
+++ b/vl.c
@@ -4285,17 +4285,12 @@ int main(int argc, char **argv, char **envp)
}
#endif
if (dpy.type == DISPLAY_TYPE_DEFAULT && !display_remote) {
-#if defined(CONFIG_GTK)
-dpy.type = DISPLAY_TYPE_GTK;
-#elif defined(CONFIG_SDL)
-dpy.type = DISPLAY_TYPE_SDL;
-#elif defined(CONFIG_COCOA)
-dpy.type = DISPLAY_TYPE_COCOA;
-#elif defined(CONFIG_VNC)
-vnc_parse("localhost:0,to=99,id=default", &error_abort);
-#else
-dpy.type = DISPLAY_TYPE_NONE;
+if (!qemu_display_find_default(&dpy)) {
+dpy.type = DISPLAY_TYPE_NONE;
+#if defined(CONFIG_VNC)
+vnc_parse("localhost:0,to=99,id=default", &error_abort);
#endif
+}
}
if (dpy.type == DISPLAY_TYPE_DEFAULT) {
dpy.type = DISPLAY_TYPE_NONE;
--
2.9.3
[Qemu-devel] [PULL 04/11] curses: switch over to new display registry
Signed-off-by: Gerd Hoffmann
Message-id: 20180221131537.31341-5-kra...@redhat.com
---
include/ui/console.h | 12
ui/curses.c | 14 +-
vl.c | 17 ++---
3 files changed, 15 insertions(+), 28 deletions(-)
diff --git a/include/ui/console.h b/include/ui/console.h
index f8c462106a..3ea6cf0870 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -453,18 +453,6 @@ int vnc_display_pw_expire(const char *id, time_t expires);
QemuOpts *vnc_parse(const char *str, Error **errp);
int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp);
-/* curses.c */
-#ifdef CONFIG_CURSES
-void curses_display_init(DisplayState *ds, DisplayOptions *opts);
-#else
-static inline void curses_display_init(DisplayState *ds, DisplayOptions *opts)
-{
-/* This must never be called if CONFIG_CURSES is disabled */
-error_report("curses support is disabled");
-abort();
-}
-#endif
-
/* input.c */
int index_from_key(const char *key, size_t key_length);
diff --git a/ui/curses.c b/ui/curses.c
index 597e47fd4a..59d819fd4d 100644
--- a/ui/curses.c
+++ b/ui/curses.c
@@ -435,7 +435,7 @@ static const DisplayChangeListenerOps dcl_ops = {
.dpy_text_cursor = curses_cursor_position,
};
-void curses_display_init(DisplayState *ds, DisplayOptions *opts)
+static void curses_display_init(DisplayState *ds, DisplayOptions *opts)
{
#ifndef _WIN32
if (!isatty(1)) {
@@ -456,3 +456,15 @@ void curses_display_init(DisplayState *ds, DisplayOptions
*opts)
invalidate = 1;
}
+
+static QemuDisplay qemu_display_curses = {
+.type = DISPLAY_TYPE_CURSES,
+.init = curses_display_init,
+};
+
+static void register_curses(void)
+{
+qemu_display_register(&qemu_display_curses);
+}
+
+type_init(register_curses);
diff --git a/vl.c b/vl.c
index 2c3cb4651c..2b4af34fbb 100644
--- a/vl.c
+++ b/vl.c
@@ -2161,12 +2161,7 @@ static void parse_display(const char *p)
exit(1);
#endif
} else if (strstart(p, "curses", &opts)) {
-#ifdef CONFIG_CURSES
dpy.type = DISPLAY_TYPE_CURSES;
-#else
-error_report("curses support is disabled");
-exit(1);
-#endif
} else if (strstart(p, "gtk", &opts)) {
dpy.type = DISPLAY_TYPE_GTK;
while (*opts) {
@@ -4647,17 +4642,9 @@ int main(int argc, char **argv, char **envp)
qemu_register_reset(restore_boot_order, g_strdup(boot_order));
}
-ds = init_displaystate();
-
/* init local displays */
-switch (dpy.type) {
-case DISPLAY_TYPE_CURSES:
-curses_display_init(ds, &dpy);
-break;
-default:
-qemu_display_init(ds, &dpy);
-break;
-}
+ds = init_displaystate();
+qemu_display_init(ds, &dpy);
/* must be after terminal init, SDL library changes signal handlers */
os_setup_signal_handling();
--
2.9.3
[Qemu-devel] [PULL 05/11] egl-headless: switch over to new display registry
Signed-off-by: Gerd Hoffmann
Message-id: 20180221131537.31341-6-kra...@redhat.com
---
include/ui/console.h | 3 ---
ui/egl-headless.c| 20 +++-
vl.c | 12
3 files changed, 19 insertions(+), 16 deletions(-)
diff --git a/include/ui/console.h b/include/ui/console.h
index 3ea6cf0870..94726cf190 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -456,7 +456,4 @@ int vnc_init_func(void *opaque, QemuOpts *opts, Error
**errp);
/* input.c */
int index_from_key(const char *key, size_t key_length);
-/* egl-headless.c */
-void egl_headless_init(DisplayOptions *opts);
-
#endif
diff --git a/ui/egl-headless.c b/ui/egl-headless.c
index b33e0b21fd..7c877122d3 100644
--- a/ui/egl-headless.c
+++ b/ui/egl-headless.c
@@ -164,7 +164,12 @@ static const DisplayChangeListenerOps egl_ops = {
.dpy_gl_update = egl_scanout_flush,
};
-void egl_headless_init(DisplayOptions *opts)
+static void early_egl_headless_init(DisplayOptions *opts)
+{
+display_opengl = 1;
+}
+
+static void egl_headless_init(DisplayState *ds, DisplayOptions *opts)
{
QemuConsole *con;
egl_dpy *edpy;
@@ -188,3 +193,16 @@ void egl_headless_init(DisplayOptions *opts)
register_displaychangelistener(&edpy->dcl);
}
}
+
+static QemuDisplay qemu_display_egl = {
+.type = DISPLAY_TYPE_EGL_HEADLESS,
+.early_init = early_egl_headless_init,
+.init = egl_headless_init,
+};
+
+static void register_egl(void)
+{
+qemu_display_register(&qemu_display_egl);
+}
+
+type_init(register_egl);
diff --git a/vl.c b/vl.c
index 2b4af34fbb..47c953f8dc 100644
--- a/vl.c
+++ b/vl.c
@@ -2153,13 +2153,7 @@ static void parse_display(const char *p)
exit(1);
}
} else if (strstart(p, "egl-headless", &opts)) {
-#ifdef CONFIG_OPENGL_DMABUF
-display_opengl = 1;
dpy.type = DISPLAY_TYPE_EGL_HEADLESS;
-#else
-error_report("egl support is disabled");
-exit(1);
-#endif
} else if (strstart(p, "curses", &opts)) {
dpy.type = DISPLAY_TYPE_CURSES;
} else if (strstart(p, "gtk", &opts)) {
@@ -4659,12 +4653,6 @@ int main(int argc, char **argv, char **envp)
qemu_spice_display_init();
}
-#ifdef CONFIG_OPENGL_DMABUF
-if (dpy.type == DISPLAY_TYPE_EGL_HEADLESS) {
-egl_headless_init(&dpy);
-}
-#endif
-
if (foreach_device_config(DEV_GDB, gdbserver_start) < 0) {
exit(1);
}
--
2.9.3
[Qemu-devel] [PULL 02/11] sdl: switch over to new display registry
Signed-off-by: Gerd Hoffmann
Message-id: 20180221131537.31341-3-kra...@redhat.com
---
include/ui/console.h | 19 ---
ui/sdl.c | 24 +---
ui/sdl2.c| 17 +++--
vl.c | 15 +--
4 files changed, 29 insertions(+), 46 deletions(-)
diff --git a/include/ui/console.h b/include/ui/console.h
index 817f9b9bcc..cb86e6a0dd 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -444,25 +444,6 @@ void qemu_display_register(QemuDisplay *ui);
void qemu_display_early_init(DisplayOptions *opts);
void qemu_display_init(DisplayState *ds, DisplayOptions *opts);
-/* sdl.c */
-#ifdef CONFIG_SDL
-void sdl_display_early_init(DisplayOptions *opts);
-void sdl_display_init(DisplayState *ds, DisplayOptions *opts);
-#else
-static inline void sdl_display_early_init(DisplayOptions *opts)
-{
-/* This must never be called if CONFIG_SDL is disabled */
-error_report("SDL support is disabled");
-abort();
-}
-static inline void sdl_display_init(DisplayState *ds, DisplayOptions *opts)
-{
-/* This must never be called if CONFIG_SDL is disabled */
-error_report("SDL support is disabled");
-abort();
-}
-#endif
-
/* cocoa.m */
#ifdef CONFIG_COCOA
void cocoa_display_init(DisplayState *ds, DisplayOptions *opts);
diff --git a/ui/sdl.c b/ui/sdl.c
index c4ae7ab05d..a5fd503c25 100644
--- a/ui/sdl.c
+++ b/ui/sdl.c
@@ -901,17 +901,7 @@ static const DisplayChangeListenerOps dcl_ops = {
.dpy_cursor_define= sdl_mouse_define,
};
-void sdl_display_early_init(DisplayOptions *opts)
-{
-if (opts->has_gl && opts->gl) {
-fprintf(stderr,
-"SDL1 display code has no opengl support.\n"
-"Please recompile qemu with SDL2, using\n"
-"./configure --enable-sdl --with-sdlabi=2.0\n");
-}
-}
-
-void sdl_display_init(DisplayState *ds, DisplayOptions *o)
+static void sdl1_display_init(DisplayState *ds, DisplayOptions *o)
{
int flags;
uint8_t data = 0;
@@ -1023,3 +1013,15 @@ void sdl_display_init(DisplayState *ds, DisplayOptions
*o)
atexit(sdl_cleanup);
}
+
+static QemuDisplay qemu_display_sdl1 = {
+.type = DISPLAY_TYPE_SDL,
+.init = sdl1_display_init,
+};
+
+static void register_sdl1(void)
+{
+qemu_display_register(&qemu_display_sdl1);
+}
+
+type_init(register_sdl1);
diff --git a/ui/sdl2.c b/ui/sdl2.c
index b5a0fa1d13..83b917fa37 100644
--- a/ui/sdl2.c
+++ b/ui/sdl2.c
@@ -751,7 +751,7 @@ static const DisplayChangeListenerOps dcl_gl_ops = {
};
#endif
-void sdl_display_early_init(DisplayOptions *o)
+static void sdl2_display_early_init(DisplayOptions *o)
{
assert(o->type == DISPLAY_TYPE_SDL);
if (o->has_gl && o->gl) {
@@ -761,7 +761,7 @@ void sdl_display_early_init(DisplayOptions *o)
}
}
-void sdl_display_init(DisplayState *ds, DisplayOptions *o)
+static void sdl2_display_init(DisplayState *ds, DisplayOptions *o)
{
int flags;
uint8_t data = 0;
@@ -861,3 +861,16 @@ void sdl_display_init(DisplayState *ds, DisplayOptions *o)
atexit(sdl_cleanup);
}
+
+static QemuDisplay qemu_display_sdl2 = {
+.type = DISPLAY_TYPE_SDL,
+.early_init = sdl2_display_early_init,
+.init = sdl2_display_init,
+};
+
+static void register_sdl1(void)
+{
+qemu_display_register(&qemu_display_sdl2);
+}
+
+type_init(register_sdl1);
diff --git a/vl.c b/vl.c
index 70458ba708..45900ba7e6 100644
--- a/vl.c
+++ b/vl.c
@@ -2085,7 +2085,6 @@ static void parse_display(const char *p)
const char *opts;
if (strstart(p, "sdl", &opts)) {
-#ifdef CONFIG_SDL
dpy.type = DISPLAY_TYPE_SDL;
while (*opts) {
const char *nextopt;
@@ -2146,10 +2145,6 @@ static void parse_display(const char *p)
}
opts = nextopt;
}
-#else
-error_report("SDL support is disabled");
-exit(1);
-#endif
} else if (strstart(p, "vnc", &opts)) {
if (*opts == '=') {
vnc_parse(opts + 1, &error_fatal);
@@ -4327,12 +4322,7 @@ int main(int argc, char **argv, char **envp)
"ignoring option");
}
-if (dpy.type == DISPLAY_TYPE_SDL) {
-sdl_display_early_init(&dpy);
-} else {
-qemu_display_early_init(&dpy);
-}
-
+qemu_display_early_init(&dpy);
qemu_console_early_init();
if (dpy.has_gl && dpy.gl && display_opengl == 0) {
@@ -4664,9 +4654,6 @@ int main(int argc, char **argv, char **envp)
case DISPLAY_TYPE_CURSES:
curses_display_init(ds, &dpy);
break;
-case DISPLAY_TYPE_SDL:
-sdl_display_init(ds, &dpy);
-break;
case DISPLAY_TYPE_COCOA:
cocoa_display_init(ds, &dpy);
break;
--
2.9.3
[Qemu-devel] [PULL 03/11] cocoa: switch over to new display registry
Signed-off-by: Gerd Hoffmann
Message-id: 20180221131537.31341-4-kra...@redhat.com
---
include/ui/console.h | 12
vl.c | 3 ---
ui/cocoa.m | 14 +-
3 files changed, 13 insertions(+), 16 deletions(-)
diff --git a/include/ui/console.h b/include/ui/console.h
index cb86e6a0dd..f8c462106a 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -444,18 +444,6 @@ void qemu_display_register(QemuDisplay *ui);
void qemu_display_early_init(DisplayOptions *opts);
void qemu_display_init(DisplayState *ds, DisplayOptions *opts);
-/* cocoa.m */
-#ifdef CONFIG_COCOA
-void cocoa_display_init(DisplayState *ds, DisplayOptions *opts);
-#else
-static inline void cocoa_display_init(DisplayState *ds, DisplayOptions *opts)
-{
-/* This must never be called if CONFIG_COCOA is disabled */
-error_report("Cocoa support is disabled");
-abort();
-}
-#endif
-
/* vnc.c */
void vnc_display_init(const char *id);
void vnc_display_open(const char *id, Error **errp);
diff --git a/vl.c b/vl.c
index 45900ba7e6..2c3cb4651c 100644
--- a/vl.c
+++ b/vl.c
@@ -4654,9 +4654,6 @@ int main(int argc, char **argv, char **envp)
case DISPLAY_TYPE_CURSES:
curses_display_init(ds, &dpy);
break;
-case DISPLAY_TYPE_COCOA:
-cocoa_display_init(ds, &dpy);
-break;
default:
qemu_display_init(ds, &dpy);
break;
diff --git a/ui/cocoa.m b/ui/cocoa.m
index 90d9aa57ea..8b0dce90cb 100644
--- a/ui/cocoa.m
+++ b/ui/cocoa.m
@@ -1683,7 +1683,7 @@ static void addRemovableDevicesMenuItems(void)
qapi_free_BlockInfoList(pointerToFree);
}
-void cocoa_display_init(DisplayState *ds, DisplayOptions *opts)
+static void cocoa_display_init(DisplayState *ds, DisplayOptions *opts)
{
COCOA_DEBUG("qemu_cocoa: cocoa_display_init\n");
@@ -1713,3 +1713,15 @@ void cocoa_display_init(DisplayState *ds, DisplayOptions
*opts)
*/
addRemovableDevicesMenuItems();
}
+
+static QemuDisplay qemu_display_cocoa = {
+.type = DISPLAY_TYPE_COCOA,
+.init = cocoa_display_init,
+};
+
+static void register_cocoa(void)
+{
+qemu_display_register(&qemu_display_cocoa);
+}
+
+type_init(register_cocoa);
--
2.9.3
[Qemu-devel] [PULL 08/11] configure: add X11 vars to config-host.mak
Simplifies handling the X11 dependency,
also makes ui/Makefile.objs more readable.
Signed-off-by: Gerd Hoffmann
Message-id: 20180221131537.31341-9-kra...@redhat.com
---
configure| 10 --
ui/Makefile.objs | 5 -
2 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/configure b/configure
index 39f3a43001..f6dc1c92b3 100755
--- a/configure
+++ b/configure
@@ -2509,9 +2509,8 @@ fi
##
# X11 probe
-x11_cflags=
-x11_libs=-lX11
if $pkg_config --exists "x11"; then
+have_x11=yes
x11_cflags=$($pkg_config --cflags x11)
x11_libs=$($pkg_config --libs x11)
fi
@@ -2544,6 +2543,7 @@ if test "$gtk" != "no"; then
gtk_libs=$($pkg_config --libs $gtkpackage)
gtk_version=$($pkg_config --modversion $gtkpackage)
if $pkg_config --exists "$gtkx11package >= $gtkversion"; then
+need_x11=yes
gtk_cflags="$gtk_cflags $x11_cflags"
gtk_libs="$gtk_libs $x11_libs"
fi
@@ -2912,6 +2912,7 @@ if test "$sdl" = "yes" ; then
int main(void) { return 0; }
EOF
if compile_prog "$sdl_cflags $x11_cflags" "$sdl_libs $x11_libs" ; then
+need_x11=yes
sdl_cflags="$sdl_cflags $x11_cflags"
sdl_libs="$sdl_libs $x11_libs"
fi
@@ -6024,6 +6025,11 @@ if test "$modules" = "yes"; then
echo "CONFIG_STAMP=_$( (echo $qemu_version; echo $pkgversion; cat $0) |
$shacmd - | cut -f1 -d\ )" >> $config_host_mak
echo "CONFIG_MODULES=y" >> $config_host_mak
fi
+if test "$have_x11" = "yes" -a "$need_x11" = "yes"; then
+ echo "CONFIG_X11=y" >> $config_host_mak
+ echo "X11_CFLAGS=$x11_cflags" >> $config_host_mak
+ echo "X11_LIBS=$x11_libs" >> $config_host_mak
+fi
if test "$sdl" = "yes" ; then
echo "CONFIG_SDL=y" >> $config_host_mak
echo "CONFIG_SDLABI=$sdlabi" >> $config_host_mak
diff --git a/ui/Makefile.objs b/ui/Makefile.objs
index ced7d91a63..9b725b07aa 100644
--- a/ui/Makefile.objs
+++ b/ui/Makefile.objs
@@ -17,7 +17,10 @@ common-obj-$(CONFIG_CURSES) += curses.o
common-obj-$(CONFIG_VNC) += $(vnc-obj-y)
common-obj-$(call lnot,$(CONFIG_VNC)) += vnc-stubs.o
common-obj-$(CONFIG_GTK) += gtk.o
-common-obj-$(if $(CONFIG_WIN32),n,$(if $(CONFIG_SDL),y,$(CONFIG_GTK))) +=
x_keymap.o
+
+common-obj-$(CONFIG_X11) += x_keymap.o
+x_keymap.o-cflags := $(X11_CFLAGS)
+x_keymap.o-libs := $(X11_LIBS)
ifeq ($(CONFIG_SDLABI),1.2)
sdl.mo-objs := sdl.o sdl_zoom.o
--
2.9.3
[Qemu-devel] [PULL 10/11] gtk: build as ui module
Also drop gtk and vte libs from libs_softmmu, so the libs are not pulled in unless the gtk module actually gets loaded. Shared library dependencies dropped from qemu-system-*: libEGL.so.1 => /lib64/libEGL.so.1 libGL.so.1 => /lib64/libGL.so.1 libXcomposite.so.1 => /lib64/libXcomposite.so.1 libXcursor.so.1 => /lib64/libXcursor.so.1 libXdamage.so.1 => /lib64/libXdamage.so.1 libXfixes.so.3 => /lib64/libXfixes.so.3 libXinerama.so.1 => /lib64/libXinerama.so.1 libXrandr.so.2 => /lib64/libXrandr.so.2 libXrender.so.1 => /lib64/libXrender.so.1 libXxf86vm.so.1 => /lib64/libXxf86vm.so.1 libatk-1.0.so.0 => /lib64/libatk-1.0.so.0 libatk-bridge-2.0.so.0 => /lib64/libatk-bridge-2.0.so.0 libatspi.so.0 => /lib64/libatspi.so.0 libcairo-gobject.so.2 => /lib64/libcairo-gobject.so.2 libcairo.so.2 => /lib64/libcairo.so.2 libfontconfig.so.1 => /lib64/libfontconfig.so.1 libfreetype.so.6 => /lib64/libfreetype.so.6 libgdk-3.so.0 => /lib64/libgdk-3.so.0 libgdk_pixbuf-2.0.so.0 => /lib64/libgdk_pixbuf-2.0.so.0 libglapi.so.0 => /lib64/libglapi.so.0 libgraphite2.so.3 => /lib64/libgraphite2.so.3 libgtk-3.so.0 => /lib64/libgtk-3.so.0 libharfbuzz.so.0 => /lib64/libharfbuzz.so.0 libpango-1.0.so.0 => /lib64/libpango-1.0.so.0 libpangocairo-1.0.so.0 => /lib64/libpangocairo-1.0.so.0 libpangoft2-1.0.so.0 => /lib64/libpangoft2-1.0.so.0 libpcre2-8.so.0 => /lib64/libpcre2-8.so.0 libthai.so.0 => /lib64/libthai.so.0 libvte-2.91.so.0 => /lib64/libvte-2.91.so.0 libwayland-cursor.so.0 => /lib64/libwayland-cursor.so.0 libwayland-egl.so.1 => /lib64/libwayland-egl.so.1 libxcb-dri2.so.0 => /lib64/libxcb-dri2.so.0 libxcb-dri3.so.0 => /lib64/libxcb-dri3.so.0 libxcb-glx.so.0 => /lib64/libxcb-glx.so.0 libxcb-present.so.0 => /lib64/libxcb-present.so.0 libxcb-render.so.0 => /lib64/libxcb-render.so.0 libxcb-shm.so.0 => /lib64/libxcb-shm.so.0 libxcb-sync.so.1 => /lib64/libxcb-sync.so.1 libxcb-xfixes.so.0 => /lib64/libxcb-xfixes.so.0 libxkbcommon.so.0 => /lib64/libxkbcommon.so.0 libxshmfence.so.1 => /lib64/libxshmfence.so.1 Signed-off-by: Gerd Hoffmann Message-id: 20180221131537.31341-12-kra...@redhat.com --- configure| 5 ++--- ui/Makefile.objs | 17 + 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/configure b/configure index ab1ba9c47d..8074fc5001 100755 --- a/configure +++ b/configure @@ -2547,7 +2547,6 @@ if test "$gtk" != "no"; then gtk_cflags="$gtk_cflags $x11_cflags" gtk_libs="$gtk_libs $x11_libs" fi -libs_softmmu="$gtk_libs $libs_softmmu" gtk="yes" elif test "$gtk" = "yes"; then feature_not_found "gtk" "Install gtk3-devel" @@ -2797,7 +2796,6 @@ if test "$vte" != "no"; then vte_cflags=$($pkg_config --cflags $vtepackage) vte_libs=$($pkg_config --libs $vtepackage) vteversion=$($pkg_config --modversion $vtepackage) -libs_softmmu="$vte_libs $libs_softmmu" vte="yes" elif test "$vte" = "yes"; then if test "$gtkabi" = "3.0"; then @@ -6137,7 +6135,7 @@ if test "$glib_subprocess" = "yes" ; then echo "CONFIG_HAS_GLIB_SUBPROCESS_TESTS=y" >> $config_host_mak fi if test "$gtk" = "yes" ; then - echo "CONFIG_GTK=y" >> $config_host_mak + echo "CONFIG_GTK=m" >> $config_host_mak echo "CONFIG_GTKABI=$gtkabi" >> $config_host_mak echo "GTK_CFLAGS=$gtk_cflags" >> $config_host_mak echo "GTK_LIBS=$gtk_libs" >> $config_host_mak @@ -6188,6 +6186,7 @@ fi if test "$vte" = "yes" ; then echo "CONFIG_VTE=y" >> $config_host_mak echo "VTE_CFLAGS=$vte_cflags" >> $config_host_mak + echo "VTE_LIBS=$vte_libs" >> $config_host_mak fi if test "$virglrenderer" = "yes" ; then echo "CONFIG_VIRGL=y" >> $config_host_mak diff --git a/ui/Makefile.objs b/ui/Makefile.objs index 9b725b07aa..49223b0573 100644 --- a/ui/Makefile.objs +++ b/ui/Makefile.objs @@ -16,7 +16,6 @@ common-obj-$(CONFIG_COCOA) += cocoa.o common-obj-$(CONFIG_CURSES) += curses.o common-obj-$(CONFIG_VNC) += $(vnc-obj-y) common-obj-$(call lnot,$(CONFIG_VNC)) += vnc-stubs.o -common-obj-$(CONFIG_GTK) += gtk.o common-obj-$(CONFIG_X11) += x_keymap.o x_keymap.o-cflags := $(X11_CFLAGS) @@ -34,6 +33,12 @@ endif sdl.mo-cflags := $(SDL_CFLAGS) sdl.mo-libs := $(SDL_LIBS) +# ui-gtk module +common-obj-$(CONFIG_GTK) += gtk.mo +gtk.mo-objs := gtk.o +gtk.mo-cflags := $(GTK_CFLAGS) $(VTE_CFLAGS) +gtk.mo-libs := $(GTK_LIBS) $(VTE_LIBS) + ifeq ($(CONFIG_OPENGL),y) common-obj-y += shader.o common-obj-y += console-gl.o @@ -41,17 +46,13 @@ common-obj-y += egl-helpers.o common-obj-y += egl-context.o common-obj-$(CONFIG_OPENGL_DMABUF) += egl-headless.o ifeq ($(CONFIG_GTK_GL),y) -common-obj-$(CONFIG_GTK) += gtk-gl-area.o +gtk.mo-objs += gtk-gl-area.o else -common-obj-$(CONFIG_GTK) += gtk-egl.o +gtk.mo-objs += gtk-egl.o +gtk.mo-libs += $(OPENGL_LIBS) endif endif -gtk.o-cflags := $(GTK_CFLAGS) $(VTE_CFLAGS) -gtk-egl.o-cflags := $(GTK_CFLAGS) $(VTE_CFLAGS) -gtk-gl-area.o-cflags := $(GTK_CFLAGS) $(VTE_CFLAGS) - -gtk-egl.o-libs += $(OPENGL_LIBS) shade
[Qemu-devel] [PULL 07/11] console: add ui module loading support
If a requested user interface is not available, try loading it as
module, simliar to block layer modules. Needed to keep things working
when followup patches start to build user interfaces as modules.
Signed-off-by: Gerd Hoffmann
Message-id: 20180221131537.31341-8-kra...@redhat.com
---
include/qemu/module.h | 1 +
ui/console.c | 6 ++
2 files changed, 7 insertions(+)
diff --git a/include/qemu/module.h b/include/qemu/module.h
index 56dd218205..9fea75aaeb 100644
--- a/include/qemu/module.h
+++ b/include/qemu/module.h
@@ -53,6 +53,7 @@ typedef enum {
#define trace_init(function) module_init(function, MODULE_INIT_TRACE)
#define block_module_load_one(lib) module_load_one("block-", lib)
+#define ui_module_load_one(lib) module_load_one("ui-", lib)
void register_module_init(void (*fn)(void), module_init_type type);
void register_dso_module_init(void (*fn)(void), module_init_type type);
diff --git a/ui/console.c b/ui/console.c
index 25d342cdcb..78efab269a 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -2199,6 +2199,9 @@ bool qemu_display_find_default(DisplayOptions *opts)
for (i = 0; i < ARRAY_SIZE(prio); i++) {
if (dpys[prio[i]] == NULL) {
+ui_module_load_one(DisplayType_lookup.array[prio[i]]);
+}
+if (dpys[prio[i]] == NULL) {
continue;
}
opts->type = prio[i];
@@ -2214,6 +2217,9 @@ void qemu_display_early_init(DisplayOptions *opts)
return;
}
if (dpys[opts->type] == NULL) {
+ui_module_load_one(DisplayType_lookup.array[opts->type]);
+}
+if (dpys[opts->type] == NULL) {
error_report("Display '%s' is not available.",
DisplayType_lookup.array[opts->type]);
exit(1);
--
2.9.3
[Qemu-devel] [PULL 09/11] configure: opengl doesn't depend on x11
So remove x11 from pkg-config check and don't add x11 cflags/libs to opengl cflags/libs. Signed-off-by: Gerd Hoffmann Message-id: 20180221131537.31341-10-kra...@redhat.com --- configure | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/configure b/configure index f6dc1c92b3..ab1ba9c47d 100755 --- a/configure +++ b/configure @@ -3768,9 +3768,9 @@ libs_softmmu="$libs_softmmu $fdt_libs" if test "$opengl" != "no" ; then opengl_pkgs="epoxy libdrm gbm" - if $pkg_config $opengl_pkgs x11; then -opengl_cflags="$($pkg_config --cflags $opengl_pkgs) $x11_cflags" -opengl_libs="$($pkg_config --libs $opengl_pkgs) $x11_libs" + if $pkg_config $opengl_pkgs; then +opengl_cflags="$($pkg_config --cflags $opengl_pkgs)" +opengl_libs="$($pkg_config --libs $opengl_pkgs)" opengl=yes if test "$gtk" = "yes" && $pkg_config --exists "$gtkpackage >= 3.16"; then gtk_gl="yes" -- 2.9.3
Re: [Qemu-devel] [PATCH v4 3/5] qcow2: Reduce REFT_OFFSET_MASK
On Tue 27 Feb 2018 05:29:42 PM CET, Eric Blake wrote: > Match our code to the spec change in the previous patch - there's > no reason for the refcount table to allow larger offsets than the > L1/L2 tables. In practice, no image has more than 64PB of > allocated clusters anyways, as anything beyond that can't be > expressed via L2 mappings to host offsets. > > Suggested-by: Alberto Garcia > Signed-off-by: Eric Blake Reviewed-by: Alberto Garcia Berto
[Qemu-devel] [PULL 00/11] Ui 20180228 patches
The following changes since commit 0a773d55ac76c5aa89ed9187a3bc5af8c5c2a6d0: maintainers: Add myself as a OpenBSD maintainer (2018-02-23 12:05:07 +) are available in the git repository at: git://git.kraxel.org/qemu tags/ui-20180228-pull-request for you to fetch changes up to 6b59af25063d9affb433b4febb8d706a62713680: curses: build as ui module (2018-02-28 07:21:44 +0100) ui: add & use display registry, build some UIs as module. Gerd Hoffmann (11): console: add qemu display registry, add gtk sdl: switch over to new display registry cocoa: switch over to new display registry curses: switch over to new display registry egl-headless: switch over to new display registry console: add and use qemu_display_find_default console: add ui module loading support configure: add X11 vars to config-host.mak configure: opengl doesn't depend on x11 gtk: build as ui module curses: build as ui module configure | 27 +++ include/qemu/module.h | 1 + include/ui/console.h | 75 --- ui/console.c | 59 ui/curses.c | 14 +- ui/egl-headless.c | 20 +- ui/gtk.c | 17 ++-- ui/sdl.c | 24 + ui/sdl2.c | 17 ++-- vl.c | 74 -- ui/Makefile.objs | 28 --- ui/cocoa.m| 14 +- 12 files changed, 204 insertions(+), 166 deletions(-) -- 2.9.3
[Qemu-devel] [PULL 01/11] console: add qemu display registry, add gtk
Add a registry for user interfaces. Add qemu_display_init and
qemu_display_early_init helper functions for display initialization.
Hook up gtk ui as first user.
Signed-off-by: Gerd Hoffmann
Message-id: 20180221131537.31341-2-kra...@redhat.com
---
include/ui/console.h | 32
ui/console.c | 34 ++
ui/gtk.c | 17 +++--
vl.c | 18 ++
4 files changed, 67 insertions(+), 34 deletions(-)
diff --git a/include/ui/console.h b/include/ui/console.h
index f29bacd625..817f9b9bcc 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -432,6 +432,18 @@ void surface_gl_setup_viewport(QemuGLShader *gls,
int ww, int wh);
#endif
+typedef struct QemuDisplay QemuDisplay;
+
+struct QemuDisplay {
+DisplayType type;
+void (*early_init)(DisplayOptions *opts);
+void (*init)(DisplayState *ds, DisplayOptions *opts);
+};
+
+void qemu_display_register(QemuDisplay *ui);
+void qemu_display_early_init(DisplayOptions *opts);
+void qemu_display_init(DisplayState *ds, DisplayOptions *opts);
+
/* sdl.c */
#ifdef CONFIG_SDL
void sdl_display_early_init(DisplayOptions *opts);
@@ -487,26 +499,6 @@ static inline void curses_display_init(DisplayState *ds,
DisplayOptions *opts)
/* input.c */
int index_from_key(const char *key, size_t key_length);
-/* gtk.c */
-#ifdef CONFIG_GTK
-void early_gtk_display_init(DisplayOptions *opts);
-void gtk_display_init(DisplayState *ds, DisplayOptions *opts);
-#else
-static inline void gtk_display_init(DisplayState *ds, DisplayOptions *opts)
-{
-/* This must never be called if CONFIG_GTK is disabled */
-error_report("GTK support is disabled");
-abort();
-}
-
-static inline void early_gtk_display_init(DisplayOptions *opts)
-{
-/* This must never be called if CONFIG_GTK is disabled */
-error_report("GTK support is disabled");
-abort();
-}
-#endif
-
/* egl-headless.c */
void egl_headless_init(DisplayOptions *opts);
diff --git a/ui/console.c b/ui/console.c
index e22931a396..a11b120fc8 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -2180,6 +2180,40 @@ PixelFormat qemu_default_pixelformat(int bpp)
return pf;
}
+static QemuDisplay *dpys[DISPLAY_TYPE__MAX];
+
+void qemu_display_register(QemuDisplay *ui)
+{
+assert(ui->type < DISPLAY_TYPE__MAX);
+dpys[ui->type] = ui;
+}
+
+void qemu_display_early_init(DisplayOptions *opts)
+{
+assert(opts->type < DISPLAY_TYPE__MAX);
+if (opts->type == DISPLAY_TYPE_NONE) {
+return;
+}
+if (dpys[opts->type] == NULL) {
+error_report("Display '%s' is not available.",
+ DisplayType_lookup.array[opts->type]);
+exit(1);
+}
+if (dpys[opts->type]->early_init) {
+dpys[opts->type]->early_init(opts);
+}
+}
+
+void qemu_display_init(DisplayState *ds, DisplayOptions *opts)
+{
+assert(opts->type < DISPLAY_TYPE__MAX);
+if (opts->type == DISPLAY_TYPE_NONE) {
+return;
+}
+assert(dpys[opts->type] != NULL);
+dpys[opts->type]->init(ds, opts);
+}
+
void qemu_chr_parse_vc(QemuOpts *opts, ChardevBackend *backend, Error **errp)
{
int val;
diff --git a/ui/gtk.c b/ui/gtk.c
index ab646b70e1..c63408e036 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -2297,7 +2297,7 @@ static void gd_create_menus(GtkDisplayState *s)
static gboolean gtkinit;
-void gtk_display_init(DisplayState *ds, DisplayOptions *opts)
+static void gtk_display_init(DisplayState *ds, DisplayOptions *opts)
{
VirtualConsole *vc;
@@ -2407,7 +2407,7 @@ void gtk_display_init(DisplayState *ds, DisplayOptions
*opts)
}
}
-void early_gtk_display_init(DisplayOptions *opts)
+static void early_gtk_display_init(DisplayOptions *opts)
{
/* The QEMU code relies on the assumption that it's always run in
* the C locale. Therefore it is not prepared to deal with
@@ -2450,3 +2450,16 @@ void early_gtk_display_init(DisplayOptions *opts)
type_register(&char_gd_vc_type_info);
#endif
}
+
+static QemuDisplay qemu_display_gtk = {
+.type = DISPLAY_TYPE_GTK,
+.early_init = early_gtk_display_init,
+.init = gtk_display_init,
+};
+
+static void register_gtk(void)
+{
+qemu_display_register(&qemu_display_gtk);
+}
+
+type_init(register_gtk);
diff --git a/vl.c b/vl.c
index 9e7235df6d..70458ba708 100644
--- a/vl.c
+++ b/vl.c
@@ -2173,7 +2173,6 @@ static void parse_display(const char *p)
exit(1);
#endif
} else if (strstart(p, "gtk", &opts)) {
-#ifdef CONFIG_GTK
dpy.type = DISPLAY_TYPE_GTK;
while (*opts) {
const char *nextopt;
@@ -2205,10 +2204,6 @@ static void parse_display(const char *p)
}
opts = nextopt;
}
-#else
-error_report("GTK support is disabled");
-exit(1);
-#endif
} else if (strstart(p, "none", &opts)) {
dpy.type = DISPLAY_TYPE_NONE;
} else {
@@ -4318,6 +4313,
Re: [Qemu-devel] [PATCH] i386: Allow monitor / mwait cpuid override
On 2/28/18 7:50 PM, Paolo Bonzini wrote: > On 28/02/2018 00:55, Alexander Graf wrote: >> >> On 27.02.18 10:52, Gonglei (Arei) wrote: >>> Hi all, >>> >>> Guests could achive good performance in 'Message Passing Workloads' >>> scenarios when knowing the X86_FEATURE_MWAIT feature which is presented by >>> qemu. >>> the reason is that after knowing that feature, >>> the guest could use mwait method, which saves VMEXIT, >>> to do idle, and achives high performace in latency-sensitive scenario. >>> >>> Is there any plan for this patch? >>> >>> Or May I send a updated version based on yours? @Alex? >> Oh, did I drop the ball on this one? If that's the case, sure, go ahead. > Hi, it is probably best to implement this feature based on the > HINT_DEDICATED cpuid bit that Wanpeng Li is adding. https://lkml.org/lkml/2018/2/13/624 As you pointed out, MWAIT/HLT/PAUSE non-exiting should be implemented at the same time and I'm still working on it. I will prepare patches for both the qemu and kvm stuffs. :) Regards, Wanpeng Li
Re: [Qemu-devel] [PULL 00/12] Ui 20180227 patches
> > I think modularizing SDL isn't that easy then. > > Can you just drop the "sdl: build as ui module" patch? > > I can't drop patches from signed pull requests -- you need > to respin it. Ah, right, that would invalidate the signature. Pull resent. cheers, Gerd
Re: [Qemu-devel] [PATCH v3 4/4] target/arm: Add arm_gdb_set_sysreg() callback
On 28 February 2018 at 11:01, Abdallah Bouassida wrote: > This is a callback to set the cp-regs registered by the dynamic XML. > > Signed-off-by: Abdallah Bouassida > --- >>> Some of our customers need to connect to Qemu using our tool TRACE32® >>> via GDB, >>> and for some use case they need to have write access to some particular >>> cpregs. >>> So, it will be nice to have this capability! >>> Usually, a user won't modify these registers unless he knows what he is >>> doing! > >> I also still don't really like using write_raw_cp_reg() here -- >> it will bypass some behaviour you want and in some cases will >> just break the emulation because invariants we assume will >> hold no longer hold. It would be a lot lot safer to not >> provide write access at all, only read access. > > Adding to that our customers may need this write access, our tool TRACE32® > needs this also in some particular cases. For example: temporary disabling MMU > to do a physical memory access. By clearing the SCTLR bit? That's a good example of a case that won't work reliably. If you clear the SCTLR.M bit via raw_write this will not perform the tlb_flush() that it needs to, which means that if anything does a memory access via the QEMU TLB it may get the wrong cached results. If you always clear the bit, do one gdb memory access then set the bit then it will probably not run into problems but you're walking on thin ice. thanks -- PMM
Re: [Qemu-devel] [qemu-s390x] [PATCH] hw/s390x: Add the possibility to specify the netboot image on the command line
On 28.02.2018 12:02, David Hildenbrand wrote:
> On 27.02.2018 12:32, Thomas Huth wrote:
>> The file name of the netboot binary is currently hard-coded to
>> "s390-netboot.img", without a possibility for the user to select
>> an alternative firmware image here. That's unfortunate, especially
>> since the basics are already there: The filename is a property of
>> the s390-ipl device. So we just have to add a check whether the user
>> already provided the property and only set the default if the string
>> is still empty. Now it is possible to select a different firmware
>> image with "-global s390-ipl.netboot_fw=/path/to/s390-netboot.img".
>>
>> Signed-off-by: Thomas Huth
>> ---
>> hw/s390x/s390-virtio-ccw.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index 4abbe89..7b3fb5f 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -254,8 +254,10 @@ static void s390_init_ipl_dev(const char
>> *kernel_filename,
>> }
>> qdev_prop_set_string(dev, "cmdline", kernel_cmdline);
>> qdev_prop_set_string(dev, "firmware", firmware);
>> -qdev_prop_set_string(dev, "netboot_fw", netboot_fw);
>> qdev_prop_set_bit(dev, "enforce_bios", enforce_bios);
>> +if (!strlen(object_property_get_str(new, "netboot_fw", &error_abort))) {
>
> Isn't it the case that object_property_get_str() can return also NULL?
>
> (looking at s390_ipl_set_loadparm())
It can return NULL in case of errors (e.g. the property is not a string
or not available at all). In this case, we know that the property is a
string and that it is available, so IMHO no need to check for NULL here.
Not sure why s390_ipl_set_loadparm() explicitely checks for this ...
maybe this was originally required to support the old s390-virtio
(non-ccw) machine, too?
Thomas
[Qemu-devel] [PATCH v6 7/9] vfio/display: core & wireup
Infrastructure for display support. Must be enabled
using 'display' property.
Signed-off-by: Gerd Hoffmann
---
hw/vfio/pci.h | 4
hw/vfio/display.c | 56 +++
hw/vfio/pci.c | 10 +
hw/vfio/Makefile.objs | 2 +-
4 files changed, 71 insertions(+), 1 deletion(-)
create mode 100644 hw/vfio/display.c
diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
index f4aa13e021..a846cf6237 100644
--- a/hw/vfio/pci.h
+++ b/hw/vfio/pci.h
@@ -133,6 +133,7 @@ typedef struct VFIOPCIDevice {
#define VFIO_FEATURE_ENABLE_IGD_OPREGION_BIT 2
#define VFIO_FEATURE_ENABLE_IGD_OPREGION \
(1 << VFIO_FEATURE_ENABLE_IGD_OPREGION_BIT)
+OnOffAuto display;
int32_t bootindex;
uint32_t igd_gms;
OffAutoPCIBAR msix_relo;
@@ -174,4 +175,7 @@ int vfio_pci_igd_opregion_init(VFIOPCIDevice *vdev,
struct vfio_region_info *info,
Error **errp);
+int vfio_display_probe(VFIOPCIDevice *vdev, Error **errp);
+void vfio_display_finalize(VFIOPCIDevice *vdev);
+
#endif /* HW_VFIO_VFIO_PCI_H */
diff --git a/hw/vfio/display.c b/hw/vfio/display.c
new file mode 100644
index 00..3e997f8a44
--- /dev/null
+++ b/hw/vfio/display.c
@@ -0,0 +1,56 @@
+/*
+ * display support for mdev based vgpu devices
+ *
+ * Copyright Red Hat, Inc. 2017
+ *
+ * Authors:
+ *Gerd Hoffmann
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2. See
+ * the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include
+#include
+
+#include "sysemu/sysemu.h"
+#include "ui/console.h"
+#include "qapi/error.h"
+#include "pci.h"
+
+int vfio_display_probe(VFIOPCIDevice *vdev, Error **errp)
+{
+struct vfio_device_gfx_plane_info probe;
+int ret;
+
+memset(&probe, 0, sizeof(probe));
+probe.argsz = sizeof(probe);
+probe.flags = VFIO_GFX_PLANE_TYPE_PROBE | VFIO_GFX_PLANE_TYPE_DMABUF;
+ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_QUERY_GFX_PLANE, &probe);
+if (ret == 0) {
+error_setg(errp, "vfio-display: dmabuf support not implemented yet");
+return -1;
+}
+
+memset(&probe, 0, sizeof(probe));
+probe.argsz = sizeof(probe);
+probe.flags = VFIO_GFX_PLANE_TYPE_PROBE | VFIO_GFX_PLANE_TYPE_REGION;
+ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_QUERY_GFX_PLANE, &probe);
+if (ret == 0) {
+error_setg(errp, "vfio-display: region support not implemented yet");
+return -1;
+}
+
+if (vdev->display == ON_OFF_AUTO_AUTO) {
+/* not an error in automatic mode */
+return 0;
+}
+
+error_setg(errp, "vfio: device doesn't support any (known) display
method");
+return -1;
+}
+
+void vfio_display_finalize(VFIOPCIDevice *vdev)
+{
+}
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 033cc8dea1..20f93bef74 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3015,6 +3015,13 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
}
}
+if (vdev->display != ON_OFF_AUTO_OFF) {
+ret = vfio_display_probe(vdev, errp);
+if (ret) {
+goto out_teardown;
+}
+}
+
vfio_register_err_notifier(vdev);
vfio_register_req_notifier(vdev);
vfio_setup_resetfn_quirk(vdev);
@@ -3035,6 +3042,7 @@ static void vfio_instance_finalize(Object *obj)
VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pci_dev);
VFIOGroup *group = vdev->vbasedev.group;
+vfio_display_finalize(vdev);
vfio_bars_finalize(vdev);
g_free(vdev->emulated_config_bits);
g_free(vdev->rom);
@@ -3123,6 +3131,8 @@ static void vfio_instance_init(Object *obj)
static Property vfio_pci_dev_properties[] = {
DEFINE_PROP_PCI_HOST_DEVADDR("host", VFIOPCIDevice, host),
DEFINE_PROP_STRING("sysfsdev", VFIOPCIDevice, vbasedev.sysfsdev),
+DEFINE_PROP_ON_OFF_AUTO("display", VFIOPCIDevice,
+display, ON_OFF_AUTO_AUTO),
DEFINE_PROP_UINT32("x-intx-mmap-timeout-ms", VFIOPCIDevice,
intx.mmap_timeout, 1100),
DEFINE_PROP_BIT("x-vga", VFIOPCIDevice, features,
diff --git a/hw/vfio/Makefile.objs b/hw/vfio/Makefile.objs
index c3ab9097f1..a2e7a0a7cf 100644
--- a/hw/vfio/Makefile.objs
+++ b/hw/vfio/Makefile.objs
@@ -1,6 +1,6 @@
ifeq ($(CONFIG_LINUX), y)
obj-$(CONFIG_SOFTMMU) += common.o
-obj-$(CONFIG_PCI) += pci.o pci-quirks.o
+obj-$(CONFIG_PCI) += pci.o pci-quirks.o display.o
obj-$(CONFIG_VFIO_CCW) += ccw.o
obj-$(CONFIG_SOFTMMU) += platform.o
obj-$(CONFIG_VFIO_XGMAC) += calxeda-xgmac.o
--
2.9.3
[Qemu-devel] [PATCH v6 3/9] ui/pixman: add qemu_drm_format_to_pixman()
Map drm fourcc codes to pixman formats.
Signed-off-by: Gerd Hoffmann
---
include/ui/qemu-pixman.h | 5 +
ui/qemu-pixman.c | 22 ++
2 files changed, 27 insertions(+)
diff --git a/include/ui/qemu-pixman.h b/include/ui/qemu-pixman.h
index 4a67e01232..b7c82d17fc 100644
--- a/include/ui/qemu-pixman.h
+++ b/include/ui/qemu-pixman.h
@@ -33,6 +33,8 @@
# define PIXMAN_BE_r8g8b8a8 PIXMAN_r8g8b8a8
# define PIXMAN_BE_x8b8g8r8 PIXMAN_x8b8g8r8
# define PIXMAN_BE_a8b8g8r8 PIXMAN_a8b8g8r8
+# define PIXMAN_LE_r8g8b8 PIXMAN_b8g8r8
+# define PIXMAN_LE_a8r8g8b8 PIXMAN_b8g8r8a8
# define PIXMAN_LE_x8r8g8b8 PIXMAN_b8g8r8x8
#else
# define PIXMAN_BE_r8g8b8 PIXMAN_b8g8r8
@@ -44,6 +46,8 @@
# define PIXMAN_BE_r8g8b8a8 PIXMAN_a8b8g8r8
# define PIXMAN_BE_x8b8g8r8 PIXMAN_r8g8b8x8
# define PIXMAN_BE_a8b8g8r8 PIXMAN_r8g8b8a8
+# define PIXMAN_LE_r8g8b8 PIXMAN_r8g8b8
+# define PIXMAN_LE_a8r8g8b8 PIXMAN_a8r8g8b8
# define PIXMAN_LE_x8r8g8b8 PIXMAN_x8r8g8b8
#endif
@@ -51,6 +55,7 @@
PixelFormat qemu_pixelformat_from_pixman(pixman_format_code_t format);
pixman_format_code_t qemu_default_pixman_format(int bpp, bool native_endian);
+pixman_format_code_t qemu_drm_format_to_pixman(uint32_t drm_format);
int qemu_pixman_get_type(int rshift, int gshift, int bshift);
pixman_format_code_t qemu_pixman_get_format(PixelFormat *pf);
bool qemu_pixman_check_format(DisplayChangeListener *dcl,
diff --git a/ui/qemu-pixman.c b/ui/qemu-pixman.c
index 6e591ab821..3e52abd92d 100644
--- a/ui/qemu-pixman.c
+++ b/ui/qemu-pixman.c
@@ -6,6 +6,7 @@
#include "qemu/osdep.h"
#include "qemu-common.h"
#include "ui/console.h"
+#include "standard-headers/drm/drm_fourcc.h"
PixelFormat qemu_pixelformat_from_pixman(pixman_format_code_t format)
{
@@ -88,6 +89,27 @@ pixman_format_code_t qemu_default_pixman_format(int bpp,
bool native_endian)
return 0;
}
+/* Note: drm is little endian, pixman is native endian */
+pixman_format_code_t qemu_drm_format_to_pixman(uint32_t drm_format)
+{
+static const struct {
+uint32_t drm_format;
+pixman_format_code_t pixman;
+} map[] = {
+{ DRM_FORMAT_RGB888, PIXMAN_LE_r8g8b8 },
+{ DRM_FORMAT_ARGB, PIXMAN_LE_a8r8g8b8 },
+{ DRM_FORMAT_XRGB, PIXMAN_LE_x8r8g8b8 }
+};
+int i;
+
+for (i = 0; i < ARRAY_SIZE(map); i++) {
+if (drm_format == map[i].drm_format) {
+return map[i].pixman;
+}
+}
+return 0;
+}
+
int qemu_pixman_get_type(int rshift, int gshift, int bshift)
{
int type = PIXMAN_TYPE_OTHER;
--
2.9.3
Re: [Qemu-devel] [PATCH v2] target-i386: add KVM_HINTS_DEDICATED performance hint
Ping,
2018-02-09 22:15 GMT+08:00 Wanpeng Li :
> From: Wanpeng Li
>
> Add KVM_HINTS_DEDICATED performance hint, guest checks this feature bit
> to determine if they run on dedicated vCPUs, allowing optimizations such
> as usage of qspinlocks.
>
> Cc: Paolo Bonzini
> Cc: Radim Krčmář
> Cc: Eduardo Habkost
> Signed-off-by: Wanpeng Li
> ---
> v1 -> v2:
> * add a new feature word
>
> target/i386/cpu.c | 14 ++
> target/i386/cpu.h | 3 +++
> target/i386/kvm.c | 4
> 3 files changed, 21 insertions(+)
>
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index d70954b..e2974ad 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -358,6 +358,20 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS]
> = {
> .cpuid_eax = KVM_CPUID_FEATURES, .cpuid_reg = R_EAX,
> .tcg_features = TCG_KVM_FEATURES,
> },
> +[FEAT_KVM_HINTS] = {
> +.feat_names = {
> +"hint-dedicated", NULL, NULL, NULL,
> +NULL, NULL, NULL, NULL,
> +NULL, NULL, NULL, NULL,
> +NULL, NULL, NULL, NULL,
> +NULL, NULL, NULL, NULL,
> +NULL, NULL, NULL, NULL,
> +NULL, NULL, NULL, NULL,
> +NULL, NULL, NULL, NULL,
> +},
> +.cpuid_eax = KVM_CPUID_FEATURES, .cpuid_reg = R_EDX,
> +.tcg_features = TCG_KVM_FEATURES,
> +},
> [FEAT_HYPERV_EAX] = {
> .feat_names = {
> NULL /* hv_msr_vp_runtime_access */, NULL /*
> hv_msr_time_refcount_access */,
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index f91e37d..9f73692 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -475,6 +475,7 @@ typedef enum FeatureWord {
> FEAT_8000_0008_EBX, /* CPUID[8000_0008].EBX */
> FEAT_C000_0001_EDX, /* CPUID[C000_0001].EDX */
> FEAT_KVM, /* CPUID[4000_0001].EAX (KVM_CPUID_FEATURES) */
> +FEAT_KVM_HINTS, /* CPUID[4000_0001].EDX */
> FEAT_HYPERV_EAX,/* CPUID[4000_0003].EAX */
> FEAT_HYPERV_EBX,/* CPUID[4000_0003].EBX */
> FEAT_HYPERV_EDX,/* CPUID[4000_0003].EDX */
> @@ -670,6 +671,8 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS];
> #define CPUID_7_0_EDX_AVX512_4FMAPS (1U << 3) /* AVX512 Multiply
> Accumulation Single Precision */
> #define CPUID_7_0_EDX_SPEC_CTRL (1U << 26) /* Speculation Control */
>
> +#define KVM_HINTS_DEDICATED (1U << 0)
> +
> #define CPUID_8000_0008_EBX_IBPB(1U << 12) /* Indirect Branch Prediction
> Barrier */
>
> #define CPUID_XSAVE_XSAVEOPT (1U << 0)
> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> index ad4b159..44ee524 100644
> --- a/target/i386/kvm.c
> +++ b/target/i386/kvm.c
> @@ -383,6 +383,9 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s,
> uint32_t function,
> if (!kvm_irqchip_in_kernel()) {
> ret &= ~(1U << KVM_FEATURE_PV_UNHALT);
> }
> +} else if (function == KVM_CPUID_FEATURES && reg == R_EDX) {
> +ret |= KVM_HINTS_DEDICATED;
> +found = 1;
> }
>
> /* fallback for older kernels */
> @@ -801,6 +804,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
> c = &cpuid_data.entries[cpuid_i++];
> c->function = KVM_CPUID_FEATURES | kvm_base;
> c->eax = env->features[FEAT_KVM];
> +c->edx = env->features[FEAT_KVM_HINTS];
> }
>
> cpu_x86_cpuid(env, 0, 0, &limit, &unused, &unused, &unused);
> --
> 2.7.4
>
[Qemu-devel] [PATCH v6 6/9] vfio/common: cleanup in vfio_region_finalize
Signed-off-by: Gerd Hoffmann --- hw/vfio/common.c | 7 +++ 1 file changed, 7 insertions(+) diff --git a/hw/vfio/common.c b/hw/vfio/common.c index f895e3c335..6a8203a532 100644 --- a/hw/vfio/common.c +++ b/hw/vfio/common.c @@ -858,6 +858,13 @@ void vfio_region_finalize(VFIORegion *region) g_free(region->mmaps); trace_vfio_region_finalize(region->vbasedev->name, region->nr); + +region->mem = NULL; +region->mmaps = NULL; +region->nr_mmaps = 0; +region->size = 0; +region->flags = 0; +region->nr = 0; } void vfio_region_mmaps_set_enabled(VFIORegion *region, bool enabled) -- 2.9.3
[Qemu-devel] [PATCH v6 8/9] vfio/display: adding region support
Wire up region-based display.
Signed-off-by: Gerd Hoffmann
---
hw/vfio/pci.h | 1 +
include/hw/vfio/vfio-common.h | 8 +++
hw/vfio/display.c | 116 +-
3 files changed, 123 insertions(+), 2 deletions(-)
diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
index a846cf6237..629c875701 100644
--- a/hw/vfio/pci.h
+++ b/hw/vfio/pci.h
@@ -148,6 +148,7 @@ typedef struct VFIOPCIDevice {
bool no_kvm_msi;
bool no_kvm_msix;
bool no_geforce_quirks;
+VFIODisplay *dpy;
} VFIOPCIDevice;
uint32_t vfio_pci_read_config(PCIDevice *pdev, uint32_t addr, int len);
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index f3a2ac9fee..fc8ae14fb7 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -142,6 +142,14 @@ typedef struct VFIOGroup {
QLIST_ENTRY(VFIOGroup) container_next;
} VFIOGroup;
+typedef struct VFIODisplay {
+QemuConsole *con;
+struct {
+VFIORegion buffer;
+DisplaySurface *surface;
+} region;
+} VFIODisplay;
+
void vfio_put_base_device(VFIODevice *vbasedev);
void vfio_disable_irqindex(VFIODevice *vbasedev, int index);
void vfio_unmask_single_irqindex(VFIODevice *vbasedev, int index);
diff --git a/hw/vfio/display.c b/hw/vfio/display.c
index 3e997f8a44..f6acbacc79 100644
--- a/hw/vfio/display.c
+++ b/hw/vfio/display.c
@@ -19,6 +19,113 @@
#include "qapi/error.h"
#include "pci.h"
+/* -- */
+
+static void vfio_display_region_update(void *opaque)
+{
+VFIOPCIDevice *vdev = opaque;
+VFIODisplay *dpy = vdev->dpy;
+struct vfio_device_gfx_plane_info plane = {
+.argsz = sizeof(plane),
+.flags = VFIO_GFX_PLANE_TYPE_REGION
+};
+pixman_format_code_t format;
+int ret;
+
+ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_QUERY_GFX_PLANE, &plane);
+if (ret < 0) {
+error_report("ioctl VFIO_DEVICE_QUERY_GFX_PLANE: %s",
+ strerror(errno));
+return;
+}
+if (!plane.drm_format || !plane.size) {
+return;
+}
+format = qemu_drm_format_to_pixman(plane.drm_format);
+if (!format) {
+return;
+}
+
+if (dpy->region.buffer.size &&
+dpy->region.buffer.nr != plane.region_index) {
+/* region changed */
+vfio_region_exit(&dpy->region.buffer);
+vfio_region_finalize(&dpy->region.buffer);
+dpy->region.surface = NULL;
+}
+
+if (dpy->region.surface &&
+(surface_width(dpy->region.surface) != plane.width ||
+ surface_height(dpy->region.surface) != plane.height ||
+ surface_format(dpy->region.surface) != format)) {
+/* size changed */
+dpy->region.surface = NULL;
+}
+
+if (!dpy->region.buffer.size) {
+/* mmap region */
+ret = vfio_region_setup(OBJECT(vdev), &vdev->vbasedev,
+&dpy->region.buffer,
+plane.region_index,
+"display");
+if (ret != 0) {
+error_report("%s: vfio_region_setup(%d): %s",
+ __func__, plane.region_index, strerror(-ret));
+goto err;
+}
+ret = vfio_region_mmap(&dpy->region.buffer);
+if (ret != 0) {
+error_report("%s: vfio_region_mmap(%d): %s", __func__,
+ plane.region_index, strerror(-ret));
+goto err;
+}
+assert(dpy->region.buffer.mmaps[0].mmap != NULL);
+}
+
+if (dpy->region.surface == NULL) {
+/* create surface */
+dpy->region.surface = qemu_create_displaysurface_from
+(plane.width, plane.height, format,
+ plane.stride, dpy->region.buffer.mmaps[0].mmap);
+dpy_gfx_replace_surface(dpy->con, dpy->region.surface);
+}
+
+/* full screen update */
+dpy_gfx_update(dpy->con, 0, 0,
+ surface_width(dpy->region.surface),
+ surface_height(dpy->region.surface));
+return;
+
+err:
+vfio_region_exit(&dpy->region.buffer);
+vfio_region_finalize(&dpy->region.buffer);
+}
+
+static const GraphicHwOps vfio_display_region_ops = {
+.gfx_update = vfio_display_region_update,
+};
+
+static int vfio_display_region_init(VFIOPCIDevice *vdev, Error **errp)
+{
+vdev->dpy = g_new0(VFIODisplay, 1);
+vdev->dpy->con = graphic_console_init(DEVICE(vdev), 0,
+ &vfio_display_region_ops,
+ vdev);
+return 0;
+}
+
+static void vfio_display_region_exit(VFIODisplay *dpy)
+{
+if (!dpy->region.buffer.size) {
+return;
+}
+
+vfio_region_exit(&dpy->region.buffer);
+vfio_region_finalize(&dpy->region.buffer);
+}
+
+/* -- */
+
int vfio_display_probe(VFIOPCIDevice *vdev, Error **errp
[Qemu-devel] [PATCH v6 4/9] console: minimal hotplug suport
This patch allows to unbind devices from QemuConsoles, using the new
graphic_console_close() function. The QemuConsole will show a static
display then, saying the device was unplugged. When re-plugging a
display later on the QemuConsole will be reused.
Eventually we will allocate and release QemuConsoles dynamically at some
point in the future, that'll need more infrastructure though to notify
user interfaces (gtk, sdl, spice, ...) about QemuConsoles coming and
going.
Signed-off-by: Gerd Hoffmann
---
include/ui/console.h | 2 ++
ui/console.c | 78
ui/trace-events | 2 ++
3 files changed, 76 insertions(+), 6 deletions(-)
diff --git a/include/ui/console.h b/include/ui/console.h
index f29bacd625..adca7954e5 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -383,6 +383,7 @@ QemuConsole *graphic_console_init(DeviceState *dev,
uint32_t head,
void graphic_console_set_hwops(QemuConsole *con,
const GraphicHwOps *hw_ops,
void *opaque);
+void graphic_console_close(QemuConsole *con);
void graphic_hw_update(QemuConsole *con);
void graphic_hw_invalidate(QemuConsole *con);
@@ -395,6 +396,7 @@ QemuConsole *qemu_console_lookup_by_index(unsigned int
index);
QemuConsole *qemu_console_lookup_by_device(DeviceState *dev, uint32_t head);
QemuConsole *qemu_console_lookup_by_device_name(const char *device_id,
uint32_t head, Error **errp);
+QemuConsole *qemu_console_lookup_unused(void);
bool qemu_console_is_visible(QemuConsole *con);
bool qemu_console_is_graphic(QemuConsole *con);
bool qemu_console_is_fixedsize(QemuConsole *con);
diff --git a/ui/console.c b/ui/console.c
index e22931a396..3b0e6cd6e1 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1266,11 +1266,16 @@ static QemuConsole *new_console(DisplayState *ds,
console_type_t console_type,
s->console_type = console_type;
consoles = g_realloc(consoles, sizeof(*consoles) * (nb_consoles+1));
-if (console_type != GRAPHIC_CONSOLE) {
+if (console_type != GRAPHIC_CONSOLE || qdev_hotplug) {
s->index = nb_consoles;
consoles[nb_consoles++] = s;
} else {
-/* HACK: Put graphical consoles before text consoles. */
+/*
+ * HACK: Put graphical consoles before text consoles.
+ *
+ * Only do that for coldplugged devices. After initial device
+ * initialization we will not renumber the consoles any more.
+ */
for (i = nb_consoles; i > 0; i--) {
if (consoles[i - 1]->console_type == GRAPHIC_CONSOLE)
break;
@@ -1858,21 +1863,60 @@ QemuConsole *graphic_console_init(DeviceState *dev,
uint32_t head,
int height = 480;
QemuConsole *s;
DisplayState *ds;
+DisplaySurface *surface;
ds = get_alloc_displaystate();
-trace_console_gfx_new();
-s = new_console(ds, GRAPHIC_CONSOLE, head);
-s->ui_timer = timer_new_ms(QEMU_CLOCK_REALTIME, dpy_set_ui_info_timer, s);
+s = qemu_console_lookup_unused();
+if (s) {
+trace_console_gfx_reuse(s->index);
+if (s->surface) {
+width = surface_width(s->surface);
+height = surface_height(s->surface);
+}
+} else {
+trace_console_gfx_new();
+s = new_console(ds, GRAPHIC_CONSOLE, head);
+s->ui_timer = timer_new_ms(QEMU_CLOCK_REALTIME, dpy_set_ui_info_timer,
s);
+}
graphic_console_set_hwops(s, hw_ops, opaque);
if (dev) {
object_property_set_link(OBJECT(s), OBJECT(dev), "device",
&error_abort);
}
-s->surface = qemu_create_message_surface(width, height, noinit);
+surface = qemu_create_message_surface(width, height, noinit);
+dpy_gfx_replace_surface(s, surface);
return s;
}
+static const GraphicHwOps unused_ops = {
+/* no callbacks */
+};
+
+void graphic_console_close(QemuConsole *con)
+{
+static const char unplugged[] =
+"Guest display has been unplugged";
+DisplaySurface *surface;
+int width = 640;
+int height = 480;
+
+if (con->surface) {
+width = surface_width(con->surface);
+height = surface_height(con->surface);
+}
+
+trace_console_gfx_close(con->index);
+object_property_set_link(OBJECT(con), NULL, "device", &error_abort);
+graphic_console_set_hwops(con, &unused_ops, NULL);
+
+if (con->gl) {
+dpy_gl_scanout_disable(con);
+}
+surface = qemu_create_message_surface(width, height, unplugged);
+dpy_gfx_replace_surface(con, surface);
+}
+
QemuConsole *qemu_console_lookup_by_index(unsigned int index)
{
if (index >= nb_consoles) {
@@ -1929,6 +1973,28 @@ QemuConsole *qemu_console_lookup_by_device_name(const
char *device_id,
return con;
}
+QemuConsole *qemu_console_lookup_unused(void)
+{
+Object *obj;
+int i;
+
+for (i = 0; i <
[Qemu-devel] [PATCH v6 9/9] vfio/display: adding dmabuf support
Wire up dmabuf-based display.
Signed-off-by: Gerd Hoffmann
---
include/hw/vfio/vfio-common.h | 14
hw/vfio/display.c | 166 +-
2 files changed, 178 insertions(+), 2 deletions(-)
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index fc8ae14fb7..994e780d51 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -26,6 +26,7 @@
#include "exec/memory.h"
#include "qemu/queue.h"
#include "qemu/notify.h"
+#include "ui/console.h"
#ifdef CONFIG_LINUX
#include
#endif
@@ -142,12 +143,25 @@ typedef struct VFIOGroup {
QLIST_ENTRY(VFIOGroup) container_next;
} VFIOGroup;
+typedef struct VFIODMABuf {
+QemuDmaBuf buf;
+uint32_t pos_x, pos_y;
+uint32_t hot_x, hot_y;
+int dmabuf_id;
+QTAILQ_ENTRY(VFIODMABuf) next;
+} VFIODMABuf;
+
typedef struct VFIODisplay {
QemuConsole *con;
struct {
VFIORegion buffer;
DisplaySurface *surface;
} region;
+struct {
+QTAILQ_HEAD(, VFIODMABuf) bufs;
+VFIODMABuf *primary;
+VFIODMABuf *cursor;
+} dmabuf;
} VFIODisplay;
void vfio_put_base_device(VFIODevice *vbasedev);
diff --git a/hw/vfio/display.c b/hw/vfio/display.c
index f6acbacc79..053d3ab67a 100644
--- a/hw/vfio/display.c
+++ b/hw/vfio/display.c
@@ -19,6 +19,168 @@
#include "qapi/error.h"
#include "pci.h"
+#ifndef DRM_PLANE_TYPE_PRIMARY
+# define DRM_PLANE_TYPE_PRIMARY 1
+# define DRM_PLANE_TYPE_CURSOR 2
+#endif
+
+static VFIODMABuf *vfio_display_get_dmabuf(VFIOPCIDevice *vdev,
+ uint32_t plane_type)
+{
+VFIODisplay *dpy = vdev->dpy;
+struct vfio_device_gfx_plane_info plane;
+VFIODMABuf *dmabuf;
+int fd, ret;
+
+memset(&plane, 0, sizeof(plane));
+plane.argsz = sizeof(plane);
+plane.flags = VFIO_GFX_PLANE_TYPE_DMABUF;
+plane.drm_plane_type = plane_type;
+ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_QUERY_GFX_PLANE, &plane);
+if (ret < 0) {
+return NULL;
+}
+if (!plane.drm_format || !plane.size) {
+return NULL;
+}
+
+QTAILQ_FOREACH(dmabuf, &dpy->dmabuf.bufs, next) {
+if (dmabuf->dmabuf_id == plane.dmabuf_id) {
+/* found in list, move to head, return it */
+QTAILQ_REMOVE(&dpy->dmabuf.bufs, dmabuf, next);
+QTAILQ_INSERT_HEAD(&dpy->dmabuf.bufs, dmabuf, next);
+if (plane_type == DRM_PLANE_TYPE_CURSOR) {
+dmabuf->pos_x = plane.x_pos;
+dmabuf->pos_y = plane.y_pos;
+}
+return dmabuf;
+}
+}
+
+fd = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_GET_GFX_DMABUF,
&plane.dmabuf_id);
+if (fd < 0) {
+return NULL;
+}
+
+dmabuf = g_new0(VFIODMABuf, 1);
+dmabuf->dmabuf_id = plane.dmabuf_id;
+dmabuf->buf.width = plane.width;
+dmabuf->buf.height = plane.height;
+dmabuf->buf.stride = plane.stride;
+dmabuf->buf.fourcc = plane.drm_format;
+dmabuf->buf.fd = fd;
+if (plane_type == DRM_PLANE_TYPE_CURSOR) {
+dmabuf->pos_x = plane.x_pos;
+dmabuf->pos_y = plane.y_pos;
+dmabuf->hot_x = plane.x_hot;
+dmabuf->hot_y = plane.y_hot;
+}
+
+QTAILQ_INSERT_HEAD(&dpy->dmabuf.bufs, dmabuf, next);
+return dmabuf;
+}
+
+static void vfio_display_free_one_dmabuf(VFIODisplay *dpy, VFIODMABuf *dmabuf)
+{
+QTAILQ_REMOVE(&dpy->dmabuf.bufs, dmabuf, next);
+dpy_gl_release_dmabuf(dpy->con, &dmabuf->buf);
+close(dmabuf->buf.fd);
+g_free(dmabuf);
+}
+
+static void vfio_display_free_dmabufs(VFIOPCIDevice *vdev)
+{
+VFIODisplay *dpy = vdev->dpy;
+VFIODMABuf *dmabuf, *tmp;
+uint32_t keep = 5;
+
+QTAILQ_FOREACH_SAFE(dmabuf, &dpy->dmabuf.bufs, next, tmp) {
+if (keep > 0) {
+keep--;
+continue;
+}
+assert(dmabuf != dpy->dmabuf.primary);
+vfio_display_free_one_dmabuf(dpy, dmabuf);
+}
+}
+
+static void vfio_display_dmabuf_update(void *opaque)
+{
+VFIOPCIDevice *vdev = opaque;
+VFIODisplay *dpy = vdev->dpy;
+VFIODMABuf *primary, *cursor;
+bool free_bufs = false;
+
+primary = vfio_display_get_dmabuf(vdev, DRM_PLANE_TYPE_PRIMARY);
+if (primary == NULL) {
+return;
+}
+
+if (dpy->dmabuf.primary != primary) {
+dpy->dmabuf.primary = primary;
+qemu_console_resize(dpy->con,
+primary->buf.width, primary->buf.height);
+dpy_gl_scanout_dmabuf(dpy->con, &primary->buf);
+free_bufs = true;
+}
+
+cursor = vfio_display_get_dmabuf(vdev, DRM_PLANE_TYPE_CURSOR);
+if (dpy->dmabuf.cursor != cursor) {
+dpy->dmabuf.cursor = cursor;
+if (cursor) {
+bool have_hot = (cursor->hot_x != 0x &&
+ cursor->hot_y != 0x);
+dpy_gl_cursor_dmabuf(dpy->con, &cursor->buf, have_hot,
+
[Qemu-devel] [PATCH v6 2/9] standard-headers: add drm/drm_fourcc.h
So we can use the drm fourcc codes without a dependency on libdrm-devel.
Signed-off-by: Gerd Hoffmann
---
include/standard-headers/drm/drm_fourcc.h | 411 ++
scripts/update-linux-headers.sh | 4 +
2 files changed, 415 insertions(+)
create mode 100644 include/standard-headers/drm/drm_fourcc.h
diff --git a/include/standard-headers/drm/drm_fourcc.h
b/include/standard-headers/drm/drm_fourcc.h
new file mode 100644
index 00..11912fde24
--- /dev/null
+++ b/include/standard-headers/drm/drm_fourcc.h
@@ -0,0 +1,411 @@
+/*
+ * Copyright 2011 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * VA LINUX SYSTEMS AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+#ifndef DRM_FOURCC_H
+#define DRM_FOURCC_H
+
+
+#if defined(__cplusplus)
+extern "C" {
+#endif
+
+#define fourcc_code(a, b, c, d) ((uint32_t)(a) | ((uint32_t)(b) << 8) | \
+((uint32_t)(c) << 16) | ((uint32_t)(d) << 24))
+
+#define DRM_FORMAT_BIG_ENDIAN (1<<31) /* format is big endian instead of
little endian */
+
+/* color index */
+#define DRM_FORMAT_C8 fourcc_code('C', '8', ' ', ' ') /* [7:0] C */
+
+/* 8 bpp Red */
+#define DRM_FORMAT_R8 fourcc_code('R', '8', ' ', ' ') /* [7:0] R */
+
+/* 16 bpp Red */
+#define DRM_FORMAT_R16 fourcc_code('R', '1', '6', ' ') /* [15:0] R
little endian */
+
+/* 16 bpp RG */
+#define DRM_FORMAT_RG88fourcc_code('R', 'G', '8', '8') /*
[15:0] R:G 8:8 little endian */
+#define DRM_FORMAT_GR88fourcc_code('G', 'R', '8', '8') /*
[15:0] G:R 8:8 little endian */
+
+/* 32 bpp RG */
+#define DRM_FORMAT_RG1616 fourcc_code('R', 'G', '3', '2') /* [31:0] R:G
16:16 little endian */
+#define DRM_FORMAT_GR1616 fourcc_code('G', 'R', '3', '2') /* [31:0] G:R
16:16 little endian */
+
+/* 8 bpp RGB */
+#define DRM_FORMAT_RGB332 fourcc_code('R', 'G', 'B', '8') /* [7:0] R:G:B
3:3:2 */
+#define DRM_FORMAT_BGR233 fourcc_code('B', 'G', 'R', '8') /* [7:0] B:G:R
2:3:3 */
+
+/* 16 bpp RGB */
+#define DRM_FORMAT_XRGBfourcc_code('X', 'R', '1', '2') /* [15:0]
x:R:G:B 4:4:4:4 little endian */
+#define DRM_FORMAT_XBGRfourcc_code('X', 'B', '1', '2') /* [15:0]
x:B:G:R 4:4:4:4 little endian */
+#define DRM_FORMAT_RGBXfourcc_code('R', 'X', '1', '2') /* [15:0]
R:G:B:x 4:4:4:4 little endian */
+#define DRM_FORMAT_BGRXfourcc_code('B', 'X', '1', '2') /* [15:0]
B:G:R:x 4:4:4:4 little endian */
+
+#define DRM_FORMAT_ARGBfourcc_code('A', 'R', '1', '2') /* [15:0]
A:R:G:B 4:4:4:4 little endian */
+#define DRM_FORMAT_ABGRfourcc_code('A', 'B', '1', '2') /* [15:0]
A:B:G:R 4:4:4:4 little endian */
+#define DRM_FORMAT_RGBAfourcc_code('R', 'A', '1', '2') /* [15:0]
R:G:B:A 4:4:4:4 little endian */
+#define DRM_FORMAT_BGRAfourcc_code('B', 'A', '1', '2') /* [15:0]
B:G:R:A 4:4:4:4 little endian */
+
+#define DRM_FORMAT_XRGB1555fourcc_code('X', 'R', '1', '5') /* [15:0]
x:R:G:B 1:5:5:5 little endian */
+#define DRM_FORMAT_XBGR1555fourcc_code('X', 'B', '1', '5') /* [15:0]
x:B:G:R 1:5:5:5 little endian */
+#define DRM_FORMAT_RGBX5551fourcc_code('R', 'X', '1', '5') /* [15:0]
R:G:B:x 5:5:5:1 little endian */
+#define DRM_FORMAT_BGRX5551fourcc_code('B', 'X', '1', '5') /* [15:0]
B:G:R:x 5:5:5:1 little endian */
+
+#define DRM_FORMAT_ARGB1555fourcc_code('A', 'R', '1', '5') /* [15:0]
A:R:G:B 1:5:5:5 little endian */
+#define DRM_FORMAT_ABGR1555fourcc_code('A', 'B', '1', '5') /* [15:0]
A:B:G:R 1:5:5:5 little endian */
+#define DRM_FORMAT_RGBA5551fourcc_code('R', 'A', '1', '5') /* [15:0]
R:G:B:A 5:5:5:1 little endian */
+#define DRM_FORMAT_BGRA5551fourcc_code('B', 'A', '1', '5') /* [15:0]
B:G:R:A 5:5:5:1 little endian */
+
+#define DRM_FORMAT_RGB565 fourcc_code('R', 'G', '1', '6') /* [15:0] R:G:B
5:6:5 little endian */
+#define DRM_FORMAT_BGR565 fourcc_code('B', 'G', '1', '6') /* [15:0] B:G
[Qemu-devel] [PATCH v6 1/9] linux-headers: update to 4.16-rc1
s390 has splitted syscall numbers into unistd_{32,64}.h files,
so update scripts/update-linux-headers.sh accordingly.
Also add a rewrite from __BITS_PER_LONG to HOST_LONG_BITS for
linux/input.h
Signed-off-by: Gerd Hoffmann
---
include/standard-headers/linux/input-event-codes.h | 1 +
include/standard-headers/linux/input.h | 11 +
include/standard-headers/linux/pci_regs.h | 30 +-
include/standard-headers/linux/virtio_net.h| 13 +
linux-headers/asm-powerpc/kvm.h| 2 +
linux-headers/asm-powerpc/unistd.h | 3 +
linux-headers/asm-s390/unistd.h| 401 +
linux-headers/asm-s390/unistd_32.h | 364 +++
linux-headers/asm-s390/unistd_64.h | 331 +
linux-headers/asm-x86/kvm_para.h | 4 +
linux-headers/linux/kvm.h | 90 +
linux-headers/linux/psci.h | 3 +
linux-headers/linux/vfio.h | 72
scripts/update-linux-headers.sh| 3 +
14 files changed, 918 insertions(+), 410 deletions(-)
create mode 100644 linux-headers/asm-s390/unistd_32.h
create mode 100644 linux-headers/asm-s390/unistd_64.h
diff --git a/include/standard-headers/linux/input-event-codes.h
b/include/standard-headers/linux/input-event-codes.h
index 79841b543f..9e6a8ba4ce 100644
--- a/include/standard-headers/linux/input-event-codes.h
+++ b/include/standard-headers/linux/input-event-codes.h
@@ -594,6 +594,7 @@
#define BTN_DPAD_RIGHT 0x223
#define KEY_ALS_TOGGLE 0x230 /* Ambient light sensor */
+#define KEY_ROTATE_LOCK_TOGGLE 0x231 /* Display rotation lock */
#define KEY_BUTTONCONFIG 0x240 /* AL Button Configuration */
#define KEY_TASKMANAGER0x241 /* AL Task/Project Manager */
diff --git a/include/standard-headers/linux/input.h
b/include/standard-headers/linux/input.h
index bc3e6d3d5b..939b62775c 100644
--- a/include/standard-headers/linux/input.h
+++ b/include/standard-headers/linux/input.h
@@ -18,10 +18,21 @@
/*
* The event structure itself
+ * Note that __USE_TIME_BITS64 is defined by libc based on
+ * application's request to use 64 bit time_t.
*/
struct input_event {
+#if (HOST_LONG_BITS != 32 || !defined(__USE_TIME_BITS64)) && !defined(__KERNEL)
struct timeval time;
+#define input_event_sec time.tv_sec
+#define input_event_usec time.tv_usec
+#else
+ __kernel_ulong_t __sec;
+ __kernel_ulong_t __usec;
+#define input_event_sec __sec
+#define input_event_usec __usec
+#endif
uint16_t type;
uint16_t code;
int32_t value;
diff --git a/include/standard-headers/linux/pci_regs.h
b/include/standard-headers/linux/pci_regs.h
index 70c2b2ade0..0c79eac5e9 100644
--- a/include/standard-headers/linux/pci_regs.h
+++ b/include/standard-headers/linux/pci_regs.h
@@ -622,15 +622,19 @@
* safely.
*/
#define PCI_EXP_DEVCAP236 /* Device Capabilities 2 */
+#define PCI_EXP_DEVCAP2_COMP_TMOUT_DIS0x0010 /* Completion
Timeout Disable supported */
#define PCI_EXP_DEVCAP2_ARI 0x0020 /* Alternative Routing-ID */
#define PCI_EXP_DEVCAP2_ATOMIC_ROUTE 0x0040 /* Atomic Op routing */
-#define PCI_EXP_DEVCAP2_ATOMIC_COMP64 0x0100 /* Atomic 64-bit compare */
+#define PCI_EXP_DEVCAP2_ATOMIC_COMP32 0x0080 /* 32b AtomicOp completion */
+#define PCI_EXP_DEVCAP2_ATOMIC_COMP64 0x0100 /* 64b AtomicOp completion */
+#define PCI_EXP_DEVCAP2_ATOMIC_COMP1280x0200 /* 128b AtomicOp
completion */
#define PCI_EXP_DEVCAP2_LTR 0x0800 /* Latency tolerance
reporting */
#define PCI_EXP_DEVCAP2_OBFF_MASK 0x000c /* OBFF support mechanism */
#define PCI_EXP_DEVCAP2_OBFF_MSG 0x0004 /* New message signaling */
#define PCI_EXP_DEVCAP2_OBFF_WAKE 0x0008 /* Re-use WAKE# for OBFF */
#define PCI_EXP_DEVCTL240 /* Device Control 2 */
#define PCI_EXP_DEVCTL2_COMP_TIMEOUT 0x000f /* Completion Timeout Value */
+#define PCI_EXP_DEVCTL2_COMP_TMOUT_DIS0x0010 /* Completion Timeout
Disable */
#define PCI_EXP_DEVCTL2_ARI 0x0020 /* Alternative Routing-ID */
#define PCI_EXP_DEVCTL2_ATOMIC_REQ 0x0040 /* Set Atomic requests */
#define PCI_EXP_DEVCTL2_ATOMIC_EGRESS_BLOCK 0x0080 /* Block atomic egress */
@@ -966,26 +970,28 @@
/* Downstream Port Containment */
#define PCI_EXP_DPC_CAP4 /* DPC Capability */
-#define PCI_EXP_DPC_IRQ0x1f/* DPC Interrupt
Message Number */
-#define PCI_EXP_DPC_CAP_RP_EXT0x20/* Root Port Extensions
for DPC */
-#define PCI_EXP_DPC_CAP_POISONED_TLP 0x40/* Poisoned TLP Egress Blocking
Supported */
-#define PCI_EXP_DPC_CAP_SW_TRIGGER0x80/* Software Triggering
Supported */
-#define PCI_EXP_DPC_RP_PIO_LOG_SIZE
[Qemu-devel] [PATCH v6 0/9] vfio: add display support
This series adds support for a vgpu display to the qemu vfio code. v6: - add support for hotplugging QemuConsoles. - drop vfio-pci-display device, re-add OnOffAuto display property. - add proper cleanup in finalize. v5: - rebase to latest master - drop DeviceState->hotpluggable patch, use separate vfio-pci-display device instead so we can use DeviceClass->hotpluggable. - add vfio dma-buf patch. Right now this can be tested with '-display egl-headless' only. gtk and spice support is almost ready for merge and should follow soon. cheers, Gerd Gerd Hoffmann (9): linux-headers: update to 4.16-rc1 standard-headers: add drm/drm_fourcc.h ui/pixman: add qemu_drm_format_to_pixman() console: minimal hotplug suport secondary-vga: properly close QemuConsole on unplug vfio/common: cleanup in vfio_region_finalize vfio/display: core & wireup vfio/display: adding region support vfio/display: adding dmabuf support hw/vfio/pci.h | 5 + include/hw/vfio/vfio-common.h | 22 ++ include/standard-headers/drm/drm_fourcc.h | 411 + include/standard-headers/linux/input-event-codes.h | 1 + include/standard-headers/linux/input.h | 11 + include/standard-headers/linux/pci_regs.h | 30 +- include/standard-headers/linux/virtio_net.h| 13 + include/ui/console.h | 2 + include/ui/qemu-pixman.h | 5 + linux-headers/asm-powerpc/kvm.h| 2 + linux-headers/asm-powerpc/unistd.h | 3 + linux-headers/asm-s390/unistd.h| 401 +--- linux-headers/asm-s390/unistd_32.h | 364 ++ linux-headers/asm-s390/unistd_64.h | 331 + linux-headers/asm-x86/kvm_para.h | 4 + linux-headers/linux/kvm.h | 90 + linux-headers/linux/psci.h | 3 + linux-headers/linux/vfio.h | 72 hw/display/vga-pci.c | 9 + hw/vfio/common.c | 7 + hw/vfio/display.c | 330 + hw/vfio/pci.c | 10 + ui/console.c | 78 +++- ui/qemu-pixman.c | 22 ++ hw/vfio/Makefile.objs | 2 +- scripts/update-linux-headers.sh| 7 + ui/trace-events| 2 + 27 files changed, 1820 insertions(+), 417 deletions(-) create mode 100644 include/standard-headers/drm/drm_fourcc.h create mode 100644 linux-headers/asm-s390/unistd_32.h create mode 100644 linux-headers/asm-s390/unistd_64.h create mode 100644 hw/vfio/display.c -- 2.9.3
[Qemu-devel] [PATCH v6 5/9] secondary-vga: properly close QemuConsole on unplug
Using the new graphic_console_close() function.
Signed-off-by: Gerd Hoffmann
---
hw/display/vga-pci.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/hw/display/vga-pci.c b/hw/display/vga-pci.c
index 1674bd3581..f312930664 100644
--- a/hw/display/vga-pci.c
+++ b/hw/display/vga-pci.c
@@ -292,6 +292,14 @@ static void pci_secondary_vga_realize(PCIDevice *dev,
Error **errp)
pci_register_bar(&d->dev, 2, PCI_BASE_ADDRESS_SPACE_MEMORY, &d->mmio);
}
+static void pci_secondary_vga_exit(PCIDevice *dev)
+{
+PCIVGAState *d = PCI_VGA(dev);
+VGACommonState *s = &d->vga;
+
+graphic_console_close(s->con);
+}
+
static void pci_secondary_vga_init(Object *obj)
{
/* Expose framebuffer byteorder via QOM */
@@ -361,6 +369,7 @@ static void secondary_class_init(ObjectClass *klass, void
*data)
PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
k->realize = pci_secondary_vga_realize;
+k->exit = pci_secondary_vga_exit;
k->class_id = PCI_CLASS_DISPLAY_OTHER;
dc->props = secondary_pci_properties;
dc->reset = pci_secondary_vga_reset;
--
2.9.3
Re: [Qemu-devel] Deprecate tilegx ?
On 02/28/2018 07:11 AM, Thomas Huth wrote: > On 27.02.2018 12:51, Peter Maydell wrote: >> I propose that we deprecate and plan to remove the unicore32 code: > [...] [...] > > Sounds reasonable to me, but let's wait a week or two for feedback from > Guan Xuetao. Agreed. > >> Possibly there are other target architectures we could reasonably >> deprecate-and-remove (though none of the other ones Linux is dropping >> in this round are ones we support)... > > I'd vote for marking tilegx as deprecated, too, since we even do not > have an active maintainer for that CPU core (at least I did not spot one > in our MAINTAINERS file). Opinions? I always saw it as a big plus that QEMU supports nearly any architecture, no matter how obscure it is. So I'm a bit more hesitant on dropping architectures quickly. Cheers, Bastian
Re: [Qemu-devel] [PATCH 0/5] Versatile Express SiI9022 emulation
On Tue, Feb 27, 2018 at 6:26 PM, Peter Maydell wrote: > On 27 February 2018 at 10:48, Linus Walleij wrote: >> This series adds proper display bridge/connector emulation >> for the Versatile Express, implementing a simple Silicon >> Image 9022 emulation spawning a DDC I2C child. >> >> After the series the Versatile Express is successfully >> presented the "QEMU monitor" through DDC I2C. >> >> The series includes two refactorings from Corey and a >> minor bug fix for the i2c-ddc so that everything is smoothly >> integrated. >> >> Corey Minyard (2): >> i2c: Fix some brace style issues >> i2c: Move the bus class to i2c.h >> >> Linus Walleij (3): >> hw/i2c-ddc: Do not fail writes >> hw/sii9022: Add support for Silicon Image SII9022 >> arm/vexpress: Add proper display connector emulation > > Hi; I've applied this to target-arm.next with the tweak to > the commit message of patch 5 and the reset function in patch 4. > Let me know if you think those are wrong or you'd prefer to > respin the series yourself. I am certain you did it better than I could, thanks a lot. I forgot about the reset business, sorry :( I will test it once it hits the trunk! Yours, Linus Walleij
Re: [Qemu-devel] [PATCH 03/14] qio: introduce qio_channel_add_watch_full()
On Wed, Feb 28, 2018 at 09:08:45AM +, Daniel P. Berrangé wrote: > On Wed, Feb 28, 2018 at 01:06:22PM +0800, Peter Xu wrote: > > It's a more powerful version of qio_channel_add_watch(), which supports > > non-default gcontext. It's stripped from the old one, then we have > > g_source_get_id() to fetch the tag ID to keep the old interface. > > > > Note that the new API will return a gsource, meanwhile keep a reference > > of it so that callers need to unref them explicitly. > > I don't really like this. Having qio_channel_add_watch and > qio_channel_add_watch_full with differing return values is > really very surprising. They should be functionally identical, > except for the extra context arg. Yeah it's not nice, but I do need the GSource and the tag ID does not help in the series. An alternative would be that I modify qio_channel_add_watch() to return GSource too. Is there an third choice that you could suggest? Thanks, -- Peter Xu
Re: [Qemu-devel] [PATCH 03/14] qio: introduce qio_channel_add_watch_full()
On Wed, Feb 28, 2018 at 08:44:24PM +0800, Peter Xu wrote: > On Wed, Feb 28, 2018 at 09:08:45AM +, Daniel P. Berrangé wrote: > > On Wed, Feb 28, 2018 at 01:06:22PM +0800, Peter Xu wrote: > > > It's a more powerful version of qio_channel_add_watch(), which supports > > > non-default gcontext. It's stripped from the old one, then we have > > > g_source_get_id() to fetch the tag ID to keep the old interface. > > > > > > Note that the new API will return a gsource, meanwhile keep a reference > > > of it so that callers need to unref them explicitly. > > > > I don't really like this. Having qio_channel_add_watch and > > qio_channel_add_watch_full with differing return values is > > really very surprising. They should be functionally identical, > > except for the extra context arg. > > Yeah it's not nice, but I do need the GSource and the tag ID does not > help in the series. > > An alternative would be that I modify qio_channel_add_watch() to > return GSource too. Is there an third choice that you could suggest? Given you have the id + GMainContext you can just acquire the GSource, if needed, using g_main_context_find_source_by_id. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH v6 0/9] vfio: add display support
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20180228123110.6507-1-kra...@redhat.com
Subject: [Qemu-devel] [PATCH v6 0/9] vfio: add display support
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
* [new tag] patchew/20180228123110.6507-1-kra...@redhat.com ->
patchew/20180228123110.6507-1-kra...@redhat.com
Switched to a new branch 'test'
2add6e2941 vfio/display: adding dmabuf support
ccdf41d292 vfio/display: adding region support
7e70abdfa2 vfio/display: core & wireup
9cdbeaf2bb vfio/common: cleanup in vfio_region_finalize
fb7b8d32f4 secondary-vga: properly close QemuConsole on unplug
75167c77d3 console: minimal hotplug suport
c7f1ec9989 ui/pixman: add qemu_drm_format_to_pixman()
3967270521 standard-headers: add drm/drm_fourcc.h
49efb2b198 linux-headers: update to 4.16-rc1
=== OUTPUT BEGIN ===
Checking PATCH 1/9: linux-headers: update to 4.16-rc1...
Checking PATCH 2/9: standard-headers: add drm/drm_fourcc.h...
WARNING: line over 80 characters
#453: FILE: scripts/update-linux-headers.sh:158:
+cp_portable "$tmpdir/include/drm/drm_fourcc.h"
"$output/include/standard-headers/drm"
total: 0 errors, 1 warnings, 433 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 3/9: ui/pixman: add qemu_drm_format_to_pixman()...
Checking PATCH 4/9: console: minimal hotplug suport...
WARNING: line over 80 characters
#82: FILE: ui/console.c:1879:
+s->ui_timer = timer_new_ms(QEMU_CLOCK_REALTIME, dpy_set_ui_info_timer,
s);
total: 0 errors, 1 warnings, 132 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 5/9: secondary-vga: properly close QemuConsole on unplug...
Checking PATCH 6/9: vfio/common: cleanup in vfio_region_finalize...
Checking PATCH 7/9: vfio/display: core & wireup...
Checking PATCH 8/9: vfio/display: adding region support...
ERROR: braces {} are necessary for all arms of this statement
#143: FILE: hw/vfio/display.c:162:
+if (!vdev->dpy)
[...]
total: 1 errors, 0 warnings, 153 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 9/9: vfio/display: adding dmabuf support...
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-de...@freelists.org
Re: [Qemu-devel] [PATCH 07/14] qio/chardev: update net listener gcontext
On Wed, Feb 28, 2018 at 09:25:11AM +, Daniel P. Berrangé wrote:
> On Wed, Feb 28, 2018 at 01:06:26PM +0800, Peter Xu wrote:
> > TCP chardevs can be using QIO network listeners working in the
> > background when in listening mode. However the network listeners are
> > always running in main context. This can race with chardevs that are
> > running in non-main contexts.
> >
> > To solve this: firstly introduce qio_net_listener_set_context() to allow
> > caller to set gcontext for network listeners. Then call it in
> > tcp_chr_update_read_handler(), with the newly cached gcontext.
> >
> > It's fairly straightforward after we have introduced some net listener
> > helper functions - basically we unregister the GSources and add them
> > back with the correct context.
> >
> > Signed-off-by: Peter Xu
> > ---
> > chardev/char-socket.c | 9 +
> > include/io/net-listener.h | 12
> > io/net-listener.c | 7 +++
> > 3 files changed, 28 insertions(+)
> >
> > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > index 43a2cc2c1c..8f0935cd15 100644
> > --- a/chardev/char-socket.c
> > +++ b/chardev/char-socket.c
> > @@ -559,6 +559,15 @@ static void tcp_chr_update_read_handler(Chardev *chr)
> > {
> > SocketChardev *s = SOCKET_CHARDEV(chr);
> >
> > +if (s->listener) {
> > +/*
> > + * It's possible that chardev context is changed in
> > + * qemu_chr_be_update_read_handlers(). Reset it for QIO net
> > + * listener if there is.
> > + */
> > +qio_net_listener_set_context(s->listener, chr->gcontext);
> > +}
> > +
> > if (!s->connected) {
> > return;
> > }
> > diff --git a/include/io/net-listener.h b/include/io/net-listener.h
> > index 566be283b3..39dede9d6f 100644
> > --- a/include/io/net-listener.h
> > +++ b/include/io/net-listener.h
> > @@ -106,6 +106,18 @@ int qio_net_listener_open_sync(QIONetListener
> > *listener,
> > SocketAddress *addr,
> > Error **errp);
> >
> > +/**
> > + * qio_net_listener_set_context:
> > + * @listener: the net listener object
> > + * @context: the context that we'd like to bind the sources to
> > + *
> > + * This helper does not do anything but moves existing net listener
> > + * sources from the old one to the new one. It can be seen as a
> > + * no-operation if there is no listening source at all.
> > + */
> > +void qio_net_listener_set_context(QIONetListener *listener,
> > + GMainContext *context);
>
> I don't think this is a good design. The GMainContext should be provided
> to the qio_net_listener_set_client_func method immediately, not updated
> after the fact
After the patches, this is qio_net_listener_set_client_func_full():
void qio_net_listener_set_client_func_full(QIONetListener *listener,
QIONetListenerClientFunc func,
gpointer data,
GDestroyNotify notify,
GMainContext *context)
{
if (listener->io_notify) {
listener->io_notify(listener->io_data);
}
listener->io_func = func;
listener->io_data = data;
listener->io_notify = notify;
qio_net_listener_sources_clear(listener);
qio_net_listener_sources_update(listener, context);
}
This is qio_net_listener_set_context():
void qio_net_listener_set_context(QIONetListener *listener,
GMainContext *context)
{
qio_net_listener_sources_clear(listener);
qio_net_listener_sources_update(listener, context);
}
So... Basically qio_net_listener_set_context() is just a simplified
version of qio_net_listener_set_client_func_full(), but without
passing in the funcs again. So do you mean that I can just avoid
introducing this function and call
qio_net_listener_set_client_func_full() directly?
Thanks,
--
Peter Xu
Re: [Qemu-devel] [PATCH 10/14] qio: refcount QIOTask
On Wed, Feb 28, 2018 at 09:16:59AM +, Daniel P. Berrangé wrote: > On Wed, Feb 28, 2018 at 01:06:29PM +0800, Peter Xu wrote: > > It will be used in multiple threads in follow-up patches. Let it start > > to have refcounts. > > > > Signed-off-by: Peter Xu > > --- > > include/io/task.h | 3 +++ > > io/task.c | 23 ++- > > 2 files changed, 25 insertions(+), 1 deletion(-) > > > > diff --git a/include/io/task.h b/include/io/task.h > > index 9dbe3758d7..c6acd6489c 100644 > > --- a/include/io/task.h > > +++ b/include/io/task.h > > @@ -322,4 +322,7 @@ gpointer qio_task_get_result_pointer(QIOTask *task); > > */ > > Object *qio_task_get_source(QIOTask *task); > > > > +void qio_task_ref(QIOTask *task); > > +void qio_task_unref(QIOTask *task); > > It should just be turned back into a QObject as it was originally, > so we get refcounting for free. Could you point me the commit that has the original code? I would be glad to revert to that, or yeah I can switch to QObject too. Thanks, -- Peter Xu
Re: [Qemu-devel] [PATCH 03/14] qio: introduce qio_channel_add_watch_full()
On Wed, Feb 28, 2018 at 12:47:43PM +, Daniel P. Berrangé wrote: > On Wed, Feb 28, 2018 at 08:44:24PM +0800, Peter Xu wrote: > > On Wed, Feb 28, 2018 at 09:08:45AM +, Daniel P. Berrangé wrote: > > > On Wed, Feb 28, 2018 at 01:06:22PM +0800, Peter Xu wrote: > > > > It's a more powerful version of qio_channel_add_watch(), which supports > > > > non-default gcontext. It's stripped from the old one, then we have > > > > g_source_get_id() to fetch the tag ID to keep the old interface. > > > > > > > > Note that the new API will return a gsource, meanwhile keep a reference > > > > of it so that callers need to unref them explicitly. > > > > > > I don't really like this. Having qio_channel_add_watch and > > > qio_channel_add_watch_full with differing return values is > > > really very surprising. They should be functionally identical, > > > except for the extra context arg. > > > > Yeah it's not nice, but I do need the GSource and the tag ID does not > > help in the series. > > > > An alternative would be that I modify qio_channel_add_watch() to > > return GSource too. Is there an third choice that you could suggest? > > Given you have the id + GMainContext you can just acquire the GSource, > if needed, using g_main_context_find_source_by_id. I always feel unsafe to play around with tag IDs since the IDs can change after GSource removed and new GSource added, and also the result of the call will depend on a correct pairing of context (so if the context is incorrect, instead of failure, we possibly got everything screwed up while we never know we failed...). But indeed for this one it seems pretty safe if I call g_main_context_find_source_by_id() right after I call qio_channel_add_watch_full() to fetch the GSource. If you agree, I can use this approach in my next post. Thanks, -- Peter Xu
Re: [Qemu-devel] [PATCH 13/14] qio: allow threaded qiotask to switch contexts
On Wed, Feb 28, 2018 at 09:23:56AM +, Daniel P. Berrangé wrote: > On Wed, Feb 28, 2018 at 01:06:32PM +0800, Peter Xu wrote: > > This is the part of work to allow the QIOTask to use a different > > gcontext rather than the default main gcontext, by providing > > qio_task_context_set() API. > > > > We have done some work before on doing similar things to add non-default > > gcontext support. The general idea is that we delete the old GSource > > from the main context, then re-add a new one to the new context when > > context changed to a non-default one. However this trick won't work > > easily for threaded QIOTasks since we can't easily stop a real thread > > and re-setup the whole thing from the very beginning. > > I think this entire usage pattern is really broken. We should not > provide a way to change the GMainContext on an existing task. We > should always just set the correct GMainContxt right from the start. > This will avoid much of the complexity you're introducing into this > patch series, and avoid having to expose GTasks to the callers of > the async methods at all. Yeah I agree with you that the threaded QIO patches are complicated. Then how about I introduce: - qio_task_thread_new(): to create QIO task and its thread (but not running) - qio_task_thread_run(): to run the thread Then I can setup the context correctly between the two in some way. After that, qio_task_run_in_thread() will be two calls of above two APIs. Thanks, -- Peter Xu
Re: [Qemu-devel] [PATCH 07/14] qio/chardev: update net listener gcontext
On Wed, Feb 28, 2018 at 08:52:16PM +0800, Peter Xu wrote:
> On Wed, Feb 28, 2018 at 09:25:11AM +, Daniel P. Berrangé wrote:
> > On Wed, Feb 28, 2018 at 01:06:26PM +0800, Peter Xu wrote:
> > > TCP chardevs can be using QIO network listeners working in the
> > > background when in listening mode. However the network listeners are
> > > always running in main context. This can race with chardevs that are
> > > running in non-main contexts.
> > >
> > > To solve this: firstly introduce qio_net_listener_set_context() to allow
> > > caller to set gcontext for network listeners. Then call it in
> > > tcp_chr_update_read_handler(), with the newly cached gcontext.
> > >
> > > It's fairly straightforward after we have introduced some net listener
> > > helper functions - basically we unregister the GSources and add them
> > > back with the correct context.
> > >
> > > Signed-off-by: Peter Xu
> > > ---
> > > chardev/char-socket.c | 9 +
> > > include/io/net-listener.h | 12
> > > io/net-listener.c | 7 +++
> > > 3 files changed, 28 insertions(+)
> > >
> > > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > > index 43a2cc2c1c..8f0935cd15 100644
> > > --- a/chardev/char-socket.c
> > > +++ b/chardev/char-socket.c
> > > @@ -559,6 +559,15 @@ static void tcp_chr_update_read_handler(Chardev *chr)
> > > {
> > > SocketChardev *s = SOCKET_CHARDEV(chr);
> > >
> > > +if (s->listener) {
> > > +/*
> > > + * It's possible that chardev context is changed in
> > > + * qemu_chr_be_update_read_handlers(). Reset it for QIO net
> > > + * listener if there is.
> > > + */
> > > +qio_net_listener_set_context(s->listener, chr->gcontext);
> > > +}
> > > +
> > > if (!s->connected) {
> > > return;
> > > }
> > > diff --git a/include/io/net-listener.h b/include/io/net-listener.h
> > > index 566be283b3..39dede9d6f 100644
> > > --- a/include/io/net-listener.h
> > > +++ b/include/io/net-listener.h
> > > @@ -106,6 +106,18 @@ int qio_net_listener_open_sync(QIONetListener
> > > *listener,
> > > SocketAddress *addr,
> > > Error **errp);
> > >
> > > +/**
> > > + * qio_net_listener_set_context:
> > > + * @listener: the net listener object
> > > + * @context: the context that we'd like to bind the sources to
> > > + *
> > > + * This helper does not do anything but moves existing net listener
> > > + * sources from the old one to the new one. It can be seen as a
> > > + * no-operation if there is no listening source at all.
> > > + */
> > > +void qio_net_listener_set_context(QIONetListener *listener,
> > > + GMainContext *context);
> >
> > I don't think this is a good design. The GMainContext should be provided
> > to the qio_net_listener_set_client_func method immediately, not updated
> > after the fact
>
> After the patches, this is qio_net_listener_set_client_func_full():
>
> void qio_net_listener_set_client_func_full(QIONetListener *listener,
>QIONetListenerClientFunc func,
>gpointer data,
>GDestroyNotify notify,
>GMainContext *context)
> {
> if (listener->io_notify) {
> listener->io_notify(listener->io_data);
> }
> listener->io_func = func;
> listener->io_data = data;
> listener->io_notify = notify;
>
> qio_net_listener_sources_clear(listener);
> qio_net_listener_sources_update(listener, context);
> }
>
> This is qio_net_listener_set_context():
>
> void qio_net_listener_set_context(QIONetListener *listener,
> GMainContext *context)
> {
> qio_net_listener_sources_clear(listener);
> qio_net_listener_sources_update(listener, context);
> }
>
> So... Basically qio_net_listener_set_context() is just a simplified
> version of qio_net_listener_set_client_func_full(), but without
> passing in the funcs again. So do you mean that I can just avoid
> introducing this function and call
> qio_net_listener_set_client_func_full() directly?
Yes, I don't see any reason to allow GMainContext to be changed separately
from setting the callback functions. The caller can easily just call
qio_net_listener_set_client_func_full() directly with the new GMainContext
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH 11/14] qio/chardev: return QIOTask when connect async
On Wed, Feb 28, 2018 at 09:20:47AM +, Daniel P. Berrangé wrote: > On Wed, Feb 28, 2018 at 01:06:30PM +0800, Peter Xu wrote: > > Let qio_channel_socket_connect_async() return the created QIOTask object > > for the async connection. In tcp chardev, cache that in SocketChardev > > for further use. With the QIOTask refcount, this is pretty safe. > > Why do you want to return QIOTask ? This is going against the intended > design pattern for QIOTask (that was based on that in GLib). The task > supposed to be an internal use only helper that callers should never > be touching until the completion callback is invoked. I proposed another solution in the other comment reply to split the threaded QIO task into create() and run(). If you like that, I can try. Any other suggestion would be welcomed too. Thanks, -- Peter Xu
Re: [Qemu-devel] [PATCH 10/14] qio: refcount QIOTask
On Wed, Feb 28, 2018 at 08:54:00PM +0800, Peter Xu wrote: > On Wed, Feb 28, 2018 at 09:16:59AM +, Daniel P. Berrangé wrote: > > On Wed, Feb 28, 2018 at 01:06:29PM +0800, Peter Xu wrote: > > > It will be used in multiple threads in follow-up patches. Let it start > > > to have refcounts. > > > > > > Signed-off-by: Peter Xu > > > --- > > > include/io/task.h | 3 +++ > > > io/task.c | 23 ++- > > > 2 files changed, 25 insertions(+), 1 deletion(-) > > > > > > diff --git a/include/io/task.h b/include/io/task.h > > > index 9dbe3758d7..c6acd6489c 100644 > > > --- a/include/io/task.h > > > +++ b/include/io/task.h > > > @@ -322,4 +322,7 @@ gpointer qio_task_get_result_pointer(QIOTask *task); > > > */ > > > Object *qio_task_get_source(QIOTask *task); > > > > > > +void qio_task_ref(QIOTask *task); > > > +void qio_task_unref(QIOTask *task); > > > > It should just be turned back into a QObject as it was originally, > > so we get refcounting for free. > > Could you point me the commit that has the original code? I would be > glad to revert to that, or yeah I can switch to QObject too. Thanks, It was never actually committed - it was just that way during initial review but was suggested to be simplified as ref counting wasn't needed. That all said, having now seen the later parts of this patch series, I don't think we would need todo this at all, because we should not be exposing the GTask to callers and thus the refcounting question goes away Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [Nbd] [PATCH] Further tidy-up on block status
Hi, Sorry, I forgot to reply to this earlier. On Fri, Feb 16, 2018 at 10:10:59AM -0600, Eric Blake wrote: > On 02/16/2018 07:53 AM, Vladimir Sementsov-Ogievskiy wrote: > > Good idea. But it would be tricky thing to maintain backward > > compatibility with published versions of virtuozzo product. And finally > > our implementation would be more complex because of this simplification. > > > Hm. Finally, you suggested several changes (including already merged > > 56c77720 :( ). Suggestions are logical. But if they will be accepted, we > > (Virtuozzo) will have to invent tricky hard-to-maintain code, to > > distinguish by third factors our already published versions. Hrm. I wasn't aware you'd already published those versions; if you had told us, we could've reviewed the spec at that point and either merge it or update it to incorporate whatever changes you think seem like they are necessary. Note that the section "Experimental extensions" contains the following wording: In addition to the normative elements of the specification set out herein, various experimental non-normative extensions have been proposed. These may not be implemented in any known server or client, and are subject to change at any point. A full implementation may require changes to the specifications, or cause the specifications to be withdrawn altogether. [...] Implementors of these extensions are strongly suggested to contact the mailinglist in order to help fine-tune the specifications before committing to a particular implementation. i.e., what's in an "extension-" branch can be described as "we're thinking about it and it will probably happen as written, but we're not entirely sure yet". The idea behind this is that we don't want to limit ourselves to things that haven't been implemented (but that the fact of "implementing something" causes it to get written into stone, sortof). This is different from how most standards bodies work, I'm sure, but it seemed to work so far, and I wish you had let us know that the work you were doing was about to be reaching customers. Anyway. [...] > Also, the Virtuozzo product hasn't been mentioned on the NBD list, and while > you are preparing patches to the public qemu based on the Virtuozzo product, > I'm not sure if there is a public repository for the existing Virtuozzo > product out in the wild. Is your product using NBD_OPT_LIST_META_CONTEXT, > or _just_ NBD_OPT_SET_META_CONTEXT? +1. What's published currently? [...] > Or, we can revert the change in commit 56c77720, and keep > NBD_REPLY_TYPE_BLOCK_STATUS at 5 (it leaves a hole in the NBD_REPLY_TYPE > numbering, where 3 and 4 might be filled in by other future extensions, or > permanently skipped). This works IF there are no OTHER incompatible changes > made to the rest of the block status extension as part of promoting it to > current (where we still haven't finished that debate, given my question on > whether 32-bit lengths and colon-separated namespace:leaf in a single string > is the best representation). > > So, I'd like some feedback from Alex or Wouter on which alternatives seem > nicest at this point. I'm thinking that reverting at least the number change seems like a good idea. If Vladimir can shed some light on what's been published so far, we can see what damage has been done and try to limit further damage. It makes no sense to ignore that the spec has been implemented; the whole point of writing a spec is so that third parties can implement it and be compatible. If we ignore that just because there was a misunderstanding, then I think we're throwing away the kid with the bathwater. Since I don't think a gap in numbers is that much of a problem, I'm happy reverting the renumbering part of 56c77720 and keep it at 5. I'd also like to know what it is exactly that Virtuozzo implemented, so we can update the extension-blockstatus (if necessary) and then merge that to master. However, for future reference, Vladimir, I would prefer it if you could give us a heads-up if you're getting close to releasing something to the public that's still in an experimental branch, so that we can make updates if necessary and merge it to master. Thanks, -- Could you people please use IRC like normal people?!? -- Amaya Rodrigo Sastre, trying to quiet down the buzz in the DebConf 2008 Hacklab
[Qemu-devel] [PATCH 2/2] iotests: Test preallocated truncate of 2G image
Signed-off-by: Max Reitz
---
tests/qemu-iotests/106 | 24
tests/qemu-iotests/106.out | 10 ++
2 files changed, 34 insertions(+)
diff --git a/tests/qemu-iotests/106 b/tests/qemu-iotests/106
index bfe71f4e60..5e51f88a78 100755
--- a/tests/qemu-iotests/106
+++ b/tests/qemu-iotests/106
@@ -86,6 +86,30 @@ for growth_mode in falloc full off; do
$QEMU_IMG resize -f "$IMGFMT" --shrink --preallocation=$growth_mode
"$TEST_IMG" -${GROWTH_SIZE}K
done
+echo
+echo '=== Testing image growth on 2G empty image ==='
+
+for growth_mode in falloc full; do
+echo
+echo "--- growth_mode=$growth_mode ---"
+
+# Maybe we want to do an lseek() to the end of the file before the
+# preallocation; if the file has a length of 2 GB, that would
+# return an integer that overflows to negative when put into a
+# plain int. We should use the correct type for the result, and
+# this tests we do.
+
+_make_test_img 2G
+$QEMU_IMG resize -f "$IMGFMT" --preallocation=$growth_mode "$TEST_IMG"
+${GROWTH_SIZE}K
+
+actual_size=$($QEMU_IMG info -f "$IMGFMT" "$TEST_IMG" | grep 'disk size')
+actual_size=$(echo "$actual_size" | sed -e 's/^[^0-9]*\([0-9]\+\).*$/\1/')
+
+if [ $actual_size -lt $GROWTH_SIZE ]; then
+echo "ERROR: Image should have at least ${GROWTH_SIZE}K, but has
${actual_size}K"
+fi
+done
+
# success, all done
echo '*** done'
rm -f $seq.full
diff --git a/tests/qemu-iotests/106.out b/tests/qemu-iotests/106.out
index 0a42312301..c459957660 100644
--- a/tests/qemu-iotests/106.out
+++ b/tests/qemu-iotests/106.out
@@ -47,4 +47,14 @@ qemu-img: Preallocation can only be used for growing images
--- growth_mode=off ---
Image resized.
+
+=== Testing image growth on 2G empty image ===
+
+--- growth_mode=falloc ---
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2147483648
+Image resized.
+
+--- growth_mode=full ---
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2147483648
+Image resized.
*** done
--
2.14.3
[Qemu-devel] [PATCH 0/2] block/file-posix: Fix fully preallocated truncate
Fully preallocated truncation has a 50 % chance of working on images files over file-posix. It works if $SIZE % 4G < 2G, and it fails otherwise. To make things even more interesting, often you would not even notice because qemu reported success even though it did nothing (because after the successful lseek(), errno was still 0, so when the file-posix driver tried to return a negative error code, it actually reported success). This issue is fixed by patch 1 in this series. Thanks to Daniel for reporting! Max Reitz (2): block/file-posix: Fix fully preallocated truncate iotests: Test preallocated truncate of 2G image block/file-posix.c | 5 +++-- tests/qemu-iotests/106 | 24 tests/qemu-iotests/106.out | 10 ++ 3 files changed, 37 insertions(+), 2 deletions(-) -- 2.14.3
[Qemu-devel] [PATCH 1/2] block/file-posix: Fix fully preallocated truncate
Storing the lseek() result in an int results in it overflowing when the
file is at least 2 GB big. Then, we have a 50 % chance of the result
being "negative" and thus thinking an error occurred when actually
everything went just fine.
So we should use the correct type for storing the result: off_t.
Reported-by: Daniel P. Berrange
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1549231
Cc: qemu-sta...@nongnu.org
Signed-off-by: Max Reitz
---
block/file-posix.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/block/file-posix.c b/block/file-posix.c
index f1591c3849..90c25864a0 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1697,6 +1697,7 @@ static int raw_regular_truncate(int fd, int64_t offset,
PreallocMode prealloc,
case PREALLOC_MODE_FULL:
{
int64_t num = 0, left = offset - current_length;
+off_t seek_result;
/*
* Knowing the final size from the beginning could allow the file
@@ -1711,8 +1712,8 @@ static int raw_regular_truncate(int fd, int64_t offset,
PreallocMode prealloc,
buf = g_malloc0(65536);
-result = lseek(fd, current_length, SEEK_SET);
-if (result < 0) {
+seek_result = lseek(fd, current_length, SEEK_SET);
+if (seek_result < 0) {
result = -errno;
error_setg_errno(errp, -result,
"Failed to seek to the old end of file");
--
2.14.3
Re: [Qemu-devel] [PATCH 10/14] qio: refcount QIOTask
On Wed, Feb 28, 2018 at 01:07:44PM +, Daniel P. Berrangé wrote: > On Wed, Feb 28, 2018 at 08:54:00PM +0800, Peter Xu wrote: > > On Wed, Feb 28, 2018 at 09:16:59AM +, Daniel P. Berrangé wrote: > > > On Wed, Feb 28, 2018 at 01:06:29PM +0800, Peter Xu wrote: > > > > It will be used in multiple threads in follow-up patches. Let it start > > > > to have refcounts. > > > > > > > > Signed-off-by: Peter Xu > > > > --- > > > > include/io/task.h | 3 +++ > > > > io/task.c | 23 ++- > > > > 2 files changed, 25 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/include/io/task.h b/include/io/task.h > > > > index 9dbe3758d7..c6acd6489c 100644 > > > > --- a/include/io/task.h > > > > +++ b/include/io/task.h > > > > @@ -322,4 +322,7 @@ gpointer qio_task_get_result_pointer(QIOTask *task); > > > > */ > > > > Object *qio_task_get_source(QIOTask *task); > > > > > > > > +void qio_task_ref(QIOTask *task); > > > > +void qio_task_unref(QIOTask *task); > > > > > > It should just be turned back into a QObject as it was originally, > > > so we get refcounting for free. > > > > Could you point me the commit that has the original code? I would be > > glad to revert to that, or yeah I can switch to QObject too. Thanks, > > It was never actually committed - it was just that way during initial > review but was suggested to be simplified as ref counting wasn't needed. > > That all said, having now seen the later parts of this patch series, I > don't think we would need todo this at all, because we should not be > exposing the GTask to callers and thus the refcounting question goes away Indeed. I suppose that means you like my proposal in the other thread. But I'll wait for your explicit acknowledgement in that too. Thanks, -- Peter Xu
Re: [Qemu-devel] [PATCH 13/14] qio: allow threaded qiotask to switch contexts
On Wed, Feb 28, 2018 at 09:05:26PM +0800, Peter Xu wrote: > On Wed, Feb 28, 2018 at 09:23:56AM +, Daniel P. Berrangé wrote: > > On Wed, Feb 28, 2018 at 01:06:32PM +0800, Peter Xu wrote: > > > This is the part of work to allow the QIOTask to use a different > > > gcontext rather than the default main gcontext, by providing > > > qio_task_context_set() API. > > > > > > We have done some work before on doing similar things to add non-default > > > gcontext support. The general idea is that we delete the old GSource > > > from the main context, then re-add a new one to the new context when > > > context changed to a non-default one. However this trick won't work > > > easily for threaded QIOTasks since we can't easily stop a real thread > > > and re-setup the whole thing from the very beginning. > > > > I think this entire usage pattern is really broken. We should not > > provide a way to change the GMainContext on an existing task. We > > should always just set the correct GMainContxt right from the start. > > This will avoid much of the complexity you're introducing into this > > patch series, and avoid having to expose GTasks to the callers of > > the async methods at all. > > Yeah I agree with you that the threaded QIO patches are complicated. > Then how about I introduce: > > - qio_task_thread_new(): to create QIO task and its thread (but not > running) > - qio_task_thread_run(): to run the thread > > Then I can setup the context correctly between the two in some way. > > After that, qio_task_run_in_thread() will be two calls of above two > APIs. I don't see why it is not enough to just pass the right GMainContext into qio_task_run_in_thread() immediately. There should not be any need to split this into two parts. ALl that's needed here is for the completion callback to rnu in the right context, which just means passing the GMainContext from qio_task_run_in_thread, through to qio_task_thread_worker via QIOTaskThreadData. Changing the GMainContext after the thread is already created/running is not something we should try todo. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH 14/14] qio/chardev: specify gcontext for TLS handshake
On Wed, Feb 28, 2018 at 01:06:33PM +0800, Peter Xu wrote:
> We allow the TLS code to be run with non-default gcontext by providing a
> new qio_channel_tls_handshake_full() API.
>
> With the new API, we can re-setup the TLS handshake GSource by calling
> it again with the correct gcontext. Any call to the function will clean
> up existing GSource tasks, and re-setup using the new gcontext.
>
> Signed-off-by: Peter Xu
> ---
> chardev/char-socket.c| 30 +---
> include/io/channel-tls.h | 22 +++-
> io/channel-tls.c | 91
>
> 3 files changed, 123 insertions(+), 20 deletions(-)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 164a64ff34..406d33c04f 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -72,6 +72,9 @@ typedef struct {
>
> static gboolean socket_reconnect_timeout(gpointer opaque);
> static void tcp_chr_telnet_init(Chardev *chr);
> +static void tcp_chr_tls_handshake_setup(Chardev *chr,
> +QIOChannelTLS *tioc,
> +GMainContext *context);
>
> static void tcp_chr_reconn_timer_cancel(SocketChardev *s)
> {
> @@ -570,6 +573,7 @@ static void tcp_chr_telnet_destroy(SocketChardev *s)
> static void tcp_chr_update_read_handler(Chardev *chr)
> {
> SocketChardev *s = SOCKET_CHARDEV(chr);
> +QIOChannelTLS *tioc;
>
> if (s->listener) {
> /*
> @@ -589,6 +593,17 @@ static void tcp_chr_update_read_handler(Chardev *chr)
> qio_task_context_set(s->thread_task, chr->gcontext);
> }
>
> +tioc = (QIOChannelTLS *)object_dynamic_cast(OBJECT(s->ioc),
> +TYPE_QIO_CHANNEL_TLS);
> +if (tioc) {
> +/*
> + * TLS session enabled; reconfigure things up. Note that, if
> + * there is existing handshake task, it'll be cleaned up first
> + * in QIO code.
> + */
> +tcp_chr_tls_handshake_setup(chr, tioc, chr->gcontext);
> +}
This is crazy - we should not be looking at specific implementations of
the channel. If the TLS object needs to use a specific GMainContext we
should make sure that is done right from the start and not try to change
the GMainContext on the fly.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [Qemu-arm] [PATCH v4 00/31] Add ARMv8.2 half-precision functions
Peter Maydell writes: > On 27 February 2018 at 14:38, Alex Bennée wrote: >> A few minor fixes and a chunk of Richard's r-b tags. Now all that is >> left is: >> >> patch 0014/arm translate a64 add FP16 FMULX MLS FMLA to simd.patch needs >> review >> >> Otherwise see comments bellow --- for other changes >> > > Thanks -- applied to target-arm.next. Some caveats: > > (1) we can fix the nit RTH noted about FMULX later > > (2) I notice that there's no patch here that adds the linux-user/elfload.c > code to set a hwcap for the guest program to indicate FP16 presence. > Presumably there is such a hwcap? I'd missed it as risu doesn't need it. I see rth has sent a patch so I'll read up on it and see if I can extend vector-benchmark to use it to detect FP16. > > (3) Is this complete fp16 support or are there still more pieces to come? > I'm assuming it's all done... All AArch64 is done. I'm not sure how much AArch32 is needed for SVE support. The ARM ARM says "When this feature is implemented it is implemented in both Advanced SIMD and floating-point, and in AArch64 and AArch32 states." but I think it is legal to have a 64 bit only CPU without AArch32? Unfortunately the magic I used to extract all the AArch64 HP instructions from the ASL doesn't work on the AArch32 definitions which put important differentiating notes in different places. Once I've got the list I'll document it so we don't forget... > > (4) I've split the "add new ARM_V8_FP16 feature bit to the enum" > and "enable the feature on the 'any' CPU" parts of patch 2, so > we can do the latter at the end. If there is still missing parts > to fp16 then we can drop the enable-feature half of that > for the moment. I guess that depends on if we model any AArch64 only CPUs? > > thanks > -- PMM -- Alex Bennée
[Qemu-devel] [PATCH] fw_cfg: avoid unused function warning
The newly introduced fw_cfg_dma_transfer() function is unused when
CONFIG_CRASH_CORE is disabled:
drivers/firmware/qemu_fw_cfg.c:89:16: error: 'fw_cfg_dma_transfer' defined but
not used [-Werror=unused-function]
static ssize_t fw_cfg_dma_transfer(void *address, u32 length, u32 control)
This moves it into the #ifdef section that hides its caller to avoid the
warning.
Fixes: 47e78bfb5426 ("fw_cfg: write vmcoreinfo details")
Signed-off-by: Arnd Bergmann
---
drivers/firmware/qemu_fw_cfg.c | 60 +-
1 file changed, 30 insertions(+), 30 deletions(-)
diff --git a/drivers/firmware/qemu_fw_cfg.c b/drivers/firmware/qemu_fw_cfg.c
index 3015e77aebca..f002bb40519b 100644
--- a/drivers/firmware/qemu_fw_cfg.c
+++ b/drivers/firmware/qemu_fw_cfg.c
@@ -66,6 +66,36 @@ static void fw_cfg_sel_endianness(u16 key)
iowrite16(key, fw_cfg_reg_ctrl);
}
+/* read chunk of given fw_cfg blob (caller responsible for sanity-check) */
+static ssize_t fw_cfg_read_blob(u16 key,
+ void *buf, loff_t pos, size_t count)
+{
+ u32 glk = -1U;
+ acpi_status status;
+
+ /* If we have ACPI, ensure mutual exclusion against any potential
+* device access by the firmware, e.g. via AML methods:
+*/
+ status = acpi_acquire_global_lock(ACPI_WAIT_FOREVER, &glk);
+ if (ACPI_FAILURE(status) && status != AE_NOT_CONFIGURED) {
+ /* Should never get here */
+ WARN(1, "fw_cfg_read_blob: Failed to lock ACPI!\n");
+ memset(buf, 0, count);
+ return -EINVAL;
+ }
+
+ mutex_lock(&fw_cfg_dev_lock);
+ fw_cfg_sel_endianness(key);
+ while (pos-- > 0)
+ ioread8(fw_cfg_reg_data);
+ ioread8_rep(fw_cfg_reg_data, buf, count);
+ mutex_unlock(&fw_cfg_dev_lock);
+
+ acpi_release_global_lock(glk);
+ return count;
+}
+
+#ifdef CONFIG_CRASH_CORE
static inline bool fw_cfg_dma_enabled(void)
{
return (fw_cfg_rev & FW_CFG_VERSION_DMA) && fw_cfg_reg_dma;
@@ -124,36 +154,6 @@ static ssize_t fw_cfg_dma_transfer(void *address, u32
length, u32 control)
return ret;
}
-/* read chunk of given fw_cfg blob (caller responsible for sanity-check) */
-static ssize_t fw_cfg_read_blob(u16 key,
- void *buf, loff_t pos, size_t count)
-{
- u32 glk = -1U;
- acpi_status status;
-
- /* If we have ACPI, ensure mutual exclusion against any potential
-* device access by the firmware, e.g. via AML methods:
-*/
- status = acpi_acquire_global_lock(ACPI_WAIT_FOREVER, &glk);
- if (ACPI_FAILURE(status) && status != AE_NOT_CONFIGURED) {
- /* Should never get here */
- WARN(1, "fw_cfg_read_blob: Failed to lock ACPI!\n");
- memset(buf, 0, count);
- return -EINVAL;
- }
-
- mutex_lock(&fw_cfg_dev_lock);
- fw_cfg_sel_endianness(key);
- while (pos-- > 0)
- ioread8(fw_cfg_reg_data);
- ioread8_rep(fw_cfg_reg_data, buf, count);
- mutex_unlock(&fw_cfg_dev_lock);
-
- acpi_release_global_lock(glk);
- return count;
-}
-
-#ifdef CONFIG_CRASH_CORE
/* write chunk of given fw_cfg blob (caller responsible for sanity-check) */
static ssize_t fw_cfg_write_blob(u16 key,
void *buf, loff_t pos, size_t count)
--
2.9.0
Re: [Qemu-devel] [PATCH 1/2] block/file-posix: Fix fully preallocated truncate
On Wed, Feb 28, 2018 at 02:13:14PM +0100, Max Reitz wrote:
> Storing the lseek() result in an int results in it overflowing when the
> file is at least 2 GB big. Then, we have a 50 % chance of the result
> being "negative" and thus thinking an error occurred when actually
> everything went just fine.
>
> So we should use the correct type for storing the result: off_t.
>
> Reported-by: Daniel P. Berrange
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1549231
> Cc: qemu-sta...@nongnu.org
> Signed-off-by: Max Reitz
> ---
> block/file-posix.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/block/file-posix.c b/block/file-posix.c
> index f1591c3849..90c25864a0 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -1697,6 +1697,7 @@ static int raw_regular_truncate(int fd, int64_t offset,
> PreallocMode prealloc,
> case PREALLOC_MODE_FULL:
> {
> int64_t num = 0, left = offset - current_length;
> +off_t seek_result;
>
> /*
> * Knowing the final size from the beginning could allow the file
> @@ -1711,8 +1712,8 @@ static int raw_regular_truncate(int fd, int64_t offset,
> PreallocMode prealloc,
>
> buf = g_malloc0(65536);
>
> -result = lseek(fd, current_length, SEEK_SET);
> -if (result < 0) {
> +seek_result = lseek(fd, current_length, SEEK_SET);
> +if (seek_result < 0) {
off_t is an unsigned type, so this comparison to "< 0" is bogus - only the
exact value (off_t)-1 indicates an error. So this needs to be
if (seek_result == (off_t)-1) {
...
}
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH 1/2] block/file-posix: Fix fully preallocated truncate
On 2018-02-28 14:34, Daniel P. Berrangé wrote:
> On Wed, Feb 28, 2018 at 02:13:14PM +0100, Max Reitz wrote:
>> Storing the lseek() result in an int results in it overflowing when the
>> file is at least 2 GB big. Then, we have a 50 % chance of the result
>> being "negative" and thus thinking an error occurred when actually
>> everything went just fine.
>>
>> So we should use the correct type for storing the result: off_t.
>>
>> Reported-by: Daniel P. Berrange
>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1549231
>> Cc: qemu-sta...@nongnu.org
>> Signed-off-by: Max Reitz
>> ---
>> block/file-posix.c | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/file-posix.c b/block/file-posix.c
>> index f1591c3849..90c25864a0 100644
>> --- a/block/file-posix.c
>> +++ b/block/file-posix.c
>> @@ -1697,6 +1697,7 @@ static int raw_regular_truncate(int fd, int64_t
>> offset, PreallocMode prealloc,
>> case PREALLOC_MODE_FULL:
>> {
>> int64_t num = 0, left = offset - current_length;
>> +off_t seek_result;
>>
>> /*
>> * Knowing the final size from the beginning could allow the file
>> @@ -1711,8 +1712,8 @@ static int raw_regular_truncate(int fd, int64_t
>> offset, PreallocMode prealloc,
>>
>> buf = g_malloc0(65536);
>>
>> -result = lseek(fd, current_length, SEEK_SET);
>> -if (result < 0) {
>> +seek_result = lseek(fd, current_length, SEEK_SET);
>> +if (seek_result < 0) {
>
> off_t is an unsigned type, so this comparison to "< 0" is bogus - only the
> exact value (off_t)-1 indicates an error. So this needs to be
>
>if (seek_result == (off_t)-1) {
> ...
>}
Hmmm... On my system, it appears to be a long int[1]. And
find_allocation() does an off_t < 0 comparison already. And
"man 0p sys_types.h" says "blkcnt_t and off_t shall be signed integer
types."
Max
[1]
#define do_stringify(x) #x
#define stringify(x) do_stringify(x)
int main(void)
{
printf("%s\n", stringify(__OFF_T_TYPE));
}
Output: long int
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 1/2] block/file-posix: Fix fully preallocated truncate
On Wed, Feb 28, 2018 at 02:45:49PM +0100, Max Reitz wrote:
> On 2018-02-28 14:34, Daniel P. Berrangé wrote:
> > On Wed, Feb 28, 2018 at 02:13:14PM +0100, Max Reitz wrote:
> >> Storing the lseek() result in an int results in it overflowing when the
> >> file is at least 2 GB big. Then, we have a 50 % chance of the result
> >> being "negative" and thus thinking an error occurred when actually
> >> everything went just fine.
> >>
> >> So we should use the correct type for storing the result: off_t.
> >>
> >> Reported-by: Daniel P. Berrange
> >> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1549231
> >> Cc: qemu-sta...@nongnu.org
> >> Signed-off-by: Max Reitz
> >> ---
> >> block/file-posix.c | 5 +++--
> >> 1 file changed, 3 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/block/file-posix.c b/block/file-posix.c
> >> index f1591c3849..90c25864a0 100644
> >> --- a/block/file-posix.c
> >> +++ b/block/file-posix.c
> >> @@ -1697,6 +1697,7 @@ static int raw_regular_truncate(int fd, int64_t
> >> offset, PreallocMode prealloc,
> >> case PREALLOC_MODE_FULL:
> >> {
> >> int64_t num = 0, left = offset - current_length;
> >> +off_t seek_result;
> >>
> >> /*
> >> * Knowing the final size from the beginning could allow the file
> >> @@ -1711,8 +1712,8 @@ static int raw_regular_truncate(int fd, int64_t
> >> offset, PreallocMode prealloc,
> >>
> >> buf = g_malloc0(65536);
> >>
> >> -result = lseek(fd, current_length, SEEK_SET);
> >> -if (result < 0) {
> >> +seek_result = lseek(fd, current_length, SEEK_SET);
> >> +if (seek_result < 0) {
> >
> > off_t is an unsigned type, so this comparison to "< 0" is bogus - only the
> > exact value (off_t)-1 indicates an error. So this needs to be
> >
> >if (seek_result == (off_t)-1) {
> > ...
> >}
>
> Hmmm... On my system, it appears to be a long int[1]. And
> find_allocation() does an off_t < 0 comparison already. And
> "man 0p sys_types.h" says "blkcnt_t and off_t shall be signed integer
> types."
Hmm, that's odd then - lseek man page explicitly said it must be cast,
which suggested to me it could be unsigned:
RETURN VALUE
Upon successful completion, lseek() returns the resulting offset loca‐
tion as measured in bytes from the beginning of the file. On error,
the value (off_t) -1 is returned and errno is set to indicate the
error.
CC'ing Eric for the "official" POSIX answer
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH 1/2] block/file-posix: Fix fully preallocated truncate
On 2018-02-28 14:53, Daniel P. Berrangé wrote:
> On Wed, Feb 28, 2018 at 02:45:49PM +0100, Max Reitz wrote:
>> On 2018-02-28 14:34, Daniel P. Berrangé wrote:
>>> On Wed, Feb 28, 2018 at 02:13:14PM +0100, Max Reitz wrote:
Storing the lseek() result in an int results in it overflowing when the
file is at least 2 GB big. Then, we have a 50 % chance of the result
being "negative" and thus thinking an error occurred when actually
everything went just fine.
So we should use the correct type for storing the result: off_t.
Reported-by: Daniel P. Berrange
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1549231
Cc: qemu-sta...@nongnu.org
Signed-off-by: Max Reitz
---
block/file-posix.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/block/file-posix.c b/block/file-posix.c
index f1591c3849..90c25864a0 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1697,6 +1697,7 @@ static int raw_regular_truncate(int fd, int64_t
offset, PreallocMode prealloc,
case PREALLOC_MODE_FULL:
{
int64_t num = 0, left = offset - current_length;
+off_t seek_result;
/*
* Knowing the final size from the beginning could allow the file
@@ -1711,8 +1712,8 @@ static int raw_regular_truncate(int fd, int64_t
offset, PreallocMode prealloc,
buf = g_malloc0(65536);
-result = lseek(fd, current_length, SEEK_SET);
-if (result < 0) {
+seek_result = lseek(fd, current_length, SEEK_SET);
+if (seek_result < 0) {
>>>
>>> off_t is an unsigned type, so this comparison to "< 0" is bogus - only the
>>> exact value (off_t)-1 indicates an error. So this needs to be
>>>
>>>if (seek_result == (off_t)-1) {
>>> ...
>>>}
>>
>> Hmmm... On my system, it appears to be a long int[1]. And
>> find_allocation() does an off_t < 0 comparison already. And
>> "man 0p sys_types.h" says "blkcnt_t and off_t shall be signed integer
>> types."
>
> Hmm, that's odd then - lseek man page explicitly said it must be cast,
> which suggested to me it could be unsigned:
>
>RETURN VALUE
>Upon successful completion, lseek() returns the resulting offset loca‐
>tion as measured in bytes from the beginning of the file. On error,
>the value (off_t) -1 is returned and errno is set to indicate the
>error.
>
> CC'ing Eric for the "official" POSIX answer
But it also says (under NOTES):
The off_t data type is a signed integer data type specified by POSIX.1.
Max
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 1/2] block/file-posix: Fix fully preallocated truncate
On Wed, Feb 28, 2018 at 02:13:14PM +0100, Max Reitz wrote:
> Storing the lseek() result in an int results in it overflowing when the
> file is at least 2 GB big. Then, we have a 50 % chance of the result
> being "negative" and thus thinking an error occurred when actually
> everything went just fine.
>
> So we should use the correct type for storing the result: off_t.
>
> Reported-by: Daniel P. Berrange
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1549231
> Cc: qemu-sta...@nongnu.org
> Signed-off-by: Max Reitz
> ---
> block/file-posix.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/block/file-posix.c b/block/file-posix.c
> index f1591c3849..90c25864a0 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -1697,6 +1697,7 @@ static int raw_regular_truncate(int fd, int64_t offset,
> PreallocMode prealloc,
> case PREALLOC_MODE_FULL:
> {
> int64_t num = 0, left = offset - current_length;
> +off_t seek_result;
>
> /*
> * Knowing the final size from the beginning could allow the file
> @@ -1711,8 +1712,8 @@ static int raw_regular_truncate(int fd, int64_t offset,
> PreallocMode prealloc,
>
> buf = g_malloc0(65536);
>
> -result = lseek(fd, current_length, SEEK_SET);
> -if (result < 0) {
> +seek_result = lseek(fd, current_length, SEEK_SET);
> +if (seek_result < 0) {
> result = -errno;
> error_setg_errno(errp, -result,
> "Failed to seek to the old end of file");
Reviewed-by: Daniel P. Berrangé
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH 1/2] block/file-posix: Fix fully preallocated truncate
On Wed, Feb 28, 2018 at 02:55:22PM +0100, Max Reitz wrote:
> On 2018-02-28 14:53, Daniel P. Berrangé wrote:
> > On Wed, Feb 28, 2018 at 02:45:49PM +0100, Max Reitz wrote:
> >> On 2018-02-28 14:34, Daniel P. Berrangé wrote:
> >>> On Wed, Feb 28, 2018 at 02:13:14PM +0100, Max Reitz wrote:
> Storing the lseek() result in an int results in it overflowing when the
> file is at least 2 GB big. Then, we have a 50 % chance of the result
> being "negative" and thus thinking an error occurred when actually
> everything went just fine.
>
> So we should use the correct type for storing the result: off_t.
>
> Reported-by: Daniel P. Berrange
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1549231
> Cc: qemu-sta...@nongnu.org
> Signed-off-by: Max Reitz
> ---
> block/file-posix.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/block/file-posix.c b/block/file-posix.c
> index f1591c3849..90c25864a0 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -1697,6 +1697,7 @@ static int raw_regular_truncate(int fd, int64_t
> offset, PreallocMode prealloc,
> case PREALLOC_MODE_FULL:
> {
> int64_t num = 0, left = offset - current_length;
> +off_t seek_result;
>
> /*
> * Knowing the final size from the beginning could allow the
> file
> @@ -1711,8 +1712,8 @@ static int raw_regular_truncate(int fd, int64_t
> offset, PreallocMode prealloc,
>
> buf = g_malloc0(65536);
>
> -result = lseek(fd, current_length, SEEK_SET);
> -if (result < 0) {
> +seek_result = lseek(fd, current_length, SEEK_SET);
> +if (seek_result < 0) {
> >>>
> >>> off_t is an unsigned type, so this comparison to "< 0" is bogus - only the
> >>> exact value (off_t)-1 indicates an error. So this needs to be
> >>>
> >>>if (seek_result == (off_t)-1) {
> >>> ...
> >>>}
> >>
> >> Hmmm... On my system, it appears to be a long int[1]. And
> >> find_allocation() does an off_t < 0 comparison already. And
> >> "man 0p sys_types.h" says "blkcnt_t and off_t shall be signed integer
> >> types."
> >
> > Hmm, that's odd then - lseek man page explicitly said it must be cast,
> > which suggested to me it could be unsigned:
> >
> >RETURN VALUE
> >Upon successful completion, lseek() returns the resulting offset
> > loca‐
> >tion as measured in bytes from the beginning of the file. On
> > error,
> >the value (off_t) -1 is returned and errno is set to indicate
> > the
> >error.
> >
> > CC'ing Eric for the "official" POSIX answer
>
> But it also says (under NOTES):
>
> The off_t data type is a signed integer data type specified by POSIX.1.
Ok, lets ignore my comments then -the "< 0" vs "!= -1" difference is
harmless given this.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH v4 2/5] qcow2: Document some maximum size constraints
On 02/28/2018 04:26 AM, Alberto Garcia wrote: On Tue 27 Feb 2018 05:29:41 PM CET, Eric Blake wrote: +The refcount table has implications on the maximum host file size; a +larger cluster size is required for the refcount table to cover larger +offsets. Why is this? Because of the refcount_table_clusters field ? I think the maximum offset allowed by that is ridiculously high, exceeding any other limit imposed by the L1/L2 tables. Good point. I was basing my comment off of qcow2.h: /* 8 MB refcount table is enough for 2 PB images at 64k cluster size * (128 GB for 512 byte clusters, 2 EB for 2 MB clusters) */ #define QCOW_MAX_REFTABLE_SIZE 0x80 But that's our implementation choice (we put a maximum amount of memory on the size of the refcount table we are willing to support, while the qcow2 spec would allow an implementation willing to reserve more memory to access even larger sizing). If my numbers are right, with the default values that's 64 ZB. In addition to that, the size that can be covered by the refcount table also depends on the size of refcount entries (refcount_order). True. With 512 byte clusters and 64 bit refcount entries I still get 8 PB, way over what's limited by the L1/L2 tables (128 GB). Do I need to make any modifications to the sentence, then? Or is it still accurate, if vague, to leave the sentence as is because there IS an impact to consider, even if the impact is unlikely to matter in relation to other sizing impacts? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH 2/2] iotests: Test preallocated truncate of 2G image
On Wed, Feb 28, 2018 at 02:13:15PM +0100, Max Reitz wrote:
> Signed-off-by: Max Reitz
> ---
> tests/qemu-iotests/106 | 24
> tests/qemu-iotests/106.out | 10 ++
> 2 files changed, 34 insertions(+)
>
> diff --git a/tests/qemu-iotests/106 b/tests/qemu-iotests/106
> index bfe71f4e60..5e51f88a78 100755
> --- a/tests/qemu-iotests/106
> +++ b/tests/qemu-iotests/106
> @@ -86,6 +86,30 @@ for growth_mode in falloc full off; do
> $QEMU_IMG resize -f "$IMGFMT" --shrink --preallocation=$growth_mode
> "$TEST_IMG" -${GROWTH_SIZE}K
> done
>
> +echo
> +echo '=== Testing image growth on 2G empty image ==='
> +
> +for growth_mode in falloc full; do
> +echo
> +echo "--- growth_mode=$growth_mode ---"
> +
> +# Maybe we want to do an lseek() to the end of the file before the
> +# preallocation; if the file has a length of 2 GB, that would
> +# return an integer that overflows to negative when put into a
> +# plain int. We should use the correct type for the result, and
> +# this tests we do.
> +
> +_make_test_img 2G
> +$QEMU_IMG resize -f "$IMGFMT" --preallocation=$growth_mode "$TEST_IMG"
> +${GROWTH_SIZE}K
> +
> +actual_size=$($QEMU_IMG info -f "$IMGFMT" "$TEST_IMG" | grep 'disk size')
> +actual_size=$(echo "$actual_size" | sed -e
> 's/^[^0-9]*\([0-9]\+\).*$/\1/')
> +
> +if [ $actual_size -lt $GROWTH_SIZE ]; then
> +echo "ERROR: Image should have at least ${GROWTH_SIZE}K, but has
> ${actual_size}K"
> +fi
> +done
> +
> # success, all done
> echo '*** done'
> rm -f $seq.full
> diff --git a/tests/qemu-iotests/106.out b/tests/qemu-iotests/106.out
> index 0a42312301..c459957660 100644
> --- a/tests/qemu-iotests/106.out
> +++ b/tests/qemu-iotests/106.out
> @@ -47,4 +47,14 @@ qemu-img: Preallocation can only be used for growing images
>
> --- growth_mode=off ---
> Image resized.
> +
> +=== Testing image growth on 2G empty image ===
> +
> +--- growth_mode=falloc ---
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2147483648
> +Image resized.
> +
> +--- growth_mode=full ---
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2147483648
> +Image resized.
> *** done
Reviewed-by: Daniel P. Berrangé
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
[Qemu-devel] [PATCH] crypto: ensure we use a predictable TLS priority setting
The TLS test cert generation relies on a fixed set of algorithms that are only usable under GNUTLS' default priority setting. When building QEMU with a custom distro specific priority setting, this can cause the TLS tests to fail. By forcing the tests to always use "NORMAL" priority we can make them more robust. Signed-off-by: Daniel P. Berrangé --- tests/test-crypto-tlssession.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test-crypto-tlssession.c b/tests/test-crypto-tlssession.c index 1a4a066d76..82f21c27f2 100644 --- a/tests/test-crypto-tlssession.c +++ b/tests/test-crypto-tlssession.c @@ -75,6 +75,7 @@ static QCryptoTLSCreds *test_tls_creds_create(QCryptoTLSCredsEndpoint endpoint, "server" : "client"), "dir", certdir, "verify-peer", "yes", +"priority", "NORMAL", /* We skip initial sanity checks here because we * want to make sure that problems are being * detected at the TLS session validation stage, -- 2.14.3
Re: [Qemu-devel] [PATCH v2 05/16] block/mirror: Convert to coroutines
On 2018-02-27 08:44, Fam Zheng wrote:
> On Mon, 01/22 23:07, Max Reitz wrote:
>> @@ -101,7 +105,7 @@ static BlockErrorAction
>> mirror_error_action(MirrorBlockJob *s, bool read,
>> }
>> }
>>
>> -static void mirror_iteration_done(MirrorOp *op, int ret)
>> +static void coroutine_fn mirror_iteration_done(MirrorOp *op, int ret)
>> {
>> MirrorBlockJob *s = op->s;
>> struct iovec *iov;
>
> I think we want s/qemu_coroutine_enter/aio_co_wake/ in
> mirror_iteration_done().
> As an AIO callback before, this didn't matter, but now we are in an
> terminating
> coroutine, so it is pointless to defer the termination, or even risky in that
> we
> are in a aio_context_acquire/release section, but have already decremented
> s->in_flight, which is fishy.
I guess I'll still do the replacement, regardless of whether the next
patch overwrites it again...
>> @@ -138,9 +142,8 @@ static void mirror_iteration_done(MirrorOp *op, int ret)
>> }
>> }
>>
>> -static void mirror_write_complete(void *opaque, int ret)
>> +static void coroutine_fn mirror_write_complete(MirrorOp *op, int ret)
>> {
>> -MirrorOp *op = opaque;
>> MirrorBlockJob *s = op->s;
>>
>> aio_context_acquire(blk_get_aio_context(s->common.blk));
>> @@ -157,9 +160,8 @@ static void mirror_write_complete(void *opaque, int ret)
>> aio_context_release(blk_get_aio_context(s->common.blk));
>> }
>>
>> -static void mirror_read_complete(void *opaque, int ret)
>> +static void coroutine_fn mirror_read_complete(MirrorOp *op, int ret)
>> {
>> -MirrorOp *op = opaque;
>> MirrorBlockJob *s = op->s;
>>
>> aio_context_acquire(blk_get_aio_context(s->common.blk));
>> @@ -174,8 +176,11 @@ static void mirror_read_complete(void *opaque, int ret)
>>
>> mirror_iteration_done(op, ret);
>> } else {
>> -blk_aio_pwritev(s->target, op->offset, &op->qiov,
>> -0, mirror_write_complete, op);
>> +int ret;
>
> s/ret/ret2/ or drop the definition?
> because ret is already the paramter of the function.
Oh, right, yes, will do.
>> +
>> +ret = blk_co_pwritev(s->target, op->offset,
>> + op->qiov.size, &op->qiov, 0);
>> +mirror_write_complete(op, ret);
>> }
>> aio_context_release(blk_get_aio_context(s->common.blk));
>> }
>
>
>
>> +static void coroutine_fn mirror_co_discard(void *opaque)
>> +{
>> +MirrorOp *op = opaque;
>> +int ret;
>> +
>> +op->s->in_flight++;
>> +op->s->bytes_in_flight += op->bytes;
>> +*op->bytes_handled = op->bytes;
>> +
>> +ret = blk_co_pdiscard(op->s->target, op->offset, op->bytes);
>> +mirror_write_complete(op, ret);
>> }
>>
>> static unsigned mirror_perform(MirrorBlockJob *s, int64_t offset,
>> unsigned bytes, MirrorMethod mirror_method)
>
> Doesn't mirror_perform need coroutine_fn annotation too?
I don't think it needs one. We could give it one, but as far as I've
understood (which may be wrong), all functions that need to be run from
a coroutine need the tag -- but functions that may be called from either
coroutines or just normal code don't need it.
(And I think this function should be fine either way, so I don't think
it needs a tag.)
Also, thanks for reviewing! :-)
Max
signature.asc
Description: OpenPGP digital signature
