Re: [Qemu-devel] [PATCH v9 4/8] migration: API to clear bits of guest free pages from the dirty bitmap
On Tue, Nov 27, 2018 at 02:52:35PM +0800, Wei Wang wrote: > On 11/27/2018 02:06 PM, Peter Xu wrote: > > On Thu, Nov 15, 2018 at 06:08:00PM +0800, Wei Wang wrote: > > Again, is it possible to resize during migration? > > > > So I think the check is fine, but uncertain about the comment. > > Yes, resize would not happen with the current implementation. > But heard it could just be a temporal implementation. Probably > we could improve the comment like this: > > " > Though the implementation might not support ram resize currently, > this could happen in theory with future updates. So the check here > handles the case that RAMBLOCK is resized after the free page hint is > reported. > " I'm not familiar with that part, but this seems ok to me. Thanks, -- Peter Xu
Re: [Qemu-devel] [virtio-dev] Re: [PATCH v9 3/8] migration: use bitmap_mutex in migration_bitmap_clear_dirty
On Tue, Nov 27, 2018 at 02:12:34PM +0800, Wei Wang wrote:
> On 11/27/2018 02:02 PM, Wei Wang wrote:
> > On 11/27/2018 01:40 PM, Peter Xu wrote:
> > > On Thu, Nov 15, 2018 at 06:07:59PM +0800, Wei Wang wrote:
> > > > The bitmap mutex is used to synchronize threads to update the dirty
> > > > bitmap and the migration_dirty_pages counter. For example, the free
> > > > page optimization clears bits of free pages from the bitmap in an
> > > > iothread context. This patch makes migration_bitmap_clear_dirty update
> > > > the bitmap and counter under the mutex.
> > > >
> > > > Signed-off-by: Wei Wang
> > > > CC: Dr. David Alan Gilbert
> > > > CC: Juan Quintela
> > > > CC: Michael S. Tsirkin
> > > > CC: Peter Xu
> > > > ---
> > > > migration/ram.c | 3 +++
> > > > 1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/migration/ram.c b/migration/ram.c
> > > > index 7e7deec..ef69dbe 100644
> > > > --- a/migration/ram.c
> > > > +++ b/migration/ram.c
> > > > @@ -1556,11 +1556,14 @@ static inline bool
> > > > migration_bitmap_clear_dirty(RAMState *rs,
> > > > {
> > > > bool ret;
> > > > +qemu_mutex_lock(>bitmap_mutex);
> > > > ret = test_and_clear_bit(page, rb->bmap);
> > > > if (ret) {
> > > > rs->migration_dirty_pages--;
> > > > }
> > > > +qemu_mutex_unlock(>bitmap_mutex);
> > > > +
> > > > return ret;
> > > > }
> > > It seems fine to me, but have you thought about
> > > test_and_clear_bit_atomic()? Note that we just had
> > > test_and_set_bit_atomic() a few months ago.
> >
> > Thanks for sharing. I think we might also need to
> > mutex migration_dirty_pages.
> >
> > >
> > > And not related to this patch: I'm unclear on why we have had
> > > bitmap_mutex before, since it seems unnecessary.
> >
> > OK. This is because with the optimization we have a thread
> > which clears bits (of free pages) from the bitmap and updates
> > migration_dirty_pages. So we need to synchronization between
> > the migration thread and the optimization thread.
> >
>
> And before this feature, I think yes, that bitmap_mutex is not needed.
> It was left there due to some historical reasons.
> I remember Dave previous said he was about to remove it. But the new
> feature will need it again.
Ok then I'm fine with it. Though you could update the comments too if
you like:
/* protects modification of the bitmap and migration_dirty_pages */
QemuMutex bitmap_mutex;
And it's tricky that sometimes we don't take the lock when reading
this variable "migration_dirty_pages". I don't see obvious issue so
far, hope it's true (at least I skipped the colo ones...).
ram_bytes_remaining[333] return ram_state ?
(ram_state->migration_dirty_pages * TARGET_PAGE_SIZE) :
migration_bitmap_clear_dirty[1562] rs->migration_dirty_pages--;
migration_bitmap_sync_range[1570] rs->migration_dirty_pages +=
postcopy_chunk_hostpages_pass[2809] rs->migration_dirty_pages +=
!test_and_set_bit(page, bitmap);
ram_state_init[3037] (*rsp)->migration_dirty_pages =
ram_bytes_total() >> TARGET_PAGE_BITS;
ram_state_resume_prepare[3112] rs->migration_dirty_pages = pages;
ram_save_pending[3344] remaining_size = rs->migration_dirty_pages *
TARGET_PAGE_SIZE;
ram_save_pending[3353] remaining_size = rs->migration_dirty_pages *
TARGET_PAGE_SIZE;
colo_cache_from_block_offset[3468] ram_state->migration_dirty_pages++;
colo_init_ram_cache[3716] ram_state->migration_dirty_pages = 0;
colo_flush_ram_cache[3997]
trace_colo_flush_ram_cache_begin(ram_state->migration_dirty_pages);
Regards,
--
Peter Xu
Re: [Qemu-devel] [PATCH v9 5/8] migration/ram.c: add a notifier chain for precopy
On Thu, Nov 15, 2018 at 06:08:01PM +0800, Wei Wang wrote:
> This patch adds a notifier chain for the memory precopy. This enables various
> precopy optimizations to be invoked at specific places.
>
> Signed-off-by: Wei Wang
> CC: Dr. David Alan Gilbert
> CC: Juan Quintela
> CC: Michael S. Tsirkin
> CC: Peter Xu
> ---
> include/migration/misc.h | 12
> migration/ram.c | 31 +++
> vl.c | 1 +
> 3 files changed, 44 insertions(+)
>
> diff --git a/include/migration/misc.h b/include/migration/misc.h
> index 113320e..0bac623 100644
> --- a/include/migration/misc.h
> +++ b/include/migration/misc.h
> @@ -19,6 +19,18 @@
>
> /* migration/ram.c */
>
> +typedef enum PrecopyNotifyReason {
> +PRECOPY_NOTIFY_ERR = 0,
> +PRECOPY_NOTIFY_START_ITERATION = 1,
> +PRECOPY_NOTIFY_BEFORE_SYNC_BITMAP = 2,
> +PRECOPY_NOTIFY_AFTER_SYNC_BITMAP = 3,
> +PRECOPY_NOTIFY_MAX = 4,
It would be nice to add some comments for each of the notify reason.
E.g., from the name PRECOPY_NOTIFY_START_ITERATION seems more like a
hook at the start of each iteration but according to [1] it should be
at the start of migration rather than each iteration (or when
migration restarts, though I'm not sure whether we really have this
yet).
> +} PrecopyNotifyReason;
> +
> +void precopy_infrastructure_init(void);
> +void precopy_add_notifier(Notifier *n);
> +void precopy_remove_notifier(Notifier *n);
> +
> void ram_mig_init(void);
> void qemu_guest_free_page_hint(void *addr, size_t len);
>
> diff --git a/migration/ram.c b/migration/ram.c
> index 229b791..65b1223 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -292,6 +292,8 @@ struct RAMState {
> bool ram_bulk_stage;
> /* How many times we have dirty too many pages */
> int dirty_rate_high_cnt;
> +/* ram save states used for notifiers */
> +int ram_save_state;
This can be removed?
> /* these variables are used for bitmap sync */
> /* last time we did a full bitmap_sync */
> int64_t time_last_bitmap_sync;
> @@ -328,6 +330,28 @@ typedef struct RAMState RAMState;
>
> static RAMState *ram_state;
>
> +static NotifierList precopy_notifier_list;
> +
> +void precopy_infrastructure_init(void)
> +{
> +notifier_list_init(_notifier_list);
> +}
> +
> +void precopy_add_notifier(Notifier *n)
> +{
> +notifier_list_add(_notifier_list, n);
> +}
> +
> +void precopy_remove_notifier(Notifier *n)
> +{
> +notifier_remove(n);
> +}
> +
> +static void precopy_notify(PrecopyNotifyReason reason)
> +{
> +notifier_list_notify(_notifier_list, );
> +}
> +
> uint64_t ram_bytes_remaining(void)
> {
> return ram_state ? (ram_state->migration_dirty_pages * TARGET_PAGE_SIZE)
> :
> @@ -1642,6 +1666,8 @@ static void migration_bitmap_sync(RAMState *rs)
> int64_t end_time;
> uint64_t bytes_xfer_now;
>
> +precopy_notify(PRECOPY_NOTIFY_BEFORE_SYNC_BITMAP);
> +
> ram_counters.dirty_sync_count++;
>
> if (!rs->time_last_bitmap_sync) {
> @@ -1699,6 +1725,8 @@ static void migration_bitmap_sync(RAMState *rs)
> if (migrate_use_events()) {
> qapi_event_send_migration_pass(ram_counters.dirty_sync_count);
> }
> +
> +precopy_notify(PRECOPY_NOTIFY_AFTER_SYNC_BITMAP);
> }
>
> /**
> @@ -2555,6 +2583,8 @@ static void ram_state_reset(RAMState *rs)
> rs->last_page = 0;
> rs->last_version = ram_list.version;
> rs->ram_bulk_stage = true;
> +
> +precopy_notify(PRECOPY_NOTIFY_START_ITERATION);
[1]
> }
>
> #define MAX_WAIT 50 /* ms, half buffered_file limit */
> @@ -3324,6 +3354,7 @@ out:
>
> ret = qemu_file_get_error(f);
> if (ret < 0) {
> +precopy_notify(PRECOPY_NOTIFY_ERR);
Could you show me which function is this line in?
Line 3324 here is ram_save_complete(), but I cannot find this exact
place.
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
- you'll have a solid point that you'll 100% guarantee that we'll
stop the free page hinting and don't need to worry about whether
there is chance the hinting will be running without an end [2].
Regarding [2] above: now the series only stops the hinting when
PRECOPY_NOTIFY_BEFORE_SYNC_BITMAP is notified. Could there be a case
that it's missing? E.g., what if we cancel/fail a migration during
precopy? Have you tried it?
Regards,
--
Peter Xu
Re: [Qemu-devel] [PATCH] fmops: fix off-by-one in AR_TABLE and DR_TABLE array size
On 2018-10-30 09:23, Gerd Hoffmann wrote:
> Fixes: CVE-2018-???
> Cc: P J P
> Reported-by: Wangjunqing
> Suggested-by: Paolo Bonzini
> Signed-off-by: Gerd Hoffmann
> ---
> hw/audio/fmopl.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/audio/fmopl.h b/hw/audio/fmopl.h
> index e7e578a48e..7199afaa3c 100644
> --- a/hw/audio/fmopl.h
> +++ b/hw/audio/fmopl.h
> @@ -72,8 +72,8 @@ typedef struct fm_opl_f {
> /* Rhythm sention */
> uint8_t rhythm; /* Rhythm mode , key flag */
> /* time tables */
> - int32_t AR_TABLE[75]; /* atttack rate tables */
> - int32_t DR_TABLE[75]; /* decay rate tables */
> + int32_t AR_TABLE[76]; /* atttack rate tables */
> + int32_t DR_TABLE[76]; /* decay rate tables */
> uint32_t FN_TABLE[1024]; /* fnumber -> increment counter */
> /* LFO */
> int32_t *ams_table;
>
CC to qemu-stable ?
Thomas
Re: [Qemu-devel] [PATCH v5 02/36] ppc/xive: add support for the LSI interrupt sources
On 11/27/18 12:48 AM, David Gibson wrote:
> On Mon, Nov 26, 2018 at 12:20:19PM +0100, Cédric Le Goater wrote:
>> On 11/26/18 6:39 AM, David Gibson wrote:
>>> On Fri, Nov 23, 2018 at 02:28:35PM +0100, Cédric Le Goater wrote:
+/*
+ * Returns whether the event notification should be forwarded.
+ */
+static bool xive_source_lsi_trigger(XiveSource *xsrc, uint32_t
srcno)
>>>
>>> What exactly "trigger" means isn't entirely obvious for an LSI. Might
>>> be clearer to have "lsi_assert" and "lsi_deassert" helpers instead.
>>
>> This is called only when the interrupt is asserted. So it is a
>> simplified LSI trigger depending only on the 'P' bit.
>
> Yes, I see that. But the result is that while the MSI logic is
> encapsulated in the MSI trigger function, this leaves the LSI logic
> split across the trigger function and set_irq() itself. I think it
> would be better to have assert and deassert helpers instead, which
> handle both the trigger/notification and also the updating of the
> ASSERTED bit.
Something like the xive_source_set_irq_lsi() below ?
>>>
>>> Uh.. not exactly what I had in mind, but close enough.
>>>
>>> [snip]
>>> +/*
+ * Returns whether the event notification should be forwarded.
+ */
static bool xive_source_esb_trigger(XiveSource *xsrc, uint32_t srcno)
{
+bool notify;
+
assert(srcno < xsrc->nr_irqs);
-return xive_esb_trigger(>status[srcno]);
+notify = xive_esb_trigger(>status[srcno]);
+
+if (xive_source_irq_is_lsi(xsrc, srcno) &&
>>>
>>> Except that this block can go, since this function is no longer called
>>> for LSIs.
>>
>> It still can be through the ESB MMIOs, if the guest does a load on the
>> trigger page.
>
> Oh, good point. That makes me rethink all my comments on this matter.
>
> In that case I think your original code was fine, except that I'd
> prefer to see the setting of the ASSERTED bit inside the trigger
> function, instead of in the set_irq() caller.
ok. I will change that.
Thanks,
C.
>
>
>>
>> C.
>>
>>
>>>
+xive_source_esb_get(xsrc, srcno) == XIVE_ESB_QUEUED) {
+qemu_log_mask(LOG_GUEST_ERROR,
+ "XIVE: queued an event on LSI IRQ %d\n", srcno);
+}
+
+return notify;
}
/*
@@ -103,9 +127,22 @@ static bool xive_source_esb_trigger(Xive
*/
static bool xive_source_esb_eoi(XiveSource *xsrc, uint32_t srcno)
{
+bool notify;
+
assert(srcno < xsrc->nr_irqs);
-return xive_esb_eoi(>status[srcno]);
+notify = xive_esb_eoi(>status[srcno]);
+
+/* LSI sources do not set the Q bit but they can still be
+ * asserted, in which case we should forward a new event
+ * notification
+ */
+if (xive_source_irq_is_lsi(xsrc, srcno)) {
+bool level = xsrc->status[srcno] & XIVE_STATUS_ASSERTED;
+notify = xive_source_set_irq_lsi(xsrc, srcno, level);
+}
+
+return notify;
}
/*
@@ -268,8 +305,12 @@ static void xive_source_set_irq(void *op
XiveSource *xsrc = XIVE_SOURCE(opaque);
bool notify = false;
-if (val) {
-notify = xive_source_esb_trigger(xsrc, srcno);
+if (xive_source_irq_is_lsi(xsrc, srcno)) {
+notify = xive_source_set_irq_lsi(xsrc, srcno, val);
+} else {
+if (val) {
+notify = xive_source_esb_trigger(xsrc, srcno);
+}
}
/* Forward the source event notification for routing */
@@ -289,9 +330,11 @@ void xive_source_pic_print_info(XiveSour
continue;
}
-monitor_printf(mon, " %08x %c%c\n", i + offset,
+monitor_printf(mon, " %08x %s %c%c%c\n", i + offset,
+ xive_source_irq_is_lsi(xsrc, i) ? "LSI" : "MSI",
pq & XIVE_ESB_VAL_P ? 'P' : '-',
- pq & XIVE_ESB_VAL_Q ? 'Q' : '-');
+ pq & XIVE_ESB_VAL_Q ? 'Q' : '-',
+ xsrc->status[i] & XIVE_STATUS_ASSERTED ? 'A' : '
');
}
}
@@ -299,6 +342,8 @@ static void xive_source_reset(DeviceStat
{
XiveSource *xsrc = XIVE_SOURCE(dev);
+/* Do not clear the LSI bitmap */
+
/* PQs are initialized to 0b01 which corresponds to "ints off" */
memset(xsrc->status, 0x1, xsrc->nr_irqs);
}
@@ -324,6 +369,7 @@ static void xive_source_realize(DeviceSt
xsrc->nr_irqs);
xsrc->status = g_malloc0(xsrc->nr_irqs);
+xsrc->lsi_map = bitmap_new(xsrc->nr_irqs);
Re: [Qemu-devel] [PATCH v5 04/36] ppc/xive: introduce the XiveRouter model
On 11/27/18 1:11 AM, David Gibson wrote: > On Mon, Nov 26, 2018 at 10:39:44AM +0100, Cédric Le Goater wrote: >> On 11/26/18 6:44 AM, David Gibson wrote: >>> On Fri, Nov 23, 2018 at 11:28:24AM +0100, Cédric Le Goater wrote: On 11/23/18 2:10 AM, David Gibson wrote: > On Thu, Nov 22, 2018 at 05:50:07PM +1100, Benjamin Herrenschmidt wrote: >> On Thu, 2018-11-22 at 15:44 +1100, David Gibson wrote: >>> >>> Sorry, didn't think of this in my first reply. >>> >>> 1) Does the hardware ever actually write back to the EAS? I know it >>> does for the END, but it's not clear why it would need to for the >>> EAS. If not, we don't need the setter. >> >> Nope, though the PAPR model will via hcalls > > Right, bit AIUI the set_eas hook is about abstracting PAPR vs bare > metal details. Since the hcall knows it's PAPR it can just update the > backing information for the EAS directly, and no need for an > abstracted hook. Indeed, the first versions of the XIVE patchset did not use such hooks, but when discussed we said we wanted abstract methods for the router to validate the overall XIVE model, which is useful for PowerNV. We can change again and have the hcalls get/set directly in the EAT and ENDT. It would certainly simplify the sPAPR model. >>> >>> I think that's the better approach. >> >> ok. let's keep that in mind for : >> >> [PATCH v5 11/36] spapr/xive: use the VCPU id as a NVT identifier >> [PATCH v5 16/36] spapr: add hcalls support for the XIVE exploitation >> >> which are using the XiveRouter methods to access the controller EAT >> and ENDT. I thought that was good practice to validate the model but >> we can use direct sPAPR table accessors or none at all. > > Ok. Consistency is good as a general rule, but I don't think it makes > sense to force the EAT and the ENDT into the same model. What do you mean by model ? the QEMU machine IC model ? > The EAT is > pure configuration, whereas the the ENDT has both configuration and > status. Or to look at it another way, the EAT is purely software > controlled, whereas the ENDT is at least partially hardware > controlled. yes but the EAT and the ENDT are XIVE internal tables of the same XIVE sub-engine, the IVRE, Interrupt Virtualization Routing Engine, formely known as VC, for Virtualization Controller. the EAS is just an entry point to the ENDT. I don't see why we would use different models for them. > (I realize that gets a bit fuzzy when considering PAPR, but I think > from the point of view of the XIVE model it makes sense to treat the > PAPR hypervisor logic as "software", even though it's "hardware" from the > guest point of view). That I agree but the resulting code is too ugly in the hcalls. Tell me when you reach patch 11, which links the machine IC model sPAPRXive to the generic XiveRouter and also check patch 16 introducing the hcalls, which update the XIVE internal tables. Thanks, C. >> >> >> I think these prereq patches could be merged now : >> >> [PATCH v5 12/36] spapr: initialize VSMT before initializing the IRQ >> [PATCH v5 13/36] spapr: introduce a spapr_irq_init() routine >> [PATCH v5 14/36] spapr: modify the irq backend 'init' method >> >> This one also : >> >> [PATCH v5 21/36] spapr: extend the sPAPR IRQ backend for XICS >> >> Thanks, >> >> C. >> >
Re: [Qemu-devel] [PATCH for-3.1 25/25] MAINTAINERS: Remove duplicate entries of qemu-devel@nongnu.org
On 2018-11-25 21:50, Philippe Mathieu-Daudé wrote: > The list is always selected by the 'All patches CC here' section. > > Signed-off-by: Philippe Mathieu-Daudé > --- > MAINTAINERS | 13 - > 1 file changed, 13 deletions(-) > > diff --git a/MAINTAINERS b/MAINTAINERS > index c616861ca3..9d6dae71ff 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -109,7 +109,6 @@ L: qemu-s3...@nongnu.org > Guest CPU cores (TCG): > -- > Overall > -L: qemu-devel@nongnu.org > M: Peter Crosthwaite > M: Richard Henderson > R: Paolo Bonzini > @@ -403,30 +402,25 @@ Hosts: > -- > > LINUX > -L: qemu-devel@nongnu.org > S: Maintained > F: linux-* > F: linux-headers/ > > POSIX > -L: qemu-devel@nongnu.org > S: Maintained > F: *posix* I agree that we can remove the "L: qemu-devel" entries everywhere, but for these subsystems that have no "M:" entry, but "S: Maintained", this now looks somewhat strange - the subsystem is "Maintained" but has no maintainer? I think we need somebody who feels responsible to pick up patches here...? Thomas
Re: [Qemu-devel] [PATCH for-3.1 24/25] MAINTAINERS: Use my work email to review Build and test automation patches
On 2018-11-25 21:49, Philippe Mathieu-Daudé wrote: > Signed-off-by: Philippe Mathieu-Daudé > --- > MAINTAINERS | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/MAINTAINERS b/MAINTAINERS > index ad82c0377c..c616861ca3 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -2388,7 +2388,7 @@ Build and test automation > Build and test automation > M: Alex Bennée > M: Fam Zheng > -R: Philippe Mathieu-Daudé > +R: Philippe Mathieu-Daudé > L: qemu-devel@nongnu.org > S: Maintained > F: .travis.yml > Reviewed-by: Thomas Huth
Re: [Qemu-devel] [PATCH for-3.1 22/25] MAINTAINERS: Add a missing entry to the New World machines
On 2018-11-25 21:49, Philippe Mathieu-Daudé wrote: > The CHRP NVRAM has his own section, but since the New World > section contains the mac_nvram.c, it makes sens to also monitor > the corresponding header. > > Signed-off-by: Philippe Mathieu-Daudé > --- > MAINTAINERS | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 92691589b1..a248c8c564 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -930,6 +930,7 @@ F: hw/nvram/mac_nvram.c > F: include/hw/misc/macio/ > F: include/hw/misc/mos6522.h > F: include/hw/ppc/mac_dbdma.h > +F: include/hw/nvram/chrp_nvram.h I don't see the point why we should add duplicated entries here? The header is also used by the Sparc machines, so why should this be added additionally to the Mac machines only here, too? I suggest to drop this patch. Thomas
Re: [Qemu-devel] [PATCH for-3.1 21/25] MAINTAINERS: Add a missing entry for the NVDIMM device
On 2018-11-25 21:49, Philippe Mathieu-Daudé wrote: > Signed-off-by: Philippe Mathieu-Daudé > --- > MAINTAINERS | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 26727c1561..92691589b1 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -1475,6 +1475,7 @@ S: Maintained > F: hw/acpi/nvdimm.c > F: hw/mem/nvdimm.c > F: include/hw/mem/nvdimm.h > +F: docs/nvdimm.txt > > e1000x > M: Dmitry Fleytman > Reviewed-by: Thomas Huth
Re: [Qemu-devel] [PATCH for-3.1 14/25] MAINTAINERS: Add missing entries to the vhost section
On 2018-11-25 21:49, Philippe Mathieu-Daudé wrote: > Signed-off-by: Philippe Mathieu-Daudé > --- > MAINTAINERS | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/MAINTAINERS b/MAINTAINERS > index e124792557..eab0fb9742 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -1359,6 +1359,7 @@ M: Michael S. Tsirkin > S: Supported > F: hw/*/*vhost* > F: docs/interop/vhost-user.txt > +F: contrib/vhost-user-*/ > > virtio > M: Michael S. Tsirkin > Reviewed-by: Thomas Huth
Re: [Qemu-devel] [PATCH v3 2/5] util: introduce threaded workqueue
On 11/26/18 6:56 PM, Dr. David Alan Gilbert wrote:
* Xiao Guangrong (guangrong.x...@gmail.com) wrote:
On 11/23/18 7:02 PM, Dr. David Alan Gilbert wrote:
+#include "qemu/osdep.h"
+#include "qemu/bitmap.h"
+#include "qemu/threaded-workqueue.h"
+
+#define SMP_CACHE_BYTES 64
That's architecture dependent isn't it?
Yes, it's arch dependent indeed.
I just used 64 for simplification and i think it is <= 64 on most CPU arch-es
so that can work.
Should i introduce statically defined CACHE LINE SIZE for all arch-es? :(
I think it depends why you need it; but we shouldn't have a constant
that is wrong, and we shouldn't define something architecture dependent
in here.
I see. I will address Emilio's suggestion that rename SMP_CACHE_BYTES to
THREAD_QUEUE_ALIGN and additional comments.
+ /*
+ * the bit in these two bitmaps indicates the index of the ＠requests
+ * respectively. If it's the same, the corresponding request is free
+ * and owned by the user, i.e, where the user fills a request. Otherwise,
+ * it is valid and owned by the thread, i.e, where the thread fetches
+ * the request and write the result.
+ */
+
+/* after the user fills the request, the bit is flipped. */
+uint64_t request_fill_bitmap QEMU_ALIGNED(SMP_CACHE_BYTES);
+/* after handles the request, the thread flips the bit. */
+uint64_t request_done_bitmap QEMU_ALIGNED(SMP_CACHE_BYTES);
Patchew complained about some type mismatches; I think those are because
you're using the bitmap_* functions on these; those functions always
operate on 'long' not on uint64_t - and on some platforms they're
unfortunately not the same.
I guess you were taking about this error:
ERROR: externs should be avoided in .c files
#233: FILE: util/threaded-workqueue.c:65:
+uint64_t request_fill_bitmap QEMU_ALIGNED(SMP_CACHE_BYTES);
The complained thing is "QEMU_ALIGNED(SMP_CACHE_BYTES)" as it gone
when the aligned thing is removed...
The issue you pointed out can be avoid by using type-casting, like:
bitmap_xor(..., (void *)>request_fill_bitmap)
cannot we?
I thought the error was just due to long vs uint64_t ratehr than the
qemu_aligned. I don't think it's just a casting problem, since I don't
think the long's are always 64bit.
Well, i made some adjustments that makes check_patch.sh really happy :),
as followings:
$ git diff util/
diff --git a/util/threaded-workqueue.c b/util/threaded-workqueue.c
index 2ab37cee8d..e34c65a8eb 100644
--- a/util/threaded-workqueue.c
+++ b/util/threaded-workqueue.c
@@ -62,21 +62,30 @@ struct ThreadLocal {
*/
/* after the user fills the request, the bit is flipped. */
-uint64_t request_fill_bitmap QEMU_ALIGNED(SMP_CACHE_BYTES);
+struct {
+uint64_t request_fill_bitmap;
+} QEMU_ALIGNED(SMP_CACHE_BYTES);
+
/* after handles the request, the thread flips the bit. */
-uint64_t request_done_bitmap QEMU_ALIGNED(SMP_CACHE_BYTES);
+struct {
+uint64_t request_done_bitmap;
+} QEMU_ALIGNED(SMP_CACHE_BYTES);
/*
* the event used to wake up the thread whenever a valid request has
* been submitted
*/
-QemuEvent request_valid_ev QEMU_ALIGNED(SMP_CACHE_BYTES);
+struct {
+QemuEvent request_valid_ev;
+} QEMU_ALIGNED(SMP_CACHE_BYTES);
/*
* the event is notified whenever a request has been completed
* (i.e, become free), which is used to wake up the user
*/
-QemuEvent request_free_ev QEMU_ALIGNED(SMP_CACHE_BYTES);
+struct {
+QemuEvent request_free_ev;
+} QEMU_ALIGNED(SMP_CACHE_BYTES);
};
typedef struct ThreadLocal ThreadLocal;
$ ./scripts/checkpatch.pl -f util/threaded-workqueue.c
total: 0 errors, 0 warnings, 472 lines checked
util/threaded-workqueue.c has no obvious style problems and is ready for
submission.
check_patch.sh somehow treats QEMU_ALIGNED as a function before the
modification.
And yes, u64 is not a long type on 32 bit arch, it's long[2] instead. that's
fine
when we pass the &(u64) to the function whose parameter is (long *). I thing
this
trick is widely used. e.g, the example in kvm that i replied to Emilio:
static inline bool kvm_test_request(int req, struct kvm_vcpu *vcpu)
{
return test_bit(req & KVM_REQUEST_MASK, (void *)>requests);
}
Re: [Qemu-devel] [PATCH for-3.1 13/25] MAINTAINERS: Add missing entries to VFIO
On 2018-11-25 21:49, Philippe Mathieu-Daudé wrote: > Signed-off-by: Philippe Mathieu-Daudé > --- > MAINTAINERS | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/MAINTAINERS b/MAINTAINERS > index bfe71f2555..e124792557 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -1326,6 +1326,8 @@ M: Alex Williamson > S: Supported > F: hw/vfio/* > F: include/hw/vfio/ > +F: include/qemu/vfio-helpers.h > +F: docs/igd-assign.txt Sounds reasonable to me. Reviewed-by: Thomas Huth
Re: [Qemu-devel] [RFC v9 00/17] VIRTIO-IOMMU device
Hi Eric, > -Original Message- > From: Eric Auger > Sent: Thursday, November 22, 2018 10:45 PM > To: eric.auger@gmail.com; eric.au...@redhat.com; qemu- > de...@nongnu.org; qemu-...@nongnu.org; peter.mayd...@linaro.org; > m...@redhat.com; jean-philippe.bruc...@arm.com > Cc: kevin.t...@intel.com; t...@semihalf.com; Bharat Bhushan > ; pet...@redhat.com > Subject: [RFC v9 00/17] VIRTIO-IOMMU device > > This series rebases the virtio-iommu device on qemu 3.1.0-rc2 and > implements the v0.8(.1) virtio-iommu spec [1]. The pci proxy for the virtio- > iommu device is now available and needs to be instantiated from the > command line using "-device virtio-iommu-pci". > The iommu machvirt option is not used anymore to instantiate the virtio- > iommu. > > At the moment the virtio-iommu-device only is functional in the ARM virt > machine. Indeed, besides its instantiation, links between the PCIe end points > and the IOMMU must be described. This is achieved by DT or ACPI > description (IORT). This description currently only is done in ARM virt. > > Best Regards > > Eric > > This series can be found at: > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit > hub.com%2Feauger%2Fqemu%2Ftree%2Fv3.1.0-rc2-virtio-iommu- > v0.9data=02%7C01%7Cbharat.bhushan%40nxp.com%7C031e61e2a2f3 > 4e3b945a08d6509e2918%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0% > 7C636785037552135579sdata=v99FRL1dQC%2BPTofwwqqoKGNiiqJ%2F > lGBX3KJB0IUKbQU%3Dreserved=0 > > References: > [1] [PATCH v3 0/7] Add virtio-iommu driver > > [2] guest branch featuring the virtio-iommu driver v0.8.1 + ACPI > integration not yet officially released by Jean. > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit > hub.com%2Feauger%2Flinux%2Ftree%2Fvirtio-iommu- > v0.8.1data=02%7C01%7Cbharat.bhushan%40nxp.com%7C031e61e2a2f > 34e3b945a08d6509e2918%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0 > %7C636785037552135579sdata=EjHA9BZ6rNX36YFKkNocBQtoz3uf3bdn > 5T9NSJ6SaRg%3Dreserved=0 > > Testing: > - tested with guest using virtio-net-pci > (,vhost=off,iommu_platform,disable-modern=off,disable-legacy=on) > and virtio-blk-pci > - VFIO/VHOST integration is not part of this series > - When using the virtio-blk-pci, some EDK2 FW versions feature > unmapped transactions and in that case the guest fails to boot. I have tested this series with virtio and VFIO both Tested-by: Bharat Bhushan Thanks -Bharat > > History: > > v8 -> v9: > - virtio-iommu-pci device needs to be instantiated from the command > line (RID is not imposed anymore). > - tail structure properly initialized > > v7 -> v8: > - virtio-iommu-pci added > - virt instantiation modified > - DT and ACPI modified to exclude the iommu RID from the mapping > - VIRTIO_IOMMU_F_BYPASS, VIRTIO_F_VERSION_1 features exposed > > v6 -> v7: > - rebase on qemu 3.0.0-rc3 > - minor update against v0.7 > - fix issue with EP not on pci.0 and ACPI probing > - change the instantiation method > > v5 -> v6: > - minor update against v0.6 spec > - fix g_hash_table_lookup in virtio_iommu_find_add_as > - replace some error_reports by qemu_log_mask(LOG_GUEST_ERROR, ...) > > v4 -> v5: > - event queue and fault reporting > - we now return the IOAPIC MSI region if the virtio-iommu is instantiated > in a PC machine. > - we bypass transactions on MSI HW region and fault on reserved ones. > - We support ACPI boot with mach-virt (based on IORT proposal) > - We moved to the new driver naming conventions > - simplified mach-virt instantiation > - worked around the disappearing of pci_find_primary_bus > - in virtio_iommu_translate, check the dev->as is not NULL > - initialize as->device_list in virtio_iommu_get_as > - initialize bufstate.error to false in virtio_iommu_probe > > v3 -> v4: > - probe request support although no reserved region is returned at > the moment > - unmap semantics less strict, as specified in v0.4 > - device registration, attach/detach revisited > - split into smaller patches to ease review > - propose a way to inform the IOMMU mr about the page_size_mask > of underlying HW IOMMU, if any > - remove warning associated with the translation of the MSI doorbell > > v2 -> v3: > - rebase on top of 2.10-rc0 and especially > [PATCH qemu v9 0/2] memory/iommu: QOM'fy IOMMU MemoryRegion > - add mutex init > - fix as->mappings deletion using g_tree_ref/unref > - when a dev is attached whereas it is already attached to > another address space, first detach it > - fix some error values > - page_sizes = TARGET_PAGE_MASK; > - I haven't changed the unmap() semantics yet, waiting for the > next virtio-iommu spec revision. > > v1 -> v2: > - fix redifinition of viommu_as typedef > > Eric Auger (17): > update-linux-headers: Import virtio_iommu.h > linux-headers: Partial update for virtio-iommu v0.8 > virtio-iommu: Add skeleton > virtio-iommu: Decode the command payload > virtio-iommu: Add the iommu regions > virtio-iommu: Endpoint and domains structs
Re: [Qemu-devel] [PATCH for-3.1 01/25] MAINTAINERS: Fix ACPI tests data files path
On 2018-11-25 21:49, Philippe Mathieu-Daudé wrote: > Missed while moving those files in 438c78dab75. > > Signed-off-by: Philippe Mathieu-Daudé > Reviewed-by: Igor Mammedov > --- > MAINTAINERS | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 1032406c56..6c4f25fb05 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -1216,8 +1216,7 @@ F: hw/i386/acpi-build.[hc] > F: hw/arm/virt-acpi-build.c > F: tests/bios-tables-test.c > F: tests/acpi-utils.[hc] > -F: tests/acpi-test-data/* > -F: tests/acpi-test-data/*/* > +F: tests/data/acpi/ > > ppc4xx > M: David Gibson > Reviewed-by: Thomas Huth
Re: [Qemu-devel] [RFC v9 15/17] hw/arm/virt: Add the virtio-iommu device tree mappings
> -Original Message-
> From: Eric Auger
> Sent: Thursday, November 22, 2018 10:46 PM
> To: eric.auger@gmail.com; eric.au...@redhat.com; qemu-
> de...@nongnu.org; qemu-...@nongnu.org; peter.mayd...@linaro.org;
> m...@redhat.com; jean-philippe.bruc...@arm.com
> Cc: kevin.t...@intel.com; t...@semihalf.com; Bharat Bhushan
> ; pet...@redhat.com
> Subject: [RFC v9 15/17] hw/arm/virt: Add the virtio-iommu device tree
> mappings
>
> Adds the "virtio,pci-iommu" node in the host bridge node and the RID
> mapping, excluding the IOMMU RID.
>
> Signed-off-by: Eric Auger
Reviewed-by: Bharat Bhushan
>
> ---
>
> v8 -> v9:
> - disable msi-bypass property
> - addition of the subnode is handled is the hotplug handler
> and IOMMU RID is notimposed anymore
>
> v6 -> v7:
> - align to the smmu instantiation code
>
> v4 -> v5:
> - VirtMachineClass no_iommu added in this patch
> - Use object_resolve_path_type
> ---
> hw/arm/virt.c | 57 +--
> include/hw/arm/virt.h | 2 ++
> 2 files changed, 52 insertions(+), 7 deletions(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c index a2b8d8f7c2..b2bbb0ef49
> 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -29,6 +29,7 @@
> */
>
> #include "qemu/osdep.h"
> +#include "monitor/qdev.h"
> #include "qapi/error.h"
> #include "hw/sysbus.h"
> #include "hw/arm/arm.h"
> @@ -49,6 +50,7 @@
> #include "qemu/bitops.h"
> #include "qemu/error-report.h"
> #include "hw/pci-host/gpex.h"
> +#include "hw/virtio/virtio-pci.h"
> #include "hw/arm/sysbus-fdt.h"
> #include "hw/platform-bus.h"
> #include "hw/arm/fdt.h"
> @@ -59,6 +61,7 @@
> #include "qapi/visitor.h"
> #include "standard-headers/linux/input.h"
> #include "hw/arm/smmuv3.h"
> +#include "hw/virtio/virtio-iommu.h"
>
> #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
> static void virt_##major##_##minor##_class_init(ObjectClass *oc, \ @@ -
> 1085,6 +1088,33 @@ static void create_smmu(const VirtMachineState *vms,
> qemu_irq *pic,
> g_free(node);
> }
>
> +static void create_virtio_iommu(VirtMachineState *vms, Error **errp) {
> +const char compat[] = "virtio,pci-iommu";
> +uint16_t bdf = vms->virtio_iommu_bdf;
> +char *node;
> +
> +vms->iommu_phandle = qemu_fdt_alloc_phandle(vms->fdt);
> +
> +node = g_strdup_printf("%s/virtio_iommu@%d", vms-
> >pciehb_nodename, bdf);
> +qemu_fdt_add_subnode(vms->fdt, node);
> +qemu_fdt_setprop(vms->fdt, node, "compatible", compat,
> sizeof(compat));
> +qemu_fdt_setprop_sized_cells(vms->fdt, node, "reg",
> + 1, bdf << 8 /* phys.hi */,
> + 1, 0/* phys.mid */,
> + 1, 0/* phys.lo */,
> + 1, 0/* size.hi */,
> + 1, 0/* size.low */);
> +
> +qemu_fdt_setprop_cell(vms->fdt, node, "#iommu-cells", 1);
> +qemu_fdt_setprop_cell(vms->fdt, node, "phandle", vms-
> >iommu_phandle);
> +g_free(node);
> +
> +qemu_fdt_setprop_cells(vms->fdt, vms->pciehb_nodename, "iommu-
> map",
> + 0x0, vms->iommu_phandle, 0x0, bdf,
> + bdf + 1, vms->iommu_phandle, bdf + 1, 0x
> +- bdf); }
> +
> static void create_pcie(VirtMachineState *vms, qemu_irq *pic) {
> hwaddr base_mmio = vms->memmap[VIRT_PCIE_MMIO].base; @@ -
> 1162,7 +1192,7 @@ static void create_pcie(VirtMachineState *vms,
> qemu_irq *pic)
> }
> }
>
> -nodename = g_strdup_printf("/pcie@%" PRIx64, base);
> +nodename = vms->pciehb_nodename = g_strdup_printf("/pcie@%"
> PRIx64,
> + base);
> qemu_fdt_add_subnode(vms->fdt, nodename);
> qemu_fdt_setprop_string(vms->fdt, nodename,
> "compatible", "pci-host-ecam-generic"); @@
> -1205,13
> +1235,17 @@ static void create_pcie(VirtMachineState *vms, qemu_irq *pic)
> if (vms->iommu) {
> vms->iommu_phandle = qemu_fdt_alloc_phandle(vms->fdt);
>
> -create_smmu(vms, pic, pci->bus);
> +switch (vms->iommu) {
> +case VIRT_IOMMU_SMMUV3:
> +create_smmu(vms, pic, pci->bus);
> +qemu_fdt_setprop_cells(vms->fdt, nodename, "iommu-map",
> + 0x0, vms->iommu_phandle, 0x0, 0x1);
> +break;
> +default:
> +g_assert_not_reached();
> +}
>
> -qemu_fdt_setprop_cells(vms->fdt, nodename, "iommu-map",
> - 0x0, vms->iommu_phandle, 0x0, 0x1);
> }
> -
> -g_free(nodename);
> }
>
> static void create_platform_bus(VirtMachineState *vms, qemu_irq *pic)
> @@ -1736,12 +1770,21 @@ static void
> virt_machine_device_plug_cb(HotplugHandler *hotplug_dev,
> SYS_BUS_DEVICE(dev));
> }
> }
> +if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) {
> +PCIDevice
Re: [Qemu-devel] [PATCH 2/2] hw: vmmouse: drop DEFINE_PROP_PTR()
Li Qiang writes:
> Use link property instead.
>
> Signed-off-by: Li Qiang
> ---
> hw/i386/pc.c | 2 +-
> hw/i386/vmmouse.c | 17 +++--
> 2 files changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 5d3fd86b83..9b343b4fd1 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1549,7 +1549,7 @@ 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",
> NULL);
NULL is correct if this can fail, but the failure is to be silently
ignored. Seems unlikely. If it isn't expected to fail, use
_abort. If failure should immediately terminate QEMU, use
_fatal.
> 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 5d2d278be4..612675d651 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
> @@ -272,10 +273,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)
> {
> @@ -284,8 +290,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;
> }
>
> @@ -293,6 +297,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,
> };
Looks good to me otherwise.
Re: [Qemu-devel] [PATCH 1/2] hw: pc: use TYPE_XXX instead of constant strings
Li Qiang writes:
> Signed-off-by: Li Qiang
> ---
> hw/i386/pc.c | 9 +++--
> hw/sparc64/sun4u.c | 2 +-
> include/hw/i386/pc.h | 7 +++
> 3 files changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index f095725dba..5d3fd86b83 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -502,9 +502,6 @@ void pc_cmos_init(PCMachineState *pcms,
> qemu_register_reset(pc_cmos_init_late, );
> }
>
> -#define TYPE_PORT92 "port92"
> -#define PORT92(obj) OBJECT_CHECK(Port92State, (obj), TYPE_PORT92)
> -
> /* port 92 stuff: could be split off */
> typedef struct Port92State {
> ISADevice parent_obj;
> @@ -1543,10 +1540,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 +1552,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/sparc64/sun4u.c b/hw/sparc64/sun4u.c
> index f76b19e4e9..c49a416b95 100644
> --- a/hw/sparc64/sun4u.c
> +++ b/hw/sparc64/sun4u.c
> @@ -305,7 +305,7 @@ static void ebus_realize(PCIDevice *pci_dev, Error **errp)
> parallel_hds_isa_init(s->isa_bus, MAX_PARALLEL_PORTS);
>
> /* Keyboard */
> -isa_create_simple(s->isa_bus, "i8042");
> +isa_create_simple(s->isa_bus, TYPE_I8042);
>
> /* Floppy */
> for (i = 0; i < MAX_FD; i++) {
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 136fe497b6..29db770d86 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -169,6 +169,13 @@ void gsi_handler(void *opaque, int n, int level);
> #define TYPE_VMPORT "vmport"
> typedef uint32_t (VMPortReadFunc)(void *opaque, uint32_t address);
>
> +#define TYPE_PORT92 "port92"
> +#define PORT92(obj) OBJECT_CHECK(Port92State, (obj), TYPE_PORT92)
Why move these to the header? They're used only in hw/i386/pc.c.
> +
> +/* vmmouse.c */
> +#define TYPE_VMMOUSE "vmmouse"
> +#define VMMOUSE(obj) OBJECT_CHECK(VMMouseState, (obj), TYPE_VMMOUSE)
> +
Likewise, why define these in the header?
TYPE_VMMOUSE is used just once, in hw/i386/pc.c. VMMOUSE() isn't used
at all. I'm okay with adding macros anyway, for consistency.
> static inline void vmport_init(ISABus *bus)
> {
> isa_create_simple(bus, TYPE_VMPORT);
[Qemu-devel] [PATCH RFC v5 4/5] virtio-iommu: add virtio-iommu replay
For virtio-iommu, on replay first unmap any previous
iommu-mapping and then map in iommu as per guest iommu
mappings.
Also if virtual iommu do have it own replay then
memory_region_iommu_replay() calls "imrc->translate()",
While virtio-iommu translate() expects device to be
registered before it is called. So having replay of
virtio-iommu helps to take no action if device not yet
probed/attached.
Signed-off-by: Bharat Bhushan
---
v4->v5:
- Rebase to v9 version from Eric (no change)
hw/virtio/trace-events | 1 +
hw/virtio/virtio-iommu.c | 38 ++
2 files changed, 39 insertions(+)
diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index 420b1e471b..f29a027258 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -74,3 +74,4 @@ virtio_iommu_fill_none_property(uint32_t devid) "devid=%d"
virtio_iommu_report_fault(uint8_t reason, uint32_t flags, uint32_t endpoint,
uint64_t addr) "FAULT reason=%d flags=%d endpoint=%d address =0x%"PRIx64
virtio_iommu_notify_map(const char *name, uint64_t iova, uint64_t paddr,
uint64_t map_size) "mr=%s iova=0x%"PRIx64" pa=0x%" PRIx64" size=0x%"PRIx64""
virtio_iommu_notify_unmap(const char *name, uint64_t iova, uint64_t map_size)
"mr=%s iova=0x%"PRIx64" size=0x%"PRIx64""
+virtio_iommu_remap(uint64_t iova, uint64_t pa, uint64_t size)
"iova=0x%"PRIx64" pa=0x%" PRIx64" size=0x%"PRIx64""
diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
index 7e8149e719..c9d8b3aa4c 100644
--- a/hw/virtio/virtio-iommu.c
+++ b/hw/virtio/virtio-iommu.c
@@ -1015,6 +1015,43 @@ static gint int_cmp(gconstpointer a, gconstpointer b,
gpointer user_data)
return (ua > ub) - (ua < ub);
}
+static gboolean virtio_iommu_remap(gpointer key, gpointer value, gpointer data)
+{
+viommu_mapping *mapping = (viommu_mapping *) value;
+IOMMUMemoryRegion *mr = (IOMMUMemoryRegion *) data;
+
+trace_virtio_iommu_remap(mapping->virt_addr, mapping->phys_addr,
+ mapping->size);
+/* unmap previous entry and map again */
+virtio_iommu_notify_unmap(mr, mapping->virt_addr, mapping->size);
+
+virtio_iommu_notify_map(mr, mapping->virt_addr, mapping->phys_addr,
+mapping->size);
+return false;
+}
+
+static void virtio_iommu_replay(IOMMUMemoryRegion *mr, IOMMUNotifier *n)
+{
+IOMMUDevice *sdev = container_of(mr, IOMMUDevice, iommu_mr);
+VirtIOIOMMU *s = sdev->viommu;
+uint32_t sid;
+viommu_endpoint *ep;
+
+sid = virtio_iommu_get_sid(sdev);
+
+qemu_mutex_lock(>mutex);
+
+ep = g_tree_lookup(s->endpoints, GUINT_TO_POINTER(sid));
+if (!ep || !ep->domain) {
+goto unlock;
+}
+
+g_tree_foreach(ep->domain->mappings, virtio_iommu_remap, mr);
+
+unlock:
+qemu_mutex_unlock(>mutex);
+}
+
static void virtio_iommu_device_realize(DeviceState *dev, Error **errp)
{
VirtIODevice *vdev = VIRTIO_DEVICE(dev);
@@ -1129,6 +1166,7 @@ static void
virtio_iommu_memory_region_class_init(ObjectClass *klass,
IOMMUMemoryRegionClass *imrc = IOMMU_MEMORY_REGION_CLASS(klass);
imrc->translate = virtio_iommu_translate;
+imrc->replay = virtio_iommu_replay;
}
static const TypeInfo virtio_iommu_info = {
--
2.19.1
[Qemu-devel] [PATCH RFC v5 5/5] virtio-iommu: handle IOMMU Notifier flag changes
Finally handle the IOMMU Notifier flag changes
for the iommu-memory region.
Signed-off-by: Bharat Bhushan
---
v4->v5:
- Rebase to v9 version from Eric (no change)
hw/virtio/trace-events | 2 ++
hw/virtio/virtio-iommu.c | 31 +++
2 files changed, 33 insertions(+)
diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index f29a027258..8c1d77b0c2 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -75,3 +75,5 @@ virtio_iommu_report_fault(uint8_t reason, uint32_t flags,
uint32_t endpoint, uin
virtio_iommu_notify_map(const char *name, uint64_t iova, uint64_t paddr,
uint64_t map_size) "mr=%s iova=0x%"PRIx64" pa=0x%" PRIx64" size=0x%"PRIx64""
virtio_iommu_notify_unmap(const char *name, uint64_t iova, uint64_t map_size)
"mr=%s iova=0x%"PRIx64" size=0x%"PRIx64""
virtio_iommu_remap(uint64_t iova, uint64_t pa, uint64_t size)
"iova=0x%"PRIx64" pa=0x%" PRIx64" size=0x%"PRIx64""
+virtio_iommu_notify_flag_add(const char *iommu) "Add virtio-iommu notifier
node for memory region %s"
+virtio_iommu_notify_flag_del(const char *iommu) "Del virtio-iommu notifier
node for memory region %s"
diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
index c9d8b3aa4c..adc37ddf1b 100644
--- a/hw/virtio/virtio-iommu.c
+++ b/hw/virtio/virtio-iommu.c
@@ -1052,6 +1052,36 @@ unlock:
qemu_mutex_unlock(>mutex);
}
+static void virtio_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu_mr,
+ IOMMUNotifierFlag old,
+ IOMMUNotifierFlag new)
+{
+IOMMUDevice *sdev = container_of(iommu_mr, IOMMUDevice, iommu_mr);
+VirtIOIOMMU *s = sdev->viommu;
+VirtioIOMMUNotifierNode *node = NULL;
+VirtioIOMMUNotifierNode *next_node = NULL;
+
+if (old == IOMMU_NOTIFIER_NONE) {
+trace_virtio_iommu_notify_flag_add(iommu_mr->parent_obj.name);
+node = g_malloc0(sizeof(*node));
+node->iommu_dev = sdev;
+QLIST_INSERT_HEAD(>notifiers_list, node, next);
+return;
+}
+
+/* update notifier node with new flags */
+QLIST_FOREACH_SAFE(node, >notifiers_list, next, next_node) {
+if (node->iommu_dev == sdev) {
+if (new == IOMMU_NOTIFIER_NONE) {
+trace_virtio_iommu_notify_flag_del(iommu_mr->parent_obj.name);
+QLIST_REMOVE(node, next);
+g_free(node);
+}
+return;
+}
+}
+}
+
static void virtio_iommu_device_realize(DeviceState *dev, Error **errp)
{
VirtIODevice *vdev = VIRTIO_DEVICE(dev);
@@ -1167,6 +1197,7 @@ static void
virtio_iommu_memory_region_class_init(ObjectClass *klass,
imrc->translate = virtio_iommu_translate;
imrc->replay = virtio_iommu_replay;
+imrc->notify_flag_changed = virtio_iommu_notify_flag_changed;
}
static const TypeInfo virtio_iommu_info = {
--
2.19.1
Re: [Qemu-devel] [PATCH v3] qapi: add query-display-options command
Hi, > If it's not too much trouble, please tweak the commit message to be a > bit more explicit. Perhaps: > > Add query-display-options command, which allows querying the qemu > display configuration. This isn't particularly useful, except it > exposes QAPI type DisplayOptions in query-qmp-schema, so that > libvirt can discover recently added -display parameter rendernode > (commit d4dc4ab133b). Works around lack of sufficiently powerful > command line introspection. Done, pull req with this and other 3.1 fixes sent. > This should give me a fighting chance to remember deprecating the > command once we got sufficiently powerful command line introspection. I'm wondering how difficuilt it would be to add that when limiting that to the command line switches which already use qapi parsers (-blockdev and -display as far I know). Might increase the motivation of others to help moving parsers from whatever they do today (QemuOpts, ...) to qapi to get introspection support ;) cheers, Gerd
[Qemu-devel] [PATCH RFC v5 3/5] virtio-iommu: Call iommu notifier on attach/detach
This patch extend the ATTACH/DETACH command handling to
call iommu-notifier to map/unmap the memory region in IOMMU
using vfio. This replay existing address space mappings on
attach command and remove existing address space mappings
on detach command.
Signed-off-by: Bharat Bhushan
Signed-off-by: Eric Auger
---
v4->v5:
- Rebase to v9 version from Eric
- PCIe device hotplug fix
hw/virtio/virtio-iommu.c | 47
1 file changed, 47 insertions(+)
diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
index 613a77521d..7e8149e719 100644
--- a/hw/virtio/virtio-iommu.c
+++ b/hw/virtio/virtio-iommu.c
@@ -131,8 +131,44 @@ static void virtio_iommu_notify_unmap(IOMMUMemoryRegion
*mr, hwaddr iova,
memory_region_notify_iommu(mr, 0, entry);
}
+static gboolean virtio_iommu_mapping_unmap(gpointer key, gpointer value,
+ gpointer data)
+{
+viommu_mapping *mapping = (viommu_mapping *) value;
+IOMMUMemoryRegion *mr = (IOMMUMemoryRegion *) data;
+
+virtio_iommu_notify_unmap(mr, mapping->virt_addr, mapping->size);
+
+return false;
+}
+
+static gboolean virtio_iommu_mapping_map(gpointer key, gpointer value,
+ gpointer data)
+{
+viommu_mapping *mapping = (viommu_mapping *) value;
+IOMMUMemoryRegion *mr = (IOMMUMemoryRegion *) data;
+
+virtio_iommu_notify_map(mr, mapping->virt_addr, mapping->phys_addr,
+mapping->size);
+
+return false;
+}
+
static void virtio_iommu_detach_endpoint_from_domain(viommu_endpoint *ep)
{
+VirtioIOMMUNotifierNode *node;
+VirtIOIOMMU *s = ep->viommu;
+viommu_domain *domain = ep->domain;
+uint32_t sid;
+
+QLIST_FOREACH(node, >notifiers_list, next) {
+sid = virtio_iommu_get_sid(node->iommu_dev);
+if (ep->id == sid) {
+g_tree_foreach(domain->mappings, virtio_iommu_mapping_unmap,
+ >iommu_dev->iommu_mr);
+}
+}
+
QLIST_REMOVE(ep, next);
ep->domain = NULL;
}
@@ -280,8 +316,10 @@ static int virtio_iommu_attach(VirtIOIOMMU *s,
{
uint32_t domain_id = le32_to_cpu(req->domain);
uint32_t ep_id = le32_to_cpu(req->endpoint);
+VirtioIOMMUNotifierNode *node;
viommu_domain *domain;
viommu_endpoint *ep;
+uint32_t sid;
trace_virtio_iommu_attach(domain_id, ep_id);
@@ -300,6 +338,15 @@ static int virtio_iommu_attach(VirtIOIOMMU *s,
ep->domain = domain;
g_tree_ref(domain->mappings);
+/* replay existing address space mappings on the associated mr */
+QLIST_FOREACH(node, >notifiers_list, next) {
+sid = virtio_iommu_get_sid(node->iommu_dev);
+if (ep->id == sid) {
+g_tree_foreach(domain->mappings, virtio_iommu_mapping_map,
+ >iommu_dev->iommu_mr);
+}
+}
+
return VIRTIO_IOMMU_S_OK;
}
--
2.19.1
[Qemu-devel] [PULL 6/6] qapi: add query-display-options command
Add query-display-options command, which allows querying the qemu
display configuration. This isn't particularly useful, except it
exposes QAPI type DisplayOptions in query-qmp-schema, so that libvirt
can discover recently added -display parameter rendernode (commit
d4dc4ab133b). Works around lack of sufficiently powerful command line
introspection.
Signed-off-by: Gerd Hoffmann
Reviewed-by: Eric Blake
Tested-by: Eric Blake
Tested-by: Erik Skultety
Message-id: 20181122071613.2889-1-kra...@redhat.com
[ kraxel: reworded commit message as suggested by armbru ]
---
vl.c | 6 ++
qapi/ui.json | 13 +
2 files changed, 19 insertions(+)
diff --git a/vl.c b/vl.c
index fa25d1ae2d..d6fd95c227 100644
--- a/vl.c
+++ b/vl.c
@@ -128,6 +128,7 @@ int main(int argc, char **argv)
#include "qapi/qapi-commands-block-core.h"
#include "qapi/qapi-commands-misc.h"
#include "qapi/qapi-commands-run-state.h"
+#include "qapi/qapi-commands-ui.h"
#include "qapi/qmp/qerror.h"
#include "sysemu/iothread.h"
@@ -2055,6 +2056,11 @@ static void parse_display_qapi(const char *optarg)
visit_free(v);
}
+DisplayOptions *qmp_query_display_options(Error **errp)
+{
+return QAPI_CLONE(DisplayOptions, );
+}
+
static void parse_display(const char *p)
{
const char *opts;
diff --git a/qapi/ui.json b/qapi/ui.json
index e248d3..fd39acb5c3 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -1102,3 +1102,16 @@
'discriminator' : 'type',
'data': { 'gtk': 'DisplayGTK',
'egl-headless' : 'DisplayEGLHeadless'} }
+
+##
+# @query-display-options:
+#
+# Returns information about display configuration
+#
+# Returns: @DisplayOptions
+#
+# Since: 3.1
+#
+##
+{ 'command': 'query-display-options',
+ 'returns': 'DisplayOptions' }
--
2.9.3
[Qemu-devel] [PATCH RFC v5 0/5] virtio-iommu: VFIO integration
This patch series integrates VFIO with virtio-iommu. This is tested with assigning 2 pci devices to Virtual Machine. This version is mainly about rebasing on v9 version on virtio-iommu device framework from Eric Augur. This patch series allows PCI pass-through using virtio-iommu. This series is based on: - virtio-iommu kernel driver by Jean-Philippe Brucker [PATCH v5 0/7] Add virtio-iommu driver git://linux-arm.org/kvmtool-jpb.git virtio-iommu/v0.9 - virtio-iommu device emulation by Eric Augur. [RFC,v9,00/17] VIRTIO-IOMMU device https://github.com/eauger/qemu/tree/v3.1.0-rc2-virtio-iommu-v0.9 v4->v5: - Rebase to v9 version from Eric - PCIe device hotplug fix - Added Patch 1/5 from Eric previous series (Eric somehow dropped in last version. - Patch "Translate the MSI doorbell in kvm_arch_fixup_msi_route" already integrated with vsmmu3 v3->v4: - Rebase to v4 version from Eric - Fixes from Eric with DPDK in VM - Logical division in multiple patches v2->v3: - This series is based on "[RFC v3 0/8] VIRTIO-IOMMU device" Which is based on top of v2.10-rc0 that - Fixed issue with two PCI devices - Addressed review comments v1->v2: - Added trace events - removed vSMMU3 link in patch description Bharat Bhushan (4): virtio-iommu: Add iommu notifier for iommu-map/unmap virtio-iommu: Call iommu notifier on attach/detach virtio-iommu: add virtio-iommu replay virtio-iommu: handle IOMMU Notifier flag changes Eric Auger (1): hw/vfio/common: Do not print error when viommu translates into an mmio region hw/vfio/common.c | 2 - hw/virtio/trace-events | 5 + hw/virtio/virtio-iommu.c | 190 ++- include/hw/virtio/virtio-iommu.h | 6 + 4 files changed, 198 insertions(+), 5 deletions(-) -- 2.19.1
[Qemu-devel] [PATCH RFC v5 2/5] virtio-iommu: Add iommu notifier for iommu-map/unmap
This patch extends VIRTIO_IOMMU_T_MAP/UNMAP request
handling to notify registered iommu-notifier. These
iommu-notifier maps the requested region in IOMMU using vfio.
Signed-off-by: Bharat Bhushan
---
v4->v5:
- Rebase to v9 version from Eric
- PCIe device hotplug fix
hw/virtio/trace-events | 2 +
hw/virtio/virtio-iommu.c | 74 ++--
include/hw/virtio/virtio-iommu.h | 6 +++
3 files changed, 79 insertions(+), 3 deletions(-)
diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index 053a07b3fc..420b1e471b 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -72,3 +72,5 @@ virtio_iommu_translate_out(uint64_t virt_addr, uint64_t
phys_addr, uint32_t sid)
virtio_iommu_fill_resv_property(uint32_t devid, uint8_t subtype, uint64_t
start, uint64_t end, uint32_t flags, size_t filled) "dev= %d, subtype=%d
start=0x%"PRIx64" end=0x%"PRIx64" flags=%d filled=0x%lx"
virtio_iommu_fill_none_property(uint32_t devid) "devid=%d"
virtio_iommu_report_fault(uint8_t reason, uint32_t flags, uint32_t endpoint,
uint64_t addr) "FAULT reason=%d flags=%d endpoint=%d address =0x%"PRIx64
+virtio_iommu_notify_map(const char *name, uint64_t iova, uint64_t paddr,
uint64_t map_size) "mr=%s iova=0x%"PRIx64" pa=0x%" PRIx64" size=0x%"PRIx64""
+virtio_iommu_notify_unmap(const char *name, uint64_t iova, uint64_t map_size)
"mr=%s iova=0x%"PRIx64" size=0x%"PRIx64""
diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
index 2ec01f3b9e..613a77521d 100644
--- a/hw/virtio/virtio-iommu.c
+++ b/hw/virtio/virtio-iommu.c
@@ -99,6 +99,38 @@ static gint interval_cmp(gconstpointer a, gconstpointer b,
gpointer user_data)
}
}
+static void virtio_iommu_notify_map(IOMMUMemoryRegion *mr, hwaddr iova,
+hwaddr paddr, hwaddr size)
+{
+IOMMUTLBEntry entry;
+
+trace_virtio_iommu_notify_map(mr->parent_obj.name, iova, paddr, size);
+
+entry.target_as = _space_memory;
+entry.addr_mask = size - 1;
+entry.iova = iova;
+entry.perm = IOMMU_RW;
+entry.translated_addr = paddr;
+
+memory_region_notify_iommu(mr, 0, entry);
+}
+
+static void virtio_iommu_notify_unmap(IOMMUMemoryRegion *mr, hwaddr iova,
+ hwaddr size)
+{
+IOMMUTLBEntry entry;
+
+trace_virtio_iommu_notify_unmap(mr->parent_obj.name, iova, size);
+
+entry.target_as = _space_memory;
+entry.addr_mask = size - 1;
+entry.iova = iova;
+entry.perm = IOMMU_NONE;
+entry.translated_addr = 0;
+
+memory_region_notify_iommu(mr, 0, entry);
+}
+
static void virtio_iommu_detach_endpoint_from_domain(viommu_endpoint *ep)
{
QLIST_REMOVE(ep, next);
@@ -301,9 +333,12 @@ static int virtio_iommu_map(VirtIOIOMMU *s,
uint64_t virt_start = le64_to_cpu(req->virt_start);
uint64_t virt_end = le64_to_cpu(req->virt_end);
uint32_t flags = le32_to_cpu(req->flags);
+VirtioIOMMUNotifierNode *node;
+viommu_endpoint *ep;
viommu_domain *domain;
viommu_interval *interval;
viommu_mapping *mapping;
+uint32_t sid;
interval = g_malloc0(sizeof(*interval));
@@ -331,9 +366,40 @@ static int virtio_iommu_map(VirtIOIOMMU *s,
g_tree_insert(domain->mappings, interval, mapping);
+/* All devices in an address-space share mapping */
+QLIST_FOREACH(node, >notifiers_list, next) {
+QLIST_FOREACH(ep, >endpoint_list, next) {
+sid = virtio_iommu_get_sid(node->iommu_dev);
+if (ep->id == sid) {
+virtio_iommu_notify_map(>iommu_dev->iommu_mr,
+virt_start, phys_start, mapping->size);
+}
+}
+}
+
return VIRTIO_IOMMU_S_OK;
}
+static void virtio_iommu_remove_mapping(VirtIOIOMMU *s, viommu_domain *domain,
+viommu_interval *interval)
+{
+VirtioIOMMUNotifierNode *node;
+viommu_endpoint *ep;
+uint32_t sid;
+
+g_tree_remove(domain->mappings, (gpointer)(interval));
+QLIST_FOREACH(node, >notifiers_list, next) {
+QLIST_FOREACH(ep, >endpoint_list, next) {
+sid = virtio_iommu_get_sid(node->iommu_dev);
+if (ep->id == sid) {
+virtio_iommu_notify_unmap(>iommu_dev->iommu_mr,
+ interval->low,
+ interval->high - interval->low + 1);
+}
+}
+}
+}
+
static int virtio_iommu_unmap(VirtIOIOMMU *s,
struct virtio_iommu_req_unmap *req)
{
@@ -366,18 +432,18 @@ static int virtio_iommu_unmap(VirtIOIOMMU *s,
current.high = high;
if (low == interval.low && size >= mapping->size) {
-g_tree_remove(domain->mappings, (gpointer)());
+virtio_iommu_remove_mapping(s, domain, );
interval.low = high + 1;
trace_virtio_iommu_unmap_left_interval(current.low, current.high,
[Qemu-devel] [PATCH RFC v5 1/5] hw/vfio/common: Do not print error when viommu translates into an mmio region
From: Eric Auger
On ARM, the MSI doorbell is translated by the virtual IOMMU.
As such address_space_translate() returns the MSI controller
MMIO region and we get an "iommu map to non memory area"
message. Let's remove this latter.
Signed-off-by: Eric Auger
Signed-off-by: Bharat Bhushan
---
v5:
- Added thi patch from Eric previous series (Eric somehow dropped in
last version and this is needed for VFIO.
hw/vfio/common.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 7c185e5a2e..fc40543121 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -328,8 +328,6 @@ static bool vfio_get_vaddr(IOMMUTLBEntry *iotlb, void
**vaddr,
, , writable,
MEMTXATTRS_UNSPECIFIED);
if (!memory_region_is_ram(mr)) {
-error_report("iommu map to non memory area %"HWADDR_PRIx"",
- xlat);
return false;
}
--
2.19.1
[Qemu-devel] [PULL 1/6] fmops: fix off-by-one in AR_TABLE and DR_TABLE array size
Cc: P J P
Reported-by: Wangjunqing
Reviewed-by: Philippe Mathieu-Daudé
Signed-off-by: Gerd Hoffmann
Message-id: 20181030082340.17170-1-kra...@redhat.com
Suggested-by: Paolo Bonzini
Signed-off-by: Gerd Hoffmann
---
hw/audio/fmopl.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/audio/fmopl.h b/hw/audio/fmopl.h
index e7e578a48e..e008e72d7a 100644
--- a/hw/audio/fmopl.h
+++ b/hw/audio/fmopl.h
@@ -72,8 +72,8 @@ typedef struct fm_opl_f {
/* Rhythm sention */
uint8_t rhythm; /* Rhythm mode , key flag */
/* time tables */
- int32_t AR_TABLE[75]; /* atttack rate tables */
- int32_t DR_TABLE[75]; /* decay rate tables */
+ int32_t AR_TABLE[76]; /* attack rate tables */
+ int32_t DR_TABLE[76]; /* decay rate tables */
uint32_t FN_TABLE[1024]; /* fnumber -> increment counter */
/* LFO */
int32_t *ams_table;
--
2.9.3
[Qemu-devel] [PULL 4/6] audio/hda: fix guest triggerable assert
Guest writes to a readonly register trigger the assert in
intel_hda_reg_write(). Add a check and just ignore them.
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1628433
Signed-off-by: Gerd Hoffmann
Reviewed-by: Dr. David Alan Gilbert
Reviewed-by: Philippe Mathieu-Daudé
Message-id: 20181123063957.9515-1-kra...@redhat.com
---
hw/audio/intel-hda.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
index 23a2cf6484..33e333cc26 100644
--- a/hw/audio/intel-hda.c
+++ b/hw/audio/intel-hda.c
@@ -23,6 +23,7 @@
#include "hw/pci/msi.h"
#include "qemu/timer.h"
#include "qemu/bitops.h"
+#include "qemu/log.h"
#include "hw/audio/soundhw.h"
#include "intel-hda.h"
#include "intel-hda-defs.h"
@@ -929,6 +930,11 @@ static void intel_hda_reg_write(IntelHDAState *d, const
IntelHDAReg *reg, uint32
if (!reg) {
return;
}
+if (!reg->wmask) {
+qemu_log_mask(LOG_GUEST_ERROR, "intel-hda: write to r/o reg %s\n",
+ reg->name);
+return;
+}
if (d->debug) {
time_t now = time(NULL);
--
2.9.3
[Qemu-devel] [PULL 5/6] usb-host: set ifs.detached as true if kernel driver is not active
From: linzhecheng
If no kernel driver is active, we can already claim and perform I/O on
it without detaching it.
Signed-off-by: linzhecheng
Message-id: 20181120083419.17716-1-linzhech...@huawei.com
Signed-off-by: Gerd Hoffmann
---
hw/usb/host-libusb.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
index f31e9cbbb8..b6602ded4e 100644
--- a/hw/usb/host-libusb.c
+++ b/hw/usb/host-libusb.c
@@ -1120,6 +1120,9 @@ static void usb_host_detach_kernel(USBHostDevice *s)
rc = libusb_kernel_driver_active(s->dh, i);
usb_host_libusb_error("libusb_kernel_driver_active", rc);
if (rc != 1) {
+if (rc == 0) {
+s->ifs[i].detached = true;
+}
continue;
}
trace_usb_host_detach_kernel(s->bus_num, s->addr, i);
--
2.9.3
[Qemu-devel] [PULL 0/6] Fixes 31 20181127 patches
The following changes since commit 5298f4d67a911dd9cefa4c4185eed242074d64c2: Merge remote-tracking branch 'remotes/kevin/tags/for-upstream' into staging (2018-11-23 08:54:52 +) are available in the git repository at: git://git.kraxel.org/qemu tags/fixes-31-20181127-pull-request for you to fetch changes up to e1ca8f7e1915496148f6e0ce1f7c2309af013312: qapi: add query-display-options command (2018-11-27 07:47:57 +0100) various bugfixes for 3.1: fmops, ps2, cirrus, hda, usb-host, qapi Gerd Hoffmann (3): fmops: fix off-by-one in AR_TABLE and DR_TABLE array size audio/hda: fix guest triggerable assert qapi: add query-display-options command Hervé Poussineau (1): ps2kbd: default to scan enabled after reset Wang Xin (1): cirrus_vga/migration: update the bank offset before use linzhecheng (1): usb-host: set ifs.detached as true if kernel driver is not active hw/audio/fmopl.h| 4 ++-- hw/audio/intel-hda.c| 6 ++ hw/display/cirrus_vga.c | 7 --- hw/input/ps2.c | 2 +- hw/usb/host-libusb.c| 3 +++ vl.c| 6 ++ qapi/ui.json| 13 + 7 files changed, 35 insertions(+), 6 deletions(-) -- 2.9.3
[Qemu-devel] [PULL 2/6] ps2kbd: default to scan enabled after reset
From: Hervé Poussineau A check for scan_enabled has been added to ps2_keyboard_event in commit 143c04c7e0639e53086519592ead15d2556bfbf2 to prevent stream corruption. This works well as long as operating system is resetting keyboard, or enabling it. This fixes IBM 40p firmware, which doesn't bother sending KBD_CMD_RESET, KBD_CMD_ENABLE or KBD_CMD_RESET_ENABLE before trying to use the keyboard. Signed-off-by: Hervé Poussineau Reviewed-by: Philippe Mathieu-Daudé Message-id: 20181021190721.2148-1-hpous...@reactos.org Signed-off-by: Gerd Hoffmann --- hw/input/ps2.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/input/ps2.c b/hw/input/ps2.c index 6c43fc2912..eb33ee9b6f 100644 --- a/hw/input/ps2.c +++ b/hw/input/ps2.c @@ -942,7 +942,7 @@ static void ps2_kbd_reset(void *opaque) trace_ps2_kbd_reset(opaque); ps2_common_reset(>common); -s->scan_enabled = 0; +s->scan_enabled = 1; s->translate = 0; s->scancode_set = 2; s->modifiers = 0; -- 2.9.3
[Qemu-devel] [PULL 3/6] cirrus_vga/migration: update the bank offset before use
From: Wang Xin The cirrus bank0/1 offset should be updated before we update the vram's alias offset. Signed-off-by: Wang Xin Message-id: 20181123064646.23036-1-linzhech...@huawei.com Signed-off-by: Gerd Hoffmann --- hw/display/cirrus_vga.c | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c index d9b854d74d..a0e71469f4 100644 --- a/hw/display/cirrus_vga.c +++ b/hw/display/cirrus_vga.c @@ -2746,11 +2746,12 @@ static int cirrus_post_load(void *opaque, int version_id) s->vga.gr[0x00] = s->cirrus_shadow_gr0 & 0x0f; s->vga.gr[0x01] = s->cirrus_shadow_gr1 & 0x0f; -cirrus_update_memory_access(s); -/* force refresh */ -s->vga.graphic_mode = -1; cirrus_update_bank_ptr(s, 0); cirrus_update_bank_ptr(s, 1); +cirrus_update_memory_access(s); +/* force refresh */ +s->vga.graphic_mode = -1; + return 0; } -- 2.9.3
Re: [Qemu-devel] [PATCH v9 4/8] migration: API to clear bits of guest free pages from the dirty bitmap
On 11/27/2018 02:06 PM, Peter Xu wrote:
On Thu, Nov 15, 2018 at 06:08:00PM +0800, Wei Wang wrote:
Again, is it possible to resize during migration?
So I think the check is fine, but uncertain about the comment.
Yes, resize would not happen with the current implementation.
But heard it could just be a temporal implementation. Probably
we could improve the comment like this:
"
Though the implementation might not support ram resize currently,
this could happen in theory with future updates. So the check here
handles the case that RAMBLOCK is resized after the free page hint is
reported.
"
And shall we print something if that happened? We can use
error_report_once(), and squashing the above assert:
if (!block || offset > block->used_length) {
/* should never happen, but if it happens we ignore the hints and warn */
error_report_once("...");
return;
}
What do you think?
Sounds good.
+
+if (len <= block->used_length - offset) {
+used_len = len;
+} else {
+used_len = block->used_length - offset;
+addr += used_len;
Maybe moving this line into the for() could be a bit better?
for (; len > 0; len -= used_len, addr += used_len) {
Yes, I think it looks better, thanks.
Best,
Wei
Re: [Qemu-devel] [PATCH] hw: fw_cfg: Improve error message when can't load splash file
Li Qiang writes:
> read_splashfile() reports "failed to read splash file" without
> further details. Get the details from g_file_get_contents(), and
> include them in the error message. Also remove unnecessary 'res'
> variable.
>
> Signed-off-by: Li Qiang
> ---
> hw/nvram/fw_cfg.c | 7 +++
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 946f765..3fcfa35 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -68,15 +68,14 @@ static char *read_splashfile(char *filename, gsize
> *file_sizep,
> int *file_typep)
> {
> GError *err = NULL;
> -gboolean res;
> gchar *content;
> int file_type;
> unsigned int filehead;
> int bmp_bpp;
>
> -res = g_file_get_contents(filename, , file_sizep, );
> -if (res == FALSE) {
> -error_report("failed to read splash file '%s'", filename);
> +if (!g_file_get_contents(filename, , file_sizep, )) {
> +error_report("failed to read splash file '%s': %s",
> + filename, err->message);
> g_error_free(err);
> return NULL;
> }
Reviewed-by: Markus Armbruster
Philippe, I assume you'll take this.
Re: [Qemu-devel] [PATCH] target: hax: fix errors in comment
Li Qiang writes: > Cc: qemu-triv...@nongnu.org > > Signed-off-by: Li Qiang > --- > target/i386/hax-all.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/target/i386/hax-all.c b/target/i386/hax-all.c > index 502ce6f0af..70213ebcf5 100644 > --- a/target/i386/hax-all.c > +++ b/target/i386/hax-all.c > @@ -205,7 +205,7 @@ int hax_vcpu_destroy(CPUState *cpu) > } > > /* > - * 1. The hax_tunnel is also destroied when vcpu destroy > + * 1. The hax_tunnel is also destroyed when vcpu is destroy when vcpu is destroyed > * 2. close fd will cause hax module vcpu be cleaned > */ > hax_close_fd(vcpu->fd);
Re: [Qemu-devel] [BUG] qemu stuck when detach host-usb device
> -Original Message- > From: Gerd Hoffmann [mailto:kra...@redhat.com] > Sent: Tuesday, November 27, 2018 2:09 PM > To: linzhecheng > Cc: qemu-devel@nongnu.org; wangxin (U) ; > Zhoujian (jay) ; libusb-de...@lists.sourceforge.net > Subject: Re: [Qemu-devel] [BUG] qemu stuck when detach host-usb device > > On Tue, Nov 27, 2018 at 01:26:24AM +, linzhecheng wrote: > > Description of problem: > > The guest has a host-usb device(Kingston Technology DataTraveler 100 > > G3/G4/SE9 G2), which is attached to xhci controller(on host). Qemu will > > stuck > if I detach it from guest. > > > > How reproducible: > > 100% > > > > Steps to Reproduce: > > 1.Use usb stick to copy files in guest , make it busy working. > > 2.virsh detach-device vm_name usb.xml > > > > Then qemu will stuck for 20s, I found this is because > > libusb_release_interface > block for 20s. > > Dmesg prints: > > > > [35442.034861] usb 4-2.1: Disable of device-initiated U1 failed. > > [35447.034993] usb 4-2.1: Disable of device-initiated U2 failed. > > [35452.035131] usb 4-2.1: Set SEL for device-initiated U1 failed. > > [35457.035259] usb 4-2.1: Set SEL for device-initiated U2 failed. > > > > Is this a hardware error or software's bug? > > I'd guess software error, could be is libusb or (host) linux kernel. > Cc'ing libusb-devel. Perhaps it's usb driver's bug. Could you also reproduce it? > > cheers, > Gerd
Re: [Qemu-devel] [BUG] qemu stuck when detach host-usb device
On Tue, Nov 27, 2018 at 01:26:24AM +, linzhecheng wrote: > Description of problem: > The guest has a host-usb device(Kingston Technology DataTraveler 100 > G3/G4/SE9 G2), which is attached > to xhci controller(on host). Qemu will stuck if I detach it from guest. > > How reproducible: > 100% > > Steps to Reproduce: > 1.Use usb stick to copy files in guest , make it busy working. > 2.virsh detach-device vm_name usb.xml > > Then qemu will stuck for 20s, I found this is because > libusb_release_interface block for 20s. > Dmesg prints: > > [35442.034861] usb 4-2.1: Disable of device-initiated U1 failed. > [35447.034993] usb 4-2.1: Disable of device-initiated U2 failed. > [35452.035131] usb 4-2.1: Set SEL for device-initiated U1 failed. > [35457.035259] usb 4-2.1: Set SEL for device-initiated U2 failed. > > Is this a hardware error or software's bug? I'd guess software error, could be is libusb or (host) linux kernel. Cc'ing libusb-devel. cheers, Gerd
Re: [Qemu-devel] [virtio-dev] Re: [PATCH v9 3/8] migration: use bitmap_mutex in migration_bitmap_clear_dirty
On 11/27/2018 02:02 PM, Wei Wang wrote:
On 11/27/2018 01:40 PM, Peter Xu wrote:
On Thu, Nov 15, 2018 at 06:07:59PM +0800, Wei Wang wrote:
The bitmap mutex is used to synchronize threads to update the dirty
bitmap and the migration_dirty_pages counter. For example, the free
page optimization clears bits of free pages from the bitmap in an
iothread context. This patch makes migration_bitmap_clear_dirty update
the bitmap and counter under the mutex.
Signed-off-by: Wei Wang
CC: Dr. David Alan Gilbert
CC: Juan Quintela
CC: Michael S. Tsirkin
CC: Peter Xu
---
migration/ram.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/migration/ram.c b/migration/ram.c
index 7e7deec..ef69dbe 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1556,11 +1556,14 @@ static inline bool
migration_bitmap_clear_dirty(RAMState *rs,
{
bool ret;
+qemu_mutex_lock(>bitmap_mutex);
ret = test_and_clear_bit(page, rb->bmap);
if (ret) {
rs->migration_dirty_pages--;
}
+qemu_mutex_unlock(>bitmap_mutex);
+
return ret;
}
It seems fine to me, but have you thought about
test_and_clear_bit_atomic()? Note that we just had
test_and_set_bit_atomic() a few months ago.
Thanks for sharing. I think we might also need to
mutex migration_dirty_pages.
And not related to this patch: I'm unclear on why we have had
bitmap_mutex before, since it seems unnecessary.
OK. This is because with the optimization we have a thread
which clears bits (of free pages) from the bitmap and updates
migration_dirty_pages. So we need to synchronization between
the migration thread and the optimization thread.
And before this feature, I think yes, that bitmap_mutex is not needed.
It was left there due to some historical reasons.
I remember Dave previous said he was about to remove it. But the new
feature will need it again.
Best,
Wei
Re: [Qemu-devel] [PATCH v9 4/8] migration: API to clear bits of guest free pages from the dirty bitmap
On Thu, Nov 15, 2018 at 06:08:00PM +0800, Wei Wang wrote:
> This patch adds an API to clear bits corresponding to guest free pages
> from the dirty bitmap. Spilt the free page block if it crosses the QEMU
> RAMBlock boundary.
>
> Signed-off-by: Wei Wang
> CC: Dr. David Alan Gilbert
> CC: Juan Quintela
> CC: Michael S. Tsirkin
> CC: Peter Xu
> ---
> include/migration/misc.h | 2 ++
> migration/ram.c | 48
>
> 2 files changed, 50 insertions(+)
>
> diff --git a/include/migration/misc.h b/include/migration/misc.h
> index 4ebf24c..113320e 100644
> --- a/include/migration/misc.h
> +++ b/include/migration/misc.h
> @@ -14,11 +14,13 @@
> #ifndef MIGRATION_MISC_H
> #define MIGRATION_MISC_H
>
> +#include "exec/cpu-common.h"
> #include "qemu/notify.h"
>
> /* migration/ram.c */
>
> void ram_mig_init(void);
> +void qemu_guest_free_page_hint(void *addr, size_t len);
>
> /* migration/block.c */
>
> diff --git a/migration/ram.c b/migration/ram.c
> index ef69dbe..229b791 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -3131,6 +3131,54 @@ static void ram_state_resume_prepare(RAMState *rs,
> QEMUFile *out)
> }
>
> /*
> + * This function clears bits of the free pages reported by the caller from
> the
> + * migration dirty bitmap. @addr is the host address corresponding to the
> + * start of the continuous guest free pages, and @len is the total bytes of
> + * those pages.
> + */
> +void qemu_guest_free_page_hint(void *addr, size_t len)
> +{
> +RAMBlock *block;
> +ram_addr_t offset;
> +size_t used_len, start, npages;
> +MigrationState *s = migrate_get_current();
> +
> +/* This function is currently expected to be used during live migration
> */
> +if (!migration_is_setup_or_active(s->state)) {
> +return;
> +}
> +
> +for (; len > 0; len -= used_len) {
> +block = qemu_ram_block_from_host(addr, false, );
> +assert(block);
> +
> +/*
> + * This handles the case that the RAMBlock is resized after the free
> + * page hint is reported.
> + */
> +if (unlikely(offset > block->used_length)) {
> +return;
> +}
Again, is it possible to resize during migration?
So I think the check is fine, but uncertain about the comment.
And shall we print something if that happened? We can use
error_report_once(), and squashing the above assert:
if (!block || offset > block->used_length) {
/* should never happen, but if it happens we ignore the hints and warn */
error_report_once("...");
return;
}
What do you think?
> +
> +if (len <= block->used_length - offset) {
> +used_len = len;
> +} else {
> +used_len = block->used_length - offset;
> +addr += used_len;
Maybe moving this line into the for() could be a bit better?
for (; len > 0; len -= used_len, addr += used_len) {
> +}
> +
> +start = offset >> TARGET_PAGE_BITS;
> +npages = used_len >> TARGET_PAGE_BITS;
> +
> +qemu_mutex_lock(_state->bitmap_mutex);
> +ram_state->migration_dirty_pages -=
> + bitmap_count_one_with_offset(block->bmap, start,
> npages);
> +bitmap_clear(block->bmap, start, npages);
> +qemu_mutex_unlock(_state->bitmap_mutex);
> +}
> +}
> +
> +/*
> * Each of ram_save_setup, ram_save_iterate and ram_save_complete has
> * long-running RCU critical section. When rcu-reclaims in the code
> * start to become numerous it will be necessary to reduce the
> --
> 1.8.3.1
>
Regards,
--
Peter Xu
Re: [Qemu-devel] [PATCH v9 3/8] migration: use bitmap_mutex in migration_bitmap_clear_dirty
On 11/27/2018 01:40 PM, Peter Xu wrote:
On Thu, Nov 15, 2018 at 06:07:59PM +0800, Wei Wang wrote:
The bitmap mutex is used to synchronize threads to update the dirty
bitmap and the migration_dirty_pages counter. For example, the free
page optimization clears bits of free pages from the bitmap in an
iothread context. This patch makes migration_bitmap_clear_dirty update
the bitmap and counter under the mutex.
Signed-off-by: Wei Wang
CC: Dr. David Alan Gilbert
CC: Juan Quintela
CC: Michael S. Tsirkin
CC: Peter Xu
---
migration/ram.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/migration/ram.c b/migration/ram.c
index 7e7deec..ef69dbe 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1556,11 +1556,14 @@ static inline bool
migration_bitmap_clear_dirty(RAMState *rs,
{
bool ret;
+qemu_mutex_lock(>bitmap_mutex);
ret = test_and_clear_bit(page, rb->bmap);
if (ret) {
rs->migration_dirty_pages--;
}
+qemu_mutex_unlock(>bitmap_mutex);
+
return ret;
}
It seems fine to me, but have you thought about
test_and_clear_bit_atomic()? Note that we just had
test_and_set_bit_atomic() a few months ago.
Thanks for sharing. I think we might also need to
mutex migration_dirty_pages.
And not related to this patch: I'm unclear on why we have had
bitmap_mutex before, since it seems unnecessary.
OK. This is because with the optimization we have a thread
which clears bits (of free pages) from the bitmap and updates
migration_dirty_pages. So we need to synchronization between
the migration thread and the optimization thread.
Best,
Wei
Re: [Qemu-devel] [PATCH for-3.1] hw/arm/virt-acpi-build: Fix SMMUv3 ACPI integration
Hi Shannon,
On 11/26/18 4:46 PM, Eric Auger wrote:
> The AcpiIortSmmu3 misses 2 32b fields corresponding to the
> proximity domain and the device id mapping index.
I fail to understand how we currently track the evolutions of the IORT
structures:
Looking at the smmuv3 node in kernel include/acpi/actbl2.h
- the following fields were added in c944230064eb ACPICA: iasl: Update
to IORT SMMUv3 disassembling (1 year, 4 months ago)
u8 pxm;
u8 reserved1;
u16 reserved2;
- id_mapping_index was added in 4c106aa411ee ACPICA: iasl: Add SMMUv3
device ID mapping index support (1 year ago)
- current u32 pxm was introduced in d87be0438e3d ACPICA: IORT: Update
for revision D (6 months ago)
I would have expected each of those evolutions to be tagged by a
revision increment in the acpi_iort_node.revision field but this is not
done. Also I fail to see any enum for encoding the revision.
The smmuv3 struct that has been exposed until now corresponds to Rev C
https://lists.linuxfoundation.org/pipermail/iommu/2017-May/022000.html
(0c2021c047ba ACPICA: IORT: Update SMMU models for revision C (1 year,
4 months ago)
Also I noticed that in rev D, new fields were added in struct
acpi_iort_root_complex. We miss them at the moment in the IORT description.
How does the guest kernel know which revision of the IORT is exposed?
What do I miss?
Thanks
Eric
>
> Also let's report IO-coherent access is supported for
> translation table walks, descriptor fetches and queues by
> setting the COHACC override flag. Without that, we observe
> wrong command opcodes. The DT description also advertises
> the dma coherency.
>
> Fixes a703b4f6c1ee ("hw/arm/virt-acpi-build: Add smmuv3 node in IORT table")
>
> Signed-off-by: Eric Auger
> Reported-by: Shameerali Kolothum Thodi
> ---
> hw/arm/virt-acpi-build.c| 1 +
> include/hw/acpi/acpi-defs.h | 8
> 2 files changed, 9 insertions(+)
>
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 5785fb697c..aa177ba64d 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -448,6 +448,7 @@ build_iort(GArray *table_data, BIOSLinker *linker,
> VirtMachineState *vms)
> smmu->mapping_count = cpu_to_le32(1);
> smmu->mapping_offset = cpu_to_le32(sizeof(*smmu));
> smmu->base_address = cpu_to_le64(vms->memmap[VIRT_SMMU].base);
> +smmu->flags = ACPI_IORT_SMMU_V3_COHACC_OVERRIDE;
> smmu->event_gsiv = cpu_to_le32(irq);
> smmu->pri_gsiv = cpu_to_le32(irq + 1);
> smmu->gerr_gsiv = cpu_to_le32(irq + 2);
> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> index af8e023968..c3ee1f517b 100644
> --- a/include/hw/acpi/acpi-defs.h
> +++ b/include/hw/acpi/acpi-defs.h
> @@ -628,6 +628,12 @@ struct AcpiIortItsGroup {
> } QEMU_PACKED;
> typedef struct AcpiIortItsGroup AcpiIortItsGroup;
>
> +enum {
> +ACPI_IORT_SMMU_V3_COHACC_OVERRIDE = 1 << 0,
> +ACPI_IORT_SMMU_V3_HTTU_OVERRIDE = 3 << 1,
> +ACPI_IORT_SMMU_V3_PXM_VALID = 1 << 3
> +};
> +
> 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;
> +uint32_t id_mapping_index;
> AcpiIortIdMapping id_mapping_array[0];
> } QEMU_PACKED;
> typedef struct AcpiIortSmmu3 AcpiIortSmmu3;
>
Re: [Qemu-devel] [PATCH v9 3/8] migration: use bitmap_mutex in migration_bitmap_clear_dirty
On Thu, Nov 15, 2018 at 06:07:59PM +0800, Wei Wang wrote:
> The bitmap mutex is used to synchronize threads to update the dirty
> bitmap and the migration_dirty_pages counter. For example, the free
> page optimization clears bits of free pages from the bitmap in an
> iothread context. This patch makes migration_bitmap_clear_dirty update
> the bitmap and counter under the mutex.
>
> Signed-off-by: Wei Wang
> CC: Dr. David Alan Gilbert
> CC: Juan Quintela
> CC: Michael S. Tsirkin
> CC: Peter Xu
> ---
> migration/ram.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/migration/ram.c b/migration/ram.c
> index 7e7deec..ef69dbe 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1556,11 +1556,14 @@ static inline bool
> migration_bitmap_clear_dirty(RAMState *rs,
> {
> bool ret;
>
> +qemu_mutex_lock(>bitmap_mutex);
> ret = test_and_clear_bit(page, rb->bmap);
>
> if (ret) {
> rs->migration_dirty_pages--;
> }
> +qemu_mutex_unlock(>bitmap_mutex);
> +
> return ret;
> }
It seems fine to me, but have you thought about
test_and_clear_bit_atomic()? Note that we just had
test_and_set_bit_atomic() a few months ago.
And not related to this patch: I'm unclear on why we have had
bitmap_mutex before, since it seems unnecessary.
Regards,
--
Peter Xu
[Qemu-devel] [PATCH] target: hax: fix errors in comment
Cc: qemu-triv...@nongnu.org Signed-off-by: Li Qiang --- target/i386/hax-all.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/target/i386/hax-all.c b/target/i386/hax-all.c index 502ce6f0af..70213ebcf5 100644 --- a/target/i386/hax-all.c +++ b/target/i386/hax-all.c @@ -205,7 +205,7 @@ int hax_vcpu_destroy(CPUState *cpu) } /* - * 1. The hax_tunnel is also destroied when vcpu destroy + * 1. The hax_tunnel is also destroyed when vcpu is destroy * 2. close fd will cause hax module vcpu be cleaned */ hax_close_fd(vcpu->fd); -- 2.11.0
[Qemu-devel] [PATCH 1/1] input: improve performance of mouse event handle
Hi Gerd Hoffmann:
Qemu doesn't put mouse event into queue. It handle mouse event one by one
independently.
So, I think there is no need to use g_new0 to malloc memory dynamically.
Using local variable can improve event handle performance.
I have tested this patch in my platform. after applying this patch. Time of
handling 100,000
mouse move events changes from 50ms to 9ms.
Please review it and consider whether it can be applied!
Signed-off-by: FelixYao
---
include/ui/input.h | 7 ---
ui/input.c | 47 +--
2 files changed, 29 insertions(+), 25 deletions(-)
diff --git a/include/ui/input.h b/include/ui/input.h
index 34ebc67..bb5e782 100644
--- a/include/ui/input.h
+++ b/include/ui/input.h
@@ -49,7 +49,8 @@ int qemu_input_key_value_to_scancode(const KeyValue *value,
bool down,
int *codes);
int qemu_input_linux_to_qcode(unsigned int lnx);
-InputEvent *qemu_input_event_new_btn(InputButton btn, bool down);
+void qemu_input_event_set_btn(InputButton btn, bool down,
+InputEvent *evt, InputBtnEvent *button_evt);
void qemu_input_queue_btn(QemuConsole *src, InputButton btn, bool down);
void qemu_input_update_buttons(QemuConsole *src, uint32_t *button_map,
uint32_t button_old, uint32_t button_new);
@@ -58,8 +59,8 @@ bool qemu_input_is_absolute(void);
int qemu_input_scale_axis(int value,
int min_in, int max_in,
int min_out, int max_out);
-InputEvent *qemu_input_event_new_move(InputEventKind kind,
- InputAxis axis, int value);
+void qemu_input_event_set_move(InputEventKind kind, InputAxis axis, int value,
+ InputEvent *evt, InputMoveEvent *move);
void qemu_input_queue_rel(QemuConsole *src, InputAxis axis, int value);
void qemu_input_queue_abs(QemuConsole *src, InputAxis axis, int value,
int min_in, int max_in);
diff --git a/ui/input.c b/ui/input.c
index 7c9a410..70494b3 100644
--- a/ui/input.c
+++ b/ui/input.c
@@ -458,22 +458,24 @@ void qemu_input_event_send_key_delay(uint32_t delay_ms)
}
}
-InputEvent *qemu_input_event_new_btn(InputButton btn, bool down)
+void qemu_input_event_set_btn(InputButton btn, bool down,
+ InputEvent *evt, InputBtnEvent *button_evt)
{
-InputEvent *evt = g_new0(InputEvent, 1);
-evt->u.btn.data = g_new0(InputBtnEvent, 1);
+memset(evt, 0, sizeof(*evt));
+memset(button_evt, 0, sizeof(*button_evt));
+evt->u.btn.data = button_evt;
evt->type = INPUT_EVENT_KIND_BTN;
evt->u.btn.data->button = btn;
evt->u.btn.data->down = down;
-return evt;
}
void qemu_input_queue_btn(QemuConsole *src, InputButton btn, bool down)
{
-InputEvent *evt;
-evt = qemu_input_event_new_btn(btn, down);
-qemu_input_event_send(src, evt);
-qapi_free_InputEvent(evt);
+InputEvent event;
+InputBtnEvent button;
+
+qemu_input_event_set_btn(btn, down, , );
+qemu_input_event_send(src, );
}
void qemu_input_update_buttons(QemuConsole *src, uint32_t *button_map,
@@ -513,37 +515,38 @@ int qemu_input_scale_axis(int value,
return ((int64_t)value - min_in) * range_out / range_in + min_out;
}
-InputEvent *qemu_input_event_new_move(InputEventKind kind,
- InputAxis axis, int value)
-{
-InputEvent *evt = g_new0(InputEvent, 1);
-InputMoveEvent *move = g_new0(InputMoveEvent, 1);
+void qemu_input_event_set_move(InputEventKind kind, InputAxis axis, int value,
+ InputEvent *evt, InputMoveEvent *move)
+{
+memset(evt, 0, sizeof(*evt));
+memset(move, 0, sizeof(*move));
evt->type = kind;
evt->u.rel.data = move; /* evt->u.rel is the same as evt->u.abs */
move->axis = axis;
move->value = value;
-return evt;
}
void qemu_input_queue_rel(QemuConsole *src, InputAxis axis, int value)
{
-InputEvent *evt;
-evt = qemu_input_event_new_move(INPUT_EVENT_KIND_REL, axis, value);
-qemu_input_event_send(src, evt);
-qapi_free_InputEvent(evt);
+InputEvent event;
+InputMoveEvent move;
+
+qemu_input_event_set_move(INPUT_EVENT_KIND_REL, axis, value, ,
);
+qemu_input_event_send(src, );
}
void qemu_input_queue_abs(QemuConsole *src, InputAxis axis, int value,
int min_in, int max_in)
{
-InputEvent *evt;
+InputEvent event;
+InputMoveEvent move;
+
int scaled = qemu_input_scale_axis(value, min_in, max_in,
INPUT_EVENT_ABS_MIN,
INPUT_EVENT_ABS_MAX);
-evt = qemu_input_event_new_move(INPUT_EVENT_KIND_ABS, axis, scaled);
-qemu_input_event_send(src, evt);
-qapi_free_InputEvent(evt);
+qemu_input_event_set_move(INPUT_EVENT_KIND_ABS, axis, scaled, ,
);
+qemu_input_event_send(src, );
}
Re: [Qemu-devel] [PATCH v9 0/8] virtio-balloon: free page hint support
On 11/15/2018 06:07 PM, Wei Wang wrote: This is the deivce part implementation to add a new feature, VIRTIO_BALLOON_F_FREE_PAGE_HINT to the virtio-balloon device. The device receives the guest free page hints from the driver and clears the corresponding bits in the dirty bitmap, so that those free pages are not sent by the migration thread to the destination. *Tests 1 Test Environment Host: Intel(R) Xeon(R) CPU E5-2699 v4 @ 2.20GHz Migration setup: migrate_set_speed 100G, migrate_set_downtime 400ms 2 Test Results (results are averaged over several repeated runs) 2.1 Guest setup: 8G RAM, 4 vCPU 2.1.1 Idle guest live migration time Optimization v.s. Legacy = 620ms vs 2970ms --> ~79% reduction 2.1.2 Guest live migration with Linux compilation workload (i.e. make bzImage -j4) running 1) Live Migration Time: Optimization v.s. Legacy = 2273ms v.s. 4502ms --> ~50% reduction 2) Linux Compilation Time: Optimization v.s. Legacy = 8min42s v.s. 8min43s --> no obvious difference 2.2 Guest setup: 128G RAM, 4 vCPU 2.2.1 Idle guest live migration time Optimization v.s. Legacy = 5294ms vs 41651ms --> ~87% reduction 2.2.2 Guest live migration with Linux compilation workload 1) Live Migration Time: Optimization v.s. Legacy = 8816ms v.s. 54201ms --> 84% reduction 2) Linux Compilation Time: Optimization v.s. Legacy = 8min30s v.s. 8min36s --> no obvious difference ChangeLog: v8->v9: bitmap: - fix bitmap_count_one to handle the nbits=0 case migration: - replace the ram save notifier chain with a more general precopy notifier chain, which is similar to the postcopy notifier chain. - Avoid exposing the RAMState struct, and add a function, precopy_disable_bulk_stage, to let the virtio-balloon notifier callback to disable the bulk stage flag. Hi Dave and Peter, Could you continue to review the patches? Thanks! Best, Wei
Re: [Qemu-devel] [PATCH] hw: fw_cfg: Improve error message when can't load splash file
Hello Paolo, Philippe
Seems this patch has been lost...
I think Philippe can merge it for 4.0, right?
Also pls notice the following fw_cfg patchset.
-->https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg04097.html
Thanks,
Li Qiang
Li Qiang 于2018年11月1日周四 下午2:02写道：
> read_splashfile() reports "failed to read splash file" without
> further details. Get the details from g_file_get_contents(), and
> include them in the error message. Also remove unnecessary 'res'
> variable.
>
> Signed-off-by: Li Qiang
> ---
> hw/nvram/fw_cfg.c | 7 +++
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 946f765..3fcfa35 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -68,15 +68,14 @@ static char *read_splashfile(char *filename, gsize
> *file_sizep,
> int *file_typep)
> {
> GError *err = NULL;
> -gboolean res;
> gchar *content;
> int file_type;
> unsigned int filehead;
> int bmp_bpp;
>
> -res = g_file_get_contents(filename, , file_sizep, );
> -if (res == FALSE) {
> -error_report("failed to read splash file '%s'", filename);
> +if (!g_file_get_contents(filename, , file_sizep, )) {
> +error_report("failed to read splash file '%s': %s",
> + filename, err->message);
> g_error_free(err);
> return NULL;
> }
> --
> 1.8.3.1
>
>
[Qemu-devel] [PATCH for-4.0 v3 0/2] virtio: Provide version-specific variants of virtio PCI devices
Existing modern-only device types are not being touched by v3, as
they don't need separate variants. However, I plan to implement
separate cleanups in the code that calls virtio_pci_force_virtio_1(),
first, and then propose additional changes (e.g. deprecating
disable-legacy and disable-modern in those device types).
Changes v2 -> v3:
* Split into two separate patches (type registration helper
and introduction of new types)
* Rewrote virtio_pci_types_register() completely:
* Replaced magic generation of type names with explicit fields in
VirtioPCIDeviceTypeInfo
* Removed modern_only field (not necessary anymore)
* Don't register a separate base type unless necessary
Changes v1 -> v2:
* Removed *-0.9 devices. Nobody will want to use them, if
transitional devices work with legacy drivers
(Gerd Hoffmann, Michael S. Tsirkin)
* Drop virtio version from name: rename -1.0-transitional to
-transitional (Michael S. Tsirkin)
* Renamed -1.0 to -non-transitional
* Don't add any extra variants to modern-only device types
(they don't need it)
* Fix typo on TYPE_VIRTIO_INPUT_HOST_PCI (crash reported by
Cornelia Huck)
* No need to change cast macros for modern-only devices
* Rename virtio_register_types() to virtio_pci_types_register()
Original patch description:
Many of the current virtio-*-pci device types actually represent
3 different types of devices:
* virtio 1.0 non-transitional devices
* virtio 1.0 transitional devices
* virtio 0.9 ("legacy device" in virtio 1.0 terminology)
That would be just an annoyance if it didn't break our device/bus
compatibility QMP interfaces. With this multi-purpose device
type, there's no way to tell management software that
transitional devices and legacy devices require a Conventional
PCI bus.
The multi-purpose device types would also prevent us from telling
management software what's the PCI vendor/device ID for them,
because their PCI IDs change at runtime depending on the bus
where they were plugged.
This patch adds separate device types for each of those virtio
device flavors:
* virtio-*-pci: the existing multi-purpose device types
* virtio-*-pci-transitional: virtio-1.0 device supporting legacy drivers
* virtio-*-pci-non-transitional: modern-only
Reference to previous discussion that originated this idea:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg558389.html
Eduardo Habkost (2):
virtio: Helper for registering virtio device types
virtio: Provide version-specific variants of virtio PCI devices
hw/virtio/virtio-pci.h | 78 +++--
hw/display/virtio-gpu-pci.c| 7 +-
hw/display/virtio-vga.c| 7 +-
hw/virtio/virtio-crypto-pci.c | 7 +-
hw/virtio/virtio-pci.c | 267 ++---
tests/acceptance/virtio_version.py | 177 +++
6 files changed, 453 insertions(+), 90 deletions(-)
create mode 100644 tests/acceptance/virtio_version.py
--
2.18.0.rc1.1.g3f1ff2140
[Qemu-devel] [PATCH for-4.0 v3 2/2] virtio: Provide version-specific variants of virtio PCI devices
Many of the current virtio-*-pci device types actually represent
3 different types of devices:
* virtio 1.0 non-transitional devices
* virtio 1.0 transitional devices
* virtio 0.9 ("legacy device" in virtio 1.0 terminology)
That would be just an annoyance if it didn't break our device/bus
compatibility QMP interfaces. With these multi-purpose device
types, there's no way to tell management software that
transitional devices and legacy devices require a Conventional
PCI bus.
The multi-purpose device types would also prevent us from telling
management software what's the PCI vendor/device ID for them,
because their PCI IDs change at runtime depending on the bus
where they were plugged.
This patch adds separate device types for each of those virtio
device flavors:
- virtio-*-pci: the existing multi-purpose device types
- Configurable using `disable-legacy` and `disable-modern`
properties
- Legacy driver support is automatically enabled/disabled
depending on the bus where it is plugged
- Supports Conventional PCI and PCI Express buses
(but Conventional PCI is incompatible with
disable-legacy=off)
- Changes PCI vendor/device IDs at runtime
- virtio-*-pci-transitional: virtio-1.0 device supporting legacy drivers
- Supports Conventional PCI buses only, because
it has a PIO BAR
- virtio-*-pci-non-transitional: modern-only
- Supports both Conventional PCI and PCI Express buses
The existing TYPE_* macros for these types will point to an
abstract base type, so existing casts in the code will keep
working for all variants.
A simple test script (tests/acceptance/virtio_version.py) is
included, to check if the new device types are equivalent to
using the `disable-legacy` and `disable-modern` options.
Acked-by: Andrea Bolognani
Signed-off-by: Eduardo Habkost
---
Changes v2 -> v3:
* Split into two separate patches (type registration helper
and introduction of new types)
* Include full type names as literals in the code instead
of generating type names automatically
Changes v1 -> v2:
* Removed *-0.9 devices. Nobody will want to use them, if
transitional devices work with legacy drivers
(Gerd Hoffmann, Michael S. Tsirkin)
* Drop virtio version from name: rename -1.0-transitional to
-transitional (Michael S. Tsirkin)
* Renamed -1.0 to -non-transitional
* Don't add any extra variants to modern-only device types
(they don't need it)
* Fix typo on TYPE_VIRTIO_INPUT_HOST_PCI (crash reported by
Cornelia Huck)
* No need to change cast macros for modern-only devices
* Rename virtio_register_types() to virtio_pci_types_register()
---
hw/virtio/virtio-pci.h | 24 ++--
hw/virtio/virtio-pci.c | 60 --
tests/acceptance/virtio_version.py | 177 +
3 files changed, 237 insertions(+), 24 deletions(-)
create mode 100644 tests/acceptance/virtio_version.py
diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
index b26731a305..cd36f7e2fa 100644
--- a/hw/virtio/virtio-pci.h
+++ b/hw/virtio/virtio-pci.h
@@ -216,7 +216,7 @@ static inline void virtio_pci_disable_modern(VirtIOPCIProxy
*proxy)
/*
* virtio-scsi-pci: This extends VirtioPCIProxy.
*/
-#define TYPE_VIRTIO_SCSI_PCI "virtio-scsi-pci"
+#define TYPE_VIRTIO_SCSI_PCI "virtio-scsi-pci-base"
#define VIRTIO_SCSI_PCI(obj) \
OBJECT_CHECK(VirtIOSCSIPCI, (obj), TYPE_VIRTIO_SCSI_PCI)
@@ -229,7 +229,7 @@ struct VirtIOSCSIPCI {
/*
* vhost-scsi-pci: This extends VirtioPCIProxy.
*/
-#define TYPE_VHOST_SCSI_PCI "vhost-scsi-pci"
+#define TYPE_VHOST_SCSI_PCI "vhost-scsi-pci-base"
#define VHOST_SCSI_PCI(obj) \
OBJECT_CHECK(VHostSCSIPCI, (obj), TYPE_VHOST_SCSI_PCI)
@@ -239,7 +239,7 @@ struct VHostSCSIPCI {
};
#endif
-#define TYPE_VHOST_USER_SCSI_PCI "vhost-user-scsi-pci"
+#define TYPE_VHOST_USER_SCSI_PCI "vhost-user-scsi-pci-base"
#define VHOST_USER_SCSI_PCI(obj) \
OBJECT_CHECK(VHostUserSCSIPCI, (obj), TYPE_VHOST_USER_SCSI_PCI)
@@ -252,7 +252,7 @@ struct VHostUserSCSIPCI {
/*
* vhost-user-blk-pci: This extends VirtioPCIProxy.
*/
-#define TYPE_VHOST_USER_BLK_PCI "vhost-user-blk-pci"
+#define TYPE_VHOST_USER_BLK_PCI "vhost-user-blk-pci-base"
#define VHOST_USER_BLK_PCI(obj) \
OBJECT_CHECK(VHostUserBlkPCI, (obj), TYPE_VHOST_USER_BLK_PCI)
@@ -265,7 +265,7 @@ struct VHostUserBlkPCI {
/*
* virtio-blk-pci: This extends VirtioPCIProxy.
*/
-#define TYPE_VIRTIO_BLK_PCI "virtio-blk-pci"
+#define TYPE_VIRTIO_BLK_PCI "virtio-blk-pci-base"
#define VIRTIO_BLK_PCI(obj) \
OBJECT_CHECK(VirtIOBlkPCI, (obj), TYPE_VIRTIO_BLK_PCI)
@@ -277,7 +277,7 @@ struct VirtIOBlkPCI {
/*
* virtio-balloon-pci: This extends VirtioPCIProxy.
*/
-#define TYPE_VIRTIO_BALLOON_PCI "virtio-balloon-pci"
+#define TYPE_VIRTIO_BALLOON_PCI "virtio-balloon-pci-base"
#define VIRTIO_BALLOON_PCI(obj) \
OBJECT_CHECK(VirtIOBalloonPCI, (obj), TYPE_VIRTIO_BALLOON_PCI)
@@ -289,7 +289,7 @@ struct VirtIOBalloonPCI {
/*
*
[Qemu-devel] [PATCH for-4.0 v3 1/2] virtio: Helper for registering virtio device types
Introduce a helper for registering different flavours of virtio
devices. Convert code to use the helper, but keep only the
existing generic types. Transitional and non-transitional device
types will be added by another patch.
Acked-by: Andrea Bolognani
Signed-off-by: Eduardo Habkost
---
Changes v2 -> v3:
* Split into two separate patches (type registration helper
and introduction of new types)
* Rewrote comments describing each flavour
* Rewrote virtio_pci_types_register() completely:
* Replaced magic generation of type names with explicit fields in
VirtioPCIDeviceTypeInfo
* Removed modern_only field (won't be used anymore)
* Don't register a separate base type unless it's required by
the caller
---
hw/virtio/virtio-pci.h| 54
hw/display/virtio-gpu-pci.c | 7 +-
hw/display/virtio-vga.c | 7 +-
hw/virtio/virtio-crypto-pci.c | 7 +-
hw/virtio/virtio-pci.c| 231 --
5 files changed, 228 insertions(+), 78 deletions(-)
diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
index 813082b0d7..b26731a305 100644
--- a/hw/virtio/virtio-pci.h
+++ b/hw/virtio/virtio-pci.h
@@ -417,4 +417,58 @@ struct VirtIOCryptoPCI {
/* Virtio ABI version, if we increment this, we break the guest driver. */
#define VIRTIO_PCI_ABI_VERSION 0
+/* Input for virtio_pci_types_register() */
+typedef struct VirtioPCIDeviceTypeInfo {
+/*
+ * Common base class for the subclasses below.
+ *
+ * Required only if transitional_name or non_transitional_name is set.
+ *
+ * We need a separate base type instead of making all types
+ * inherit from generic_name for two reasons:
+ * 1) generic_name implements INTERFACE_PCIE_DEVICE, but
+ *transitional_name don't.
+ * 2) generic_name has the "disable-legacy" and "disable-modern"
+ *properties, transitional_name and non_transitional name don't.
+ */
+const char *base_name;
+/*
+ * Generic device type. Optional.
+ *
+ * Supports both transitional and non-transitional modes,
+ * using the disable-legacy and disable-modern properties.
+ * If disable-legacy=auto, (non-)transitional mode is selected
+ * depending on the bus where the device is plugged.
+ *
+ * Implements both INTERFACE_PCIE_DEVICE and
INTERFACE_CONVENTIONAL_PCI_DEVICE,
+ * but PCI Express is supported only in non-transitional mode.
+ *
+ * The only type implemented by QEMU 3.1 and older.
+ */
+const char *generic_name;
+/*
+ * The non-transitional device type. Optional.
+ *
+ * Implements both INTERFACE_PCIE_DEVICE and
INTERFACE_CONVENTIONAL_PCI_DEVICE.
+ */
+const char *transitional_name;
+/*
+ * The transitional device type. Optional.
+ *
+ * Implements INTERFACE_CONVENTIONAL_PCI_DEVICE only.
+ */
+const char *non_transitional_name;
+
+/* Parent type. If NULL, TYPE_VIRTIO_PCI is used */
+const char *parent;
+
+/* Same as TypeInfo fields: */
+size_t instance_size;
+void (*instance_init)(Object *obj);
+void (*class_init)(ObjectClass *klass, void *data);
+} VirtioPCIDeviceTypeInfo;
+
+/* Register virtio-pci type(s). @t must be static. */
+void virtio_pci_types_register(const VirtioPCIDeviceTypeInfo *t);
+
#endif
diff --git a/hw/display/virtio-gpu-pci.c b/hw/display/virtio-gpu-pci.c
index cece4aa495..faf76a8bc4 100644
--- a/hw/display/virtio-gpu-pci.c
+++ b/hw/display/virtio-gpu-pci.c
@@ -69,9 +69,8 @@ static void virtio_gpu_initfn(Object *obj)
TYPE_VIRTIO_GPU);
}
-static const TypeInfo virtio_gpu_pci_info = {
-.name = TYPE_VIRTIO_GPU_PCI,
-.parent = TYPE_VIRTIO_PCI,
+static const VirtioPCIDeviceTypeInfo virtio_gpu_pci_info = {
+.generic_name = TYPE_VIRTIO_GPU_PCI,
.instance_size = sizeof(VirtIOGPUPCI),
.instance_init = virtio_gpu_initfn,
.class_init = virtio_gpu_pci_class_init,
@@ -79,6 +78,6 @@ static const TypeInfo virtio_gpu_pci_info = {
static void virtio_gpu_pci_register_types(void)
{
-type_register_static(_gpu_pci_info);
+virtio_pci_types_register(_gpu_pci_info);
}
type_init(virtio_gpu_pci_register_types)
diff --git a/hw/display/virtio-vga.c b/hw/display/virtio-vga.c
index ab2e369b28..8db4d916f2 100644
--- a/hw/display/virtio-vga.c
+++ b/hw/display/virtio-vga.c
@@ -207,9 +207,8 @@ static void virtio_vga_inst_initfn(Object *obj)
TYPE_VIRTIO_GPU);
}
-static TypeInfo virtio_vga_info = {
-.name = TYPE_VIRTIO_VGA,
-.parent= TYPE_VIRTIO_PCI,
+static VirtioPCIDeviceTypeInfo virtio_vga_info = {
+.generic_name = TYPE_VIRTIO_VGA,
.instance_size = sizeof(struct VirtIOVGA),
.instance_init = virtio_vga_inst_initfn,
.class_init= virtio_vga_class_init,
@@ -217,7 +216,7 @@ static TypeInfo virtio_vga_info = {
static void virtio_vga_register_types(void)
{
-
Re: [Qemu-devel] [RFC 38/48] translator: implement 2-pass translation
On Mon, Nov 26, 2018 at 15:16:00 +, Alex Bennée wrote:
> Emilio G. Cota writes:
(snip)
> > +if (tb_trans_cb && first_pass) {
> > +qemu_plugin_tb_trans_cb(cpu, plugin_tb);
> > +first_pass = false;
> > +goto translate;
> > +}
>
> So the only reason we are doing this two pass tango is to ensure the
> plugin can insert TCG ops before the actual translation has occurred?
Not only. The idea is to provide plugins with well-defined TBs,
i.e. the instruction sizes and contents can be queried by the plugin
before the plugin decides how/where to instrument the TB.
Since in the targets we generate TCG code and also generate
host code in a single shot (TranslatorOps.translate_insn),
the 2-pass approach is a workaround to first get the
well-defined TB, and in the second pass inject the instrumentation
in the appropriate places.
This is a bit of a waste but given that it only happens at
translate time, it can have negligible performance impact --
I measured a 0.13% gmean slowdown for SPEC06int.
> I think we can do better, especially as the internal structures of
> TCGops are implemented as a list so ops and be inserted before and after
> other ops. This is currently only done by the optimiser at the moment,
> see:
>
> TCGOp *tcg_op_insert_before(TCGContext *s, TCGOp *op, TCGOpcode opc, int
> narg);
> TCGOp *tcg_op_insert_after(TCGContext *s, TCGOp *op, TCGOpcode opc, int
> narg);
>
> and all the base tcg ops end up going to tcg_emit_op which just appends
> to the tail. But if we can come up with a neater way to track the op
> used before the current translated expression we could do away with two
> phases translation completely.
This list of ops is generated via TranslatorOps.translate_insn.
Unfortunately, this function also defines the guest insns that form the TB.
Decoupling the two actions ("define the TB" and "translate to TCG ops")
would be ideal, but I didn't want to rewrite all the target translators
in QEMU, and opted instead for the 2-pass approach as a compromise.
Thanks,
Emilio
Re: [Qemu-devel] [PATCH v5 04/36] ppc/xive: introduce the XiveRouter model
On Fri, Nov 23, 2018 at 09:06:07AM +0100, Cédric Le Goater wrote: > On 11/23/18 4:50 AM, David Gibson wrote: > > On Thu, Nov 22, 2018 at 08:53:00AM +0100, Cédric Le Goater wrote: > >> On 11/22/18 5:11 AM, David Gibson wrote: > >>> On Fri, Nov 16, 2018 at 11:56:57AM +0100, Cédric Le Goater > wrote: [snip] > >>> So as far as I can see so far, the XiveFabric interface will > >>> essentially have to be implemented on the router object, so I'm not > >>> seeing much point to having the interface rather than just a direct > >>> call on the router object. But I haven't read the whole series yet, > >>> so maybe I'm missing something. > >> > >> The PSIHB and PHB4 models are using it but there are not in the series. > >> > >> I can send the PSIHB patch in the next version if you like, it's the > >> patch right after PnvXive. It's attached below for the moment. Look at > >> pnv_psi_notify(). > > > > Hrm, I see. This seems like a really convoluted way of achieving what > > you need here. We want to abstract exactly how the source delivers > > notifies, > > on sPAPR, I agree that the forwarding of event notification could be a > simple XiveRouter call but the XiveRouter covers both machines :/ > > On PowerNV, HW uses MMIOs to forward events and only the device knows > about the IRQ number offset in the global IRQ number space and the > notification port to use for the MMIO store. A PowerNV XIVE source > would forward the event notification to a piece of logic which sends > a PowerBUS event notification message. How it reaches the XIVE IC is > beyong QEMU as it would means modeling the PowerBUS. > > > but doing it with an interface on some object that's not necessarily > > either the source or the router seems odd. > There is no direct link between the device owing the source and the > XIVE controller, they could be on the same Power chip but the routing > could be done by some other chips. This scenario is covered btw. > > See it as a connector object. > > > At the very least the names need to change (of both interface and > > > property for the target object). > > I am fine with renaming it. With the above explanations, if they are > clear enough, how do see them ? TBH, I didn't find the info above particularly illuminating. However, I think perusing the code has finally gotten my head around the model (sorry it's taken so long). I think two things were confusing me. 1) The name had be thinking in terms of the XicsFabric, but the function here is totally different. 2) I was thinking of the XiveSource as handling all source-side irq related logic, but I guess it's real function is a bit more limited. As I now understand it, it's only really handling the ESB and immediately surrounding logic - the "owning" device (e.g. PHB or PSI) is responsible for the connection "up the stack" as it were. So, I'm ok with the model. Just to verify that my understanding is correct, can you confirm my reasoning below: * For PowerNV, we'd generally expect the notify function to be implemented by the "owning" device. For the XIVE internal source, that would be the XiveRouter itself, immediately triggering the right EAS. For the PHB and PSI irq sources, that will code in the PHB/PSI which performs the MMIO to poke a router. * For PAPR, for simplicity, we'd expect to wire all sources direct to a single system-wide router object. I definitely think it needs a name change though. "XiveNotify" perhaps? And the property to configure it on the XiveSource, maybe "notify" or "notify_via". -- 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] [RFC 23/48] translator: add plugin_insn argument to translate_insn
On Mon, Nov 26, 2018 at 11:30:25 -0800, Richard Henderson wrote: > On 11/26/18 11:07 AM, Emilio G. Cota wrote: > > The main reason why I added the qemu_plugin_insn_append calls > > was to avoid reading the instructions twice from guest memory, > > because I was worried that doing so might somehow alter the > > guest's execution, e.g. what if we read a cross-page instruction, > > and both pages mapped to the same TLB entry? We'd end up having > > more TLB misses because instrumentation was enabled. > > A better solution for this, I think is to change direct calls from > > cpu_ldl_code(env, pc); > to > translator_ldl_code(dc_base, env, pc); > > instead of passing around a new argument separate from DisasContextBase? I think this + diff'ing pc_next should work to figure out the contents and size of each instruction. I'll do it this way in v2. Thanks, Emilio
[Qemu-devel] [BUG] qemu stuck when detach host-usb device
Description of problem: The guest has a host-usb device(Kingston Technology DataTraveler 100 G3/G4/SE9 G2), which is attached to xhci controller(on host). Qemu will stuck if I detach it from guest. How reproducible: 100% Steps to Reproduce: 1.Use usb stick to copy files in guest , make it busy working. 2.virsh detach-device vm_name usb.xml Then qemu will stuck for 20s, I found this is because libusb_release_interface block for 20s. Dmesg prints: [35442.034861] usb 4-2.1: Disable of device-initiated U1 failed. [35447.034993] usb 4-2.1: Disable of device-initiated U2 failed. [35452.035131] usb 4-2.1: Set SEL for device-initiated U1 failed. [35457.035259] usb 4-2.1: Set SEL for device-initiated U2 failed. Is this a hardware error or software's bug?
Re: [Qemu-devel] [RFC 22/48] cpu: hook plugin vcpu events
On Mon, Nov 26, 2018 at 11:17:27 +, Alex Bennée wrote:
>
> Emilio G. Cota writes:
>
> > On Fri, Nov 23, 2018 at 17:10:53 +, Alex Bennée wrote:
> >> Emilio G. Cota writes:
> > (snip)
> >> > @@ -1322,12 +1323,21 @@ static void qemu_tcg_rr_wait_io_event(CPUState
> >> > *cpu)
> >> >
> >> > static void qemu_wait_io_event(CPUState *cpu)
> >> > {
> >> > +bool asleep = false;
> >> > +
> >>
> >> slept?
> >
> > Changed to slept, thanks.
> >
> >> > g_assert(cpu_mutex_locked(cpu));
> >> > g_assert(!qemu_mutex_iothread_locked());
> >> >
> >> > while (cpu_thread_is_idle(cpu)) {
> >> > +if (!asleep) {
> >> > +asleep = true;
> >> > +qemu_plugin_vcpu_idle_cb(cpu);
> >> > +}
> >> > qemu_cond_wait(>halt_cond, >lock);
> >> > }
> >> > +if (asleep) {
> >> > +qemu_plugin_vcpu_resume_cb(cpu);
> >> > +}
> >>
> >> I wonder if having two hooks is too much? What might a plugin want to do
> >> before we go into idle sleep?
> >>
> >> It feels like we are exposing too much of the guts of TCG to the plugin
> >> here as wait_io could be for any number of internal reasons other than
> >> the actual emulation blocking for IO through a WFI or something. If a
> >> plugin really wants to track such things shouldn't it be hooking to the
> >> guest sleep points?
> >>
> >> If idle sleeps really are that important maybe we could just report our
> >> sleep time on resume - so a single hook but passing a bit more
> >> information?
> >
> > I don't have all the use cases for this figured out. For now I'm using
> > this in plugins as a way to know when a vCPU is for sure idle, which
> > is used in memory reclamation schemes such as RCU.
>
> Couldn't we achieve the same by scheduling async or safe async work on
> the vCPU? Maybe we would expose this to the plugin as a "run callback
> when idle" function.
I'm not sure I understand the first question. Do you mean to schedule
regular work just to make the CPU idle every now and then?
Letting the CPU idle whenever it wants to is fine, so I don't see
the need to force those transitions.
BTW I just thought of a different use case for this. Imagine a plugin
is estimating how much power a CPU is consuming; if the CPU is idle,
the power model would bring down the voltage/freq and account for that.
Of course the model would also want to track the instructions being
executed, which is a nice segue to your point below.
> > What are the "guest sleep points" you mention? Are they
> > target-agnostic?
>
> I mean we can arrive here for a variety of reasons - not all of which
> are triggered by the guest going to sleep. But that isn't your current
> use case.
>
> They aren't target agnostic as not all guests try to fully model their
> "sleeping" instructions. Some just end up busy spinning until the event
> they should have been sleeping until happens.
I see, so you mean something like a "pause" instruction in the guest.
Plugins that needed such level of detail (like the imaginary power
model plugin I mentioned above) would have to track these
instructions as well. IOW, this would be orthogonal to the "idle"
callback as implemented in this series.
Emilio
Re: [Qemu-devel] [RFC 12/48] atomic_template: define pre/post macros
On Mon, Nov 26, 2018 at 11:21:13 +, Alex Bennée wrote: > > Emilio G. Cota writes: > > > On Thu, Nov 22, 2018 at 17:12:34 +, Alex Bennée wrote: > >> > >> Emilio G. Cota writes: > >> > >> > In preparation for plugin support. > >> > > >> > Signed-off-by: Emilio G. Cota > >> > >> More macros for the macro-god. I guess this works but I wonder if it's > >> possible to do a clean-up ala softfloat and the experimental softmmu > >> re-factor that makes this less a mess of macros? > > > > Heh, point taken. > > > > I'll try to fix this once (and if!) the series graduates from RFC > > to proper patchset. BTW I'm planning to pick rth's softfloat > > cleanup soon, so that should help. > > Yeah I was in the middle of re-spinning the softmmu demacro patches but > ran into a bug I didn't get time to track down. I'll get it re-spun once > I've finished going through this. Nice! > I assume the rth's softfloat cleanup is for hardfloat support? I stand > ready to review the next version. We should get it in well before the > 4.0 soft freeze (next release). Yes I meant hardfloat, not softfloat :P I submitted v6 over the weekend. You'll be happy to hear that after applying rth's cleanups, the series only has four macros left, the largest of which only takes 8 lines :-) Emilio
Re: [Qemu-devel] [PATCH v5 02/36] ppc/xive: add support for the LSI interrupt sources
On Mon, Nov 26, 2018 at 12:20:19PM +0100, Cédric Le Goater wrote:
> On 11/26/18 6:39 AM, David Gibson wrote:
> > On Fri, Nov 23, 2018 at 02:28:35PM +0100, Cédric Le Goater wrote:
> >>
> >> +/*
> >> + * Returns whether the event notification should be forwarded.
> >> + */
> >> +static bool xive_source_lsi_trigger(XiveSource *xsrc, uint32_t
> >> srcno)
> >
> > What exactly "trigger" means isn't entirely obvious for an LSI. Might
> > be clearer to have "lsi_assert" and "lsi_deassert" helpers instead.
>
> This is called only when the interrupt is asserted. So it is a
> simplified LSI trigger depending only on the 'P' bit.
> >>>
> >>> Yes, I see that. But the result is that while the MSI logic is
> >>> encapsulated in the MSI trigger function, this leaves the LSI logic
> >>> split across the trigger function and set_irq() itself. I think it
> >>> would be better to have assert and deassert helpers instead, which
> >>> handle both the trigger/notification and also the updating of the
> >>> ASSERTED bit.
> >>
> >> Something like the xive_source_set_irq_lsi() below ?
> >
> > Uh.. not exactly what I had in mind, but close enough.
> >
> > [snip]
> > +/*
> >> + * Returns whether the event notification should be forwarded.
> >> + */
> >> static bool xive_source_esb_trigger(XiveSource *xsrc, uint32_t srcno)
> >> {
> >> +bool notify;
> >> +
> >> assert(srcno < xsrc->nr_irqs);
> >>
> >> -return xive_esb_trigger(>status[srcno]);
> >> +notify = xive_esb_trigger(>status[srcno]);
> >> +
> >> +if (xive_source_irq_is_lsi(xsrc, srcno) &&
> >
> > Except that this block can go, since this function is no longer called
> > for LSIs.
>
> It still can be through the ESB MMIOs, if the guest does a load on the
> trigger page.
Oh, good point. That makes me rethink all my comments on this matter.
In that case I think your original code was fine, except that I'd
prefer to see the setting of the ASSERTED bit inside the trigger
function, instead of in the set_irq() caller.
>
> C.
>
>
> >
> >> +xive_source_esb_get(xsrc, srcno) == XIVE_ESB_QUEUED) {
> >> +qemu_log_mask(LOG_GUEST_ERROR,
> >> + "XIVE: queued an event on LSI IRQ %d\n", srcno);
> >> +}
> >> +
> >> +return notify;
> >> }
> >>
> >> /*
> >> @@ -103,9 +127,22 @@ static bool xive_source_esb_trigger(Xive
> >> */
> >> static bool xive_source_esb_eoi(XiveSource *xsrc, uint32_t srcno)
> >> {
> >> +bool notify;
> >> +
> >> assert(srcno < xsrc->nr_irqs);
> >>
> >> -return xive_esb_eoi(>status[srcno]);
> >> +notify = xive_esb_eoi(>status[srcno]);
> >> +
> >> +/* LSI sources do not set the Q bit but they can still be
> >> + * asserted, in which case we should forward a new event
> >> + * notification
> >> + */
> >> +if (xive_source_irq_is_lsi(xsrc, srcno)) {
> >> +bool level = xsrc->status[srcno] & XIVE_STATUS_ASSERTED;
> >> +notify = xive_source_set_irq_lsi(xsrc, srcno, level);
> >> +}
> >> +
> >> +return notify;
> >> }
> >>
> >> /*
> >> @@ -268,8 +305,12 @@ static void xive_source_set_irq(void *op
> >> XiveSource *xsrc = XIVE_SOURCE(opaque);
> >> bool notify = false;
> >>
> >> -if (val) {
> >> -notify = xive_source_esb_trigger(xsrc, srcno);
> >> +if (xive_source_irq_is_lsi(xsrc, srcno)) {
> >> +notify = xive_source_set_irq_lsi(xsrc, srcno, val);
> >> +} else {
> >> +if (val) {
> >> +notify = xive_source_esb_trigger(xsrc, srcno);
> >> +}
> >> }
> >>
> >> /* Forward the source event notification for routing */
> >> @@ -289,9 +330,11 @@ void xive_source_pic_print_info(XiveSour
> >> continue;
> >> }
> >>
> >> -monitor_printf(mon, " %08x %c%c\n", i + offset,
> >> +monitor_printf(mon, " %08x %s %c%c%c\n", i + offset,
> >> + xive_source_irq_is_lsi(xsrc, i) ? "LSI" : "MSI",
> >> pq & XIVE_ESB_VAL_P ? 'P' : '-',
> >> - pq & XIVE_ESB_VAL_Q ? 'Q' : '-');
> >> + pq & XIVE_ESB_VAL_Q ? 'Q' : '-',
> >> + xsrc->status[i] & XIVE_STATUS_ASSERTED ? 'A' : '
> >> ');
> >> }
> >> }
> >>
> >> @@ -299,6 +342,8 @@ static void xive_source_reset(DeviceStat
> >> {
> >> XiveSource *xsrc = XIVE_SOURCE(dev);
> >>
> >> +/* Do not clear the LSI bitmap */
> >> +
> >> /* PQs are initialized to 0b01 which corresponds to "ints off" */
> >> memset(xsrc->status, 0x1, xsrc->nr_irqs);
> >> }
> >> @@ -324,6 +369,7 @@ static void xive_source_realize(DeviceSt
> >> xsrc->nr_irqs);
> >>
> >> xsrc->status = g_malloc0(xsrc->nr_irqs);
> >> +xsrc->lsi_map = bitmap_new(xsrc->nr_irqs);
> >>
> >> memory_region_init_io(>esb_mmio, OBJECT(xsrc),
> >>_source_esb_ops, xsrc, "xive.esb",
> >>
> >
Re: [Qemu-devel] [PATCH v5 04/36] ppc/xive: introduce the XiveRouter model
On Mon, Nov 26, 2018 at 10:39:44AM +0100, Cédric Le Goater wrote: > On 11/26/18 6:44 AM, David Gibson wrote: > > On Fri, Nov 23, 2018 at 11:28:24AM +0100, Cédric Le Goater wrote: > >> On 11/23/18 2:10 AM, David Gibson wrote: > >>> On Thu, Nov 22, 2018 at 05:50:07PM +1100, Benjamin Herrenschmidt wrote: > On Thu, 2018-11-22 at 15:44 +1100, David Gibson wrote: > > > > Sorry, didn't think of this in my first reply. > > > > 1) Does the hardware ever actually write back to the EAS? I know it > > does for the END, but it's not clear why it would need to for the > > EAS. If not, we don't need the setter. > > Nope, though the PAPR model will via hcalls > >>> > >>> Right, bit AIUI the set_eas hook is about abstracting PAPR vs bare > >>> metal details. Since the hcall knows it's PAPR it can just update the > >>> backing information for the EAS directly, and no need for an > >>> abstracted hook. > >> > >> Indeed, the first versions of the XIVE patchset did not use such hooks, > >> but when discussed we said we wanted abstract methods for the router > >> to validate the overall XIVE model, which is useful for PowerNV. > >> > >> We can change again and have the hcalls get/set directly in the EAT > >> and ENDT. It would certainly simplify the sPAPR model. > > > > I think that's the better approach. > > ok. let's keep that in mind for : > > [PATCH v5 11/36] spapr/xive: use the VCPU id as a NVT identifier > [PATCH v5 16/36] spapr: add hcalls support for the XIVE exploitation > > which are using the XiveRouter methods to access the controller EAT > and ENDT. I thought that was good practice to validate the model but > we can use direct sPAPR table accessors or none at all. Ok. Consistency is good as a general rule, but I don't think it makes sense to force the EAT and the ENDT into the same model. The EAT is pure configuration, whereas the the ENDT has both configuration and status. Or to look at it another way, the EAT is purely software controlled, whereas the ENDT is at least partially hardware controlled. (I realize that gets a bit fuzzy when considering PAPR, but I think from the point of view of the XIVE model it makes sense to treat the PAPR hypervisor logic as "software", even though it's "hardware" from the guest point of view). > > > I think these prereq patches could be merged now : > > [PATCH v5 12/36] spapr: initialize VSMT before initializing the IRQ > [PATCH v5 13/36] spapr: introduce a spapr_irq_init() routine > [PATCH v5 14/36] spapr: modify the irq backend 'init' method > > This one also : > > [PATCH v5 21/36] spapr: extend the sPAPR IRQ backend for XICS > > Thanks, > > C. > -- 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 V9 9/9] pvpanic : update pvpanic document
>> Add mmio support info in docs/specs/pvpanic.txt. >> >> Signed-off-by: Peng Hao >> --- >> docs/specs/pvpanic.txt | 15 +-- >> 1 file changed, 13 insertions(+), 2 deletions(-) >> >> diff --git a/docs/specs/pvpanic.txt b/docs/specs/pvpanic.txt >> index c7bbacc..67f5591 100644 >> --- a/docs/specs/pvpanic.txt >> +++ b/docs/specs/pvpanic.txt >> @@ -1,14 +1,18 @@ >> PVPANIC DEVICE >> == [...] >> +When pvpanic device is implemented as a ISA device, it supports IOPORT >> +mode. Since QEMU v3.2 pvpanic also supports MMIO mode, it will be > >The next qemu release will be 4.0, not 3.2. > I will change. >> +implemented as a SYSBUS device. > >Grammar suggestion: > >The pvpanic device can be implemented as an ISA device (using IOPORT), >or, since qemu 4.0, as a SYSBUS device (using MMIO). > It's fine. thanks. >> + >> ISA Interface >> - > >-- >Eric Blake, Principal Software Engineer >Red Hat, Inc. +1-919-301-3266 >Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH for-4.0 v2] virtio: Provide version-specific variants of virtio PCI devices
On Thu, Nov 15, 2018 at 12:21:55PM +0100, Cornelia Huck wrote:
> On Wed, 14 Nov 2018 21:38:31 -0200
> Eduardo Habkost wrote:
>
> > diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
> > index 813082b0d7..1d2a11504f 100644
> > --- a/hw/virtio/virtio-pci.h
> > +++ b/hw/virtio/virtio-pci.h
>
> (...)
>
> > +/**
> > + * VirtioPCIDeviceTypeInfo:
> > + *
> > + * Template for virtio PCI device types. See virtio_pci_types_register()
> > + * for details.
> > + */
> > +typedef struct VirtioPCIDeviceTypeInfo {
> > +/* Prefix for the class names */
> > +const char *name_prefix;
>
> Maybe call this the vpci_name instead? It's not really a
> prefix in the way I would usually use the term, but rather a type name
> with a possible suffix tacked onto it.
I'm considering getting rid of all magic type name generation,
and make the struct explicit list all the type names to be
registered. This way people won't be confused when grepping the
code for the type names.
>
> > +/* Parent type. If NULL, TYPE_VIRTIO_PCI is used */
> > +const char *parent;
> > +/* If modern_only is true, only type will be registered
> > */
> > +bool modern_only;
> > +
> > +/* Same as TypeInfo fields: */
> > +size_t instance_size;
> > +void (*instance_init)(Object *obj);
> > +void (*class_init)(ObjectClass *klass, void *data);
> > +} VirtioPCIDeviceTypeInfo;
> > +
> > +/*
> > + * Register virtio-pci types. Each virtio-pci device type will be
> > available
> > + * in 3 flavors:
> > + * - -transitional: modern device supporting legacy drivers
> > + * - Supports Conventional PCI buses only
> > + * - -non-transitional: modern-only
> > + * - Supports Conventional PCI and PCI Express buses
> > + * - : virtio version configurable, legacy driver support
> > + * automatically selected when device is plugged
> > + * _ This was the only flavor supported until QEMU 3.1
>
> s/_/-/
> s/until/up to/ ?
Done. Thanks!
>
> > + * - Supports Conventional PCI and PCI Express buses
> > + *
> > + * All the types above will inherit from "-base", so generic
> > + * code can use "-base" on type casts.
> > + *
> > + * We need a separate "-base" type instead of making all types
> > inherit
> > + * from for two reasons:
> > + * 1) will implement INTERFACE_PCIE_DEVICE, but
> > + *-transitional won't.
> > + * 2) will have the "disable-legacy" and "disable-modern"
> > properties,
> > + *-transitional and -non-transitional won't.
>
> I'd formulate this rather as "x implements/has something, y
> and z do not", as the code actually does that (and does not just intend
> to do so :)
Done. Thanks!
>
> > + */
> > +void virtio_pci_types_register(const VirtioPCIDeviceTypeInfo *t);
> > +
> > #endif
>
> (...)
>
> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > index a954799267..0fa248d68c 100644
> > --- a/hw/virtio/virtio-pci.c
> > +++ b/hw/virtio/virtio-pci.c
>
> (...)
>
> > +static void virtio_pci_1_0_instance_init(Object *obj)
>
> Ditch the _0? I don't expect this to change the name when virtio 1.1
> arrives.
I was going to rename this to
virtio_pci_non_transitional_instance_init() for consistency, but
I forgot. Thanks for noting.
>
> > +{
> > +VirtIOPCIProxy *proxy = VIRTIO_PCI(obj);
> > +
> > +proxy->disable_legacy = ON_OFF_AUTO_ON;
> > +proxy->disable_modern = false;
> > +}
>
> (...)
>
> After a quick look, this seems fine; have not actually tried to run it
> yet.
--
Eduardo
Re: [Qemu-devel] [PATCH v3 02/16] i2c: have I2C receive operation return uint8_t
On 11/26/18 2:23 PM, Philippe Mathieu-Daudé wrote:
Hi Corey,
On 26/11/18 21:04, miny...@acm.org wrote:
From: Corey Minyard
It is never supposed to fail and cannot return an error, so just
have it return the proper type. Have it return 0xff on nothing
available, since that's what would happen on a real bus.
Signed-off-by: Corey Minyard
Reviewed-by: Peter Maydell
---
hw/arm/pxa2xx.c | 2 +-
hw/arm/tosa.c | 4 ++--
hw/arm/z2.c | 2 +-
hw/audio/wm8750.c | 2 +-
hw/display/sii9022.c| 2 +-
hw/display/ssd0303.c| 4 ++--
hw/gpio/max7310.c | 2 +-
hw/i2c/core.c | 32 +---
hw/i2c/i2c-ddc.c| 2 +-
hw/i2c/smbus_slave.c| 4 ++--
hw/input/lm832x.c | 2 +-
hw/misc/pca9552.c | 2 +-
hw/misc/tmp105.c| 2 +-
hw/misc/tmp421.c| 2 +-
hw/nvram/eeprom_at24c.c | 4 ++--
hw/timer/ds1338.c | 2 +-
hw/timer/m41t80.c | 2 +-
hw/timer/twl92230.c | 2 +-
include/hw/i2c/i2c.h| 7 +++
19 files changed, 37 insertions(+), 44 deletions(-)
diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
index f598a1c053..3d7c88910e 100644
--- a/hw/arm/pxa2xx.c
+++ b/hw/arm/pxa2xx.c
@@ -1286,7 +1286,7 @@ static int pxa2xx_i2c_event(I2CSlave *i2c, enum i2c_event
event)
return 0;
}
-static int pxa2xx_i2c_rx(I2CSlave *i2c)
+static uint8_t pxa2xx_i2c_rx(I2CSlave *i2c)
{
PXA2xxI2CSlaveState *slave = PXA2XX_I2C_SLAVE(i2c);
PXA2xxI2CState *s = slave->host;
diff --git a/hw/arm/tosa.c b/hw/arm/tosa.c
index 7a925fa5e6..eef9d427e7 100644
--- a/hw/arm/tosa.c
+++ b/hw/arm/tosa.c
@@ -197,10 +197,10 @@ static int tosa_dac_event(I2CSlave *i2c, enum i2c_event
event)
return 0;
}
-static int tosa_dac_recv(I2CSlave *s)
+static uint8_t tosa_dac_recv(I2CSlave *s)
{
printf("%s: recv not supported!!!\n", __func__);
-return -1;
+return 0xff;
}
static void tosa_tg_init(PXA2xxState *cpu)
diff --git a/hw/arm/z2.c b/hw/arm/z2.c
index 697a822f1e..6f18d924df 100644
--- a/hw/arm/z2.c
+++ b/hw/arm/z2.c
@@ -243,7 +243,7 @@ static int aer915_event(I2CSlave *i2c, enum i2c_event event)
return 0;
}
-static int aer915_recv(I2CSlave *slave)
+static uint8_t aer915_recv(I2CSlave *slave)
{
AER915State *s = AER915(slave);
int retval = 0x00;
diff --git a/hw/audio/wm8750.c b/hw/audio/wm8750.c
index f4aa838f62..169b006ade 100644
--- a/hw/audio/wm8750.c
+++ b/hw/audio/wm8750.c
@@ -561,7 +561,7 @@ static int wm8750_tx(I2CSlave *i2c, uint8_t data)
return 0;
}
-static int wm8750_rx(I2CSlave *i2c)
+static uint8_t wm8750_rx(I2CSlave *i2c)
{
return 0x00;
}
diff --git a/hw/display/sii9022.c b/hw/display/sii9022.c
index eaf11a6e7b..9994385c35 100644
--- a/hw/display/sii9022.c
+++ b/hw/display/sii9022.c
@@ -79,7 +79,7 @@ static int sii9022_event(I2CSlave *i2c, enum i2c_event event)
return 0;
}
-static int sii9022_rx(I2CSlave *i2c)
+static uint8_t sii9022_rx(I2CSlave *i2c)
{
sii9022_state *s = SII9022(i2c);
uint8_t res = 0x00;
diff --git a/hw/display/ssd0303.c b/hw/display/ssd0303.c
index eb90ba26be..8edf34986c 100644
--- a/hw/display/ssd0303.c
+++ b/hw/display/ssd0303.c
@@ -62,10 +62,10 @@ typedef struct {
uint8_t framebuffer[132*8];
} ssd0303_state;
-static int ssd0303_recv(I2CSlave *i2c)
+static uint8_t ssd0303_recv(I2CSlave *i2c)
{
BADF("Reads not implemented\n");
-return -1;
+return 0xff;
}
static int ssd0303_send(I2CSlave *i2c, uint8_t data)
diff --git a/hw/gpio/max7310.c b/hw/gpio/max7310.c
index a560e3afd2..f35a930276 100644
--- a/hw/gpio/max7310.c
+++ b/hw/gpio/max7310.c
@@ -39,7 +39,7 @@ static void max7310_reset(DeviceState *dev)
s->command = 0x00;
}
-static int max7310_rx(I2CSlave *i2c)
+static uint8_t max7310_rx(I2CSlave *i2c)
{
MAX7310State *s = MAX7310(i2c);
diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index b54725985a..15237ad073 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -191,23 +191,17 @@ int i2c_send_recv(I2CBus *bus, uint8_t *data, bool send)
i2c_send_recv() could benefit the same improvement.
I thought some about that, but the send function has to be able to return -1
for a NACK. It would be nice to have separate send and recv functions, but
several host devices use i2c_send_recv() directly.
}
return ret ? -1 : 0;
} else {
-if ((QLIST_EMPTY(>current_devs)) || (bus->broadcast)) {
-return -1;
-}
-
-sc = I2C_SLAVE_GET_CLASS(QLIST_FIRST(>current_devs)->elt);
-if (sc->recv) {
-s = QLIST_FIRST(>current_devs)->elt;
-ret = sc->recv(s);
-trace_i2c_recv(s->address, ret);
-if (ret < 0) {
-return ret;
-} else {
-*data = ret;
-return 0;
+ret = 0xff;
+if (!QLIST_EMPTY(>current_devs) && !bus->broadcast) {
+
[Qemu-devel] [Bug 1805256] Re: qemu-img hangs on high core count ARM system
ext4 filesystem, SATA drive: (gdb) thread apply all bt Thread 3 (Thread 0x9bffc9a0 (LWP 9015)): #0 0xaaa462cc in __GI___sigtimedwait (set=, set@entry=0xe725c070, info=info@entry=0x9bffbf18, timeout=0x3ff1, timeout@entry=0x0) at ../sysdeps/unix/sysv/linux/sigtimedwait.c:42 #1 0xaab7dfac in __sigwait (set=set@entry=0xe725c070, sig=sig@entry=0x9bffbff4) at ../sysdeps/unix/sysv/linux/sigwait.c:28 #2 0xd998a628 in sigwait_compat (opaque=0xe725c070) at util/compatfd.c:36 #3 0xd998bce0 in qemu_thread_start (args=) at util/qemu-thread-posix.c:498 #4 0xaab73088 in start_thread (arg=0xc528531f) at pthread_create.c:463 #5 0xaaae34ec in thread_start () at ../sysdeps/unix/sysv/linux/aarch64/clone.S:78 Thread 2 (Thread 0xa0e779a0 (LWP 9014)): #0 syscall () at ../sysdeps/unix/sysv/linux/aarch64/syscall.S:38 #1 0xd998c9e8 in qemu_futex_wait (val=, f=) at /home/ubuntu/qemu/include/qemu/futex.h:29 #2 qemu_event_wait (ev=ev@entry=0xd9a091c0 ) at util/qemu-thread-posix.c:442 #3 0xd99a6834 in call_rcu_thread (opaque=) at util/rcu.c:261 #4 0xd998bce0 in qemu_thread_start (args=) at util/qemu-thread-posix.c:498 #5 0xaab73088 in start_thread (arg=0xc528542f) at pthread_create.c:463 #6 0xaaae34ec in thread_start () at ../sysdeps/unix/sysv/linux/aarch64/clone.S:78 Thread 1 (Thread 0xa0fa8010 (LWP 9013)): #0 0xaaada154 in __GI_ppoll (fds=0xe7291dc0, nfds=187650771816320, timeout=, timeout@entry=0x0, sigmask=0xc52852e0) at ../sysdeps/unix/sysv/linux/ppoll.c:39 #1 0xd9987f00 in ppoll (__ss=0x0, __timeout=0x0, __nfds=, __fds=) at /usr/include/aarch64-linux-gnu/bits/poll2.h:77 #2 qemu_poll_ns (fds=, nfds=, timeout=timeout@entry=-1) at util/qemu-timer.c:322 #3 0xd9988f80 in os_host_main_loop_wait (timeout=-1) at util/main-loop.c:233 #4 main_loop_wait (nonblocking=) at util/main-loop.c:497 #5 0xd98b7a30 in convert_do_copy (s=0xc52854e8) at qemu-img.c:1980 #6 img_convert (argc=, argv=) at qemu-img.c:2456 #7 0xd98b033c in main (argc=7, argv=) at qemu-img.c:4975 -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1805256 Title: qemu-img hangs on high core count ARM system Status in QEMU: New Bug description: On the HiSilicon D06 system - a 96 core NUMA arm64 box - qemu-img frequently hangs (~50% of the time) with this command: qemu-img convert -f qcow2 -O qcow2 /tmp/cloudimg /tmp/cloudimg2 Where "cloudimg" is a standard qcow2 Ubuntu cloud image. This qcow2->qcow2 conversion happens to be something uvtool does every time it fetches images. Once hung, attaching gdb gives the following backtrace: (gdb) bt #0 0xae4f8154 in __GI_ppoll (fds=0xe8a67dc0, nfds=187650274213760, timeout=, timeout@entry=0x0, sigmask=0xc123b950) at ../sysdeps/unix/sysv/linux/ppoll.c:39 #1 0xbbefaf00 in ppoll (__ss=0x0, __timeout=0x0, __nfds=, __fds=) at /usr/include/aarch64-linux-gnu/bits/poll2.h:77 #2 qemu_poll_ns (fds=, nfds=, timeout=timeout@entry=-1) at util/qemu-timer.c:322 #3 0xbbefbf80 in os_host_main_loop_wait (timeout=-1) at util/main-loop.c:233 #4 main_loop_wait (nonblocking=) at util/main-loop.c:497 #5 0xbbe2aa30 in convert_do_copy (s=0xc123bb58) at qemu-img.c:1980 #6 img_convert (argc=, argv=) at qemu-img.c:2456 #7 0xbbe2333c in main (argc=7, argv=) at qemu-img.c:4975 Reproduced w/ latest QEMU git (@ 53744e0a182) To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1805256/+subscriptions
Re: [Qemu-devel] [PATCH v3 16/16] i2c:smbus_eeprom: Add a reset function to smbus_eeprom
On 11/26/18 5:01 PM, Philippe Mathieu-Daudé wrote:
On 26/11/18 23:41, Corey Minyard wrote:
On 11/26/18 2:42 PM, Philippe Mathieu-Daudé wrote:
Hi Corey,
On 26/11/18 21:04, miny...@acm.org wrote:
From: Corey Minyard
Reset the contents to init data and reset the offset on a machine
reset.
Signed-off-by: Corey Minyard
---
hw/i2c/smbus_eeprom.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
index a0dcadbd60..de3a492df4 100644
--- a/hw/i2c/smbus_eeprom.c
+++ b/hw/i2c/smbus_eeprom.c
@@ -107,7 +107,7 @@ static const VMStateDescription
vmstate_smbus_eeprom = {
}
};
-static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
+static void smbus_eeprom_reset(DeviceState *dev)
{
SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev);
'git diff -U4' also shows this line:
memcpy(eeprom->data, eeprom->init_data, SMBUS_EEPROM_SIZE);
I don't think this is correct.
One test I'd like to have is a machine booting, updating the EPROM then
rebooting calling hw reset() to use the new values (BIOS use this).
With this patch this won't work, you'll restore the EPROM content on
each machine reset.
I'd move the memcpy() call to the realize() function.
What do you think?
There was some debate on this in the earlier patch set. The general
principle
Hmm I missed it and can't find it (quick basic search). I only find
references about VMState.
It starts at
http://lists.nongnu.org/archive/html/qemu-devel/2018-11/msg01737.html
The patch set was fairly different at that point.
is that a reset is the same as starting up qemu from scratch, so I added
this
code based on that principle. But I'm not really sure.
eeprom->offset = 0;
This is correct, the offset reset belongs to the reset() function.
Actually, on a real system, a hardware reset will generally not affect the
eeprom current offset register. So if we don't take the above code, then
IMHO this is wrong, too.
Indeed cpu reset shouldn't affect the EEPROM, but a board powercycle would.
Maybe we can argue QEMU system reset doesn't work correctly yet to use
this feature. Personally I wouldn't expect the EEPROM content be be
reset after a reset, but maybe I should rely on a block backend for a
such feature, and not the current simple approach.
Yeah, it was mentioned that to do this correctly would require a block
backend.
I'll let others comment on the correctness of this, I guess. It's a
separate patch
so it can be easily dropped.
The current code is far too broken for anyone to be using it, so we won't be
breaking any current users, I don't think.
-corey
-corey
Regards,
Phil.
}
+static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
+{
+ smbus_eeprom_reset(dev);
+}
+
static Property smbus_eeprom_properties[] = {
DEFINE_PROP_PTR("data", SMBusEEPROMDevice, init_data),
DEFINE_PROP_END_OF_LIST(),
@@ -126,6 +131,7 @@ static void smbus_eeprom_class_initfn(ObjectClass
*klass, void *data)
SMBusDeviceClass *sc = SMBUS_DEVICE_CLASS(klass);
dc->realize = smbus_eeprom_realize;
+ dc->reset = smbus_eeprom_reset;
sc->receive_byte = eeprom_receive_byte;
sc->write_data = eeprom_write_data;
dc->props = smbus_eeprom_properties;
Re: [Qemu-devel] [RFC 10/48] exec: export do_tb_flush
On Mon, Nov 26, 2018 at 11:11:53 +, Alex Bennée wrote:
>
> Emilio G. Cota writes:
>
> > On Thu, Nov 22, 2018 at 17:09:22 +, Alex Bennée wrote:
> >>
> >> Emilio G. Cota writes:
> >>
> >> > This will be used by plugin code to flush the code cache as well
> >> > as doing other bookkeeping in a safe work environment.
> >>
> >> This seems a little excessive given the plugin code could just call
> >> tb_flush() directly. Wouldn't calling tb_flush after scheduling the
> >> plugin_destroy be enough?
> >>
> >> If there is a race condition here maybe we could build some sort of
> >> awareness into tb_flush as to the current run state. But having two
> >> entry points to this rather fundamental action seems likely to either be
> >> misused or misunderstood.
> >
> > We have to make sure that no callback left in the generated code is
> > called once a plugin has been uninstalled. To me, using the same safe
> > work window to both flush the TB and uninstall the plugin seems the
> > simplest way to do this.
>
> I still think making tb_flush() aware that it can run in an exclusive
> period would be a better solution than exposing two functions for the
> operation. So tb_flush could be something like:
>
> void tb_flush(CPUState *cpu)
> {
> if (tcg_enabled()) {
> unsigned tb_flush_count = atomic_mb_read(_ctx.tb_flush_count);
> if (cpu_current_and_exclusive(cpu)) {
> do_tb_flush(RUN_ON_CPU_HOST_INT(tb_flush_count))
> } else {
> async_safe_run_on_cpu(cpu, do_tb_flush,
> RUN_ON_CPU_HOST_INT(tb_flush_count));
> }
> }
> }
>
> Or possibly push that logic down into async_safe_run_on_cpu()?
The latter option would be much harder, because in async_safe_run_on_cpu
we always queue the work and kick the CPU (which could be ourselves).
IOW the job is always asynchronous, as the name implies.
I've thus implemented the former in v2, as follows (I'm using a hole
in struct CPUState to add the bool):
@@ -1277,8 +1277,13 @@ void tb_flush(CPUState *cpu)
{
if (tcg_enabled()) {
unsigned tb_flush_count = atomic_mb_read(_ctx.tb_flush_count);
-async_safe_run_on_cpu(cpu, do_tb_flush,
- RUN_ON_CPU_HOST_INT(tb_flush_count));
+
+if (cpu_in_exclusive_work_context(cpu)) {
+do_tb_flush(cpu, RUN_ON_CPU_HOST_INT(tb_flush_count));
+} else {
+async_safe_run_on_cpu(cpu, do_tb_flush,
+ RUN_ON_CPU_HOST_INT(tb_flush_count));
+}
}
}
+++ b/cpus-common.c
@@ -386,7 +386,9 @@ static void process_queued_cpu_work_locked(CPUState *cpu)
qemu_mutex_unlock_iothread();
}
start_exclusive();
+cpu->in_exclusive_work_context = true;
wi->func(cpu, wi->data);
+cpu->in_exclusive_work_context = false;
end_exclusive();
I've also fixed a couple of unrelated bugs when uninstalling a plugin
with memory callbacks enabled.
Thanks,
Emilio
[Qemu-devel] [Bug 1805256] Re: qemu-img hangs on high core count ARM system
Hi, can you do a `thread apply all bt` instead? If I were to bet, we're probably waiting for some slow call like lseek to return in another thread. What filesystem/blockdevice is involved here? -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1805256 Title: qemu-img hangs on high core count ARM system Status in QEMU: New Bug description: On the HiSilicon D06 system - a 96 core NUMA arm64 box - qemu-img frequently hangs (~50% of the time) with this command: qemu-img convert -f qcow2 -O qcow2 /tmp/cloudimg /tmp/cloudimg2 Where "cloudimg" is a standard qcow2 Ubuntu cloud image. This qcow2->qcow2 conversion happens to be something uvtool does every time it fetches images. Once hung, attaching gdb gives the following backtrace: (gdb) bt #0 0xae4f8154 in __GI_ppoll (fds=0xe8a67dc0, nfds=187650274213760, timeout=, timeout@entry=0x0, sigmask=0xc123b950) at ../sysdeps/unix/sysv/linux/ppoll.c:39 #1 0xbbefaf00 in ppoll (__ss=0x0, __timeout=0x0, __nfds=, __fds=) at /usr/include/aarch64-linux-gnu/bits/poll2.h:77 #2 qemu_poll_ns (fds=, nfds=, timeout=timeout@entry=-1) at util/qemu-timer.c:322 #3 0xbbefbf80 in os_host_main_loop_wait (timeout=-1) at util/main-loop.c:233 #4 main_loop_wait (nonblocking=) at util/main-loop.c:497 #5 0xbbe2aa30 in convert_do_copy (s=0xc123bb58) at qemu-img.c:1980 #6 img_convert (argc=, argv=) at qemu-img.c:2456 #7 0xbbe2333c in main (argc=7, argv=) at qemu-img.c:4975 Reproduced w/ latest QEMU git (@ 53744e0a182) To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1805256/+subscriptions
[Qemu-devel] [PATCH 4/4] tcg: Add reachable_code_pass
Delete trivially dead code that follows unconditional branches and
noreturn helpers. These can occur either via optimization or via
the structure of a target's translator following an exception.
Signed-off-by: Richard Henderson
---
tcg/tcg.c | 76 +++
1 file changed, 76 insertions(+)
diff --git a/tcg/tcg.c b/tcg/tcg.c
index 31b9b58240..ffbf8f01ad 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -2301,6 +2301,81 @@ static void tcg_la_bb_end(TCGContext *s)
}
}
+/* Reachable analysis : remove unreachable code. */
+static void reachable_code_pass(TCGContext *s)
+{
+TCGOp *op, *op_next;
+bool dead = false;
+
+QTAILQ_FOREACH_SAFE(op, >ops, link, op_next) {
+bool remove = dead;
+TCGLabel *label;
+int call_flags;
+
+switch (op->opc) {
+case INDEX_op_set_label:
+label = arg_label(op->args[0]);
+if (label->refs == 0) {
+/*
+ * While there is an occasional backward branch, virtually
+ * all branches generated by the translators are forward.
+ * Which means that generally we will have already removed
+ * all references to the label that will be, and there is
+ * little to be gained by iterating.
+ */
+remove = true;
+} else {
+/* Once we see a label, insns become live again. */
+dead = false;
+remove = false;
+
+/*
+ * Optimization can fold conditional branches to unconditional.
+ * If we find a label with one reference which is preceeded by
+ * an unconditional branch to it, remove both. This needed to
+ * wait until the dead code in between them was removed.
+ */
+if (label->refs == 1) {
+TCGOp *op_prev = QTAILQ_PREV(op, TCGOpHead, link);
+if (op_prev->opc == INDEX_op_br &&
+label == arg_label(op_prev->args[0])) {
+tcg_op_remove(s, op_prev);
+remove = true;
+}
+}
+}
+break;
+
+case INDEX_op_br:
+case INDEX_op_exit_tb:
+case INDEX_op_goto_ptr:
+/* Unconditional branches; everything following is dead. */
+dead = true;
+break;
+
+case INDEX_op_call:
+/* Notice noreturn helper calls, raising exceptions. */
+call_flags = op->args[TCGOP_CALLO(op) + TCGOP_CALLI(op) + 1];
+if (call_flags & TCG_CALL_NO_RETURN) {
+dead = true;
+}
+break;
+
+case INDEX_op_insn_start:
+/* Never remove -- we need to keep these for unwind. */
+remove = false;
+break;
+
+default:
+break;
+}
+
+if (remove) {
+tcg_op_remove(s, op);
+}
+}
+}
+
/* Liveness analysis : update the opc_arg_life array to tell if a
given input arguments is dead. Instructions updating dead
temporaries are removed. */
@@ -3537,6 +3612,7 @@ int tcg_gen_code(TCGContext *s, TranslationBlock *tb)
atomic_set(>la_time, prof->la_time - profile_getclock());
#endif
+reachable_code_pass(s);
liveness_pass_1(s);
if (s->nb_indirects > 0) {
--
2.17.2
[Qemu-devel] [PATCH 3/4] tcg: Reference count labels
Increment when adding branches, and decrement when removing them.
Signed-off-by: Richard Henderson
---
tcg/tcg-op.h | 1 +
tcg/tcg.h| 3 ++-
tcg/tcg-op.c | 2 ++
tcg/tcg.c| 20
4 files changed, 25 insertions(+), 1 deletion(-)
diff --git a/tcg/tcg-op.h b/tcg/tcg-op.h
index db4e9188f4..7007ec0d4d 100644
--- a/tcg/tcg-op.h
+++ b/tcg/tcg-op.h
@@ -260,6 +260,7 @@ static inline void gen_set_label(TCGLabel *l)
static inline void tcg_gen_br(TCGLabel *l)
{
+l->refs++;
tcg_gen_op1(INDEX_op_br, label_arg(l));
}
diff --git a/tcg/tcg.h b/tcg/tcg.h
index 6b6bc75c82..c6caeeb42b 100644
--- a/tcg/tcg.h
+++ b/tcg/tcg.h
@@ -246,7 +246,8 @@ typedef struct TCGRelocation {
typedef struct TCGLabel {
unsigned has_value : 1;
-unsigned id : 31;
+unsigned id : 15;
+unsigned refs : 16;
union {
uintptr_t value;
tcg_insn_unit *value_ptr;
diff --git a/tcg/tcg-op.c b/tcg/tcg-op.c
index 1ad095cc35..bab889662d 100644
--- a/tcg/tcg-op.c
+++ b/tcg/tcg-op.c
@@ -240,6 +240,7 @@ void tcg_gen_brcond_i32(TCGCond cond, TCGv_i32 arg1,
TCGv_i32 arg2, TCGLabel *l)
if (cond == TCG_COND_ALWAYS) {
tcg_gen_br(l);
} else if (cond != TCG_COND_NEVER) {
+l->refs++;
tcg_gen_op4ii_i32(INDEX_op_brcond_i32, arg1, arg2, cond, label_arg(l));
}
}
@@ -1405,6 +1406,7 @@ void tcg_gen_brcond_i64(TCGCond cond, TCGv_i64 arg1,
TCGv_i64 arg2, TCGLabel *l)
if (cond == TCG_COND_ALWAYS) {
tcg_gen_br(l);
} else if (cond != TCG_COND_NEVER) {
+l->refs++;
if (TCG_TARGET_REG_BITS == 32) {
tcg_gen_op6ii_i32(INDEX_op_brcond2_i32, TCGV_LOW(arg1),
TCGV_HIGH(arg1), TCGV_LOW(arg2),
diff --git a/tcg/tcg.c b/tcg/tcg.c
index 17c193791f..31b9b58240 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -2191,6 +2191,26 @@ static void process_op_defs(TCGContext *s)
void tcg_op_remove(TCGContext *s, TCGOp *op)
{
+TCGLabel *label;
+
+switch (op->opc) {
+case INDEX_op_br:
+label = arg_label(op->args[0]);
+label->refs--;
+break;
+case INDEX_op_brcond_i32:
+case INDEX_op_brcond_i64:
+label = arg_label(op->args[3]);
+label->refs--;
+break;
+case INDEX_op_brcond2_i32:
+label = arg_label(op->args[5]);
+label->refs--;
+break;
+default:
+break;
+}
+
QTAILQ_REMOVE(>ops, op, link);
QTAILQ_INSERT_TAIL(>free_ops, op, link);
s->nb_ops--;
--
2.17.2
[Qemu-devel] [PATCH 1/4] tcg: Renumber TCG_CALL_* flags
Previously, the low 4 bits were used for TCG_CALL_TYPE_MASK, which was removed in 6a18ae2d2947532d5c26439548afa0481c4529f9. Signed-off-by: Richard Henderson --- tcg/tcg.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tcg/tcg.h b/tcg/tcg.h index 73737dc671..e94f805370 100644 --- a/tcg/tcg.h +++ b/tcg/tcg.h @@ -464,11 +464,11 @@ typedef TCGv_ptr TCGv_env; /* call flags */ /* Helper does not read globals (either directly or through an exception). It implies TCG_CALL_NO_WRITE_GLOBALS. */ -#define TCG_CALL_NO_READ_GLOBALS0x0010 +#define TCG_CALL_NO_READ_GLOBALS0x0001 /* Helper does not write globals */ -#define TCG_CALL_NO_WRITE_GLOBALS 0x0020 +#define TCG_CALL_NO_WRITE_GLOBALS 0x0002 /* Helper can be safely suppressed if the return value is not used. */ -#define TCG_CALL_NO_SIDE_EFFECTS0x0040 +#define TCG_CALL_NO_SIDE_EFFECTS0x0004 /* convenience version of most used call flags */ #define TCG_CALL_NO_RWG TCG_CALL_NO_READ_GLOBALS -- 2.17.2
[Qemu-devel] [PATCH 2/4] tcg: Add TCG_CALL_NO_RETURN
Remember which helpers have been marked noreturn.
Signed-off-by: Richard Henderson
---
include/exec/helper-head.h | 13 +
include/exec/helper-tcg.h | 21 ++---
tcg/tcg.h | 2 ++
3 files changed, 29 insertions(+), 7 deletions(-)
diff --git a/include/exec/helper-head.h b/include/exec/helper-head.h
index 276dd5afce..ab4f8b6623 100644
--- a/include/exec/helper-head.h
+++ b/include/exec/helper-head.h
@@ -108,6 +108,19 @@
#define dh_is_signed_env dh_is_signed_ptr
#define dh_is_signed(t) dh_is_signed_##t
+#define dh_callflag_i32 0
+#define dh_callflag_s32 0
+#define dh_callflag_int 0
+#define dh_callflag_i64 0
+#define dh_callflag_s64 0
+#define dh_callflag_f16 0
+#define dh_callflag_f32 0
+#define dh_callflag_f64 0
+#define dh_callflag_ptr 0
+#define dh_callflag_void 0
+#define dh_callflag_noreturn TCG_CALL_NO_RETURN
+#define dh_callflag(t) glue(dh_callflag_, dh_alias(t))
+
#define dh_sizemask(t, n) \
((dh_is_64bit(t) << (n*2)) | (dh_is_signed(t) << (n*2+1)))
diff --git a/include/exec/helper-tcg.h b/include/exec/helper-tcg.h
index b3bdb0c399..268e0f804b 100644
--- a/include/exec/helper-tcg.h
+++ b/include/exec/helper-tcg.h
@@ -11,36 +11,43 @@
#define str(s) #s
#define DEF_HELPER_FLAGS_0(NAME, FLAGS, ret) \
- { .func = HELPER(NAME), .name = str(NAME), .flags = FLAGS, \
+ { .func = HELPER(NAME), .name = str(NAME), \
+.flags = FLAGS | dh_callflag(ret), \
.sizemask = dh_sizemask(ret, 0) },
#define DEF_HELPER_FLAGS_1(NAME, FLAGS, ret, t1) \
- { .func = HELPER(NAME), .name = str(NAME), .flags = FLAGS, \
+ { .func = HELPER(NAME), .name = str(NAME), \
+.flags = FLAGS | dh_callflag(ret), \
.sizemask = dh_sizemask(ret, 0) | dh_sizemask(t1, 1) },
#define DEF_HELPER_FLAGS_2(NAME, FLAGS, ret, t1, t2) \
- { .func = HELPER(NAME), .name = str(NAME), .flags = FLAGS, \
+ { .func = HELPER(NAME), .name = str(NAME), \
+.flags = FLAGS | dh_callflag(ret), \
.sizemask = dh_sizemask(ret, 0) | dh_sizemask(t1, 1) \
| dh_sizemask(t2, 2) },
#define DEF_HELPER_FLAGS_3(NAME, FLAGS, ret, t1, t2, t3) \
- { .func = HELPER(NAME), .name = str(NAME), .flags = FLAGS, \
+ { .func = HELPER(NAME), .name = str(NAME), \
+.flags = FLAGS | dh_callflag(ret), \
.sizemask = dh_sizemask(ret, 0) | dh_sizemask(t1, 1) \
| dh_sizemask(t2, 2) | dh_sizemask(t3, 3) },
#define DEF_HELPER_FLAGS_4(NAME, FLAGS, ret, t1, t2, t3, t4) \
- { .func = HELPER(NAME), .name = str(NAME), .flags = FLAGS, \
+ { .func = HELPER(NAME), .name = str(NAME), \
+.flags = FLAGS | dh_callflag(ret), \
.sizemask = dh_sizemask(ret, 0) | dh_sizemask(t1, 1) \
| dh_sizemask(t2, 2) | dh_sizemask(t3, 3) | dh_sizemask(t4, 4) },
#define DEF_HELPER_FLAGS_5(NAME, FLAGS, ret, t1, t2, t3, t4, t5) \
- { .func = HELPER(NAME), .name = str(NAME), .flags = FLAGS, \
+ { .func = HELPER(NAME), .name = str(NAME), \
+.flags = FLAGS | dh_callflag(ret), \
.sizemask = dh_sizemask(ret, 0) | dh_sizemask(t1, 1) \
| dh_sizemask(t2, 2) | dh_sizemask(t3, 3) | dh_sizemask(t4, 4) \
| dh_sizemask(t5, 5) },
#define DEF_HELPER_FLAGS_6(NAME, FLAGS, ret, t1, t2, t3, t4, t5, t6) \
- { .func = HELPER(NAME), .name = str(NAME), .flags = FLAGS, \
+ { .func = HELPER(NAME), .name = str(NAME), \
+.flags = FLAGS | dh_callflag(ret), \
.sizemask = dh_sizemask(ret, 0) | dh_sizemask(t1, 1) \
| dh_sizemask(t2, 2) | dh_sizemask(t3, 3) | dh_sizemask(t4, 4) \
| dh_sizemask(t5, 5) | dh_sizemask(t6, 6) },
diff --git a/tcg/tcg.h b/tcg/tcg.h
index e94f805370..6b6bc75c82 100644
--- a/tcg/tcg.h
+++ b/tcg/tcg.h
@@ -469,6 +469,8 @@ typedef TCGv_ptr TCGv_env;
#define TCG_CALL_NO_WRITE_GLOBALS 0x0002
/* Helper can be safely suppressed if the return value is not used. */
#define TCG_CALL_NO_SIDE_EFFECTS0x0004
+/* Helper is QEMU_NORETURN. */
+#define TCG_CALL_NO_RETURN 0x0008
/* convenience version of most used call flags */
#define TCG_CALL_NO_RWG TCG_CALL_NO_READ_GLOBALS
--
2.17.2
[Qemu-devel] [PATCH 0/4] tcg: Remove unreachable code
I've been meaning to add a trivial cleanup pass like this for some time. There have occasionally been instaces within front ends wherein we want to raise an invalid operand exception (or some such) deep within a set of subroutines. And without a longjmp (or some such) back to the top level of the translator loop we must return a dummy value so that we produce valid tcg code following the exception. While we still probably have to return a dummy value, we can clean up the dead code that follows the exception. In addition, when optimization is able to fold a conditional branch, the original code sequence: brcond x,y,$L1 goto_tb mov pc,foo exit_tb $1 set_label $L1 goto_tb mov pc,bar exit_tb $2 The initial brcond is either folded to br, or removed as a never taken branch, which leaves one of the two sets of goto_tb ... exit_tb unreachable. With this we can completely remove the code for the dead branch. While I do not expect this to have any noticable effect, this dead code could get in the way of some other changes that I'm planning. r~ Richard Henderson (4): tcg: Renumber TCG_CALL_* flags tcg: Add TCG_CALL_NO_RETURN tcg: Reference count labels tcg: Add reachable_code_pass include/exec/helper-head.h | 13 ++ include/exec/helper-tcg.h | 21 ++--- tcg/tcg-op.h | 1 + tcg/tcg.h | 11 +++-- tcg/tcg-op.c | 2 + tcg/tcg.c | 96 ++ 6 files changed, 133 insertions(+), 11 deletions(-) -- 2.17.2
Re: [Qemu-devel] [PATCH v3 16/16] i2c:smbus_eeprom: Add a reset function to smbus_eeprom
On 26/11/18 23:41, Corey Minyard wrote:
> On 11/26/18 2:42 PM, Philippe Mathieu-Daudé wrote:
>> Hi Corey,
>>
>> On 26/11/18 21:04, miny...@acm.org wrote:
>>> From: Corey Minyard
>>>
>>> Reset the contents to init data and reset the offset on a machine
>>> reset.
>>>
>>> Signed-off-by: Corey Minyard
>>> ---
>>> hw/i2c/smbus_eeprom.c | 8 +++-
>>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
>>> index a0dcadbd60..de3a492df4 100644
>>> --- a/hw/i2c/smbus_eeprom.c
>>> +++ b/hw/i2c/smbus_eeprom.c
>>> @@ -107,7 +107,7 @@ static const VMStateDescription
>>> vmstate_smbus_eeprom = {
>>> }
>>> };
>>> -static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
>>> +static void smbus_eeprom_reset(DeviceState *dev)
>>> {
>>> SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev);
>>>
>> 'git diff -U4' also shows this line:
>>
>> memcpy(eeprom->data, eeprom->init_data, SMBUS_EEPROM_SIZE);
>>
>> I don't think this is correct.
>>
>> One test I'd like to have is a machine booting, updating the EPROM then
>> rebooting calling hw reset() to use the new values (BIOS use this).
>>
>> With this patch this won't work, you'll restore the EPROM content on
>> each machine reset.
>>
>> I'd move the memcpy() call to the realize() function.
>>
>> What do you think?
>
> There was some debate on this in the earlier patch set. The general
> principle
Hmm I missed it and can't find it (quick basic search). I only find
references about VMState.
> is that a reset is the same as starting up qemu from scratch, so I added
> this
> code based on that principle. But I'm not really sure.
>
>>> eeprom->offset = 0;
>> This is correct, the offset reset belongs to the reset() function.
>
> Actually, on a real system, a hardware reset will generally not affect the
> eeprom current offset register. So if we don't take the above code, then
> IMHO this is wrong, too.
Indeed cpu reset shouldn't affect the EEPROM, but a board powercycle would.
Maybe we can argue QEMU system reset doesn't work correctly yet to use
this feature. Personally I wouldn't expect the EEPROM content be be
reset after a reset, but maybe I should rely on a block backend for a
such feature, and not the current simple approach.
>
> -corey
>
>
>> Regards,
>>
>> Phil.
>>
>>> }
>>> +static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
>>> +{
>>> + smbus_eeprom_reset(dev);
>>> +}
>>> +
>>> static Property smbus_eeprom_properties[] = {
>>> DEFINE_PROP_PTR("data", SMBusEEPROMDevice, init_data),
>>> DEFINE_PROP_END_OF_LIST(),
>>> @@ -126,6 +131,7 @@ static void smbus_eeprom_class_initfn(ObjectClass
>>> *klass, void *data)
>>> SMBusDeviceClass *sc = SMBUS_DEVICE_CLASS(klass);
>>> dc->realize = smbus_eeprom_realize;
>>> + dc->reset = smbus_eeprom_reset;
>>> sc->receive_byte = eeprom_receive_byte;
>>> sc->write_data = eeprom_write_data;
>>> dc->props = smbus_eeprom_properties;
>>>
>
[Qemu-devel] [Bug 1805256] [NEW] qemu-img hangs on high core count ARM system
Public bug reported: On the HiSilicon D06 system - a 96 core NUMA arm64 box - qemu-img frequently hangs (~50% of the time) with this command: qemu-img convert -f qcow2 -O qcow2 /tmp/cloudimg /tmp/cloudimg2 Where "cloudimg" is a standard qcow2 Ubuntu cloud image. This qcow2->qcow2 conversion happens to be something uvtool does every time it fetches images. Once hung, attaching gdb gives the following backtrace: (gdb) bt #0 0xae4f8154 in __GI_ppoll (fds=0xe8a67dc0, nfds=187650274213760, timeout=, timeout@entry=0x0, sigmask=0xc123b950) at ../sysdeps/unix/sysv/linux/ppoll.c:39 #1 0xbbefaf00 in ppoll (__ss=0x0, __timeout=0x0, __nfds=, __fds=) at /usr/include/aarch64-linux-gnu/bits/poll2.h:77 #2 qemu_poll_ns (fds=, nfds=, timeout=timeout@entry=-1) at util/qemu-timer.c:322 #3 0xbbefbf80 in os_host_main_loop_wait (timeout=-1) at util/main-loop.c:233 #4 main_loop_wait (nonblocking=) at util/main-loop.c:497 #5 0xbbe2aa30 in convert_do_copy (s=0xc123bb58) at qemu-img.c:1980 #6 img_convert (argc=, argv=) at qemu-img.c:2456 #7 0xbbe2333c in main (argc=7, argv=) at qemu-img.c:4975 Reproduced w/ latest QEMU git (@ 53744e0a182) ** Affects: qemu Importance: Undecided Status: New -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1805256 Title: qemu-img hangs on high core count ARM system Status in QEMU: New Bug description: On the HiSilicon D06 system - a 96 core NUMA arm64 box - qemu-img frequently hangs (~50% of the time) with this command: qemu-img convert -f qcow2 -O qcow2 /tmp/cloudimg /tmp/cloudimg2 Where "cloudimg" is a standard qcow2 Ubuntu cloud image. This qcow2->qcow2 conversion happens to be something uvtool does every time it fetches images. Once hung, attaching gdb gives the following backtrace: (gdb) bt #0 0xae4f8154 in __GI_ppoll (fds=0xe8a67dc0, nfds=187650274213760, timeout=, timeout@entry=0x0, sigmask=0xc123b950) at ../sysdeps/unix/sysv/linux/ppoll.c:39 #1 0xbbefaf00 in ppoll (__ss=0x0, __timeout=0x0, __nfds=, __fds=) at /usr/include/aarch64-linux-gnu/bits/poll2.h:77 #2 qemu_poll_ns (fds=, nfds=, timeout=timeout@entry=-1) at util/qemu-timer.c:322 #3 0xbbefbf80 in os_host_main_loop_wait (timeout=-1) at util/main-loop.c:233 #4 main_loop_wait (nonblocking=) at util/main-loop.c:497 #5 0xbbe2aa30 in convert_do_copy (s=0xc123bb58) at qemu-img.c:1980 #6 img_convert (argc=, argv=) at qemu-img.c:2456 #7 0xbbe2333c in main (argc=7, argv=) at qemu-img.c:4975 Reproduced w/ latest QEMU git (@ 53744e0a182) To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1805256/+subscriptions
Re: [Qemu-devel] [RFC v1 15/23] riscv: tcg-target: Add branch and jump instructions
On Tue, Nov 20, 2018 at 11:40 PM Richard Henderson
wrote:
>
> On 11/21/18 12:49 AM, Alistair Francis wrote:
> > On Fri, Nov 16, 2018 at 1:14 AM Richard Henderson
> > wrote:
> >>
> >> On 11/15/18 11:36 PM, Alistair Francis wrote:
> >>> +static void tcg_out_brcond(TCGContext *s, TCGCond cond, TCGReg arg1,
> >>> + TCGReg arg2, TCGLabel *l)
> >>> +{
> >>> +RISCVInsn op = tcg_brcond_to_riscv[cond].op;
> >>> +bool swap = tcg_brcond_to_riscv[cond].swap;
> >>> +
> >>> +tcg_out_opc_branch(s, op, swap ? arg2 : arg1, swap ? arg1 : arg2, 0);
> >>
> >> You might want to tcg_debug_assert(op != 0) here.
> >>
> >>> +if (l->has_value) {
> >>> +reloc_sbimm12(s->code_ptr - 1, l->u.value_ptr);
> >>
> >> I'm concerned about the conditional branch range. +-4K isn't much to work
> >> with. The minimum we have for other hosts is +-32K.
> >>
> >> We have two options: (1) greatly reduce the max size of the TB for this
> >> host;
> >> (2) be prepared to emit a 2 insn sequence: conditional branch across
> >> unconditional branch, with forward branches that turn out to be small
> >> patched
> >> with a nop.
> >>
> >> FWIW, the first case would be done via modification of tcg_op_buf_full.
> >> You
> >> might have to go as low as 500 opcodes, I'm not sure.
> >
> > How do we do option 2?
>
> If l->has_value, just check the actual range. But of course backward
> branching
> isn't that common in tcg generated code. Most branches within the TB are
> short
> forward branches, but we also don't know how short is short.
>
> But every TB begins with a test of env->exit_code and a conditional branch to
> the end of the block, where we place some code to return to the main loop and
> return the pointer to the TB at which we exited. Thus every TB has a branch
> that spans the size of the entire TB.
>
> So, invent (or repurpose) an R_RISCV_FOO value. It doesn't matter which
> because it's private within tcg/riscv/. Just add some commentary. (See e.g.
> tcg/sparc/ and its use of R_SPARC_13.)
>
> While generating code, emit the conditional branch as normal; leave the
> unknown
> destination 0 for now. Emit a nop as the second insn.
>
> When resolving R_RISCV_FOO, if the conditional branch is in range, great!
> Just
> patch it. If it is out of range, then you need to edit the conditional branch
> to reverse the condition (insn ^ (1 << 12)) and branch to pc+8, i.e. over the
> next instruction. Which was a nop during generation, but you will now install
> jal r0,dest going to the real destination.
Ok, I think I have done this correctly. I have something that compiles and runs.
I'll send a second RFC out sometime this week. It won't be based on
your latest patches and it's still missing some things but I want to
keep this moving along.
Thanks so much for your help Richard!
Alistair
>
>
> r~
[Qemu-devel] [PATCH] apic: Make APIC ID limit error message clearer
Remove the "apic initialization failed" prefix (it conveys no
useful information), replace "invalid" with "too large", and add
an error hint with two possible solutions for the problem.
Before:
$ qemu-system-x86_64 -machine q35 -smp 256
qemu-system-x86_64: apic initialization failed. APIC ID 255 is invalid
After:
$ qemu-system-x86_64 -machine q35 -smp 256 -display none
qemu-system-x86_64: APIC ID 255 is too large
Possible solutions:
* Lowering the number of VCPUs on the -smp option
* Using accel=kvm,kernel-irqchip=on or accel=kvm,kernel-irqchip=split
Signed-off-by: Eduardo Habkost
---
I'm not sure this is the best way to provide usage hints to the
user. Any suggestions?
---
hw/intc/apic.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/hw/intc/apic.c b/hw/intc/apic.c
index 97ffdd820f..f08006334d 100644
--- a/hw/intc/apic.c
+++ b/hw/intc/apic.c
@@ -886,8 +886,11 @@ static void apic_realize(DeviceState *dev, Error **errp)
APICCommonState *s = APIC(dev);
if (s->id >= MAX_APICS) {
-error_setg(errp, "%s initialization failed. APIC ID %d is invalid",
- object_get_typename(OBJECT(dev)), s->id);
+error_setg(errp, "APIC ID %d is too large", s->id);
+error_append_hint(errp,
+"Possible solutions:\n"
+"* Lowering the number of VCPUs on the -smp option\n"
+"* Using accel=kvm,kernel-irqchip=on or
accel=kvm,kernel-irqchip=split\n");
return;
}
--
2.18.0.rc1.1.g3f1ff2140
Re: [Qemu-devel] [PATCH 2/5] vhost-net-user: add stubs for when no virtio-net device is present
On 26/11/18 23:43, Michael S. Tsirkin wrote:
> On Mon, Nov 26, 2018 at 05:45:47PM +0100, Philippe Mathieu-Daudé wrote:
>> On 26/11/18 14:20, Paolo Bonzini wrote:
>>> hw/net/vhost_net.c needs functions that are declared in net/vhost-user.c:
>>> the
>>> vhost-user code is always compiled into QEMU, only the constructor
>>> net_init_vhost_user is unreachable. Also, net/vhost-user.c needs functions
>>> declared in hw/virtio/vhost-stub.c even if no virtio device exists.
>>>
>>> Break this dependency. First, add a minimal version of net/vhost-user.c,
>>> with no functionality and no dependency on vhost code. Second, #ifdef out
>>> the calls back to net/vhost-user.c from hw/net/vhost_net.c.
>>>
>>> Signed-off-by: Paolo Bonzini
>>
>> Reviewed-by: Philippe Mathieu-Daudé
>> Tested-by: Philippe Mathieu-Daudé
>
> whic hosts/targets and how did you test exactly?
This series is a buildsys refactor; after reviewing I building tested
linux (KVM) and win32 (default, --enable-vhost-user,
--enable-vhost-kernel). I did not test on BSD.
I did not test the running vhost code, simply assumed linking is enough.
>
>>> ---
>>> configure | 2 +-
>>> hw/net/vhost_net.c| 4
>>> net/Makefile.objs | 4 +++-
>>> net/net.c | 2 +-
>>> net/vhost-user-stub.c | 23 +++
>>> 5 files changed, 32 insertions(+), 3 deletions(-)
>>> create mode 100644 net/vhost-user-stub.c
>>>
>>> diff --git a/configure b/configure
>>> index 0a3c6a7..cda17ef 100755
>>> --- a/configure
>>> +++ b/configure
>>> @@ -6513,7 +6513,7 @@ if test "$vhost_scsi" = "yes" ; then
>>>echo "CONFIG_VHOST_SCSI=y" >> $config_host_mak
>>> fi
>>> if test "$vhost_net" = "yes" -a "$vhost_user" = "yes"; then
>>> - echo "CONFIG_VHOST_NET_USED=y" >> $config_host_mak
>>> + echo "CONFIG_VHOST_NET_USER=y" >> $config_host_mak
>>> fi
>>> if test "$vhost_crypto" = "yes" ; then
>>>echo "CONFIG_VHOST_CRYPTO=y" >> $config_host_mak
>>> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
>>> index b901306..fe6202a 100644
>>> --- a/hw/net/vhost_net.c
>>> +++ b/hw/net/vhost_net.c
>>> @@ -193,6 +193,7 @@ struct vhost_net *vhost_net_init(VhostNetOptions
>>> *options)
>>> }
>>>
>>> /* Set sane init value. Override when guest acks. */
>>> +#ifdef CONFIG_VHOST_USER
>>> if (net->nc->info->type == NET_CLIENT_DRIVER_VHOST_USER) {
>>> features = vhost_user_get_acked_features(net->nc);
>>> if (~net->dev.features & features) {
>>> @@ -202,6 +203,7 @@ struct vhost_net *vhost_net_init(VhostNetOptions
>>> *options)
>>> goto fail;
>>> }
>>> }
>>> +#endif
>>>
>>> vhost_net_ack_features(net, features);
>>>
>>> @@ -413,10 +415,12 @@ VHostNetState *get_vhost_net(NetClientState *nc)
>>> case NET_CLIENT_DRIVER_TAP:
>>> vhost_net = tap_get_vhost_net(nc);
>>> break;
>>> +#ifdef CONFIG_VHOST_NET_USER
>>> case NET_CLIENT_DRIVER_VHOST_USER:
>>> vhost_net = vhost_user_get_vhost_net(nc);
>>> assert(vhost_net);
>>> break;
>>> +#endif
>>> default:
>>> break;
>>> }
>>> diff --git a/net/Makefile.objs b/net/Makefile.objs
>>> index b2bf88a..df2b409 100644
>>> --- a/net/Makefile.objs
>>> +++ b/net/Makefile.objs
>>> @@ -3,7 +3,9 @@ common-obj-y += socket.o
>>> common-obj-y += dump.o
>>> common-obj-y += eth.o
>>> common-obj-$(CONFIG_L2TPV3) += l2tpv3.o
>>> -common-obj-$(CONFIG_POSIX) += vhost-user.o
>>> +common-obj-$(call land,$(CONFIG_VIRTIO_NET),$(CONFIG_VHOST_NET_USER)) +=
>>> vhost-user.o
>>> +common-obj-$(call land,$(call
>>> lnot,$(CONFIG_VIRTIO_NET)),$(CONFIG_VHOST_NET_USER)) += vhost-user-stub.o
>>> +common-obj-$(CONFIG_ALL) += vhost-user-stub.o
>>> common-obj-$(CONFIG_SLIRP) += slirp.o
>>> common-obj-$(CONFIG_VDE) += vde.o
>>> common-obj-$(CONFIG_NETMAP) += netmap.o
>>> diff --git a/net/net.c b/net/net.c
>>> index 07c194a..95a74ad 100644
>>> --- a/net/net.c
>>> +++ b/net/net.c
>>> @@ -955,7 +955,7 @@ static int (* const
>>> net_client_init_fun[NET_CLIENT_DRIVER__MAX])(
>>> [NET_CLIENT_DRIVER_BRIDGE]= net_init_bridge,
>>> #endif
>>> [NET_CLIENT_DRIVER_HUBPORT] = net_init_hubport,
>>> -#ifdef CONFIG_VHOST_NET_USED
>>> +#ifdef CONFIG_VHOST_NET_USER
>>> [NET_CLIENT_DRIVER_VHOST_USER] = net_init_vhost_user,
>>> #endif
>>> #ifdef CONFIG_L2TPV3
>>> diff --git a/net/vhost-user-stub.c b/net/vhost-user-stub.c
>>> new file mode 100644
>>> index 000..52ab4e1
>>> --- /dev/null
>>> +++ b/net/vhost-user-stub.c
>>> @@ -0,0 +1,23 @@
>>> +/*
>>> + * vhost-user-stub.c
>>> + *
>>> + * Copyright (c) 2018 Red Hat, Inc.
>>> + *
>>> + * This work is licensed under the terms of the GNU GPL, version 2 or
>>> later.
>>> + * See the COPYING file in the top-level directory.
>>> + *
>>> + */
>>> +
>>> +#include "qemu/osdep.h"
>>> +#include "clients.h"
>>> +#include "net/vhost_net.h"
>>> +#include "net/vhost-user.h"
>>> +#include "qemu/error-report.h"
>>> +#include "qapi/error.h"
Re: [Qemu-devel] [PATCH 2/5] vhost-net-user: add stubs for when no virtio-net device is present
On Mon, Nov 26, 2018 at 05:45:47PM +0100, Philippe Mathieu-Daudé wrote:
> On 26/11/18 14:20, Paolo Bonzini wrote:
> > hw/net/vhost_net.c needs functions that are declared in net/vhost-user.c:
> > the
> > vhost-user code is always compiled into QEMU, only the constructor
> > net_init_vhost_user is unreachable. Also, net/vhost-user.c needs functions
> > declared in hw/virtio/vhost-stub.c even if no virtio device exists.
> >
> > Break this dependency. First, add a minimal version of net/vhost-user.c,
> > with no functionality and no dependency on vhost code. Second, #ifdef out
> > the calls back to net/vhost-user.c from hw/net/vhost_net.c.
> >
> > Signed-off-by: Paolo Bonzini
>
> Reviewed-by: Philippe Mathieu-Daudé
> Tested-by: Philippe Mathieu-Daudé
whic hosts/targets and how did you test exactly?
> > ---
> > configure | 2 +-
> > hw/net/vhost_net.c| 4
> > net/Makefile.objs | 4 +++-
> > net/net.c | 2 +-
> > net/vhost-user-stub.c | 23 +++
> > 5 files changed, 32 insertions(+), 3 deletions(-)
> > create mode 100644 net/vhost-user-stub.c
> >
> > diff --git a/configure b/configure
> > index 0a3c6a7..cda17ef 100755
> > --- a/configure
> > +++ b/configure
> > @@ -6513,7 +6513,7 @@ if test "$vhost_scsi" = "yes" ; then
> >echo "CONFIG_VHOST_SCSI=y" >> $config_host_mak
> > fi
> > if test "$vhost_net" = "yes" -a "$vhost_user" = "yes"; then
> > - echo "CONFIG_VHOST_NET_USED=y" >> $config_host_mak
> > + echo "CONFIG_VHOST_NET_USER=y" >> $config_host_mak
> > fi
> > if test "$vhost_crypto" = "yes" ; then
> >echo "CONFIG_VHOST_CRYPTO=y" >> $config_host_mak
> > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > index b901306..fe6202a 100644
> > --- a/hw/net/vhost_net.c
> > +++ b/hw/net/vhost_net.c
> > @@ -193,6 +193,7 @@ struct vhost_net *vhost_net_init(VhostNetOptions
> > *options)
> > }
> >
> > /* Set sane init value. Override when guest acks. */
> > +#ifdef CONFIG_VHOST_USER
> > if (net->nc->info->type == NET_CLIENT_DRIVER_VHOST_USER) {
> > features = vhost_user_get_acked_features(net->nc);
> > if (~net->dev.features & features) {
> > @@ -202,6 +203,7 @@ struct vhost_net *vhost_net_init(VhostNetOptions
> > *options)
> > goto fail;
> > }
> > }
> > +#endif
> >
> > vhost_net_ack_features(net, features);
> >
> > @@ -413,10 +415,12 @@ VHostNetState *get_vhost_net(NetClientState *nc)
> > case NET_CLIENT_DRIVER_TAP:
> > vhost_net = tap_get_vhost_net(nc);
> > break;
> > +#ifdef CONFIG_VHOST_NET_USER
> > case NET_CLIENT_DRIVER_VHOST_USER:
> > vhost_net = vhost_user_get_vhost_net(nc);
> > assert(vhost_net);
> > break;
> > +#endif
> > default:
> > break;
> > }
> > diff --git a/net/Makefile.objs b/net/Makefile.objs
> > index b2bf88a..df2b409 100644
> > --- a/net/Makefile.objs
> > +++ b/net/Makefile.objs
> > @@ -3,7 +3,9 @@ common-obj-y += socket.o
> > common-obj-y += dump.o
> > common-obj-y += eth.o
> > common-obj-$(CONFIG_L2TPV3) += l2tpv3.o
> > -common-obj-$(CONFIG_POSIX) += vhost-user.o
> > +common-obj-$(call land,$(CONFIG_VIRTIO_NET),$(CONFIG_VHOST_NET_USER)) +=
> > vhost-user.o
> > +common-obj-$(call land,$(call
> > lnot,$(CONFIG_VIRTIO_NET)),$(CONFIG_VHOST_NET_USER)) += vhost-user-stub.o
> > +common-obj-$(CONFIG_ALL) += vhost-user-stub.o
> > common-obj-$(CONFIG_SLIRP) += slirp.o
> > common-obj-$(CONFIG_VDE) += vde.o
> > common-obj-$(CONFIG_NETMAP) += netmap.o
> > diff --git a/net/net.c b/net/net.c
> > index 07c194a..95a74ad 100644
> > --- a/net/net.c
> > +++ b/net/net.c
> > @@ -955,7 +955,7 @@ static int (* const
> > net_client_init_fun[NET_CLIENT_DRIVER__MAX])(
> > [NET_CLIENT_DRIVER_BRIDGE]= net_init_bridge,
> > #endif
> > [NET_CLIENT_DRIVER_HUBPORT] = net_init_hubport,
> > -#ifdef CONFIG_VHOST_NET_USED
> > +#ifdef CONFIG_VHOST_NET_USER
> > [NET_CLIENT_DRIVER_VHOST_USER] = net_init_vhost_user,
> > #endif
> > #ifdef CONFIG_L2TPV3
> > diff --git a/net/vhost-user-stub.c b/net/vhost-user-stub.c
> > new file mode 100644
> > index 000..52ab4e1
> > --- /dev/null
> > +++ b/net/vhost-user-stub.c
> > @@ -0,0 +1,23 @@
> > +/*
> > + * vhost-user-stub.c
> > + *
> > + * Copyright (c) 2018 Red Hat, Inc.
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or
> > later.
> > + * See the COPYING file in the top-level directory.
> > + *
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "clients.h"
> > +#include "net/vhost_net.h"
> > +#include "net/vhost-user.h"
> > +#include "qemu/error-report.h"
> > +#include "qapi/error.h"
> > +
> > +int net_init_vhost_user(const Netdev *netdev, const char *name,
> > +NetClientState *peer, Error **errp)
> > +{
> > +error_setg(errp, "vhost-user requires frontend driver virtio-net-*");
> > +return -1;
> > +}
> >
Re: [Qemu-devel] [PATCH v3 16/16] i2c:smbus_eeprom: Add a reset function to smbus_eeprom
On 11/26/18 2:42 PM, Philippe Mathieu-Daudé wrote:
Hi Corey,
On 26/11/18 21:04, miny...@acm.org wrote:
From: Corey Minyard
Reset the contents to init data and reset the offset on a machine
reset.
Signed-off-by: Corey Minyard
---
hw/i2c/smbus_eeprom.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
index a0dcadbd60..de3a492df4 100644
--- a/hw/i2c/smbus_eeprom.c
+++ b/hw/i2c/smbus_eeprom.c
@@ -107,7 +107,7 @@ static const VMStateDescription vmstate_smbus_eeprom = {
}
};
-static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
+static void smbus_eeprom_reset(DeviceState *dev)
{
SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev);
'git diff -U4' also shows this line:
memcpy(eeprom->data, eeprom->init_data, SMBUS_EEPROM_SIZE);
I don't think this is correct.
One test I'd like to have is a machine booting, updating the EPROM then
rebooting calling hw reset() to use the new values (BIOS use this).
With this patch this won't work, you'll restore the EPROM content on
each machine reset.
I'd move the memcpy() call to the realize() function.
What do you think?
There was some debate on this in the earlier patch set. The general
principle
is that a reset is the same as starting up qemu from scratch, so I added
this
code based on that principle. But I'm not really sure.
eeprom->offset = 0;
This is correct, the offset reset belongs to the reset() function.
Actually, on a real system, a hardware reset will generally not affect the
eeprom current offset register. So if we don't take the above code, then
IMHO this is wrong, too.
-corey
Regards,
Phil.
}
+static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
+{
+smbus_eeprom_reset(dev);
+}
+
static Property smbus_eeprom_properties[] = {
DEFINE_PROP_PTR("data", SMBusEEPROMDevice, init_data),
DEFINE_PROP_END_OF_LIST(),
@@ -126,6 +131,7 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass,
void *data)
SMBusDeviceClass *sc = SMBUS_DEVICE_CLASS(klass);
dc->realize = smbus_eeprom_realize;
+dc->reset = smbus_eeprom_reset;
sc->receive_byte = eeprom_receive_byte;
sc->write_data = eeprom_write_data;
dc->props = smbus_eeprom_properties;
Re: [Qemu-devel] [PATCH 0/5] vhost: enable for all targets
On Mon, Nov 26, 2018 at 02:20:38PM +0100, Paolo Bonzini wrote: > vhost does not have to be supported only if KVM is present, in fact > vhost-user does not even need any kind of kernel support. This series > changes this. The rationale is that, when vhost-user-test.c will be > converted to qgraph, it will be able to test vhost-user support > for virtio-mmio backend even on x86. The reason for limiting it to KVM was very simple: it has the same set of problems with ordering as mttcg. So I guess it's fine but I think we must then limit it to when tcg emits fence instructions. Otherwise we'll get subtle race conditions. > Paolo Bonzini (5): > vhost-net: move stubs to a separate file > vhost-net-user: add stubs for when no virtio-net device is present > vhost: restrict Linux dependency to kernel vhost > vhost-net: compile it on all targets that have virtio-net. > vhost-net: revamp configure logic > > backends/Makefile.objs | 5 +-- > configure | 101 > - > default-configs/virtio.mak | 4 +- > hw/net/Makefile.objs | 4 +- > hw/net/vhost_net-stub.c| 95 ++ > hw/net/vhost_net.c | 78 ++ > hw/virtio/Makefile.objs| 5 ++- > hw/virtio/vhost-backend.c | 11 - > include/exec/poison.h | 1 - > net/Makefile.objs | 4 +- > net/net.c | 2 +- > net/vhost-user-stub.c | 23 +++ > tests/Makefile.include | 5 +-- > 13 files changed, 209 insertions(+), 129 deletions(-) > create mode 100644 hw/net/vhost_net-stub.c > create mode 100644 net/vhost-user-stub.c > > -- > 1.8.3.1
Re: [Qemu-devel] [RFC PATCH v2 3/3] target/ppc: support single stepping with KVM HV
On Wed, Nov 21, 2018 at 04:13:47PM -0200, Fabiano Rosas wrote:
> The hardware singlestep mechanism in POWER works via a Trace Interrupt
> (0xd00) that happens after any instruction executes, whenever MSR_SE =
> 1 (PowerISA Section 6.5.15 - Trace Interrupt).
>
> However, with kvm_hv, the Trace Interrupt happens inside the guest and
> KVM has no visibility of it. Therefore, when the gdbstub uses the
> KVM_SET_GUEST_DEBUG ioctl to enable singlestep, KVM simply ignores it.
>
> This patch takes advantage of the Trace Interrupt to perform the step
> inside the guest, but uses a breakpoint at the Trace Interrupt handler
> to return control to KVM. The exit is treated by KVM as a regular
> breakpoint and it returns to the host (and QEMU eventually).
>
> Before signalling GDB, QEMU sets the Next Instruction Pointer to the
> instruction following the one being stepped, effectively skipping the
> interrupt handler execution and hiding the trace interrupt breakpoint
> from GDB.
>
> This approach works with both of GDB's 'scheduler-locking' options
> (off, step).
>
> Signed-off-by: Fabiano Rosas
> ---
> target/ppc/kvm.c | 61 +++-
> 1 file changed, 60 insertions(+), 1 deletion(-)
>
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 9d0b4f1f3f..93c20099e6 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -94,6 +94,7 @@ static int cap_ppc_safe_indirect_branch;
> static int cap_ppc_nested_kvm_hv;
>
> static uint32_t debug_inst_opcode;
> +static target_ulong trace_handler_addr;
>
> /* XXX We have a race condition where we actually have a level triggered
> * interrupt, but the infrastructure can't expose that yet, so the guest
> @@ -509,6 +510,9 @@ int kvm_arch_init_vcpu(CPUState *cs)
> kvm_get_one_reg(cs, KVM_REG_PPC_DEBUG_INST, _inst_opcode);
> kvmppc_hw_debug_points_init(cenv);
>
> +trace_handler_addr = (cenv->excp_vectors[POWERPC_EXCP_TRACE] |
> + AIL_C000___4000_OFFSET);
Effectively caching this as a global variable is pretty horrible. A
function to make the above calculation when you need it would be
better.
Also, won't the calculation above be wrong if the guest is using the
low AIL value?
> +
> return ret;
> }
>
> @@ -1553,6 +1557,24 @@ void kvm_arch_remove_all_hw_breakpoints(void)
>
> void kvm_arch_set_singlestep(CPUState *cs, int enabled)
> {
> +PowerPCCPU *cpu = POWERPC_CPU(cs);
> +CPUPPCState *env = >env;
> +
> +if (kvmppc_is_pr(kvm_state)) {
Explicitly testing for KVM PR is generally wrong (except as a
fallback). Instead you should add a capability flag to KVM which you
use to test the feature's presence in qemu. That capability can then
be set in HV, but not PR.
> +return;
> +}
> +
> +if (enabled) {
> +/* MSR_SE = 1 will cause a Trace Interrupt in the guest after
> + * the next instruction executes. */
> +env->msr |= (1ULL << MSR_SE);
> +
> +/* We set a breakpoint at the interrupt handler address so
> + * that the singlestep will be seen by KVM (this is treated by
> + * KVM like an ordinary breakpoint) and control is returned to
> + * QEMU. */
> +kvm_insert_breakpoint(cs, trace_handler_addr, 4, GDB_BREAKPOINT_SW);
> +}
> }
>
> void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
> @@ -1594,6 +1616,43 @@ void kvm_arch_update_guest_debug(CPUState *cs, struct
> kvm_guest_debug *dbg)
> }
> }
>
> +static int kvm_handle_singlestep(CPUState *cs,
> + struct kvm_debug_exit_arch *arch_info)
> +{
> +PowerPCCPU *cpu = POWERPC_CPU(cs);
> +CPUPPCState *env = >env;
> +target_ulong msr = env->msr;
> +uint32_t insn;
> +int ret = 1;
You never set this to anything other than 1...
> +int reg;
> +
> +if (kvmppc_is_pr(kvm_state)) {
> +return ret;
> +}
> +
> +if (arch_info->address == trace_handler_addr) {
> +cpu_synchronize_state(cs);
> +kvm_remove_breakpoint(cs, trace_handler_addr, 4, GDB_BREAKPOINT_SW);
> +
> +cpu_memory_rw_debug(cs, env->spr[SPR_SRR0] - 4, (uint8_t *),
> +sizeof(insn), 0);
> +
> +/* If the last instruction was a mfmsr, make sure that the
> + * MSR_SE bit is not set to avoid the guest kernel knowing
> + * that it is being single-stepped */
> +if (extract32(insn, 26, 6) == 31 && extract32(insn, 1, 10) == 83) {
> +reg = extract32(insn, 21, 5);
> +env->gpr[reg] &= ~(1ULL << MSR_SE);
> +}
Hm. What happens if both qemu and the guest itself attempt to single
step at the same time? How do you distinguish between the cases?
> +
> +env->nip = env->spr[SPR_SRR0];
> +env->msr = msr &= ~(1ULL << MSR_SE);
You clear SE after tracing one instruction; is that intentional?
> +cpu_synchronize_state(cs);
You're effectively
[Qemu-devel] [Bug 1793859] Re: GTK display and mouse input area scaling fails when using vfio-pci device
Chen, have you seen the following bug report? https://bugs.launchpad.net/qemu/+bug/1592351 See my comment there, with recent git build of 3.1.0 I don't have a pointer offset issue. -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1793859 Title: GTK display and mouse input area scaling fails when using vfio-pci device Status in QEMU: New Bug description: Version qemu 3.0.0-1 running on Arch. Found on Windows 8.1 and Windows 10 VM's when using Intel gvt-g device. While in fullscreen the GTK display is scaled larger than the x11 screen or virtual machine resolution. Without choosing zoom-to-fit portions of the VM display are not shown on x11 screen regardless of the VM resolution. When zoom-to-fit is done the mouse that's shown on screen and actual input are off sync. The mouse can wander off screen when going left and down. This message is shown when changing from gxl-vga to vfio-pci in view menu. (qemu-system-x86_64:6472): Gtk-WARNING **: 09:50:06.663: drawing failure for widget 'GtkDrawingArea': NULL pointer (qemu-system-x86_64:6472): Gtk-WARNING **: 09:50:06.664: drawing failure for widget 'GtkNotebook': NULL pointer (qemu-system-x86_64:6472): Gtk-WARNING **: 09:50:06.664: drawing failure for widget 'GtkBox': NULL pointer (qemu-system-x86_64:6472): Gtk-WARNING **: 09:50:06.664: drawing failure for widget 'GtkWindow': NULL pointer To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1793859/+subscriptions
[Qemu-devel] [Bug 1336794] Re: 9pfs does not honor open file handles on unlinked files
hi,
i am probably trying to ride a dead horse here, but is there any chance this
patch will make its way into master?
thanks,
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] [RFC 19/48] translate-all: notify plugin code of tb_flush
On Mon, Nov 26, 2018 at 11:02:24 +, Alex Bennée wrote: > Emilio G. Cota writes: > > On Fri, Nov 23, 2018 at 17:00:59 +, Alex Bennée wrote: > >> What is the purpose of letting the plugin know a flush has occurred? > > > > Plugins might allocate per-TB data that then they get passed each > > time the TB is executed (via the *userdata pointer). For example, > > in a simulator we'd allocate a per-TB struct that describes the > > guest instructions, after having disassembled them at translate time. > > > > It is therefore useful for plugins to know when all TB's have been > > flushed, so that they can then free that per-TB data. > > Would they need a generation count propagated here or would it just be > flush anything translation related at this point? The callback is guaranteed to be called in an exclusive context, so plugins can just rely on that; since no code execution can happen at that time, there's no need to pass further info, since the plugins can free that data right from the callback. > >> It shouldn't have any knowledge of the details of liveliness of the > >> translated code and if it still exits or not. If all it wants to do is > >> look at the counts then I think we can provide a simpler less abuse-able > >> way to do this. > > > > I'm confused. What does "look at the counts" mean here? > > > > To reiterate, plugins should have a way to know when a TB doesn't > > exist any longer, so that they can reclaim memory. > > OK that makes sense. Could you expand the commit message just to explain > the use case? > > Reviewed-by: Alex Bennée Updated the commit message with: translate-all: notify plugin code of tb_flush Plugins might allocate per-TB data that then they get passed each time a TB is executed (via the *userdata pointer). Notify plugin code every time a code cache flush occurs, so that plugins can then reclaim the memory of the per-TB data. Thanks, Emilio
Re: [Qemu-devel] [PATCH for-3.2 v7 0/6] Connect a PCIe host and graphics support to RISC-V
On Mon, Nov 26, 2018 at 11:34:58AM -0800, Palmer Dabbelt wrote: > On Thu, 22 Nov 2018 02:59:18 PST (-0800), abolo...@redhat.com wrote: > > On Wed, 2018-11-21 at 17:02 +, Alistair Francis wrote: > >> V7: > >> - Fix the GPEX memory mapping thanks to Bin Meng > >> - Fix the interrupt mapping thanks to Logan Gunthorpe > >> > > [...] > >> > >> Alistair Francis (6): > >> hw/riscv/virt: Increase the number of interrupts > >> hw/riscv/virt: Adjust memory layout spacing > >> hw/riscv/virt: Connect the gpex PCIe > >> riscv: Enable VGA and PCIE_VGA > >> hw/riscv/sifive_u: Connect the Xilinx PCIe > >> hw/riscv/virt: Connect a VirtIO net PCIe device > > > > Using QEMU master + these patches, libvirt master + my own patches, > > a disk image from [1] and the bbl from [2], I was able to run a pure > > PCI RISC-V guest, including connecting to it through ssh and > > performing a system update using dnf; based on these results, the > > series gets a big old > > > > Tested-by: Andrea Bolognani > > > > > > [1] https://fedora-riscv.tranquillity.se/koji/ > > Unfortunately the site is broken at the moment :( > > FWIW, I get my images from here > > https://fedorapeople.org/groups/risc-v/disk-images/ > > which is linked to from the Fedora wiki > > https://fedoraproject.org/wiki/Architectures/RISC-V/Disk_images > > the stage4 there is quite old (April, 2018), is yours newer? I'm having some > problems updating my disk image and since I'm not a Fedora user I have no > idea > what's going on. > > > [2] https://fedorapeople.org/groups/risc-v/disk-images/ > > PCI support is included in the bbl now! \o/ > > Great! I've yet to figure out how to get all the PCI stuff working, but I > was > trying to go with a virgl-based GPU which I suspect is broken for other > reasons. What sort of devices did you attach? FWIW, here is my list of tests. Building riscv:virt:defconfig:initrd ... running ... passed Building riscv:virt:defconfig:virtio-blk:rootfs ... running passed Building riscv:virt:defconfig:virtio:rootfs ... running passed Building riscv:virt:defconfig:virtio-pci:rootfs ... running passed Building riscv:virt:defconfig:mmc:rootfs ... running passed Building riscv:virt:defconfig:nvme:rootfs ... running passed Building riscv:virt:defconfig:usb-ohci:rootfs ... running passed Building riscv:virt:defconfig:usb-ehci:rootfs ... running passed Building riscv:virt:defconfig:usb-xhci:rootfs ... running . passed Building riscv:virt:defconfig:usb-uas-ehci:rootfs ... running . passed Building riscv:virt:defconfig:usb-uas-xhci:rootfs ... running . passed Building riscv:virt:defconfig:scsi[53C810]:rootfs ... running ... passed Building riscv:virt:defconfig:scsi[53C895A]:rootfs ... running . passed Building riscv:virt:defconfig:scsi[AM53C974]:rootfs ... skipped Building riscv:virt:defconfig:scsi[DC395]:rootfs ... skipped Building riscv:virt:defconfig:scsi[MEGASAS]:rootfs ... running . passed Building riscv:virt:defconfig:scsi[MEGASAS2]:rootfs ... running passed Building riscv:virt:defconfig:scsi[FUSION]:rootfs ... running ... passed Building riscv:virt:defconfig:scsi[virtio]:rootfs ... running ... passed Building riscv:virt:defconfig:scsi[virtio-pci]:rootfs ... running passed The skipped tests work as well, but the emulation is flaky and fails once in a while. This is obviously with usb enabled. Guenter
Re: [Qemu-devel] [PATCH] target: hax: fix a typo
On 11/26/18 1:25 PM, Philippe Mathieu-Daudé wrote: >>> - * 1. The hax_tunnel is also destroied when vcpu destroy >>> + * 1. The hax_tunnel is also destroyed when vcpu destroy > I'm not native english speaker. Isn't it: > > 'The hax_tunnel is also destroyed at vcpu destroy' > > or > > 'The hax_tunnel is also destroyed when vcpu is destroyed'? This second option is best, fwiw. r~
Re: [Qemu-devel] [PATCH] target: hax: fix a typo
Hi Li, Alex. On 26/11/18 12:51, Alex Bennée wrote: > > Li Qiang writes: > >> Cc: qemu-triv...@nongnu.org >> >> Signed-off-by: Li Qiang > > Reviewed-by: Alex Bennée > >> --- >> target/i386/hax-all.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/target/i386/hax-all.c b/target/i386/hax-all.c >> index 502ce6f0af..464744a406 100644 >> --- a/target/i386/hax-all.c >> +++ b/target/i386/hax-all.c >> @@ -205,7 +205,7 @@ int hax_vcpu_destroy(CPUState *cpu) >> } >> >> /* >> - * 1. The hax_tunnel is also destroied when vcpu destroy >> + * 1. The hax_tunnel is also destroyed when vcpu destroy I'm not native english speaker. Isn't it: 'The hax_tunnel is also destroyed at vcpu destroy' or 'The hax_tunnel is also destroyed when vcpu is destroyed'? >> * 2. close fd will cause hax module vcpu be cleaned >> */ >> hax_close_fd(vcpu->fd); > > > -- > Alex Bennée >
Re: [Qemu-devel] [PATCH] hw/hyperv: fix NULL dereference with pure-kvm SynIC
On Mon, Nov 26, 2018 at 06:13:49PM +0100, Paolo Bonzini wrote:
> On 26/11/18 16:28, Roman Kagan wrote:
> > When started in compat configuration of SynIC, e.g.
> >
> > qemu-system-x86_64 -machine pc-i440fx-2.10,accel=kvm \
> > -cpu host,-vmx,hv-relaxed,hv_spinlocks=0x1fff,hv-vpindex,hv-synic
> >
> > or explicitly
> >
> > qemu-system-x86_64 -enable-kvm -cpu host,hv-synic,x-hv-synic-kvm-only=on
> >
> > QEMU crashes in hyperv_synic_reset() trying to access the non-present
> > qobject for SynIC.
> >
> > Add the missing check for NULL.
> >
> > Reported-by: Vitaly Kuznetsov
> > Reported-by: Igor Mammedov
> > Fixes: 9b4cf107b09d18ac30f46fd1c4de8585ccba030c
> > Fixes: 4a93722f9c279184e95b1e1ad775c01deec05065
> > Signed-off-by: Roman Kagan
> > ---
> > hw/hyperv/hyperv.c | 6 +-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/hyperv/hyperv.c b/hw/hyperv/hyperv.c
> > index a28e7249d8..8758635227 100644
> > --- a/hw/hyperv/hyperv.c
> > +++ b/hw/hyperv/hyperv.c
> > @@ -136,7 +136,11 @@ void hyperv_synic_add(CPUState *cs)
> >
> > void hyperv_synic_reset(CPUState *cs)
> > {
> > -device_reset(DEVICE(get_synic(cs)));
> > +SynICState *synic = get_synic(cs);
> > +
> > +if (synic) {
> > +device_reset(DEVICE(synic));
> > +}
> > }
> >
> > static const TypeInfo synic_type_info = {
> >
>
> Queued, thanks.
Oops, I had queued it earlier today and just submitted a pull
request.
--
Eduardo
[Qemu-devel] [PULL 2/2] hw/hyperv: fix NULL dereference with pure-kvm SynIC
From: Roman Kagan
When started in compat configuration of SynIC, e.g.
qemu-system-x86_64 -machine pc-i440fx-2.10,accel=kvm \
-cpu host,-vmx,hv-relaxed,hv_spinlocks=0x1fff,hv-vpindex,hv-synic
or explicitly
qemu-system-x86_64 -enable-kvm -cpu host,hv-synic,x-hv-synic-kvm-only=on
QEMU crashes in hyperv_synic_reset() trying to access the non-present
qobject for SynIC.
Add the missing check for NULL.
Reported-by: Vitaly Kuznetsov
Reported-by: Igor Mammedov
Fixes: 9b4cf107b09d18ac30f46fd1c4de8585ccba030c
Fixes: 4a93722f9c279184e95b1e1ad775c01deec05065
Signed-off-by: Roman Kagan
Message-Id: <20181126152836.25379-1-rka...@virtuozzo.com>
Reviewed-by: Philippe Mathieu-Daudé
Tested-by: Philippe Mathieu-Daudé
Signed-off-by: Eduardo Habkost
---
hw/hyperv/hyperv.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/hw/hyperv/hyperv.c b/hw/hyperv/hyperv.c
index a28e7249d8..8758635227 100644
--- a/hw/hyperv/hyperv.c
+++ b/hw/hyperv/hyperv.c
@@ -136,7 +136,11 @@ void hyperv_synic_add(CPUState *cs)
void hyperv_synic_reset(CPUState *cs)
{
-device_reset(DEVICE(get_synic(cs)));
+SynICState *synic = get_synic(cs);
+
+if (synic) {
+device_reset(DEVICE(synic));
+}
}
static const TypeInfo synic_type_info = {
--
2.18.0.rc1.1.g3f1ff2140
[Qemu-devel] [PULL 1/2] kvm: Use KVM_GET_MSR_INDEX_LIST for MSR_IA32_ARCH_CAPABILITIES support
From: Bandan Das
When writing to guest's MSR_IA32_ARCH_CAPABILITIES, check whether it's
supported in the guest using the KVM_GET_MSR_INDEX_LIST ioctl.
Fixes: d86f963694df27f11b3681ffd225c9362de1b634
Suggested-by: Eduardo Habkost
Tested-by: baldu...@units.it
Signed-off-by: Bandan Das
Message-Id:
Signed-off-by: Eduardo Habkost
---
target/i386/kvm.c | 15 +++
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index f524e7d929..3d6739a2b2 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -95,6 +95,7 @@ static bool has_msr_xss;
static bool has_msr_spec_ctrl;
static bool has_msr_virt_ssbd;
static bool has_msr_smi_count;
+static bool has_msr_arch_capabs;
static uint32_t has_architectural_pmu_version;
static uint32_t num_architectural_pmu_gp_counters;
@@ -1481,6 +1482,9 @@ static int kvm_get_supported_msrs(KVMState *s)
case MSR_VIRT_SSBD:
has_msr_virt_ssbd = true;
break;
+case MSR_IA32_ARCH_CAPABILITIES:
+has_msr_arch_capabs = true;
+break;
}
}
}
@@ -2002,14 +2006,9 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
#endif
/* If host supports feature MSR, write down. */
-if (kvm_feature_msrs) {
-int i;
-for (i = 0; i < kvm_feature_msrs->nmsrs; i++)
-if (kvm_feature_msrs->indices[i] == MSR_IA32_ARCH_CAPABILITIES) {
-kvm_msr_entry_add(cpu, MSR_IA32_ARCH_CAPABILITIES,
- env->features[FEAT_ARCH_CAPABILITIES]);
-break;
-}
+if (has_msr_arch_capabs) {
+kvm_msr_entry_add(cpu, MSR_IA32_ARCH_CAPABILITIES,
+ env->features[FEAT_ARCH_CAPABILITIES]);
}
/*
--
2.18.0.rc1.1.g3f1ff2140
[Qemu-devel] [PULL 0/2] x86 fixes for -rc3
The following changes since commit d522fba24478474911b0e6e488b6d1dcf1af54f8: Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-20181126' into staging (2018-11-26 13:58:46 +) are available in the Git repository at: git://github.com/ehabkost/qemu.git tags/x86-for-3.1-pull-request for you to fetch changes up to 30a759b61a9247378a9cb84fbe4e437ae66e0461: hw/hyperv: fix NULL dereference with pure-kvm SynIC (2018-11-26 14:14:38 -0200) x86 fixes for -rc3 * Fix SynIC crash * Fix x86 crash on MSR code on AMD hosts Bandan Das (1): kvm: Use KVM_GET_MSR_INDEX_LIST for MSR_IA32_ARCH_CAPABILITIES support Roman Kagan (1): hw/hyperv: fix NULL dereference with pure-kvm SynIC hw/hyperv/hyperv.c | 6 +- target/i386/kvm.c | 15 +++ 2 files changed, 12 insertions(+), 9 deletions(-) -- 2.18.0.rc1.1.g3f1ff2140
Re: [Qemu-devel] [PATCH for-3.2 01/11] vhost-user: define conventions for vhost-user backends
On 11/26/18 6:42 AM, Marc-André Lureau wrote: As discussed during "[PATCH v4 00/29] vhost-user for input & GPU" review, let's define a common set of backend conventions to help with management layer implementation, and interoperability. Signed-off-by: Marc-André Lureau Reviewed-by: Daniel P. Berrangé --- MAINTAINERS | 1 + docs/interop/vhost-user.json | 219 +++ docs/interop/vhost-user.txt | 101 +++- 3 files changed, 319 insertions(+), 2 deletions(-) create mode 100644 docs/interop/vhost-user.json +++ b/docs/interop/vhost-user.json +# @VHostUserBackendType: +# +# List the various vhost user backend types. +# +# @net: virtio net +# @block: virtio block +# @console: virtio console +# @rng: virtio rng +# @balloon: virtio balloon +# @rpmsg: virtio remote processor messaging +# @scsi: virtio scsi +# @9p: 9p virtio console +# @rproc-serial: virtio remoteproc serial link +# @caif: virtio caif +# @gpu: virtio gpu +# @input: virtio input +# @vsock: virtio vsock transport +# @crypto: virtio crypto +# +# Since: 3.2 The next release will be 4.0, not 3.2. (We'll probably have to do a global search-and-replace in January, as you're not the only one proposing a patch with this version number). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH v5 10/24] qapi: Define new QMP message for pvrdma
On 11/26/18 4:01 AM, Markus Armbruster wrote: Yuval Shaia writes: pvrdma requires that the same GID attached to it will be attached to the backend device in the host. A new QMP messages is defined so pvrdma device can broadcast any change made to its GID table. This event is captured by libvirt which in turn will update the GID table in the backend device. Signed-off-by: Yuval Shaia Reviewed-by: Marcel Apfelbaum --- +++ b/qapi/rdma.json @@ -0,0 +1,38 @@ +# -*- Mode: Python -*- +# + +## +# = RDMA device +## + +## +# @RDMA_GID_STATUS_CHANGED: +# +# Emitted when guest driver adds/deletes GID to/from device +# +# @netdev: RoCE Network Device name - char * +# +# @gid-status: Add or delete indication - bool +# +# @subnet-prefix: Subnet Prefix - uint64 +# +# @interface-id : Interface ID - uint64 +# +# Since: 3.2 The next release will be 4.0, not 3.2 (we'll probably have to do a global search-and-replace in January to catch things that have slipped in, as your patch is not alone in that). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH v3 16/16] i2c:smbus_eeprom: Add a reset function to smbus_eeprom
Hi Corey,
On 26/11/18 21:04, miny...@acm.org wrote:
> From: Corey Minyard
>
> Reset the contents to init data and reset the offset on a machine
> reset.
>
> Signed-off-by: Corey Minyard
> ---
> hw/i2c/smbus_eeprom.c | 8 +++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
> index a0dcadbd60..de3a492df4 100644
> --- a/hw/i2c/smbus_eeprom.c
> +++ b/hw/i2c/smbus_eeprom.c
> @@ -107,7 +107,7 @@ static const VMStateDescription vmstate_smbus_eeprom = {
> }
> };
>
> -static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
> +static void smbus_eeprom_reset(DeviceState *dev)
> {
> SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev);
>
'git diff -U4' also shows this line:
memcpy(eeprom->data, eeprom->init_data, SMBUS_EEPROM_SIZE);
I don't think this is correct.
One test I'd like to have is a machine booting, updating the EPROM then
rebooting calling hw reset() to use the new values (BIOS use this).
With this patch this won't work, you'll restore the EPROM content on
each machine reset.
I'd move the memcpy() call to the realize() function.
What do you think?
> eeprom->offset = 0;
This is correct, the offset reset belongs to the reset() function.
Regards,
Phil.
> }
>
> +static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
> +{
> +smbus_eeprom_reset(dev);
> +}
> +
> static Property smbus_eeprom_properties[] = {
> DEFINE_PROP_PTR("data", SMBusEEPROMDevice, init_data),
> DEFINE_PROP_END_OF_LIST(),
> @@ -126,6 +131,7 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass,
> void *data)
> SMBusDeviceClass *sc = SMBUS_DEVICE_CLASS(klass);
>
> dc->realize = smbus_eeprom_realize;
> +dc->reset = smbus_eeprom_reset;
> sc->receive_byte = eeprom_receive_byte;
> sc->write_data = eeprom_write_data;
> dc->props = smbus_eeprom_properties;
>
Re: [Qemu-devel] [PATCH v3 12/16] i2c:smbus_eeprom: Add normal type name and cast to smbus_eeprom.c
On 26/11/18 21:04, miny...@acm.org wrote:
> From: Corey Minyard
>
> Create a type name and a cast macro and use those through the
> code.
>
> Signed-off-by: Corey Minyard
> Reviewed-by: Peter Maydell
Reviewed-by: Philippe Mathieu-Daudé
> ---
> hw/i2c/smbus_eeprom.c | 15 ++-
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
> index 4d25222e23..8d4eed129f 100644
> --- a/hw/i2c/smbus_eeprom.c
> +++ b/hw/i2c/smbus_eeprom.c
> @@ -30,6 +30,11 @@
>
> //#define DEBUG
>
> +#define TYPE_SMBUS_EEPROM "smbus-eeprom"
> +
> +#define SMBUS_EEPROM(obj) \
> +OBJECT_CHECK(SMBusEEPROMDevice, (obj), TYPE_SMBUS_EEPROM)
> +
> typedef struct SMBusEEPROMDevice {
> SMBusDevice smbusdev;
> void *data;
> @@ -38,7 +43,7 @@ typedef struct SMBusEEPROMDevice {
>
> static uint8_t eeprom_receive_byte(SMBusDevice *dev)
> {
> -SMBusEEPROMDevice *eeprom = (SMBusEEPROMDevice *) dev;
> +SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev);
> uint8_t *data = eeprom->data;
> uint8_t val = data[eeprom->offset++];
>
> @@ -51,7 +56,7 @@ static uint8_t eeprom_receive_byte(SMBusDevice *dev)
>
> static int eeprom_write_data(SMBusDevice *dev, uint8_t *buf, uint8_t len)
> {
> -SMBusEEPROMDevice *eeprom = (SMBusEEPROMDevice *) dev;
> +SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev);
> uint8_t *data = eeprom->data;
>
> #ifdef DEBUG
> @@ -73,7 +78,7 @@ static int eeprom_write_data(SMBusDevice *dev, uint8_t
> *buf, uint8_t len)
>
> static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
> {
> -SMBusEEPROMDevice *eeprom = (SMBusEEPROMDevice *)dev;
> +SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev);
>
> eeprom->offset = 0;
> }
> @@ -97,7 +102,7 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass,
> void *data)
> }
>
> static const TypeInfo smbus_eeprom_info = {
> -.name = "smbus-eeprom",
> +.name = TYPE_SMBUS_EEPROM,
> .parent= TYPE_SMBUS_DEVICE,
> .instance_size = sizeof(SMBusEEPROMDevice),
> .class_init= smbus_eeprom_class_initfn,
> @@ -114,7 +119,7 @@ void smbus_eeprom_init_one(I2CBus *smbus, uint8_t
> address, uint8_t *eeprom_buf)
> {
> DeviceState *dev;
>
> -dev = qdev_create((BusState *) smbus, "smbus-eeprom");
> +dev = qdev_create((BusState *) smbus, TYPE_SMBUS_EEPROM);
> qdev_prop_set_uint8(dev, "address", address);
> qdev_prop_set_ptr(dev, "data", eeprom_buf);
> qdev_init_nofail(dev);
>
Re: [Qemu-devel] [PATCH] [PATCH] Allow AArch64 processors to boot from a kernel placed over 4GB.
On Mon, 26 Nov 2018 at 19:15, Perez Blanco, Ricardo (Nokia - BE/Antwerp) wrote: > > Some machine based on AArch64 can have its main memory over 4GBs. With > the current path, these machines can support "-kernel" in qemu > > Signed-off-by: Ricardo Perez Blanco Does this fix an issue with one of the current boards we have in QEMU? I think they all have RAM below 4GB... thanks -- PMM
[Qemu-devel] [PATCH v3 16/16] i2c:smbus_eeprom: Add a reset function to smbus_eeprom
From: Corey Minyard
Reset the contents to init data and reset the offset on a machine
reset.
Signed-off-by: Corey Minyard
---
hw/i2c/smbus_eeprom.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
index a0dcadbd60..de3a492df4 100644
--- a/hw/i2c/smbus_eeprom.c
+++ b/hw/i2c/smbus_eeprom.c
@@ -107,7 +107,7 @@ static const VMStateDescription vmstate_smbus_eeprom = {
}
};
-static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
+static void smbus_eeprom_reset(DeviceState *dev)
{
SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev);
@@ -115,6 +115,11 @@ static void smbus_eeprom_realize(DeviceState *dev, Error
**errp)
eeprom->offset = 0;
}
+static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
+{
+smbus_eeprom_reset(dev);
+}
+
static Property smbus_eeprom_properties[] = {
DEFINE_PROP_PTR("data", SMBusEEPROMDevice, init_data),
DEFINE_PROP_END_OF_LIST(),
@@ -126,6 +131,7 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass,
void *data)
SMBusDeviceClass *sc = SMBUS_DEVICE_CLASS(klass);
dc->realize = smbus_eeprom_realize;
+dc->reset = smbus_eeprom_reset;
sc->receive_byte = eeprom_receive_byte;
sc->write_data = eeprom_write_data;
dc->props = smbus_eeprom_properties;
--
2.17.1
Re: [Qemu-devel] [PATCH 2/5] The discard flag for block stream operation
On 11/22/18 12:48 PM, Andrey Shinkevich wrote: Adding a parameter to QMP block-stream command to allow discarding blocks in the backing chain while blocks are being copied to the active layer. Signed-off-by: Andrey Shinkevich --- block/stream.c| 3 +-- blockdev.c| 8 +++- hmp-commands.hx | 4 ++-- hmp.c | 4 +++- include/block/block_int.h | 2 +- qapi/block-core.json | 5 - 6 files changed, 18 insertions(+), 8 deletions(-) +++ b/qapi/block-core.json @@ -2334,6 +2334,9 @@ # # @speed: the maximum speed, in bytes per second # +# @discard: true to delete blocks duplicated in old backing files. +# (default: false). Since 3.1. +# This feels like a feature addition and not a bug fix, so not appropriate for 3.1-rc3. The next release will be 4.0, so the "since" line should be updated accordingly. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
[Qemu-devel] [PATCH v3 05/16] i2c: Simplify and correct the SMBus state machine
From: Corey Minyard
The SMBus slave code had an unneeded state, unnecessary function
pointers and incorrectly handled quick commands. Rewrite it
to simplify the code and make it work correctly.
smbus_eeprom is the only user, so no other effects and the eeprom
code also gets a significant simplification.
Signed-off-by: Corey Minyard
---
hw/i2c/smbus_eeprom.c| 58 ++-
hw/i2c/smbus_slave.c | 91
include/hw/i2c/smbus_slave.h | 45 +-
3 files changed, 86 insertions(+), 108 deletions(-)
diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
index d82423aa7e..4d25222e23 100644
--- a/hw/i2c/smbus_eeprom.c
+++ b/hw/i2c/smbus_eeprom.c
@@ -36,28 +36,12 @@ typedef struct SMBusEEPROMDevice {
uint8_t offset;
} SMBusEEPROMDevice;
-static void eeprom_quick_cmd(SMBusDevice *dev, uint8_t read)
-{
-#ifdef DEBUG
-printf("eeprom_quick_cmd: addr=0x%02x read=%d\n", dev->i2c.address, read);
-#endif
-}
-
-static void eeprom_send_byte(SMBusDevice *dev, uint8_t val)
-{
-SMBusEEPROMDevice *eeprom = (SMBusEEPROMDevice *) dev;
-#ifdef DEBUG
-printf("eeprom_send_byte: addr=0x%02x val=0x%02x\n",
- dev->i2c.address, val);
-#endif
-eeprom->offset = val;
-}
-
static uint8_t eeprom_receive_byte(SMBusDevice *dev)
{
SMBusEEPROMDevice *eeprom = (SMBusEEPROMDevice *) dev;
uint8_t *data = eeprom->data;
uint8_t val = data[eeprom->offset++];
+
#ifdef DEBUG
printf("eeprom_receive_byte: addr=0x%02x val=0x%02x\n",
dev->i2c.address, val);
@@ -65,37 +49,26 @@ static uint8_t eeprom_receive_byte(SMBusDevice *dev)
return val;
}
-static void eeprom_write_data(SMBusDevice *dev, uint8_t cmd, uint8_t *buf, int
len)
+static int eeprom_write_data(SMBusDevice *dev, uint8_t *buf, uint8_t len)
{
SMBusEEPROMDevice *eeprom = (SMBusEEPROMDevice *) dev;
-int n;
+uint8_t *data = eeprom->data;
+
#ifdef DEBUG
printf("eeprom_write_byte: addr=0x%02x cmd=0x%02x val=0x%02x\n",
dev->i2c.address, cmd, buf[0]);
#endif
-/* A page write operation is not a valid SMBus command.
- It is a block write without a length byte. Fortunately we
- get the full block anyway. */
-/* TODO: Should this set the current location? */
-if (cmd + len > 256)
-n = 256 - cmd;
-else
-n = len;
-memcpy(eeprom->data + cmd, buf, n);
-len -= n;
-if (len)
-memcpy(eeprom->data, buf + n, len);
-}
+/* len is guaranteed to be > 0 */
+eeprom->offset = buf[0];
+buf++;
+len--;
+
+for (; len > 0; len--) {
+data[eeprom->offset] = *buf++;
+eeprom->offset = (eeprom->offset + 1) % 256;
+}
-static uint8_t eeprom_read_data(SMBusDevice *dev, uint8_t cmd, int n)
-{
-SMBusEEPROMDevice *eeprom = (SMBusEEPROMDevice *) dev;
-/* If this is the first byte then set the current position. */
-if (n == 0)
-eeprom->offset = cmd;
-/* As with writes, we implement block reads without the
- SMBus length byte. */
-return eeprom_receive_byte(dev);
+return 0;
}
static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
@@ -116,11 +89,8 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass,
void *data)
SMBusDeviceClass *sc = SMBUS_DEVICE_CLASS(klass);
dc->realize = smbus_eeprom_realize;
-sc->quick_cmd = eeprom_quick_cmd;
-sc->send_byte = eeprom_send_byte;
sc->receive_byte = eeprom_receive_byte;
sc->write_data = eeprom_write_data;
-sc->read_data = eeprom_read_data;
dc->props = smbus_eeprom_properties;
/* Reason: pointer property "data" */
dc->user_creatable = false;
diff --git a/hw/i2c/smbus_slave.c b/hw/i2c/smbus_slave.c
index 549e7ae933..70ff29c095 100644
--- a/hw/i2c/smbus_slave.c
+++ b/hw/i2c/smbus_slave.c
@@ -34,7 +34,6 @@ do { fprintf(stderr, "smbus: error: " fmt , ## __VA_ARGS__);}
while (0)
enum {
SMBUS_IDLE,
SMBUS_WRITE_DATA,
-SMBUS_RECV_BYTE,
SMBUS_READ_DATA,
SMBUS_DONE,
SMBUS_CONFUSED = -1
@@ -54,20 +53,9 @@ static void smbus_do_write(SMBusDevice *dev)
{
SMBusDeviceClass *sc = SMBUS_DEVICE_GET_CLASS(dev);
-if (dev->data_len == 0) {
-smbus_do_quick_cmd(dev, 0);
-} else if (dev->data_len == 1) {
-DPRINTF("Send Byte\n");
-if (sc->send_byte) {
-sc->send_byte(dev, dev->data_buf[0]);
-}
-} else {
-dev->command = dev->data_buf[0];
-DPRINTF("Command %d len %d\n", dev->command, dev->data_len - 1);
-if (sc->write_data) {
-sc->write_data(dev, dev->command, dev->data_buf + 1,
- dev->data_len - 1);
-}
+DPRINTF("Command %d len %d\n", dev->data_buf[0], dev->data_len);
+if (sc->write_data) {
+sc->write_data(dev, dev->data_buf, dev->data_len);
}
}
@@ -82,6 +70,7 @@ static int smbus_i2c_event(I2CSlave *s, enum i2c_event event)
Re: [Qemu-devel] [PATCH v3 03/16] arm:i2c: Don't mask return from i2c_recv()
On 26/11/18 21:04, miny...@acm.org wrote:
> From: Corey Minyard
>
> It can't fail, and now that it returns a uint8_t a 0xff mask
> is unnecessary.
>
> Signed-off-by: Corey Minyard
> Suggested-by: Peter Maydell
> ---
> hw/arm/stellaris.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
> index 6c69ce79b2..638b649911 100644
> --- a/hw/arm/stellaris.c
> +++ b/hw/arm/stellaris.c
> @@ -811,7 +811,7 @@ static void stellaris_i2c_write(void *opaque, hwaddr
> offset,
> /* TODO: Handle errors. */
> if (s->msa & 1) {
> /* Recv */
> -s->mdr = i2c_recv(s->bus) & 0xff;
> +s->mdr = i2c_recv(s->bus);
> } else {
> /* Send */
> i2c_send(s->bus, s->mdr);
>
This could be squashed in the previous patch.
Reviewed-by: Philippe Mathieu-Daudé
Tested-by: Philippe Mathieu-Daudé
[Qemu-devel] [PATCH v3 13/16] i2c:smbus_eeprom: Add a size constant for the smbus_eeprom size
From: Corey Minyard
It was hard-coded to 256 in a number of places, create a constant
for that.
Signed-off-by: Corey Minyard
Reviewed-by: Philippe Mathieu-Daudé
---
hw/i2c/smbus_eeprom.c | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
index 8d4eed129f..8e9b734c09 100644
--- a/hw/i2c/smbus_eeprom.c
+++ b/hw/i2c/smbus_eeprom.c
@@ -35,6 +35,8 @@
#define SMBUS_EEPROM(obj) \
OBJECT_CHECK(SMBusEEPROMDevice, (obj), TYPE_SMBUS_EEPROM)
+#define SMBUS_EEPROM_SIZE 256
+
typedef struct SMBusEEPROMDevice {
SMBusDevice smbusdev;
void *data;
@@ -70,7 +72,7 @@ static int eeprom_write_data(SMBusDevice *dev, uint8_t *buf,
uint8_t len)
for (; len > 0; len--) {
data[eeprom->offset] = *buf++;
-eeprom->offset = (eeprom->offset + 1) % 256;
+eeprom->offset = (eeprom->offset + 1) % SMBUS_EEPROM_SIZE;
}
return 0;
@@ -129,12 +131,14 @@ void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom,
const uint8_t *eeprom_spd, int eeprom_spd_size)
{
int i;
-uint8_t *eeprom_buf = g_malloc0(8 * 256); /* XXX: make this persistent */
+ /* XXX: make this persistent */
+uint8_t *eeprom_buf = g_malloc0(8 * SMBUS_EEPROM_SIZE);
if (eeprom_spd_size > 0) {
memcpy(eeprom_buf, eeprom_spd, eeprom_spd_size);
}
for (i = 0; i < nb_eeprom; i++) {
-smbus_eeprom_init_one(smbus, 0x50 + i, eeprom_buf + (i * 256));
+smbus_eeprom_init_one(smbus, 0x50 + i,
+ eeprom_buf + (i * SMBUS_EEPROM_SIZE));
}
}
--
2.17.1
Re: [Qemu-devel] [PATCH v3 06/16] i2c: Add a length check to the SMBus write handling
On 26/11/18 21:04, miny...@acm.org wrote:
> From: Corey Minyard
>
> Avoid an overflow.
>
> Signed-off-by: Corey Minyard
> Reviewed-by: Peter Maydell
Reviewed-by: Philippe Mathieu-Daudé
Tested-by: Philippe Mathieu-Daudé
> ---
> hw/i2c/smbus_slave.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/hw/i2c/smbus_slave.c b/hw/i2c/smbus_slave.c
> index 70ff29c095..d03f714608 100644
> --- a/hw/i2c/smbus_slave.c
> +++ b/hw/i2c/smbus_slave.c
> @@ -182,7 +182,11 @@ static int smbus_i2c_send(I2CSlave *s, uint8_t data)
> switch (dev->mode) {
> case SMBUS_WRITE_DATA:
> DPRINTF("Write data %02x\n", data);
> -dev->data_buf[dev->data_len++] = data;
> +if (dev->data_len >= sizeof(dev->data_buf)) {
> +BADF("Too many bytes sent\n");
> +} else {
> +dev->data_buf[dev->data_len++] = data;
> +}
> break;
>
> default:
>
[Qemu-devel] [PATCH v3 12/16] i2c:smbus_eeprom: Add normal type name and cast to smbus_eeprom.c
From: Corey Minyard
Create a type name and a cast macro and use those through the
code.
Signed-off-by: Corey Minyard
Reviewed-by: Peter Maydell
---
hw/i2c/smbus_eeprom.c | 15 ++-
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
index 4d25222e23..8d4eed129f 100644
--- a/hw/i2c/smbus_eeprom.c
+++ b/hw/i2c/smbus_eeprom.c
@@ -30,6 +30,11 @@
//#define DEBUG
+#define TYPE_SMBUS_EEPROM "smbus-eeprom"
+
+#define SMBUS_EEPROM(obj) \
+OBJECT_CHECK(SMBusEEPROMDevice, (obj), TYPE_SMBUS_EEPROM)
+
typedef struct SMBusEEPROMDevice {
SMBusDevice smbusdev;
void *data;
@@ -38,7 +43,7 @@ typedef struct SMBusEEPROMDevice {
static uint8_t eeprom_receive_byte(SMBusDevice *dev)
{
-SMBusEEPROMDevice *eeprom = (SMBusEEPROMDevice *) dev;
+SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev);
uint8_t *data = eeprom->data;
uint8_t val = data[eeprom->offset++];
@@ -51,7 +56,7 @@ static uint8_t eeprom_receive_byte(SMBusDevice *dev)
static int eeprom_write_data(SMBusDevice *dev, uint8_t *buf, uint8_t len)
{
-SMBusEEPROMDevice *eeprom = (SMBusEEPROMDevice *) dev;
+SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev);
uint8_t *data = eeprom->data;
#ifdef DEBUG
@@ -73,7 +78,7 @@ static int eeprom_write_data(SMBusDevice *dev, uint8_t *buf,
uint8_t len)
static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
{
-SMBusEEPROMDevice *eeprom = (SMBusEEPROMDevice *)dev;
+SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev);
eeprom->offset = 0;
}
@@ -97,7 +102,7 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass,
void *data)
}
static const TypeInfo smbus_eeprom_info = {
-.name = "smbus-eeprom",
+.name = TYPE_SMBUS_EEPROM,
.parent= TYPE_SMBUS_DEVICE,
.instance_size = sizeof(SMBusEEPROMDevice),
.class_init= smbus_eeprom_class_initfn,
@@ -114,7 +119,7 @@ void smbus_eeprom_init_one(I2CBus *smbus, uint8_t address,
uint8_t *eeprom_buf)
{
DeviceState *dev;
-dev = qdev_create((BusState *) smbus, "smbus-eeprom");
+dev = qdev_create((BusState *) smbus, TYPE_SMBUS_EEPROM);
qdev_prop_set_uint8(dev, "address", address);
qdev_prop_set_ptr(dev, "data", eeprom_buf);
qdev_init_nofail(dev);
--
2.17.1
