[Qemu-devel] [Question] Hotplug rescan/remove by manual
Hi guys, When I tested hotplug on pci.0 on pc platform, it seems that we have to echo 1 > /sys/bus/pci/rescan manually in guest? Is this a non-complete feature because of something like gpe interrupt issue? For hot-unplug, I uses echo 1 > /sys/bus/pci//remove but this seems only remove the device in guest but we still can see it in qemu monitor by "info pci". I guess this is not a right way to unplug. I'm not sure if I missed something? Looking forwards to your response. Thanks! Jing
Re: [Qemu-devel] [PATCH v5 2/6] fsdev-throttle-qmp: Rename the ThrottleLimits member names
> Subject: Re: [Qemu-devel] [PATCH v5 2/6] fsdev-throttle-qmp: Rename the
> ThrottleLimits member names
>
> On 11/28/18 3:25 AM, Markus Armbruster wrote:
> > xiezhide writes:
> >
> >> Rename the ThrottleLimits member names and modify related code
> >>
> >> Signed-off-by: xiezhide
> >> ---
> >> qapi/block-core.json | 70 +++---
> >> util/throttle.c | 163
> +--
> >> 2 files changed, 116 insertions(+), 117 deletions(-)
> >>
> >> diff --git a/qapi/block-core.json b/qapi/block-core.json index
> >> d4fe710..4ffaaea 100644
> >> --- a/qapi/block-core.json
> >> +++ b/qapi/block-core.json
>
> >> ##
> >> { 'struct': 'ThrottleLimits',
> >> - 'data': { '*iops-total' : 'int', '*iops-total-max' : 'int',
>
> >> + 'data': { '*iops' : 'int', '*iops_max' : 'int',
> >> +'*iops_max_length' : 'int', '*iops_rd' : 'int',
> >> +'*iops_rd_max' : 'int', '*iops_rd_max_length' : 'int',
> >> +'*iops_wr' : 'int', '*iops_wr_max' : 'int',
> >> +'*iops_wr_max_length' : 'int', '*bps' : 'int',
> >> +'*bps_max' : 'int', '*bps_max_length' : 'int',
> >> +'*bps_rd' : 'int', '*bps_rd_max' : 'int',
> >> +'*bps_rd_max_length' : 'int', '*bps_wr' : 'int',
> >> +'*bps_wr_max' : 'int', '*bps_wr_max_length' : 'int',
> >> +'*iops_size' : 'int' } }
> >
> > Compatibility break. Why is that okay?
>
> Grepping qapi/qapi-introspection.c shows 0 hits for either ThrottleLimits or
> for
> iops-total, so there are no QMP commands affected.
> There might, however, be command line and/or QOM paths affected, which is
> harder to audit since those don't affect instrospection.
>
> >
> > Even if it is, you still run afoul of docs/devel/qapi-code-gen.txt:
> >
> > Command names, and member names within a type, should be all
> lower
> > case with words separated by a hyphen. However, some existing
> older
> > commands and complex types use underscore; when extending such
> > expressions, consistency is preferred over blindly avoiding
> > underscore.
> >
> > The exception doesn't apply here.
>
> Ah, but it does, because we are refactoring code to share a common QAPI
> struct in a later patch, where we need this exact naming to avoid breaking
> that
> command.
>
> So the REAL problem with this commit is that the commit message does not
> give enough details, either why this is safe (because it does not impact
> existing
> QMP commands) or needed (because we will be using it to rewrite an existing
> QMP command that needs this spelling).
>
@Erick, thanks for your simple but exact explaining for purpose of this patch
Thanks
xiezhide
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc. +1-919-301-3266
> Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH v5 2/6] fsdev-throttle-qmp: Rename the ThrottleLimits member names
> Subject: Re: [Qemu-devel] [PATCH v5 2/6] fsdev-throttle-qmp: Rename the
> ThrottleLimits member names
>
> xiezhide writes:
>
> > Rename the ThrottleLimits member names and modify related code
> >
> > Signed-off-by: xiezhide
> > ---
> > qapi/block-core.json | 70 +++---
> > util/throttle.c | 163
> +--
> > 2 files changed, 116 insertions(+), 117 deletions(-)
> >
> > diff --git a/qapi/block-core.json b/qapi/block-core.json index
> > d4fe710..4ffaaea 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -2240,45 +2240,45 @@
> > # transaction. All fields are optional. When setting limits, if a
> > field is # missing the current value is not changed.
> > #
> > -# @iops-total: limit total I/O operations per second
> > -# @iops-total-max: I/O operations burst
> > -# @iops-total-max-length: length of the iops-total-max burst period, in
> seconds
> > -# It must only be set if @iops-total-max is set
> as well.
> > -# @iops-read: limit read operations per second
> > -# @iops-read-max: I/O operations read burst
> > -# @iops-read-max-length: length of the iops-read-max burst period, in
> seconds
> > -# It must only be set if @iops-read-max is set
> as well.
> > -# @iops-write: limit write operations per second
> > -# @iops-write-max: I/O operations write burst
> > -# @iops-write-max-length: length of the iops-write-max burst period, in
> seconds
> > -# It must only be set if @iops-write-max is
> set as well.
> > -# @bps-total: limit total bytes per second
> > -# @bps-total-max: total bytes burst
> > -# @bps-total-max-length: length of the bps-total-max burst period, in
> seconds.
> > -# It must only be set if @bps-total-max is set
> as well.
> > -# @bps-read: limit read bytes per second
> > -# @bps-read-max: total bytes read burst
> > -# @bps-read-max-length:length of the bps-read-max burst period, in
> seconds
> > -# It must only be set if @bps-read-max is set
> as well.
> > -# @bps-write: limit write bytes per second
> > -# @bps-write-max: total bytes write burst
> > -# @bps-write-max-length: length of the bps-write-max burst period, in
> seconds
> > -# It must only be set if @bps-write-max is set
> as well.
> > -# @iops-size: when limiting by iops max size of an I/O in
> bytes
> > +# @iops: limit total I/O operations per second
> > +# @iops_max: I/O operations burst
> > +# @iops_max_length: length of the iops_total_max burst period, in
> seconds
> > +# It must only be set if @iops_total_max is
> set as well.
> > +# @iops_rd: limit read operations per second
> > +# @iops_rd_max: I/O operations read burst
> > +# @iops_rd_max_length: length of the iops_read_max burst period, in
> seconds
> > +# It must only be set if @iops_read_max is
> set as well.
> > +# @iops_wr: limit write operations per second
> > +# @iops_wr_max: I/O operations write burst
> > +# @iops_wr_max_length: length of the iops_write_max burst period, in
> seconds
> > +# It must only be set if @iops_write_max is
> set as well.
> > +# @bps: limit total bytes per second
> > +# @bps_max: total bytes burst
> > +# @bps_max_length: length of the bps_total_max burst period, in
> seconds.
> > +# It must only be set if @bps_total_max is
> set as well.
> > +# @bps_rd: limit read bytes per second
> > +# @bps_rd_max: total bytes read burst
> > +# @bps_rd_max_length:length of the bps_read_max burst period, in
> seconds
> > +# It must only be set if @bps_read_max is
> set as well.
> > +# @bps_wr: limit write bytes per second
> > +# @bps_wr_max: total bytes write burst
> > +# @bps_wr_max_length: length of the bps_write_max burst period, in
> seconds
> > +# It must only be set if @bps_write_max is
> set as well.
> > +# @iops_size: when limiting by iops max size of an I/O in
> bytes
> > #
> > # Since: 2.11
> > ##
> > { 'struct': 'ThrottleLimits',
> > - 'data': { '*iops-total' : 'int', '*iops-total-max' : 'int',
> > -'*iops-total-max-length' : 'int', '*iops-read' : 'int',
> > -'*iops-read-max' : 'int', '*iops-read-max-length' : 'int',
> > -'*iops-write' : 'int', '*iops-write-max' : 'int',
> > -'*iops-write-max-length' : 'int', '*bps-total' : 'int',
> > -'*bps-total-max' : 'int', '*bps-total-max-length' : 'int',
> > -'*bps-read' : 'int', '*bps-read-max' : 'int',
> > -
Re: [Qemu-devel] [Qemu-ppc] [PATCH v5 12/36] spapr: initialize VSMT before initializing the IRQ backend
On Thu, 29 Nov 2018 12:02:48 +1100
David Gibson wrote:
> On Wed, Nov 28, 2018 at 10:35:51AM +0100, Greg Kurz wrote:
> > On Wed, 28 Nov 2018 13:57:14 +1100
> > David Gibson wrote:
> >
> > > On Fri, Nov 16, 2018 at 11:57:05AM +0100, Cédric Le Goater wrote:
> > > > We will need to use xics_max_server_number() to create the sPAPRXive
> > > > object modeling the interrupt controller of the machine which is
> > > > created before the CPUs.
> > > >
> > > > Signed-off-by: Cédric Le Goater
> > >
> > > My only concern here is that this moves the spapr_set_vsmt_mode()
> > > before some of the sanity checks in spapr_init_cpus(). Are we certain
> > > there are no edge cases that could cause badness?
> > >
> >
> > The early checks in spapr_init_cpus() filter out topologies that would
> > result in partially filled cores. They're only related to the rest of
> > the code that creates the boot CPUs. Before commit 1a5008fc17,
> > spapr_set_vsmt_mode() was even being called before spapr_init_cpus().
> > The rationale to move it there was to ensure it is called before the
> > first user of spapr->vsmt, which happens to be a call to
> > xics_max_server_number().
>
> Ok.
>
> > Now that xics_max_server_number() needs to be called even earlier, I think a
> > better change is to have xics_max_server_number() to call
> > spapr_set_vsmt_mode()
> > if spapr->vsmt isn't set.
>
> I'd rather not do that, but instead move it statically to where it
> needs to be. That sort of lazy/on-demand initialization can result in
> really confusing behaviours depending on when a seemingly innocuous
> data-returning function is called, so I consider it a code smell.
>
Fair enough, then:
Reviewed-by: Greg Kurz
> >
> > > > ---
> > > > hw/ppc/spapr.c | 10 +-
> > > > 1 file changed, 5 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > index 7afd1a175bf2..50cb9f9f4a02 100644
> > > > --- a/hw/ppc/spapr.c
> > > > +++ b/hw/ppc/spapr.c
> > > > @@ -2466,11 +2466,6 @@ static void spapr_init_cpus(sPAPRMachineState
> > > > *spapr)
> > > > boot_cores_nr = possible_cpus->len;
> > > > }
> > > >
> > > > -/* VSMT must be set in order to be able to compute VCPU ids, ie to
> > > > - * call xics_max_server_number() or spapr_vcpu_id().
> > > > - */
> > > > -spapr_set_vsmt_mode(spapr, _fatal);
> > > > -
> > > > if (smc->pre_2_10_has_unused_icps) {
> > > > int i;
> > > >
> > > > @@ -2593,6 +2588,11 @@ static void spapr_machine_init(MachineState
> > > > *machine)
> > > > /* Setup a load limit for the ramdisk leaving room for SLOF and
> > > > FDT */
> > > > load_limit = MIN(spapr->rma_size, RTAS_MAX_ADDR) - FW_OVERHEAD;
> > > >
> > > > +/* VSMT must be set in order to be able to compute VCPU ids, ie to
> > > > + * call xics_max_server_number() or spapr_vcpu_id().
> > > > + */
> > > > +spapr_set_vsmt_mode(spapr, _fatal);
> > > > +
> > > > /* Set up Interrupt Controller before we create the VCPUs */
> > > > smc->irq->init(spapr, _fatal);
> > > >
> > >
> >
>
>
>
pgp35Qoq9Zp0U.pgp
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v2 01/16] virtio: split vhost vsock bits from virtio-pci
On 2018-11-29 07:30, Thomas Huth wrote: > On 2018-11-26 20:59, Juan Quintela wrote: >> Signed-off-by: Juan Quintela >> >> --- >> >> Updated copyright info >> Also split virtio-pci.h bits >> --- >> hw/virtio/Makefile.objs | 2 +- >> hw/virtio/vhost-vsock-pci.c | 82 + >> hw/virtio/virtio-pci.c | 51 --- >> hw/virtio/virtio-pci.h | 18 >> 4 files changed, 83 insertions(+), 70 deletions(-) >> create mode 100644 hw/virtio/vhost-vsock-pci.c >> >> diff --git a/hw/virtio/Makefile.objs b/hw/virtio/Makefile.objs >> index 1b2799cfd8..4fbf7de84b 100644 >> --- a/hw/virtio/Makefile.objs >> +++ b/hw/virtio/Makefile.objs >> @@ -10,7 +10,7 @@ obj-$(CONFIG_VIRTIO_CRYPTO) += virtio-crypto.o >> obj-$(call land,$(CONFIG_VIRTIO_CRYPTO),$(CONFIG_VIRTIO_PCI)) += >> virtio-crypto-pci.o >> >> obj-$(CONFIG_LINUX) += vhost.o vhost-backend.o vhost-user.o >> -obj-$(CONFIG_VHOST_VSOCK) += vhost-vsock.o >> +obj-$(CONFIG_VHOST_VSOCK) += vhost-vsock.o vhost-vsock-pci.o > > Please change this into: > > obj-$(call land,$(CONFIG_VHOST_VSOCK),$(CONFIG_VIRTIO_PCI)) += > vhost-vsock-pci.o ... or alternatively, wrap the new lines in a "ifeq ($(CONFIG_VIRTIO_PCI),y)" block, that's maybe easier. Thomas
Re: [Qemu-devel] [PATCH v2 01/16] virtio: split vhost vsock bits from virtio-pci
On 2018-11-26 20:59, Juan Quintela wrote: > Signed-off-by: Juan Quintela > > --- > > Updated copyright info > Also split virtio-pci.h bits > --- > hw/virtio/Makefile.objs | 2 +- > hw/virtio/vhost-vsock-pci.c | 82 + > hw/virtio/virtio-pci.c | 51 --- > hw/virtio/virtio-pci.h | 18 > 4 files changed, 83 insertions(+), 70 deletions(-) > create mode 100644 hw/virtio/vhost-vsock-pci.c > > diff --git a/hw/virtio/Makefile.objs b/hw/virtio/Makefile.objs > index 1b2799cfd8..4fbf7de84b 100644 > --- a/hw/virtio/Makefile.objs > +++ b/hw/virtio/Makefile.objs > @@ -10,7 +10,7 @@ obj-$(CONFIG_VIRTIO_CRYPTO) += virtio-crypto.o > obj-$(call land,$(CONFIG_VIRTIO_CRYPTO),$(CONFIG_VIRTIO_PCI)) += > virtio-crypto-pci.o > > obj-$(CONFIG_LINUX) += vhost.o vhost-backend.o vhost-user.o > -obj-$(CONFIG_VHOST_VSOCK) += vhost-vsock.o > +obj-$(CONFIG_VHOST_VSOCK) += vhost-vsock.o vhost-vsock-pci.o Please change this into: obj-$(call land,$(CONFIG_VHOST_VSOCK),$(CONFIG_VIRTIO_PCI)) += vhost-vsock-pci.o Rationale: There could be targets that don't use virtio-pci (but virtio-mmio or virtio-ccw), and without this, the PCI devices are suddenly included while they were not included before. Same change in the other patches of this series, too, please. Thomas
Re: [Qemu-devel] [PATCH v9 5/8] migration/ram.c: add a notifier chain for precopy
On 11/29/2018 01:10 PM, Peter Xu wrote:
On Thu, Nov 29, 2018 at 11:40:57AM +0800, Wei Wang wrote:
On 11/28/2018 05:32 PM, Peter Xu wrote:
I'm not sure we can use start_postcopy. It's a variable being set in
the QMP handler but it does not mean postcopy has started. I'm afraid
there can be race where it's still precopy but the variable is set so
event could be missed...
IMHO the problem is not that complicated. How about this proposal:
[1]
typedef enum PrecopyNotifyReason {
PRECOPY_NOTIFY_RAM_START,
PRECOPY_NOTIFY_RAM_BEFORE_SYNC,
PRECOPY_NOTIFY_RAM_AFTER_SYNC,
PRECOPY_NOTIFY_COMPLETE,
PRECOPY_NOTIFY_RAM_CLEANUP,
};
The first three keep the same as your old ones. Notify RAM_CLEANUP in
ram_save_cleanup() to make sure it'll always be cleaned up (the same
as PRECOPY_END, just another name). Notify COMPLETE in
qemu_savevm_state_complete_precopy() to show that precopy is
completed. Meanwhile on balloon side you should stop the hinting for
either RAM_CLEANUP or COMPLETE event. Then either:
- precopy is switching to postcopy, or
- precopy completed, or
- precopy failed/cancelled
You should always get at least a notification to stop the balloon.
Though you could also get one RAM_CLEANUP after one COMPLETE, but
the balloon should easily handle it (stop the hinting twice).
Sounds better, thanks.
Here maybe you can even remove the "RAM_" in both RAM_START and
RAM_CLEANUP if we're going to have COMPLETE since after all it'll be
not only limited to RAM.
Another suggestion is that you can add an Error into the notify hooks,
please refer to the postcopy one:
int postcopy_notify(enum PostcopyNotifyReason reason, Error **errp);
So the hook functions have a way to even stop the migration (though
for balloon hinting it'll be always optional so no error should be
reported...), then the two interfaces are matched.
Yes, I removed that "errp" as it's not needed by the balloon usage.
But no problem, I can add it back if no objections from others.
Best,
Wei
Re: [Qemu-devel] [PATCH for 3.1 4/4] virtio-net-test: add large tx buffer test
On 2018-11-29 04:12, Jason Wang wrote:
> This test tries to build a packet whose size is greater than INT_MAX
> which tries to trigger integer overflow in qemu_net_queue_append_iov()
> which may result OOB.
>
> Signed-off-by: Jason Wang
> ---
> tests/virtio-net-test.c | 46 +
> 1 file changed, 46 insertions(+)
>
> diff --git a/tests/virtio-net-test.c b/tests/virtio-net-test.c
> index 33d26ab079..09c220c2fa 100644
> --- a/tests/virtio-net-test.c
> +++ b/tests/virtio-net-test.c
> @@ -245,6 +245,51 @@ static void pci_basic(gconstpointer data)
> g_free(dev);
> qtest_shutdown(qs);
> }
> +
> +static void large_tx(gconstpointer data)
> +{
> +QVirtioPCIDevice *dev;
> +QOSState *qs;
> +QVirtQueuePCI *tx, *rx;
> +QVirtQueue *vq;
> +const char *cmd = "-netdev hubport,id=hp0,hubid=0 "
> + "-device virtio-net-pci,netdev=hp0 ";
> +uint64_t req_addr;
> +uint32_t free_head;
> +size_t alloc_size = UINT_MAX / 64;
> +int i;
> +
> +qs = pci_test_start(cmd);
> +dev = virtio_net_pci_init(qs->pcibus, PCI_SLOT);
> +
> +rx = (QVirtQueuePCI *)qvirtqueue_setup(>vdev, qs->alloc, 0);
> +tx = (QVirtQueuePCI *)qvirtqueue_setup(>vdev, qs->alloc, 1);
> +
> +driver_init(>vdev);
> +vq = >vq;
> +
> +/* Bypass the limitation by pointing several descriptors to a single
> + * smaller area */
> +req_addr = guest_alloc(qs->alloc, alloc_size);
> +for (i = 0; i < 64; i ++) {
> +if (i == 0)
> +free_head = qvirtqueue_add(vq, req_addr, alloc_size, false,
> true);
> +else
> +qvirtqueue_add(vq, req_addr, alloc_size, false, true);
QEMU coding style requires always curly braces. Maybe move the initial
case before the for-loop and then start the for loop with i=1 instead?
> +}
> +qvirtqueue_add(vq, req_addr, alloc_size, false, false);
> +qvirtqueue_kick(>vdev, vq, free_head);
> +
> +qvirtio_wait_used_elem(>vdev, vq, free_head, NULL,
> + QVIRTIO_NET_TIMEOUT_US);
> +
> +qvirtqueue_cleanup(dev->vdev.bus, >vq, qs->alloc);
> +qvirtqueue_cleanup(dev->vdev.bus, >vq, qs->alloc);
> +qvirtio_pci_device_disable(dev);
> +g_free(dev->pdev);
> +g_free(dev);
> +qtest_shutdown(qs);
> +}
> #endif
>
> static void hotplug(void)
> @@ -270,6 +315,7 @@ int main(int argc, char **argv)
> qtest_add_data_func("/virtio/net/pci/basic", send_recv_test, pci_basic);
> qtest_add_data_func("/virtio/net/pci/rx_stop_cont",
> stop_cont_test, pci_basic);
> +qtest_add_data_func("/virtio/net/pci/large_tx", NULL, large_tx);
> #endif
> qtest_add_func("/virtio/net/pci/hotplug", hotplug);
Thomas
Re: [Qemu-devel] [PATCH for 3.1 2/4] virtio-net-test: remove unused macro
On 2018-11-29 04:12, Jason Wang wrote: > Signed-off-by: Jason Wang > --- > tests/virtio-net-test.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/tests/virtio-net-test.c b/tests/virtio-net-test.c > index dcb87a8b6e..231e7c767e 100644 > --- a/tests/virtio-net-test.c > +++ b/tests/virtio-net-test.c > @@ -24,7 +24,6 @@ > > #define PCI_SLOT_HP 0x06 > #define PCI_SLOT0x04 > -#define PCI_FN 0x00 > > #define QVIRTIO_NET_TIMEOUT_US (30 * 1000 * 1000) > #define VNET_HDR_SIZE sizeof(struct virtio_net_hdr_mrg_rxbuf) > Reviewed-by: Thomas Huth
Re: [Qemu-devel] [PATCH RFC 1/5] Fix segmentation fault when qemu_signal_init fails
On 11/28/2018 08:53 PM, Markus Armbruster wrote:
Fei Li writes:
When qemu_signal_init() fails in qemu_init_main_loop(), we return
without setting an error. Its callers crash then when they try to
report the error with error_report_err().
Yes, that's a bug. Broken in 2f78e491d7b, v2.2.0. Has escaped notice
since qemu_signalfd() is quite unlikely to fail. Could go into 3.1 as a
bug fix, but I think punting it to the next release is just fine.
Thanks. :) BTW, should I send the next version only includes patch 1/5
and 2/5 separately so that you can merge? (I guess Dave will help to
merge the other three migration related patches)
To avoid such segmentation fault, add a new Error parameter to make
the call trace to propagate the err to the final caller.
Let's add:
Fixes: 2f78e491d7b46542158ce0b8132ee4e05bc0ade4
ok.
Cc: Markus Armbruster
Cc: Fam Zheng
Signed-off-by: Fei Li
Reviewed-by: Fam Zheng
---
util/main-loop.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/util/main-loop.c b/util/main-loop.c
index affe0403c5..443cb4cfe8 100644
--- a/util/main-loop.c
+++ b/util/main-loop.c
@@ -71,7 +71,7 @@ static void sigfd_handler(void *opaque)
}
}
-static int qemu_signal_init(void)
+static int qemu_signal_init(Error **errp)
{
int sigfd;
sigset_t set;
@@ -96,7 +96,7 @@ static int qemu_signal_init(void)
sigdelset(, SIG_IPI);
sigfd = qemu_signalfd();
if (sigfd == -1) {
-fprintf(stderr, "failed to create signalfd\n");
+error_setg_errno(errp, errno, "failed to create signalfd");
return -errno;
}
@@ -109,7 +109,7 @@ static int qemu_signal_init(void)
#else /* _WIN32 */
-static int qemu_signal_init(void)
+static int qemu_signal_init(Error **errp)
{
return 0;
}
@@ -148,7 +148,7 @@ int qemu_init_main_loop(Error **errp)
init_clocks(qemu_timer_notify_cb);
-ret = qemu_signal_init();
+ret = qemu_signal_init(errp);
if (ret) {
return ret;
}
Reviewed-by: Markus Armbruster
Thanks again for the review.
Have a nice day
Fei
Re: [Qemu-devel] [PATCH for 3.1 3/4] virtio-net-test: accept variable length argument in pci_test_start()
On 2018-11-29 04:12, Jason Wang wrote:
> This allows flexibility to be reused for all kinds of command line
> used by other tests.
>
> Signed-off-by: Jason Wang
> ---
> tests/virtio-net-test.c | 17 +++--
> 1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/tests/virtio-net-test.c b/tests/virtio-net-test.c
> index 231e7c767e..33d26ab079 100644
> --- a/tests/virtio-net-test.c
> +++ b/tests/virtio-net-test.c
> @@ -51,17 +51,20 @@ static QVirtioPCIDevice *virtio_net_pci_init(QPCIBus
> *bus, int slot)
> return dev;
> }
>
> -static QOSState *pci_test_start(int socket)
> +static QOSState *pci_test_start(const char *cmd, ...)
> {
> QOSState *qs;
> +va_list ap;
> const char *arch = qtest_get_arch();
> -const char *cmd = "-netdev socket,fd=%d,id=hs0 -device "
> - "virtio-net-pci,netdev=hs0";
>
> if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
> -qs = qtest_pc_boot(cmd, socket);
> +va_start(ap, cmd);
> +qs = qtest_pc_vboot(cmd, ap);
> +va_end(ap);
> } else if (strcmp(arch, "ppc64") == 0) {
> -qs = qtest_spapr_boot(cmd, socket);
> +va_start(ap, cmd);
> +qs = qtest_spapr_vboot(cmd, ap);
> +va_end(ap);
> } else {
> g_printerr("virtio-net tests are only available on x86 or ppc64\n");
> exit(EXIT_FAILURE);
> @@ -218,11 +221,13 @@ static void pci_basic(gconstpointer data)
>QVirtQueue *tvq,
>int socket) = data;
> int sv[2], ret;
> +const char *cmd = "-netdev socket,fd=%d,id=hs0 -device "
> + "virtio-net-pci,netdev=hs0";
>
> ret = socketpair(PF_UNIX, SOCK_STREAM, 0, sv);
> g_assert_cmpint(ret, !=, -1);
>
> -qs = pci_test_start(sv[1]);
> +qs = pci_test_start(cmd, sv[1]);
> dev = virtio_net_pci_init(qs->pcibus, PCI_SLOT);
>
> rx = (QVirtQueuePCI *)qvirtqueue_setup(>vdev, qs->alloc, 0);
>
Reviewed-by: Thomas Huth
Re: [Qemu-devel] [PATCH v9 5/8] migration/ram.c: add a notifier chain for precopy
On Thu, Nov 29, 2018 at 01:10:14PM +0800, Peter Xu wrote:
> On Thu, Nov 29, 2018 at 11:40:57AM +0800, Wei Wang wrote:
> > On 11/28/2018 05:32 PM, Peter Xu wrote:
> > >
> > > So what I am worrying here are corner cases where we might forget to
> > > stop the hinting. I'm fabricating one example sequence of events:
> > >
> > >(start migration)
> > >START_MIGRATION
> > >BEFORE_SYNC
> > >AFTER_SYNC
> > >...
> > >BEFORE_SYNC
> > >AFTER_SYNC
> > >(some SaveStateEntry failed rather than RAM, then
> > > migration_detect_error returned MIG_THR_ERR_FATAL so we need to
> > > fail the migration, however when running the previous
> > > ram_save_iterate for RAM's specific SaveStateEntry we didn't see
> > > any error so no ERROR event detected)
> > >
> > > Then it seems the hinting will last forever. Considering that now I'm
> > > not sure whether this can be done ram-only, since even if you capture
> > > ram_save_complete() and at the same time you introduce PRECOPY_END you
> > > may still miss the PRECOPY_END event since AFAIU ram_save_complete()
> > > won't be called at all in this case.
> > >
> > > Could this happen?
> >
> > Thanks, indeed this case could happen if we add PRECOPY_END in
> > ram_save_complete.
> >
> > How about putting PRECOPY_END in ram_save_cleanup?
> > I think it would be called in any case.
>
> Sounds good.
>
> >
> > I'm also thinking probably we don't need PRECOPY_ERR when we have
> > PRECOPY_END,
> > and what do you think of the notifier names below:
> >
> > +typedef enum PrecopyNotifyReason {
> > +PRECOPY_NOTIFY_RAM_SAVE_END = 0,
> > +PRECOPY_NOTIFY_RAM_SAVE_START = 1,
> > +PRECOPY_NOTIFY_RAM_SAVE_BEFORE_SYNC_BITMAP = 2,
> > +PRECOPY_NOTIFY_RAM_SAVE_AFTER_SYNC_BITMAP = 3,
> > +PRECOPY_NOTIFY_RAM_SAVE_MAX = 4,
> > +} PrecopyNotifyReason;
>
> (please see below [1]...)
>
> >
> >
> > >
> > > >
> > > > > [1]
> > > > >
> > > > > > > Another thing to mention about the "reasons" (though I see it more
> > > > > > > like "events"): have you thought about adding a
> > > > > > > PRECOPY_NOTIFY_END?
> > > > > > > It might help in some cases:
> > > > > > >
> > > > > > > - then you don't need to trickily export the
> > > > > > > migrate_postcopy()
> > > > > > >since you'll notify that before postcopy starts
> > > > > > I'm thinking probably we don't need to export migrate_postcopy even
> > > > > > now.
> > > > > > It's more like a sanity check, and not needed because now we have
> > > > > > the
> > > > > > notifier registered to the precopy specific callchain, which has
> > > > > > ensured
> > > > > > that
> > > > > > it is invoked via precopy.
> > > > > But postcopy will always start with precopy, no?
> > > > Yes, but I think we could add the check in precopy_notify()
> > > I'm not sure that's good. If the notifier could potentially have
> > > other user, they might still work with postcopy, and they might expect
> > > e.g. BEFORE_SYNC to be called for every sync, even if it's at the
> > > precopy stage of a postcopy.
> >
> > I think this precopy notifier callchain is expected to be used only for
> > the precopy mode. Postcopy has its dedicated notifier callchain that
> > users could use.
> >
> > How about changing the migrate_postcopy() check to "ms->start_postcopy":
> >
> > bool migration_postcopy_start(void)
> > {
> > MigrationState *s;
> >
> > s = migrate_get_current();
> >
> > return atomic_read(>start_postcopy);
> > }
> >
> >
> > static void precopy_notify(PrecopyNotifyReason reason)
> > {
> > if (migration_postcopy_start())
> > return;
> >
> > notifier_list_notify(_notifier_list, );
> > }
> >
> > If postcopy started with precopy, the precopy optimization feature
> > could still be used until it switches to the postcopy mode.
>
> I'm not sure we can use start_postcopy. It's a variable being set in
> the QMP handler but it does not mean postcopy has started. I'm afraid
> there can be race where it's still precopy but the variable is set so
> event could be missed...
>
> IMHO the problem is not that complicated. How about this proposal:
>
> [1]
>
> typedef enum PrecopyNotifyReason {
> PRECOPY_NOTIFY_RAM_START,
> PRECOPY_NOTIFY_RAM_BEFORE_SYNC,
> PRECOPY_NOTIFY_RAM_AFTER_SYNC,
> PRECOPY_NOTIFY_COMPLETE,
> PRECOPY_NOTIFY_RAM_CLEANUP,
> };
>
> The first three keep the same as your old ones. Notify RAM_CLEANUP in
> ram_save_cleanup() to make sure it'll always be cleaned up (the same
> as PRECOPY_END, just another name). Notify COMPLETE in
> qemu_savevm_state_complete_precopy() to show that precopy is
> completed. Meanwhile on balloon side you should stop the hinting for
> either RAM_CLEANUP or COMPLETE event. Then either:
>
> - precopy is switching to postcopy, or
> - precopy completed, or
> - precopy failed/cancelled
>
> You should always get at least a notification to stop the balloon.
> Though you could also get one
Re: [Qemu-devel] [PATCH v9 5/8] migration/ram.c: add a notifier chain for precopy
On Thu, Nov 29, 2018 at 11:40:57AM +0800, Wei Wang wrote:
> On 11/28/2018 05:32 PM, Peter Xu wrote:
> >
> > So what I am worrying here are corner cases where we might forget to
> > stop the hinting. I'm fabricating one example sequence of events:
> >
> >(start migration)
> >START_MIGRATION
> >BEFORE_SYNC
> >AFTER_SYNC
> >...
> >BEFORE_SYNC
> >AFTER_SYNC
> >(some SaveStateEntry failed rather than RAM, then
> > migration_detect_error returned MIG_THR_ERR_FATAL so we need to
> > fail the migration, however when running the previous
> > ram_save_iterate for RAM's specific SaveStateEntry we didn't see
> > any error so no ERROR event detected)
> >
> > Then it seems the hinting will last forever. Considering that now I'm
> > not sure whether this can be done ram-only, since even if you capture
> > ram_save_complete() and at the same time you introduce PRECOPY_END you
> > may still miss the PRECOPY_END event since AFAIU ram_save_complete()
> > won't be called at all in this case.
> >
> > Could this happen?
>
> Thanks, indeed this case could happen if we add PRECOPY_END in
> ram_save_complete.
>
> How about putting PRECOPY_END in ram_save_cleanup?
> I think it would be called in any case.
Sounds good.
>
> I'm also thinking probably we don't need PRECOPY_ERR when we have
> PRECOPY_END,
> and what do you think of the notifier names below:
>
> +typedef enum PrecopyNotifyReason {
> +PRECOPY_NOTIFY_RAM_SAVE_END = 0,
> +PRECOPY_NOTIFY_RAM_SAVE_START = 1,
> +PRECOPY_NOTIFY_RAM_SAVE_BEFORE_SYNC_BITMAP = 2,
> +PRECOPY_NOTIFY_RAM_SAVE_AFTER_SYNC_BITMAP = 3,
> +PRECOPY_NOTIFY_RAM_SAVE_MAX = 4,
> +} PrecopyNotifyReason;
(please see below [1]...)
>
>
> >
> > >
> > > > [1]
> > > >
> > > > > > Another thing to mention about the "reasons" (though I see it more
> > > > > > like "events"): have you thought about adding a PRECOPY_NOTIFY_END?
> > > > > > It might help in some cases:
> > > > > >
> > > > > > - then you don't need to trickily export the migrate_postcopy()
> > > > > >since you'll notify that before postcopy starts
> > > > > I'm thinking probably we don't need to export migrate_postcopy even
> > > > > now.
> > > > > It's more like a sanity check, and not needed because now we have the
> > > > > notifier registered to the precopy specific callchain, which has
> > > > > ensured
> > > > > that
> > > > > it is invoked via precopy.
> > > > But postcopy will always start with precopy, no?
> > > Yes, but I think we could add the check in precopy_notify()
> > I'm not sure that's good. If the notifier could potentially have
> > other user, they might still work with postcopy, and they might expect
> > e.g. BEFORE_SYNC to be called for every sync, even if it's at the
> > precopy stage of a postcopy.
>
> I think this precopy notifier callchain is expected to be used only for
> the precopy mode. Postcopy has its dedicated notifier callchain that
> users could use.
>
> How about changing the migrate_postcopy() check to "ms->start_postcopy":
>
> bool migration_postcopy_start(void)
> {
> MigrationState *s;
>
> s = migrate_get_current();
>
> return atomic_read(>start_postcopy);
> }
>
>
> static void precopy_notify(PrecopyNotifyReason reason)
> {
> if (migration_postcopy_start())
> return;
>
> notifier_list_notify(_notifier_list, );
> }
>
> If postcopy started with precopy, the precopy optimization feature
> could still be used until it switches to the postcopy mode.
I'm not sure we can use start_postcopy. It's a variable being set in
the QMP handler but it does not mean postcopy has started. I'm afraid
there can be race where it's still precopy but the variable is set so
event could be missed...
IMHO the problem is not that complicated. How about this proposal:
[1]
typedef enum PrecopyNotifyReason {
PRECOPY_NOTIFY_RAM_START,
PRECOPY_NOTIFY_RAM_BEFORE_SYNC,
PRECOPY_NOTIFY_RAM_AFTER_SYNC,
PRECOPY_NOTIFY_COMPLETE,
PRECOPY_NOTIFY_RAM_CLEANUP,
};
The first three keep the same as your old ones. Notify RAM_CLEANUP in
ram_save_cleanup() to make sure it'll always be cleaned up (the same
as PRECOPY_END, just another name). Notify COMPLETE in
qemu_savevm_state_complete_precopy() to show that precopy is
completed. Meanwhile on balloon side you should stop the hinting for
either RAM_CLEANUP or COMPLETE event. Then either:
- precopy is switching to postcopy, or
- precopy completed, or
- precopy failed/cancelled
You should always get at least a notification to stop the balloon.
Though you could also get one RAM_CLEANUP after one COMPLETE, but
the balloon should easily handle it (stop the hinting twice).
Here maybe you can even remove the "RAM_" in both RAM_START and
RAM_CLEANUP if we're going to have COMPLETE since after all it'll be
not only limited to RAM.
Another suggestion is that you can add an Error into the notify hooks,
please refer to the postcopy one:
int
[Qemu-devel] [Bug 1804961] Re: qemu-system-aarch64: Windows 10 ARM64 BSoD on boot while using virt-3.0
And the namespace objects tree of virt-2.12 is this. https://pastebin.com/GybigJ9r -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1804961 Title: qemu-system-aarch64: Windows 10 ARM64 BSoD on boot while using virt-3.0 Status in QEMU: New Bug description: When I emulate a virt-3.0 machine, Windows 10 BSoD on boot, with the error code being ACPI_BIOS_ERROR(0x00A5), virt-2.12 boots fine. Windows Build: 10.0.17134.1 QEMU version: 3.0.0 Commandline: qemu-system-aarch64 -M virt -accel tcg,thread=multi -cpu cortex-a57 -smp 2 -m 2048 -bios QEMU_EFI.fd -device ramfb -device nec-usb-xhci -device usb-kbd -device usb-tablet -hda disk.vhd -vnc :0 By the way, the patch to add DBG2 table discussed here https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg01719.html works (although minor change is required to adapt to the qemu 3.0.0 code), the table is accepted by Windows (Windows require both DBG2 and SPCR to be valid for serial kernel debugging to work), so it may help further diagnosing this issue. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1804961/+subscriptions
Re: [Qemu-devel] [PATCH v2 2/2] hw: vmmouse: drop DEFINE_PROP_PTR()
Markus Armbruster 于2018年11月27日周二 下午8:38写道： > Darren Kenny writes: > > > Hi Li Qiang, > > > > This is only a suggestion, I'm sure someone else might also correct > > me, but I'm not sure the subject above really describes what is > > happening in the commit as a whole. > > > > It seems to miss the point that the main change here is to use a > > link type property, so maybe it might be better written as something > > along the lines of: > > > > Subject: [PATCH v2 2/2] hw: vmmouse: use link property instead of > pointer > > Drop DEFINE_PROP_PTR() and replace with a link pointer via an > > instance init function (vmmouse_instance_initfn). > > Yes, that's better. Or take inspiration from recent similar work, such > as commit a8299ec1b24, and say something like > > vmmouse: Use link instead of pointer property > > According to qdev-properties.h, properties of pointer type should > be avoided. Turn "ps2_mouse" into a link. > > Preferably with the commit message improved along these lines: > Reviewed-by: Markus Armbruster > Hi Darren and Markus, Thanks so much for your advice. A new revision has been sent out. Thanks, Li Qiang > > [...] >
[Qemu-devel] [PATCH v3 2/2] hw: vmmouse: Use link instead of pointer property
According to qdev-properties.h, properties of pointer type should
be avoided. Turn "ps2_mouse" into a link.
Reviewed-by: Markus Armbruster
Reviewed-by: Darren Kenny
Signed-off-by: Li Qiang
---
Change since v2: detailed commit message
Change since v1: use error_abort in object_property_set_link()
hw/i386/pc.c | 3 ++-
hw/i386/vmmouse.c | 17 +++--
2 files changed, 13 insertions(+), 7 deletions(-)
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 73c7b777a0..64f0f233b8 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1552,7 +1552,8 @@ static void pc_superio_init(ISABus *isa_bus, bool
create_fdctrl, bool no_vmport)
}
if (vmmouse) {
DeviceState *dev = DEVICE(vmmouse);
-qdev_prop_set_ptr(dev, "ps2_mouse", i8042);
+object_property_set_link(OBJECT(dev), OBJECT(i8042),
+ "ps2_mouse", _abort);
qdev_init_nofail(dev);
}
port92 = isa_create_simple(isa_bus, TYPE_PORT92);
diff --git a/hw/i386/vmmouse.c b/hw/i386/vmmouse.c
index 4412eaf604..f63aac6673 100644
--- a/hw/i386/vmmouse.c
+++ b/hw/i386/vmmouse.c
@@ -27,6 +27,7 @@
#include "hw/i386/pc.h"
#include "hw/input/i8042.h"
#include "hw/qdev.h"
+#include "qapi/error.h"
/* debug only vmmouse */
//#define DEBUG_VMMOUSE
@@ -271,10 +272,15 @@ static void vmmouse_realizefn(DeviceState *dev, Error
**errp)
vmport_register(VMMOUSE_DATA, vmmouse_ioport_read, s);
}
-static Property vmmouse_properties[] = {
-DEFINE_PROP_PTR("ps2_mouse", VMMouseState, ps2_mouse),
-DEFINE_PROP_END_OF_LIST(),
-};
+static void vmmouse_instance_initfn(Object *obj)
+{
+VMMouseState *s = VMMOUSE(obj);
+
+object_property_add_link(obj, "ps2_mouse", TYPE_I8042,
+ (Object **)>ps2_mouse,
+ qdev_prop_allow_set_link_before_realize,
+ 0, _abort);
+}
static void vmmouse_class_initfn(ObjectClass *klass, void *data)
{
@@ -283,8 +289,6 @@ static void vmmouse_class_initfn(ObjectClass *klass, void
*data)
dc->realize = vmmouse_realizefn;
dc->reset = vmmouse_reset;
dc->vmsd = _vmmouse;
-dc->props = vmmouse_properties;
-/* Reason: pointer property "ps2_mouse" */
dc->user_creatable = false;
}
@@ -292,6 +296,7 @@ static const TypeInfo vmmouse_info = {
.name = TYPE_VMMOUSE,
.parent= TYPE_ISA_DEVICE,
.instance_size = sizeof(VMMouseState),
+.instance_init = vmmouse_instance_initfn,
.class_init= vmmouse_class_initfn,
};
--
2.11.0
[Qemu-devel] [PATCH v3 1/2] hw: pc: use TYPE_XXX instead of constant strings
TYPE_VMMOUSE is defined in vmmouse.c currently, move it
to pc.h in order to use it in pc.c.
Reviewed-by: Darren Kenny
Reviewed-by: Markus Armbruster
Signed-off-by: Li Qiang
---
hw/i386/pc.c | 6 +++---
hw/i386/vmmouse.c| 1 -
include/hw/i386/pc.h | 3 +++
3 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index f095725dba..73c7b777a0 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1543,10 +1543,10 @@ static void pc_superio_init(ISABus *isa_bus, bool
create_fdctrl, bool no_vmport)
fdctrl_init_isa(isa_bus, fd);
}
-i8042 = isa_create_simple(isa_bus, "i8042");
+i8042 = isa_create_simple(isa_bus, TYPE_I8042);
if (!no_vmport) {
vmport_init(isa_bus);
-vmmouse = isa_try_create(isa_bus, "vmmouse");
+vmmouse = isa_try_create(isa_bus, TYPE_VMMOUSE);
} else {
vmmouse = NULL;
}
@@ -1555,7 +1555,7 @@ static void pc_superio_init(ISABus *isa_bus, bool
create_fdctrl, bool no_vmport)
qdev_prop_set_ptr(dev, "ps2_mouse", i8042);
qdev_init_nofail(dev);
}
-port92 = isa_create_simple(isa_bus, "port92");
+port92 = isa_create_simple(isa_bus, TYPE_PORT92);
a20_line = qemu_allocate_irqs(handle_a20_line_change, first_cpu, 2);
i8042_setup_a20_line(i8042, a20_line[0]);
diff --git a/hw/i386/vmmouse.c b/hw/i386/vmmouse.c
index 5d2d278be4..4412eaf604 100644
--- a/hw/i386/vmmouse.c
+++ b/hw/i386/vmmouse.c
@@ -52,7 +52,6 @@
#define DPRINTF(fmt, ...) do { } while (0)
#endif
-#define TYPE_VMMOUSE "vmmouse"
#define VMMOUSE(obj) OBJECT_CHECK(VMMouseState, (obj), TYPE_VMMOUSE)
typedef struct VMMouseState
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 136fe497b6..c708ac9265 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -169,6 +169,9 @@ void gsi_handler(void *opaque, int n, int level);
#define TYPE_VMPORT "vmport"
typedef uint32_t (VMPortReadFunc)(void *opaque, uint32_t address);
+/* vmmouse.c */
+#define TYPE_VMMOUSE "vmmouse"
+
static inline void vmport_init(ISABus *bus)
{
isa_create_simple(bus, TYPE_VMPORT);
--
2.11.0
Re: [Qemu-devel] [PATCH v5 05/36] ppc/xive: introduce the XIVE Event Notification Descriptors
On Fri, Nov 23, 2018 at 12:01:27PM +0100, Cédric Le Goater wrote:
> On 11/23/18 5:35 AM, David Gibson wrote:
> > On Thu, Nov 22, 2018 at 10:47:44PM +0100, Cédric Le Goater wrote:
> >> On 11/22/18 5:41 AM, David Gibson wrote:
> >>> On Fri, Nov 16, 2018 at 11:56:58AM +0100, Cédric Le Goater
> wrote:
[snip]
> +/*
> + * XiveEND helpers
> + */
> +
> +void xive_end_reset(XiveEND *end)
> +{
> +memset(end, 0, sizeof(*end));
> +
> +/* switch off the escalation and notification ESBs */
> +end->w1 = END_W1_ESe_Q | END_W1_ESn_Q;
> >>>
> >>> It's not obvious to me what circumstances this would be called under.
> >>> Since the ENDs are in system memory, a memset() seems like an odd
> >>> thing for (virtual) hardware to be doing to it.
> >>
> >> It makes sense on sPAPR if one day some OS starts using the END ESBs for
> >> further coalescing of the events. None does for now but I have added the
> >> model though.
> >
> > Hrm, I think that belongs in PAPR specific code. It's not really part
> > of the router model - it's the PAPR stuff configuring the router at
> > reset time (much as firmware would configure it at reset time for bare
> > metal).
>
> This is true this routine is only used by the H_INT_RESET hcall and by
> the reset handler of the sPAPR controller model. But it made sense to put
> this END helper routine with the other END routines. Don't you think so ?
Actually, no. In real hardware this would be handled by a different
component - the system firmware would do this setup of the ENDs as it
configures the XIVE. So it makes sense to do this in a separate
component for PAPR as well. In this case that's another piece of qemu
(the spapr stuff) rather than being within the VM, but the difference
isn't important to the END handling itself.
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
[Qemu-devel] [PATCH v3 0/2] hw: vmmouse: use link property instead of DEFINE_PROP_PTR
According https://wiki.qemu.org/Contribute/BiteSizedTasks the 'DEFINE_PROP_PTR' should be replaced by QOM link property. The first patch replace constant strings with TYPE_ and move some definition to pc.h header file so that the second patch can work. Change since v2: detail commit message Change since v1: fix some issues per Markus' review Li Qiang (2): hw: pc: use TYPE_XXX instead of constant strings hw: vmmouse: Use link instead of pointer property hw/i386/pc.c | 9 + hw/i386/vmmouse.c| 18 +++--- include/hw/i386/pc.h | 3 +++ 3 files changed, 19 insertions(+), 11 deletions(-) -- 2.11.0
Re: [Qemu-devel] [PATCH v5 27/36] sysbus: add a sysbus_mmio_unmap() helper
On Fri, Nov 16, 2018 at 11:57:20AM +0100, Cédric Le Goater wrote:
> This will be used to remove the MMIO regions of the POWER9 XIVE
> interrupt controller when the sPAPR machine is reseted.
>
> Signed-off-by: Cédric Le Goater
Reviewed-by: David Gibson
Since the code looks sane.
Hoever, I think using memory_region_set_enabled() would be a better
idea for our purposes than actually adding/deleting the subregion.
> ---
> include/hw/sysbus.h | 1 +
> hw/core/sysbus.c| 10 ++
> 2 files changed, 11 insertions(+)
>
> diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
> index 0b59a3b8d605..bc641984b5da 100644
> --- a/include/hw/sysbus.h
> +++ b/include/hw/sysbus.h
> @@ -92,6 +92,7 @@ qemu_irq sysbus_get_connected_irq(SysBusDevice *dev, int n);
> void sysbus_mmio_map(SysBusDevice *dev, int n, hwaddr addr);
> void sysbus_mmio_map_overlap(SysBusDevice *dev, int n, hwaddr addr,
> int priority);
> +void sysbus_mmio_unmap(SysBusDevice *dev, int n);
> void sysbus_add_io(SysBusDevice *dev, hwaddr addr,
> MemoryRegion *mem);
> MemoryRegion *sysbus_address_space(SysBusDevice *dev);
> diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
> index 7ac36ad3e707..09f202167dcb 100644
> --- a/hw/core/sysbus.c
> +++ b/hw/core/sysbus.c
> @@ -153,6 +153,16 @@ static void sysbus_mmio_map_common(SysBusDevice *dev,
> int n, hwaddr addr,
> }
> }
>
> +void sysbus_mmio_unmap(SysBusDevice *dev, int n)
> +{
> +assert(n >= 0 && n < dev->num_mmio);
> +
> +if (dev->mmio[n].addr != (hwaddr)-1) {
> +memory_region_del_subregion(get_system_memory(),
> dev->mmio[n].memory);
> +dev->mmio[n].addr = (hwaddr)-1;
> +}
> +}
> +
> void sysbus_mmio_map(SysBusDevice *dev, int n, hwaddr addr)
> {
> sysbus_mmio_map_common(dev, n, addr, false, 0);
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH v5 23/36] spapr/xive: add migration support for KVM
On Fri, Nov 16, 2018 at 11:57:16AM +0100, Cédric Le Goater wrote:
> This extends the KVM XIVE models to handle the state synchronization
> with KVM, for the monitor usage and for the migration.
>
> The migration priority of the XIVE interrupt controller sPAPRXive is
> raised for KVM. It operates first and orchestrates the capture
> sequence of the states of all the XIVE models. The XIVE sources are
> masked to quiesce the interrupt flow and a XIVE xync is performed to
> stabilize the OS Event Queues. The state of the ENDs are then captured
> by the XIVE interrupt controller model, sPAPRXive, and the state of
> the thread contexts by the thread interrupt presenter model,
> XiveTCTX. When done, a rollback is performed to restore the sources to
> their initial state.
>
> The sPAPRXive 'post_load' method is called from the sPAPR machine,
> after all XIVE device states have been transfered and loaded. First,
> sPAPRXive restores the XIVE routing tables: ENDT and EAT. Next, are
> restored the thread interrupt context registers and the source PQ
> bits.
>
> The get/set operations rely on their KVM counterpart in the host
> kernel which acts as a proxy for OPAL, the host firmware.
>
> Signed-off-by: Cédric Le Goater
> ---
>
> WIP:
>
> If migration occurs when a VCPU is 'ceded', some the OS event
> notification queues are mapped to the ZERO_PAGE on the receiving
> side. As if the HW had triggered a page fault before the dirty
> page was transferred from the source or as if we were not using
> the correct page table.
>
> include/hw/ppc/spapr_xive.h | 5 +
> include/hw/ppc/xive.h | 3 +
> include/migration/vmstate.h | 1 +
> linux-headers/asm-powerpc/kvm.h | 33 +++
> hw/intc/spapr_xive.c| 32 +++
> hw/intc/spapr_xive_kvm.c| 494
> hw/intc/xive.c | 46 +++
> hw/ppc/spapr_irq.c | 2 +-
> 8 files changed, 615 insertions(+), 1 deletion(-)
>
> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
> index 9c817bb7ae74..d2517c040958 100644
> --- a/include/hw/ppc/spapr_xive.h
> +++ b/include/hw/ppc/spapr_xive.h
> @@ -55,12 +55,17 @@ typedef struct sPAPRXiveClass {
> XiveRouterClass parent_class;
>
> DeviceRealize parent_realize;
> +
> +void (*synchronize_state)(sPAPRXive *xive);
> +int (*pre_save)(sPAPRXive *xsrc);
> +int (*post_load)(sPAPRXive *xsrc, int version_id);
This should go away if the KVM and non-KVM versions are in the same
object.
> } sPAPRXiveClass;
>
> bool spapr_xive_irq_enable(sPAPRXive *xive, uint32_t lisn, bool lsi);
> bool spapr_xive_irq_disable(sPAPRXive *xive, uint32_t lisn);
> void spapr_xive_pic_print_info(sPAPRXive *xive, Monitor *mon);
> qemu_irq spapr_xive_qirq(sPAPRXive *xive, uint32_t lisn);
> +int spapr_xive_post_load(sPAPRXive *xive, int version_id);
>
> /*
> * sPAPR NVT and END indexing helpers
> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
> index 7aaf5a182cb3..c8201462d698 100644
> --- a/include/hw/ppc/xive.h
> +++ b/include/hw/ppc/xive.h
> @@ -309,6 +309,9 @@ typedef struct XiveTCTXClass {
> DeviceClass parent_class;
>
> DeviceRealize parent_realize;
> +
> +void (*synchronize_state)(XiveTCTX *tctx);
> +int (*post_load)(XiveTCTX *tctx, int version_id);
.. and this too.
> } XiveTCTXClass;
>
> /*
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 2b501d04669a..ee2e836cc1c1 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -154,6 +154,7 @@ typedef enum {
> MIG_PRI_PCI_BUS,/* Must happen before IOMMU */
> MIG_PRI_GICV3_ITS, /* Must happen before PCI devices */
> MIG_PRI_GICV3, /* Must happen before the ITS */
> +MIG_PRI_XIVE_IC,/* Must happen before all XIVE models */
Ugh.. explicit priority / order levels are a pretty bad code smell.
Usually migration ordering can be handled by getting the object
heirarchy right. What exactly is the problem you're addessing with
this?
> MIG_PRI_MAX,
> } MigrationPriority;
>
> diff --git a/linux-headers/asm-powerpc/kvm.h b/linux-headers/asm-powerpc/kvm.h
> index f34c971491dd..9d55ade23634 100644
> --- a/linux-headers/asm-powerpc/kvm.h
> +++ b/linux-headers/asm-powerpc/kvm.h
Again, linux-headers need to be split out.
> @@ -480,6 +480,8 @@ struct kvm_ppc_cpu_char {
> #define KVM_REG_PPC_ICP_PPRI_SHIFT 16 /* pending irq priority */
> #define KVM_REG_PPC_ICP_PPRI_MASK 0xff
>
> +#define KVM_REG_PPC_NVT_STATE(KVM_REG_PPC | KVM_REG_SIZE_U256 | 0x8d)
> +
> /* Device control API: PPC-specific devices */
> #define KVM_DEV_MPIC_GRP_MISC1
> #define KVM_DEV_MPIC_BASE_ADDR 0 /* 64-bit */
> @@ -681,10 +683,41 @@ struct kvm_ppc_cpu_char {
> #define KVM_DEV_XIVE_GET_TIMA_FD 2
> #define KVM_DEV_XIVE_VC_BASE 3
>
Re: [Qemu-devel] [PATCH v5 28/36] ppc/xics: introduce a icp_kvm_init() routine
On Fri, Nov 16, 2018 at 11:57:21AM +0100, Cédric Le Goater wrote:
> This routine gathers all the KVM initialization of the XICS KVM
> presenter. It will be useful when the initialization of the KVM XICS
> device is moved to a global routine.
>
> Signed-off-by: Cédric Le Goater
I dislike calling things *_init() because it's not clear which of
qemu's many "init" hooks it belongs with.
> ---
> hw/intc/xics_kvm.c | 29 +++--
> 1 file changed, 19 insertions(+), 10 deletions(-)
>
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index e8fa9a53aeba..efad1b19d821 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -123,11 +123,8 @@ static void icp_kvm_reset(DeviceState *dev)
> icp_set_kvm_state(ICP(dev), 1);
> }
>
> -static void icp_kvm_realize(DeviceState *dev, Error **errp)
> +static void icp_kvm_init(ICPState *icp, Error **errp)
> {
> -ICPState *icp = ICP(dev);
> -ICPStateClass *icpc = ICP_GET_CLASS(icp);
> -Error *local_err = NULL;
> CPUState *cs;
> KVMEnabledICP *enabled_icp;
> unsigned long vcpu_id;
> @@ -137,12 +134,6 @@ static void icp_kvm_realize(DeviceState *dev, Error
> **errp)
> abort();
> }
>
> -icpc->parent_realize(dev, _err);
> -if (local_err) {
> -error_propagate(errp, local_err);
> -return;
> -}
> -
> cs = icp->cs;
> vcpu_id = kvm_arch_vcpu_id(cs);
>
> @@ -168,6 +159,24 @@ static void icp_kvm_realize(DeviceState *dev, Error
> **errp)
> QLIST_INSERT_HEAD(_enabled_icps, enabled_icp, node);
> }
>
> +static void icp_kvm_realize(DeviceState *dev, Error **errp)
> +{
> +ICPStateClass *icpc = ICP_GET_CLASS(dev);
> +Error *local_err = NULL;
> +
> +icpc->parent_realize(dev, _err);
> +if (local_err) {
> +error_propagate(errp, local_err);
> +return;
> +}
> +
> +icp_kvm_init(ICP(dev), _err);
> +if (local_err) {
> +error_propagate(errp, local_err);
> +return;
> +}
> +}
> +
> static void icp_kvm_class_init(ObjectClass *klass, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(klass);
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH v5 34/36] spapr: add KVM support to the 'dual' machine
On Fri, Nov 16, 2018 at 11:57:27AM +0100, Cédric Le Goater wrote:
> The interrupt mode is chosen by the CAS negotiation process and
> activated after a reset to take into account the required changes in
> the machine. This brings new constraints on how the associated KVM IRQ
> device is initialized.
>
> Currently, each model takes care of the initialization of the KVM
> device in their realize method but this is not possible anymore as the
> initialization needs to done globaly when the interrupt mode is known,
> i.e. when machine is reseted. It also means that we need a way to
> delete a KVM device when another mode is chosen.
>
> Also, to support migration, the QEMU objects holding the state to
> transfer should always be available but not necessarily activated.
>
> The overall approach of this proposal is to initialize both interrupt
> mode at the QEMU level and keep the IRQ number space in sync to allow
> switching from one mode to another. For the KVM side of things, the
> whole initialization of the KVM device, sources and presenters, is
> grouped in a single routine. The XICS and XIVE sPAPR IRQ reset
> handlers are modified accordingly to handle the init and delete
> sequences of the KVM device. The post_load handlers also are, to take
> into account a possible change of interrupt mode after transfer.
>
> As KVM is now initialized at reset, we loose the possiblity to
> fallback to the QEMU emulated mode in case of failure and failures
> become fatal to the machine.
>
> Signed-off-by: Cédric Le Goater
> ---
> hw/intc/spapr_xive_kvm.c | 48 +++---
> hw/intc/xics_kvm.c | 18 ++---
> hw/ppc/spapr_irq.c | 86 +---
> 3 files changed, 98 insertions(+), 54 deletions(-)
>
> diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
> index 0672d8bcbc6b..9c7d36f51e3d 100644
> --- a/hw/intc/spapr_xive_kvm.c
> +++ b/hw/intc/spapr_xive_kvm.c
> @@ -148,7 +148,6 @@ static void xive_tctx_kvm_init(XiveTCTX *tctx, Error
> **errp)
>
> static void xive_tctx_kvm_realize(DeviceState *dev, Error **errp)
> {
> -XiveTCTX *tctx = XIVE_TCTX_KVM(dev);
> XiveTCTXClass *xtc = XIVE_TCTX_BASE_GET_CLASS(dev);
> Error *local_err = NULL;
>
> @@ -157,12 +156,6 @@ static void xive_tctx_kvm_realize(DeviceState *dev,
> Error **errp)
> error_propagate(errp, local_err);
> return;
> }
> -
> -xive_tctx_kvm_init(tctx, _err);
> -if (local_err) {
> -error_propagate(errp, local_err);
> -return;
> -}
> }
>
> static void xive_tctx_kvm_class_init(ObjectClass *klass, void *data)
> @@ -222,12 +215,9 @@ static void xive_source_kvm_init(XiveSource *xsrc, Error
> **errp)
>
> static void xive_source_kvm_reset(DeviceState *dev)
> {
> -XiveSource *xsrc = XIVE_SOURCE_KVM(dev);
> XiveSourceClass *xsc = XIVE_SOURCE_BASE_GET_CLASS(dev);
>
> xsc->parent_reset(dev);
> -
> -xive_source_kvm_init(xsrc, _fatal);
> }
>
> /*
> @@ -346,12 +336,6 @@ static void xive_source_kvm_realize(DeviceState *dev,
> Error **errp)
>
> xsrc->qirqs = qemu_allocate_irqs(xive_source_kvm_set_irq, xsrc,
> xsrc->nr_irqs);
> -
> -xive_source_kvm_mmap(xsrc, _err);
> -if (local_err) {
> -error_propagate(errp, local_err);
> -return;
> -}
> }
>
> static void xive_source_kvm_unrealize(DeviceState *dev, Error **errp)
> @@ -823,6 +807,7 @@ void spapr_xive_kvm_init(sPAPRXive *xive, Error **errp)
> {
> Error *local_err = NULL;
> size_t tima_len;
> +CPUState *cs;
>
> if (!kvm_enabled() || !kvmppc_has_cap_xive()) {
> error_setg(errp,
> @@ -850,7 +835,18 @@ void spapr_xive_kvm_init(sPAPRXive *xive, Error **errp)
> return;
> }
>
> -/* Let the XiveSource KVM model handle the mapping for the moment */
> +xive_source_kvm_mmap(>source, _err);
> +if (local_err) {
> +error_propagate(errp, local_err);
> +return;
> +}
> +
> +/* Create the KVM interrupt sources */
> +xive_source_kvm_init(>source, _err);
> +if (local_err) {
> +error_propagate(errp, local_err);
> +return;
> +}
>
> /* TIMA KVM mapping
> *
> @@ -869,6 +865,17 @@ void spapr_xive_kvm_init(sPAPRXive *xive, Error **errp)
>"xive.tima", tima_len, xive->tm_mmap);
> sysbus_init_mmio(SYS_BUS_DEVICE(xive), >tm_mmio);
>
> +/* Connect the presenters to the VCPU */
> +CPU_FOREACH(cs) {
> +PowerPCCPU *cpu = POWERPC_CPU(cs);
> +
> +xive_tctx_kvm_init(XIVE_TCTX_BASE(cpu->intc), _err);
> +if (local_err) {
> +error_propagate(errp, local_err);
> +return;
> +}
> +}
> +
> kvm_kernel_irqchip = true;
> kvm_msi_via_irqfd_allowed = true;
> kvm_gsi_direct_mapping = true;
> @@ -920,16 +927,9 @@ void spapr_xive_kvm_fini(sPAPRXive *xive, Error **errp)
>
> static
Re: [Qemu-devel] [PATCH v5 32/36] spapr/rtas: modify spapr_rtas_register() to remove RTAS handlers
On Fri, Nov 16, 2018 at 11:57:25AM +0100, Cédric Le Goater wrote: > Removing RTAS handlers will become necessary when the new pseries > machine supporting multiple interrupt mode is introduced. I'd prefer this to be done as a separate spapr_rtas_unregister() helper, just to improve greppability. > > Signed-off-by: Cédric Le Goater > --- > hw/ppc/spapr_rtas.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c > index d6a0952154ac..e005d5d08151 100644 > --- a/hw/ppc/spapr_rtas.c > +++ b/hw/ppc/spapr_rtas.c > @@ -404,7 +404,7 @@ void spapr_rtas_register(int token, const char *name, > spapr_rtas_fn fn) > > token -= RTAS_TOKEN_BASE; > > -assert(!rtas_table[token].name); > +assert(!name || !rtas_table[token].name); > > rtas_table[token].name = name; > rtas_table[token].fn = fn; -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [Qemu-devel] [PATCH v5 31/36] spapr/xive: export the spapr_xive_kvm_init() routine
On Fri, Nov 16, 2018 at 11:57:24AM +0100, Cédric Le Goater wrote:
> We will need it to initialize the KVM XIVE device globally from the
> machine when the XIVE interrupt mode is selected.
>
> Signed-off-by: Cédric Le Goater
This is so trivial, I think it's better to fold it into the patch
which uses it.
> ---
> include/hw/ppc/spapr_xive.h | 2 ++
> hw/intc/spapr_xive_kvm.c| 2 +-
> 2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
> index fa7f3d7718da..1d134a681326 100644
> --- a/include/hw/ppc/spapr_xive.h
> +++ b/include/hw/ppc/spapr_xive.h
> @@ -107,4 +107,6 @@ void spapr_xive_mmio_map(sPAPRXive *xive);
> #define TYPE_XIVE_TCTX_KVM "xive-tctx-kvm"
> #define XIVE_TCTX_KVM(obj) OBJECT_CHECK(XiveTCTX, (obj),
> TYPE_XIVE_TCTX_KVM)
>
> +void spapr_xive_kvm_init(sPAPRXive *xive, Error **errp);
> +
> #endif /* PPC_SPAPR_XIVE_H */
> diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
> index b9fee4ea240f..cb2aa6e81274 100644
> --- a/hw/intc/spapr_xive_kvm.c
> +++ b/hw/intc/spapr_xive_kvm.c
> @@ -809,7 +809,7 @@ static void spapr_xive_kvm_instance_init(Object *obj)
>NULL);
> }
>
> -static void spapr_xive_kvm_init(sPAPRXive *xive, Error **errp)
> +void spapr_xive_kvm_init(sPAPRXive *xive, Error **errp)
> {
> Error *local_err = NULL;
> size_t tima_len;
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH v5 33/36] spapr: introduce routines to delete the KVM IRQ device
On Fri, Nov 16, 2018 at 11:57:26AM +0100, Cédric Le Goater wrote:
> If a new interrupt mode is chosen by CAS, the machine generates a
> reset to reconfigure. At this point, the connection with the previous
> KVM device needs to be closed and a new connection needs to opened
> with the KVM device operating the chosen interrupt mode.
>
> New routines are introduced to destroy the XICS and XIVE KVM
> devices. They make use of a new KVM device ioctl which destroys the
> device and also disconnects the IRQ presenters from the VCPUs.
>
> Signed-off-by: Cédric Le Goater
> ---
> include/hw/ppc/spapr_xive.h | 1 +
> include/hw/ppc/xics.h | 1 +
> linux-headers/linux/kvm.h | 2 ++
> hw/intc/spapr_xive_kvm.c| 54 +++
> hw/intc/xics_kvm.c | 57 +
> 5 files changed, 115 insertions(+)
>
> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
> index 1d134a681326..c913c0aed08a 100644
> --- a/include/hw/ppc/spapr_xive.h
> +++ b/include/hw/ppc/spapr_xive.h
> @@ -108,5 +108,6 @@ void spapr_xive_mmio_map(sPAPRXive *xive);
> #define XIVE_TCTX_KVM(obj) OBJECT_CHECK(XiveTCTX, (obj),
> TYPE_XIVE_TCTX_KVM)
>
> void spapr_xive_kvm_init(sPAPRXive *xive, Error **errp);
> +void spapr_xive_kvm_fini(sPAPRXive *xive, Error **errp);
>
> #endif /* PPC_SPAPR_XIVE_H */
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 9958443d1984..a5468c6eb6e3 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -205,6 +205,7 @@ void icp_resend(ICPState *ss);
> typedef struct sPAPRMachineState sPAPRMachineState;
>
> int xics_kvm_init(sPAPRMachineState *spapr, Error **errp);
> +int xics_kvm_fini(sPAPRMachineState *spapr, Error **errp);
> void xics_spapr_init(sPAPRMachineState *spapr);
>
> Object *icp_create(Object *cpu, const char *type, XICSFabric *xi,
> diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
> index 59fa8d8d7f39..b7a74c58d0db 100644
> --- a/linux-headers/linux/kvm.h
> +++ b/linux-headers/linux/kvm.h
linux-headers updates separate.
> @@ -1309,6 +1309,8 @@ struct kvm_s390_ucas_mapping {
> #define KVM_GET_DEVICE_ATTR_IOW(KVMIO, 0xe2, struct kvm_device_attr)
> #define KVM_HAS_DEVICE_ATTR_IOW(KVMIO, 0xe3, struct kvm_device_attr)
>
> +#define KVM_DESTROY_DEVICE _IOWR(KVMIO, 0xf0, struct kvm_create_device)
> +
> /*
> * ioctls for vcpu fds
> */
> diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
> index cb2aa6e81274..0672d8bcbc6b 100644
> --- a/hw/intc/spapr_xive_kvm.c
> +++ b/hw/intc/spapr_xive_kvm.c
> @@ -55,6 +55,16 @@ static void kvm_cpu_enable(CPUState *cs)
> QLIST_INSERT_HEAD(_enabled_cpus, enabled_cpu, node);
> }
>
> +static void kvm_cpu_disable_all(void)
> +{
> +KVMEnabledCPU *enabled_cpu, *next;
> +
> +QLIST_FOREACH_SAFE(enabled_cpu, _enabled_cpus, node, next) {
> +QLIST_REMOVE(enabled_cpu, node);
> +g_free(enabled_cpu);
> +}
> +}
> +
> /*
> * XIVE Thread Interrupt Management context (KVM)
> */
> @@ -864,6 +874,50 @@ void spapr_xive_kvm_init(sPAPRXive *xive, Error **errp)
> kvm_gsi_direct_mapping = true;
> }
>
> +void spapr_xive_kvm_fini(sPAPRXive *xive, Error **errp)
> +{
> +XiveSource *xsrc = >source;
> +struct kvm_create_device xive_destroy_device = {
> +.fd = xive->fd,
> +.type = KVM_DEV_TYPE_XIVE,
> +.flags = 0,
> +};
> +size_t esb_len = (1ull << xsrc->esb_shift) * xsrc->nr_irqs;
> +int rc;
> +
> +/* The KVM XIVE device is not in use */
> +if (xive->fd == -1) {
> +return;
> +}
> +
> +if (!kvm_enabled() || !kvmppc_has_cap_xive()) {
> +error_setg(errp,
> + "IRQ_XIVE capability must be present for KVM XIVE
> device");
> +return;
If we're here, xive->fd, checked above, definitely shouldn't have been
valid, so you can just assert().
> +}
> +
> +/* Clear the KVM mapping */
> +sysbus_mmio_unmap(SYS_BUS_DEVICE(xsrc), 0);
> +munmap(xsrc->esb_mmap, esb_len);
> +sysbus_mmio_unmap(SYS_BUS_DEVICE(xive), 0);
> +munmap(xive->tm_mmap, 4ull << TM_SHIFT);
> +
> +/* Destroy the KVM device. This also clears the VCPU presenters */
> +rc = kvm_vm_ioctl(kvm_state, KVM_DESTROY_DEVICE, _destroy_device);
> +if (rc < 0) {
> +error_setg_errno(errp, -rc, "Error on KVM_DESTROY_DEVICE for XIVE");
> +}
> +close(xive->fd);
> +xive->fd = -1;
> +
> +kvm_kernel_irqchip = false;
> +kvm_msi_via_irqfd_allowed = false;
> +kvm_gsi_direct_mapping = false;
> +
> +/* Clear the local list of presenter (hotplug) */
> +kvm_cpu_disable_all();
> +}
> +
> static void spapr_xive_kvm_realize(DeviceState *dev, Error **errp)
> {
> sPAPRXive *xive = SPAPR_XIVE_KVM(dev);
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index eabc901a4556..a7e3ec32a761 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -50,6 +50,16
Re: [Qemu-devel] [PATCH v5 22/36] spapr/xive: add models for KVM support
On Wed, Nov 28, 2018 at 11:45:46PM +0100, Cédric Le Goater wrote:
> On 11/28/18 6:52 AM, David Gibson wrote:
> > On Fri, Nov 16, 2018 at 11:57:15AM +0100, Cédric Le Goater wrote:
> >> This introduces a set of XIVE models specific to KVM which derive from
> >> the XIVE base models. The interfaces with KVM are a new capability and
> >> a new KVM device for the XIVE native exploitation interrupt mode.
> >>
> >> They handle the initialization of the TIMA and the source ESB memory
> >> regions which have a different type under KVM. These are 'ram device'
> >> memory mappings, similarly to VFIO, exposed to the guest and the
> >> associated VMAs on the host are populated dynamically with the
> >> appropriate pages using a fault handler.
> >>
> >> Signed-off-by: Cédric Le Goater
> >
> > The logic here looks fine, but I think it would be better to activate
> > it with explicit if (kvm) type logic rather than using a subclass.
>
> ok. ARM has taken a different path, the one proposed below, but it should
> be possible to use a "if (kvm)" type logic. There should be less noise
> in the object design.
Yeah, it seemed like a good path when I wrote the XICS code, but
experience with that has led me to decide it wasn't a good idea. I'm
not sure if the ARM people copied that or came up with it on their
own.
[snip]
> >> +/*
> >> + * XIVE Thread Interrupt Management context (KVM)
> >> + */
> >> +
> >> +static void xive_tctx_kvm_init(XiveTCTX *tctx, Error **errp)
> >> +{
> >> +sPAPRXive *xive;
> >> +unsigned long vcpu_id;
> >> +int ret;
> >> +
> >> +/* Check if CPU was hot unplugged and replugged. */
> >> +if (kvm_cpu_is_enabled(tctx->cs)) {
> >> +return;
> >> +}
> >> +
> >> +vcpu_id = kvm_arch_vcpu_id(tctx->cs);
> >> +xive = SPAPR_XIVE_KVM(tctx->xrtr);
> >
> > Is this the first use of tctx->xrtr?
>
> No, the second. the first is the reset_tctx() ops doing the CAM reset.
> But we said that we could remove it.
And I think we can remove it here too. We know this is PAPR specific
so we can go qdev_get_machine() -> PAPR xive object.
I normally don't like using qdev_get_machine(), but I think it's a
little less ugly than including this backlink (and it is sometimes
necessary to deal with the different abstraction boundaries between
qemu and KVM).
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH v5 25/36] spapr: set the interrupt presenter at reset
On Fri, Nov 16, 2018 at 11:57:18AM +0100, Cédric Le Goater wrote:
> Currently, the interrupt presenter of the VPCU is set at realize
> time. Setting it at reset will become useful when the new machine
> supporting both interrupt modes is introduced. In this machine, the
> interrupt mode is chosen at CAS time and activated after a reset.
>
> Signed-off-by: Cédric Le Goater
> ---
> include/hw/ppc/spapr_cpu_core.h | 2 ++
> hw/ppc/spapr_cpu_core.c | 26 ++
> hw/ppc/spapr_irq.c | 11 +++
> 3 files changed, 39 insertions(+)
>
> diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
> index 9e2821e4b31f..fc8ea9021656 100644
> --- a/include/hw/ppc/spapr_cpu_core.h
> +++ b/include/hw/ppc/spapr_cpu_core.h
> @@ -53,4 +53,6 @@ static inline sPAPRCPUState *spapr_cpu_state(PowerPCCPU
> *cpu)
> return (sPAPRCPUState *)cpu->machine_data;
> }
>
> +void spapr_cpu_core_set_intc(PowerPCCPU *cpu, const char *intc_type);
> +
> #endif
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 1811cd48db90..529de0b6b9c8 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -398,3 +398,29 @@ static const TypeInfo spapr_cpu_core_type_infos[] = {
> };
>
> DEFINE_TYPES(spapr_cpu_core_type_infos)
> +
> +typedef struct ForeachFindIntCArgs {
> +const char *intc_type;
> +Object *intc;
> +} ForeachFindIntCArgs;
> +
> +static int spapr_cpu_core_find_intc(Object *child, void *opaque)
> +{
> +ForeachFindIntCArgs *args = opaque;
> +
> +if (object_dynamic_cast(child, args->intc_type)) {
> +args->intc = child;
> +}
> +
> +return args->intc != NULL;
> +}
> +
> +void spapr_cpu_core_set_intc(PowerPCCPU *cpu, const char *intc_type)
> +{
> +ForeachFindIntCArgs args = { intc_type, NULL };
> +
> +object_child_foreach(OBJECT(cpu), spapr_cpu_core_find_intc, );
> +g_assert(args.intc);
We could create some extra links on the cpu to avoid scanning all the
children, but I guess that's a refinement.
Then again.. what do we actually use the cpu->intc pointer for in XIVE
context? I had a feeling because of the different way notifications
are handled we might not ever need to go from a cpu handle to the
associated TCTX.
> +cpu->intc = args.intc;
> +}
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index 984c6d60cd9f..969efad7e6e9 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -218,6 +218,11 @@ static int spapr_irq_post_load_xics(sPAPRMachineState
> *spapr, int version_id)
>
> static void spapr_irq_reset_xics(sPAPRMachineState *spapr, Error **errp)
> {
> +CPUState *cs;
> +
> +CPU_FOREACH(cs) {
> +spapr_cpu_core_set_intc(POWERPC_CPU(cs), spapr->icp_type);
> +}
> }
>
> #define SPAPR_IRQ_XICS_NR_IRQS 0x1000
> @@ -370,6 +375,12 @@ static int spapr_irq_post_load_xive(sPAPRMachineState
> *spapr, int version_id)
>
> static void spapr_irq_reset_xive(sPAPRMachineState *spapr, Error **errp)
> {
> +CPUState *cs;
> +
> +CPU_FOREACH(cs) {
> +spapr_cpu_core_set_intc(POWERPC_CPU(cs), spapr->xive_tctx_type);
> +}
> +
> spapr_xive_mmio_map(spapr->xive);
> }
>
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH v5 24/36] spapr: add a 'reset' method to the sPAPR IRQ backend
On Fri, Nov 16, 2018 at 11:57:17AM +0100, Cédric Le Goater wrote:
> This method will become useful when the new machine supporting both
> interrupt modes, XIVE and XICS, is introduced. In this machine, the
> interrupt mode is chosen by the CAS negotiation process and activated
> after a reset.
>
> For the time being, the only thing that can be done in the XIVE reset
> handler is to map the pages for the TIMA and for the source ESBs.
>
> Signed-off-by: Cédric Le Goater
> ---
> include/hw/ppc/spapr_irq.h | 2 ++
> include/hw/ppc/spapr_xive.h | 1 +
> hw/intc/spapr_xive.c| 4 +---
> hw/ppc/spapr.c | 2 ++
> hw/ppc/spapr_irq.c | 21 +
> 5 files changed, 27 insertions(+), 3 deletions(-)
>
> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
> index 4e36c0984e1a..34128976e21c 100644
> --- a/include/hw/ppc/spapr_irq.h
> +++ b/include/hw/ppc/spapr_irq.h
> @@ -46,6 +46,7 @@ typedef struct sPAPRIrq {
> Object *(*cpu_intc_create)(sPAPRMachineState *spapr, Object *cpu,
> Error **errp);
> int (*post_load)(sPAPRMachineState *spapr, int version_id);
> +void (*reset)(sPAPRMachineState *spapr, Error **errp);
> } sPAPRIrq;
>
> extern sPAPRIrq spapr_irq_xics;
> @@ -57,6 +58,7 @@ int spapr_irq_claim(sPAPRMachineState *spapr, int irq, bool
> lsi, Error **errp);
> void spapr_irq_free(sPAPRMachineState *spapr, int irq, int num);
> qemu_irq spapr_qirq(sPAPRMachineState *spapr, int irq);
> int spapr_irq_post_load(sPAPRMachineState *spapr, int version_id);
> +void spapr_irq_reset(sPAPRMachineState *spapr, Error **errp);
>
> /*
> * XICS legacy routines
> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
> index d2517c040958..fa7f3d7718da 100644
> --- a/include/hw/ppc/spapr_xive.h
> +++ b/include/hw/ppc/spapr_xive.h
> @@ -91,6 +91,7 @@ typedef struct sPAPRMachineState sPAPRMachineState;
> void spapr_xive_hcall_init(sPAPRMachineState *spapr);
> void spapr_dt_xive(sPAPRXive *xive, int nr_servers, void *fdt,
> uint32_t phandle);
> +void spapr_xive_mmio_map(sPAPRXive *xive);
>
> /*
> * XIVE KVM models
> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> index c5c0e063dc33..def43160e12a 100644
> --- a/hw/intc/spapr_xive.c
> +++ b/hw/intc/spapr_xive.c
> @@ -51,7 +51,7 @@ void spapr_xive_pic_print_info(sPAPRXive *xive, Monitor
> *mon)
> }
>
> /* Map the ESB pages and the TIMA pages */
> -static void spapr_xive_mmio_map(sPAPRXive *xive)
> +void spapr_xive_mmio_map(sPAPRXive *xive)
> {
> sysbus_mmio_map(SYS_BUS_DEVICE(>source), 0, xive->vc_base);
> sysbus_mmio_map(SYS_BUS_DEVICE(>end_source), 0, xive->end_base);
> @@ -77,8 +77,6 @@ static void spapr_xive_base_reset(DeviceState *dev)
> for (i = 0; i < xive->nr_ends; i++) {
> xive_end_reset(>endt[i]);
> }
> -
> -spapr_xive_mmio_map(xive);
> }
>
> static void spapr_xive_base_instance_init(Object *obj)
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index d1be2579cd9b..013e6ea8aa64 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1628,6 +1628,8 @@ static void spapr_machine_reset(void)
> spapr_irq_msi_reset(spapr);
> }
>
> +spapr_irq_reset(spapr, _fatal);
> +
> qemu_devices_reset();
>
> /* DRC reset may cause a device to be unplugged. This will cause troubles
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index 6fac6ca70595..984c6d60cd9f 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -13,6 +13,7 @@
> #include "qapi/error.h"
> #include "hw/ppc/spapr.h"
> #include "hw/ppc/spapr_xive.h"
> +#include "hw/ppc/spapr_cpu_core.h"
> #include "hw/ppc/xics.h"
> #include "sysemu/kvm.h"
>
> @@ -215,6 +216,10 @@ static int spapr_irq_post_load_xics(sPAPRMachineState
> *spapr, int version_id)
> return 0;
> }
>
> +static void spapr_irq_reset_xics(sPAPRMachineState *spapr, Error **errp)
> +{
> +}
> +
> #define SPAPR_IRQ_XICS_NR_IRQS 0x1000
> #define SPAPR_IRQ_XICS_NR_MSIS \
> (XICS_IRQ_BASE + SPAPR_IRQ_XICS_NR_IRQS - SPAPR_IRQ_MSI)
> @@ -232,6 +237,7 @@ sPAPRIrq spapr_irq_xics = {
> .dt_populate = spapr_irq_dt_populate_xics,
> .cpu_intc_create = spapr_irq_cpu_intc_create_xics,
> .post_load = spapr_irq_post_load_xics,
> +.reset = spapr_irq_reset_xics,
> };
>
> /*
> @@ -362,6 +368,11 @@ static int spapr_irq_post_load_xive(sPAPRMachineState
> *spapr, int version_id)
> return spapr_xive_post_load(spapr->xive, version_id);
> }
>
> +static void spapr_irq_reset_xive(sPAPRMachineState *spapr, Error **errp)
> +{
> +spapr_xive_mmio_map(spapr->xive);
It's usually not a good idea to actually construct different
MemoryRegion's at run time. Instead map them all in, but disable the
ones you don't want (with memory_region_set_enabled()).
I think your current version will also leave the TIMA etc. still
mapped if you reboot from a XIVE guest to a XICS
Re: [Qemu-devel] [PATCH v5 4/6] nbd/server: implement dirty bitmap export
On 6/9/18 10:17 AM, Vladimir Sementsov-Ogievskiy wrote:
Handle new NBD meta namespace: "qemu", and corresponding queries:
"qemu:dirty-bitmap:".
With new metadata context negotiated, BLOCK_STATUS query will reply
with dirty-bitmap data, converted to extents. New public function
nbd_export_bitmap selects bitmap to export. For now, only one bitmap
may be exported.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
include/block/nbd.h | 6 ++
nbd/server.c| 271
2 files changed, 259 insertions(+), 18 deletions(-)
diff --git a/include/block/nbd.h b/include/block/nbd.h
index fcdcd54502..a653d0fba7 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -234,6 +234,10 @@ enum {
#define NBD_STATE_HOLE (1 << 0)
#define NBD_STATE_ZERO (1 << 1)
+/* Flags for extents (NBDExtent.flags) of NBD_REPLY_TYPE_BLOCK_STATUS,
+ * for qemu:dirty-bitmap:* meta contexts */
+#define NBD_STATE_DIRTY (1 << 0)
Hindsight is 20:20, so there's nothing we can do about this definition
now; commit 3d068aff is already released in 3.0. However, if I had been
more careful when this was first introduced, I would have negated the
sense of this bit. The NBD protocol recommends that a status of all 0
represent the default state, and when it comes to dirty bitmaps, the
safest fallback path is to declare the entire image as dirty (the way
that we have a bit named NBD_STATE_HOLE that is 1 for a hole, but 0 for
data, because the safest fallback path is to treat the entire image as
data).
Why does this matter? Because with the 3.0 hack of qemu-img map with
x-dirty-bitmap=qemu:dirty-bitmap:FOO (commit 216ee365), if the client
requests a particular bitmap name but the server does NOT have that name
present, the client does NOT refuse to connect, but rather acts as
though bdrv_block_status() treats the entire image as data. Since the
hack relies on treating "data":false sections of the disk as dirty, but
the fallback causes qemu-img map to report "data":true for the entire
image, relying on the hack will end up seeing NO clusters as dirty, and
with NO indication that the x-dirty-bitmap= failed to work.
Had we used NBD_STATE_CLEAN (1 for clean, 0 for dirty), we would have at
least had the safe fallback of treating the entire image as dirty, which
would result in a full backup (a bit wasteful, but better than not
backing up any data on the incorrect assumption that the image is clean).
I'm in the process of implementing 'qemu-nbd --list' which will show
exactly which meta contexts an export supports, so that we can use that
as a fallback mechanism for a client to check whether the NBD server is
actually advertising the desired qemu:dirty-bitmap: context prior to
blindly requesting that context via x-dirty-bitmap. But even that hits
a slight snag:
@@ -924,6 +987,16 @@ static int nbd_negotiate_meta_queries(NBDClient *client,
}
}
+if (meta->dirty_bitmap) {
+ret = nbd_negotiate_send_meta_context(client,
+ meta->exp->export_bitmap_context,
+ NBD_META_ID_DIRTY_BITMAP,
+ errp);
+if (ret < 0) {
+return ret;
+}
+}
+
The NBD spec says a client is allowed to issue NBD_OPT_LIST_META_CONTEXT
with 0 queries in order to learn about all supported contexts, but this
hunk forgot to advertise qemu:dirty-bitmap:FOO in that case. My full
patch series for 'qemu-nbd --list' is coming up soon, and includes a
patch that works around the 3.0/3.1 flaw by following up with a second
LIST_META_CONTEXT with 1 query of "qemu:' if the 0-query version didn't
get any "qemu:..." responses.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH] memory: update coalesced_range on transaction_commit
On Wed, 28 Nov 2018 17:30:59 +0100, Paolo Bonzini wrote:
>> The e1000 driver calls memory_region_add_coalescing but
>> kvm_coalesce_mmio_region never been called for those regions.
>>
>> Update coalesced_range on memory_region_transaction_commit to fix this.
>>
>> It seems very old bug since memory region API
>> was merged on commit 093bc2cd885e ("Hierarchical memory region API").
>>
>> Signed-off-by: Atsushi Nemoto
>
> I attach the patch I came up with. What do you think?
Thank you for the alternative fix.
It works well and looks good to me.
---
Atsushi Nemoto
Re: [Qemu-devel] [PATCH for 3.1 1/4] net: drop too large packet early
Jason Wang 于2018年11月29日周四 上午11:12写道：
> We try to detect and drop too large packet (>INT_MAX) in 1592a9947036
> ("net: ignore packet size greater than INT_MAX") during packet
> delivering. Unfortunately, this is not sufficient as we may hit
> another integer overflow when trying to queue such large packet in
> qemu_net_queue_append_iov():
>
> - size of the allocation may overflow on 32bit
> - packet->size is integer which may overflow even on 64bit
>
> Fixing this by move the check to qemu_sendv_packet_async() which is
> the entrance of all networking codes and reduce the limit to
> NET_BUFSIZE to be more conservative.
>
> Cc: qemu-sta...@nongnu.org
> Cc: Li Qiang
> Reported-by: Li Qiang
> Signed-off-by: Jason Wang
>
Looks ok to me.
Reviewed-by: Li Qiang
> ---
> net/net.c | 13 +++--
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/net/net.c b/net/net.c
> index 07c194a8f6..affe1877cf 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -712,15 +712,11 @@ ssize_t qemu_deliver_packet_iov(NetClientState
> *sender,
> void *opaque)
> {
> NetClientState *nc = opaque;
> -size_t size = iov_size(iov, iovcnt);
> int ret;
>
> -if (size > INT_MAX) {
> -return size;
> -}
>
> if (nc->link_down) {
> -return size;
> +return iov_size(iov, iovcnt);
> }
>
> if (nc->receive_disabled) {
> @@ -745,10 +741,15 @@ ssize_t qemu_sendv_packet_async(NetClientState
> *sender,
> NetPacketSent *sent_cb)
> {
> NetQueue *queue;
> +size_t size = iov_size(iov, iovcnt);
> int ret;
>
> +if (size > NET_BUFSIZE) {
> +return size;
> +}
> +
> if (sender->link_down || !sender->peer) {
> -return iov_size(iov, iovcnt);
> +return size;
> }
>
> /* Let filters handle the packet first */
> --
> 2.17.1
>
>
Re: [Qemu-devel] [PATCH v5 08/36] ppc/xive: introduce a simplified XIVE presenter
On Thu, 2018-11-29 at 11:47 +1100, David Gibson wrote: > > 1) read/write accessors which take a word number > > 2) A "get" accessor which copies the whole structure, but "write" > accessor which takes a word number. The asymmetry is a bit ugly, but > it's the non-atomic writeback of the whole structure which I'm most > uncomfortable with. It shouldn't be a big deal though, there are HW facilities to access the structures "atomically" anyway, due to the caching done by the XIVE. > 3) A map/unmap interface which gives you / releases a pointer to the > "live" structure. For powernv that would become > address_space_map()/unmap(). For PAPR it would just be reutn pointer > / no-op.
Re: [Qemu-devel] [PATCH v9 5/8] migration/ram.c: add a notifier chain for precopy
On 11/28/2018 05:32 PM, Peter Xu wrote:
So what I am worrying here are corner cases where we might forget to
stop the hinting. I'm fabricating one example sequence of events:
(start migration)
START_MIGRATION
BEFORE_SYNC
AFTER_SYNC
...
BEFORE_SYNC
AFTER_SYNC
(some SaveStateEntry failed rather than RAM, then
migration_detect_error returned MIG_THR_ERR_FATAL so we need to
fail the migration, however when running the previous
ram_save_iterate for RAM's specific SaveStateEntry we didn't see
any error so no ERROR event detected)
Then it seems the hinting will last forever. Considering that now I'm
not sure whether this can be done ram-only, since even if you capture
ram_save_complete() and at the same time you introduce PRECOPY_END you
may still miss the PRECOPY_END event since AFAIU ram_save_complete()
won't be called at all in this case.
Could this happen?
Thanks, indeed this case could happen if we add PRECOPY_END in
ram_save_complete.
How about putting PRECOPY_END in ram_save_cleanup?
I think it would be called in any case.
I'm also thinking probably we don't need PRECOPY_ERR when we have
PRECOPY_END,
and what do you think of the notifier names below:
+typedef enum PrecopyNotifyReason {
+PRECOPY_NOTIFY_RAM_SAVE_END = 0,
+PRECOPY_NOTIFY_RAM_SAVE_START = 1,
+PRECOPY_NOTIFY_RAM_SAVE_BEFORE_SYNC_BITMAP = 2,
+PRECOPY_NOTIFY_RAM_SAVE_AFTER_SYNC_BITMAP = 3,
+PRECOPY_NOTIFY_RAM_SAVE_MAX = 4,
+} PrecopyNotifyReason;
[1]
Another thing to mention about the "reasons" (though I see it more
like "events"): have you thought about adding a PRECOPY_NOTIFY_END?
It might help in some cases:
- then you don't need to trickily export the migrate_postcopy()
since you'll notify that before postcopy starts
I'm thinking probably we don't need to export migrate_postcopy even now.
It's more like a sanity check, and not needed because now we have the
notifier registered to the precopy specific callchain, which has ensured
that
it is invoked via precopy.
But postcopy will always start with precopy, no?
Yes, but I think we could add the check in precopy_notify()
I'm not sure that's good. If the notifier could potentially have
other user, they might still work with postcopy, and they might expect
e.g. BEFORE_SYNC to be called for every sync, even if it's at the
precopy stage of a postcopy.
I think this precopy notifier callchain is expected to be used only for
the precopy mode. Postcopy has its dedicated notifier callchain that
users could use.
How about changing the migrate_postcopy() check to "ms->start_postcopy":
bool migration_postcopy_start(void)
{
MigrationState *s;
s = migrate_get_current();
return atomic_read(>start_postcopy);
}
static void precopy_notify(PrecopyNotifyReason reason)
{
if (migration_postcopy_start())
return;
notifier_list_notify(_notifier_list, );
}
If postcopy started with precopy, the precopy optimization feature
could still be used until it switches to the postcopy mode.
In that sense I still feel the
PRECOPY_END is better (so contantly call it at the end of precopy, no
matter whether there's another postcopy afterwards). It sounds like a
cleaner interface.
Probably I still haven't got the point how PRECOPY_END could help above yet.
Best,
Wei
[Qemu-devel] [PATCH for 3.1 1/4] net: drop too large packet early
We try to detect and drop too large packet (>INT_MAX) in 1592a9947036
("net: ignore packet size greater than INT_MAX") during packet
delivering. Unfortunately, this is not sufficient as we may hit
another integer overflow when trying to queue such large packet in
qemu_net_queue_append_iov():
- size of the allocation may overflow on 32bit
- packet->size is integer which may overflow even on 64bit
Fixing this by move the check to qemu_sendv_packet_async() which is
the entrance of all networking codes and reduce the limit to
NET_BUFSIZE to be more conservative.
Cc: qemu-sta...@nongnu.org
Cc: Li Qiang
Reported-by: Li Qiang
Signed-off-by: Jason Wang
---
net/net.c | 13 +++--
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/net/net.c b/net/net.c
index 07c194a8f6..affe1877cf 100644
--- a/net/net.c
+++ b/net/net.c
@@ -712,15 +712,11 @@ ssize_t qemu_deliver_packet_iov(NetClientState *sender,
void *opaque)
{
NetClientState *nc = opaque;
-size_t size = iov_size(iov, iovcnt);
int ret;
-if (size > INT_MAX) {
-return size;
-}
if (nc->link_down) {
-return size;
+return iov_size(iov, iovcnt);
}
if (nc->receive_disabled) {
@@ -745,10 +741,15 @@ ssize_t qemu_sendv_packet_async(NetClientState *sender,
NetPacketSent *sent_cb)
{
NetQueue *queue;
+size_t size = iov_size(iov, iovcnt);
int ret;
+if (size > NET_BUFSIZE) {
+return size;
+}
+
if (sender->link_down || !sender->peer) {
-return iov_size(iov, iovcnt);
+return size;
}
/* Let filters handle the packet first */
--
2.17.1
[Qemu-devel] [PATCH for 3.1 0/4] Fix possible OOB during queuing packets
Hi: This series tries to fix a possible OOB during queueing packets through qemu_net_queue_append_iov(). This could happen when it tries to queue a packet whose size is larger than INT_MAX which may lead integer overflow. We've fixed similar issue in the past during qemu_net_queue_deliver_iov() by ignoring large packets there. Let's just move the check earlier to qemu_sendv_packet_async() and reduce the limitation to NET_BUFSIZE. A simple qtest were also added this. Please review. Thanks Jason Wang (4): net: drop too large packet early virtio-net-test: remove unused macro virtio-net-test: accept variable length argument in pci_test_start() virtio-net-test: add large tx buffer test net/net.c | 13 + tests/virtio-net-test.c | 64 - 2 files changed, 64 insertions(+), 13 deletions(-) -- 2.17.1
[Qemu-devel] [PATCH for 3.1 4/4] virtio-net-test: add large tx buffer test
This test tries to build a packet whose size is greater than INT_MAX
which tries to trigger integer overflow in qemu_net_queue_append_iov()
which may result OOB.
Signed-off-by: Jason Wang
---
tests/virtio-net-test.c | 46 +
1 file changed, 46 insertions(+)
diff --git a/tests/virtio-net-test.c b/tests/virtio-net-test.c
index 33d26ab079..09c220c2fa 100644
--- a/tests/virtio-net-test.c
+++ b/tests/virtio-net-test.c
@@ -245,6 +245,51 @@ static void pci_basic(gconstpointer data)
g_free(dev);
qtest_shutdown(qs);
}
+
+static void large_tx(gconstpointer data)
+{
+QVirtioPCIDevice *dev;
+QOSState *qs;
+QVirtQueuePCI *tx, *rx;
+QVirtQueue *vq;
+const char *cmd = "-netdev hubport,id=hp0,hubid=0 "
+ "-device virtio-net-pci,netdev=hp0 ";
+uint64_t req_addr;
+uint32_t free_head;
+size_t alloc_size = UINT_MAX / 64;
+int i;
+
+qs = pci_test_start(cmd);
+dev = virtio_net_pci_init(qs->pcibus, PCI_SLOT);
+
+rx = (QVirtQueuePCI *)qvirtqueue_setup(>vdev, qs->alloc, 0);
+tx = (QVirtQueuePCI *)qvirtqueue_setup(>vdev, qs->alloc, 1);
+
+driver_init(>vdev);
+vq = >vq;
+
+/* Bypass the limitation by pointing several descriptors to a single
+ * smaller area */
+req_addr = guest_alloc(qs->alloc, alloc_size);
+for (i = 0; i < 64; i ++) {
+if (i == 0)
+free_head = qvirtqueue_add(vq, req_addr, alloc_size, false, true);
+else
+qvirtqueue_add(vq, req_addr, alloc_size, false, true);
+}
+qvirtqueue_add(vq, req_addr, alloc_size, false, false);
+qvirtqueue_kick(>vdev, vq, free_head);
+
+qvirtio_wait_used_elem(>vdev, vq, free_head, NULL,
+ QVIRTIO_NET_TIMEOUT_US);
+
+qvirtqueue_cleanup(dev->vdev.bus, >vq, qs->alloc);
+qvirtqueue_cleanup(dev->vdev.bus, >vq, qs->alloc);
+qvirtio_pci_device_disable(dev);
+g_free(dev->pdev);
+g_free(dev);
+qtest_shutdown(qs);
+}
#endif
static void hotplug(void)
@@ -270,6 +315,7 @@ int main(int argc, char **argv)
qtest_add_data_func("/virtio/net/pci/basic", send_recv_test, pci_basic);
qtest_add_data_func("/virtio/net/pci/rx_stop_cont",
stop_cont_test, pci_basic);
+qtest_add_data_func("/virtio/net/pci/large_tx", NULL, large_tx);
#endif
qtest_add_func("/virtio/net/pci/hotplug", hotplug);
--
2.17.1
[Qemu-devel] [PATCH for 3.1 3/4] virtio-net-test: accept variable length argument in pci_test_start()
This allows flexibility to be reused for all kinds of command line
used by other tests.
Signed-off-by: Jason Wang
---
tests/virtio-net-test.c | 17 +++--
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/tests/virtio-net-test.c b/tests/virtio-net-test.c
index 231e7c767e..33d26ab079 100644
--- a/tests/virtio-net-test.c
+++ b/tests/virtio-net-test.c
@@ -51,17 +51,20 @@ static QVirtioPCIDevice *virtio_net_pci_init(QPCIBus *bus,
int slot)
return dev;
}
-static QOSState *pci_test_start(int socket)
+static QOSState *pci_test_start(const char *cmd, ...)
{
QOSState *qs;
+va_list ap;
const char *arch = qtest_get_arch();
-const char *cmd = "-netdev socket,fd=%d,id=hs0 -device "
- "virtio-net-pci,netdev=hs0";
if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
-qs = qtest_pc_boot(cmd, socket);
+va_start(ap, cmd);
+qs = qtest_pc_vboot(cmd, ap);
+va_end(ap);
} else if (strcmp(arch, "ppc64") == 0) {
-qs = qtest_spapr_boot(cmd, socket);
+va_start(ap, cmd);
+qs = qtest_spapr_vboot(cmd, ap);
+va_end(ap);
} else {
g_printerr("virtio-net tests are only available on x86 or ppc64\n");
exit(EXIT_FAILURE);
@@ -218,11 +221,13 @@ static void pci_basic(gconstpointer data)
QVirtQueue *tvq,
int socket) = data;
int sv[2], ret;
+const char *cmd = "-netdev socket,fd=%d,id=hs0 -device "
+ "virtio-net-pci,netdev=hs0";
ret = socketpair(PF_UNIX, SOCK_STREAM, 0, sv);
g_assert_cmpint(ret, !=, -1);
-qs = pci_test_start(sv[1]);
+qs = pci_test_start(cmd, sv[1]);
dev = virtio_net_pci_init(qs->pcibus, PCI_SLOT);
rx = (QVirtQueuePCI *)qvirtqueue_setup(>vdev, qs->alloc, 0);
--
2.17.1
Re: [Qemu-devel] [RFC v2 14/24] riscv: tcg-target: Add branch and jump instructions
On 11/28/18 5:06 PM, Alistair Francis wrote:
> On Wed, Nov 28, 2018 at 12:15 PM Richard Henderson
> wrote:
>>
>> On 11/27/18 1:08 PM, Alistair Francis wrote:
>>> +static inline void tcg_out_goto_long(TCGContext *s, tcg_insn_unit *target)
>>> +{
>>> +ptrdiff_t offset = tcg_pcrel_diff(s, target);
>>> +
>>> +if (offset == sextract64(offset, 0, 26)) {
>>> +tcg_out_opc_jump(s, OPC_JAL, TCG_REG_ZERO, offset);
>>> +} else {
>>> +tcg_out_movi(s, TCG_TYPE_I64, TCG_REG_TMP0, (intptr_t)target);
>>> +tcg_out_opc_jump(s, OPC_JAL, TCG_REG_TMP0, 0);
>>> +}
>>> +}
>>> +
>>> +static void tcg_out_call_int(TCGContext *s, tcg_insn_unit *arg, bool tail)
>>> +{
>>> +TCGReg link = tail ? TCG_REG_ZERO : TCG_REG_RA;
>>> +ptrdiff_t offset = tcg_pcrel_diff(s, arg);
>>> +if (offset == sextract64(offset, 1, 20) << 1) {
>>
>> s/20/26/
>>
>> Seems like there ought to be more shared code between tcg_out_call_int and
>> tcg_out_goto_long, really.
>
> I think tcg_out_goto_long can just be removed and replaced with
> tcg_out_call() right?
Yes, tcg_out_goto_long(s, dest) = tcg_out_call_int(s, dest, true).
r~
[Qemu-devel] [PATCH for 3.1 2/4] virtio-net-test: remove unused macro
Signed-off-by: Jason Wang --- tests/virtio-net-test.c | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/virtio-net-test.c b/tests/virtio-net-test.c index dcb87a8b6e..231e7c767e 100644 --- a/tests/virtio-net-test.c +++ b/tests/virtio-net-test.c @@ -24,7 +24,6 @@ #define PCI_SLOT_HP 0x06 #define PCI_SLOT0x04 -#define PCI_FN 0x00 #define QVIRTIO_NET_TIMEOUT_US (30 * 1000 * 1000) #define VNET_HDR_SIZE sizeof(struct virtio_net_hdr_mrg_rxbuf) -- 2.17.1
Re: [Qemu-devel] [PATCH v5 20/36] spapr: add classes for the XIVE models
On Wed, Nov 28, 2018 at 11:38:50PM +0100, Cédric Le Goater wrote:
> On 11/28/18 6:13 AM, David Gibson wrote:
> > On Fri, Nov 16, 2018 at 11:57:13AM +0100, Cédric Le Goater wrote:
> >> The XIVE models for the QEMU and KVM accelerators will have a lot in
> >> common. Introduce an abstract class for the source, the thread context
> >> and the interrupt controller object to handle the differences in the
> >> object initialization. These classes will also be used to define state
> >> synchronization handlers for the monitor and migration usage.
> >>
> >> This is very much like the XICS models.
> >
> > Yeah.. so I know it's my code, but in hindsight I think making
> > separate subclasses for TCG and KVM was a mistake. The distinction
> > between emulated and KVM version is supposed to be invisible to both
> > guest and (almost) to user, whereas a subclass usually indicates a
> > visibly different device.
>
> so how do you want to model the KVM part ? with a single object and
> kvm_enabled() sections ?
Basically, yes. In practice I think you probably want a helper called
xive_is_kvm() or something, which would evaluate to (kvm_enabled() &&
kvm_irqchip_in_kernel()).
> >> Signed-off-by: Cédric Le Goater
> >> ---
> >> include/hw/ppc/spapr_xive.h | 15 +
> >> include/hw/ppc/xive.h | 30 ++
> >> hw/intc/spapr_xive.c| 86 +++-
> >> hw/intc/xive.c | 109 +---
> >> hw/ppc/spapr_irq.c | 4 +-
> >> 5 files changed, 182 insertions(+), 62 deletions(-)
> >>
> >> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
> >> index 5b3fab192d41..aca2969a09ab 100644
> >> --- a/include/hw/ppc/spapr_xive.h
> >> +++ b/include/hw/ppc/spapr_xive.h
> >> @@ -13,6 +13,10 @@
> >> #include "hw/sysbus.h"
> >> #include "hw/ppc/xive.h"
> >>
> >> +#define TYPE_SPAPR_XIVE_BASE "spapr-xive-base"
> >> +#define SPAPR_XIVE_BASE(obj) \
> >> +OBJECT_CHECK(sPAPRXive, (obj), TYPE_SPAPR_XIVE_BASE)
> >> +
> >> #define TYPE_SPAPR_XIVE "spapr-xive"
> >> #define SPAPR_XIVE(obj) OBJECT_CHECK(sPAPRXive, (obj), TYPE_SPAPR_XIVE)
> >>
> >> @@ -38,6 +42,17 @@ typedef struct sPAPRXive {
> >> MemoryRegion tm_mmio;
> >> } sPAPRXive;
> >>
> >> +#define SPAPR_XIVE_BASE_CLASS(klass) \
> >> + OBJECT_CLASS_CHECK(sPAPRXiveClass, (klass), TYPE_SPAPR_XIVE_BASE)
> >> +#define SPAPR_XIVE_BASE_GET_CLASS(obj) \
> >> + OBJECT_GET_CLASS(sPAPRXiveClass, (obj), TYPE_SPAPR_XIVE_BASE)
> >> +
> >> +typedef struct sPAPRXiveClass {
> >> +XiveRouterClass parent_class;
> >> +
> >> +DeviceRealize parent_realize;
> >> +} sPAPRXiveClass;
> >> +
> >> bool spapr_xive_irq_enable(sPAPRXive *xive, uint32_t lisn, bool lsi);
> >> bool spapr_xive_irq_disable(sPAPRXive *xive, uint32_t lisn);
> >> void spapr_xive_pic_print_info(sPAPRXive *xive, Monitor *mon);
> >> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
> >> index b74eb326dcd1..281ed370121c 100644
> >> --- a/include/hw/ppc/xive.h
> >> +++ b/include/hw/ppc/xive.h
> >> @@ -38,6 +38,10 @@ typedef struct XiveFabricClass {
> >> * XIVE Interrupt Source
> >> */
> >>
> >> +#define TYPE_XIVE_SOURCE_BASE "xive-source-base"
> >> +#define XIVE_SOURCE_BASE(obj) \
> >> +OBJECT_CHECK(XiveSource, (obj), TYPE_XIVE_SOURCE_BASE)
> >> +
> >> #define TYPE_XIVE_SOURCE "xive-source"
> >> #define XIVE_SOURCE(obj) OBJECT_CHECK(XiveSource, (obj), TYPE_XIVE_SOURCE)
> >>
> >> @@ -68,6 +72,18 @@ typedef struct XiveSource {
> >> XiveFabric *xive;
> >> } XiveSource;
> >>
> >> +#define XIVE_SOURCE_BASE_CLASS(klass) \
> >> + OBJECT_CLASS_CHECK(XiveSourceClass, (klass), TYPE_XIVE_SOURCE_BASE)
> >> +#define XIVE_SOURCE_BASE_GET_CLASS(obj) \
> >> + OBJECT_GET_CLASS(XiveSourceClass, (obj), TYPE_XIVE_SOURCE_BASE)
> >> +
> >> +typedef struct XiveSourceClass {
> >> +SysBusDeviceClass parent_class;
> >> +
> >> +DeviceRealize parent_realize;
> >> +DeviceReset parent_reset;
> >> +} XiveSourceClass;
> >> +
> >> /*
> >> * ESB MMIO setting. Can be one page, for both source triggering and
> >> * source management, or two different pages. See below for magic
> >> @@ -253,6 +269,9 @@ void xive_end_pic_print_info(XiveEND *end, uint32_t
> >> end_idx, Monitor *mon);
> >> * XIVE Thread interrupt Management (TM) context
> >> */
> >>
> >> +#define TYPE_XIVE_TCTX_BASE "xive-tctx-base"
> >> +#define XIVE_TCTX_BASE(obj) OBJECT_CHECK(XiveTCTX, (obj),
> >> TYPE_XIVE_TCTX_BASE)
> >> +
> >> #define TYPE_XIVE_TCTX "xive-tctx"
> >> #define XIVE_TCTX(obj) OBJECT_CHECK(XiveTCTX, (obj), TYPE_XIVE_TCTX)
> >>
> >> @@ -278,6 +297,17 @@ typedef struct XiveTCTX {
> >> XiveRouter *xrtr;
> >> } XiveTCTX;
> >>
> >> +#define XIVE_TCTX_BASE_CLASS(klass) \
> >> + OBJECT_CLASS_CHECK(XiveTCTXClass, (klass), TYPE_XIVE_TCTX_BASE)
> >> +#define XIVE_TCTX_BASE_GET_CLASS(obj) \
> >> + OBJECT_GET_CLASS(XiveTCTXClass, (obj), TYPE_XIVE_TCTX_BASE)
> >> +
> >>
[Qemu-devel] [ANNOUNCE] QEMU 3.1.0-rc3 is now available
Hello, On behalf of the QEMU Team, I'd like to announce the availability of the fourth release candidate for the QEMU 3.1 release. This release is meant for testing purposes and should not be used in a production environment. http://download.qemu-project.org/qemu-3.1.0-rc3.tar.xz http://download.qemu-project.org/qemu-3.1.0-rc3.tar.xz.sig A note from the maintainer: If no remaining critical issues crop up, we'll release 3.1.0 next week with no further changes; otherwise we'll do an rc4 and release after that. You can help improve the quality of the QEMU 3.1 release by testing this release and reporting bugs on Launchpad: https://bugs.launchpad.net/qemu/ The release plan, as well a documented known issues for release candidates, are available at: http://wiki.qemu.org/Planning/3.1 Please add entries to the ChangeLog for the 3.1 release below: http://wiki.qemu.org/ChangeLog/3.1 Thank you to everyone involved! Changes since rc2: 4750e1a888: Update version for v3.1.0-rc3 release (Peter Maydell) 3f2f3b33db: target/arm/sve_helper: Fix compilation with clang 3.4 (Thomas Huth) ea066d39ac: hw/arm/aspeed: Fix build issue with clang 3.4 (Thomas Huth) 86100290cb: hostmem: no need to check for host_memory_backend_mr_inited() in alloc() (Marc-André Lureau) 039d4e3df0: scsi: Address spurious clang warning (John Snow) 549b50a31d: vfio-helpers: Fix qemu_vfio_open_pci() crash (Markus Armbruster) 36ea397956: hostmem-memfd: honour share=on/off property (Marc-André Lureau) 15ffb43cbf: MAINTAINERS: Add an entry for the Firmware Configuration (fw_cfg) device (Philippe Mathieu-Daudé) 9681ad3e2b: MAINTAINERS: Add some missing entries related to accelerators (Thomas Huth) e84fcd7f66: target/i386: Generate #UD when applying LOCK to a register destination (Richard Henderson) f1e35acf78: checkpatch: g_test_message does not need a trailing newline (Paolo Bonzini) d4c7e7e7e0: vl.c: remove outdated comment (Li Qiang) 8f1d22d970: vhost-user-bridge: fix recvmsg iovlen (Marc-André Lureau) 353c7d58b9: vl: Improve error message when we can't load fw_cfg from file (Li Qiang) 03fee66fde: vmstate: constify VMStateField (Marc-André Lureau) 5aaac46793: migration: savevm: consult migration blockers (Paolo Bonzini) 56333e69ee: lsi: Reselection needed to remove pending commands from queue (George Kennedy) a8efa60633: cpus: run work items for all vCPUs if single-threaded (Paolo Bonzini) d98f26073b: target/i386: kvm: add VMX migration blocker (Paolo Bonzini) 2264faa55f: hw/virt/arm: Add support for Cortex-A72 in virt (ZhiPeng Lu) 6da021815e: nvme: Fix spurious interrupts (Keith Busch) 330ca111ea: iotests: Test migration with -blockdev (Kevin Wolf) 9e37271f50: block: Don't inactivate children before parents (Kevin Wolf) e1ca8f7e19: qapi: add query-display-options command (Gerd Hoffmann) 933d2d4bf2: usb-host: set ifs.detached as true if kernel driver is not active (linzhecheng) 7ec9106759: audio/hda: fix guest triggerable assert (Gerd Hoffmann) b7ee9e4970: cirrus_vga/migration: update the bank offset before use (Wang Xin) d2e550a828: ps2kbd: default to scan enabled after reset (Hervé Poussineau) 30a759b61a: hw/hyperv: fix NULL dereference with pure-kvm SynIC (Roman Kagan) aec5e9c3a9: kvm: Use KVM_GET_MSR_INDEX_LIST for MSR_IA32_ARCH_CAPABILITIES support (Bandan Das) 58102ce7fb: net: cadence_gem: Remove incorrect assert() (Edgar E. Iglesias) 49154ea0bf: MAINTAINERS: Add an ARM SMMU section (Eric Auger) 123a069ae6: MAINTAINERS: Assign some more files in the hw/arm/ directory (Thomas Huth) 57ac4a7a28: fmops: fix off-by-one in AR_TABLE and DR_TABLE array size (Gerd Hoffmann) 1d20398694: 9p: fix QEMU crash when renaming files (Greg Kurz) 895e4897e2: MAINTAINERS: add missing xtensa patterns (Max Filippov) 6bd858b311: block: Update BlockDriverState.inherits_from on bdrv_drop_intermediate() (Alberto Garcia) 0065c455f9: block: Update BlockDriverState.inherits_from on bdrv_set_backing_hd() (Alberto Garcia) a237dea330: iotests: Enhance 223 to cover multiple bitmap granularities (Eric Blake) ad3a7e4555: nvme: fix bug with PCI IRQ pins on teardown (Logan Gunthorpe) 71a86ddece: nvme: fix CMB endianness confusion (Paolo Bonzini) 2067d39e5e: Revert "nvme: fix oob access issue(CVE-2018-16847)" (Kevin Wolf) 87ad860c62: nvme: fix out-of-bounds access to the CMB (Paolo Bonzini) 6bf7463615: nvme: call blk_drain in NVMe reset code to avoid lockups (Igor Druzhinin) e4c8f2925d: iotests: fix nbd test 233 to work correctly with raw images (Daniel P. Berrangé) 2a3d4331fa: block: Fix update of BDRV_O_AUTO_RDONLY in update_flags_from_options() (Alberto Garcia) 1c7f618f68: scsi-disk: Fix crash if underlying host file or disk returns error (Richard W.M. Jones) 59b5e9bbea: target/xtensa: xtfpga: provide default memory sizes (Max Filippov) 3ecd5a4f19: qemu-img: Fix leak (Max Reitz) f0998879e0: qemu-img: Fix typo (Max Reitz) 155af09d44: iotests: Skip 233 if certtool not installed (Eric Blake) 7e934f5b27: migration/migration.c: Add COLO
Re: [Qemu-devel] [PATCH for-3.2 v4 00/28] Generalize machine compatibility properties
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Subject: [Qemu-devel] [PATCH for-3.2 v4 00/28] Generalize machine compatibility
properties
Message-id: 20181127092801.21777-1-marcandre.lur...@redhat.com
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
dc9253d hostmem: use object id for memory region name with >= 3.1
fe3bf67 hw/arm/virt: add virt-3.2 machine type
31f5bd6 hw/i386: add pc-i440fx-3.2 & pc-q35-3.2
3b0b8e6 machine: add compat-props interface
4a14ed5 arm: replace instance_post_init()
f3d5327 qom: add object_class_get_class_data()
c29b239 qom: teach interfaces to implement post-init
9cc5ad4 qdev-props: call object_apply_global_props()
70e9d46 qdev-props: remove errp from GlobalProperty
0e3d981 qdev-props: convert global_props to GPtrArray
2a2ec6a qdev: all globals are now user-provided
0bc5b7b hw: remove SET_MACHINE_COMPAT
851e70b hw: apply machine compat properties without touching globals
acd5576 hw: apply accel compat properties without touching globals
c4af6bd qom: remove unimplemented class_finalize
db6199f qdev: move qdev_prop_register_global_list() to tests
fae3261 accel: register global_props like machine globals
e86b095 qom: make user_creatable_complete() specific to UserCreatable
758abb5 qom: make interface types abstract
f2b0f9b tests: qdev_prop_check_globals() doesn't return "all_used"
65d13a9 Merge remote-tracking branch
'remotes/pmaydell/tags/pull-target-arm-20181126' into staging
134e216 net: cadence_gem: Remove incorrect assert()
421842e MAINTAINERS: Add an ARM SMMU section
cda0d1d MAINTAINERS: Assign some more files in the hw/arm/ directory
=== OUTPUT BEGIN ===
Checking PATCH 1/24: MAINTAINERS: Assign some more files in the hw/arm/
directory...
Checking PATCH 2/24: MAINTAINERS: Add an ARM SMMU section...
Checking PATCH 3/24: net: cadence_gem: Remove incorrect assert()...
Checking PATCH 4/24: Merge remote-tracking branch
'remotes/pmaydell/tags/pull-target-arm-20181126' into staging...
Checking PATCH 5/24: tests: qdev_prop_check_globals() doesn't return
"all_used"...
Checking PATCH 6/24: qom: make interface types abstract...
Checking PATCH 7/24: qom: make user_creatable_complete() specific to
UserCreatable...
Checking PATCH 8/24: accel: register global_props like machine globals...
Checking PATCH 9/24: qdev: move qdev_prop_register_global_list() to tests...
Checking PATCH 10/24: qom: remove unimplemented class_finalize...
Checking PATCH 11/24: hw: apply accel compat properties without touching
globals...
ERROR: Macros with multiple statements should be enclosed in a do - while loop
#95: FILE: hw/xen/xen-common.c:162:
+#define XEN_COMPAT \
+{ \
+.driver = "migration", \
+.property = "store-global-state", \
+.value = "off", \
+}, \
+{ \
+.driver = "migration", \
+.property = "send-configuration", \
+.value = "off", \
+}, \
+{ \
+.driver = "migration", \
+.property = "send-section-footer", \
+.value = "off", \
+}
WARNING: line over 80 characters
#193: FILE: qom/object.c:373:
+void object_apply_global_props(Object *obj, const GPtrArray *props, Error
**errp)
total: 1 errors, 1 warnings, 170 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 12/24: hw: apply machine compat properties without touching
globals...
Checking PATCH 13/24: hw: remove SET_MACHINE_COMPAT...
Checking PATCH 14/24: qdev: all globals are now user-provided...
Checking PATCH 15/24: qdev-props: convert global_props to GPtrArray...
Checking PATCH 16/24: qdev-props: remove errp from GlobalProperty...
Checking PATCH 17/24: qdev-props: call object_apply_global_props()...
Checking PATCH 18/24: qom: teach interfaces to implement post-init...
Checking PATCH 19/24: qom: add object_class_get_class_data()...
Checking PATCH 20/24: arm: replace instance_post_init()...
Checking PATCH 21/24:
Re: [Qemu-devel] [PATCH for-3.1] hw/arm/virt-acpi-build: Fix SMMUv3 ACPI integration
On 2018/11/29 1:26, Auger Eric wrote:
struct AcpiIortSmmu3 {
ACPI_IORT_NODE_HEADER_DEF
uint64_t base_address;
@@ -639,6 +645,8 @@ struct AcpiIortSmmu3 {
uint32_t pri_gsiv;
uint32_t gerr_gsiv;
uint32_t sync_gsiv;
+ uint32_t pxm;
So if we use this field ,we need to set the flags with
ACPI_IORT_SMMU_V3_PXM_VALID
+ uint32_t id_mapping_index;
And if we use this field, it needs to set the revision to at least 1.
But is it harmful to add those fields in the struct as this patch does?
- in our case ACPI_IORT_SMMU_V3_PXM_VALID flag is not set so the field
value is ignored according to the spec and arm_smmu_v3_set_proximity()
will not do anything.
- SMMU control interrupts are all GSIV based so spec says that deviceID
index is ignored.
So eventually the fact the struct size is changing over the revisions
does not look a problem because the node length is part of the struct
and the offset to the ID array also is part of the structure.
So I could have left the structure as before (because we don't use the
functionalities associated to those fields). But on the other hand it's
good to upgrade the struct according to Rev D now.
So I think the patch is correct, isn't?
Yes, I think it's not harmful but it would be better to add some
comments to explain why we don't increase the revision number ATM.
Thanks,
Shannon
Re: [Qemu-devel] [qemu-web PATCH] Document how to test the site with jekyll locally
> -Original Message- > From: Paolo Bonzini [mailto:pbonz...@redhat.com] > Sent: Thursday, November 29, 2018 0:44 > To: Daniel P. Berrangé ; qemu-devel@nongnu.org > Cc: Thomas Huth ; rai...@macports.org; > alex.ben...@linaro.org; Ning, Yu ; Eric Blake > ; Stefan Hajnoczi > Subject: Re: [qemu-web PATCH] Document how to test the site with jekyll > locally > > [...] > > Anybody (especially non-RH people) disagrees with dual-license CC-BY-SA > 4.0 and GPLv2+? (So that we can copy from blog posts to manuals)? I agree with using this dual license. Thanks, Yu
Re: [Qemu-devel] (no subject)
I suspect the main problem is the blocking call to qemu_main from the UI
thread in the app delegate didFinishLoadingWithOptions if i’m not mistaken
and everything else grows from there. Going to build and run it now, since
I woke up in the middle of the night anyway for reasons unexplainable)
On Thu, 29 Nov 2018 at 02:21, Programmingkid
wrote:
>
> > On Nov 28, 2018, at 2:39 PM, Peter Maydell
> wrote:
> >
> > On Wed, 28 Nov 2018 at 01:12, John Arbuckle
> wrote:
> >>
> >> From af4497f2b161bb4165acb8eee5cae3f2a7ea2227 Mon Sep 17 00:00:00 2001
> >> From: John Arbuckle
> >> Date: Tue, 27 Nov 2018 20:01:20 -0500
> >> Subject: [PATCH] ui/cocoa.m: fix crash due to cocoa_refresh() on Mac OS
> 10.14
> >
> > Something seems to have got the formatting of this patch email
> > wrong -- it's got all this in the body and the actual Subject
> > line of the email is blank.
>
> I don't know what happened.
>
> >
> >> Mac OS 10.14 only wants UI code to be called from the main thread. The
> >> cocoa_refresh() function is called on another thread and this causes a
> >> crash to take place. To fix this problem the cocoa_refresh() code is
> >> called from the main thread only.
> >>
> >> Signed-off-by: John Arbuckle
> >> ---
> >> ui/cocoa.m | 59
> ++-
> >> 1 file changed, 34 insertions(+), 25 deletions(-)
> >
> > I get a compile warning with this patch:
> > /Users/pm215/src/qemu/ui/cocoa.m:1615:23: warning: instance method
> > '-cocoa_refresh' not found (return type defaults to 'id')
> > [-Wobjc-method-access]
> >[[NSApp delegate] cocoa_refresh];
> > ^
>
> This will fix the problem:
>
> static void cocoa_refresh(DisplayChangeListener *dcl)
> {
> QemuCocoaAppController *controller = (QemuCocoaAppController *)[NSApp
> delegate];
> [controller cocoa_refresh];
> }
>
> > To be honest, I'm still confused about what is causing the
> > problems on Mojave. The refresh method should be being called
> > on the main thread even with the code on master -- it's just
> > that that is the iothread and it's running the event pumping
> > code in the refresh callback rather than a standard OSX
> > event loop. I'm hoping that the backtrace with symbols of
> > the Mojave assertion failure will help there, since I
> > can't currently see where refresh gets called from some
> > non-main thread.
>
> This might be a Mojave issue and not a QEMU issue.
> I'm on Mac OS 10.12 so I can't confirm anything.
>
> > I also think that this patch doesn't address the problems
> > of locking that I mention on the discussion thread for
> > Berkus' patch. It also doesn't handle any of the other
> > callbacks from QEMU to the cocoa UI -- surely we need to
> > handle all of them if there is a problem here?
>
> To answer this question I would have to know how my patch
> does on Mac OS 10.14. Does it stop the crashing issue? If
> this patch does fix that problem then I think sticking to
> a simple solution may be the answer. The use of locks may
> not be needed.
>
> > (I have some prototype patches which I've been working
> > on which address the locking problem and also make all
> > the QEMU callbacks run their work on the main thread.
> > I may be able get those into shape to post those next week.)
>
> Please CC me when do release it. I will test it on Mac OS 10.12
> and Mac OS 10.6.
Re: [Qemu-devel] [PATCH v5 16/36] spapr: add hcalls support for the XIVE exploitation interrupt mode
On Wed, Nov 28, 2018 at 11:21:37PM +0100, Cédric Le Goater wrote:
> On 11/28/18 5:25 AM, David Gibson wrote:
> > On Fri, Nov 16, 2018 at 11:57:09AM +0100, Cédric Le Goater wrote:
> >> The different XIVE virtualization structures (sources and event queues)
> >> are configured with a set of Hypervisor calls :
> >>
> >> - H_INT_GET_SOURCE_INFO
> >>
> >>used to obtain the address of the MMIO page of the Event State
> >>Buffer (ESB) entry associated with the source.
> >>
> >> - H_INT_SET_SOURCE_CONFIG
> >>
> >>assigns a source to a "target".
> >>
> >> - H_INT_GET_SOURCE_CONFIG
> >>
> >>determines which "target" and "priority" is assigned to a source
> >>
> >> - H_INT_GET_QUEUE_INFO
> >>
> >>returns the address of the notification management page associated
> >>with the specified "target" and "priority".
> >>
> >> - H_INT_SET_QUEUE_CONFIG
> >>
> >>sets or resets the event queue for a given "target" and "priority".
> >>It is also used to set the notification configuration associated
> >>with the queue, only unconditional notification is supported for
> >>the moment. Reset is performed with a queue size of 0 and queueing
> >>is disabled in that case.
> >>
> >> - H_INT_GET_QUEUE_CONFIG
> >>
> >>returns the queue settings for a given "target" and "priority".
> >>
> >> - H_INT_RESET
> >>
> >>resets all of the guest's internal interrupt structures to their
> >>initial state, losing all configuration set via the hcalls
> >>H_INT_SET_SOURCE_CONFIG and H_INT_SET_QUEUE_CONFIG.
> >>
> >> - H_INT_SYNC
> >>
> >>issue a synchronisation on a source to make sure all notifications
> >>have reached their queue.
> >>
> >> Calls that still need to be addressed :
> >>
> >>H_INT_SET_OS_REPORTING_LINE
> >>H_INT_GET_OS_REPORTING_LINE
> >>
> >> See the code for more documentation on each hcall.
> >>
> >> Signed-off-by: Cédric Le Goater
> >> ---
> >> include/hw/ppc/spapr.h | 15 +-
> >> include/hw/ppc/spapr_xive.h | 6 +
> >> hw/intc/spapr_xive_hcall.c | 892
> >> hw/ppc/spapr_irq.c | 2 +
> >> hw/intc/Makefile.objs | 2 +-
> >> 5 files changed, 915 insertions(+), 2 deletions(-)
> >> create mode 100644 hw/intc/spapr_xive_hcall.c
> >>
> >> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> >> index 1fbc2663e06c..8415faea7b82 100644
> >> --- a/include/hw/ppc/spapr.h
> >> +++ b/include/hw/ppc/spapr.h
> >> @@ -452,7 +452,20 @@ struct sPAPRMachineState {
> >> #define H_INVALIDATE_PID0x378
> >> #define H_REGISTER_PROC_TBL 0x37C
> >> #define H_SIGNAL_SYS_RESET 0x380
> >> -#define MAX_HCALL_OPCODEH_SIGNAL_SYS_RESET
> >> +
> >> +#define H_INT_GET_SOURCE_INFO 0x3A8
> >> +#define H_INT_SET_SOURCE_CONFIG 0x3AC
> >> +#define H_INT_GET_SOURCE_CONFIG 0x3B0
> >> +#define H_INT_GET_QUEUE_INFO0x3B4
> >> +#define H_INT_SET_QUEUE_CONFIG 0x3B8
> >> +#define H_INT_GET_QUEUE_CONFIG 0x3BC
> >> +#define H_INT_SET_OS_REPORTING_LINE 0x3C0
> >> +#define H_INT_GET_OS_REPORTING_LINE 0x3C4
> >> +#define H_INT_ESB 0x3C8
> >> +#define H_INT_SYNC 0x3CC
> >> +#define H_INT_RESET 0x3D0
> >> +
> >> +#define MAX_HCALL_OPCODEH_INT_RESET
> >>
> >> /* The hcalls above are standardized in PAPR and implemented by pHyp
> >> * as well.
> >> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
> >> index 3f65b8f485fd..418511f3dc10 100644
> >> --- a/include/hw/ppc/spapr_xive.h
> >> +++ b/include/hw/ppc/spapr_xive.h
> >> @@ -60,4 +60,10 @@ int spapr_xive_target_to_end(sPAPRXive *xive, uint32_t
> >> target, uint8_t prio,
> >> int spapr_xive_cpu_to_end(sPAPRXive *xive, PowerPCCPU *cpu, uint8_t prio,
> >>uint8_t *out_end_blk, uint32_t *out_end_idx);
> >>
> >> +bool spapr_xive_priority_is_valid(uint8_t priority);
> >
> > AFAICT this could be a local function.
>
> the KVM model uses it also, when collecting state from the KVM device
> to build the QEMU ENDT.
>
> >> +
> >> +typedef struct sPAPRMachineState sPAPRMachineState;
> >> +
> >> +void spapr_xive_hcall_init(sPAPRMachineState *spapr);
> >> +
> >> #endif /* PPC_SPAPR_XIVE_H */
> >> diff --git a/hw/intc/spapr_xive_hcall.c b/hw/intc/spapr_xive_hcall.c
> >> new file mode 100644
> >> index ..52e4e23995f5
> >> --- /dev/null
> >> +++ b/hw/intc/spapr_xive_hcall.c
> >> @@ -0,0 +1,892 @@
> >> +/*
> >> + * QEMU PowerPC sPAPR XIVE interrupt controller model
> >> + *
> >> + * Copyright (c) 2017-2018, IBM Corporation.
> >> + *
> >> + * This code is licensed under the GPL version 2 or later. See the
> >> + * COPYING file in the top-level directory.
> >> + */
> >> +
> >> +#include "qemu/osdep.h"
> >> +#include "qemu/log.h"
> >> +#include "qapi/error.h"
> >> +#include "cpu.h"
> >> +#include "hw/ppc/fdt.h"
> >> +#include "hw/ppc/spapr.h"
> >> +#include "hw/ppc/spapr_xive.h"
> >> +#include "hw/ppc/xive_regs.h"
> >> +#include
Re: [Qemu-devel] [PATCH v5 15/36] spapr: introdude a new machine IRQ backend for XIVE
On Wed, Nov 28, 2018 at 06:16:58PM +0100, Cédric Le Goater wrote:
> On 11/28/18 4:28 AM, David Gibson wrote:
> > On Fri, Nov 16, 2018 at 11:57:08AM +0100, Cédric Le Goater wrote:
> >> The XIVE IRQ backend uses the same layout as the new XICS backend but
> >> covers the full range of the IRQ number space. The IRQ numbers for the
> >> CPU IPIs are allocated at the bottom of this space, below 4K, to
> >> preserve compatibility with XICS which does not use that range.
> >>
> >> This should be enough given that the maximum number of CPUs is 1024
> >> for the sPAPR machine under QEMU. For the record, the biggest POWER8
> >> or POWER9 system has a maximum of 1536 HW threads (16 sockets, 192
> >> cores, SMT8).
> >>
> >> Signed-off-by: Cédric Le Goater
> >> ---
> >> include/hw/ppc/spapr.h | 2 +
> >> include/hw/ppc/spapr_irq.h | 7 ++-
> >> hw/ppc/spapr.c | 2 +-
> >> hw/ppc/spapr_irq.c | 119 -
> >> 4 files changed, 124 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> >> index 6279711fe8f7..1fbc2663e06c 100644
> >> --- a/include/hw/ppc/spapr.h
> >> +++ b/include/hw/ppc/spapr.h
> >> @@ -16,6 +16,7 @@ typedef struct sPAPREventLogEntry sPAPREventLogEntry;
> >> typedef struct sPAPREventSource sPAPREventSource;
> >> typedef struct sPAPRPendingHPT sPAPRPendingHPT;
> >> typedef struct ICSState ICSState;
> >> +typedef struct sPAPRXive sPAPRXive;
> >>
> >> #define HPTE64_V_HPTE_DIRTY 0x0040ULL
> >> #define SPAPR_ENTRY_POINT 0x100
> >> @@ -175,6 +176,7 @@ struct sPAPRMachineState {
> >> const char *icp_type;
> >> int32_t irq_map_nr;
> >> unsigned long *irq_map;
> >> +sPAPRXive *xive;
> >>
> >> bool cmd_line_caps[SPAPR_CAP_NUM];
> >> sPAPRCapabilities def, eff, mig;
> >> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
> >> index 0e9229bf219e..c854ae527808 100644
> >> --- a/include/hw/ppc/spapr_irq.h
> >> +++ b/include/hw/ppc/spapr_irq.h
> >> @@ -13,6 +13,7 @@
> >> /*
> >> * IRQ range offsets per device type
> >> */
> >> +#define SPAPR_IRQ_IPI0x0
> >> #define SPAPR_IRQ_EPOW 0x1000 /* XICS_IRQ_BASE offset */
> >> #define SPAPR_IRQ_HOTPLUG0x1001
> >> #define SPAPR_IRQ_VIO0x1100 /* 256 VIO devices */
> >> @@ -33,7 +34,8 @@ typedef struct sPAPRIrq {
> >> uint32_tnr_irqs;
> >> uint32_tnr_msis;
> >>
> >> -void (*init)(sPAPRMachineState *spapr, int nr_irqs, Error **errp);
> >> +void (*init)(sPAPRMachineState *spapr, int nr_irqs, int nr_servers,
> >> + Error **errp);
> >> int (*claim)(sPAPRMachineState *spapr, int irq, bool lsi, Error
> >> **errp);
> >> void (*free)(sPAPRMachineState *spapr, int irq, int num);
> >> qemu_irq (*qirq)(sPAPRMachineState *spapr, int irq);
> >> @@ -42,8 +44,9 @@ typedef struct sPAPRIrq {
> >>
> >> extern sPAPRIrq spapr_irq_xics;
> >> extern sPAPRIrq spapr_irq_xics_legacy;
> >> +extern sPAPRIrq spapr_irq_xive;
> >>
> >> -void spapr_irq_init(sPAPRMachineState *spapr, Error **errp);
> >> +void spapr_irq_init(sPAPRMachineState *spapr, int nr_servers, Error
> >> **errp);
> >
> > I don't see why nr_servers needs to become a parameter, since it can
> > be derived from spapr within this routine.
>
> ok. This is true. We can use directly xics_max_server_number(spapr).
>
> >> int spapr_irq_claim(sPAPRMachineState *spapr, int irq, bool lsi, Error
> >> **errp);
> >> void spapr_irq_free(sPAPRMachineState *spapr, int irq, int num);
> >> qemu_irq spapr_qirq(sPAPRMachineState *spapr, int irq);
> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >> index e470efe7993c..9f8c19e56e7a 100644
> >> --- a/hw/ppc/spapr.c
> >> +++ b/hw/ppc/spapr.c
> >> @@ -2594,7 +2594,7 @@ static void spapr_machine_init(MachineState *machine)
> >> spapr_set_vsmt_mode(spapr, _fatal);
> >>
> >> /* Set up Interrupt Controller before we create the VCPUs */
> >> -spapr_irq_init(spapr, _fatal);
> >> +spapr_irq_init(spapr, xics_max_server_number(spapr), _fatal);
> >
> > We should rename xics_max_server_number() since it's no longer xics
> > specific.
>
> yes.
>
> >> /* Set up containers for ibm,client-architecture-support negotiated
> >> options
> >> */
> >> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> >> index bac45023..2569ae1bc7f8 100644
> >> --- a/hw/ppc/spapr_irq.c
> >> +++ b/hw/ppc/spapr_irq.c
> >> @@ -12,6 +12,7 @@
> >> #include "qemu/error-report.h"
> >> #include "qapi/error.h"
> >> #include "hw/ppc/spapr.h"
> >> +#include "hw/ppc/spapr_xive.h"
> >> #include "hw/ppc/xics.h"
> >> #include "sysemu/kvm.h"
> >>
> >> @@ -91,7 +92,7 @@ error:
> >> }
> >>
> >> static void spapr_irq_init_xics(sPAPRMachineState *spapr, int nr_irqs,
> >> -Error **errp)
> >> +int nr_servers, Error **errp)
> >> {
> >> MachineState
Re: [Qemu-devel] [PATCH v5 08/36] ppc/xive: introduce a simplified XIVE presenter
On Wed, Nov 28, 2018 at 11:59:58AM +0100, Cédric Le Goater wrote:
> On 11/28/18 12:49 AM, David Gibson wrote:
> > On Fri, Nov 16, 2018 at 11:57:01AM +0100, Cédric Le Goater wrote:
> >> The last sub-engine of the XIVE architecture is the Interrupt
> >> Virtualization Presentation Engine (IVPE). On HW, they share elements,
> >> the Power Bus interface (CQ), the routing table descriptors, and they
> >> can be combined in the same HW logic. We do the same in QEMU and
> >> combine both engines in the XiveRouter for simplicity.
> >
> > Ok, I'm not entirely convinced combining the IVPE and IVRE into a
> > single object is a good idea, but we can probably discuss that once
> > I've read further.
>
> We could introduce a simplified presenter for sPAPR but I am not even
> sure of that as it will get more complex if we support the EBB one day.
I wasn't really thinking about PAPR for this comment.
> >> When the IVRE has completed its job of matching an event source with a
> >> Notification Virtual Target (NVT) to notify, it forwards the event
> >> notification to the IVPE sub-engine. The IVPE scans the thread
> >> interrupt contexts of the Notification Virtual Targets (NVT)
> >> dispatched on the HW processor threads and if a match is found, it
> >> signals the thread. If not, the IVPE escalates the notification to
> >> some other targets and records the notification in a backlog queue.
> >>
> >> The IVPE maintains the thread interrupt context state for each of its
> >> NVTs not dispatched on HW processor threads in the Notification
> >> Virtual Target table (NVTT).
> >>
> >> The model currently only supports single NVT notifications.
> >>
> >> Signed-off-by: Cédric Le Goater
> >> ---
> >> include/hw/ppc/xive.h | 13 +++
> >> include/hw/ppc/xive_regs.h | 22
> >> hw/intc/xive.c | 223 +
> >> 3 files changed, 258 insertions(+)
> >>
> >> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
> >> index 5987f26ddb98..e715a6c6923d 100644
> >> --- a/include/hw/ppc/xive.h
> >> +++ b/include/hw/ppc/xive.h
> >> @@ -197,6 +197,10 @@ typedef struct XiveRouterClass {
> >> XiveEND *end);
> >> int (*set_end)(XiveRouter *xrtr, uint8_t end_blk, uint32_t end_idx,
> >> XiveEND *end);
> >> +int (*get_nvt)(XiveRouter *xrtr, uint8_t nvt_blk, uint32_t nvt_idx,
> >> + XiveNVT *nvt);
> >> +int (*set_nvt)(XiveRouter *xrtr, uint8_t nvt_blk, uint32_t nvt_idx,
> >> + XiveNVT *nvt);
> >
> > As with the ENDs, I don't think get/set is a good interface for a
> > bigger-than-word-size object.
>
> We need to agree on this interface before I respin. So you would like
> to add a extra argument specifying the word being accessed ?
Yes. Ok, 3 options I can see at this point:
1) read/write accessors which take a word number
2) A "get" accessor which copies the whole structure, but "write"
accessor which takes a word number. The asymmetry is a bit ugly, but
it's the non-atomic writeback of the whole structure which I'm most
uncomfortable with.
3) A map/unmap interface which gives you / releases a pointer to the
"live" structure. For powernv that would become
address_space_map()/unmap(). For PAPR it would just be reutn pointer
/ no-op.
>
> >
> >> } XiveRouterClass;
> >>
> >> void xive_eas_pic_print_info(XiveEAS *eas, uint32_t lisn, Monitor *mon);
> >> @@ -207,6 +211,10 @@ int xive_router_get_end(XiveRouter *xrtr, uint8_t
> >> end_blk, uint32_t end_idx,
> >> XiveEND *end);
> >> int xive_router_set_end(XiveRouter *xrtr, uint8_t end_blk, uint32_t
> >> end_idx,
> >> XiveEND *end);
> >> +int xive_router_get_nvt(XiveRouter *xrtr, uint8_t nvt_blk, uint32_t
> >> nvt_idx,
> >> +XiveNVT *nvt);
> >> +int xive_router_set_nvt(XiveRouter *xrtr, uint8_t nvt_blk, uint32_t
> >> nvt_idx,
> >> +XiveNVT *nvt);
> >>
> >> /*
> >> * XIVE END ESBs
> >> @@ -274,4 +282,9 @@ extern const MemoryRegionOps xive_tm_ops;
> >>
> >> void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon);
> >>
> >> +static inline uint32_t xive_tctx_cam_line(uint8_t nvt_blk, uint32_t
> >> nvt_idx)
> >> +{
> >> +return (nvt_blk << 19) | nvt_idx;
> >
> > I'm guessing this formula is the standard way of combining the NVT
> > block and index into a single word?
>
> That number is the VP/NVT identifier which is written in the CAM value.
> The index is on 19 bits because of the NVT definition in the END
> structure. It is being increased to 24 bits on Power10
>
> > If so, I think we should
> > standardize on passing a single word "nvt_id" around and only
> > splitting it when we need to use the block separately.
>
> This is really the only place where we concatenate the two NVT values,
> block and index.
Hm, ok. I know we don't model them (yet, maybe ever) but could
combined values appear in the PowerBUS
Re: [Qemu-devel] [PATCH v5 11/36] spapr/xive: use the VCPU id as a NVT identifier
On Wed, Nov 28, 2018 at 05:48:32PM +0100, Cédric Le Goater wrote:
> On 11/28/18 3:39 AM, David Gibson wrote:
> > On Fri, Nov 16, 2018 at 11:57:04AM +0100, Cédric Le Goater wrote:
> >> The IVPE scans the O/S CAM line of the XIVE thread interrupt contexts
> >> to find a matching Notification Virtual Target (NVT) among the NVTs
> >> dispatched on the HW processor threads.
> >>
> >> On a real system, the thread interrupt contexts are updated by the
> >> hypervisor when a Virtual Processor is scheduled to run on a HW
> >> thread. Under QEMU, the model emulates the same behavior by hardwiring
> >> the NVT identifier in the thread context registers at reset.
> >>
> >> The NVT identifier used by the sPAPRXive model is the VCPU id. The END
> >> identifier is also derived from the VCPU id. A set of helpers doing
> >> the conversion between identifiers are provided for the hcalls
> >> configuring the sources and the ENDs.
> >>
> >> The model does not need a NVT table but The XiveRouter NVT operations
> >> are provided to perform some extra checks in the routing algorithm.
> >>
> >> Signed-off-by: Cédric Le Goater
> >> ---
> >> include/hw/ppc/spapr_xive.h | 17 +
> >> include/hw/ppc/xive.h | 3 +
> >> hw/intc/spapr_xive.c| 136
> >> hw/intc/xive.c | 9 +++
> >> 4 files changed, 165 insertions(+)
> >>
> >> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
> >> index 06727bd86aa9..3f65b8f485fd 100644
> >> --- a/include/hw/ppc/spapr_xive.h
> >> +++ b/include/hw/ppc/spapr_xive.h
> >> @@ -43,4 +43,21 @@ bool spapr_xive_irq_disable(sPAPRXive *xive, uint32_t
> >> lisn);
> >> void spapr_xive_pic_print_info(sPAPRXive *xive, Monitor *mon);
> >> qemu_irq spapr_xive_qirq(sPAPRXive *xive, uint32_t lisn);
> >>
> >> +/*
> >> + * sPAPR NVT and END indexing helpers
> >> + */
> >> +uint32_t spapr_xive_nvt_to_target(sPAPRXive *xive, uint8_t nvt_blk,
> >> + uint32_t nvt_idx);
> >> +int spapr_xive_target_to_nvt(sPAPRXive *xive, uint32_t target,
> >> +uint8_t *out_nvt_blk, uint32_t *out_nvt_idx);
> >> +int spapr_xive_cpu_to_nvt(sPAPRXive *xive, PowerPCCPU *cpu,
> >> + uint8_t *out_nvt_blk, uint32_t *out_nvt_idx);
> >> +
> >> +int spapr_xive_end_to_target(sPAPRXive *xive, uint8_t end_blk, uint32_t
> >> end_idx,
> >> + uint32_t *out_server, uint8_t *out_prio);
> >> +int spapr_xive_target_to_end(sPAPRXive *xive, uint32_t target, uint8_t
> >> prio,
> >> + uint8_t *out_end_blk, uint32_t *out_end_idx);
> >> +int spapr_xive_cpu_to_end(sPAPRXive *xive, PowerPCCPU *cpu, uint8_t prio,
> >> + uint8_t *out_end_blk, uint32_t *out_end_idx);
> >> +
> >> #endif /* PPC_SPAPR_XIVE_H */
> >> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
> >> index e715a6c6923d..e6931ddaa83f 100644
> >> --- a/include/hw/ppc/xive.h
> >> +++ b/include/hw/ppc/xive.h
> >> @@ -187,6 +187,8 @@ typedef struct XiveRouter {
> >> #define XIVE_ROUTER_GET_CLASS(obj) \
> >> OBJECT_GET_CLASS(XiveRouterClass, (obj), TYPE_XIVE_ROUTER)
> >>
> >> +typedef struct XiveTCTX XiveTCTX;
> >> +
> >> typedef struct XiveRouterClass {
> >> SysBusDeviceClass parent;
> >>
> >> @@ -201,6 +203,7 @@ typedef struct XiveRouterClass {
> >> XiveNVT *nvt);
> >> int (*set_nvt)(XiveRouter *xrtr, uint8_t nvt_blk, uint32_t nvt_idx,
> >> XiveNVT *nvt);
> >> +void (*reset_tctx)(XiveRouter *xrtr, XiveTCTX *tctx);
> >> } XiveRouterClass;
> >>
> >> void xive_eas_pic_print_info(XiveEAS *eas, uint32_t lisn, Monitor *mon);
> >> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> >> index 5d038146c08e..3bf77ace11a2 100644
> >> --- a/hw/intc/spapr_xive.c
> >> +++ b/hw/intc/spapr_xive.c
> >> @@ -199,6 +199,139 @@ static int spapr_xive_set_end(XiveRouter *xrtr,
> >> return 0;
> >> }
> >>
> >> +static int spapr_xive_get_nvt(XiveRouter *xrtr,
> >> + uint8_t nvt_blk, uint32_t nvt_idx, XiveNVT
> >> *nvt)
> >> +{
> >> +sPAPRXive *xive = SPAPR_XIVE(xrtr);
> >> +uint32_t vcpu_id = spapr_xive_nvt_to_target(xive, nvt_blk, nvt_idx);
> >> +PowerPCCPU *cpu = spapr_find_cpu(vcpu_id);
> >> +
> >> +if (!cpu) {
> >> +return -1;
> >> +}
> >> +
> >> +/*
> >> + * sPAPR does not maintain a NVT table. Return that the NVT is
> >> + * valid if we have found a matching CPU
> >> + */
> >> +nvt->w0 = NVT_W0_VALID;
> >> +return 0;
> >> +}
> >> +
> >> +static int spapr_xive_set_nvt(XiveRouter *xrtr,
> >> + uint8_t nvt_blk, uint32_t nvt_idx, XiveNVT
> >> *nvt)
> >> +{
> >> +/* no NVT table */
> >> +return 0;
> >> +}
> >> +
> >> +/*
> >> + * When a Virtual Processor is scheduled to run on a HW thread, the
> >> + * hypervisor pushes its identifier in
Re: [Qemu-devel] [Qemu-ppc] [PATCH v5 12/36] spapr: initialize VSMT before initializing the IRQ backend
On Wed, Nov 28, 2018 at 10:35:51AM +0100, Greg Kurz wrote:
> On Wed, 28 Nov 2018 13:57:14 +1100
> David Gibson wrote:
>
> > On Fri, Nov 16, 2018 at 11:57:05AM +0100, Cédric Le Goater wrote:
> > > We will need to use xics_max_server_number() to create the sPAPRXive
> > > object modeling the interrupt controller of the machine which is
> > > created before the CPUs.
> > >
> > > Signed-off-by: Cédric Le Goater
> >
> > My only concern here is that this moves the spapr_set_vsmt_mode()
> > before some of the sanity checks in spapr_init_cpus(). Are we certain
> > there are no edge cases that could cause badness?
> >
>
> The early checks in spapr_init_cpus() filter out topologies that would
> result in partially filled cores. They're only related to the rest of
> the code that creates the boot CPUs. Before commit 1a5008fc17,
> spapr_set_vsmt_mode() was even being called before spapr_init_cpus().
> The rationale to move it there was to ensure it is called before the
> first user of spapr->vsmt, which happens to be a call to
> xics_max_server_number().
Ok.
> Now that xics_max_server_number() needs to be called even earlier, I think a
> better change is to have xics_max_server_number() to call
> spapr_set_vsmt_mode()
> if spapr->vsmt isn't set.
I'd rather not do that, but instead move it statically to where it
needs to be. That sort of lazy/on-demand initialization can result in
really confusing behaviours depending on when a seemingly innocuous
data-returning function is called, so I consider it a code smell.
>
> > > ---
> > > hw/ppc/spapr.c | 10 +-
> > > 1 file changed, 5 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 7afd1a175bf2..50cb9f9f4a02 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -2466,11 +2466,6 @@ static void spapr_init_cpus(sPAPRMachineState
> > > *spapr)
> > > boot_cores_nr = possible_cpus->len;
> > > }
> > >
> > > -/* VSMT must be set in order to be able to compute VCPU ids, ie to
> > > - * call xics_max_server_number() or spapr_vcpu_id().
> > > - */
> > > -spapr_set_vsmt_mode(spapr, _fatal);
> > > -
> > > if (smc->pre_2_10_has_unused_icps) {
> > > int i;
> > >
> > > @@ -2593,6 +2588,11 @@ static void spapr_machine_init(MachineState
> > > *machine)
> > > /* Setup a load limit for the ramdisk leaving room for SLOF and FDT
> > > */
> > > load_limit = MIN(spapr->rma_size, RTAS_MAX_ADDR) - FW_OVERHEAD;
> > >
> > > +/* VSMT must be set in order to be able to compute VCPU ids, ie to
> > > + * call xics_max_server_number() or spapr_vcpu_id().
> > > + */
> > > +spapr_set_vsmt_mode(spapr, _fatal);
> > > +
> > > /* Set up Interrupt Controller before we create the VCPUs */
> > > smc->irq->init(spapr, _fatal);
> > >
> >
>
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH v5 10/36] spapr/xive: introduce a XIVE interrupt controller
On Wed, Nov 28, 2018 at 05:27:29PM +0100, Cédric Le Goater wrote:
> On 11/28/18 1:52 AM, David Gibson wrote:
> > On Fri, Nov 16, 2018 at 11:57:03AM +0100, Cédric Le Goater wrote:
> >> sPAPRXive models the XIVE interrupt controller of the sPAPR machine.
> >> It inherits from the XiveRouter and provisions storage for the routing
> >> tables :
> >>
> >> - Event Assignment Structure (EAS)
> >> - Event Notification Descriptor (END)
> >>
> >> The sPAPRXive model incorporates an internal XiveSource for the IPIs
> >> and for the interrupts of the virtual devices of the guest. This model
> >> is consistent with XIVE architecture which also incorporates an
> >> internal IVSE for IPIs and accelerator interrupts in the IVRE
> >> sub-engine.
> >>
> >> The sPAPRXive model exports two memory regions, one for the ESB
> >> trigger and management pages used to control the sources and one for
> >> the TIMA pages. They are mapped by default at the addresses found on
> >> chip 0 of a baremetal system. This is also consistent with the XIVE
> >> architecture which defines a Virtualization Controller BAR for the
> >> internal IVSE ESB pages and a Thread Managment BAR for the TIMA.
> >>
> >> Signed-off-by: Cédric Le Goater
> >> ---
> >> default-configs/ppc64-softmmu.mak | 1 +
> >> include/hw/ppc/spapr_xive.h | 46 +
> >> hw/intc/spapr_xive.c | 323 ++
> >> hw/intc/Makefile.objs | 1 +
> >> 4 files changed, 371 insertions(+)
> >> create mode 100644 include/hw/ppc/spapr_xive.h
> >> create mode 100644 hw/intc/spapr_xive.c
> >>
> >> diff --git a/default-configs/ppc64-softmmu.mak
> >> b/default-configs/ppc64-softmmu.mak
> >> index 2d1e7c5c4668..7f34ad0528ed 100644
> >> --- a/default-configs/ppc64-softmmu.mak
> >> +++ b/default-configs/ppc64-softmmu.mak
> >> @@ -17,6 +17,7 @@ CONFIG_XICS=$(CONFIG_PSERIES)
> >> CONFIG_XICS_SPAPR=$(CONFIG_PSERIES)
> >> CONFIG_XICS_KVM=$(call land,$(CONFIG_PSERIES),$(CONFIG_KVM))
> >> CONFIG_XIVE=$(CONFIG_PSERIES)
> >> +CONFIG_XIVE_SPAPR=$(CONFIG_PSERIES)
> >> CONFIG_MEM_DEVICE=y
> >> CONFIG_DIMM=y
> >> CONFIG_SPAPR_RNG=y
> >> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
> >> new file mode 100644
> >> index ..06727bd86aa9
> >> --- /dev/null
> >> +++ b/include/hw/ppc/spapr_xive.h
> >> @@ -0,0 +1,46 @@
> >> +/*
> >> + * QEMU PowerPC sPAPR XIVE interrupt controller model
> >> + *
> >> + * Copyright (c) 2017-2018, IBM Corporation.
> >> + *
> >> + * This code is licensed under the GPL version 2 or later. See the
> >> + * COPYING file in the top-level directory.
> >> + */
> >> +
> >> +#ifndef PPC_SPAPR_XIVE_H
> >> +#define PPC_SPAPR_XIVE_H
> >> +
> >> +#include "hw/sysbus.h"
> >> +#include "hw/ppc/xive.h"
> >> +
> >> +#define TYPE_SPAPR_XIVE "spapr-xive"
> >> +#define SPAPR_XIVE(obj) OBJECT_CHECK(sPAPRXive, (obj), TYPE_SPAPR_XIVE)
> >> +
> >> +typedef struct sPAPRXive {
> >> +XiveRouterparent;
> >> +
> >> +/* Internal interrupt source for IPIs and virtual devices */
> >> +XiveSourcesource;
> >> +hwaddrvc_base;
> >> +
> >> +/* END ESB MMIOs */
> >> +XiveENDSource end_source;
> >> +hwaddrend_base;
> >> +
> >> +/* Routing table */
> >> +XiveEAS *eat;
> >> +uint32_t nr_irqs;
> >> +XiveEND *endt;
> >> +uint32_t nr_ends;
> >> +
> >> +/* TIMA mapping address */
> >> +hwaddrtm_base;
> >> +MemoryRegion tm_mmio;
> >> +} sPAPRXive;
> >> +
> >> +bool spapr_xive_irq_enable(sPAPRXive *xive, uint32_t lisn, bool lsi);
> >> +bool spapr_xive_irq_disable(sPAPRXive *xive, uint32_t lisn);
> >> +void spapr_xive_pic_print_info(sPAPRXive *xive, Monitor *mon);
> >> +qemu_irq spapr_xive_qirq(sPAPRXive *xive, uint32_t lisn);
> >> +
> >> +#endif /* PPC_SPAPR_XIVE_H */
> >> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> >> new file mode 100644
> >> index ..5d038146c08e
> >> --- /dev/null
> >> +++ b/hw/intc/spapr_xive.c
> >> @@ -0,0 +1,323 @@
> >> +/*
> >> + * QEMU PowerPC sPAPR XIVE interrupt controller model
> >> + *
> >> + * Copyright (c) 2017-2018, IBM Corporation.
> >> + *
> >> + * This code is licensed under the GPL version 2 or later. See the
> >> + * COPYING file in the top-level directory.
> >> + */
> >> +
> >> +#include "qemu/osdep.h"
> >> +#include "qemu/log.h"
> >> +#include "qapi/error.h"
> >> +#include "target/ppc/cpu.h"
> >> +#include "sysemu/cpus.h"
> >> +#include "monitor/monitor.h"
> >> +#include "hw/ppc/spapr.h"
> >> +#include "hw/ppc/spapr_xive.h"
> >> +#include "hw/ppc/xive.h"
> >> +#include "hw/ppc/xive_regs.h"
> >> +
> >> +/*
> >> + * XIVE Virtualization Controller BAR and Thread Managment BAR that we
> >> + * use for the ESB pages and the TIMA pages
> >> + */
> >> +#define SPAPR_XIVE_VC_BASE 0x00060100ull
> >> +#define SPAPR_XIVE_TM_BASE 0x000603020318ull
> >> +
> >> +void spapr_xive_pic_print_info(sPAPRXive *xive, Monitor *mon)
> >> +{
> >> +
Re: [Qemu-devel] [PATCH v5 09/36] ppc/xive: notify the CPU when the interrupt priority is more privileged
On Wed, Nov 28, 2018 at 12:30:45PM +0100, Cédric Le Goater wrote:
> On 11/28/18 1:13 AM, David Gibson wrote:
> > On Fri, Nov 16, 2018 at 11:57:02AM +0100, Cédric Le Goater wrote:
> >> After the event data was pushed in the O/S Event Queue, the IVPE
> >> raises the bit corresponding to the priority of the pending interrupt
> >> in the register IBP (Interrupt Pending Buffer) to indicate there is an
> >> event pending in one of the 8 priority queues. The Pending Interrupt
> >> Priority Register (PIPR) is also updated using the IPB. This register
> >> represent the priority of the most favored pending notification.
> >>
> >> The PIPR is then compared to the the Current Processor Priority
> >> Register (CPPR). If it is more favored (numerically less than), the
> >> CPU interrupt line is raised and the EO bit of the Notification Source
> >> Register (NSR) is updated to notify the presence of an exception for
> >> the O/S. The check needs to be done whenever the PIPR or the CPPR are
> >> changed.
> >>
> >> The O/S acknowledges the interrupt with a special load in the Thread
> >> Interrupt Management Area. If the EO bit of the NSR is set, the CPPR
> >> takes the value of PIPR. The bit number in the IBP corresponding to
> >> the priority of the pending interrupt is reseted and so is the EO bit
> >> of the NSR.
> >>
> >> Signed-off-by: Cédric Le Goater
> >> ---
> >> hw/intc/xive.c | 94 +-
> >> 1 file changed, 93 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> >> index 5ba3b06e6e25..c49932d2b799 100644
> >> --- a/hw/intc/xive.c
> >> +++ b/hw/intc/xive.c
> >> @@ -21,9 +21,73 @@
> >> * XIVE Thread Interrupt Management context
> >> */
> >>
> >> +/* Convert a priority number to an Interrupt Pending Buffer (IPB)
> >> + * register, which indicates a pending interrupt at the priority
> >> + * corresponding to the bit number
> >> + */
> >> +static uint8_t priority_to_ipb(uint8_t priority)
> >> +{
> >> +return priority > XIVE_PRIORITY_MAX ?
> >> +0 : 1 << (XIVE_PRIORITY_MAX - priority);
> >> +}
> >> +
> >> +/* Convert an Interrupt Pending Buffer (IPB) register to a Pending
> >> + * Interrupt Priority Register (PIPR), which contains the priority of
> >> + * the most favored pending notification.
> >> + */
> >> +static uint8_t ipb_to_pipr(uint8_t ibp)
> >> +{
> >> +return ibp ? clz32((uint32_t)ibp << 24) : 0xff;
> >> +}
> >> +
> >> +static void ipb_update(uint8_t *regs, uint8_t priority)
> >> +{
> >> +regs[TM_IPB] |= priority_to_ipb(priority);
> >> +regs[TM_PIPR] = ipb_to_pipr(regs[TM_IPB]);
> >> +}
> >> +
> >> +static uint8_t exception_mask(uint8_t ring)
> >> +{
> >> +switch (ring) {
> >> +case TM_QW1_OS:
> >> +return TM_QW1_NSR_EO;
> >> +default:
> >> +g_assert_not_reached();
> >> +}
> >> +}
> >> +
> >> static uint64_t xive_tctx_accept(XiveTCTX *tctx, uint8_t ring)
> >> {
> >> -return 0;
> >> +uint8_t *regs = >regs[ring];
> >> +uint8_t nsr = regs[TM_NSR];
> >> +uint8_t mask = exception_mask(ring);
> >> +
> >> +qemu_irq_lower(tctx->output);
> >> +
> >> +if (regs[TM_NSR] & mask) {
> >> +uint8_t cppr = regs[TM_PIPR];
> >> +
> >> +regs[TM_CPPR] = cppr;
> >> +
> >> +/* Reset the pending buffer bit */
> >> +regs[TM_IPB] &= ~priority_to_ipb(cppr);
> >> +regs[TM_PIPR] = ipb_to_pipr(regs[TM_IPB]);
> >> +
> >> +/* Drop Exception bit */
> >> +regs[TM_NSR] &= ~mask;
> >> +}
> >> +
> >> +return (nsr << 8) | regs[TM_CPPR];
> >
> > Don't you need a cast to avoid (nsr << 8) being a shift-wider-than-size?
>
> I will check.
According to Eric, it doesn't, and given the compiler isn't
complaining I'm pretty sure that's right. Makes me a bit nervous
though.
>
> >
> >> +}
> >> +
> >> +static void xive_tctx_notify(XiveTCTX *tctx, uint8_t ring)
> >> +{
> >> +uint8_t *regs = >regs[ring];
> >> +
> >> +if (regs[TM_PIPR] < regs[TM_CPPR]) {
> >> +regs[TM_NSR] |= exception_mask(ring);
> >> +qemu_irq_raise(tctx->output);
> >> +}
> >> }
> >>
> >> static void xive_tctx_set_cppr(XiveTCTX *tctx, uint8_t ring, uint8_t cppr)
> >> @@ -33,6 +97,9 @@ static void xive_tctx_set_cppr(XiveTCTX *tctx, uint8_t
> >> ring, uint8_t cppr)
> >> }
> >>
> >> tctx->regs[ring + TM_CPPR] = cppr;
> >> +
> >> +/* CPPR has changed, check if we need to raise a pending exception */
> >> +xive_tctx_notify(tctx, ring);
> >> }
> >>
> >> /*
> >> @@ -198,6 +265,17 @@ static void xive_tm_set_os_cppr(XiveTCTX *tctx,
> >> hwaddr offset,
> >> xive_tctx_set_cppr(tctx, TM_QW1_OS, value & 0xff);
> >> }
> >>
> >> +/*
> >> + * Adjust the IPB to allow a CPU to process event queues of other
> >> + * priorities during one physical interrupt cycle.
> >> + */
> >> +static void xive_tm_set_os_pending(XiveTCTX *tctx, hwaddr offset,
> >> + uint64_t value, unsigned
Re: [Qemu-devel] [RFC v2 14/24] riscv: tcg-target: Add branch and jump instructions
On Wed, Nov 28, 2018 at 12:15 PM Richard Henderson
wrote:
>
> On 11/27/18 1:08 PM, Alistair Francis wrote:
> > +static inline void tcg_out_goto_long(TCGContext *s, tcg_insn_unit *target)
> > +{
> > +ptrdiff_t offset = tcg_pcrel_diff(s, target);
> > +
> > +if (offset == sextract64(offset, 0, 26)) {
> > +tcg_out_opc_jump(s, OPC_JAL, TCG_REG_ZERO, offset);
> > +} else {
> > +tcg_out_movi(s, TCG_TYPE_I64, TCG_REG_TMP0, (intptr_t)target);
> > +tcg_out_opc_jump(s, OPC_JAL, TCG_REG_TMP0, 0);
> > +}
> > +}
> > +
> > +static void tcg_out_call_int(TCGContext *s, tcg_insn_unit *arg, bool tail)
> > +{
> > +TCGReg link = tail ? TCG_REG_ZERO : TCG_REG_RA;
> > +ptrdiff_t offset = tcg_pcrel_diff(s, arg);
> > +if (offset == sextract64(offset, 1, 20) << 1) {
>
> s/20/26/
>
> Seems like there ought to be more shared code between tcg_out_call_int and
> tcg_out_goto_long, really.
I think tcg_out_goto_long can just be removed and replaced with
tcg_out_call() right?
Alistair
>
>
> r~
Re: [Qemu-devel] (no subject)
> On Nov 28, 2018, at 2:39 PM, Peter Maydell wrote:
>
> On Wed, 28 Nov 2018 at 01:12, John Arbuckle wrote:
>>
>> From af4497f2b161bb4165acb8eee5cae3f2a7ea2227 Mon Sep 17 00:00:00 2001
>> From: John Arbuckle
>> Date: Tue, 27 Nov 2018 20:01:20 -0500
>> Subject: [PATCH] ui/cocoa.m: fix crash due to cocoa_refresh() on Mac OS 10.14
>
> Something seems to have got the formatting of this patch email
> wrong -- it's got all this in the body and the actual Subject
> line of the email is blank.
I don't know what happened.
>
>> Mac OS 10.14 only wants UI code to be called from the main thread. The
>> cocoa_refresh() function is called on another thread and this causes a
>> crash to take place. To fix this problem the cocoa_refresh() code is
>> called from the main thread only.
>>
>> Signed-off-by: John Arbuckle
>> ---
>> ui/cocoa.m | 59 ++-
>> 1 file changed, 34 insertions(+), 25 deletions(-)
>
> I get a compile warning with this patch:
> /Users/pm215/src/qemu/ui/cocoa.m:1615:23: warning: instance method
> '-cocoa_refresh' not found (return type defaults to 'id')
> [-Wobjc-method-access]
>[[NSApp delegate] cocoa_refresh];
> ^
This will fix the problem:
static void cocoa_refresh(DisplayChangeListener *dcl)
{
QemuCocoaAppController *controller = (QemuCocoaAppController *)[NSApp
delegate];
[controller cocoa_refresh];
}
> To be honest, I'm still confused about what is causing the
> problems on Mojave. The refresh method should be being called
> on the main thread even with the code on master -- it's just
> that that is the iothread and it's running the event pumping
> code in the refresh callback rather than a standard OSX
> event loop. I'm hoping that the backtrace with symbols of
> the Mojave assertion failure will help there, since I
> can't currently see where refresh gets called from some
> non-main thread.
This might be a Mojave issue and not a QEMU issue.
I'm on Mac OS 10.12 so I can't confirm anything.
> I also think that this patch doesn't address the problems
> of locking that I mention on the discussion thread for
> Berkus' patch. It also doesn't handle any of the other
> callbacks from QEMU to the cocoa UI -- surely we need to
> handle all of them if there is a problem here?
To answer this question I would have to know how my patch
does on Mac OS 10.14. Does it stop the crashing issue? If
this patch does fix that problem then I think sticking to
a simple solution may be the answer. The use of locks may
not be needed.
> (I have some prototype patches which I've been working
> on which address the locking problem and also make all
> the QEMU callbacks run their work on the main thread.
> I may be able get those into shape to post those next week.)
Please CC me when do release it. I will test it on Mac OS 10.12
and Mac OS 10.6.
Re: [Qemu-devel] [PATCH] i2c: Move typedef of bitbang_i2c_interface to i2c.h
On Wed, Nov 28, 2018 at 08:27:06PM +0100, BALATON Zoltan wrote: > Clang 3.4 considers duplicate typedef in ppc4xx_i2c.h and > bitbang_i2c.h an error even if they are identical. Move it to a common > place to allow building with this clang version. > > Reported-by: Thomas Huth > Signed-off-by: BALATON Zoltan Acked-by: David Gibson > --- > hw/i2c/bitbang_i2c.h| 2 -- > include/hw/i2c/i2c.h| 2 ++ > include/hw/i2c/ppc4xx_i2c.h | 3 --- > 3 files changed, 2 insertions(+), 5 deletions(-) > > diff --git a/hw/i2c/bitbang_i2c.h b/hw/i2c/bitbang_i2c.h > index 3a7126d5de..9443021710 100644 > --- a/hw/i2c/bitbang_i2c.h > +++ b/hw/i2c/bitbang_i2c.h > @@ -3,8 +3,6 @@ > > #include "hw/i2c/i2c.h" > > -typedef struct bitbang_i2c_interface bitbang_i2c_interface; > - > #define BITBANG_I2C_SDA 0 > #define BITBANG_I2C_SCL 1 > > diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h > index 5dc166158b..cf4c45a98f 100644 > --- a/include/hw/i2c/i2c.h > +++ b/include/hw/i2c/i2c.h > @@ -82,6 +82,8 @@ int i2c_recv(I2CBus *bus); > > DeviceState *i2c_create_slave(I2CBus *bus, const char *name, uint8_t addr); > > +typedef struct bitbang_i2c_interface bitbang_i2c_interface; > + > /* lm832x.c */ > void lm832x_key_event(DeviceState *dev, int key, int state); > > diff --git a/include/hw/i2c/ppc4xx_i2c.h b/include/hw/i2c/ppc4xx_i2c.h > index 0891a9c948..b3450bacf7 100644 > --- a/include/hw/i2c/ppc4xx_i2c.h > +++ b/include/hw/i2c/ppc4xx_i2c.h > @@ -31,9 +31,6 @@ > #include "hw/sysbus.h" > #include "hw/i2c/i2c.h" > > -/* from hw/i2c/bitbang_i2c.h */ > -typedef struct bitbang_i2c_interface bitbang_i2c_interface; > - > #define TYPE_PPC4xx_I2C "ppc4xx-i2c" > #define PPC4xx_I2C(obj) OBJECT_CHECK(PPC4xxI2CState, (obj), TYPE_PPC4xx_I2C) > -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [Qemu-devel] [PULL 0/6] Fixes 31 20181127 patches
Hi, This series seems to have some coding style problems. See output below for more information: Type: series Subject: [Qemu-devel] [PULL 0/6] Fixes 31 20181127 patches Message-id: 20181127064932.7299-1-kra...@redhat.com === TEST SCRIPT BEGIN === #!/bin/bash BASE=base n=1 total=$(git log --oneline $BASE.. | wc -l) failed=0 git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram commits="$(git log --format=%H --reverse $BASE..)" for c in $commits; do echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..." if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then failed=1 echo fi n=$((n+1)) done exit $failed === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 Switched to a new branch 'test' 47e8b7b qapi: add query-display-options command b38268d usb-host: set ifs.detached as true if kernel driver is not active 36275b8 audio/hda: fix guest triggerable assert a20816e cirrus_vga/migration: update the bank offset before use 93d3ea2 ps2kbd: default to scan enabled after reset 0d552dd fmops: fix off-by-one in AR_TABLE and DR_TABLE array size === OUTPUT BEGIN === Checking PATCH 1/6: fmops: fix off-by-one in AR_TABLE and DR_TABLE array size... ERROR: code indent should never use tabs #28: FILE: hw/audio/fmopl.h:75: +^Iint32_t AR_TABLE[76];^I/* attack rate tables */$ ERROR: code indent should never use tabs #29: FILE: hw/audio/fmopl.h:76: +^Iint32_t DR_TABLE[76];^I/* decay rate tables */$ total: 2 errors, 0 warnings, 10 lines checked Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. Checking PATCH 2/6: ps2kbd: default to scan enabled after reset... Checking PATCH 3/6: cirrus_vga/migration: update the bank offset before use... Checking PATCH 4/6: audio/hda: fix guest triggerable assert... Checking PATCH 5/6: usb-host: set ifs.detached as true if kernel driver is not active... Checking PATCH 6/6: qapi: add query-display-options command... === OUTPUT END === Test command exited with code: 1 --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
[Qemu-devel] [RFC/PATCH] i386: Atomically update PTEs with mttcg
Afaik, this isn't well documented (at least it wasn't when I last looked)
but OSes such as Linux rely on this behaviour:
The HW updates to the page tables need to be done atomically with the
checking of the present bit (and other permissions).
This is what allows Linux to do simple xchg of PTEs with 0 and assume
the value read has "final" stable dirty and accessed bits (the TLB
invalidation is deferred).
Signed-off-by: Benjamin Herrenschmidt
---
This is lightly tested at this point, mostly to gather comments on the
approach.
I noticed that RiscV is attempting to do something similar but with endian
bugs, at least from my reading of the code.
include/exec/memory_ldst.inc.h | 3 +
memory_ldst.inc.c | 38
target/i386/excp_helper.c | 104 +
3 files changed, 121 insertions(+), 24 deletions(-)
diff --git a/include/exec/memory_ldst.inc.h b/include/exec/memory_ldst.inc.h
index 272c20f02e..b7a41a0ad4 100644
--- a/include/exec/memory_ldst.inc.h
+++ b/include/exec/memory_ldst.inc.h
@@ -28,6 +28,9 @@ extern uint64_t glue(address_space_ldq, SUFFIX)(ARG1_DECL,
hwaddr addr, MemTxAttrs attrs, MemTxResult *result);
extern void glue(address_space_stl_notdirty, SUFFIX)(ARG1_DECL,
hwaddr addr, uint32_t val, MemTxAttrs attrs, MemTxResult *result);
+extern uint32_t glue(address_space_cmpxchgl_notdirty, SUFFIX)(ARG1_DECL,
+hwaddr addr, uint32_t old, uint32_t new, MemTxAttrs attrs,
+MemTxResult *result);
extern void glue(address_space_stw, SUFFIX)(ARG1_DECL,
hwaddr addr, uint32_t val, MemTxAttrs attrs, MemTxResult *result);
extern void glue(address_space_stl, SUFFIX)(ARG1_DECL,
diff --git a/memory_ldst.inc.c b/memory_ldst.inc.c
index acf865b900..63af8f7dd2 100644
--- a/memory_ldst.inc.c
+++ b/memory_ldst.inc.c
@@ -320,6 +320,44 @@ void glue(address_space_stl_notdirty, SUFFIX)(ARG1_DECL,
RCU_READ_UNLOCK();
}
+/* This is meant to be used for atomic PTE updates under MT-TCG */
+uint32_t glue(address_space_cmpxchgl_notdirty, SUFFIX)(ARG1_DECL,
+hwaddr addr, uint32_t old, uint32_t new, MemTxAttrs attrs, MemTxResult
*result)
+{
+uint8_t *ptr;
+MemoryRegion *mr;
+hwaddr l = 4;
+hwaddr addr1;
+MemTxResult r;
+uint8_t dirty_log_mask;
+
+/* Must test result */
+assert(result);
+
+RCU_READ_LOCK();
+mr = TRANSLATE(addr, , , true, attrs);
+if (l < 4 || !memory_access_is_direct(mr, true)) {
+r = MEMTX_ERROR;
+} else {
+uint32_t orig = old;
+
+ptr = qemu_map_ram_ptr(mr->ram_block, addr1);
+old = atomic_cmpxchg(ptr, orig, new);
+
+if (old == orig) {
+dirty_log_mask = memory_region_get_dirty_log_mask(mr);
+dirty_log_mask &= ~(1 << DIRTY_MEMORY_CODE);
+cpu_physical_memory_set_dirty_range(memory_region_get_ram_addr(mr)
+ addr,
+4, dirty_log_mask);
+}
+r = MEMTX_OK;
+}
+*result = r;
+RCU_READ_UNLOCK();
+
+return old;
+}
+
/* warning: addr must be aligned */
static inline void glue(address_space_stl_internal, SUFFIX)(ARG1_DECL,
hwaddr addr, uint32_t val, MemTxAttrs attrs,
diff --git a/target/i386/excp_helper.c b/target/i386/excp_helper.c
index 49231f6b69..5ccb9d6d6a 100644
--- a/target/i386/excp_helper.c
+++ b/target/i386/excp_helper.c
@@ -157,11 +157,45 @@ int x86_cpu_handle_mmu_fault(CPUState *cs, vaddr addr,
int size,
#else
+static inline uint64_t update_entry(CPUState *cs, target_ulong addr,
+uint64_t orig_entry, uint32_t bits)
+{
+uint64_t new_entry = orig_entry | bits;
+
+/* Write the updated bottom 32-bits */
+if (qemu_tcg_mttcg_enabled()) {
+uint32_t old_le = cpu_to_le32(orig_entry);
+uint32_t new_le = cpu_to_le32(new_entry);
+MemTxResult result;
+uint32_t old_ret;
+
+old_ret = address_space_cmpxchgl_notdirty(cs->as, addr,
+ old_le, new_le,
+ MEMTXATTRS_UNSPECIFIED,
+ );
+if (result == MEMTX_OK) {
+if (old_ret != old_le) {
+new_entry = 0;
+}
+return new_entry;
+}
+
+/* Do we need to support this case where PTEs aren't in RAM ?
+ *
+ * For now fallback to non-atomic case
+ */
+}
+
+x86_stl_phys_notdirty(cs, addr, new_entry);
+
+return new_entry;
+}
+
static hwaddr get_hphys(CPUState *cs, hwaddr gphys, MMUAccessType access_type,
int *prot)
{
CPUX86State *env = _CPU(cs)->env;
-uint64_t rsvd_mask = PG_HI_RSVD_MASK;
+uint64_t rsvd_mask;
uint64_t ptep, pte;
uint64_t exit_info_1 = 0;
target_ulong pde_addr, pte_addr;
@@ -172,6 +206,8 @@ static hwaddr get_hphys(CPUState *cs, hwaddr gphys,
MMUAccessType access_type,
Re: [Qemu-devel] [PATCH v5 22/36] spapr/xive: add models for KVM support
On 11/28/18 6:52 AM, David Gibson wrote:
> On Fri, Nov 16, 2018 at 11:57:15AM +0100, Cédric Le Goater wrote:
>> This introduces a set of XIVE models specific to KVM which derive from
>> the XIVE base models. The interfaces with KVM are a new capability and
>> a new KVM device for the XIVE native exploitation interrupt mode.
>>
>> They handle the initialization of the TIMA and the source ESB memory
>> regions which have a different type under KVM. These are 'ram device'
>> memory mappings, similarly to VFIO, exposed to the guest and the
>> associated VMAs on the host are populated dynamically with the
>> appropriate pages using a fault handler.
>>
>> Signed-off-by: Cédric Le Goater
>
> The logic here looks fine, but I think it would be better to activate
> it with explicit if (kvm) type logic rather than using a subclass.
ok. ARM has taken a different path, the one proposed below, but it should
be possible to use a "if (kvm)" type logic. There should be less noise
in the object design.
>> ---
>> default-configs/ppc64-softmmu.mak | 1 +
>> include/hw/ppc/spapr_xive.h | 18 ++
>> include/hw/ppc/xive.h | 3 +
>> linux-headers/asm-powerpc/kvm.h | 12 +
>> linux-headers/linux/kvm.h | 4 +
>> target/ppc/kvm_ppc.h | 6 +
>> hw/intc/spapr_xive_kvm.c | 430 ++
>> hw/ppc/spapr.c| 7 +-
>> hw/ppc/spapr_irq.c| 19 +-
>> target/ppc/kvm.c | 7 +
>> hw/intc/Makefile.objs | 1 +
>> 11 files changed, 503 insertions(+), 5 deletions(-)
>> create mode 100644 hw/intc/spapr_xive_kvm.c
>>
>> diff --git a/default-configs/ppc64-softmmu.mak
>> b/default-configs/ppc64-softmmu.mak
>> index 7f34ad0528ed..c1bf5cd951f5 100644
>> --- a/default-configs/ppc64-softmmu.mak
>> +++ b/default-configs/ppc64-softmmu.mak
>> @@ -18,6 +18,7 @@ CONFIG_XICS_SPAPR=$(CONFIG_PSERIES)
>> CONFIG_XICS_KVM=$(call land,$(CONFIG_PSERIES),$(CONFIG_KVM))
>> CONFIG_XIVE=$(CONFIG_PSERIES)
>> CONFIG_XIVE_SPAPR=$(CONFIG_PSERIES)
>> +CONFIG_XIVE_KVM=$(call land,$(CONFIG_PSERIES),$(CONFIG_KVM))
>> CONFIG_MEM_DEVICE=y
>> CONFIG_DIMM=y
>> CONFIG_SPAPR_RNG=y
>> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
>> index aca2969a09ab..9c817bb7ae74 100644
>> --- a/include/hw/ppc/spapr_xive.h
>> +++ b/include/hw/ppc/spapr_xive.h
>> @@ -40,6 +40,10 @@ typedef struct sPAPRXive {
>> /* TIMA mapping address */
>> hwaddrtm_base;
>> MemoryRegion tm_mmio;
>> +
>> +/* KVM support */
>> +int fd;
>> +void *tm_mmap;
>> } sPAPRXive;
>>
>> #define SPAPR_XIVE_BASE_CLASS(klass) \
>> @@ -83,4 +87,18 @@ void spapr_xive_hcall_init(sPAPRMachineState *spapr);
>> void spapr_dt_xive(sPAPRXive *xive, int nr_servers, void *fdt,
>> uint32_t phandle);
>>
>> +/*
>> + * XIVE KVM models
>> + */
>> +
>> +#define TYPE_SPAPR_XIVE_KVM "spapr-xive-kvm"
>> +#define SPAPR_XIVE_KVM(obj) OBJECT_CHECK(sPAPRXive, (obj),
>> TYPE_SPAPR_XIVE_KVM)
>> +
>> +#define TYPE_XIVE_SOURCE_KVM "xive-source-kvm"
>> +#define XIVE_SOURCE_KVM(obj) \
>> +OBJECT_CHECK(XiveSource, (obj), TYPE_XIVE_SOURCE_KVM)
>> +
>> +#define TYPE_XIVE_TCTX_KVM "xive-tctx-kvm"
>> +#define XIVE_TCTX_KVM(obj) OBJECT_CHECK(XiveTCTX, (obj),
>> TYPE_XIVE_TCTX_KVM)
>> +
>> #endif /* PPC_SPAPR_XIVE_H */
>> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
>> index 281ed370121c..7aaf5a182cb3 100644
>> --- a/include/hw/ppc/xive.h
>> +++ b/include/hw/ppc/xive.h
>> @@ -69,6 +69,9 @@ typedef struct XiveSource {
>> uint32_tesb_shift;
>> MemoryRegionesb_mmio;
>>
>> +/* KVM support */
>> +void*esb_mmap;
>> +
>> XiveFabric *xive;
>> } XiveSource;
>>
>> diff --git a/linux-headers/asm-powerpc/kvm.h
>> b/linux-headers/asm-powerpc/kvm.h
>> index 8c876c166ef2..f34c971491dd 100644
>> --- a/linux-headers/asm-powerpc/kvm.h
>> +++ b/linux-headers/asm-powerpc/kvm.h
>
> Updates to linux-headers need to be split out into a separate patch.
> Eventually (i.e. by the time we merge) they should be just "update
> headers to SHA XXX" not picking and choosing pieces.
ok. I am starting to separate the KVM definition from the patch now
that the interface is stabilizing.
>> @@ -675,4 +675,16 @@ struct kvm_ppc_cpu_char {
>> #define KVM_XICS_PRESENTED (1ULL << 43)
>> #define KVM_XICS_QUEUED(1ULL << 44)
>>
>> +/* POWER9 XIVE Native Interrupt Controller */
>> +#define KVM_DEV_XIVE_GRP_CTRL 1
>> +#define KVM_DEV_XIVE_GET_ESB_FD 1
>> +#define KVM_DEV_XIVE_GET_TIMA_FD 2
>> +#define KVM_DEV_XIVE_VC_BASE 3
>> +#define KVM_DEV_XIVE_GRP_SOURCES2 /* 64-bit source attributes */
>> +
>> +/* Layout of 64-bit XIVE source attribute values */
>> +#define KVM_XIVE_LEVEL_SENSITIVE(1ULL << 0)
>> +#define KVM_XIVE_LEVEL_ASSERTED (1ULL << 1)
>> +
>> +
>> #endif /*
Re: [Qemu-devel] [PATCH v5 20/36] spapr: add classes for the XIVE models
On 11/28/18 6:13 AM, David Gibson wrote:
> On Fri, Nov 16, 2018 at 11:57:13AM +0100, Cédric Le Goater wrote:
>> The XIVE models for the QEMU and KVM accelerators will have a lot in
>> common. Introduce an abstract class for the source, the thread context
>> and the interrupt controller object to handle the differences in the
>> object initialization. These classes will also be used to define state
>> synchronization handlers for the monitor and migration usage.
>>
>> This is very much like the XICS models.
>
> Yeah.. so I know it's my code, but in hindsight I think making
> separate subclasses for TCG and KVM was a mistake. The distinction
> between emulated and KVM version is supposed to be invisible to both
> guest and (almost) to user, whereas a subclass usually indicates a
> visibly different device.
so how do you want to model the KVM part ? with a single object and
kvm_enabled() sections ?
>
>> Signed-off-by: Cédric Le Goater
>> ---
>> include/hw/ppc/spapr_xive.h | 15 +
>> include/hw/ppc/xive.h | 30 ++
>> hw/intc/spapr_xive.c| 86 +++-
>> hw/intc/xive.c | 109 +---
>> hw/ppc/spapr_irq.c | 4 +-
>> 5 files changed, 182 insertions(+), 62 deletions(-)
>>
>> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
>> index 5b3fab192d41..aca2969a09ab 100644
>> --- a/include/hw/ppc/spapr_xive.h
>> +++ b/include/hw/ppc/spapr_xive.h
>> @@ -13,6 +13,10 @@
>> #include "hw/sysbus.h"
>> #include "hw/ppc/xive.h"
>>
>> +#define TYPE_SPAPR_XIVE_BASE "spapr-xive-base"
>> +#define SPAPR_XIVE_BASE(obj) \
>> +OBJECT_CHECK(sPAPRXive, (obj), TYPE_SPAPR_XIVE_BASE)
>> +
>> #define TYPE_SPAPR_XIVE "spapr-xive"
>> #define SPAPR_XIVE(obj) OBJECT_CHECK(sPAPRXive, (obj), TYPE_SPAPR_XIVE)
>>
>> @@ -38,6 +42,17 @@ typedef struct sPAPRXive {
>> MemoryRegion tm_mmio;
>> } sPAPRXive;
>>
>> +#define SPAPR_XIVE_BASE_CLASS(klass) \
>> + OBJECT_CLASS_CHECK(sPAPRXiveClass, (klass), TYPE_SPAPR_XIVE_BASE)
>> +#define SPAPR_XIVE_BASE_GET_CLASS(obj) \
>> + OBJECT_GET_CLASS(sPAPRXiveClass, (obj), TYPE_SPAPR_XIVE_BASE)
>> +
>> +typedef struct sPAPRXiveClass {
>> +XiveRouterClass parent_class;
>> +
>> +DeviceRealize parent_realize;
>> +} sPAPRXiveClass;
>> +
>> bool spapr_xive_irq_enable(sPAPRXive *xive, uint32_t lisn, bool lsi);
>> bool spapr_xive_irq_disable(sPAPRXive *xive, uint32_t lisn);
>> void spapr_xive_pic_print_info(sPAPRXive *xive, Monitor *mon);
>> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
>> index b74eb326dcd1..281ed370121c 100644
>> --- a/include/hw/ppc/xive.h
>> +++ b/include/hw/ppc/xive.h
>> @@ -38,6 +38,10 @@ typedef struct XiveFabricClass {
>> * XIVE Interrupt Source
>> */
>>
>> +#define TYPE_XIVE_SOURCE_BASE "xive-source-base"
>> +#define XIVE_SOURCE_BASE(obj) \
>> +OBJECT_CHECK(XiveSource, (obj), TYPE_XIVE_SOURCE_BASE)
>> +
>> #define TYPE_XIVE_SOURCE "xive-source"
>> #define XIVE_SOURCE(obj) OBJECT_CHECK(XiveSource, (obj), TYPE_XIVE_SOURCE)
>>
>> @@ -68,6 +72,18 @@ typedef struct XiveSource {
>> XiveFabric *xive;
>> } XiveSource;
>>
>> +#define XIVE_SOURCE_BASE_CLASS(klass) \
>> + OBJECT_CLASS_CHECK(XiveSourceClass, (klass), TYPE_XIVE_SOURCE_BASE)
>> +#define XIVE_SOURCE_BASE_GET_CLASS(obj) \
>> + OBJECT_GET_CLASS(XiveSourceClass, (obj), TYPE_XIVE_SOURCE_BASE)
>> +
>> +typedef struct XiveSourceClass {
>> +SysBusDeviceClass parent_class;
>> +
>> +DeviceRealize parent_realize;
>> +DeviceReset parent_reset;
>> +} XiveSourceClass;
>> +
>> /*
>> * ESB MMIO setting. Can be one page, for both source triggering and
>> * source management, or two different pages. See below for magic
>> @@ -253,6 +269,9 @@ void xive_end_pic_print_info(XiveEND *end, uint32_t
>> end_idx, Monitor *mon);
>> * XIVE Thread interrupt Management (TM) context
>> */
>>
>> +#define TYPE_XIVE_TCTX_BASE "xive-tctx-base"
>> +#define XIVE_TCTX_BASE(obj) OBJECT_CHECK(XiveTCTX, (obj),
>> TYPE_XIVE_TCTX_BASE)
>> +
>> #define TYPE_XIVE_TCTX "xive-tctx"
>> #define XIVE_TCTX(obj) OBJECT_CHECK(XiveTCTX, (obj), TYPE_XIVE_TCTX)
>>
>> @@ -278,6 +297,17 @@ typedef struct XiveTCTX {
>> XiveRouter *xrtr;
>> } XiveTCTX;
>>
>> +#define XIVE_TCTX_BASE_CLASS(klass) \
>> + OBJECT_CLASS_CHECK(XiveTCTXClass, (klass), TYPE_XIVE_TCTX_BASE)
>> +#define XIVE_TCTX_BASE_GET_CLASS(obj) \
>> + OBJECT_GET_CLASS(XiveTCTXClass, (obj), TYPE_XIVE_TCTX_BASE)
>> +
>> +typedef struct XiveTCTXClass {
>> +DeviceClass parent_class;
>> +
>> +DeviceRealize parent_realize;
>> +} XiveTCTXClass;
>> +
>> /*
>> * XIVE Thread Interrupt Management Aera (TIMA)
>> */
>> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
>> index 3bf77ace11a2..ec85f7e4f88d 100644
>> --- a/hw/intc/spapr_xive.c
>> +++ b/hw/intc/spapr_xive.c
>> @@ -53,9 +53,9 @@ static void spapr_xive_mmio_map(sPAPRXive *xive)
>>
Re: [Qemu-devel] [PATCH v5 19/36] spapr: add a 'pseries-3.1-xive' machine type
On 11/28/18 5:42 AM, David Gibson wrote:
> On Fri, Nov 16, 2018 at 11:57:12AM +0100, Cédric Le Goater wrote:
>> The interrupt mode is statically defined to XIVE only for this machine.
>> The guest OS is required to have support for the XIVE exploitation
>> mode of the POWER9 interrupt controller.
>>
>> Signed-off-by: Cédric Le Goater
>> ---
>> include/hw/ppc/spapr_irq.h | 1 +
>> hw/ppc/spapr.c | 36 +++-
>> hw/ppc/spapr_irq.c | 3 +++
>> 3 files changed, 35 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
>> index c3b4c38145eb..b299dd794bff 100644
>> --- a/include/hw/ppc/spapr_irq.h
>> +++ b/include/hw/ppc/spapr_irq.h
>> @@ -33,6 +33,7 @@ void spapr_irq_msi_reset(sPAPRMachineState *spapr);
>> typedef struct sPAPRIrq {
>> uint32_tnr_irqs;
>> uint32_tnr_msis;
>> +uint8_t ov5;
>
> I'm a bit confused as to what exactly this represents..
The option vector 5 bits advertised by CAS for the platform. What the
hypervisor supports.
>
>> void (*init)(sPAPRMachineState *spapr, int nr_irqs, int nr_servers,
>> Error **errp);
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index ad1692cdcd0f..8fbb743769db 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -1097,12 +1097,14 @@ static void spapr_dt_rtas(sPAPRMachineState *spapr,
>> void *fdt)
>> spapr_dt_rtas_tokens(fdt, rtas);
>> }
>>
>> -/* Prepare ibm,arch-vec-5-platform-support, which indicates the MMU features
>> - * that the guest may request and thus the valid values for bytes 24..26 of
>> - * option vector 5: */
>> -static void spapr_dt_ov5_platform_support(void *fdt, int chosen)
>> +/* Prepare ibm,arch-vec-5-platform-support, which indicates the MMU
>> + * and the XIVE features that the guest may request and thus the valid
>> + * values for bytes 23..26 of option vector 5: */
>> +static void spapr_dt_ov5_platform_support(sPAPRMachineState *spapr, void
>> *fdt,
>> + int chosen)
>> {
>> PowerPCCPU *first_ppc_cpu = POWERPC_CPU(first_cpu);
>> +sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
>>
>> char val[2 * 4] = {
>> 23, 0x00, /* Xive mode, filled in below. */
>> @@ -1123,7 +1125,11 @@ static void spapr_dt_ov5_platform_support(void *fdt,
>> int chosen)
>> } else {
>> val[3] = 0x00; /* Hash */
>> }
>> +/* TODO: test KVM support */
>> +val[1] = smc->irq->ov5;
>> } else {
>> +val[1] = smc->irq->ov5;
>
> ..here it seems to be a specific value for this OV5 byte, indicating the
> supported intc...
yes.
>
>> +
>> /* V3 MMU supports both hash and radix in tcg (with dynamic
>> switching) */
>> val[3] = 0xC0;
>> }
>> @@ -1191,7 +1197,7 @@ static void spapr_dt_chosen(sPAPRMachineState *spapr,
>> void *fdt)
>> _FDT(fdt_setprop_string(fdt, chosen, "stdout-path", stdout_path));
>> }
>>
>> -spapr_dt_ov5_platform_support(fdt, chosen);
>> +spapr_dt_ov5_platform_support(spapr, fdt, chosen);
>>
>> g_free(stdout_path);
>> g_free(bootlist);
>> @@ -2622,6 +2628,11 @@ static void spapr_machine_init(MachineState *machine)
>> /* advertise support for ibm,dyamic-memory-v2 */
>> spapr_ovec_set(spapr->ov5, OV5_DRMEM_V2);
>>
>> +/* advertise XIVE */
>> +if (smc->irq->ov5) {
>
> ..but here it seems to be a bool indicating XIVE support specifically.
ah. yes. I need to check this part. That was a while ago.
>> +spapr_ovec_set(spapr->ov5, OV5_XIVE_EXPLOIT);
>> +}
>> +
>> /* init CPUs */
>> spapr_init_cpus(spapr);
>>
>> @@ -3971,6 +3982,21 @@ static void
>> spapr_machine_3_1_class_options(MachineClass *mc)
>>
>> DEFINE_SPAPR_MACHINE(3_1, "3.1", true);
>>
>> +static void spapr_machine_3_1_xive_instance_options(MachineState *machine)
>> +{
>> +spapr_machine_3_1_instance_options(machine);
>> +}
>> +
>> +static void spapr_machine_3_1_xive_class_options(MachineClass *mc)
>> +{
>> +sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
>> +
>> +spapr_machine_3_1_class_options(mc);
>> +smc->irq = _irq_xive;
>> +}
>> +
>> +DEFINE_SPAPR_MACHINE(3_1_xive, "3.1-xive", false);
>> +
>> /*
>> * pseries-3.0
>> */
>> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
>> index 253abc10e780..42e73851b174 100644
>> --- a/hw/ppc/spapr_irq.c
>> +++ b/hw/ppc/spapr_irq.c
>> @@ -210,6 +210,7 @@ static Object
>> *spapr_irq_cpu_intc_create_xics(sPAPRMachineState *spapr,
>> sPAPRIrq spapr_irq_xics = {
>> .nr_irqs = SPAPR_IRQ_XICS_NR_IRQS,
>> .nr_msis = SPAPR_IRQ_XICS_NR_MSIS,
>> +.ov5 = 0x0, /* XICS only */
>>
>> .init= spapr_irq_init_xics,
>> .claim = spapr_irq_claim_xics,
>> @@ -341,6 +342,7 @@ static Object
>> *spapr_irq_cpu_intc_create_xive(sPAPRMachineState *spapr,
>> sPAPRIrq spapr_irq_xive = {
>>
Re: [Qemu-devel] [libvirt] [PATCH for-4.0 v3 0/2] virtio: Provide version-specific variants of virtio PCI devices
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Subject: [libvirt] [PATCH for-4.0 v3 0/2] virtio: Provide version-specific
variants of virtio PCI devices
Message-id: 20181127024959.7060-1-ehabk...@redhat.com
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
421d93d virtio: Provide version-specific variants of virtio PCI devices
bef1223 virtio: Helper for registering virtio device types
=== OUTPUT BEGIN ===
Checking PATCH 1/2: virtio: Helper for registering virtio device types...
WARNING: line over 80 characters
#495: FILE: hw/virtio/virtio-pci.h:443:
+ * Implements both INTERFACE_PCIE_DEVICE and
INTERFACE_CONVENTIONAL_PCI_DEVICE,
WARNING: line over 80 characters
#504: FILE: hw/virtio/virtio-pci.h:452:
+ * Implements both INTERFACE_PCIE_DEVICE and
INTERFACE_CONVENTIONAL_PCI_DEVICE.
total: 0 errors, 2 warnings, 469 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 2/2: virtio: Provide version-specific variants of virtio PCI
devices...
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#315:
new file mode 100644
ERROR: line over 90 characters
#371: FILE: tests/acceptance/virtio_version.py:52:
+return devtype in [d['name'] for d in vm.command('qom-list-types',
implements=implements)]
ERROR: line over 90 characters
#373: FILE: tests/acceptance/virtio_version.py:54:
+def get_interfaces(vm, devtype, interfaces=['pci-express-device',
'conventional-pci-device']):
WARNING: line over 80 characters
#427: FILE: tests/acceptance/virtio_version.py:108:
+dev_1_0,nt_ifaces = self.run_device('%s-non-transitional' %
(qemu_devtype))
WARNING: line over 80 characters
#451: FILE: tests/acceptance/virtio_version.py:132:
+dev_trans,trans_ifaces = self.run_device('%s-transitional' %
(qemu_devtype))
total: 2 errors, 3 warnings, 405 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-de...@redhat.com
Re: [Qemu-devel] [PATCH v5 17/36] spapr: add device tree support for the XIVE exploitation mode
On 11/28/18 5:31 AM, David Gibson wrote:
> On Fri, Nov 16, 2018 at 11:57:10AM +0100, Cédric Le Goater wrote:
>> The XIVE interface for the guest is described in the device tree under
>> the "interrupt-controller" node. A couple of new properties are
>> specific to XIVE :
>>
>> - "reg"
>>
>>contains the base address and size of the thread interrupt
>>managnement areas (TIMA), for the User level and for the Guest OS
>>level. Only the Guest OS level is taken into account today.
>>
>> - "ibm,xive-eq-sizes"
>>
>>the size of the event queues. One cell per size supported, contains
>>log2 of size, in ascending order.
>>
>> - "ibm,xive-lisn-ranges"
>>
>>the IRQ interrupt number ranges assigned to the guest for the IPIs.
>>
>> and also under the root node :
>>
>> - "ibm,plat-res-int-priorities"
>>
>>contains a list of priorities that the hypervisor has reserved for
>>its own use. OPAL uses the priority 7 queue to automatically
>>escalate interrupts for all other queues (DD2.X POWER9). So only
>>priorities [0..6] are allowed for the guest.
>>
>> Extend the sPAPR IRQ backend with a new handler to populate the DT
>> with the appropriate "interrupt-controller" node.
>>
>> Signed-off-by: Cédric Le Goater
>> ---
>> include/hw/ppc/spapr_irq.h | 2 ++
>> include/hw/ppc/spapr_xive.h | 2 ++
>> hw/intc/spapr_xive_hcall.c | 62 +
>> hw/ppc/spapr.c | 3 +-
>> hw/ppc/spapr_irq.c | 17 ++
>> 5 files changed, 85 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
>> index c854ae527808..cfdc1f86e713 100644
>> --- a/include/hw/ppc/spapr_irq.h
>> +++ b/include/hw/ppc/spapr_irq.h
>> @@ -40,6 +40,8 @@ typedef struct sPAPRIrq {
>> void (*free)(sPAPRMachineState *spapr, int irq, int num);
>> qemu_irq (*qirq)(sPAPRMachineState *spapr, int irq);
>> void (*print_info)(sPAPRMachineState *spapr, Monitor *mon);
>> +void (*dt_populate)(sPAPRMachineState *spapr, uint32_t nr_servers,
>> +void *fdt, uint32_t phandle);
>> } sPAPRIrq;
>>
>> extern sPAPRIrq spapr_irq_xics;
>> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
>> index 418511f3dc10..5b3fab192d41 100644
>> --- a/include/hw/ppc/spapr_xive.h
>> +++ b/include/hw/ppc/spapr_xive.h
>> @@ -65,5 +65,7 @@ bool spapr_xive_priority_is_valid(uint8_t priority);
>> typedef struct sPAPRMachineState sPAPRMachineState;
>>
>> void spapr_xive_hcall_init(sPAPRMachineState *spapr);
>> +void spapr_dt_xive(sPAPRXive *xive, int nr_servers, void *fdt,
>> + uint32_t phandle);
>>
>> #endif /* PPC_SPAPR_XIVE_H */
>> diff --git a/hw/intc/spapr_xive_hcall.c b/hw/intc/spapr_xive_hcall.c
>> index 52e4e23995f5..66c78aa88500 100644
>> --- a/hw/intc/spapr_xive_hcall.c
>> +++ b/hw/intc/spapr_xive_hcall.c
>> @@ -890,3 +890,65 @@ void spapr_xive_hcall_init(sPAPRMachineState *spapr)
>> spapr_register_hypercall(H_INT_SYNC, h_int_sync);
>> spapr_register_hypercall(H_INT_RESET, h_int_reset);
>> }
>> +
>> +void spapr_dt_xive(sPAPRXive *xive, int nr_servers, void *fdt, uint32_t
>> phandle)
>> +{
>> +int node;
>> +uint64_t timas[2 * 2];
>> +/* Interrupt number ranges for the IPIs */
>> +uint32_t lisn_ranges[] = {
>> +cpu_to_be32(0),
>> +cpu_to_be32(nr_servers),
>> +};
>> +uint32_t eq_sizes[] = {
>> +cpu_to_be32(12), /* 4K */
>> +cpu_to_be32(16), /* 64K */
>> +cpu_to_be32(21), /* 2M */
>> +cpu_to_be32(24), /* 16M */
>> +};
>> +/* The following array is in sync with the
>> 'spapr_xive_priority_is_valid'
>> + * routine above. The O/S is expected to choose priority 6.
>> + */
>> +uint32_t plat_res_int_priorities[] = {
>> +cpu_to_be32(7),/* start */
>> +cpu_to_be32(0xf8), /* count */
>> +};
>> +gchar *nodename;
>> +
>> +/* Thread Interrupt Management Area : User (ring 3) and OS (ring 2) */
>> +timas[0] = cpu_to_be64(xive->tm_base + 3 * (1ull << TM_SHIFT));
>> +timas[1] = cpu_to_be64(1ull << TM_SHIFT);
>> +timas[2] = cpu_to_be64(xive->tm_base + 2 * (1ull << TM_SHIFT));
>
> Don't you have symbolic constants for the ring numbers, instead of '2'
> and '3' above?
I have offsets in the TIMA, 0x0, 0x10, ... We could add constants
for the ring numbers as they are used in the TIMA memory ops.
>
>> +timas[3] = cpu_to_be64(1ull << TM_SHIFT);
>> +
>> +nodename = g_strdup_printf("interrupt-controller@%" PRIx64,
>> + xive->tm_base + 3 * (1 << TM_SHIFT));
>> +_FDT(node = fdt_add_subnode(fdt, 0, nodename));
>> +g_free(nodename);
>> +
>> +_FDT(fdt_setprop_string(fdt, node, "device_type", "power-ivpe"));
>> +_FDT(fdt_setprop(fdt, node, "reg", timas, sizeof(timas)));
>> +
>> +_FDT(fdt_setprop_string(fdt, node, "compatible", "ibm,power-ivpe"));
>> +_FDT(fdt_setprop(fdt, node,
Re: [Qemu-devel] [PATCH v5 16/36] spapr: add hcalls support for the XIVE exploitation interrupt mode
On 11/28/18 5:25 AM, David Gibson wrote:
> On Fri, Nov 16, 2018 at 11:57:09AM +0100, Cédric Le Goater wrote:
>> The different XIVE virtualization structures (sources and event queues)
>> are configured with a set of Hypervisor calls :
>>
>> - H_INT_GET_SOURCE_INFO
>>
>>used to obtain the address of the MMIO page of the Event State
>>Buffer (ESB) entry associated with the source.
>>
>> - H_INT_SET_SOURCE_CONFIG
>>
>>assigns a source to a "target".
>>
>> - H_INT_GET_SOURCE_CONFIG
>>
>>determines which "target" and "priority" is assigned to a source
>>
>> - H_INT_GET_QUEUE_INFO
>>
>>returns the address of the notification management page associated
>>with the specified "target" and "priority".
>>
>> - H_INT_SET_QUEUE_CONFIG
>>
>>sets or resets the event queue for a given "target" and "priority".
>>It is also used to set the notification configuration associated
>>with the queue, only unconditional notification is supported for
>>the moment. Reset is performed with a queue size of 0 and queueing
>>is disabled in that case.
>>
>> - H_INT_GET_QUEUE_CONFIG
>>
>>returns the queue settings for a given "target" and "priority".
>>
>> - H_INT_RESET
>>
>>resets all of the guest's internal interrupt structures to their
>>initial state, losing all configuration set via the hcalls
>>H_INT_SET_SOURCE_CONFIG and H_INT_SET_QUEUE_CONFIG.
>>
>> - H_INT_SYNC
>>
>>issue a synchronisation on a source to make sure all notifications
>>have reached their queue.
>>
>> Calls that still need to be addressed :
>>
>>H_INT_SET_OS_REPORTING_LINE
>>H_INT_GET_OS_REPORTING_LINE
>>
>> See the code for more documentation on each hcall.
>>
>> Signed-off-by: Cédric Le Goater
>> ---
>> include/hw/ppc/spapr.h | 15 +-
>> include/hw/ppc/spapr_xive.h | 6 +
>> hw/intc/spapr_xive_hcall.c | 892
>> hw/ppc/spapr_irq.c | 2 +
>> hw/intc/Makefile.objs | 2 +-
>> 5 files changed, 915 insertions(+), 2 deletions(-)
>> create mode 100644 hw/intc/spapr_xive_hcall.c
>>
>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>> index 1fbc2663e06c..8415faea7b82 100644
>> --- a/include/hw/ppc/spapr.h
>> +++ b/include/hw/ppc/spapr.h
>> @@ -452,7 +452,20 @@ struct sPAPRMachineState {
>> #define H_INVALIDATE_PID0x378
>> #define H_REGISTER_PROC_TBL 0x37C
>> #define H_SIGNAL_SYS_RESET 0x380
>> -#define MAX_HCALL_OPCODEH_SIGNAL_SYS_RESET
>> +
>> +#define H_INT_GET_SOURCE_INFO 0x3A8
>> +#define H_INT_SET_SOURCE_CONFIG 0x3AC
>> +#define H_INT_GET_SOURCE_CONFIG 0x3B0
>> +#define H_INT_GET_QUEUE_INFO0x3B4
>> +#define H_INT_SET_QUEUE_CONFIG 0x3B8
>> +#define H_INT_GET_QUEUE_CONFIG 0x3BC
>> +#define H_INT_SET_OS_REPORTING_LINE 0x3C0
>> +#define H_INT_GET_OS_REPORTING_LINE 0x3C4
>> +#define H_INT_ESB 0x3C8
>> +#define H_INT_SYNC 0x3CC
>> +#define H_INT_RESET 0x3D0
>> +
>> +#define MAX_HCALL_OPCODEH_INT_RESET
>>
>> /* The hcalls above are standardized in PAPR and implemented by pHyp
>> * as well.
>> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
>> index 3f65b8f485fd..418511f3dc10 100644
>> --- a/include/hw/ppc/spapr_xive.h
>> +++ b/include/hw/ppc/spapr_xive.h
>> @@ -60,4 +60,10 @@ int spapr_xive_target_to_end(sPAPRXive *xive, uint32_t
>> target, uint8_t prio,
>> int spapr_xive_cpu_to_end(sPAPRXive *xive, PowerPCCPU *cpu, uint8_t prio,
>>uint8_t *out_end_blk, uint32_t *out_end_idx);
>>
>> +bool spapr_xive_priority_is_valid(uint8_t priority);
>
> AFAICT this could be a local function.
the KVM model uses it also, when collecting state from the KVM device
to build the QEMU ENDT.
>> +
>> +typedef struct sPAPRMachineState sPAPRMachineState;
>> +
>> +void spapr_xive_hcall_init(sPAPRMachineState *spapr);
>> +
>> #endif /* PPC_SPAPR_XIVE_H */
>> diff --git a/hw/intc/spapr_xive_hcall.c b/hw/intc/spapr_xive_hcall.c
>> new file mode 100644
>> index ..52e4e23995f5
>> --- /dev/null
>> +++ b/hw/intc/spapr_xive_hcall.c
>> @@ -0,0 +1,892 @@
>> +/*
>> + * QEMU PowerPC sPAPR XIVE interrupt controller model
>> + *
>> + * Copyright (c) 2017-2018, IBM Corporation.
>> + *
>> + * This code is licensed under the GPL version 2 or later. See the
>> + * COPYING file in the top-level directory.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qemu/log.h"
>> +#include "qapi/error.h"
>> +#include "cpu.h"
>> +#include "hw/ppc/fdt.h"
>> +#include "hw/ppc/spapr.h"
>> +#include "hw/ppc/spapr_xive.h"
>> +#include "hw/ppc/xive_regs.h"
>> +#include "monitor/monitor.h"
>
> Fwiw, I don't think it's particularly necessary to split the hcall
> handling out into a separate .c file.
ok. let's move it to spapr_xive then ? It might help in reducing the
exported funtions.
>> +/*
>> + * OPAL uses the priority 7 EQ to automatically escalate interrupts
>> + * for all other queues (DD2.X POWER9). So
Re: [Qemu-devel] [PATCH 00/12] tcg: Improve register allocation for calls
On Tue, Nov 27, 2018 at 21:38:22 -0800, Richard Henderson wrote:
> The intent here is to remove several move insns putting the
> function arguments into the proper place. I'm hoping that
> this will solve the skylake regression with spec2006, as
> seen with the ool softmmu patch set.
>
> Emilio, all of this is present on my tcg-next-for-4.0 branch.
Thanks for this.
Unfortunately, it doesn't seem to help, performance-wise.
I've benchmarked this on three different machines: Sandy
Bridge, Haswell and Skylake. The average slowdown vs.
the baseline is ~0%, ~5%, and ~10%, respectively.
So it seems the more modern the microarchitecture, the more
severe the slowdown (this is consistent with the assumption
that processors are getting better at caching over time).
Here are all the bar charts:
https://imgur.com/a/k7vmjVd
- baseline: tcg-next-for-4.0's parent from master, i.e.
4822f1e ("Merge remote-tracking branch
'remotes/kraxel/tags/fixes-31-20181127-pull-request'
into staging", 2018-11-27)
- ool: dc93c4a ("tcg/ppc: Use TCG_TARGET_NEED_LDST_OOL_LABELS",
2018-11-27)
- ool-regs: a9bac58 ("tcg: Record register preferences during
liveness", 2018-11-27)
I've also looked at hardware event counts on Skylake for
the above three commits. It seems that the indirection of
the (very) frequent ool calls/rets are what cause the large
reduction in IPC (results for bootup + hmmer):
- baseline:
291,451,142,426 instructions #2.94 insn per cycle
(71.45%)
99,050,829,190 cycles
(71.49%)
2,678,751,743 br_inst_retired.near_call
(71.43%)
2,674,367,278 br_inst_retired.near_return
(71.42%)
34,065,079,963 branches
(57.09%)
161,441,496 branch-misses #0.47% of all branches
(57.17%)
29.916874137 seconds time elapsed
- ool:
312,368,465,806 instructions #2.79 insn per cycle
(71.45%)
111,863,014,212 cycles
(71.31%)
11,751,151,140 br_inst_retired.near_call
(71.30%)
11,736,770,191 br_inst_retired.near_return
(71.41%)
24,660,597 br_misp_retired.near_call
(71.49%)
52,096,512,558 branches
(57.28%)
176,951,727 branch-misses #0.34% of all branches
(57.20%)
33.285149773 seconds time elapsed
- ool-regs:
309,253,149,588 instructions #2.71 insn per cycle
(71.47%)
113,938,069,597 cycles
(71.50%)
11,735,199,530 br_inst_retired.near_call
(71.51%)
11,725,686,909 br_inst_retired.near_return
(71.54%)
24,885,204 br_misp_retired.near_call
(71.46%)
52,768,150,694 branches
(56.97%)
184,421,824 branch-misses #0.35% of all branches
(57.03%)
33.867122498 seconds time elapsed
The additional branches are all from call/ret. I double-checked the generated
code and these are all well-matched (no jmp's instead of ret's), so
I don't think we can optimize anything there; it seems to me that this
is just a code size vs. speed trade-off.
ool-regs has even lower IPC, but it also uses less instructions, which
mitigates the slowdown due to lower IPC. The bottleneck in the ool
calls/rets remains, which explains why there isn't much to
be gained from the lower number of insns.
Let me know if you want me to do any other data collection.
Thanks,
Emilio
[Qemu-devel] [Bug 1805445] Re: QEMU arm virt machine was stopped by STMFD command while debug process
/*
* Allocate RAM after the memory controller has checked the size
* was valid. If not, a default value is used.
*/
ram_size = object_property_get_uint(OBJECT(>soc), "ram-size",
_abort);
memory_region_allocate_system_memory(>ram1, NULL, "ram1", ram_size);
memory_region_allocate_system_memory(>ram2, NULL, "ram2", ram_size);
memory_region_allocate_system_memory(>ram3, NULL, "ram3", ram_size);
memory_region_add_subregion(get_system_memory(), sc->info->sdram_base[1],
>ram1);
memory_region_add_subregion(get_system_memory(), sc->info->sdram_base[0],
>ram2);
memory_region_add_subregion(get_system_memory(), sc->info->sdram_base[2],
>ram3);
object_property_add_const_link(OBJECT(>soc), "ram1",
OBJECT(>ram1), _abort);
object_property_add_const_link(OBJECT(>soc), "ram2",
OBJECT(>ram2), _abort);
object_property_add_const_link(OBJECT(>soc), "ram3",
OBJECT(>ram3), _abort);
And I can create 3 RAM section for avoid exeption handler! Maybe any
devices from this adress does not appear in qemu...
--
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1805445
Title:
QEMU arm virt machine was stopped by STMFD command while debug process
Status in QEMU:
New
Bug description:
Hello, i have a big problem with QEMU arm virtual machine. So...
I run QEMU machine with bare-metal ThreadX fullflash from Texet TM-333 phone
(Spreadtrum platform)
[CODE]qemu-system-arm -S -gdb tcp::1234,ipv4 -drive
file=C:\cygwin64\home\flash.bin,if=mtd,format=raw -M palmetto-bmc -cpu arm926
-m 64M[/CODE]
I use palmetto-bmc platform because it have ARM926EJ-S core and support SPI
Flash.
Then, i attach to gdb qemu process from IDA and run code step-by-step.
When the IDA run 00032534 STR R11, [R10] command
instead of store R11 on R10 adress, it jump 000328DC STMFD SP!, {R0-R12,LR}
instruction...
and virt machine not execute new instruction...
[IMG]https://pp.userapi.com/c850624/v850624111/528f3/N7FTpgloWVU.jpg[/IMG]
and why i did not change flash from n25q256a to n25q032a11 in aspeed.c
without rebuild qemu?
To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1805445/+subscriptions
[Qemu-devel] [PATCH 2/2] scsi: esp: Improve consistency of RSTAT, RSEQ, and RINTR
The guest OS reads RSTAT, RSEQ, and RINTR, and expects those registers
to reflect a consistent state. However, it is possible that the registers
can change after RSTAT was read, but before RINTR is read.
Guest OSqemu
Read RSTAT
esp_command_complete()
RSTAT = STAT_ST
esp_dma_done()
RSTAT |= STAT_TC
RSEQ = 0
RINTR = INTR_BS
Read RSEQ
Read RINTR RINTR = 0
RSTAT &= ~STAT_TC
RSEQ = SEQ_CD
The guest OS would then try to handle INTR_BS combined with an old
value of RSTAT. This sometimes resulted in lost events, spurious
interrupts, guest OS confusion, and stalled SCSI operations.
A typical guest error log (observed with various versions of Linux)
looks as follows.
scsi host1: Spurious irq, sreg=13.
...
scsi host1: Aborting command [84531f10:2a]
scsi host1: Current command [f882eea8:35]
scsi host1: Queued command [84531f10:2a]
scsi host1: Active command [f882eea8:35]
scsi host1: Dumping command log
scsi host1: ent[15] CMD val[44] sreg[90] seqreg[00] sreg2[00] ireg[20] ss[00]
event[0c]
scsi host1: ent[16] CMD val[01] sreg[90] seqreg[00] sreg2[00] ireg[20] ss[02]
event[0c]
scsi host1: ent[17] CMD val[43] sreg[90] seqreg[00] sreg2[00] ireg[20] ss[02]
event[0c]
scsi host1: ent[18] EVENT val[0d] sreg[92] seqreg[04] sreg2[00] ireg[18] ss[00]
event[0c]
...
Adress the situation by caching RINTR and RSEQ when RSTAT is read and
using the cached values when the respective registers are read.
Signed-off-by: Guenter Roeck
---
I am not too happy with this solution (it looks kludgy to me), but it does
work. With this series applied, I have not seen a single spurious interrupt
after hundreds of boots, and no stalls. Prior to that, spurious interrupts
were seen with pretty much every boot, and the stall occurred on a regular
basis (roughly every other boot with qemu-system-hppa, less with others).
If anyone has a better idea, please let me know, and I'll be happy to
test it.
hw/scsi/esp.c | 40
include/hw/scsi/esp.h | 2 ++
2 files changed, 42 insertions(+)
diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 630d923..6af74bc 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -415,6 +415,16 @@ uint64_t esp_reg_read(ESPState *s, uint32_t saddr)
}
break;
case ESP_RINTR:
+if (s->rintr_saved) {
+old_val = s->rintr_saved;
+s->rintr_saved = 0;
+if (!(s->rregs[ESP_RINTR])) {
+s->rregs[ESP_RSTAT] &= ~STAT_TC;
+s->rregs[ESP_RSEQ] = SEQ_CD;
+esp_lower_irq(s);
+}
+return old_val;
+}
/* Clear sequence step, interrupt register and all status bits
except TC */
old_val = s->rregs[ESP_RINTR];
@@ -429,6 +439,34 @@ uint64_t esp_reg_read(ESPState *s, uint32_t saddr)
if (!s->tchi_written) {
return s->chip_id;
}
+case ESP_RSTAT:
+/*
+ * After receiving an interrupt, the guest OS reads
+ * RSTAT, RSEQ, and RINTR. When reading RINTR,
+ * RSTAT and RSEQ are updated. It was observed that
+ * esp_command_complete() and with it esp_dma_done()
+ * was called after the guest OS reads RSTAT, but
+ * before it was able to read RINTR. In other words,
+ * the host would read RINTR associated with the
+ * old value of RSTAT, not the new value. Since RSTAT
+ * was supposed to reflect STAT_ST in this situation,
+ * the host OS got confused, and the operation stalled.
+ * Remedy the situation by caching both ESP_RINTR and
+ * ESP_RSEQ. Return those values until ESP_RINTR is read.
+ * Only do this if an interrupt is pending to limit its
+ * impact.
+ */
+if (s->rregs[ESP_RSTAT] & STAT_INT) {
+s->rintr_saved = s->rregs[ESP_RINTR];
+s->rseq_saved = s->rregs[ESP_RSEQ];
+s->rregs[ESP_RINTR] = 0;
+}
+break;
+case ESP_RSEQ:
+if (s->rintr_saved) {
+return s->rseq_saved;
+}
+break;
default:
break;
}
@@ -577,6 +615,8 @@ const VMStateDescription vmstate_esp = {
.fields = (VMStateField[]) {
VMSTATE_BUFFER(rregs, ESPState),
VMSTATE_BUFFER(wregs, ESPState),
+VMSTATE_UINT8(rintr_saved, ESPState),
+VMSTATE_UINT8(rseq_saved, ESPState),
VMSTATE_INT32(ti_size, ESPState),
VMSTATE_UINT32(ti_rptr, ESPState),
VMSTATE_UINT32(ti_wptr, ESPState),
diff --git a/include/hw/scsi/esp.h b/include/hw/scsi/esp.h
index 682a0d2..342f607 100644
--- a/include/hw/scsi/esp.h
+++ b/include/hw/scsi/esp.h
@@ -17,6 +17,8 @@ typedef struct ESPState ESPState;
struct ESPState {
[Qemu-devel] [PATCH 1/2] esp-pci: Fix status register write erase control
Per AM53C974 datasheet, definition of "SCSI Bus and Control (SBAC)"
register:
Bit 24 – STATUS – Write Erase Control
This bit controls the Write Erase feature on bits 3:1 and bit 6 of the DMA
Status Register ((B)+54h). When this bit is programmed to ‘1’, the state
of bits 3:1 are preserved when read. Bits 3:1 are only cleared when a ‘1’
is written to the corresponding bit location. For example, to clear bit 1,
the value of ‘_0010b’ should be written to the register. When the DMA
Status Preserve bit is ‘0’, bits 3:1 are cleared when read.
The status register is currently defined to bit 12, not bit 24.
Also, its implementation is reversed: The status is auto-cleared if
the bit is set to 1, and must be cleared explicitly when the bit is
set to 0. This results in spurious interrupts reported by the Linux
kernel, and in some cases even results in stalled SCSI operations.
Set SBAC_STATUS to bit 24 and reverse the logic to fix the problem.
Signed-off-by: Guenter Roeck
---
hw/scsi/esp-pci.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/hw/scsi/esp-pci.c b/hw/scsi/esp-pci.c
index 419fc66..d956909 100644
--- a/hw/scsi/esp-pci.c
+++ b/hw/scsi/esp-pci.c
@@ -59,7 +59,7 @@
#define DMA_STAT_SCSIINT 0x10
#define DMA_STAT_BCMBLT 0x20
-#define SBAC_STATUS 0x1000
+#define SBAC_STATUS (1 << 24)
typedef struct PCIESPState {
/*< private >*/
@@ -136,7 +136,7 @@ static void esp_pci_dma_write(PCIESPState *pci, uint32_t
saddr, uint32_t val)
pci->dma_regs[saddr] = val;
break;
case DMA_STAT:
-if (!(pci->sbac & SBAC_STATUS)) {
+if (pci->sbac & SBAC_STATUS) {
/* clear some bits on write */
uint32_t mask = DMA_STAT_ERROR | DMA_STAT_ABORT | DMA_STAT_DONE;
pci->dma_regs[DMA_STAT] &= ~(val & mask);
@@ -157,7 +157,7 @@ static uint32_t esp_pci_dma_read(PCIESPState *pci, uint32_t
saddr)
if (pci->esp.rregs[ESP_RSTAT] & STAT_INT) {
val |= DMA_STAT_SCSIINT;
}
-if (pci->sbac & SBAC_STATUS) {
+if (!(pci->sbac & SBAC_STATUS)) {
pci->dma_regs[DMA_STAT] &= ~(DMA_STAT_ERROR | DMA_STAT_ABORT |
DMA_STAT_DONE);
}
--
2.7.4
[Qemu-devel] [Bug 1336794] Re: 9pfs does not honor open file handles on unlinked files
hi greg,
thanks very much for you answer. i saw the proposed kernel patch from eric van
hensbergen - even tried to build my own kernel with the patch applied, i was
ready to run this on a custom kernel with a custom built qemu, but although the
patch can be applied, there have been too many changes in the surrounding code
for it to be able to work.
the idea of the 9p file sharing in qemu is really nice (and fast). i am (was)
trying to use it as a persistent storage on a kubernetes cluster and it is much
better than nfs (performance wise) locking works just dandy.
with 9p i thought i was golden, unfortunately no cigar.
since there are different parties involved (and to get something into the linux
kernel requires - from what i have read - the patience of a buddhist monk) i
think it will be very hard to get this picked up.
because of the time frame this will probably not be a solution for me, but i am
nonetheless willing to invest some time to bringing this forward.
how is a good way to proceed? (sorry, this question might sound dumb, but
despite being a software developer for most of my working life the ways of the
open source community have never revealed themselves to me).
-alex
--
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1336794
Title:
9pfs does not honor open file handles on unlinked files
Status in QEMU:
In Progress
Status in Ubuntu:
Confirmed
Bug description:
This was originally filed over here:
https://bugzilla.redhat.com/show_bug.cgi?id=1114221
The open-unlink-fstat idiom used in some places to create an anonymous
private temporary file does not work in a QEMU guest over a virtio-9p
filesystem.
Version-Release number of selected component (if applicable):
qemu-kvm-1.6.2-6.fc20.x86_64
qemu-system-x86-1.6.2-6.fc20.x86_64
(those are fedora RPMs)
How reproducible:
Always. See this example C program:
https://bugzilla.redhat.com/attachment.cgi?id=913069
Steps to Reproduce:
1. Export a filesystem with virt-manager for the guest.
(type: mount, driver: default, mode: passthrough)
2. Start guest and mount that filesystem
(mount -t 9p -o trans=virtio,version=9p2000.L ...)
3. Run a program that uses open-unlink-fstat
(in my case it was trying to compile Perl 5.20)
Actual results:
fstat fails:
open("/home/tst/filename", O_RDWR|O_CREAT|O_EXCL, 0600) = 3
unlink("/home/tst/filename")= 0
fstat(3, 0x23aa1a8) = -1 ENOENT (No such file or
directory)
close(3)
Expected results:
open("/home/tst/filename", O_RDWR|O_CREAT|O_EXCL, 0600) = 3
unlink("/home/tst/filename")= 0
fstat(3, {st_mode=S_IFREG|0600, st_size=0, ...}) = 0
fcntl(3, F_SETFD, FD_CLOEXEC) = 0
close(3)
Additional info:
There was a patch put into the kernel back in '07 to handle this very
problem for other filesystems; maybe its helpful:
http://lwn.net/Articles/251228/
There is also a thread on LKML from last December specifically about
this very problem:
https://lkml.org/lkml/2013/12/31/163
There was a discussion on the QEMU list back in '11 that doesn't seem
to have come to a conclusion, but did provide the test program that
i've attached to this report:
http://marc.info/?l=qemu-devel=130443605720648=2
To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1336794/+subscriptions
Re: [Qemu-devel] [qemu-web PATCH] Document how to test the site with jekyll locally
On 28.11.18 17:44, Paolo Bonzini wrote: > On 28/11/18 16:35, Daniel P. Berrangé wrote: >> Add a README file that tells people this is a jekyll based static >> website, and shows people how to run jekyll for testing purposes. >> >> Signed-off-by: Daniel P. Berrangé >> --- >> >> NB, we should really mention a license in the README too, but I don't >> see info about what license we consider qemu-web to be covered by... > 1Rainer Müller > Anybody (especially non-RH people) disagrees with dual-license CC-BY-SA > 4.0 and GPLv2+? (So that we can copy from blog posts to manuals)? No objections from my side. Rainer
Re: [Qemu-devel] [RFC v2 19/24] riscv: tcg-target: Add the target init code
On 11/27/18 1:09 PM, Alistair Francis wrote: > Signed-off-by: Alistair Francis > Signed-off-by: Michael Clark > --- > tcg/riscv/tcg-target.inc.c | 31 +++ > 1 file changed, 31 insertions(+) Reviewed-by: Richard Henderson r~
[Qemu-devel] [Bug 1805697] [NEW] egl-headless crashes
Public bug reported: egl-headless crashes when it is trying change the resolution. After XFCE login, for example. I tryed it on 2.12, 3.0 and 3.1.0-rc2 versions. # qemu-system-x86_64 -enable-kvm -enable-kvm -M q35 -smp 8 -vga virtio -spice port=59011,addr=0.0.0.0,disable-ticketing -hda image.qcow2 -m 4G -display egl-headless -chardev spicevmc,name=vdagent,id=vdagent main_channel_link: add main channel client main_channel_client_handle_pong: net test: latency 6.942000 ms, bitrate 8497925311 bps (8104.253112 Mbps) inputs_connect: inputs channel client create red_qxl_set_cursor_peer: gl_version 31 - compat profile qemu-system-x86_64: ui/egl-headless.c:128: egl_scanout_flush: Assertion `surface_width(edpy->ds) == edpy->guest_fb.width' failed. Aborted (core dumped) ** Affects: qemu Importance: Undecided Status: New ** Tags: egl-headless virgl -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1805697 Title: egl-headless crashes Status in QEMU: New Bug description: egl-headless crashes when it is trying change the resolution. After XFCE login, for example. I tryed it on 2.12, 3.0 and 3.1.0-rc2 versions. # qemu-system-x86_64 -enable-kvm -enable-kvm -M q35 -smp 8 -vga virtio -spice port=59011,addr=0.0.0.0,disable-ticketing -hda image.qcow2 -m 4G -display egl-headless -chardev spicevmc,name=vdagent,id=vdagent main_channel_link: add main channel client main_channel_client_handle_pong: net test: latency 6.942000 ms, bitrate 8497925311 bps (8104.253112 Mbps) inputs_connect: inputs channel client create red_qxl_set_cursor_peer: gl_version 31 - compat profile qemu-system-x86_64: ui/egl-headless.c:128: egl_scanout_flush: Assertion `surface_width(edpy->ds) == edpy->guest_fb.width' failed. Aborted (core dumped) To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1805697/+subscriptions
[Qemu-devel] [Bug 1805697] Re: egl-headless crashes
Is it possible to set init resolution when VM is starting? ** Tags added: egl-headless ** Tags added: virgl -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1805697 Title: egl-headless crashes Status in QEMU: New Bug description: egl-headless crashes when it is trying change the resolution. After XFCE login, for example. I tryed it on 2.12, 3.0 and 3.1.0-rc2 versions. # qemu-system-x86_64 -enable-kvm -enable-kvm -M q35 -smp 8 -vga virtio -spice port=59011,addr=0.0.0.0,disable-ticketing -hda image.qcow2 -m 4G -display egl-headless -chardev spicevmc,name=vdagent,id=vdagent main_channel_link: add main channel client main_channel_client_handle_pong: net test: latency 6.942000 ms, bitrate 8497925311 bps (8104.253112 Mbps) inputs_connect: inputs channel client create red_qxl_set_cursor_peer: gl_version 31 - compat profile qemu-system-x86_64: ui/egl-headless.c:128: egl_scanout_flush: Assertion `surface_width(edpy->ds) == edpy->guest_fb.width' failed. Aborted (core dumped) To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1805697/+subscriptions
Re: [Qemu-devel] [PATCH v2 16/16] virtio: virtio 9p really requires CONFIG_VIRTFS to work
On 26/11/2018 21:00, Juan Quintela wrote: > Otherwise, it has no implementation. > > Signed-off-by: Juan Quintela > --- > default-configs/virtio.mak | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/default-configs/virtio.mak b/default-configs/virtio.mak > index 5ae4a61018..ecb4420e74 100644 > --- a/default-configs/virtio.mak > +++ b/default-configs/virtio.mak > @@ -1,7 +1,7 @@ > CONFIG_VHOST_USER_SCSI=$(call land,$(CONFIG_VHOST_USER),$(CONFIG_LINUX)) > CONFIG_VHOST_USER_BLK=$(call land,$(CONFIG_VHOST_USER),$(CONFIG_LINUX)) > CONFIG_VIRTIO=y > -CONFIG_VIRTIO_9P=y > +CONFIG_VIRTIO_9P=$(CONFIG_VIRTFS) > CONFIG_VIRTIO_BALLOON=y > CONFIG_VIRTIO_BLK=y > CONFIG_VIRTIO_CRYPTO=y > I think you should also replace "#ifdef CONFIG_VIRTFS" by "#ifdef CONFIG_VIRTIO_9P" in: hw/s390x/virtio-ccw.h include/hw/xen/xen_backend.h hw/xen/xen_backend.c Thanks, Laurent
Re: [Qemu-devel] [RFC v2 18/24] riscv: tcg-target: Add the prologue generation and register the JIT
On 11/27/18 1:09 PM, Alistair Francis wrote: > Signed-off-by: Alistair Francis > Signed-off-by: Michael Clark > --- > tcg/riscv/tcg-target.inc.c | 111 + > 1 file changed, 111 insertions(+) Reviewed-by: Richard Henderson r~
Re: [Qemu-devel] [PATCH v2 15/16] virtio: split virtio crypto bits rom virtio-pci.h
On 26/11/2018 21:00, Juan Quintela wrote: > Signed-off-by: Juan Quintela > --- > hw/virtio/virtio-crypto-pci.c | 14 ++ > hw/virtio/virtio-pci.h| 14 -- > 2 files changed, 14 insertions(+), 14 deletions(-) > Reviewed-by: Laurent Vivier
Re: [Qemu-devel] [RFC v2 24/24] WIP: Try to patch longer branches
On 11/27/18 1:10 PM, Alistair Francis wrote:
> +if (short_jmp) {
> +reloc_sbimm12(code_ptr, (tcg_insn_unit *)value);
> +} else {
> +/* Invert the condition */
> +insn = insn ^ (1 << 12);
> +/* Clear the offset */
> +insn &= 0xFFF;
> +/* Set the offset to the PC + 8 */
> +insn |= ((unsigned int)(code_ptr + 8)) << 12;
This isn't a pc-relative value you're storing.
reloc_sbimm12(code_ptr, code_ptr + 2);
> +/* Overwrite the NOP with jal x0,value */
> +insn = encode_uj(OPC_JAL, TCG_REG_ZERO, value);
This isn't pc-relative either. Perhaps best as
code_ptr[1] = encode_uj(OPC_JAL, TCG_REG_ZERO, 0);
reloc_jimm20(code_ptr + 1, (tcg_insn_unit *)value);
r~
Re: [Qemu-devel] [PATCH v2 14/16] virtio: split virtio gpu bits rom virtio-pci.h
On 26/11/2018 21:00, Juan Quintela wrote: > Signed-off-by: Juan Quintela > --- > hw/display/virtio-gpu-pci.c | 14 ++ > hw/display/virtio-vga.c | 1 + > hw/virtio/virtio-pci.h | 14 -- > 3 files changed, 15 insertions(+), 14 deletions(-) > Reviewed-by: Laurent Vivier
Re: [Qemu-devel] [PATCH v2 13/16] virtio: split virtio serial bits rom virtio-pci
On 26/11/2018 21:00, Juan Quintela wrote: > Virtio console and qga tests also depend on CONFIG_VIRTIO_SERIAL. > > Reviewed-by: Thomas Huth > Signed-off-by: Juan Quintela > --- > hw/virtio/Makefile.objs | 1 + > hw/virtio/virtio-pci.c| 75 --- > hw/virtio/virtio-pci.h| 14 - > hw/virtio/virtio-serial-pci.c | 112 ++ > tests/Makefile.include| 6 +- > 5 files changed, 116 insertions(+), 92 deletions(-) > create mode 100644 hw/virtio/virtio-serial-pci.c > Reviewed-by: Laurent Vivier
Re: [Qemu-devel] [PATCH v2 12/16] virtio: split virtio net bits rom virtio-pci
On 26/11/2018 21:00, Juan Quintela wrote: > Reviewed-by: Thomas Huth > Signed-off-by: Juan Quintela > --- > hw/virtio/Makefile.objs| 1 + > hw/virtio/virtio-net-pci.c | 96 ++ > hw/virtio/virtio-pci.c | 57 -- > hw/virtio/virtio-pci.h | 14 -- > tests/Makefile.include | 2 +- > 5 files changed, 98 insertions(+), 72 deletions(-) > create mode 100644 hw/virtio/virtio-net-pci.c Reviewed-by: Laurent Vivier
Re: [Qemu-devel] [PATCH v2 11/16] virtio: split virtio blk bits rom virtio-pci
On 26/11/2018 21:00, Juan Quintela wrote: > Reviewed-by: Thomas Huth > Signed-off-by: Juan Quintela > --- > hw/virtio/Makefile.objs| 1 + > hw/virtio/virtio-blk-pci.c | 97 ++ > hw/virtio/virtio-pci.c | 59 --- > hw/virtio/virtio-pci.h | 14 -- > tests/Makefile.include | 5 +- > 5 files changed, 100 insertions(+), 76 deletions(-) > create mode 100644 hw/virtio/virtio-blk-pci.c > > diff --git a/tests/Makefile.include b/tests/Makefile.include > index 9b3e7d7403..36d95fe913 100644 > --- a/tests/Makefile.include > +++ b/tests/Makefile.include > @@ -147,8 +147,7 @@ check-qtest-virtioserial-y += > tests/virtio-console-test$(EXESUF) > > check-qtest-virtio-y += tests/virtio-net-test$(EXESUF) > check-qtest-virtio-$(CONFIG_VIRTIO_BALLOON) += > tests/virtio-balloon-test$(EXESUF) > -check-qtest-virtio-y += tests/virtio-blk-test$(EXESUF) > -check-qtest-virtio-y += tests/virtio-rng-test$(EXESUF) This one (virtio-rng-test) should be part of PATCH 04/16 Reviewed-by: Laurent Vivier
Re: [Qemu-devel] [PATCH v2 07/16] virtio: split vhost user blk bits from virtio-pci
On 26/11/2018 21:00, Juan Quintela wrote: > Reviewed-by: Thomas Huth > Signed-off-by: Juan Quintela > --- > hw/virtio/Makefile.objs| 1 + > hw/virtio/vhost-user-blk-pci.c | 101 + > hw/virtio/virtio-pci.c | 60 > hw/virtio/virtio-pci.h | 18 -- > 4 files changed, 102 insertions(+), 78 deletions(-) > create mode 100644 hw/virtio/vhost-user-blk-pci.c > Reviewed-by: Laurent Vivier
Re: [Qemu-devel] [PATCH v2 10/16] virtio: split virtio scsi bits from virtio-pci
On 26/11/2018 21:00, Juan Quintela wrote: > Notice that we can't still run tests with it disabled. Both cdrom-test and > drive_del-test use virtio-scsi without checking if it is enabled. > > Reviewed-by: Thomas Huth > Signed-off-by: Juan Quintela > --- > hw/virtio/Makefile.objs | 1 + > hw/virtio/virtio-pci.c | 69 --- > hw/virtio/virtio-pci.h | 14 - > hw/virtio/virtio-scsi-pci.c | 106 > tests/Makefile.include | 2 +- > 5 files changed, 108 insertions(+), 84 deletions(-) > create mode 100644 hw/virtio/virtio-scsi-pci.c > Reviewed-by: Laurent Vivier
Re: [Qemu-devel] [PATCH v2 09/16] virtio: split vhost scsi bits from virtio-pci
On 26/11/2018 21:00, Juan Quintela wrote: > Signed-off-by: Juan Quintela > --- > hw/virtio/Makefile.objs| 1 + > hw/virtio/vhost-scsi-pci.c | 95 ++ > hw/virtio/virtio-pci.c | 59 --- > hw/virtio/virtio-pci.h | 19 > 4 files changed, 96 insertions(+), 78 deletions(-) > create mode 100644 hw/virtio/vhost-scsi-pci.c > Reviewed-by: Laurent Vivier
Re: [Qemu-devel] [PATCH v2 08/16] virtio: split vhost user scsi bits from virtio-pci
On 26/11/2018 21:00, Juan Quintela wrote: > Signed-off-by: Juan Quintela > --- > hw/virtio/Makefile.objs | 1 + > hw/virtio/vhost-user-scsi-pci.c | 101 > hw/virtio/virtio-pci.c | 58 -- > hw/virtio/virtio-pci.h | 11 > 4 files changed, 102 insertions(+), 69 deletions(-) > create mode 100644 hw/virtio/vhost-user-scsi-pci.c > Reviewed-by: Laurent Vivier
Re: [Qemu-devel] [RFC v2 23/24] WIP: Add missing instructions
On 11/27/18 1:10 PM, Alistair Francis wrote:
> Signed-off-by: Alistair Francis
> ---
> tcg/riscv/tcg-target.inc.c | 77 ++
> 1 file changed, 77 insertions(+)
>
> diff --git a/tcg/riscv/tcg-target.inc.c b/tcg/riscv/tcg-target.inc.c
> index 13756f6d0d..b8e9c0e126 100644
> --- a/tcg/riscv/tcg-target.inc.c
> +++ b/tcg/riscv/tcg-target.inc.c
> @@ -606,6 +606,57 @@ static bool tcg_out_sti(TCGContext *s, TCGType type,
> TCGArg val,
> return false;
> }
>
> +static void tcg_out_addsub2(TCGContext *s,
> +TCGReg rl, TCGReg rh,
> +TCGReg al, TCGReg ah,
> +TCGReg bl, TCGReg bh,
> +bool cbl, bool cbh, bool is_sub)
> +{
> +/* FIXME: This is just copied from MIPS */
> +TCGReg th = TCG_REG_TMP1;
> +
> +/* If we have a negative constant such that negating it would
> + make the high part zero, we can (usually) eliminate one insn. */
> +if (cbl && cbh && bh == -1 && bl != 0) {
> +bl = -bl;
> +bh = 0;
> +is_sub = !is_sub;
> +}
> +
> +/* By operating on the high part first, we get to use the final
> + carry operation to move back from the temporary. */
> +if (!cbh) {
> +tcg_out_opc_reg(s, (is_sub ? OPC_SUB : OPC_ADDI), th, ah, bh);
> +} else if (bh != 0 || ah == rl) {
> +tcg_out_opc_imm(s, OPC_ADDI, th, ah, (is_sub ? -bh : bh));
> +} else {
> +th = ah;
> +}
> +
> +if (is_sub) {
> +if (cbl) {
> +tcg_out_opc_imm(s, OPC_SLLI, TCG_REG_TMP0, al, bl);
s/SLLI/SLTIU/
> +tcg_out_opc_imm(s, OPC_ADDI, rl, al, -bl);
> +} else {
> +tcg_out_opc_reg(s, OPC_SLLI, TCG_REG_TMP0, al, bl);
s/SLLI/SLTU/
> +tcg_out_opc_reg(s, OPC_SUB, rl, al, bl);
> +}
> +tcg_out_opc_reg(s, OPC_SUB, rh, th, TCG_REG_TMP0);
> +} else {
> +if (cbl) {
> +tcg_out_opc_imm(s, OPC_ADDI, rl, al, bl);
> +tcg_out_opc_imm(s, OPC_SLLI, TCG_REG_TMP0, rl, bl);
SLTIU
> +} else if (rl == al && rl == bl) {
> +tcg_out_opc_imm(s, OPC_SRLI, TCG_REG_TMP0, al, 31);
s/31/TCG_TARGET_REG_BITS - 1/
or somesuch. I think that's an existing mips bug too...
> +tcg_out_opc_reg(s, OPC_ADDI, rl, al, bl);
ADD, not ADDI.
> +} else {
> +tcg_out_opc_reg(s, OPC_ADDI, rl, al, bl);
> +tcg_out_opc_reg(s, OPC_SLLI, TCG_REG_TMP0, rl, (rl == bl ? al :
> bl));
Similarly.
> +}
> +tcg_out_opc_reg(s, OPC_ADDI, rh, th, TCG_REG_TMP0);
ADD.
> @@ -1537,6 +1604,8 @@ static const TCGTargetOpDef
> *tcg_target_op_def(TCGOpcode op)
> case INDEX_op_ext16s_i32:
> case INDEX_op_ext16s_i64:
> case INDEX_op_ext32s_i64:
> +case INDEX_op_extrl_i64_i32:
> +case INDEX_op_extrh_i64_i32:
This belongs in a previous patch.
> +case INDEX_op_add2_i32:
> +case INDEX_op_add2_i64:
> +return _rZ_rZ_rZ_rZ_rZ;
> +
> +case INDEX_op_sub2_i32:
> +case INDEX_op_sub2_i64:
> +return _rZ_rZ_rZ_rZ_rZ;
Adding 0 with TCG_REG_ZERO is sorta pointless. You want the I and N
constraints. Note that MIPS uses a modified symmetric N that can always be
negated [-32767, 32767] instead of [-32767, 32768]. Otherwise that first if
statement doesn't work.
r~
Re: [Qemu-devel] [PATCH v2 06/16] virtio: split virtio 9p bits from virtio-pci
On 26/11/2018 20:59, Juan Quintela wrote: > Signed-off-by: Juan Quintela > --- > hw/virtio/Makefile.objs | 1 + > hw/virtio/virtio-9p-pci.c | 86 +++ > hw/virtio/virtio-pci.c| 52 --- > hw/virtio/virtio-pci.h| 20 - > 4 files changed, 87 insertions(+), 72 deletions(-) > create mode 100644 hw/virtio/virtio-9p-pci.c > And what about tests/virtio-9p-test ? except that: Reviewed-by: Laurent Vivier
Re: [Qemu-devel] [RFC v2 14/24] riscv: tcg-target: Add branch and jump instructions
On 11/27/18 1:08 PM, Alistair Francis wrote:
> +static inline void tcg_out_goto_long(TCGContext *s, tcg_insn_unit *target)
> +{
> +ptrdiff_t offset = tcg_pcrel_diff(s, target);
> +
> +if (offset == sextract64(offset, 0, 26)) {
> +tcg_out_opc_jump(s, OPC_JAL, TCG_REG_ZERO, offset);
> +} else {
> +tcg_out_movi(s, TCG_TYPE_I64, TCG_REG_TMP0, (intptr_t)target);
> +tcg_out_opc_jump(s, OPC_JAL, TCG_REG_TMP0, 0);
> +}
> +}
> +
> +static void tcg_out_call_int(TCGContext *s, tcg_insn_unit *arg, bool tail)
> +{
> +TCGReg link = tail ? TCG_REG_ZERO : TCG_REG_RA;
> +ptrdiff_t offset = tcg_pcrel_diff(s, arg);
> +if (offset == sextract64(offset, 1, 20) << 1) {
s/20/26/
Seems like there ought to be more shared code between tcg_out_call_int and
tcg_out_goto_long, really.
r~
[Qemu-devel] [PATCH] mirror dead-lock
Hi all!
We've faced the following mirror bug:
Just run mirror on qcow2 image more than 1G, and qemu is in dead lock.
Note: I've decided to send this as a patch with reproducer, to make it
easier to reproduce). No needs to commit this before mirror fix, but
after, commit message may be a bit shortened, may be even up to just
"iotests: add simple mirror test".
Note2: if drop 'kvm' option from the test it still reproduces, but if
use iotests.VM() - does not (may be, because of qtest?). If add
iothread, it doesn't reproduce too. Also, it doesn't reproduce with raw
instead of qcow2 and doesn't reproduce for small images.
So, here is the dead-lock:
(gdb) info thr
Id Target Id Frame
3Thread 0x7fd7fd4ea700 (LWP 145412) "qemu-system-x86" syscall () at
../sysdeps/unix/sysv/linux/x86_64/syscall.S:38
2Thread 0x7fd7fc205700 (LWP 145416) "qemu-system-x86" __lll_lock_wait ()
at ../nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135
* 1Thread 0x7fd8102cfcc0 (LWP 145411) "qemu-system-x86" __lll_lock_wait ()
at ../nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135
(gdb) bt
#0 0x7fd80e8864cd in __lll_lock_wait () at
../nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135
#1 0x7fd80e881dcb in _L_lock_812 () at /lib64/libpthread.so.0
#2 0x7fd80e881c98 in __GI___pthread_mutex_lock (mutex=0x561032dce420
) at ../nptl/pthread_mutex_lock.c:79
#3 0x561032654c3b in qemu_mutex_lock_impl (mutex=0x561032dce420
, file=0x5610327d8654 "util/main-loop.c", line=236) at
util/qemu-thread-posix.c:66
#4 0x5610320cb327 in qemu_mutex_lock_iothread_impl (file=0x5610327d8654
"util/main-loop.c", line=236) at /work/src/qemu/up-mirror-dead-lock/cpus.c:1849
#5 0x56103265038f in os_host_main_loop_wait (timeout=480116000) at
util/main-loop.c:236
#6 0x561032650450 in main_loop_wait (nonblocking=0) at
util/main-loop.c:497
#7 0x5610322575c9 in main_loop () at vl.c:1892
#8 0x56103225f3c7 in main (argc=13, argv=0x7ffcc8bb15a8,
envp=0x7ffcc8bb1618) at vl.c:4648
(gdb) p qemu_global_mutex
$1 = {lock = {__data = {__lock = 2, __count = 0, __owner = 145416, __nusers =
1, __kind = 0, __spins = 0, __elision = 0, __list = {__prev = 0x0, __next =
0x0}},
__size = "\002\000\000\000\000\000\000\000\b8\002\000\001", '\000'
, __align = 2}, file = 0x56103267bcb0
"/work/src/qemu/up-mirror-dead-lock/exec.c", line = 3197, initialized = true}
So, we see, that thr1 is main loop, and now it wants BQL, which is
owned by thr2.
(gdb) thr 2
[Switching to thread 2 (Thread 0x7fd7fc205700 (LWP 145416))]
#0 __lll_lock_wait () at
../nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135
135 2: movl%edx, %eax
(gdb) bt
#0 0x7fd80e8864cd in __lll_lock_wait () at
../nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135
#1 0x7fd80e881de6 in _L_lock_870 () at /lib64/libpthread.so.0
#2 0x7fd80e881cdf in __GI___pthread_mutex_lock (mutex=0x561034d25dc0) at
../nptl/pthread_mutex_lock.c:114
#3 0x561032654c3b in qemu_mutex_lock_impl (mutex=0x561034d25dc0,
file=0x5610327d81d1 "util/async.c", line=511) at util/qemu-thread-posix.c:66
#4 0x56103264d840 in aio_context_acquire (ctx=0x561034d25d60) at
util/async.c:511
#5 0x561032254554 in dma_blk_cb (opaque=0x7fd7f41463b0, ret=0) at
dma-helpers.c:169
#6 0x56103225479d in dma_blk_io (ctx=0x561034d25d60, sg=0x561035a80210,
offset=0, align=512, io_func=0x5610322547a3 ,
io_func_opaque=0x561034d40870, cb=0x561032340b35 ,
opaque=0x561035a7fee8, dir=DMA_DIRECTION_FROM_DEVICE) at dma-helpers.c:227
#7 0x561032254855 in dma_blk_read (blk=0x561034d40870, sg=0x561035a80210,
offset=0, align=512, cb=0x561032340b35 , opaque=0x561035a7fee8) at
dma-helpers.c:245
#8 0x561032340e6e in ide_dma_cb (opaque=0x561035a7fee8, ret=0) at
hw/ide/core.c:910
#9 0x56103234a912 in bmdma_cmd_writeb (bm=0x561035a81030, val=9) at
hw/ide/pci.c:240
#10 0x56103234b5bd in bmdma_write (opaque=0x561035a81030, addr=0, val=9,
size=1) at hw/ide/piix.c:76
#11 0x5610320e5763 in memory_region_write_accessor (mr=0x561035a81180,
addr=0, value=0x7fd7fc204678, size=1, shift=0, mask=255, attrs=...) at
/work/src/qemu/up-mirror-dead-lock/memory.c:504
#12 0x5610320e596d in access_with_adjusted_size (addr=0,
value=0x7fd7fc204678, size=1, access_size_min=1, access_size_max=4, access_fn=
0x5610320e5683 , mr=0x561035a81180,
attrs=...) at /work/src/qemu/up-mirror-dead-lock/memory.c:570
#13 0x5610320e86ce in memory_region_dispatch_write (mr=0x561035a81180,
addr=0, data=9, size=1, attrs=...) at
/work/src/qemu/up-mirror-dead-lock/memory.c:1452
#14 0x561032083770 in flatview_write_continue (fv=0x7fd7f4108090,
addr=49216, attrs=..., buf=0x7fd810309000 "\t\371\061", len=1, addr1=0, l=1,
mr=0x561035a81180)
at /work/src/qemu/up-mirror-dead-lock/exec.c:3233
#15 0x5610320838cf in flatview_write (fv=0x7fd7f4108090, addr=49216,
attrs=..., buf=0x7fd810309000
Re: [Qemu-devel] [PATCH] i2c: Move typedef of bitbang_i2c_interface to i2c.h
On 2018-11-28 21:03, Eric Blake wrote: > On 11/28/18 1:27 PM, BALATON Zoltan wrote: >> Clang 3.4 considers duplicate typedef in ppc4xx_i2c.h and >> bitbang_i2c.h an error even if they are identical. Move it to a common >> place to allow building with this clang version. >> >> Reported-by: Thomas Huth >> Signed-off-by: BALATON Zoltan >> --- >> hw/i2c/bitbang_i2c.h | 2 -- >> include/hw/i2c/i2c.h | 2 ++ >> include/hw/i2c/ppc4xx_i2c.h | 3 --- >> 3 files changed, 2 insertions(+), 5 deletions(-) >> >> diff --git a/hw/i2c/bitbang_i2c.h b/hw/i2c/bitbang_i2c.h >> index 3a7126d5de..9443021710 100644 >> --- a/hw/i2c/bitbang_i2c.h >> +++ b/hw/i2c/bitbang_i2c.h >> @@ -3,8 +3,6 @@ >> #include "hw/i2c/i2c.h" >> -typedef struct bitbang_i2c_interface bitbang_i2c_interface; > > Would this also be an opportunity to rename the struct to something > CamelCase to match our coding conventions? That's a different issue, so I'd say it should be done in a different patch. Thomas
Re: [Qemu-devel] [PATCH v2 05/16] virtio: split virtio balloon bits from virtio-pci
On 26/11/2018 20:59, Juan Quintela wrote: > Signed-off-by: Juan Quintela > Reviewed-by: Thomas Huth > Signed-off-by: Juan Quintela > --- > hw/virtio/Makefile.objs| 1 + > hw/virtio/virtio-balloon-pci.c | 94 ++ > hw/virtio/virtio-pci.c | 58 - > hw/virtio/virtio-pci.h | 14 - > tests/Makefile.include | 2 +- > 5 files changed, 96 insertions(+), 73 deletions(-) > create mode 100644 hw/virtio/virtio-balloon-pci.c > Reviewed-by: Laurent Vivier
Re: [Qemu-devel] [PATCH] MAINTAINERS: Add more files to sam460ex
On 2018-11-28 20:41, BALATON Zoltan wrote: > The sm501 model belonged to SH before but that seems to be inactive > now and latest changes were for sam460ex which is the more active user > of this device at the moment so let's adopt sm501 for sam460ex. > > Also add device tree and firmware sources and binaries. > > Signed-off-by: BALATON Zoltan > --- > MAINTAINERS | 4 > 1 file changed, 4 insertions(+) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 63effdc473..0c373be86e 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -1003,8 +1003,12 @@ sam460ex > M: BALATON Zoltan > L: qemu-...@nongnu.org > S: Maintained > +F: hw/display/sm501* > F: hw/ide/sii3112.c > F: hw/timer/m41t80.c > +F: pc-bios/canyonlands.dt[sb] > +F: pc-bios/u-boot-sam460ex-20100605.bin > +F: roms/u-boot-sam460ex Thanks for the update! ... but in case the SH4 R2D maintainer is interested in the sm501 device, too, I think it would maybe be cleaner to add a new paragraph in the "Devices" section of the MAINTAINERS file for the sm501 device...? Otherwise: Reviewed-by: Thomas Huth
Re: [Qemu-devel] [PATCH v2 04/16] virtio: split virtio rng bits from virtio-pci
On 26/11/2018 20:59, Juan Quintela wrote: > Signed-off-by: Juan Quintela > > --- > > Remove the "contributions after" clause. This is based on > > commit 59ccd20a9ac719cff82180429458728f03ec612f > Author: KONRAD Frederic > Date: Wed Apr 24 10:07:56 2013 +0200 > --- > hw/virtio/Makefile.objs| 1 + > hw/virtio/virtio-pci.c | 52 --- > hw/virtio/virtio-pci.h | 14 --- > hw/virtio/virtio-rng-pci.c | 86 ++ > tests/Makefile.include | 1 + > 5 files changed, 88 insertions(+), 66 deletions(-) > create mode 100644 hw/virtio/virtio-rng-pci.c ... > diff --git a/tests/Makefile.include b/tests/Makefile.include > index fb0b449c02..e8235890ec 100644 > --- a/tests/Makefile.include > +++ b/tests/Makefile.include > @@ -149,6 +149,7 @@ check-qtest-virtio-y += tests/virtio-net-test$(EXESUF) > check-qtest-virtio-y += tests/virtio-balloon-test$(EXESUF) > check-qtest-virtio-y += tests/virtio-blk-test$(EXESUF) > check-qtest-virtio-y += tests/virtio-rng-test$(EXESUF) > +check-qtest-virtio-$(CONFIG_VIRTIO_RNG) += tests/virtio-rng-test$(EXESUF) I think you should remove the line just above. except that: Reviewed-by: Laurent Vivier
Re: [Qemu-devel] [PATCH] i2c: Move typedef of bitbang_i2c_interface to i2c.h
On 11/28/18 1:27 PM, BALATON Zoltan wrote: Clang 3.4 considers duplicate typedef in ppc4xx_i2c.h and bitbang_i2c.h an error even if they are identical. Move it to a common place to allow building with this clang version. Reported-by: Thomas Huth Signed-off-by: BALATON Zoltan --- hw/i2c/bitbang_i2c.h| 2 -- include/hw/i2c/i2c.h| 2 ++ include/hw/i2c/ppc4xx_i2c.h | 3 --- 3 files changed, 2 insertions(+), 5 deletions(-) diff --git a/hw/i2c/bitbang_i2c.h b/hw/i2c/bitbang_i2c.h index 3a7126d5de..9443021710 100644 --- a/hw/i2c/bitbang_i2c.h +++ b/hw/i2c/bitbang_i2c.h @@ -3,8 +3,6 @@ #include "hw/i2c/i2c.h" -typedef struct bitbang_i2c_interface bitbang_i2c_interface; Would this also be an opportunity to rename the struct to something CamelCase to match our coding conventions? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH] i2c: Move typedef of bitbang_i2c_interface to i2c.h
On 2018-11-28 20:27, BALATON Zoltan wrote: > Clang 3.4 considers duplicate typedef in ppc4xx_i2c.h and > bitbang_i2c.h an error even if they are identical. Move it to a common > place to allow building with this clang version. > > Reported-by: Thomas Huth > Signed-off-by: BALATON Zoltan > --- > hw/i2c/bitbang_i2c.h| 2 -- > include/hw/i2c/i2c.h| 2 ++ > include/hw/i2c/ppc4xx_i2c.h | 3 --- > 3 files changed, 2 insertions(+), 5 deletions(-) > > diff --git a/hw/i2c/bitbang_i2c.h b/hw/i2c/bitbang_i2c.h > index 3a7126d5de..9443021710 100644 > --- a/hw/i2c/bitbang_i2c.h > +++ b/hw/i2c/bitbang_i2c.h > @@ -3,8 +3,6 @@ > > #include "hw/i2c/i2c.h" > > -typedef struct bitbang_i2c_interface bitbang_i2c_interface; > - > #define BITBANG_I2C_SDA 0 > #define BITBANG_I2C_SCL 1 > > diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h > index 5dc166158b..cf4c45a98f 100644 > --- a/include/hw/i2c/i2c.h > +++ b/include/hw/i2c/i2c.h > @@ -82,6 +82,8 @@ int i2c_recv(I2CBus *bus); > > DeviceState *i2c_create_slave(I2CBus *bus, const char *name, uint8_t addr); > > +typedef struct bitbang_i2c_interface bitbang_i2c_interface; > + > /* lm832x.c */ > void lm832x_key_event(DeviceState *dev, int key, int state); > > diff --git a/include/hw/i2c/ppc4xx_i2c.h b/include/hw/i2c/ppc4xx_i2c.h > index 0891a9c948..b3450bacf7 100644 > --- a/include/hw/i2c/ppc4xx_i2c.h > +++ b/include/hw/i2c/ppc4xx_i2c.h > @@ -31,9 +31,6 @@ > #include "hw/sysbus.h" > #include "hw/i2c/i2c.h" > > -/* from hw/i2c/bitbang_i2c.h */ > -typedef struct bitbang_i2c_interface bitbang_i2c_interface; > - > #define TYPE_PPC4xx_I2C "ppc4xx-i2c" > #define PPC4xx_I2C(obj) OBJECT_CHECK(PPC4xxI2CState, (obj), TYPE_PPC4xx_I2C) Reviewed-by: Thomas Huth Peter, in case you've got to roll another rc, I think this should be included, too. Otherwise, I think this can also be included later via qemu-stable (for the few people like me who still use clang 3.4). Thomas
[Qemu-devel] [PATCH] i2c: Move typedef of bitbang_i2c_interface to i2c.h
Clang 3.4 considers duplicate typedef in ppc4xx_i2c.h and bitbang_i2c.h an error even if they are identical. Move it to a common place to allow building with this clang version. Reported-by: Thomas Huth Signed-off-by: BALATON Zoltan --- hw/i2c/bitbang_i2c.h| 2 -- include/hw/i2c/i2c.h| 2 ++ include/hw/i2c/ppc4xx_i2c.h | 3 --- 3 files changed, 2 insertions(+), 5 deletions(-) diff --git a/hw/i2c/bitbang_i2c.h b/hw/i2c/bitbang_i2c.h index 3a7126d5de..9443021710 100644 --- a/hw/i2c/bitbang_i2c.h +++ b/hw/i2c/bitbang_i2c.h @@ -3,8 +3,6 @@ #include "hw/i2c/i2c.h" -typedef struct bitbang_i2c_interface bitbang_i2c_interface; - #define BITBANG_I2C_SDA 0 #define BITBANG_I2C_SCL 1 diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h index 5dc166158b..cf4c45a98f 100644 --- a/include/hw/i2c/i2c.h +++ b/include/hw/i2c/i2c.h @@ -82,6 +82,8 @@ int i2c_recv(I2CBus *bus); DeviceState *i2c_create_slave(I2CBus *bus, const char *name, uint8_t addr); +typedef struct bitbang_i2c_interface bitbang_i2c_interface; + /* lm832x.c */ void lm832x_key_event(DeviceState *dev, int key, int state); diff --git a/include/hw/i2c/ppc4xx_i2c.h b/include/hw/i2c/ppc4xx_i2c.h index 0891a9c948..b3450bacf7 100644 --- a/include/hw/i2c/ppc4xx_i2c.h +++ b/include/hw/i2c/ppc4xx_i2c.h @@ -31,9 +31,6 @@ #include "hw/sysbus.h" #include "hw/i2c/i2c.h" -/* from hw/i2c/bitbang_i2c.h */ -typedef struct bitbang_i2c_interface bitbang_i2c_interface; - #define TYPE_PPC4xx_I2C "ppc4xx-i2c" #define PPC4xx_I2C(obj) OBJECT_CHECK(PPC4xxI2CState, (obj), TYPE_PPC4xx_I2C) -- 2.13.7
[Qemu-devel] [PATCH] MAINTAINERS: Add more files to sam460ex
The sm501 model belonged to SH before but that seems to be inactive now and latest changes were for sam460ex which is the more active user of this device at the moment so let's adopt sm501 for sam460ex. Also add device tree and firmware sources and binaries. Signed-off-by: BALATON Zoltan --- MAINTAINERS | 4 1 file changed, 4 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 63effdc473..0c373be86e 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1003,8 +1003,12 @@ sam460ex M: BALATON Zoltan L: qemu-...@nongnu.org S: Maintained +F: hw/display/sm501* F: hw/ide/sii3112.c F: hw/timer/m41t80.c +F: pc-bios/canyonlands.dt[sb] +F: pc-bios/u-boot-sam460ex-20100605.bin +F: roms/u-boot-sam460ex SH4 Machines -- 2.13.7
Re: [Qemu-devel] [RFC v2 11/24] riscv: tcg-target: Add the mov and movi instruction
On 11/27/18 1:08 PM, Alistair Francis wrote:
> +static void tcg_out_movi(TCGContext *s, TCGType type, TCGReg rd,
> + tcg_target_long val)
> +{
> +#if TCG_TARGET_REG_BITS == 64
> +tcg_target_long lo = sextract64(val, 0, 12);
> +#else
> +tcg_target_long lo = sextract32(val, 0, 12);
> +#endif
I really think this ifdef should be moved into a helper function. There are
quite a number of copies of it throughout.
> +tcg_target_long hi = val - lo;
> +int shift;
> +tcg_target_long tmp;
> +
> +RISCVInsn add32_op = TCG_TARGET_REG_BITS == 64 ? OPC_ADDIW : OPC_ADDI;
> +
> +#if TCG_TARGET_REG_BITS == 64
> +ptrdiff_t offset = tcg_pcrel_diff(s, (void *)val);
> +#endif
> +
> +if (TCG_TARGET_REG_BITS == 32 || val == (int32_t)val) {
> +tcg_out_opc_upper(s, OPC_LUI, rd, hi);
> +if (lo != 0) {
> +tcg_out_opc_imm(s, add32_op, rd, rd, lo);
> +}
> +
> +return;
> +}
Your reorg has failed to keep the single insn ADDI(W) case.
> +/* We can only be here if TCG_TARGET_REG_BITS != 32 */
> +if (offset == sextract64(offset, 1, 31) << 1) {
The ifdef above won't compile for rv32, because offset is used unconditionally
here. Remove the ifdef and allow that to be removed as dead code by the
compiler.
> +tcg_out_opc_upper(s, OPC_AUIPC, rd, 0);
> +tcg_out_opc_imm(s, OPC_ADDI, rd, rd, 0);
> +reloc_call(s->code_ptr - 2, (tcg_insn_unit *)val);
> +return;
> +}
> +
> +shift = ctz64(val);
> +tmp = val >> shift;
> +
> +if (tmp == sextract64(tmp, 0, 12)) {
> +tcg_out_opc_imm(s, OPC_ADDI, rd, TCG_REG_ZERO, 1);
s/1/tmp/
> +tcg_out_opc_imm(s, OPC_SLLI, rd, rd, ctz64(val));
> +} else if (!(val >> 31 == 0 || val >> 31 == -1)) {
> +shift = ctz64(hi);
> +hi >>= shift;
> +tcg_out_movi(s, type, rd, hi);
> +tcg_out_opc_imm(s, OPC_SLLI, rd, rd, shift);
> +if (lo != 0) {
> +tcg_out_opc_imm(s, OPC_ADDI, rd, rd, lo);
> +}
> +} else {
> +if (hi != 0) {
> +tcg_out_opc_upper(s, OPC_LUI, rd, hi);
> +}
> +if (lo != 0) {
> +tcg_out_opc_imm(s, add32_op, rd, hi == 0 ? TCG_REG_ZERO : rd,
> lo);
> +}
> +}
The final else case is identical with the first if.
r~
Re: [Qemu-devel] (no subject)
On Wed, 28 Nov 2018 at 01:12, John Arbuckle wrote: > > From af4497f2b161bb4165acb8eee5cae3f2a7ea2227 Mon Sep 17 00:00:00 2001 > From: John Arbuckle > Date: Tue, 27 Nov 2018 20:01:20 -0500 > Subject: [PATCH] ui/cocoa.m: fix crash due to cocoa_refresh() on Mac OS 10.14 Something seems to have got the formatting of this patch email wrong -- it's got all this in the body and the actual Subject line of the email is blank. > Mac OS 10.14 only wants UI code to be called from the main thread. The > cocoa_refresh() function is called on another thread and this causes a > crash to take place. To fix this problem the cocoa_refresh() code is > called from the main thread only. > > Signed-off-by: John Arbuckle > --- > ui/cocoa.m | 59 ++- > 1 file changed, 34 insertions(+), 25 deletions(-) I get a compile warning with this patch: /Users/pm215/src/qemu/ui/cocoa.m:1615:23: warning: instance method '-cocoa_refresh' not found (return type defaults to 'id') [-Wobjc-method-access] [[NSApp delegate] cocoa_refresh]; ^ To be honest, I'm still confused about what is causing the problems on Mojave. The refresh method should be being called on the main thread even with the code on master -- it's just that that is the iothread and it's running the event pumping code in the refresh callback rather than a standard OSX event loop. I'm hoping that the backtrace with symbols of the Mojave assertion failure will help there, since I can't currently see where refresh gets called from some non-main thread. I also think that this patch doesn't address the problems of locking that I mention on the discussion thread for Berkus' patch. It also doesn't handle any of the other callbacks from QEMU to the cocoa UI -- surely we need to handle all of them if there is a problem here? (I have some prototype patches which I've been working on which address the locking problem and also make all the QEMU callbacks run their work on the main thread. I may be able get those into shape to post those next week.) thanks -- PMM
Re: [Qemu-devel] [RFC v2 10/24] riscv: tcg-target: Add the relocation functions
On 11/27/18 1:08 PM, Alistair Francis wrote: > Signed-off-by: Alistair Francis > Signed-off-by: Michael Clark > --- > tcg/riscv/tcg-target.inc.c | 51 ++ > 1 file changed, 51 insertions(+) Reviewed-by: Richard Henderson r~
