Re: [Qemu-devel] [PATCH] hw/arm/bcm283x: Fix crash with device_add bcm2837 on unsupported machines

[Thomas Huth]

On 11.07.2018 22:15, Paolo Bonzini wrote:
[...]
> I think you're on the right track, after object_property_add_child you
> need to drop the reference to the object.

Yes, that's the issue indeed! The child objects get properly cleaned up
once I add the object_unref() after the object_property_add_child(), and
then the crash does not occur anymore. I'll check the other spots, then
I'll write a patch...

 Thomas

Re: [Qemu-devel] [Qemu-ppc] [GIT PULL for qemu-pseries] pseries: Update SLOF firmware image

[Thomas Huth]

On 12.07.2018 07:14, Alexey Kardashevskiy wrote:
> On Thu, 12 Jul 2018 07:01:34 +0200
> Thomas Huth  wrote:
> 
>> On 12.07.2018 03:01, David Gibson wrote:
>>> On Wed, Jul 11, 2018 at 08:53:05PM +1000, Alexey Kardashevskiy wrote:  
 On Wed, 11 Jul 2018 16:26:19 +1000
 David Gibson  wrote:
  
> On Tue, Jul 10, 2018 at 05:14:53PM +1000, Alexey Kardashevskiy wrote:  
>> On Tue, 10 Jul 2018 16:42:48 +1000
>> David Gibson  wrote:
>> 
>>> On Tue, Jul 10, 2018 at 12:22:44PM +1000, Alexey Kardashevskiy wrote:   
>>>  
 On Mon,  2 Jul 2018 16:26:44 +1000
 Alexey Kardashevskiy  wrote:
   
> The following changes since commit 
> edb1fb337f65f82fe32b989c4f018efe85c1dddb:
>
>   pseries: Update SLOF firmware image (2018-07-02 16:21:30 +1000)
>
> are available in the git repository at:
>
>   g...@github.com:aik/qemu.git tags/qemu-slof-20180702
>
> for you to fetch changes up to 
> edb1fb337f65f82fe32b989c4f018efe85c1dddb:
>
>   pseries: Update SLOF firmware image (2018-07-02 16:21:30 +1000)
>
> 
>
> I just missed a couple of gcc 8.1 fixes and thought I can still 
> squeeze
> them before the 3.0  soft freeze.
>
>
> *** Note: this is not for master, this is for pseries  


 Not good for 3.1 either?  
>>>
>>> Ugh.. somehow I missed this SLOF update request, and now we're in 3.0
>>> hard freeze.
>>>
>>> How bad are the bugs this SLOF update fixes?
>>
>> These only fix gcc 8.1 warnings and runtime errors => not really
>> bugfixes so it is not a big deal if they miss 3.0.
>
> Ok, well, if you want to resend the update, I'll queue it for 3.1.  


 It is still on github, why resend?  
>>>
>>> Good point, done.  
>>
>> Won't there be another update in the 3.1 time frame? If yes, this sounds
>> like wasted space in the repository to me (the firmware blobs are rather
>> big, and there are no new features in that binary, just fixes for
>> compiling with GCC 8 ... and people who want to compile SLOF on their
>> own can use the sources from aik's git tree, too).
> 
> I compiled the blob with gcc 8.1 and if something goes wrong I'd rather
> know this earlier than later - this my motivation, not the fixes
> themselves.

Ok, that's a fair argument. But in that case, I think it is even good
that this missed the hard freeze - you likely don't want to hunt
hard-to-debug compiler bugs in the freeze period, do you?

 Thomas



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [Qemu-ppc] [GIT PULL for qemu-pseries] pseries: Update SLOF firmware image

[Alexey Kardashevskiy]

On Thu, 12 Jul 2018 07:01:34 +0200
Thomas Huth  wrote:

> On 12.07.2018 03:01, David Gibson wrote:
> > On Wed, Jul 11, 2018 at 08:53:05PM +1000, Alexey Kardashevskiy wrote:  
> >> On Wed, 11 Jul 2018 16:26:19 +1000
> >> David Gibson  wrote:
> >>  
> >>> On Tue, Jul 10, 2018 at 05:14:53PM +1000, Alexey Kardashevskiy wrote:  
>  On Tue, 10 Jul 2018 16:42:48 +1000
>  David Gibson  wrote:
>  
> > On Tue, Jul 10, 2018 at 12:22:44PM +1000, Alexey Kardashevskiy wrote:   
> >  
> >> On Mon,  2 Jul 2018 16:26:44 +1000
> >> Alexey Kardashevskiy  wrote:
> >>   
> >>> The following changes since commit 
> >>> edb1fb337f65f82fe32b989c4f018efe85c1dddb:
> >>>
> >>>   pseries: Update SLOF firmware image (2018-07-02 16:21:30 +1000)
> >>>
> >>> are available in the git repository at:
> >>>
> >>>   g...@github.com:aik/qemu.git tags/qemu-slof-20180702
> >>>
> >>> for you to fetch changes up to 
> >>> edb1fb337f65f82fe32b989c4f018efe85c1dddb:
> >>>
> >>>   pseries: Update SLOF firmware image (2018-07-02 16:21:30 +1000)
> >>>
> >>> 
> >>>
> >>> I just missed a couple of gcc 8.1 fixes and thought I can still 
> >>> squeeze
> >>> them before the 3.0  soft freeze.
> >>>
> >>>
> >>> *** Note: this is not for master, this is for pseries  
> >>
> >>
> >> Not good for 3.1 either?  
> >
> > Ugh.. somehow I missed this SLOF update request, and now we're in 3.0
> > hard freeze.
> >
> > How bad are the bugs this SLOF update fixes?
> 
>  These only fix gcc 8.1 warnings and runtime errors => not really
>  bugfixes so it is not a big deal if they miss 3.0.
> >>>
> >>> Ok, well, if you want to resend the update, I'll queue it for 3.1.  
> >>
> >>
> >> It is still on github, why resend?  
> > 
> > Good point, done.  
> 
> Won't there be another update in the 3.1 time frame? If yes, this sounds
> like wasted space in the repository to me (the firmware blobs are rather
> big, and there are no new features in that binary, just fixes for
> compiling with GCC 8 ... and people who want to compile SLOF on their
> own can use the sources from aik's git tree, too).


I compiled the blob with gcc 8.1 and if something goes wrong I'd rather
know this earlier than later - this my motivation, not the fixes
themselves.



--
Alexey


pgp1U7tPOt7gg.pgp
Description: OpenPGP digital signature

Re: [Qemu-devel] [Qemu-ppc] [GIT PULL for qemu-pseries] pseries: Update SLOF firmware image

[Thomas Huth]

On 12.07.2018 03:01, David Gibson wrote:
> On Wed, Jul 11, 2018 at 08:53:05PM +1000, Alexey Kardashevskiy wrote:
>> On Wed, 11 Jul 2018 16:26:19 +1000
>> David Gibson  wrote:
>>
>>> On Tue, Jul 10, 2018 at 05:14:53PM +1000, Alexey Kardashevskiy wrote:
 On Tue, 10 Jul 2018 16:42:48 +1000
 David Gibson  wrote:
   
> On Tue, Jul 10, 2018 at 12:22:44PM +1000, Alexey Kardashevskiy wrote:  
>> On Mon,  2 Jul 2018 16:26:44 +1000
>> Alexey Kardashevskiy  wrote:
>> 
>>> The following changes since commit 
>>> edb1fb337f65f82fe32b989c4f018efe85c1dddb:
>>>
>>>   pseries: Update SLOF firmware image (2018-07-02 16:21:30 +1000)
>>>
>>> are available in the git repository at:
>>>
>>>   g...@github.com:aik/qemu.git tags/qemu-slof-20180702
>>>
>>> for you to fetch changes up to edb1fb337f65f82fe32b989c4f018efe85c1dddb:
>>>
>>>   pseries: Update SLOF firmware image (2018-07-02 16:21:30 +1000)
>>>
>>> 
>>>
>>> I just missed a couple of gcc 8.1 fixes and thought I can still squeeze
>>> them before the 3.0  soft freeze.
>>>
>>>
>>> *** Note: this is not for master, this is for pseries
>>
>>
>> Not good for 3.1 either?
>
> Ugh.. somehow I missed this SLOF update request, and now we're in 3.0
> hard freeze.
>
> How bad are the bugs this SLOF update fixes?  

 These only fix gcc 8.1 warnings and runtime errors => not really
 bugfixes so it is not a big deal if they miss 3.0.  
>>>
>>> Ok, well, if you want to resend the update, I'll queue it for 3.1.
>>
>>
>> It is still on github, why resend?
> 
> Good point, done.

Won't there be another update in the 3.1 time frame? If yes, this sounds
like wasted space in the repository to me (the firmware blobs are rather
big, and there are no new features in that binary, just fixes for
compiling with GCC 8 ... and people who want to compile SLOF on their
own can use the sources from aik's git tree, too).

 Thomas



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 2/2] vga: don't pick cirrus by default

[Thomas Huth]

On 11.07.2018 20:43, Eduardo Habkost wrote:
> On Wed, Jul 11, 2018 at 07:00:54PM +0200, Sebastian Bauer wrote:
>> Am 2018-07-11 17:48, schrieb Eduardo Habkost:
>>> "none" looked like a false positive when I first looked, but now
>>> I think it's not.  Shouldn't it set default_display="none"?
>>
>> I think that there is some other logic burried that these machine doesn't
>> get a graphics display. But overall it is indeed not clearly defined.
>>
>> But see below.
>>
 If the patch is applied to 3.1 then I think there is enough time to
 fix
 issues caused by the patch. Additionally, a warning could be put in
 the
 ChangeLog for 3.0 that in 3.1 that the default mode will be std unless
 machines define an own default. This is should be enough time for
 people to
 complain or to fix things.
>>> I don't think we will really make user-visible changes: we can
>>> simply work to keep existing behavior, but the difference is that
>>> this will be implemented by setting default_display explicitly on
>>> all machines.
>>
>> Even if all machines were using explicit default settings the patch will
>> affect machines that are not inside the QEMU tree. If the patch is to be
>> applied as it is these are affected. To warn users (or devs in this case)
>> about this, an entry in the ChangeLog would be appropriate.
> 
> What do you mean by "machines that are not inside the QEMU tree"?
> MachineClass registration is not an API for external use.
> 
>>
 If the patch is to be applied to 3.0 then all non-ppc ones need to be
 reconsidered.
 The "important" ppc machines have been fixed already. I can do the
 remaining
 if this is wanted.
>>> This part worries me: do we have other machines that are broken
>>> right now?
>>
>> I don't know which of them are broken or how this can be elaborated, but
>> they are potentially affected. For instance, the sam460ex platform doesn't
>> care about this setting, I can say that it is not broken. Other platforms
>> like the mac apparently were broken (and fixed in the meantime). It is hard
>> to tell which of them are really broken without someone that knows the
>> platform trying it and telling it. 'Make check' did catch only one single
>> case. It could also be that nobody cares about other affected machines.
>>
>> Overall I think the patch is an improvement over the previous state as
>> preferring the Cirrus doesn't make much sense if most machines don't prefer
>> it. The more I think over it, the more I think that the concept needs
>> further fine-tuning though (not necessarily in this patch).
> 
> I'm not sure yet if most machines with default_display==NULL
> don't want Cirrus.  I'm also unsure if any of the machines from
> that list will break if we start using VGA by default.

I think most machines with default_display == NULL simply do not want
any graphic cards at all. So we likely document that default_display ==
NULL means no graphic card, and fix the machines that have other
assumptions if necessary.

>> [...]  I still think that the most common value can be a
>> default (strictly bailing out when it is not available unlike it is done
>> now), but this is a matter of taste I guess.
> 
> This is doable too, if we have a clear consensus on what would be
> a reasonable default on TYPE_MACHINE.  Personally I prefer the
> default on TYPE_MACHINE to be "none".

Yes, please, no magic default hardware for all machines. That will only
cause confusion and problems in the future. People who add new boards
will keep forgetting to set a value in their new boards and then you
later wonder whether that was on purpose or by accident ==> If there is
no default display set, this should simply mean "no display".

 Thomas

Re: [Qemu-devel] [PATCH] ppc/xics: split ICP into icp-base and icp class

[David Gibson]

On Wed, Jul 11, 2018 at 11:26:41AM +0200, Greg Kurz wrote:
> On Wed, 11 Jul 2018 11:28:02 +1000
> David Gibson  wrote:
> 
> > On Tue, Jul 10, 2018 at 05:55:14PM +0200, Greg Kurz wrote:
> > > Recent cleanup in commit a028dd423ee6 causes QEMU to crash during CPU
> > > hotplug:
> > > 
> > > (qemu) device_add host-spapr-cpu-core,id=core1,core-id=1
> > > Segmentation fault (core dumped)
> > > 
> > > When the hotplug path tries to reset the ICP device, we end
> > > up calling:
> > > 
> > > static void icp_kvm_reset(DeviceState *dev)
> > > {
> > > ICPStateClass *icpc = ICP_GET_CLASS(dev);
> > > 
> > > icpc->parent_reset(dev);
> > > 
> > > but icpc->parent_reset is NULL.  
> > 
> > This seems terribly complex to address this symptom.  Couldn't we just
> > do a
> > 
> > if (icpc->parent_reset)
> > icpc->parent_reset(dev);
> > 
> > ?
> > 
> 
> This would certainly avoid the SEGV but this would mean that
> we skip icp_reset() for hot plugged ICPs... and it doesn't
> address the fact that icp_kvm_reset() is never called for cold
> plugged ones.

Ah, yeah, duh.

> Commit a028dd423ee6 inverted the control: icp_reset() no
> longer calls per-ICP type reset function. It is up to each
> type to call icp_reset().

Hrm.. could we have icp_realize() do a qemu_register_reset() on
dc->reset, rather than on icp_reset() directly.  And make sure the
base class sets dc->reset correctly too, of course.

I think that'll accomplish what we want without rearranging the class
structure again.

> Maybe I can split the patch to ease review. But if you're really
> seeking simplicity, the best call is probably to simply revert
> a028dd423ee6 at this point.




> 
> > > Indeed, the TYPE_ICP class doesn't implement DeviceClass::reset, but
> > > rather directly registers a reset handler with qemu_register_reset().
> > > This is done this way because ICPs aren't SysBus devices but we want
> > > machine reset to reset them anyway (commit 7ea6e0671754).
> > > 
> > > The crash also proves that ipc_kvm_reset() is obviously not called for
> > > cold plugged devices, ie, TYPE_ICP_KVM should register its own handler
> > > with qemu_register_reset().
> > > 
> > > The motivation of commit a028dd423ee6 to have cleaner object patterns
> > > is good, but it means that all specialized ICP types should register
> > > their own reset handler to be called at machine reset.
> > > 
> > > So this patchs converts the current TYPE_ICP class into an abstract
> > > TYPE_ICP_BASE class that implements DeviceClass::reset instead of
> > > calling qemu_register_reset().
> > > 
> > > The _new_ TYPE_ICP class is derived from TYPE_ICP_BASE to cover the
> > > the pseries.kernel-irqchip=off case. It merely registers a reset
> > > handler with qemu_register_reset(). Its device class functions
> > > are renamed as icp_simple_* to avoid confusion with the base class.
> > > 
> > > All other specialized ICP types are also converted to register their
> > > own reset handler as well.
> > > 
> > > This matches what we already have with ICS.
> > > 
> > > Since we support CPU hot unplug with spapr, unrealize functions
> > > are added to TYPE_ICP and TYPE_ICP_KVM so that they can call
> > > qemu_unregister_reset().
> > > 
> > > Reported-by: Satheesh Rajendran 
> > > Fixes: a028dd423ee6dfd091a8c63028240832bf10f671
> > > Signed-off-by: Greg Kurz 
> > > ---
> > > 
> > > I've successfully tested the following:
> > > - pseries with in-kernel XICS
> > > - pseries with emulated XICS
> > > - powernv
> > > - migration of pseries, including migration to QEMU 2.12 and back
> > > ---
> > >  hw/intc/xics.c|   97 
> > > -
> > >  hw/intc/xics_kvm.c|   27 +++---
> > >  hw/intc/xics_pnv.c|   27 +++---
> > >  include/hw/ppc/xics.h |   12 --
> > >  4 files changed, 129 insertions(+), 34 deletions(-)
> > > 
> > > diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> > > index b9f1a3c97214..b478b9120062 100644
> > > --- a/hw/intc/xics.c
> > > +++ b/hw/intc/xics.c
> > > @@ -40,7 +40,7 @@
> > >  
> > >  void icp_pic_print_info(ICPState *icp, Monitor *mon)
> > >  {
> > > -ICPStateClass *icpc = ICP_GET_CLASS(icp);
> > > +ICPStateClass *icpc = ICP_BASE_GET_CLASS(icp);
> > >  int cpu_index = icp->cs ? icp->cs->cpu_index : -1;
> > >  
> > >  if (!icp->output) {
> > > @@ -255,7 +255,7 @@ static void icp_irq(ICSState *ics, int server, int 
> > > nr, uint8_t priority)
> > >  static int icp_dispatch_pre_save(void *opaque)
> > >  {
> > >  ICPState *icp = opaque;
> > > -ICPStateClass *info = ICP_GET_CLASS(icp);
> > > +ICPStateClass *info = ICP_BASE_GET_CLASS(icp);
> > >  
> > >  if (info->pre_save) {
> > >  info->pre_save(icp);
> > > @@ -267,7 +267,7 @@ static int icp_dispatch_pre_save(void *opaque)
> > >  static int icp_dispatch_post_load(void *opaque, int version_id)
> > >  {
> > >  ICPState *icp = opaque;
> > > -ICPStateClass *info = ICP_GET_CLASS(icp);
> > > +

Re: [Qemu-devel] [PATCH] migration: release MigrationIncomingState in migration_object_finalize

[858585 jemmy]

On Fri, Jul 6, 2018 at 6:41 PM, Dr. David Alan Gilbert
 wrote:
> * Dr. David Alan Gilbert (dgilb...@redhat.com) wrote:
>> * Lidong Chen (jemmy858...@gmail.com) wrote:
>> > Qemu initialize the MigrationIncomingState structure in 
>> > migration_object_init,
>> > but not release it. this patch release it in migration_object_finalize.
>> >
>> > Signed-off-by: Lidong Chen 
>>
>> Queued
>
> I've had to unqueue this, see below:
>
>>
>> > ---
>> >  migration/migration.c | 7 +++
>> >  1 file changed, 7 insertions(+)
>> >
>> > diff --git a/migration/migration.c b/migration/migration.c
>> > index 05aec2c..e009a05 100644
>> > --- a/migration/migration.c
>> > +++ b/migration/migration.c
>> > @@ -156,6 +156,13 @@ void migration_object_init(void)
>> >  void migration_object_finalize(void)
>> >  {
>> >  object_unref(OBJECT(current_migration));
>> > +
>> > +qemu_sem_destroy(_incoming->postcopy_pause_sem_fault);
>> > +qemu_sem_destroy(_incoming->postcopy_pause_sem_dst);
>> > +qemu_event_destroy(_incoming->main_thread_load_event);
>> > +qemu_mutex_destroy(_incoming->rp_mutex);
>> > +g_array_free(current_incoming->postcopy_remote_fds, true);
>
> That array is already free'd in migration_incoming_state_destroy,
> so I see reliable glib assert's from this array free.

The migration_incoming_state_destroy only invoked in destination qemu.
The source qemu will not free this memory.
So I think free current_incoming->postcopy_remote_fds is not good way.

and migration_object_init and migration_object_finalize should not be
invoked in main
function. It's better to  alloc memory when start migration and
release it when migration finished.

I will submit a new version patch to fix it.

>
> Dave
>
>> > +g_free(current_incoming);
>> >  }
>> >
>> >  /* For outgoing */
>> > --
>> > 1.8.3.1
>> >
>> --
>> Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
> --
> Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

[Qemu-devel] [PATCH v2] nvme: Fix nvme_init error handling

[Fam Zheng]

It is wrong to leave this field as 1, as nvme_close() called in the
error handling code in nvme_file_open() will use it and try to free
s->queues again.

Another problem is the cleaning ups are duplicated between the fail*
labels of nvme_init() and nvme_file_open(), which calls nvme_close().

A third problem is nvme_close() misses g_free() and
event_notifier_cleanup().

Fix all of them.

Cc: qemu-sta...@nongnu.org
Signed-off-by: Fam Zheng 

---

v2: Adopt the suggested fix by Kevin.
---
 block/nvme.c | 37 -
 1 file changed, 12 insertions(+), 25 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index 6f71122bf5..37805e8890 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -569,13 +569,13 @@ static int nvme_init(BlockDriverState *bs, const char 
*device, int namespace,
 s->vfio = qemu_vfio_open_pci(device, errp);
 if (!s->vfio) {
 ret = -EINVAL;
-goto fail;
+goto out;
 }
 
 s->regs = qemu_vfio_pci_map_bar(s->vfio, 0, 0, NVME_BAR_SIZE, errp);
 if (!s->regs) {
 ret = -EINVAL;
-goto fail;
+goto out;
 }
 
 /* Perform initialize sequence as described in NVMe spec "7.6.1
@@ -585,7 +585,7 @@ static int nvme_init(BlockDriverState *bs, const char 
*device, int namespace,
 if (!(cap & (1ULL << 37))) {
 error_setg(errp, "Device doesn't support NVMe command set");
 ret = -EINVAL;
-goto fail;
+goto out;
 }
 
 s->page_size = MAX(4096, 1 << (12 + ((cap >> 48) & 0xF)));
@@ -603,7 +603,7 @@ static int nvme_init(BlockDriverState *bs, const char 
*device, int namespace,
  PRId64 " ms)",
timeout_ms);
 ret = -ETIMEDOUT;
-goto fail;
+goto out;
 }
 }
 
@@ -613,7 +613,7 @@ static int nvme_init(BlockDriverState *bs, const char 
*device, int namespace,
 s->queues[0] = nvme_create_queue_pair(bs, 0, NVME_QUEUE_SIZE, errp);
 if (!s->queues[0]) {
 ret = -EINVAL;
-goto fail;
+goto out;
 }
 QEMU_BUILD_BUG_ON(NVME_QUEUE_SIZE & 0xF000);
 s->regs->aqa = cpu_to_le32((NVME_QUEUE_SIZE << 16) | NVME_QUEUE_SIZE);
@@ -633,14 +633,14 @@ static int nvme_init(BlockDriverState *bs, const char 
*device, int namespace,
  PRId64 " ms)",
timeout_ms);
 ret = -ETIMEDOUT;
-goto fail_queue;
+goto out;
 }
 }
 
 ret = qemu_vfio_pci_init_irq(s->vfio, >irq_notifier,
  VFIO_PCI_MSIX_IRQ_INDEX, errp);
 if (ret) {
-goto fail_queue;
+goto out;
 }
 aio_set_event_notifier(bdrv_get_aio_context(bs), >irq_notifier,
false, nvme_handle_event, nvme_poll_cb);
@@ -649,30 +649,15 @@ static int nvme_init(BlockDriverState *bs, const char 
*device, int namespace,
 if (local_err) {
 error_propagate(errp, local_err);
 ret = -EIO;
-goto fail_handler;
+goto out;
 }
 
 /* Set up command queues. */
 if (!nvme_add_io_queue(bs, errp)) {
 ret = -EIO;
-goto fail_handler;
 }
-return 0;
-
-fail_handler:
-aio_set_event_notifier(bdrv_get_aio_context(bs), >irq_notifier,
-   false, NULL, NULL);
-fail_queue:
-nvme_free_queue_pair(bs, s->queues[0]);
-fail:
-g_free(s->queues);
-if (s->regs) {
-qemu_vfio_pci_unmap_bar(s->vfio, 0, (void *)s->regs, 0, NVME_BAR_SIZE);
-}
-if (s->vfio) {
-qemu_vfio_close(s->vfio);
-}
-event_notifier_cleanup(>irq_notifier);
+out:
+/* Cleaning up is done in nvme_file_open() upon error. */
 return ret;
 }
 
@@ -739,8 +724,10 @@ static void nvme_close(BlockDriverState *bs)
 for (i = 0; i < s->nr_queues; ++i) {
 nvme_free_queue_pair(bs, s->queues[i]);
 }
+g_free(s->queues);
 aio_set_event_notifier(bdrv_get_aio_context(bs), >irq_notifier,
false, NULL, NULL);
+event_notifier_cleanup(>irq_notifier);
 qemu_vfio_pci_unmap_bar(s->vfio, 0, (void *)s->regs, 0, NVME_BAR_SIZE);
 qemu_vfio_close(s->vfio);
 }
-- 
2.17.1

[Qemu-devel] [PATCH v2] target-i386: coalesced PIO support for RTC

[Wanpeng Li]

From: Peng Hao 

Windows I/O, such as the real-time clock. The address register (port
0x70 in the RTC case) can use coalesced I/O, cutting the number of
userspace exits by half when reading or writing the RTC.

Guest access rtc like this: write register index to 0x70, then write or 
read data from 0x71. writing 0x70 port is just as index and do nothing 
else. So we can use coalesced mmio to handle this scene to reduce VM-EXIT 
time.

In our environment, 12 windows guest running on a Skylake server:

Before patch:

IO Port Access  Samples  Samples%   Time%Avg time

0x70:POUT2067546.04%92.72%   67.15us ( +-   7.93% )

After patch:

IO Port Access  Samples  Samples%   Time%Avg time

0x70:POUT1750945.42%42.08%   6.37us ( +-  20.37% )

Cc: Paolo Bonzini 
Cc: Radim Krčmář 
Cc: Eduardo Habkost 
Cc: Peng Hao 
Signed-off-by: Peng Hao 
Signed-off-by: Wanpeng Li 
---
v1 -> v2:
 * add the original author

 accel/kvm/kvm-all.c   | 56 +++
 hw/timer/mc146818rtc.c|  8 +++
 include/exec/memattrs.h   |  1 +
 include/exec/memory.h |  5 +
 include/sysemu/kvm.h  |  8 +++
 linux-headers/linux/kvm.h |  5 +++--
 memory.c  |  5 +
 7 files changed, 82 insertions(+), 6 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index eb7db92..7a12341 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -128,6 +128,7 @@ bool kvm_direct_msi_allowed;
 bool kvm_ioeventfd_any_length_allowed;
 bool kvm_msi_use_devid;
 static bool kvm_immediate_exit;
+bool kvm_coalesced_pio_allowed;
 
 static const KVMCapabilityInfo kvm_required_capabilites[] = {
 KVM_CAP_INFO(USER_MEMORY),
@@ -536,7 +537,7 @@ static void kvm_coalesce_mmio_region(MemoryListener 
*listener,
 
 zone.addr = start;
 zone.size = size;
-zone.pad = 0;
+zone.pio = 0;
 
 (void)kvm_vm_ioctl(s, KVM_REGISTER_COALESCED_MMIO, );
 }
@@ -553,7 +554,7 @@ static void kvm_uncoalesce_mmio_region(MemoryListener 
*listener,
 
 zone.addr = start;
 zone.size = size;
-zone.pad = 0;
+zone.pio = 0;
 
 (void)kvm_vm_ioctl(s, KVM_UNREGISTER_COALESCED_MMIO, );
 }
@@ -877,6 +878,45 @@ static void kvm_io_ioeventfd_del(MemoryListener *listener,
 }
 }
 
+static void kvm_coalesce_io_add(MemoryListener *listener,
+MemoryRegionSection *section,
+hwaddr start, hwaddr size)
+{
+KVMState *s = kvm_state;
+
+if (kvm_coalesced_pio_allowed) {
+struct kvm_coalesced_mmio_zone zone;
+
+zone.addr = start;
+zone.size = size;
+zone.pio = 1;
+
+(void)kvm_vm_ioctl(s, KVM_REGISTER_COALESCED_MMIO, );
+}
+}
+
+static void kvm_coalesce_io_del(MemoryListener *listener,
+MemoryRegionSection *section,
+hwaddr start, hwaddr size)
+{
+KVMState *s = kvm_state;
+
+if (kvm_coalesced_pio_allowed) {
+struct kvm_coalesced_mmio_zone zone;
+
+zone.addr = start;
+zone.size = size;
+zone.pio = 1;
+
+(void)kvm_vm_ioctl(s, KVM_UNREGISTER_COALESCED_MMIO, );
+}
+}
+
+static MemoryListener kvm_coalesced_io_listener = {
+.coalesced_mmio_add = kvm_coalesce_io_add,
+.coalesced_mmio_del = kvm_coalesce_io_del,
+.priority = 10,
+};
 void kvm_memory_listener_register(KVMState *s, KVMMemoryListener *kml,
   AddressSpace *as, int as_id)
 {
@@ -1615,6 +1655,8 @@ static int kvm_init(MachineState *ms)
 }
 
 s->coalesced_mmio = kvm_check_extension(s, KVM_CAP_COALESCED_MMIO);
+kvm_coalesced_pio_allowed = s->coalesced_mmio &&
+kvm_check_extension(s, KVM_CAP_COALESCED_PIO);
 
 #ifdef KVM_CAP_VCPU_EVENTS
 s->vcpu_events = kvm_check_extension(s, KVM_CAP_VCPU_EVENTS);
@@ -1694,6 +1736,8 @@ static int kvm_init(MachineState *ms)
  _space_memory, 0);
 memory_listener_register(_io_listener,
  _space_io);
+memory_listener_register(_coalesced_io_listener,
+ _space_io);
 
 s->many_ioeventfds = kvm_check_many_ioeventfds();
 
@@ -1775,8 +1819,12 @@ void kvm_flush_coalesced_mmio_buffer(void)
 struct kvm_coalesced_mmio *ent;
 
 ent = >coalesced_mmio[ring->first];
-
-cpu_physical_memory_write(ent->phys_addr, ent->data, ent->len);
+if (ent->pio) {
+address_space_rw(_space_io, ent->phys_addr,
+ MEMTXATTRS_NONE, ent->data, ent->len, true);
+} else {
+cpu_physical_memory_write(ent->phys_addr, ent->data, ent->len);
+}
 smp_wmb();
 ring->first = (ring->first + 1) % KVM_COALESCED_MMIO_MAX;
 }
diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index 

[Qemu-devel] [Bug 1777969] Re: Crash with UEFI, q35, AHCI, and <= SystemRescueCD 4.3.0

[Matthew Stapleton]

It looks like this crash is fixed with git commit:
bed9bcfa3275a9cfee82846a9f521c4858a9739a

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1777969

Title:
  Crash with UEFI, q35, AHCI, and <= SystemRescueCD 4.3.0

Status in QEMU:
  New

Bug description:
  I am getting a crash when booting <= SystemRescueCD 4.3.0 in UEFI mode
  with q35 machine and from a AHCI device with qemu 2.11.1 and 2.12.0.
  The crash doesn't occur if I compile with --enable-trace-
  backends=simple or if I use virtio-scsi.  The original crash was
  noticed on Gentoo with hardened gcc 6.4.0 and an Intel CPU, the test
  system to reproduce the crash is on Gentoo with non-hardened gcc 5.4.0
  and an Intel CPU.

  OVMF version is from Gentoo: edk2-ovmf-2017_p20180211-bin.tar.xz

  Here is the commands I have run on qemu 2.12.0 to reproduce the issue 
although it also crashes with accel=kvm removed:
  ./configure --target-list="x86_64-softmmu"
  make
  qemu-system-x86_64 -nodefaults -machine q35,accel=kvm -cpu qemu64 -drive 
if=pflash,format=raw,unit=0,file=/usr/share/edk2-ovmf/OVMF_CODE.fd,readonly=on 
-drive if=pflash,format=raw,unit=1,file=OVMF_VARS.fd -m 512 -drive 
file=systemrescuecd-x86-4.3.0.iso,if=none,id=cdrom-sysresc,readonly=on -device 
ide-cd,bus=ide.0,unit=0,drive=cdrom-sysresc,bootindex=5 -device VGA -display gtk

  Valgrind says "Bad permissions for mapped region at address
  0x4C022FE0" for the crash.

  Here is a backtrace from gdb:
  Program received signal SIGSEGV, Segmentation fault.
  0x7f42dcbc5833 in malloc () from /lib64/libc.so.6
  (gdb) bt
  #0  0x7f42dcbc5833 in malloc () from /lib64/libc.so.6
  #1  0x7f42e10117d9 in g_malloc () from /usr/lib64/libglib-2.0.so.0
  #2  0x55a3ff9def8f in qemu_aio_get 
(aiocb_info=aiocb_info@entry=0x55a4001b39a0 , 
bs=bs@entry=0x0, cb=cb@entry=0x55a3ff9dfe20 , 
opaque=opaque@entry=0x7f42961e30b0) at util/aiocb.c:33
  #3  0x55a3ff9e0249 in thread_pool_submit_aio 
(pool=pool@entry=0x55a400c038d0, func=func@entry=0x55a3ff956620 , 
arg=arg@entry=0x55a400bd30b0, cb=cb@entry=0x55a3ff9dfe20 , 
  opaque=opaque@entry=0x7f42961e30b0) at util/thread-pool.c:251
  #4  0x55a3ff9e0423 in thread_pool_submit_co (pool=0x55a400c038d0, 
func=func@entry=0x55a3ff956620 , arg=arg@entry=0x55a400bd30b0) at 
util/thread-pool.c:289
  #5  0x55a3ff956b50 in paio_submit_co (bs=0x55a400bff180, fd=, offset=362702848, qiov=, bytes=2048, type=1) at 
block/file-posix.c:1536
  #6  0x55a3ff95c82a in bdrv_driver_preadv (bs=bs@entry=0x55a400bff180, 
offset=offset@entry=362702848, bytes=bytes@entry=2048, 
qiov=qiov@entry=0x7f42961e3650, flags=0) at block/io.c:924
  #7  0x55a3ff960154 in bdrv_aligned_preadv 
(child=child@entry=0x55a400c03a20, req=req@entry=0x7f42961e32e0, 
offset=offset@entry=362702848, bytes=bytes@entry=2048, align=align@entry=1, 
qiov=qiov@entry=0x7f42961e3650, flags=0)
  at block/io.c:1228
  #8  0x55a3ff960434 in bdrv_co_preadv (child=0x55a400c03a20, 
offset=362702848, bytes=2048, qiov=0x7f42961e3650, flags=0) at block/io.c:1324
  #9  0x55a3ff95c82a in bdrv_driver_preadv (bs=bs@entry=0x55a400bf8e50, 
offset=offset@entry=362702848, bytes=bytes@entry=2048, 
qiov=qiov@entry=0x7f42961e3650, flags=0) at block/io.c:924
  #10 0x55a3ff960154 in bdrv_aligned_preadv 
(child=child@entry=0x55a400be92c0, req=req@entry=0x7f42961e3510, 
offset=offset@entry=362702848, bytes=bytes@entry=2048, align=align@entry=512, 
qiov=qiov@entry=0x7f42961e3650, flags=0)
  at block/io.c:1228
  #11 0x55a3ff960434 in bdrv_co_preadv (child=0x55a400be92c0, 
offset=offset@entry=362702848, bytes=bytes@entry=2048, 
qiov=qiov@entry=0x7f42961e3650, flags=flags@entry=0) at block/io.c:1324
  #12 0x55a3ff94f4ce in blk_co_preadv (blk=0x55a400bf8ba0, 
offset=362702848, bytes=2048, qiov=0x7f42961e3650, flags=0) at 
block/block-backend.c:1158
  #13 0x55a3ff94f5ac in blk_read_entry (opaque=0x7f42961e3670) at 
block/block-backend.c:1206
  #14 0x55a3ff94e000 in blk_prw (blk=0x55a400bf8ba0, offset=362702848, 
buf=, bytes=bytes@entry=2048, 
co_entry=co_entry@entry=0x55a3ff94f590 , flags=flags@entry=0) 
at block/block-backend.c:1243
  #15 0x55a3ff94f076 in blk_pread (blk=, offset=, buf=, count=count@entry=2048) at block/block-backend.c:1409
  #16 0x55a3ff7d8b93 in cd_read_sector_sync (s=0x55a401a0faa0) at 
hw/ide/atapi.c:124
  #17 ide_atapi_cmd_reply_end (s=0x55a401a0faa0) at hw/ide/atapi.c:269
  #18 0x55a3ff7dde0e in ahci_start_transfer (dma=0x55a401a0f9f0) at 
hw/ide/ahci.c:1325
  #19 0x55a3ff7d870c in ide_atapi_cmd_reply_end (s=0x55a401a0faa0) at 
hw/ide/atapi.c:285
  #20 0x55a3ff7dde0e in ahci_start_transfer (dma=0x55a401a0f9f0) at 
hw/ide/ahci.c:1325
  #21 0x55a3ff7d870c in ide_atapi_cmd_reply_end (s=0x55a401a0faa0) at 
hw/ide/atapi.c:285
  #22 0x55a3ff7dde0e in ahci_start_transfer (dma=0x55a401a0f9f0) at 
hw/ide/ahci.c:1325
  #23 0x55a3ff7d870c 

[Qemu-devel] [PATCH v3 4/4] tests: Add centos VM testing

[Fam Zheng]

This one does docker testing in the VM. It is intended to replace the
native docker testing on patchew testers.

Signed-off-by: Fam Zheng 
---
 tests/vm/Makefile.include |  3 +-
 tests/vm/centos   | 84 +++
 2 files changed, 86 insertions(+), 1 deletion(-)
 create mode 100755 tests/vm/centos

diff --git a/tests/vm/Makefile.include b/tests/vm/Makefile.include
index 5daa2a3b73..af19b7a4e6 100644
--- a/tests/vm/Makefile.include
+++ b/tests/vm/Makefile.include
@@ -2,7 +2,7 @@
 
 .PHONY: vm-build-all
 
-IMAGES := ubuntu.i386 freebsd netbsd openbsd
+IMAGES := ubuntu.i386 freebsd netbsd openbsd centos
 IMAGE_FILES := $(patsubst %, tests/vm/%.img, $(IMAGES))
 
 .PRECIOUS: $(IMAGE_FILES)
@@ -14,6 +14,7 @@ vm-test:
@echo "  vm-build-freebsd- Build QEMU in FreeBSD VM"
@echo "  vm-build-netbsd - Build QEMU in NetBSD VM"
@echo "  vm-build-openbsd- Build QEMU in OpenBSD VM"
+   @echo "  vm-build-centos - Build QEMU in CentOS VM, 
with Docker"
 
 vm-build-all: $(addprefix vm-build-, $(IMAGES))
 
diff --git a/tests/vm/centos b/tests/vm/centos
new file mode 100755
index 00..afd560c564
--- /dev/null
+++ b/tests/vm/centos
@@ -0,0 +1,84 @@
+#!/usr/bin/env python
+#
+# CentOS image
+#
+# Copyright 2018 Red Hat Inc.
+#
+# Authors:
+#  Fam Zheng 
+#
+# This code is licensed under the GPL version 2 or later.  See
+# the COPYING file in the top-level directory.
+#
+
+import os
+import sys
+import subprocess
+import basevm
+import time
+
+class CentosVM(basevm.BaseVM):
+name = "centos"
+BUILD_SCRIPT = """
+set -e;
+cd $(mktemp -d);
+export SRC_ARCHIVE=/dev/vdb;
+sudo chmod a+r $SRC_ARCHIVE;
+tar -xf $SRC_ARCHIVE;
+make docker-test-block@centos7 V={verbose} J={jobs};
+make docker-test-quick@centos7 V={verbose} J={jobs};
+make docker-test-mingw@fedora V={verbose} J={jobs};
+"""
+
+def _gen_cloud_init_iso(self):
+cidir = self._tmpdir
+mdata = open(os.path.join(cidir, "meta-data"), "w")
+mdata.writelines(["instance-id: centos-vm-0\n",
+  "local-hostname: centos-guest\n"])
+mdata.close()
+udata = open(os.path.join(cidir, "user-data"), "w")
+udata.writelines(["#cloud-config\n",
+  "chpasswd:\n",
+  "  list: |\n",
+  "root:%s\n" % self.ROOT_PASS,
+  "%s:%s\n" % (self.GUEST_USER, self.GUEST_PASS),
+  "  expire: False\n",
+  "users:\n",
+  "  - name: %s\n" % self.GUEST_USER,
+  "sudo: ALL=(ALL) NOPASSWD:ALL\n",
+  "ssh-authorized-keys:\n",
+  "- %s\n" % basevm.SSH_PUB_KEY,
+  "  - name: root\n",
+  "ssh-authorized-keys:\n",
+  "- %s\n" % basevm.SSH_PUB_KEY,
+  "locale: en_US.UTF-8\n"])
+udata.close()
+subprocess.check_call(["genisoimage", "-output", "cloud-init.iso",
+   "-volid", "cidata", "-joliet", "-rock",
+   "user-data", "meta-data"],
+   cwd=cidir,
+   stdin=self._devnull, stdout=self._stdout,
+   stderr=self._stdout)
+return os.path.join(cidir, "cloud-init.iso")
+
+def build_image(self, img):
+cimg = 
self._download_with_cache("https://cloud.centos.org/centos/7/images/CentOS-7-x86_64-GenericCloud-1802.qcow2.xz;)
+img_tmp = img + ".tmp"
+subprocess.check_call(["cp", "-f", cimg, img_tmp + ".xz"])
+subprocess.check_call(["xz", "-df", img_tmp + ".xz"])
+subprocess.check_call(["qemu-img", "resize", img_tmp, "50G"])
+self.boot(img_tmp, extra_args = ["-cdrom", self._gen_cloud_init_iso()])
+self.wait_ssh()
+self.ssh_root_check("touch /etc/cloud/cloud-init.disabled")
+self.ssh_root_check("yum update -y")
+self.ssh_root_check("yum install -y docker make git")
+self.ssh_root_check("systemctl enable docker")
+self.ssh_root("poweroff")
+self.wait()
+if os.path.exists(img):
+os.remove(img)
+os.rename(img_tmp, img)
+return 0
+
+if __name__ == "__main__":
+sys.exit(basevm.main(CentosVM))
-- 
2.17.1

[Qemu-devel] [PATCH v3 2/4] tests/vm: Pass verbose flag into VM make commands

[Fam Zheng]

Our Makefile has:

vm-build-%: tests/vm/%.img
$(call quiet-command, \
$(SRC_PATH)/tests/vm/$* \
$(if $(V)$(DEBUG), --debug) \
$(if $(DEBUG), --interactive) \

the intention of which is to let the make command in VM have V=1 if
V=1 is set. We pass this to the BUILD_SCRIPT format.

Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Fam Zheng 
---
 tests/vm/basevm.py   | 3 ++-
 tests/vm/freebsd | 4 ++--
 tests/vm/netbsd  | 4 ++--
 tests/vm/openbsd | 4 ++--
 tests/vm/ubuntu.i386 | 4 ++--
 5 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py
index e5d6a328d5..25361c6b7d 100755
--- a/tests/vm/basevm.py
+++ b/tests/vm/basevm.py
@@ -240,7 +240,8 @@ def main(vmcls):
 vm.add_source_dir(args.build_qemu)
 cmd = [vm.BUILD_SCRIPT.format(
configure_opts = " ".join(argv),
-   jobs=args.jobs)]
+   jobs=args.jobs,
+   verbose="1" if args.debug else "")]
 else:
 cmd = argv
 img = args.image
diff --git a/tests/vm/freebsd b/tests/vm/freebsd
index 039dad8f69..b20612a6c9 100755
--- a/tests/vm/freebsd
+++ b/tests/vm/freebsd
@@ -23,8 +23,8 @@ class FreeBSDVM(basevm.BaseVM):
 cd $(mktemp -d /var/tmp/qemu-test.XX);
 tar -xf /dev/vtbd1;
 ./configure {configure_opts};
-gmake -j{jobs};
-gmake check;
+gmake -j{jobs} V={verbose};
+gmake check V={verbose};
 """
 
 def build_image(self, img):
diff --git a/tests/vm/netbsd b/tests/vm/netbsd
index 3972d8b45c..3f9d34a208 100755
--- a/tests/vm/netbsd
+++ b/tests/vm/netbsd
@@ -23,8 +23,8 @@ class NetBSDVM(basevm.BaseVM):
 cd $(mktemp -d /var/tmp/qemu-test.XX);
 tar -xf /dev/rld1a;
 ./configure --python=python2.7 {configure_opts};
-gmake -j{jobs};
-gmake check;
+gmake -j{jobs} V={verbose};
+gmake check V={verbose};
 """
 
 def build_image(self, img):
diff --git a/tests/vm/openbsd b/tests/vm/openbsd
index 6ae16d97fd..3285c1abde 100755
--- a/tests/vm/openbsd
+++ b/tests/vm/openbsd
@@ -23,9 +23,9 @@ class OpenBSDVM(basevm.BaseVM):
 cd $(mktemp -d /var/tmp/qemu-test.XX);
 tar -xf /dev/rsd1c;
 ./configure --cc=x86_64-unknown-openbsd6.1-gcc-4.9.4 
--python=python2.7 {configure_opts};
-gmake -j{jobs};
+gmake -j{jobs} V={verbose};
 # XXX: "gmake check" seems to always hang or fail
-#gmake check;
+#gmake check V={verbose};
 """
 
 def build_image(self, img):
diff --git a/tests/vm/ubuntu.i386 b/tests/vm/ubuntu.i386
index fc319e0e6e..b4eaa0dedc 100755
--- a/tests/vm/ubuntu.i386
+++ b/tests/vm/ubuntu.i386
@@ -25,8 +25,8 @@ class UbuntuX86VM(basevm.BaseVM):
 sudo chmod a+r /dev/vdb;
 tar -xf /dev/vdb;
 ./configure {configure_opts};
-make -j{jobs};
-make check;
+make -j{jobs} V={verbose};
+make check V={verbose};
 """
 
 def _gen_cloud_init_iso(self):
-- 
2.17.1

[Qemu-devel] [PATCH v3 1/4] tests: Add an option for snapshot (default: off)

[Fam Zheng]

Not using snapshot has the benefit of automatically persisting useful
test harnesses, such as docker images and ccache database. Although it
will lose some cleanness, it is imaginably useful for patchew.

Signed-off-by: Fam Zheng 
---
 tests/vm/basevm.py | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py
index 3643117816..e5d6a328d5 100755
--- a/tests/vm/basevm.py
+++ b/tests/vm/basevm.py
@@ -216,6 +216,8 @@ def parse_args(vm_name):
   help="build QEMU from source in guest")
 parser.add_option("--interactive", "-I", action="store_true",
   help="Interactively run command")
+parser.add_option("--snapshot", "-s", action="store_true",
+  help="run tests with a snapshot")
 parser.disable_interspersed_args()
 return parser.parse_args()
 
@@ -241,7 +243,10 @@ def main(vmcls):
jobs=args.jobs)]
 else:
 cmd = argv
-vm.boot(args.image + ",snapshot=on")
+img = args.image
+if args.snapshot:
+img += ",snapshot=on"
+vm.boot(img)
 vm.wait_ssh()
 except Exception as e:
 if isinstance(e, SystemExit) and e.code == 0:
-- 
2.17.1

Re: [Qemu-devel] [GIT PULL for qemu-pseries] pseries: Update SLOF firmware image

[David Gibson]

On Wed, Jul 11, 2018 at 08:53:05PM +1000, Alexey Kardashevskiy wrote:
> On Wed, 11 Jul 2018 16:26:19 +1000
> David Gibson  wrote:
> 
> > On Tue, Jul 10, 2018 at 05:14:53PM +1000, Alexey Kardashevskiy wrote:
> > > On Tue, 10 Jul 2018 16:42:48 +1000
> > > David Gibson  wrote:
> > >   
> > > > On Tue, Jul 10, 2018 at 12:22:44PM +1000, Alexey Kardashevskiy wrote:  
> > > > > On Mon,  2 Jul 2018 16:26:44 +1000
> > > > > Alexey Kardashevskiy  wrote:
> > > > > 
> > > > > > The following changes since commit 
> > > > > > edb1fb337f65f82fe32b989c4f018efe85c1dddb:
> > > > > > 
> > > > > >   pseries: Update SLOF firmware image (2018-07-02 16:21:30 +1000)
> > > > > > 
> > > > > > are available in the git repository at:
> > > > > > 
> > > > > >   g...@github.com:aik/qemu.git tags/qemu-slof-20180702
> > > > > > 
> > > > > > for you to fetch changes up to 
> > > > > > edb1fb337f65f82fe32b989c4f018efe85c1dddb:
> > > > > > 
> > > > > >   pseries: Update SLOF firmware image (2018-07-02 16:21:30 +1000)
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > I just missed a couple of gcc 8.1 fixes and thought I can still 
> > > > > > squeeze
> > > > > > them before the 3.0  soft freeze.
> > > > > > 
> > > > > > 
> > > > > > *** Note: this is not for master, this is for pseries
> > > > > 
> > > > > 
> > > > > Not good for 3.1 either?
> > > > 
> > > > Ugh.. somehow I missed this SLOF update request, and now we're in 3.0
> > > > hard freeze.
> > > > 
> > > > How bad are the bugs this SLOF update fixes?  
> > > 
> > > These only fix gcc 8.1 warnings and runtime errors => not really
> > > bugfixes so it is not a big deal if they miss 3.0.  
> > 
> > Ok, well, if you want to resend the update, I'll queue it for 3.1.
> 
> 
> It is still on github, why resend?

Good point, done.

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

[Qemu-devel] [PATCH v3 0/4] Add a CentOS test image to run docker tests

[Fam Zheng]

v3: Add 'make vm-test' document. [Phil]

v2: Drop archive-source.sh changes.
The new test depends on the iotests nbd fix I posted today to pass.

Docker testing on patchew has long suffered from 'make check' hangings. The
cleanness of VM testing is the cure. Now let's add a CentOS 7 image to run the
tests.  It's purely ad-hoc, but hopefully still easy to understand and use for
everyone.

The first patch makes passing source code from host to the container in VM
working, and is a nice clean up.

The second patch makes caches work, to speed up repetitive runs like on
patchew.

The last patch adds the new image that does the job. Two out of three docker
tests running on patchew.org are added to the image. I'll wait for Peter to fix
the 'docker-test-quick@centos6' hanging (oob test) before adding it too.

Fam

Fam Zheng (4):
  tests: Add an option for snapshot (default: off)
  tests/vm: Pass verbose flag into VM make commands
  tests: Allow overriding archive path with SRC_ARCHIVE
  tests: Add centos VM testing

 tests/docker/Makefile.include |  7 ++-
 tests/vm/Makefile.include |  3 +-
 tests/vm/basevm.py| 10 -
 tests/vm/centos   | 84 +++
 tests/vm/freebsd  |  4 +-
 tests/vm/netbsd   |  4 +-
 tests/vm/openbsd  |  4 +-
 tests/vm/ubuntu.i386  |  4 +-
 8 files changed, 107 insertions(+), 13 deletions(-)
 create mode 100755 tests/vm/centos

-- 
2.17.1

[Qemu-devel] [PATCH v3 3/4] tests: Allow overriding archive path with SRC_ARCHIVE

[Fam Zheng]

In VM based tests, the source archive is created in host, we don't have
to run archive-source.sh again, as it complicates the Makefile and
scripts.

Signed-off-by: Fam Zheng 
---
 tests/docker/Makefile.include | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
index b2a7e761cc..f0525b91a7 100644
--- a/tests/docker/Makefile.include
+++ b/tests/docker/Makefile.include
@@ -27,8 +27,11 @@ DOCKER_SRC_COPY := $(BUILD_DIR)/docker-src.$(CUR_TIME)
 
 $(DOCKER_SRC_COPY):
@mkdir $@
-   $(call quiet-command, cd $(SRC_PATH) && scripts/archive-source.sh 
$@/qemu.tar, \
-   "GEN", "$@/qemu.tar")
+   $(if $(SRC_ARCHIVE), \
+   $(call quiet-command, cp "$(SRC_ARCHIVE)" $@/qemu.tar, \
+   "CP", "$@/qemu.tar"), \
+   $(call quiet-command, cd $(SRC_PATH) && 
scripts/archive-source.sh $@/qemu.tar, \
+   "GEN", "$@/qemu.tar"))
$(call quiet-command, cp $(SRC_PATH)/tests/docker/run $@/run, \
"COPY","RUNNER")
 
-- 
2.17.1

[Qemu-devel] [Bug 1719339] Re: serial8250: too much work for irq3

[Paul Gear]

Further update: AWS kernel experienced the same error messages after
just over 3 hours of runtime.

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1719339

Title:
  serial8250: too much work for irq3

Status in QEMU:
  New

Bug description:
  It's know issue and sometimes mentioned since 2007. But it seems not
  fixed.

  http://lists.gnu.org/archive/html/qemu-devel/2008-02/msg00140.html
  https://bugzilla.redhat.com/show_bug.cgi?id=986761
  
http://old-list-archives.xenproject.org/archives/html/xen-devel/2009-02/msg00696.html

  I don't think fixes like increases PASS_LIMIT
  (/drivers/tty/serial/8250.c) or remove this annoying message
  (https://patchwork.kernel.org/patch/3920801/) is real fix. Some fix
  was proposed by H. Peter Anvin  https://lkml.org/lkml/2008/2/7/485.

  Can reproduce on Debian Strech host (Qemu 1:2.8+dfsg-6+deb9u2), Ubuntu
  16.04.2 LTS (Qemu 1:2.5+dfsg-5ubuntu10.15) also tried to use master
  branch (QEMU emulator version 2.10.50 (v2.10.0-766-ga43415ebfd-dirty))
  if we write a lot of message into console (dmesg or dd if=/dev/zero
  of=/dev/ttyS1).

  /usr/local/bin/qemu-system-x86_64 -name guest=ultra1,debug-threads=on
  -S -object
  secret,id=masterKey0,format=raw,file=/var/lib/libvirt/qemu/domain-27-ultra1
  /master-key.aes -machine pc-i440fx-2.8,accel=kvm,usb=off,dump-guest-
  core=off -cpu Skylake-
  
Client,ds=on,acpi=on,ss=on,ht=on,tm=on,pbe=on,dtes64=on,monitor=on,ds_cpl=on,vmx=on,smx=on,est=on,tm2=on,xtpr=on,pdcm=on,osxsave=on,tsc_adjust=on,clflushopt=on,pdpe1gb=on
  -m 4096 -realtime mlock=off -smp 4,sockets=1,cores=4,threads=1 -uuid
  4537ca29-73b2-40c3-9b43-666de182ba5f -display none -no-user-config
  -nodefaults -chardev
  
socket,id=charmonitor,path=/var/lib/libvirt/qemu/domain-27-ultra1/monitor.sock,server,nowait
  -mon chardev=charmonitor,id=monitor,mode=control -rtc
  base=utc,driftfix=slew -global kvm-pit.lost_tick_policy=delay -no-hpet
  -no-shutdown -global PIIX4_PM.disable_s3=1 -global
  PIIX4_PM.disable_s4=1 -boot strict=on -device ich9-usb-
  ehci1,id=usb,bus=pci.0,addr=0x8.0x7 -drive
  file=/home/dzagorui/csr/csr_disk.qcow2,format=qcow2,if=none,id=drive-
  ide0-0-0 -device ide-hd,bus=ide.0,unit=0,drive=drive-
  ide0-0-0,id=ide0-0-0,bootindex=1 -netdev tap,fd=26,id=hostnet0 -device
  e1000,netdev=hostnet0,id=net0,mac=52:54:00:a9:4c:86,bus=pci.0,addr=0x3
  -chardev
  socket,id=charserial0,host=127.0.0.1,port=4000,telnet,server,nowait
  -device isa-serial,chardev=charserial0,id=serial0 -chardev
  socket,id=charserial1,host=127.0.0.1,port=4001,telnet,server,nowait
  -device isa-serial,chardev=charserial1,id=serial1 -device virtio-
  balloon-pci,id=balloon0,bus=pci.0,addr=0x2 -msg timestamp=on

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1719339/+subscriptions

[Qemu-devel] [PATCH v2] Zero out the host's `msg_control` buffer

[Jonas Schievink]

If this is not done, qemu would drop any control message after the first
one.

This is because glibc's `CMSG_NXTHDR` macro accesses the uninitialized
cmsghdr's length field in order to find out if the message fits into the
`msg_control` buffer, wrongly assuming that it doesn't because the
length field contains garbage. Accessing the length field is fine for
completed messages we receive from the kernel, but is - as far as I know
- not needed since the kernel won't return such an invalid cmsghdr in
the first place.

This is tracked as this glibc bug:
https://sourceware.org/bugzilla/show_bug.cgi?id=13500

It's probably also a good idea to bail with an error if `CMSG_NXTHDR`
returns NULL but `TARGET_CMSG_NXTHDR` doesn't (ie. we still expect
cmsgs).

Signed-off-by: Jonas Schievink 
---
Changes in v2:
- put the memset right after the msg_control alloca
- added missing Signed-off-by line

 linux-user/syscall.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index e4b1b7d7da..3c427500ef 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -3843,6 +3843,8 @@ static abi_long do_sendrecvmsg_locked(int fd, struct 
target_msghdr *msgp,
 }
 msg.msg_controllen = 2 * tswapal(msgp->msg_controllen);
 msg.msg_control = alloca(msg.msg_controllen);
+memset(msg.msg_control, 0, msg.msg_controllen);
+
 msg.msg_flags = tswap32(msgp->msg_flags);
 
 count = tswapal(msgp->msg_iovlen);
-- 
2.18.0

[Qemu-devel] [Bug 1781281] Re: qemu-ppc64le uncaught target signal 4 (Illegal instruction)

[Laurent Vivier]

If it works fine on a POWER9 machine, you should try to run qemu-ppc64le
with "-cpu power9".

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1781281

Title:
  qemu-ppc64le uncaught target signal 4 (Illegal instruction)

Status in QEMU:
  New

Bug description:
  qemu-ppc64le version 2.12.0
  host machine: x86_64 Arch Linux 

  I'm currently working on VSX support in libVPX, I'm using qemu to
  test, on line 723 of vpx_dsp/ppc/loopfilter_vsx.c, when I change the
  vec_sub to vec_subs I get:

  qemu: uncaught target signal 4 (Illegal instruction) - core dumped

  Thread 1 "qemu-ppc64le" received signal SIGILL, Illegal instruction.
  0x766d1bf6 in sigsuspend () from /usr/lib/libc.so.6
  (gdb) bt
  #0  0x766d1bf6 in sigsuspend () from /usr/lib/libc.so.6
  #1  0x5567ee68 in ?? ()
  #2  0x5567fd18 in ?? ()
  #3  0x556805ef in process_pending_signals ()
  #4  0x55661e69 in cpu_loop ()
  #5  0x5561fd72 in main ()

  This can be reproduced by downloading this patch (patchset 1):

  https://chromium-review.googlesource.com/c/webm/libvpx/+/1134034

  and running

  qemu-ppc64le -L /home/ltrudeau/x-tools/powerpc64le-unknown-linux-gnu
  /powerpc64le-unknown-linux-gnu/sysroot/  ./test_libvpx
  --gtest_also_run_disabled_tests "--gtest_filter=VSX/*Loop*/*"

  I don't observe any issues when running the code on a POWER9 machine.

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1781281/+subscriptions

Re: [Qemu-devel] [PATCH v3] ui/cocoa.m: replace scrollingDeltaY with deltaY

[Peter Maydell]

On 9 July 2018 at 16:02, John Arbuckle  wrote:
> The NSEvent class method scrollingDeltaY is available
> for Mac OS 10.7 and newer. Since QEMU supports Mac OS
> 10.5 and up, we need to be using a method that is
> available on these version of Mac OS X. The deltaY
> method is a method that does the same thing as
> scrollingDeltaY and is available on Mac OS 10.5 and
> up. So we replace scrollingDeltaY with deltaY.
>
> We only check deltaY's value if it is not zero
> because zero means no scrolling took place.

Not quite -- it means that the scrolling was fine enough
that it doesn't add up to a big movement. If you use a
fine-scrolling input device (I used the trackpad on my
macbook air), then you will get a sequence of events
like this for a very slow trackpad scroll:

scroll: deltaY 0 scrollingDeltaY -1
scroll: deltaY 0 scrollingDeltaY -2
scroll: deltaY 0 scrollingDeltaY -1
scroll: deltaY 0 scrollingDeltaY -3
scroll: deltaY 0 scrollingDeltaY -1
scroll: deltaY 0 scrollingDeltaY -3
scroll: deltaY 0 scrollingDeltaY -1
scroll: deltaY 0 scrollingDeltaY -2
scroll: deltaY 0 scrollingDeltaY -2
scroll: deltaY 0 scrollingDeltaY -1
scroll: deltaY 0 scrollingDeltaY -3
scroll: deltaY 0 scrollingDeltaY -2
scroll: deltaY 0 scrollingDeltaY -1
scroll: deltaY 0 scrollingDeltaY -3
scroll: deltaY 0 scrollingDeltaY -1
scroll: deltaY 0 scrollingDeltaY -4
scroll: deltaY 0 scrollingDeltaY -2
scroll: deltaY 0 scrollingDeltaY -2
scroll: deltaY 0 scrollingDeltaY -2
scroll: deltaY 0 scrollingDeltaY 0

(interestingly it does give events with both deltas 0; maybe
that event had a horizontal scroll component to it).

Larger scroll gestures get you this sort of thing:

scroll: deltaY 0 scrollingDeltaY 0
scroll: deltaY 0 scrollingDeltaY -1
scroll: deltaY -3 scrollingDeltaY -31
scroll: deltaY -3 scrollingDeltaY -36
scroll: deltaY -2 scrollingDeltaY -30
scroll: deltaY 0 scrollingDeltaY 0
scroll: deltaY -1 scrollingDeltaY -16
scroll: deltaY -3 scrollingDeltaY -32
scroll: deltaY -3 scrollingDeltaY -30
scroll: deltaY -2 scrollingDeltaY -28
scroll: deltaY -2 scrollingDeltaY -25
scroll: deltaY -2 scrollingDeltaY -23
scroll: deltaY -1 scrollingDeltaY -19
scroll: deltaY -1 scrollingDeltaY -15
scroll: deltaY -1 scrollingDeltaY -14
scroll: deltaY 0 scrollingDeltaY -12

where OSX has decided that there's enough movement to
report a deltaY change, not just the fine-grained
scrollingDeltaY.

Anyway, since the QEMU input layer doesn't provide a mechanism
for reporting fine-scrolling I guess this patch is OK.
If we get complaints about the trackpad no longer being
as responsive as it used to be to scrolls then we can
look at doing something more complicated then.

I'll tweak the comments and put this in for 3.0 rc1.

> Signed-off-by: John Arbuckle 
> ---
> v3 changes:
> - Added a comment explaining why we drop scrolling events in both the code and
> the patch comment.
>
> v2 changes:
> - Added a condition that drops scroll events that have a deltaY value of zero.
>
>  ui/cocoa.m | 19 ---
>  1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/ui/cocoa.m b/ui/cocoa.m
> index 2991ed4f19..3bae090101 100644
> --- a/ui/cocoa.m
> +++ b/ui/cocoa.m
> @@ -802,14 +802,19 @@ - (void) handleEvent:(NSEvent *)event
>   * This is in-line with standard Mac OS X UI behaviour.
>   */
>
> +/*
> + * When deltaY is zero, it means the scrolling device did not 
> move
> + * for this event. So we drop the event.
> + */
> +if ([event deltaY] != 0) {
>  /* Determine if this is a scroll up or scroll down event */
> -buttons = ([event scrollingDeltaY] > 0) ?
> -INPUT_BUTTON_WHEEL_UP : INPUT_BUTTON_WHEEL_DOWN;
> -qemu_input_queue_btn(dcl->con, buttons, true);
> -qemu_input_event_sync();
> -qemu_input_queue_btn(dcl->con, buttons, false);
> -qemu_input_event_sync();
> -
> +buttons = ([event deltaY] > 0) ?
> +INPUT_BUTTON_WHEEL_UP : INPUT_BUTTON_WHEEL_DOWN;
> +qemu_input_queue_btn(dcl->con, buttons, true);
> +qemu_input_event_sync();
> +qemu_input_queue_btn(dcl->con, buttons, false);
> +qemu_input_event_sync();
> +}
>  /*
>   * Since deltaY also reports scroll wheel events we prevent mouse
>   * movement code from executing.
> --
> 2.14.3 (Apple Git-98)
>

thanks
-- PMM

[Qemu-devel] [Bug 1781281] [NEW] qemu-ppc64le uncaught target signal 4 (Illegal instruction)

[Luc]

Public bug reported:

qemu-ppc64le version 2.12.0
host machine: x86_64 Arch Linux 

I'm currently working on VSX support in libVPX, I'm using qemu to test,
on line 723 of vpx_dsp/ppc/loopfilter_vsx.c, when I change the vec_sub
to vec_subs I get:

qemu: uncaught target signal 4 (Illegal instruction) - core dumped

Thread 1 "qemu-ppc64le" received signal SIGILL, Illegal instruction.
0x766d1bf6 in sigsuspend () from /usr/lib/libc.so.6
(gdb) bt
#0  0x766d1bf6 in sigsuspend () from /usr/lib/libc.so.6
#1  0x5567ee68 in ?? ()
#2  0x5567fd18 in ?? ()
#3  0x556805ef in process_pending_signals ()
#4  0x55661e69 in cpu_loop ()
#5  0x5561fd72 in main ()

This can be reproduced by downloading this patch (patchset 1):

https://chromium-review.googlesource.com/c/webm/libvpx/+/1134034

and running

qemu-ppc64le -L /home/ltrudeau/x-tools/powerpc64le-unknown-linux-gnu
/powerpc64le-unknown-linux-gnu/sysroot/  ./test_libvpx
--gtest_also_run_disabled_tests "--gtest_filter=VSX/*Loop*/*"

I don't observe any issues when running the code on a POWER9 machine.

** 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/1781281

Title:
  qemu-ppc64le uncaught target signal 4 (Illegal instruction)

Status in QEMU:
  New

Bug description:
  qemu-ppc64le version 2.12.0
  host machine: x86_64 Arch Linux 

  I'm currently working on VSX support in libVPX, I'm using qemu to
  test, on line 723 of vpx_dsp/ppc/loopfilter_vsx.c, when I change the
  vec_sub to vec_subs I get:

  qemu: uncaught target signal 4 (Illegal instruction) - core dumped

  Thread 1 "qemu-ppc64le" received signal SIGILL, Illegal instruction.
  0x766d1bf6 in sigsuspend () from /usr/lib/libc.so.6
  (gdb) bt
  #0  0x766d1bf6 in sigsuspend () from /usr/lib/libc.so.6
  #1  0x5567ee68 in ?? ()
  #2  0x5567fd18 in ?? ()
  #3  0x556805ef in process_pending_signals ()
  #4  0x55661e69 in cpu_loop ()
  #5  0x5561fd72 in main ()

  This can be reproduced by downloading this patch (patchset 1):

  https://chromium-review.googlesource.com/c/webm/libvpx/+/1134034

  and running

  qemu-ppc64le -L /home/ltrudeau/x-tools/powerpc64le-unknown-linux-gnu
  /powerpc64le-unknown-linux-gnu/sysroot/  ./test_libvpx
  --gtest_also_run_disabled_tests "--gtest_filter=VSX/*Loop*/*"

  I don't observe any issues when running the code on a POWER9 machine.

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1781281/+subscriptions

[Qemu-devel] [PATCH v1 1/1] scsi-disk: Block Device Characteristics emulation fix

[Daniel Henrique Barboza]

The current BDC VPD page (page 0xb1) is too short. This can be
seen running sg_utils:

$ sg_vpd --page=bdc /dev/sda
Block device characteristics VPD page (SBC):
Block device characteristics VPD page length too short=8

By the SCSI spec, the expected size of the SBC page is 0x40.
There is no telling how the guest will behave with a shorter
message - it can ignore it, or worse, make (wrong)
assumptions.

This patch fixes the emulation by setting the size to 0x40.
This is the output of the previous sg_vpd command after
applying it:

$ sg_vpd --page=bdc /dev/sda -v
inquiry cdb: 12 01 b1 00 fc 00
Block device characteristics VPD page (SBC):
   [PQual=0  Peripheral device type: disk]
  Medium rotation rate is not reported
  Product type: Not specified
  WABEREQ=0
  WACEREQ=0
  Nominal form factor not reported
  FUAB=0
  VBULS=0

To improve readability, this patch also adds the VBULS value
explictly and add comments on the existing fields we're
setting.

Signed-off-by: Daniel Henrique Barboza 
---
 hw/scsi/scsi-disk.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 32f3f96ff8..5ae7baa082 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -775,11 +775,12 @@ int scsi_disk_emulate_vpd_page(SCSIRequest *req, uint8_t 
*outbuf)
 }
 case 0xb1: /* block device characteristics */
 {
-buflen = 8;
+buflen = 0x40;
 outbuf[4] = (s->rotation_rate >> 8) & 0xff;
 outbuf[5] = s->rotation_rate & 0xff;
-outbuf[6] = 0;
-outbuf[7] = 0;
+outbuf[6] = 0; /* PRODUCT TYPE */
+outbuf[7] = 0; /* WABEREQ | WACEREQ | NOMINAL FORM FACTOR */
+outbuf[8] = 0; /* VBULS */
 break;
 }
 case 0xb2: /* thin provisioning */
-- 
2.17.1

Re: [Qemu-devel] [PATCH] Zero out the host's `msg_control` buffer

[Philippe Mathieu-Daudé]

Hi Jonas,

You forgot to notify the maintainers, see
https://wiki.qemu.org/Contribute/SubmitAPatch#CC_the_relevant_maintainer :

./scripts/get_maintainer.pl -f linux-user/syscall.c
Riku Voipio  (maintainer:Linux user)
Laurent Vivier  (reviewer:Linux user)
qemu-devel@nongnu.org (open list:All patches CC here)

On 07/10/2018 08:32 PM, Jonas Schievink wrote:
> (Apparently I messed up my git config for the last email so it didn't
> send the correct name - please bear with me, this is my first time
> submitting a patch to a mailing list. I've also added a link to the
> upstream bug in the commit description.)
> 
> If this is not done, qemu would drop any control message after the first
> one.
> 
> This is because glibc's `CMSG_NXTHDR` macro accesses the uninitialized
> cmsghdr's length field in order to find out if the message fits into the
> `msg_control` buffer, wrongly assuming that it doesn't because the
> length field contains garbage. Accessing the length field is fine for
> completed messages we receive from the kernel, but is - as far as I know
> - not needed since the kernel won't return such an invalid cmsghdr in
> the first place.
> 
> This is tracked as this glibc bug:
> https://sourceware.org/bugzilla/show_bug.cgi?id=13500
> 
> It's probably also a good idea to bail with an error if `CMSG_NXTHDR`
> returns NULL but `TARGET_CMSG_NXTHDR` doesn't (ie. we still expect
> cmsgs).

This misses your Signed-off-by:

https://wiki.qemu.org/Contribute/SubmitAPatch#Patch_emails_must_include_a_Signed-off-by:_line

> ---
>  linux-user/syscall.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index e4b1b7d7da..77ce173b27 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -3845,6 +3845,8 @@ static abi_long do_sendrecvmsg_locked(int fd, struct 
> target_msghdr *msgp,
>  msg.msg_control = alloca(msg.msg_controllen);
>  msg.msg_flags = tswap32(msgp->msg_flags);
>  
> +memset(msg.msg_control, 0, msg.msg_controllen);
> +
>  count = tswapal(msgp->msg_iovlen);
>  target_vec = tswapal(msgp->msg_iov);

Do you mind swapping:

>  msg.msg_control = alloca(msg.msg_controllen);
> +memset(msg.msg_control, 0, msg.msg_controllen);
> +
>  msg.msg_flags = tswap32(msgp->msg_flags);

With your Signed-off-by:
Reviewed-by: Philippe Mathieu-Daudé 

Regards,

Phil.

Re: [Qemu-devel] [PATCH] linux-user: ppc64: use the correct values for F_*LK64s

[Laurent Vivier]

Le 11/07/2018 à 15:04, Laurent Vivier a écrit :
> Le 11/07/2018 à 12:55, Shivaprasad G Bhat a écrit :
>> Qemu includes the glibc headers for the host defines and target headers are
>> part of the qemu source themselves. The glibc has the F_GETLK64, F_SETLK64
>> and F_SETLKW64 defined to 12, 13 and 14 for all archs(generic) in
>> sysdeps/unix/sysv/linux/bits/fcntl-linux.h. The linux kernel generic
>> definition for F_*LK is 5, 6 & 7 and F_*LK64* is 12,13, and 14 as seen in
>> include/uapi/asm-generic/fcntl.h. On 64bit machine, by default the kernel
>> assumes all F_*LK to 64bit calls and doesnt support use of F_*LK64* as
>> can be seen in include/linux/fcntl.h in linux source.
>>
>> On x86_64 host, the values for F_*LK64* are set to 5, 6 and 7
>> explicitly in /usr/include/x86_64-linux-gnu/bits/fcntl.h by the glibc.
>> Whereas, a PPC64 host doesn't have such a definition in
>> /usr/include/powerpc64le-linux-gnu/bits/fcntl.h by the glibc. So,
>> the sources on PPC64 host sees the default value of F_*LK64*
>> as 12, 13 & 14(fcntl-linux.h).
>>
>> Since the 64bit kernel doesnt support 12, 13 & 14; the glibc fcntl syscall
>> implementation(__libc_fcntl*(), __fcntl64_nocancel) does the F_*LK64* value
>> convertion back to F_*LK* values on PPC64 as seen in
>> sysdeps/unix/sysv/linux/powerpc/powerpc64/sysdep.h with FCNTL_ADJUST_CMD()
>> macro. Whereas on x86_64 host the values for F_*LK64* are set to 5, 6 and 7
>> and no adjustments are needed.
>>
>> Since qemu doesnt use the glibc fcntl, but makes the safe_syscall* on its
>> own, the PPC64 qemu is calling the syscall with 12, 13, and 14(without
>> adjustment) and they all fail. The fcntl calls to F_GETLK/F_SETLK|W all
>> fail by all pplications run on PPC64 host user emulation.
>>
>> The fix here could be to see why on PPC64 the glibc is still keeping
>> F_*LK64* different from F_*LK and why adjusting them to 5, 6 and 7 before
>> the syscall for PPC only. See if we can make the
>> /usr/include/powerpc64le-linux-gnu/bits/fcntl.h to have the values
>> 5, 6 & 7 just like x86_64 and remove the adjustment code in glibc. That way,
>> qemu sources see the kernel supported values in glibc headers.
>>
>> OR
>>
>> On PPC64 host, qemu sources see both F_LK* & F_LK64* as same and set to
>> 12, 13 and 14 because __USE_FILE_OFFSET64 is defined in qemu
>> sources(also refer sysdeps/unix/sysv/linux/bits/fcntl-linux.h).
>> Since F_*LK and F_*LK64 are same, the value adjument like done by glibc in
>> qemu sources is difficult. So, Overwrite the glibc defaults with the actual
>> supported values in Qemu. The current patch is doing this.
>>
>> Signed-off-by: Shivaprasad G Bhat 
>> ---
>>  linux-user/syscall.c |   14 ++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>> index 7b9ac3b408..1693e69ce0 100644
>> --- a/linux-user/syscall.c
>> +++ b/linux-user/syscall.c
>> @@ -250,6 +250,20 @@ static type name (type1 arg1,type2 arg2,type3 
>> arg3,type4 arg4,type5 arg5,   \
>>  #define TARGET_NR__llseek TARGET_NR_llseek
>>  #endif
>>  
>> +/* glibc headers has these defined to 12, 13 and 14 and is not supported
>> + * by kernel. The glibc fcntl call actually adjusts them back to 5, 6 and 7
>> + * before making the syscall(). Since we make the syscall directly,
>> + * overwite/adjust to what is supported by the kernel.
>> + */
>> +#if defined(__linux__) && defined(__powerpc64__)
>> +#undef F_GETLK64
>> +#define F_GETLK64  5   /* Get record locking info.  */
>> +#undef F_SETLK64
>> +#define F_SETLK64  6   /* Set record locking info (non-blocking).  
>> */
>> +#undef F_SETLKW64
>> +#define F_SETLKW64 7   /* Set record locking info (blocking).  */
>> +#endif
>> +
>>  #ifdef __NR_gettid
>>  _syscall0(int, gettid)
>>  #else
>>
> 
> These macros are used in target_to_host_fcntl_cmd(), and this function
> is used with safe_fcntl() and fcntl().
> 
> So I think it would be cleaner to do the change after
> target_to_host_fcntl_cmd() in do_fcntl() as it is done in glibc instead
> of redefining system values. Something like:
> 
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -6782,6 +6782,12 @@ static abi_long do_fcntl(int fd, int cmd,
> abi_ulong arg)
>  if (host_cmd == -TARGET_EINVAL)
> return host_cmd;
> 
> +#if defined(__linux__) && defined(__powerpc64__)
> +if (host_cmd >= F_GETLK64 && host_cmd <= F_SETLKW64) {
> +host_cmd -= F_GETLK64 - F_GETLK;

But as you said, __USE_FILE_OFFSET64 is defined in qemu, and F_GETLK is
equal to F_GETLK64, so we should use something like:

...
host_cmd -= F_GETLK64 - 5;
...

Thanks,
Laurent

Re: [Qemu-devel] [PATCH v2] linux-user: fix mmap_find_vma_reserved()

[Richard Henderson]

On 07/11/2018 01:27 PM, Laurent Vivier wrote:
> Richard,
> 
> I think this fix could be merged into your "linux-user: Fix shmat
> emulation by honoring host SHMLBA" patch, by adding something like this
> instead:

Well, not "instead", but "in addition".

Nothing works right when the guest adjustment itself is unaligned, as is the
case with reserved_va as you noted, and with (non-reserved) guest_base as I
noted in that patch.

For v2, I'll split the guest_base fix out to a separate patch.


r~

Re: [Qemu-devel] [PATCH 3/7] tests/qgraph: sdhci driver and interface nodes

[Emanuele]

Hi Philippe,

On 07/11/2018 10:13 PM, Philippe Mathieu-Daudé wrote:

Hi Emanuele,

On 07/09/2018 06:11 AM, Emanuele Giuseppe Esposito wrote:

Add qgraph nodes for sdhci-pci and generic-sdhci (memory mapped) drivers.
Both drivers implement (produce) the same interface sdhci, that provides the
readw - readq - writeq functions.

Signed-off-by: Emanuele Giuseppe Esposito 
---
  tests/Makefile.include |   1 +
  tests/libqos/sdhci.c   | 142 +
  tests/libqos/sdhci.h   |  68 
  3 files changed, 211 insertions(+)
  create mode 100644 tests/libqos/sdhci.c
  create mode 100644 tests/libqos/sdhci.h

diff --git a/tests/Makefile.include b/tests/Makefile.include
index b16bbd55df..acbf704a8a 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -770,6 +770,7 @@ libqos-usb-obj-y = $(libqos-spapr-obj-y) $(libqos-pc-obj-y) 
tests/libqos/usb.o
  libqos-virtio-obj-y = $(libqos-spapr-obj-y) $(libqos-pc-obj-y) 
tests/libqos/virtio.o tests/libqos/virtio-pci.o tests/libqos/virtio-mmio.o 
tests/libqos/malloc-generic.o
  
  libqgraph-obj-y = tests/libqos/qgraph.o

+libqgraph-pc-obj-y = $(libqos-pc-obj-y) tests/libqos/sdhci.o

Shouldn't this be:

libqgraph-obj-y = tests/libqos/qgraph.o tests/libqos/sdhci.o

(not PC related)
I need to add $(libqos-pc-obj-y) because it will include pc-pci.c and 
all the pci targerts, loading their libqos_init() functions and building 
the graph.


Doing as you suggested won't work for these reasons:
- pci-bus-pc and pci-bus nodes won't be created, so graph would be 
incomplete
- sdhci needs libqos to use global_qtest,  qpci_device_init, etc. that 
won't be provided by check-unit-y even by adding $(libqos-pc-obj-y).


So since test-qgraph do not need libqos, I (suggested by Paolo) added 
the test to the check-unit-y target, adding to it just libqgraph-obj.


For other devices like sdhci that need libqos, I created 
libqgraph-pc-obj-y (to be renamed to libqgraph-pci-obj-y, since it will 
also contain libqos-spapr ?) that will be added to target 
check-qtest-pci-y (that provides libqos).

+QSDHCIProperties props;
+};
+
+/* Memory Mapped implementation of QSDHCI */
+struct QSDHCI_MemoryMapped {
+QOSGraphObject obj;
+QSDHCI sdhci;
+uint64_t addr;
+};
+
+/* PCI implementation of QSDHCI */
+struct QSDHCI_PCI {
+QOSGraphObject obj;
+QPCIDevice dev;
+QSDHCI sdhci;
+QPCIBar mem_bar;
+};
+
+/**
+ * qos_create_sdhci_mm(): external constructor used by all drivers/machines
+ * that "contain" a #QSDHCI_MemoryMapped driver
+ */
+void qos_create_sdhci_mm(QSDHCI_MemoryMapped *sdhci, uint32_t addr,
+QSDHCIProperties *common);

Your IDE uses a weird indentation.
I actually did by hand, not sure how the indentation to split >80 lines 
should be.

Maybe I should remove a tab? Like this:

+void qos_create_sdhci_mm(QSDHCI_MemoryMapped *sdhci, uint32_t addr,
+    QSDHCIProperties *common);

Re: [Qemu-devel] [PATCH v2] linux-user: fix mmap_find_vma_reserved()

[Richard Henderson]

On 07/11/2018 09:40 AM, Laurent Vivier wrote:
> The value given by mmap_find_vma_reserved() is used with mmap(),
> so it is needed to be aligned with the host page size.
> 
> Since commit 18e80c55bb, reserved_va is only aligned to TARGET_PAGE_SIZE,
> and it works well if this size is greater or equal to the host page size.
> 
> But ppc64 hosts have 64kB page size and when we start a 4kiB page size
> guest (like i386), it fails when it tries to mmap the stack:
> 
> mmap stack: Invalid argument
> 
> Fixes: 18e80c55bb (linux-user: Tidy and enforce reserved_va initialization)
> Signed-off-by: Laurent Vivier 
> ---
> 
> Notes:
> v2:
>   fix typo s/has/as/
> 
>  linux-user/main.c | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/linux-user/main.c b/linux-user/main.c
> index 52b5a618fe..15299e9dd7 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -689,6 +689,11 @@ int main(int argc, char **argv, char **envp)
>  target_environ = envlist_to_environ(envlist, NULL);
>  envlist_free(envlist);
>  
> +/* reserved_va must be aligned with the host page size
> + * as it is used with mmap()
> + */
> +reserved_va &= qemu_host_page_mask;
> +

So... this silently overrides the command-line argument.  The current code is
only a problem because we assign the default to a global variable, which must
be a compile-time constant.

I wonder if it's worth add an error message in handle_arg_reserved_va, and
moving the default initialization logic from the global variable to here, as

if (HOST_LONG_BITS == 64 &&
TARGET_VIRT_ADDR_SPACE_BITS <= 32 &&
reserved_va == 0) {
reserved_va = MAX_RESERVED_VA & qemu_host_page_mask;
}

merging your comment with the moved comment from the global variable init.


r~

[Qemu-devel] [Bug 1781280] [NEW] QEMU ignores all but the first control message sent over a Unix socket

[Jonas Schievink]

Public bug reported:

I've written a test program that sends both an SCM_CREDENTIALS and an
SCM_RIGHTS cmsg in the same sendmsg call. On native x86-64, armv6 and
armv7 Linux, this works as expected (the recvmsg receives both control
messages). On QEMU (both qemu-x86_64 and qemu-arm), only the first
message is received.

I've traced the problem back to a glibc bug:
https://sourceware.org/bugzilla/show_bug.cgi?id=13500

This means that writing control messages into an uninitialized buffer
makes CMSG_NXTHDR erroneously return NULL even though there's still
space inside the allocated buffer. QEMU's logic inside
`target_to_host_cmsg` is a bit questionable here, since it stops
encoding cmsgs as soon as *either* the host or the target buffer reaches
its end, so we lose the target's cmsgs when the host's buffer runs out.
However, the host buffer should *never* reach its end before the target
buffer does, so an assertion might be more useful there. Anyway, the
actual fix for this bug is simply zeroing out the buffer created for the
host. I've attached a patch doing that and verified that it fixes the
issue.

The test program I used can be found here: https://gist.github.com
/jonas-schievink/cb6e6584a055539d2113f22d91068e2d

** Affects: qemu
 Importance: Undecided
 Status: New

** Patch added: "0001-Zero-out-the-host-s-msg_control-buffer.patch"
   
https://bugs.launchpad.net/bugs/1781280/+attachment/5162557/+files/0001-Zero-out-the-host-s-msg_control-buffer.patch

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1781280

Title:
  QEMU ignores all but the first control message sent over a Unix socket

Status in QEMU:
  New

Bug description:
  I've written a test program that sends both an SCM_CREDENTIALS and an
  SCM_RIGHTS cmsg in the same sendmsg call. On native x86-64, armv6 and
  armv7 Linux, this works as expected (the recvmsg receives both control
  messages). On QEMU (both qemu-x86_64 and qemu-arm), only the first
  message is received.

  I've traced the problem back to a glibc bug:
  https://sourceware.org/bugzilla/show_bug.cgi?id=13500

  This means that writing control messages into an uninitialized buffer
  makes CMSG_NXTHDR erroneously return NULL even though there's still
  space inside the allocated buffer. QEMU's logic inside
  `target_to_host_cmsg` is a bit questionable here, since it stops
  encoding cmsgs as soon as *either* the host or the target buffer
  reaches its end, so we lose the target's cmsgs when the host's buffer
  runs out. However, the host buffer should *never* reach its end before
  the target buffer does, so an assertion might be more useful there.
  Anyway, the actual fix for this bug is simply zeroing out the buffer
  created for the host. I've attached a patch doing that and verified
  that it fixes the issue.

  The test program I used can be found here: https://gist.github.com
  /jonas-schievink/cb6e6584a055539d2113f22d91068e2d

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1781280/+subscriptions

Re: [Qemu-devel] [PATCH] hw/arm/bcm283x: Fix crash with device_add bcm2837 on unsupported machines

[Eduardo Habkost]

On Wed, Jul 11, 2018 at 10:16:42PM +0200, Paolo Bonzini wrote:
> On 11/07/2018 20:30, Eduardo Habkost wrote:
> >> The theoretical behavior should be:
> > It's not clear below where you expect
> >   qdev_set_parent_bus(..., sysbus_get_default())
> > to be called (if it should be called at all).
> > 
> > I don't know where it should be called, but I'm absolutely sure
> > instance_init is not the right place.
> 
> I think instance_init is fine to call qdev_set_parent_bus on contained
> devices.  Why do you say it's not?

Because object_unref(object_new(...)) is not supposed to affect
QEMU global state at all.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 4/7] tests/qgraph: arm/raspi2 machine node

[Philippe Mathieu-Daudé]

On 07/11/2018 12:30 PM, Paolo Bonzini wrote:
> On 11/07/2018 16:59, Stefan Hajnoczi wrote:
>>> +machine->obj.get_device = raspi2_get_device;
>>> +machine->obj.destructor = raspi2_destroy;
>>> +qos_create_sdhci_mm(>sdhci, 0x3f30, &(QSDHCIProperties) {
>>> +.version = 3,
>>> +.baseclock = 52,
>>> +.capab.sdma = false,
>>> +.capab.reg = 0x052134b4
>>> +});
>>> +return >obj;
>>> +}
>>> +
>>> +static void raspi2(void)
>>> +{
>>> +qos_node_create_machine("arm/raspi2", qos_create_machine_arm_raspi2);
>>> +qos_node_contains("arm/raspi2", "generic-sdhci");
>>> +}
>>> +
>>> +libqos_init(raspi2);
>> Hmm...I didn't realize it would be necessary to manually define each
>> machine type.  I thought qgraph would use qemu-system-x86_64
>> -machine/-device/info qtree to introspect QEMU and instantiate the
>> appropriate drivers.
> 
> That doesn't provide enough information yet.  However, PCI devices are
> already discovered automatically and the next step (next year :)) could
> be to use device tree or ACPI to discover embedded devices.
> 
> Using QOM properties instead of duplicating things like QSDHCIProperties
> is a great idea.  Of course the duplication was already there ("old
> sdhci_t structure" in patch 7), so I don't think it should be a blocker,
> but indeed there's a better way to do it.
> 
>> My concern is that this manual approach of defining machines complicates
>> qtests.  This file will need to be modified by multiple people - each of
>> them will be interested in a specific driver and not interested in the
>> driver that other people have added.
> 
> This is exactly the opposite problem that we have now: when you write a
> test you are interested typically only in one machine, and the
> "if(x86)elseif(arm)" gets in the way.  (Also: it's also difficult to
> split the ifs across files, i.e. it scales worse; and there's lots of
> duplicated code across tests to do "for" loops of g_test_add, because
> the ifs don't lend itself to "automatic" generation of test cases via
> path walk).
> 
> The advantage is that (just like for OS vs. QEMU) the structure of this
> file matches closely the board files in hw/arm/, which is something you
> should already be familiar with if you're adding a new board to QEMU.

Some projects organize their tests alongside the source to be tested.
This is often messy, but might be clearer to see source testing coverage.
That said, I'd rather keep it out of 'source' tree (as of now). Using a
simple script we can notice the same (which device models matches a test).

Re: [Qemu-devel] [PATCH] hw/arm/bcm283x: Fix crash with device_add bcm2837 on unsupported machines

[Paolo Bonzini]

On 11/07/2018 20:30, Eduardo Habkost wrote:
>> The theoretical behavior should be:
> It's not clear below where you expect
>   qdev_set_parent_bus(..., sysbus_get_default())
> to be called (if it should be called at all).
> 
> I don't know where it should be called, but I'm absolutely sure
> instance_init is not the right place.

I think instance_init is fine to call qdev_set_parent_bus on contained
devices.  Why do you say it's not?

Thanks,

Paolo

Re: [Qemu-devel] [PATCH] hw/arm/bcm283x: Fix crash with device_add bcm2837 on unsupported machines

[Paolo Bonzini]

On 11/07/2018 20:43, Thomas Huth wrote:
>>
>> - realize fails
> In this case, the failure is before realize is attempted,
> qdev_device_add() already stop with "Device '%s' can not be hotplugged
> on this machine".

Still, object_unparent is called by qdev_device_add in the error path,
and it should work the same way (in a nutshell, recursive unparent when
child properties are deleted, and finalization of the contained objects
as the last reference is dropped).

>> - object_unparent is called on the device that failed to realize (see
>> qdev_device_add).  object_unparent calls device_unparent
> Hmm, are you sure? I can see that object_unparent calls device_unparent
> indirectly for the *child* nodes of the device, but not for the device
> itself...

object_unparent -> object_property_del_child ->
object_finalize_child_property -> device_unparent

I think you're on the right track, after object_property_add_child you
need to drop the reference to the object.  For example qmp_device_add
does it after qdev_device_add returns a device successfully (just an
example---I understand it is not the case with bcm283x).  In that case
the call to object_property_add_child is in qdev_set_id.

Paolo

Re: [Qemu-devel] [PATCH 3/7] tests/qgraph: sdhci driver and interface nodes

[Philippe Mathieu-Daudé]

Hi Emanuele,

On 07/09/2018 06:11 AM, Emanuele Giuseppe Esposito wrote:
> Add qgraph nodes for sdhci-pci and generic-sdhci (memory mapped) drivers.
> Both drivers implement (produce) the same interface sdhci, that provides the
> readw - readq - writeq functions.
> 
> Signed-off-by: Emanuele Giuseppe Esposito 
> ---
>  tests/Makefile.include |   1 +
>  tests/libqos/sdhci.c   | 142 +
>  tests/libqos/sdhci.h   |  68 
>  3 files changed, 211 insertions(+)
>  create mode 100644 tests/libqos/sdhci.c
>  create mode 100644 tests/libqos/sdhci.h
> 
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index b16bbd55df..acbf704a8a 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -770,6 +770,7 @@ libqos-usb-obj-y = $(libqos-spapr-obj-y) 
> $(libqos-pc-obj-y) tests/libqos/usb.o
>  libqos-virtio-obj-y = $(libqos-spapr-obj-y) $(libqos-pc-obj-y) 
> tests/libqos/virtio.o tests/libqos/virtio-pci.o tests/libqos/virtio-mmio.o 
> tests/libqos/malloc-generic.o
>  
>  libqgraph-obj-y = tests/libqos/qgraph.o
> +libqgraph-pc-obj-y = $(libqos-pc-obj-y) tests/libqos/sdhci.o

Shouldn't this be:

   libqgraph-obj-y = tests/libqos/qgraph.o tests/libqos/sdhci.o

(not PC related)

>  
>  check-unit-y += tests/test-qgraph$(EXESUF)
>  tests/test-qgraph$(EXESUF): tests/test-qgraph.o $(libqgraph-obj-y)
> diff --git a/tests/libqos/sdhci.c b/tests/libqos/sdhci.c
> new file mode 100644
> index 00..213c2657c3
> --- /dev/null
> +++ b/tests/libqos/sdhci.c
> @@ -0,0 +1,142 @@
> +/*
> + * libqos driver framework
> + *
> + * Copyright (c) 2018 Emanuele Giuseppe Esposito 
> 
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License version 2 as published by the Free Software Foundation.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see 
> 
> + */
> +
> +#include "qemu/osdep.h"
> +#include "libqtest.h"
> +#include "pci.h"
> +#include "pci-pc.h"
> +#include "qgraph.h"
> +#include "sdhci.h"
> +#include "hw/pci/pci.h"
> +
> +static void set_qsdhci_fields(QSDHCI *s, uint8_t version, uint8_t baseclock,
> +bool sdma, uint64_t reg)
> +{
> +s->props.version = version;
> +s->props.baseclock = baseclock;
> +s->props.capab.sdma = sdma;
> +s->props.capab.reg = reg;
> +}
> +
> +/* Memory mapped implementation of QSDHCI */
> +
> +static uint16_t sdhci_mm_readw(QSDHCI *s, uint32_t reg)
> +{
> +QSDHCI_MemoryMapped *smm = container_of(s, QSDHCI_MemoryMapped, sdhci);
> +return qtest_readw(global_qtest, smm->addr + reg);
> +}
> +
> +static uint64_t sdhci_mm_readq(QSDHCI *s, uint32_t reg)
> +{
> +QSDHCI_MemoryMapped *smm = container_of(s, QSDHCI_MemoryMapped, sdhci);
> +return qtest_readq(global_qtest, smm->addr + reg);
> +}
> +
> +static void sdhci_mm_writeq(QSDHCI *s, uint32_t reg, uint64_t val)
> +{
> +QSDHCI_MemoryMapped *smm = container_of(s, QSDHCI_MemoryMapped, sdhci);
> +qtest_writeq(global_qtest, smm->addr + reg, val);
> +}
> +
> +static void *sdhci_mm_get_driver(void *obj, const char *interface)
> +{
> +QSDHCI_MemoryMapped *spci = obj;
> +if (!g_strcmp0(interface, "sdhci")) {
> +return >sdhci;
> +}
> +printf("%s not present in generic-sdhci\n", interface);
> +abort();
> +}
> +
> +void qos_create_sdhci_mm(QSDHCI_MemoryMapped *sdhci, uint32_t addr,
> +QSDHCIProperties *common)
> +{
> +sdhci->obj.get_driver = sdhci_mm_get_driver;
> +sdhci->sdhci.sdhci_readw = sdhci_mm_readw;

Lot of 'shdci'!

I'd name the argument 'QSDHCI_MemoryMapped *smm' to be consistent with
the previous sdhci_mm_read/write*(), that'd become more readable IMO:

   smm->sdhci.readw = sdhci_mm_readw;

> +sdhci->sdhci.sdhci_readq = sdhci_mm_readq;
> +sdhci->sdhci.sdhci_writeq = sdhci_mm_writeq;
> +memcpy(>sdhci.props, common, sizeof(QSDHCIProperties));
> +sdhci->addr = addr;
> +}
> +
> +/* PCI implementation of QSDHCI */
> +
> +static uint16_t sdhci_pci_readw(QSDHCI *s, uint32_t reg)
> +{
> +QSDHCI_PCI *spci = container_of(s, QSDHCI_PCI, sdhci);
> +return qpci_io_readw(>dev, spci->mem_bar, reg);
> +}
> +
> +static uint64_t sdhci_readq(QSDHCI *s, uint32_t reg)
> +{
> +QSDHCI_PCI *spci = container_of(s, QSDHCI_PCI, sdhci);
> +return qpci_io_readq(>dev, spci->mem_bar, reg);
> +}
> +
> +static void sdhci_writeq(QSDHCI *s, uint32_t reg, uint64_t val)
> +{
> +QSDHCI_PCI *spci = container_of(s, QSDHCI_PCI, sdhci);
> +return qpci_io_writeq(>dev, spci->mem_bar, reg, 

Re: [Qemu-devel] [PATCH for-3.0] target/arm: Fix LD1W and LDFF1W (scalar plus vector)

[Richard Henderson]

On 07/11/2018 04:04 AM, Laurent Desnogues wrote:
> Hello,
> 
> On Wed, Jul 11, 2018 at 12:39 PM, Richard Henderson
>  wrote:
>> 'I' was being double-incremented; correctly within the inner loop
>> and incorrectly within the outer loop.
>>
>> Signed-off-by: Richard Henderson 
> 
> I didn't try to apply the patch but line numbers look wrong.
> 
> I guess this should apply to DO_LD1_ZPZ_S and DO_LDFF1_ZPZ macros, in
> which case you get my review:

The patch was pulled out of the middle of a branch,
so the line numbers might be off.  I probably should
have cherry-picked it to master before posting...


r~

Re: [Qemu-devel] [PATCH 2/7] tests/qgraph: pci-pc driver and interface nodes

[Philippe Mathieu-Daudé]

Hi Emanuele,

On 07/09/2018 06:11 AM, Emanuele Giuseppe Esposito wrote:
> Add pci-bus-pc node and pci-bus interface, moved QPCIBusPC struct

"move"

> declaration in its header (since it will be needed by other drivers)
> and introduced a setter method for drivers that do not need to allocate

"introduce"

> but have to initialize QPCIBusPC.
> 
> Signed-off-by: Emanuele Giuseppe Esposito 
> ---
>  tests/libqos/pci-pc.c | 53 ---
>  tests/libqos/pci-pc.h |  8 +++
>  tests/libqos/pci.c|  8 +++
>  3 files changed, 61 insertions(+), 8 deletions(-)
> 
> diff --git a/tests/libqos/pci-pc.c b/tests/libqos/pci-pc.c
> index a7803308b7..f1c1741279 100644
> --- a/tests/libqos/pci-pc.c
> +++ b/tests/libqos/pci-pc.c
> @@ -18,15 +18,9 @@
>  
>  #include "qemu-common.h"
>  
> -
>  #define ACPI_PCIHP_ADDR 0xae00
>  #define PCI_EJ_BASE 0x0008
>  
> -typedef struct QPCIBusPC
> -{
> -QPCIBus bus;
> -} QPCIBusPC;
> -
>  static uint8_t qpci_pc_pio_readb(QPCIBus *bus, uint32_t addr)
>  {
>  return inb(addr);
> @@ -115,10 +109,33 @@ static void qpci_pc_config_writel(QPCIBus *bus, int 
> devfn, uint8_t offset, uint3
>  outl(0xcfc, value);
>  }
>  
> -QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc)
> +static void *qpci_get_driver(void *obj, const char *interface)
>  {
> -QPCIBusPC *ret = g_new0(QPCIBusPC, 1);
> +QPCIBusPC *qpci = obj;
> +if (!g_strcmp0(interface, "pci-bus")) {
> +return >bus;
> +}
> +printf("%s not present in pci-bus-pc", interface);
> +abort();
> +}
>  
> +void qpci_device_init(QPCIDevice *dev, QPCIBus *bus, int devfn)

Maybe this one belongs to "pci.c" (not being 'pc' related).

> +{
> +if (!bus) {
> +return;
> +}
> +dev->bus = bus;
> +dev->devfn = devfn;
> +
> +if (qpci_config_readw(dev, PCI_VENDOR_ID) == 0x) {
> +printf("PCI Device not found\n");
> +abort();
> +}
> +qpci_device_enable(dev);
> +}
> +
> +void qpci_set_pc(QPCIBusPC *ret, QTestState *qts, QGuestAllocator *alloc)
> +{
>  assert(qts);
>  
>  ret->bus.pio_readb = qpci_pc_pio_readb;
> @@ -147,11 +164,23 @@ QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator 
> *alloc)
>  ret->bus.mmio_alloc_ptr = 0xE000;
>  ret->bus.mmio_limit = 0x1ULL;
>  
> +ret->obj.get_driver = qpci_get_driver;
> +}
> +
> +QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc)
> +{
> +QPCIBusPC *ret = g_new0(QPCIBusPC, 1);
> +qpci_set_pc(ret, qts, alloc);
> +
>  return >bus;
>  }
>  
>  void qpci_free_pc(QPCIBus *bus)
>  {
> +if (!bus) {
> +return;
> +}
> +
>  QPCIBusPC *s = container_of(bus, QPCIBusPC, bus);
>  
>  g_free(s);
> @@ -176,3 +205,11 @@ void qpci_unplug_acpi_device_test(const char *id, 
> uint8_t slot)
>  
>  qmp_eventwait("DEVICE_DELETED");
>  }
> +
> +static void qpci_pc(void)
> +{
> +qos_node_create_driver("pci-bus-pc", NULL);
> +qos_node_produces("pci-bus-pc", "pci-bus");
> +}
> +
> +libqos_init(qpci_pc);
> diff --git a/tests/libqos/pci-pc.h b/tests/libqos/pci-pc.h
> index 491eeac756..ee381c5667 100644
> --- a/tests/libqos/pci-pc.h
> +++ b/tests/libqos/pci-pc.h
> @@ -15,7 +15,15 @@
>  
>  #include "libqos/pci.h"
>  #include "libqos/malloc.h"
> +#include "qgraph.h"
>  
> +typedef struct QPCIBusPC {
> +QOSGraphObject obj;
> +QPCIBus bus;
> +} QPCIBusPC;
> +
> +void qpci_device_init(QPCIDevice *dev, QPCIBus *bus, int devfn);

Ditto, belongs to "libqos/pci.h".

> +void qpci_set_pc(QPCIBusPC *ret, QTestState *qts, QGuestAllocator *alloc);
>  QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc);
>  void qpci_free_pc(QPCIBus *bus);
>  
> diff --git a/tests/libqos/pci.c b/tests/libqos/pci.c
> index 0b73cb23d0..c51c186867 100644
> --- a/tests/libqos/pci.c
> +++ b/tests/libqos/pci.c
> @@ -15,6 +15,7 @@
>  
>  #include "hw/pci/pci_regs.h"
>  #include "qemu/host-utils.h"
> +#include "qgraph.h"
>  
>  void qpci_device_foreach(QPCIBus *bus, int vendor_id, int device_id,
>   void (*func)(QPCIDevice *dev, int devfn, void 
> *data),
> @@ -402,3 +403,10 @@ void qpci_plug_device_test(const char *driver, const 
> char *id,
>  qtest_qmp_device_add(driver, id, "'addr': '%d'%s%s", slot,
>   opts ? ", " : "", opts ? opts : "");
>  }
> +
> +static void qpci(void)
> +{
> +qos_node_create_interface("pci-bus");
> +}
> +
> +libqos_init(qpci);
>

Re: [Qemu-devel] [PATCH] hw/arm/bcm283x: Fix crash with device_add bcm2837 on unsupported machines

[Eduardo Habkost]

On Wed, Jul 11, 2018 at 09:04:35PM +0200, Thomas Huth wrote:
> On 11.07.2018 19:21, Paolo Bonzini wrote:
> > On 10/07/2018 08:50, Peter Maydell wrote:
>  Yuck. The real problem here is that we're still requiring the
>  code that creates these QOM devices to manually set the parent
>  in the first place. It's not surprising that we don't get it right
>  (either parenting in the wrong place or not at all). I'd much
>  rather see us fix that properly than keep papering over places
>  where we get it wrong.
> >>> Sorry, I'm still not an expert in all this QOM stuff yet ... so what do
> >>> you exactly recommend to do instead?
> >> I'm not clear either, but I don't think that what we're
> >> currently doing can be right.
> > 
> > Well, in theory it should work...  I sent the expected flow in another 
> > email.
> 
> Something that just came to my mind:
> 
> bcm2836_init() creates the TYPE_BCM2835_PERIPHERALS object with
> object_initialize(). This creates one reference to the object already.
> Then the object is linked to its parent with
> object_property_add_child(), which creates another reference to the
> object. But where are the two references correctly destroyed again? One
> is certainly destroyed by device_unparent later, but the initial one?
> Could it be that we are simply lacking one object_unref() after the
> object_property_add_child() here?

This seems to be true, but I'm confused about the reference
counting model, here:

What exactly guarantees there will be no other references to
(e.g.) `>control` when `s` is freed?

We know the references added by object_initialize(),
object_property_add_child() and qdev_set_parent_bus() will be
dropped, but what about other code calling object_ref()?

-- 
Eduardo

[Qemu-devel] [PULL 1/1] vfio/pci: do not set the PCIDevice 'has_rom' attribute

[Alex Williamson]

From: Cédric Le Goater 

PCI devices needing a ROM allocate an optional MemoryRegion with
pci_add_option_rom(). pci_del_option_rom() does the cleanup when the
device is destroyed. The only action taken by this routine is to call
vmstate_unregister_ram() which clears the id string of the optional
ROM RAMBlock and now, also flags the RAMBlock as non-migratable. This
was recently added by commit b895de502717 ("migration: discard
non-migratable RAMBlocks"), .

VFIO devices do their own loading of the PCI option ROM in
vfio_pci_size_rom(). The memory region is switched to an I/O region
and the PCI attribute 'has_rom' is set but the RAMBlock of the ROM
region is not allocated. When the associated PCI device is deleted,
pci_del_option_rom() calls vmstate_unregister_ram() which tries to
flag a NULL RAMBlock, leading to a SEGV.

It seems that 'has_rom' was set to have memory_region_destroy()
called, but since commit 469b046ead06 ("memory: remove
memory_region_destroy") this is not necessary anymore as the
MemoryRegion is freed automagically.

Remove the PCIDevice 'has_rom' attribute setting in vfio.

Fixes: b895de502717 ("migration: discard non-migratable RAMBlocks")
Signed-off-by: Cédric Le Goater 
Reviewed-by: Michael S. Tsirkin 
Signed-off-by: Alex Williamson 
---
 hw/vfio/pci.c |1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index a1577dea7fdb..6cbb8fa0549d 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -990,7 +990,6 @@ static void vfio_pci_size_rom(VFIOPCIDevice *vdev)
 pci_register_bar(>pdev, PCI_ROM_SLOT,
  PCI_BASE_ADDRESS_SPACE_MEMORY, >pdev.rom);
 
-vdev->pdev.has_rom = true;
 vdev->rom_read_failed = false;
 }
 

[Qemu-devel] [PULL 0/1] VFIO fixes for qemu-3.0-rc1

[Alex Williamson]

The following changes since commit c447afd5783b9237fa51b7a85777007d8d568bfc:

  Update version for v3.0.0-rc0 release (2018-07-10 18:19:50 +0100)

are available in the Git repository at:

  git://github.com/awilliam/qemu-vfio.git tags/vfio-fixes-20180711.1

for you to fetch changes up to 26c0ae56386edacc8b0da40264748f59afedb1bb:

  vfio/pci: do not set the PCIDevice 'has_rom' attribute (2018-07-11 13:43:57 
-0600)


VFIO fixes 2018-07-11

 - Avoid RAMBlock segfault in option ROM teardown for vfio-pci devices
   (Cédric Le Goater)


Cédric Le Goater (1):
  vfio/pci: do not set the PCIDevice 'has_rom' attribute

 hw/vfio/pci.c | 1 -
 1 file changed, 1 deletion(-)

Re: [Qemu-devel] [PATCH] dump: add kernel_gs_base to QEMU CPU state

[Eduardo Habkost]

On Wed, Jul 11, 2018 at 06:19:33PM +0200, Paolo Bonzini wrote:
> On 11/07/2018 18:00, Eduardo Habkost wrote:
> >> @@ -237,7 +237,7 @@ int x86_cpu_write_elf32_note(WriteCoreDumpFunction f, 
> >> CPUState *cs,
> >>   * please count up QEMUCPUSTATE_VERSION if you have changed definition of
> >>   * QEMUCPUState, and modify the tools using this information accordingly.
> > Where are the tools using this information, that need to be
> > updated?  Won't this break existing versions of those tools?
> 
> I think it's okay to _not_ change the version, since the format is
> backwards-compatible.  Each QEMUCPUState struct is in a separate ELF
> note, and the presence of the new field is visible in both 1) the size
> of the note 2) the size field of the struct.

If we do it, can we please make the documentation (or at least
code comments, as documentation doesn't seem to exist) clearly
state that the new field is optional in version 1?

-- 
Eduardo

[Qemu-devel] [PULL 0/1] Monitor patches for 2018-07-11

[Markus Armbruster]

The following changes since commit c447afd5783b9237fa51b7a85777007d8d568bfc:

  Update version for v3.0.0-rc0 release (2018-07-10 18:19:50 +0100)

are available in the Git repository at:

  git://repo.or.cz/qemu/armbru.git tags/pull-monitor-2018-07-11

for you to fetch changes up to 42eab8dbec2f3fd4a14bd63ab01aa155ce5724a3:

  monitor: fix double-free of request error (2018-07-11 21:11:15 +0200)


Monitor patches for 2018-07-11


Marc-André Lureau (1):
  monitor: fix double-free of request error

 monitor.c | 1 +
 1 file changed, 1 insertion(+)

Marc-André Lureau (1):
  monitor: fix double-free of request error

 monitor.c | 1 +
 1 file changed, 1 insertion(+)

-- 
2.17.1

[Qemu-devel] [PULL 1/1] monitor: fix double-free of request error

[Markus Armbruster]

From: Marc-André Lureau 

qmp_error_response() will free the given error. Fix double-free in
later qmp_request_free().

Signed-off-by: Marc-André Lureau 
Message-Id: <20180705164201.9853-1-marcandre.lur...@redhat.com>
Reviewed-by: Markus Armbruster 
Fixes: 1cc37471525d03f963bc71d724f0dc9eab888fc1
Signed-off-by: Markus Armbruster 
---
 monitor.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/monitor.c b/monitor.c
index 3c9c97b73f..7af1f18d13 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4186,6 +4186,7 @@ static void monitor_qmp_bh_dispatcher(void *data)
 } else {
 assert(req_obj->err);
 rsp = qmp_error_response(req_obj->err);
+req_obj->err = NULL;
 monitor_qmp_respond(req_obj->mon, rsp, NULL);
 qobject_unref(rsp);
 }
-- 
2.17.1

Re: [Qemu-devel] [PATCH] hw/arm/bcm283x: Fix crash with device_add bcm2837 on unsupported machines

[Thomas Huth]

On 11.07.2018 19:21, Paolo Bonzini wrote:
> On 10/07/2018 08:50, Peter Maydell wrote:
 Yuck. The real problem here is that we're still requiring the
 code that creates these QOM devices to manually set the parent
 in the first place. It's not surprising that we don't get it right
 (either parenting in the wrong place or not at all). I'd much
 rather see us fix that properly than keep papering over places
 where we get it wrong.
>>> Sorry, I'm still not an expert in all this QOM stuff yet ... so what do
>>> you exactly recommend to do instead?
>> I'm not clear either, but I don't think that what we're
>> currently doing can be right.
> 
> Well, in theory it should work...  I sent the expected flow in another email.

Something that just came to my mind:

bcm2836_init() creates the TYPE_BCM2835_PERIPHERALS object with
object_initialize(). This creates one reference to the object already.
Then the object is linked to its parent with
object_property_add_child(), which creates another reference to the
object. But where are the two references correctly destroyed again? One
is certainly destroyed by device_unparent later, but the initial one?
Could it be that we are simply lacking one object_unref() after the
object_property_add_child() here?

 Thomas

Re: [Qemu-devel] [PATCH] hw/arm/bcm283x: Fix crash with device_add bcm2837 on unsupported machines

[Thomas Huth]

On 11.07.2018 19:20, Paolo Bonzini wrote:
> On 09/07/2018 23:03, Thomas Huth wrote:
>>
>> The problem is that qdev_set_parent_bus() from instance_init adds a link
>> to the child devices which is not valid anymore after the device init
>> failed. Thus the qdev_set_parent_bus() must rather be done in the realize
>> function instead.
> 
> The theoretical behavior should be:
> 
> - realize fails

In this case, the failure is before realize is attempted,
qdev_device_add() already stop with "Device '%s' can not be hotplugged
on this machine".

> - object_unparent is called on the device that failed to realize (see
> qdev_device_add).  object_unparent calls device_unparent

Hmm, are you sure? I can see that object_unparent calls device_unparent
indirectly for the *child* nodes of the device, but not for the device
itself...

 Thomas

Re: [Qemu-devel] [PATCH v2 2/2] vga: don't pick cirrus by default

[Eduardo Habkost]

On Wed, Jul 11, 2018 at 07:00:54PM +0200, Sebastian Bauer wrote:
> Am 2018-07-11 17:48, schrieb Eduardo Habkost:
> > "none" looked like a false positive when I first looked, but now
> > I think it's not.  Shouldn't it set default_display="none"?
> 
> I think that there is some other logic burried that these machine doesn't
> get a graphics display. But overall it is indeed not clearly defined.
> 
> But see below.
> 
> > > If the patch is applied to 3.1 then I think there is enough time to
> > > fix
> > > issues caused by the patch. Additionally, a warning could be put in
> > > the
> > > ChangeLog for 3.0 that in 3.1 that the default mode will be std unless
> > > machines define an own default. This is should be enough time for
> > > people to
> > > complain or to fix things.
> > I don't think we will really make user-visible changes: we can
> > simply work to keep existing behavior, but the difference is that
> > this will be implemented by setting default_display explicitly on
> > all machines.
> 
> Even if all machines were using explicit default settings the patch will
> affect machines that are not inside the QEMU tree. If the patch is to be
> applied as it is these are affected. To warn users (or devs in this case)
> about this, an entry in the ChangeLog would be appropriate.

What do you mean by "machines that are not inside the QEMU tree"?
MachineClass registration is not an API for external use.

> 
> > > If the patch is to be applied to 3.0 then all non-ppc ones need to be
> > > reconsidered.
> > > The "important" ppc machines have been fixed already. I can do the
> > > remaining
> > > if this is wanted.
> > This part worries me: do we have other machines that are broken
> > right now?
> 
> I don't know which of them are broken or how this can be elaborated, but
> they are potentially affected. For instance, the sam460ex platform doesn't
> care about this setting, I can say that it is not broken. Other platforms
> like the mac apparently were broken (and fixed in the meantime). It is hard
> to tell which of them are really broken without someone that knows the
> platform trying it and telling it. 'Make check' did catch only one single
> case. It could also be that nobody cares about other affected machines.
> 
> Overall I think the patch is an improvement over the previous state as
> preferring the Cirrus doesn't make much sense if most machines don't prefer
> it. The more I think over it, the more I think that the concept needs
> further fine-tuning though (not necessarily in this patch).

I'm not sure yet if most machines with default_display==NULL
don't want Cirrus.  I'm also unsure if any of the machines from
that list will break if we start using VGA by default.

> 
> If OTOH it would become a requirement for machines to set a default display
> then this fallback logic could removed. Instead, qemu could simply bailout
> on machines that define no default display (including "none") and also
> bailout when the requested default display is not available. This would be
> checkable by 'make check'. [...]

This is the solution I would like to see implemented.

> [...]  I still think that the most common value can be a
> default (strictly bailing out when it is not available unlike it is done
> now), but this is a matter of taste I guess.

This is doable too, if we have a clear consensus on what would be
a reasonable default on TYPE_MACHINE.  Personally I prefer the
default on TYPE_MACHINE to be "none".

-- 
Eduardo

Re: [Qemu-devel] [PATCH] hw/arm/bcm283x: Fix crash with device_add bcm2837 on unsupported machines

[Eduardo Habkost]

On Wed, Jul 11, 2018 at 07:20:42PM +0200, Paolo Bonzini wrote:
> On 09/07/2018 23:03, Thomas Huth wrote:
> > 
> > The problem is that qdev_set_parent_bus() from instance_init adds a link
> > to the child devices which is not valid anymore after the device init
> > failed. Thus the qdev_set_parent_bus() must rather be done in the realize
> > function instead.
> 
> The theoretical behavior should be:

It's not clear below where you expect
  qdev_set_parent_bus(..., sysbus_get_default())
to be called (if it should be called at all).

I don't know where it should be called, but I'm absolutely sure
instance_init is not the right place.

> 
> - realize fails
> 
> - object_unparent is called on the device that failed to realize (see
> qdev_device_add).  object_unparent calls device_unparent
> 
> - after device_unparent finishes, the last reference to the device has
> been dropped and the device is freed
> 
> - object finalization releases all properties
> 
> - this includes child properties, so for each child device
> object_unparent is called
> 
> - again device_unparent is called (for the child) and this removes the
> child from the bus.
> 
> Paolo

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2] linux-user: fix mmap_find_vma_reserved()

[Laurent Vivier]

Le 11/07/2018 à 18:40, Laurent Vivier a écrit :
> The value given by mmap_find_vma_reserved() is used with mmap(),
> so it is needed to be aligned with the host page size.
> 
> Since commit 18e80c55bb, reserved_va is only aligned to TARGET_PAGE_SIZE,
> and it works well if this size is greater or equal to the host page size.
> 
> But ppc64 hosts have 64kB page size and when we start a 4kiB page size
> guest (like i386), it fails when it tries to mmap the stack:
> 
> mmap stack: Invalid argument
> 
> Fixes: 18e80c55bb (linux-user: Tidy and enforce reserved_va initialization)
> Signed-off-by: Laurent Vivier 

Richard,

I think this fix could be merged into your "linux-user: Fix shmat
emulation by honoring host SHMLBA" patch, by adding something like this
instead:

--- a/linux-user/mmap.c
+++ b/linux-user/mmap.c
@@ -262,6 +262,8 @@ abi_ulong mmap_find_vma(abi_ulong start, abi_ulong
size, abi_ulong align)
 abi_ulong addr;
 int wrapped, repeat;

+align = MAX(align, qemu_host_page_size);
+
 /* If 'start' == 0, then a default start address is used. */
 if (start == 0) {
 start = mmap_next_start;

Thanks,
Laurent

Re: [Qemu-devel] [PATCH 7/7] tests/qgraph: sdhci test node

[Emanuele]

On 07/11/2018 05:15 PM, Stefan Hajnoczi wrote:


On Mon, Jul 09, 2018 at 11:11:36AM +0200, Emanuele Giuseppe Esposito wrote:

+/**
+ * Old sdhci_t structure:

Do you intend to delete this comment before this series is merged?  It
seems like a TODO that doesn't need to be kept around.
Paolo suggested me to put it there, because there still are some devices 
that need to be implemented.



+qos_add_test("sdhci-test", "sdhci", test_machine);

How does this work for tests that need access to more than 1 device?
Can they request a driver instance via the API?
Uhm accessing more than one device is something I did not take into 
account. It is possible to add additional command line to the test, with 
qos_add_test_args(), but no multiple devices. I'll add it as TODO in my 
list, thanks

Re: [Qemu-devel] [PATCH 2/7] tests/qgraph: pci-pc driver and interface nodes

[Emanuele]

On 07/11/2018 04:49 PM, Stefan Hajnoczi wrote:


On Mon, Jul 09, 2018 at 11:11:31AM +0200, Emanuele Giuseppe Esposito wrote:

-QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc)
+static void *qpci_get_driver(void *obj, const char *interface)
  {
-QPCIBusPC *ret = g_new0(QPCIBusPC, 1);
+QPCIBusPC *qpci = obj;
+if (!g_strcmp0(interface, "pci-bus")) {
+return >bus;
+}
+printf("%s not present in pci-bus-pc", interface);
+abort();
+}

At this point I wonder if it makes sense to use the QEMU Object Model
(QOM), which has interfaces and inheritance.  qgraph duplicates part of
the object model.


+void qpci_device_init(QPCIDevice *dev, QPCIBus *bus, int devfn)
+{
+if (!bus) {
+return;
+}

When does this happen and why?
Ok maybe this is unneeded, I added it because I was creating a test with 
a NULL bus, but we ended up putting it aside. So you're right, this 
should be eliminated.

+dev->bus = bus;
+dev->devfn = devfn;
+
+if (qpci_config_readw(dev, PCI_VENDOR_ID) == 0x) {
+printf("PCI Device not found\n");
+abort();
+}
+qpci_device_enable(dev);
+}
+
+void qpci_set_pc(QPCIBusPC *ret, QTestState *qts, QGuestAllocator *alloc)

It's not clear to me what the purpose of this function is - at least the
name is a bit cryptic since it seems more like an initialization
function than 'setting pc' on QPCIBusPC.  How about inlining this in
qpci_init_pc() instead of keeping a separate function?
The idea is that, following the graph that Paolo wrote in the GSoC 
project wiki (https://wiki.qemu.org/Features/qtest_driver_framework), 
QPCIBusPC is "contained" in i440FX-pcihost. This means that the 
i440FX-pcihost struct has a QPCIBusPC field.


Therefore I had to put QPCIBusPC as public (in order to have it as 
field), even though qpci_init_pc() was not what I needed to initialize 
it, because that function is allocating a new QPCIBusPC, while I just 
needed to "set" its values.

From here, qpci_set_pc().

+{
  assert(qts);
  
  ret->bus.pio_readb = qpci_pc_pio_readb;

@@ -147,11 +164,23 @@ QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator 
*alloc)
  ret->bus.mmio_alloc_ptr = 0xE000;
  ret->bus.mmio_limit = 0x1ULL;
  
+ret->obj.get_driver = qpci_get_driver;

+}
+
+QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc)
+{
+QPCIBusPC *ret = g_new0(QPCIBusPC, 1);
+qpci_set_pc(ret, qts, alloc);
+
  return >bus;
  }
  
  void qpci_free_pc(QPCIBus *bus)

  {
+if (!bus) {
+return;
+}

Why is this needed now?
Not having this would mean failure of tests like vrtio-user-test, 
because now QPCIBusPC has two fields, and QPCIBus bus; is not the first 
one either.

Therefore, if I remember correctly doing something like
container_of(NULL, QPCIBusPC, bus)
where bus is not the first field of QPCIBusPC would result in a negative 
number, and freeing that would make the test crash.


I discovered this issue while doing some checks, and already proposed a 
patch for it, even though we ended up agreeing that this fix was only 
needed in my patch and not in the current QEMU implementation.

+
  QPCIBusPC *s = container_of(bus, QPCIBusPC, bus);
  
  g_free(s);

@@ -176,3 +205,11 @@ void qpci_unplug_acpi_device_test(const char *id, uint8_t 
slot)
  
  qmp_eventwait("DEVICE_DELETED");

  }
+
+static void qpci_pc(void)
+{
+qos_node_create_driver("pci-bus-pc", NULL);
+qos_node_produces("pci-bus-pc", "pci-bus");

In QOM pci-bus-pc would be a class, pci-bus would be an interface.  From
a driver perspective it seems QOM can already do what is needed and the
qgraph infrastructure isn't necessary.

Obviously the depth-first search *is* unique and not in QOM, although
QOM does offer a tree namespace which can be used for looking up object
instances and I guess this could be used to configure tests at runtime.

I'll think about this more as I read the rest of the patches.


+}
+
+libqos_init(qpci_pc);
diff --git a/tests/libqos/pci-pc.h b/tests/libqos/pci-pc.h
index 491eeac756..ee381c5667 100644
--- a/tests/libqos/pci-pc.h
+++ b/tests/libqos/pci-pc.h
@@ -15,7 +15,15 @@
  
  #include "libqos/pci.h"

  #include "libqos/malloc.h"
+#include "qgraph.h"
  
+typedef struct QPCIBusPC {

+QOSGraphObject obj;
+QPCIBus bus;
+} QPCIBusPC;

Why does this need to be public?

See previous answer



+
+void qpci_device_init(QPCIDevice *dev, QPCIBus *bus, int devfn);

Why does this need to be public?
I'll use that in the next patches. I also realized this function should 
go in pci.h, and not pci-pc.h



+void qpci_set_pc(QPCIBusPC *ret, QTestState *qts, QGuestAllocator *alloc);

Why does this need to be public?
Because I use it in the next patch to set the QPCIBusPC in the x86_64/pc 
machine.



  QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc);
  void qpci_free_pc(QPCIBus *bus);
  
diff --git a/tests/libqos/pci.c b/tests/libqos/pci.c

index 

Re: [Qemu-devel] [PATCH] hw/arm/bcm283x: Fix crash with device_add bcm2837 on unsupported machines

[Paolo Bonzini]

On 10/07/2018 08:50, Peter Maydell wrote:
>>> Yuck. The real problem here is that we're still requiring the
>>> code that creates these QOM devices to manually set the parent
>>> in the first place. It's not surprising that we don't get it right
>>> (either parenting in the wrong place or not at all). I'd much
>>> rather see us fix that properly than keep papering over places
>>> where we get it wrong.
>> Sorry, I'm still not an expert in all this QOM stuff yet ... so what do
>> you exactly recommend to do instead?
> I'm not clear either, but I don't think that what we're
> currently doing can be right.

Well, in theory it should work...  I sent the expected flow in another email.

Paolo

Re: [Qemu-devel] [PATCH] hw/arm/bcm283x: Fix crash with device_add bcm2837 on unsupported machines

[Paolo Bonzini]

On 09/07/2018 23:03, Thomas Huth wrote:
> 
> The problem is that qdev_set_parent_bus() from instance_init adds a link
> to the child devices which is not valid anymore after the device init
> failed. Thus the qdev_set_parent_bus() must rather be done in the realize
> function instead.

The theoretical behavior should be:

- realize fails

- object_unparent is called on the device that failed to realize (see
qdev_device_add).  object_unparent calls device_unparent

- after device_unparent finishes, the last reference to the device has
been dropped and the device is freed

- object finalization releases all properties

- this includes child properties, so for each child device
object_unparent is called

- again device_unparent is called (for the child) and this removes the
child from the bus.

Paolo

Re: [Qemu-devel] [PATCH] hw/arm/bcm283x: Fix crash with device_add bcm2837 on unsupported machines

[Peter Maydell]

On 11 July 2018 at 17:12, Eduardo Habkost  wrote:
> On Wed, Jul 11, 2018 at 09:21:48AM +0200, Thomas Huth wrote:
>> On 10.07.2018 08:50, Peter Maydell wrote:
>> > On 9 July 2018 at 23:03, Thomas Huth  wrote:
>> >> On 09.07.2018 23:42, Peter Maydell wrote:
>> >>> On 9 July 2018 at 22:03, Thomas Huth  wrote:
>>  When trying to "device_add bcm2837" on a machine that is not suitable 
>>  for
>>  this device, you can quickly crash QEMU afterwards, e.g. with "info 
>>  qtree":
>> 
>>  echo "{'execute':'qmp_capabilities'} {'execute':'device_add', " \
>>   "'arguments':{'driver':'bcm2837'}} {'execute': 
>>  'human-monitor-command', " \
>>   "'arguments': {'command-line': 'info qtree'}}" | \
>>   aarch64-softmmu/qemu-system-aarch64 -M integratorcp,accel=qtest -S 
>>  -qmp stdio
>> 
>>  {"QMP": {"version": {"qemu": {"micro": 50, "minor": 12, "major": 2},
>>   "package": "build-all"}, "capabilities": []}}
>>  {"return": {}}
>>  {"error": {"class": "GenericError", "desc": "Device 'bcm2837' can not be
>>   hotplugged on this machine"}}
>>  Segmentation fault (core dumped)
>> 
>>  The problem is that qdev_set_parent_bus() from instance_init adds a link
>>  to the child devices which is not valid anymore after the device init
>>  failed. Thus the qdev_set_parent_bus() must rather be done in the 
>>  realize
>>  function instead.
>> >>>
>> >>> Yuck. The real problem here is that we're still requiring the
>> >>> code that creates these QOM devices to manually set the parent
>> >>> in the first place. It's not surprising that we don't get it right
>> >>> (either parenting in the wrong place or not at all). I'd much
>> >>> rather see us fix that properly than keep papering over places
>> >>> where we get it wrong.
>> >>
>> >> Sorry, I'm still not an expert in all this QOM stuff yet ... so what do
>> >> you exactly recommend to do instead?
>> >
>> > I'm not clear either, but I don't think that what we're
>> > currently doing can be right.
>>
>> Hm, ok, so how to continue here now? Shall we at least mark the
>> bcm2836/7 devices with user_creatable=false, so that users can not crash
>> their QEMU so easily with device_add? The problem with introspection via
>> device-list-properties would still continue to exist, but I think that's
>> less likely used in practice... otherwise we could still move the
>> qdev_set_parent_bus() calls to the realize() function instead, and just
>> add a big fat FIXME comment in front of the code block, so that we
>> remember to clean that up one day...
>
> Crashing device-list-properties should be a blocker bug, IMO.
>
> Moving to realize is not the best solution, but I would prefer to
> do that in 3.0 instead of leaving the device-list-properties
> crash unfixed.

I would like to see the crash fixed too. But I'd like to
see it fixed:
 (a) by having clear documentation about how the QOM
system works, what you should do in init and what you
should do in realize, when and why you need to manually
parent objects, etc
 (b) as far as possible making our APIs for doing this
easy to use correctly and difficult to use wrongly. At
the moment we have APIs that are far too easy to misuse,
which means we will continue to get bugs like this and spend
a lot of time on one-off fixes for them.

In particular I don't understand why we need to manually
parent these objects at all.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 2/2] vga: don't pick cirrus by default

[Sebastian Bauer]

Am 2018-07-11 17:48, schrieb Eduardo Habkost:

"none" looked like a false positive when I first looked, but now
I think it's not.  Shouldn't it set default_display="none"?


I think that there is some other logic burried that these machine 
doesn't get a graphics display. But overall it is indeed not clearly 
defined.


But see below.

If the patch is applied to 3.1 then I think there is enough time to 
fix
issues caused by the patch. Additionally, a warning could be put in 
the

ChangeLog for 3.0 that in 3.1 that the default mode will be std unless
machines define an own default. This is should be enough time for 
people to

complain or to fix things.

I don't think we will really make user-visible changes: we can
simply work to keep existing behavior, but the difference is that
this will be implemented by setting default_display explicitly on
all machines.


Even if all machines were using explicit default settings the patch will 
affect machines that are not inside the QEMU tree. If the patch is to be 
applied as it is these are affected. To warn users (or devs in this 
case) about this, an entry in the ChangeLog would be appropriate.



If the patch is to be applied to 3.0 then all non-ppc ones need to be
reconsidered.
The "important" ppc machines have been fixed already. I can do the 
remaining

if this is wanted.

This part worries me: do we have other machines that are broken
right now?


I don't know which of them are broken or how this can be elaborated, but 
they are potentially affected. For instance, the sam460ex platform 
doesn't care about this setting, I can say that it is not broken. Other 
platforms like the mac apparently were broken (and fixed in the 
meantime). It is hard to tell which of them are really broken without 
someone that knows the platform trying it and telling it. 'Make check' 
did catch only one single case. It could also be that nobody cares about 
other affected machines.


Overall I think the patch is an improvement over the previous state as 
preferring the Cirrus doesn't make much sense if most machines don't 
prefer it. The more I think over it, the more I think that the concept 
needs further fine-tuning though (not necessarily in this patch).


If OTOH it would become a requirement for machines to set a default 
display then this fallback logic could removed. Instead, qemu could 
simply bailout on machines that define no default display (including 
"none") and also bailout when the requested default display is not 
available. This would be checkable by 'make check'. I still think that 
the most common value can be a default (strictly bailing out when it is 
not available unlike it is done now), but this is a matter of taste I 
guess.


Bye
Sebastian

Re: [Qemu-devel] [PATCH 1/2] qdev: add HotplugHandler->post_plug() callback

[Paolo Bonzini]

On 11/07/2018 17:22, Igor Mammedov wrote:
> It also seems wrong to call _plug handler on maybe partially
> initialized device so perhaps we should first finish devices/children
> realization then do reset and only after that call _plug() handler

I agree but this is too dangerous until we look at plug() handlers and
remove the usage of Error **.  Introducing a new callback helps the
transition.

Paolo

[Qemu-devel] [PATCH v2] linux-user: fix mmap_find_vma_reserved()

[Laurent Vivier]

The value given by mmap_find_vma_reserved() is used with mmap(),
so it is needed to be aligned with the host page size.

Since commit 18e80c55bb, reserved_va is only aligned to TARGET_PAGE_SIZE,
and it works well if this size is greater or equal to the host page size.

But ppc64 hosts have 64kB page size and when we start a 4kiB page size
guest (like i386), it fails when it tries to mmap the stack:

mmap stack: Invalid argument

Fixes: 18e80c55bb (linux-user: Tidy and enforce reserved_va initialization)
Signed-off-by: Laurent Vivier 
---

Notes:
v2:
  fix typo s/has/as/

 linux-user/main.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/linux-user/main.c b/linux-user/main.c
index 52b5a618fe..15299e9dd7 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -689,6 +689,11 @@ int main(int argc, char **argv, char **envp)
 target_environ = envlist_to_environ(envlist, NULL);
 envlist_free(envlist);
 
+/* reserved_va must be aligned with the host page size
+ * as it is used with mmap()
+ */
+reserved_va &= qemu_host_page_mask;
+
 /*
  * Now that page sizes are configured in tcg_exec_init() we can do
  * proper page alignment for guest_base.
-- 
2.17.1

[Qemu-devel] [PATCH] linux-user: fix mmap_find_vma_reserved()

[Laurent Vivier]

The value given by mmap_find_vma_reserved() is used with mmap(),
so it is needed to be aligned with the host page size.

Since commit 18e80c55bb, reserved_va is only aligned to TARGET_PAGE_SIZE,
and it works well if this size is greater or equal to the host page size.

But ppc64 hosts have 64kB page size and when we start a 4kiB page size
guest (like i386), it fails when it tries to mmap the stack:

mmap stack: Invalid argument

Fixes: 18e80c55bb (linux-user: Tidy and enforce reserved_va initialization)
Signed-off-by: Laurent Vivier 
---
 linux-user/main.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/linux-user/main.c b/linux-user/main.c
index 52b5a618fe..a370b89ee6 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -689,6 +689,11 @@ int main(int argc, char **argv, char **envp)
 target_environ = envlist_to_environ(envlist, NULL);
 envlist_free(envlist);
 
+/* reserved_va must be aligned with the host page size
+ * has it is used with mmap()
+ */
+reserved_va &= qemu_host_page_mask;
+
 /*
  * Now that page sizes are configured in tcg_exec_init() we can do
  * proper page alignment for guest_base.
-- 
2.17.1

Re: [Qemu-devel] [PATCH] dump: add kernel_gs_base to QEMU CPU state

[Paolo Bonzini]

On 11/07/2018 18:26, Viktor Prutyanov wrote:
>> Where are the tools using this information, that need to be
>> updated?  Won't this break existing versions of those tools?
>>
>> Is the dump format and pointers to available tools documented
>> somewhere?
> I hope that someone from community knows about those tools because I
> can't find such tools.
> 

I checked crash, and it doesn't have any issue with QEMUCPUState that is
bigger than what is currently in QEMU.  It doesn't use anything but CR3
and IDT base.  It also doesn't at all check the version! :O

Paolo

Re: [Qemu-devel] [PATCH] dump: add kernel_gs_base to QEMU CPU state

[Viktor Prutyanov]

On Wed, 11 Jul 2018 13:00:25 -0300
Eduardo Habkost  wrote:

> On Tue, Jul 10, 2018 at 06:21:09PM +0300, Viktor Prutyanov wrote:
> > This patch adds field with content of KERNEL_GS_BASE MSR to QEMU
> > note in ELF dump.
> > 
> > On Windows, if all vCPUs are running usermode tasks at the time the
> > dump is created, this can be helpful in the discovery of guest
> > system structures during conversion ELF dump to MEMORY.DMP dump.
> > 
> > Signed-off-by: Viktor Prutyanov 
> > ---
> >  target/i386/arch_dump.c | 5 -
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/target/i386/arch_dump.c b/target/i386/arch_dump.c
> > index 35b55fc..a702138 100644
> > --- a/target/i386/arch_dump.c
> > +++ b/target/i386/arch_dump.c
> > @@ -237,7 +237,7 @@ int
> > x86_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
> >   * please count up QEMUCPUSTATE_VERSION if you have changed
> > definition of
> >   * QEMUCPUState, and modify the tools using this information
> > accordingly.  
> 
> Where are the tools using this information, that need to be
> updated?  Won't this break existing versions of those tools?
> 
> Is the dump format and pointers to available tools documented
> somewhere?

I hope that someone from community knows about those tools because I
can't find such tools.

> 
> >   */
> > -#define QEMUCPUSTATE_VERSION (1)
> > +#define QEMUCPUSTATE_VERSION (2)
> >  
> >  struct QEMUCPUSegment {
> >  uint32_t selector;
> > @@ -258,6 +258,7 @@ struct QEMUCPUState {
> >  QEMUCPUSegment cs, ds, es, fs, gs, ss;
> >  QEMUCPUSegment ldt, tr, gdt, idt;
> >  uint64_t cr[5];
> > +uint64_t kernel_gs_base;
> >  };
> >  
> >  typedef struct QEMUCPUState QEMUCPUState;
> > @@ -315,6 +316,8 @@ static void qemu_get_cpustate(QEMUCPUState *s,
> > CPUX86State *env) s->cr[2] = env->cr[2];
> >  s->cr[3] = env->cr[3];
> >  s->cr[4] = env->cr[4];
> > +
> > +s->kernel_gs_base = env->kernelgsbase;
> >  }
> >  
> >  static inline int cpu_write_qemu_note(WriteCoreDumpFunction f,
> > -- 
> > 2.7.4
> >   
> 

Re: [Qemu-devel] [PATCH v6 4/4] acpi: build TPM Physical Presence interface

[Igor Mammedov]

On Wed, 4 Jul 2018 18:00:41 +0200
Marc-André Lureau  wrote:

> HI
> 
> On Wed, Jul 4, 2018 at 5:39 PM, Igor Mammedov  wrote:
> > On Thu, 28 Jun 2018 19:26:57 +0200
> > Marc-André Lureau  wrote:
> >  
> >> From: Stefan Berger 
> >>
> >> The TPM Physical Presence interface consists of an ACPI part, a shared
> >> memory part, and code in the firmware. Users can send messages to the
> >> firmware by writing a code into the shared memory through invoking the
> >> ACPI code. When a reboot happens, the firmware looks for the code and
> >> acts on it by sending sequences of commands to the TPM.
> >>
> >> This patch adds the ACPI code. It is similar to the one in EDK2 but doesn't
> >> assume that SMIs are necessary to use. It uses a similar datastructure for
> >> the shared memory as EDK2 does so that EDK2 and SeaBIOS could both make use
> >> of it. I extended the shared memory data structure with an array of 256
> >> bytes, one for each code that could be implemented. The array contains
> >> flags describing the individual codes. This decouples the ACPI 
> >> implementation
> >> from the firmware implementation.
> >>
> >> The underlying TCG specification is accessible from the following page.
> >>
> >> https://trustedcomputinggroup.org/tcg-physical-presence-interface-specification/
> >>
> >> This patch implements version 1.30.
> >>
> >> Signed-off-by: Stefan Berger 
> >> [ Marc-André - ACPI code improvements and windows fixes ]
> >> Signed-off-by: Marc-André Lureau 
> >>
> >> ---
> >>
> >> v7: (Marc-André)
> >>  - use first spec version/section in code comments
> >>  - use more variables for ASL code building
> >>  - remove unnecessering ToInteger() calls
> >>
> >> v6:
> >>  - more code documentation (Marc-André)
> >>  - use some explicit named variables to ease reading (Marc-André)
> >>  - use fixed size fields/memory regions, remove PPI struct (Marc-André)
> >>  - only add PPI ACPI methods if PPI is enabled (Marc-André)
> >>  - document the qemu/firmware ACPI memory region (Stefan)
> >>
> >> v5 (Marc-André):
> >>  - /struct tpm_ppi/struct TPMPPIData
> >>
> >> v4 (Marc-André):
> >>  - replace 'DerefOf (FUNC [N])' with a function, to fix Windows ACPI
> >> handling.
> >>  - replace 'return Package (..) {} ' with scoped variables, to fix
> >>Windows ACPI handling.
> >>
> >> v3:
> >>  - add support for PPI to CRB
> >>  - split up OperationRegion TPPI into two parts, one containing
> >>the registers (TPP1) and the other one the flags (TPP2); switched
> >>the order of the flags versus registers in the code
> >>  - adapted ACPI code to small changes to the array of flags where
> >>previous flag 0 was removed and now shifting right wasn't always
> >>necessary anymore
> >>
> >> v2:
> >>  - get rid of FAIL variable; function 5 was using it and always
> >>returns 0; the value is related to the ACPI function call not
> >>a possible failure of the TPM function call.
> >>  - extend shared memory data structure with per-opcode entries
> >>holding flags and use those flags to determine what to return
> >>to caller
> >>  - implement interface version 1.3
> >> ---
> >>  include/hw/acpi/tpm.h |   8 +
> >>  hw/i386/acpi-build.c  | 403 +-
> >>  docs/specs/tpm.txt|  79 +
> >>  3 files changed, 487 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
> >> index f79d68a77a..e0bd07862e 100644
> >> --- a/include/hw/acpi/tpm.h
> >> +++ b/include/hw/acpi/tpm.h
> >> @@ -196,4 +196,12 @@ REG32(CRB_DATA_BUFFER, 0x80)
> >>  #define TPM_PPI_VERSION_NONE0
> >>  #define TPM_PPI_VERSION_1_301
> >>
> >> +/* whether function is blocked by BIOS settings; bits 0, 1, 2 */
> >> +#define TPM_PPI_FUNC_NOT_IMPLEMENTED (0 << 0)
> >> +#define TPM_PPI_FUNC_BIOS_ONLY   (1 << 0)
> >> +#define TPM_PPI_FUNC_BLOCKED (2 << 0)
> >> +#define TPM_PPI_FUNC_ALLOWED_USR_REQ (3 << 0)
> >> +#define TPM_PPI_FUNC_ALLOWED_USR_NOT_REQ (4 << 0)
> >> +#define TPM_PPI_FUNC_MASK(7 << 0)
> >> +
> >>  #endif /* HW_ACPI_TPM_H */
> >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> >> index ebd64c4818..263677f3e4 100644
> >> --- a/hw/i386/acpi-build.c
> >> +++ b/hw/i386/acpi-build.c
> >> @@ -43,6 +43,7 @@
> >>  #include "hw/acpi/memory_hotplug.h"
> >>  #include "sysemu/tpm.h"
> >>  #include "hw/acpi/tpm.h"
> >> +#include "hw/tpm/tpm_ppi.h"
> >>  #include "hw/acpi/vmgenid.h"
> >>  #include "sysemu/tpm_backend.h"
> >>  #include "hw/timer/mc146818rtc_regs.h"
> >> @@ -1789,6 +1790,396 @@ static Aml *build_q35_osc_method(void)
> >>  return method;
> >>  }
> >>
> >> +static void
> >> +build_tpm_ppi(TPMIf *tpm, Aml *dev)
> >> +{
> >> +Aml *method, *field, *ifctx, *ifctx2, *ifctx3;
> >> +Aml *pak, *tpm2, *tpm3, *pprm, *pprq;
> >> +int i;
> >> +
> >> +if (!object_property_get_bool(OBJECT(tpm), "ppi", _abort)) {
> >> +return;
> >> +}  
> > I'd prefer this 

[Qemu-devel] [Bug 1772075] Re: Segmentation fault on aarch64 vm at powerdown

[Peter Maydell]

As I said, I don't want to have to deal with image generation tools and
extracting initrds from disk images. The easiest thing for me is if you
can just provide all the files and the command line I can use to
reproduce.

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1772075

Title:
  Segmentation fault on aarch64 vm at powerdown

Status in QEMU:
  New

Bug description:
  OS Arch Linux
  x86_64
  qemu version: 2.12

  cmdline:
  qemu-system-aarch64 -nographic -cpu cortex-a57 -m 2048 -M virt,gic_version=3 
-machine virtualization=true -bios /usr/share/ovmf/AARCH64/QEMU_EFI.fd -drive 
file=fat:rw:/opt/simonpiemu/kernels/rpi-3,if=none,format=raw,cache=none,id=hd0 
-device virtio-blk-device,drive=hd0 -drive 
file=/home/morfeo/.simonpi/sd-arch-rpi-3-qemu.img,if=none,format=raw,cache=none,id=hd1
 -device virtio-blk-device,drive=hd1 -kernel 
/opt/simonpiemu/kernels/rpi-3/Image -append "root=/dev/vda2 fstab=no 
rootfstype=ext4 rw console=ttyAMA0" -initrd 
/home/morfeo/.simonpi/rpi-3/boot/initramfs-linux.img -device 
virtio-net-device,mac=52:54:26:11:72:9b,netdev=net0 -netdev 
tap,id=net0,ifname=rasp-tap0,script=no,downscript=no

  error:

  qemu-system-aarch64: /build/qemu/src/qemu-2.12.0/block.c:3375:
  bdrv_close_all: Assertion `QTAILQ_EMPTY(_bdrv_states)' failed.

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1772075/+subscriptions

Re: [Qemu-devel] [PATCH] dump: add kernel_gs_base to QEMU CPU state

[Paolo Bonzini]

On 11/07/2018 18:00, Eduardo Habkost wrote:
>> @@ -237,7 +237,7 @@ int x86_cpu_write_elf32_note(WriteCoreDumpFunction f, 
>> CPUState *cs,
>>   * please count up QEMUCPUSTATE_VERSION if you have changed definition of
>>   * QEMUCPUState, and modify the tools using this information accordingly.
> Where are the tools using this information, that need to be
> updated?  Won't this break existing versions of those tools?

I think it's okay to _not_ change the version, since the format is
backwards-compatible.  Each QEMUCPUState struct is in a separate ELF
note, and the presence of the new field is visible in both 1) the size
of the note 2) the size field of the struct.

Another possibility is to stash kernel_gs_base in cr[1].  This approach
doesn't scale, but the word is otherwise unused if we want to make it
super safe.  I don't recommend it.

Paolo


> Is the dump format and pointers to available tools documented
> somewhere?
> 
> 

Re: [Qemu-devel] [PATCH] hw/arm/bcm283x: Fix crash with device_add bcm2837 on unsupported machines

[Eduardo Habkost]

On Wed, Jul 11, 2018 at 09:21:48AM +0200, Thomas Huth wrote:
> On 10.07.2018 08:50, Peter Maydell wrote:
> > On 9 July 2018 at 23:03, Thomas Huth  wrote:
> >> On 09.07.2018 23:42, Peter Maydell wrote:
> >>> On 9 July 2018 at 22:03, Thomas Huth  wrote:
>  When trying to "device_add bcm2837" on a machine that is not suitable for
>  this device, you can quickly crash QEMU afterwards, e.g. with "info 
>  qtree":
> 
>  echo "{'execute':'qmp_capabilities'} {'execute':'device_add', " \
>   "'arguments':{'driver':'bcm2837'}} {'execute': 'human-monitor-command', 
>  " \
>   "'arguments': {'command-line': 'info qtree'}}" | \
>   aarch64-softmmu/qemu-system-aarch64 -M integratorcp,accel=qtest -S -qmp 
>  stdio
> 
>  {"QMP": {"version": {"qemu": {"micro": 50, "minor": 12, "major": 2},
>   "package": "build-all"}, "capabilities": []}}
>  {"return": {}}
>  {"error": {"class": "GenericError", "desc": "Device 'bcm2837' can not be
>   hotplugged on this machine"}}
>  Segmentation fault (core dumped)
> 
>  The problem is that qdev_set_parent_bus() from instance_init adds a link
>  to the child devices which is not valid anymore after the device init
>  failed. Thus the qdev_set_parent_bus() must rather be done in the realize
>  function instead.
> >>>
> >>> Yuck. The real problem here is that we're still requiring the
> >>> code that creates these QOM devices to manually set the parent
> >>> in the first place. It's not surprising that we don't get it right
> >>> (either parenting in the wrong place or not at all). I'd much
> >>> rather see us fix that properly than keep papering over places
> >>> where we get it wrong.
> >>
> >> Sorry, I'm still not an expert in all this QOM stuff yet ... so what do
> >> you exactly recommend to do instead?
> > 
> > I'm not clear either, but I don't think that what we're
> > currently doing can be right.
> 
> Hm, ok, so how to continue here now? Shall we at least mark the
> bcm2836/7 devices with user_creatable=false, so that users can not crash
> their QEMU so easily with device_add? The problem with introspection via
> device-list-properties would still continue to exist, but I think that's
> less likely used in practice... otherwise we could still move the
> qdev_set_parent_bus() calls to the realize() function instead, and just
> add a big fat FIXME comment in front of the code block, so that we
> remember to clean that up one day...

Crashing device-list-properties should be a blocker bug, IMO.

Moving to realize is not the best solution, but I would prefer to
do that in 3.0 instead of leaving the device-list-properties
crash unfixed.

Another solution is to reintroduce
DeviceClass::cannot_destroy_with_object_finalize_yet (commit
08f00df4f4b8b4e38ad620477cc90cf5f73832d9), and set
cannot_destroy_with_object_finalize_yet=true on bcm2837.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 2/2] memory: fix possible NULL pointer dereference

[Dima Stepanov]

On Wed, Jul 11, 2018 at 03:09:13PM +0100, Peter Maydell wrote:
> On 11 July 2018 at 14:47, Philippe Mathieu-Daudé  wrote:
> > Hi Dima,
> >
> > On 07/11/2018 05:34 AM, Dima Stepanov wrote:
> >> Gentle ping. CCing Paolo Bonzini.
> >>
> >> Regards, Dima.
> >>
> >> On Tue, Jun 19, 2018 at 05:12:16PM +0300, Dima Stepanov wrote:
> >>> Ping.
> >>>
> >>> Regards, Dima.
> >>>
> >>> On Wed, Jun 13, 2018 at 11:19:55AM +0300, Dima Stepanov wrote:
>  In the memory_region_do_invalidate_mmio_ptr() routine the section
>  variable is intialized by the memory_region_find() call. The section.mr
>  field can be set to NULL.
> 
>  Add the check for NULL before trying to drop a section.
> 
>  Signed-off-by: Dima Stepanov 
>  ---
>   memory.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
>  diff --git a/memory.c b/memory.c
>  index 3212acc..bb45248 100644
>  --- a/memory.c
>  +++ b/memory.c
>  @@ -2712,7 +2712,7 @@ static void 
>  memory_region_do_invalidate_mmio_ptr(CPUState *cpu,
>   /* Reset dirty so this doesn't happen later. */
>   cpu_physical_memory_test_and_clear_dirty(offset, size, 1);
> 
>  -if (section.mr != mr) {
>  +if (section.mr && (section.mr != mr)) {
> >
> > section.mr can't be NULL here.
> >
> > You can give the static analyzer a hint using "assert(section.mr);"
> 
> Not in my view much point in messing with this code, though:
> (a) it's broken and unusable as it stands (race conditions)
> (b) it's obsoleted by the execute-from-mmio patchset
> http://patchwork.ozlabs.org/cover/942090/ and if/when that
> goes in it will all just get deleted.


Got it.

Thanks, Dima.

> 
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH] dump: add kernel_gs_base to QEMU CPU state

[Eduardo Habkost]

On Tue, Jul 10, 2018 at 06:21:09PM +0300, Viktor Prutyanov wrote:
> This patch adds field with content of KERNEL_GS_BASE MSR to QEMU note in
> ELF dump.
> 
> On Windows, if all vCPUs are running usermode tasks at the time the dump is
> created, this can be helpful in the discovery of guest system structures
> during conversion ELF dump to MEMORY.DMP dump.
> 
> Signed-off-by: Viktor Prutyanov 
> ---
>  target/i386/arch_dump.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/target/i386/arch_dump.c b/target/i386/arch_dump.c
> index 35b55fc..a702138 100644
> --- a/target/i386/arch_dump.c
> +++ b/target/i386/arch_dump.c
> @@ -237,7 +237,7 @@ int x86_cpu_write_elf32_note(WriteCoreDumpFunction f, 
> CPUState *cs,
>   * please count up QEMUCPUSTATE_VERSION if you have changed definition of
>   * QEMUCPUState, and modify the tools using this information accordingly.

Where are the tools using this information, that need to be
updated?  Won't this break existing versions of those tools?

Is the dump format and pointers to available tools documented
somewhere?


>   */
> -#define QEMUCPUSTATE_VERSION (1)
> +#define QEMUCPUSTATE_VERSION (2)
>  
>  struct QEMUCPUSegment {
>  uint32_t selector;
> @@ -258,6 +258,7 @@ struct QEMUCPUState {
>  QEMUCPUSegment cs, ds, es, fs, gs, ss;
>  QEMUCPUSegment ldt, tr, gdt, idt;
>  uint64_t cr[5];
> +uint64_t kernel_gs_base;
>  };
>  
>  typedef struct QEMUCPUState QEMUCPUState;
> @@ -315,6 +316,8 @@ static void qemu_get_cpustate(QEMUCPUState *s, 
> CPUX86State *env)
>  s->cr[2] = env->cr[2];
>  s->cr[3] = env->cr[3];
>  s->cr[4] = env->cr[4];
> +
> +s->kernel_gs_base = env->kernelgsbase;
>  }
>  
>  static inline int cpu_write_qemu_note(WriteCoreDumpFunction f,
> -- 
> 2.7.4
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 2/2] memory: fix possible NULL pointer dereference

[Dima Stepanov]

Hi Phil,

On Wed, Jul 11, 2018 at 10:47:18AM -0300, Philippe Mathieu-Daudé wrote:
> Hi Dima,
> 
> On 07/11/2018 05:34 AM, Dima Stepanov wrote:
> > Gentle ping. CCing Paolo Bonzini.
> > 
> > Regards, Dima.
> > 
> > On Tue, Jun 19, 2018 at 05:12:16PM +0300, Dima Stepanov wrote:
> >> Ping.
> >>
> >> Regards, Dima.
> >>
> >> On Wed, Jun 13, 2018 at 11:19:55AM +0300, Dima Stepanov wrote:
> >>> In the memory_region_do_invalidate_mmio_ptr() routine the section
> >>> variable is intialized by the memory_region_find() call. The section.mr
> >>> field can be set to NULL.
> >>>
> >>> Add the check for NULL before trying to drop a section.
> >>>
> >>> Signed-off-by: Dima Stepanov 
> >>> ---
> >>>  memory.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/memory.c b/memory.c
> >>> index 3212acc..bb45248 100644
> >>> --- a/memory.c
> >>> +++ b/memory.c
> >>> @@ -2712,7 +2712,7 @@ static void 
> >>> memory_region_do_invalidate_mmio_ptr(CPUState *cpu,
> >>>  /* Reset dirty so this doesn't happen later. */
> >>>  cpu_physical_memory_test_and_clear_dirty(offset, size, 1);
> >>>  
> >>> -if (section.mr != mr) {
> >>> +if (section.mr && (section.mr != mr)) {
> 
> section.mr can't be NULL here.
> 
> You can give the static analyzer a hint using "assert(section.mr);"

My point was:
  section = memory_region_find(mr, offset, size) ->
ret = memory_region_find_rcu(mr, addr, size); ->
  MemoryRegionSection ret = { .mr = NULL };
  ...
  as = memory_region_to_address_space(root);
  if this call returns NULL, then the ret value will be returned
  (with .mr == NULL)
But maybe it is impossible for this case. Thanks for the reply.

Regards, Dima.

> 
> >>>  /* memory_region_find add a ref on section.mr */
> >>>  memory_region_unref(section.mr);
> >>>  if (MMIO_INTERFACE(section.mr->owner)) {
> >>> -- 
> >>> 2.7.4
> >>>
> 
> Regards,
> 
> Phil.
> 

Re: [Qemu-devel] [PATCH v2 2/2] vga: don't pick cirrus by default

[Eduardo Habkost]

On Tue, Jul 10, 2018 at 12:26:52AM +0200, Sebastian Bauer wrote:
> Hi,
> 
> Am 2018-07-09 23:23, schrieb Eduardo Habkost:
> > List of machines with default_display==NULL on those
> > architectures:
> > 
> > alpha:
> > none empty machine
> > 
> > mips:
> > mipssim  MIPS MIPSsim platform
> > none empty machine
> > 
> > ppc*:
> > bamboo   bamboo
> > mpc8544dsmpc8544ds
> > none empty machine
> > powernv  IBM PowerNV (Non-Virtualized)
> > ppce500  generic paravirt e500 platform
> > ref405ep ref405ep
> > sam460ex aCube Sam460ex
> > taihutaihu
> > virtex-ml507 Xilinx Virtex ML507 reference design
> > 
> > x86_64:
> > isapcISA-only PC
> > none empty machine
> > xenfvXen Fully-virtualized PC
> > xenpvXen Para-virtualized PC
> 
> Which of these machines really require the Cirrus? The xen ones look like
> that they can deal with std. The isapc is should probably stay at the
> Cirrus.
> 
> Also the "none" seems to be a false-positive. I suppose they mean "empty",
> i.e., no graphics card at all?

"none" looked like a false positive when I first looked, but now
I think it's not.  Shouldn't it set default_display="none"?

> 
> And at least the ppc ones can be canceled out, they should work with std,
> the new default (expect the sam460ex which goes an own route for now).

If machines prefer "std", they should set default_display="std"
explicitly.

> 
> What is the indented target release for the patch?

I'm not convinced this patch is appropriate for 3.0.  If we have
remaining bugs they should be fixed by setting default_display
explicitly on the affected machines.

> 
> If the patch is applied to 3.1 then I think there is enough time to fix
> issues caused by the patch. Additionally, a warning could be put in the
> ChangeLog for 3.0 that in 3.1 that the default mode will be std unless
> machines define an own default. This is should be enough time for people to
> complain or to fix things.

I don't think we will really make user-visible changes: we can
simply work to keep existing behavior, but the difference is that
this will be implemented by setting default_display explicitly on
all machines.

> 
> If the patch is to be applied to 3.0 then all non-ppc ones need to be
> reconsidered.
> 
> The "important" ppc machines have been fixed already. I can do the remaining
> if this is wanted.

This part worries me: do we have other machines that are broken
right now?

-- 
Eduardo

[Qemu-devel] [Bug 1781211] Re: HAXM acceleration does not work at all.

[Dmitriy via Qemu-devel]

After some time I decided it is haxm bug - so i created the same issue
on haxm project too

https://github.com/intel/haxm/issues/74

** Bug watch added: github.com/intel/haxm/issues #74
   https://github.com/intel/haxm/issues/74

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1781211

Title:
  HAXM acceleration does not work at all.

Status in QEMU:
  New

Bug description:
  I have qemu windows build 2.12.90, haxm 7.2.0. Ubuntu, nor arch linux does 
not works when i turn on hax acceleration. Permanent kernel panics, black 
screen freezing and other crashes happens when i run qemu.
  Qemu crashed with hax - when i ran it from iso. It crashed on already 
installed system - it's not matters. 

  Versions:
  archlinux-2018.07.01-x86_64
  ubuntu-18.04-live-server-amd64.iso

  I run qemu-system-x86_64.exe binary.

  My CPU:
  core i7 2600k

  See screenshot

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1781211/+subscriptions

Re: [Qemu-devel] [PATCH for-3.0] pc: Use "3.0+" constant as default SMBIOS version

[Eduardo Habkost]

On Tue, Jul 10, 2018 at 10:07:31AM +0100, Daniel P. Berrangé wrote:
> On Mon, Jul 09, 2018 at 05:37:31PM -0300, Eduardo Habkost wrote:
> > Every time we create new PC machine-types in QEMU, the defaults
> > for SMBIOS fields change unnecessarily because the version field
> > defaults to MachineClass::name.
> > 
> > This can cause unexpected side-effects, like triggering license
> > reactivation on guest software, or changing the VM memory layout
> > because of BIOS table size changes.
> 
> Does that really matter though ? By its very nature the 'Version'
> field in SMBIOS is expected to change if you alter something about
> the hardware. If guests OS don't want to be exposed to changes in
> SMBIOS they would be using a fixed machine type, not the variable
> "pc" type that continually changes.
> 
> We could put padding in the string if we want to avoid BIOS table
> layout changes.
> 
> Having version change though feels like it is working as intended
> for the semantics of these Version: fields in BIOS.

Michael, do you have additional info on the original motivation
for suggesting this change and why do you consider it a bug?
(I don't have any concrete examples to justify the change)

-- 
Eduardo

Re: [Qemu-devel] [PATCH 4/7] tests/qgraph: arm/raspi2 machine node

[Paolo Bonzini]

On 11/07/2018 16:59, Stefan Hajnoczi wrote:
>> +machine->obj.get_device = raspi2_get_device;
>> +machine->obj.destructor = raspi2_destroy;
>> +qos_create_sdhci_mm(>sdhci, 0x3f30, &(QSDHCIProperties) {
>> +.version = 3,
>> +.baseclock = 52,
>> +.capab.sdma = false,
>> +.capab.reg = 0x052134b4
>> +});
>> +return >obj;
>> +}
>> +
>> +static void raspi2(void)
>> +{
>> +qos_node_create_machine("arm/raspi2", qos_create_machine_arm_raspi2);
>> +qos_node_contains("arm/raspi2", "generic-sdhci");
>> +}
>> +
>> +libqos_init(raspi2);
> Hmm...I didn't realize it would be necessary to manually define each
> machine type.  I thought qgraph would use qemu-system-x86_64
> -machine/-device/info qtree to introspect QEMU and instantiate the
> appropriate drivers.

That doesn't provide enough information yet.  However, PCI devices are
already discovered automatically and the next step (next year :)) could
be to use device tree or ACPI to discover embedded devices.

Using QOM properties instead of duplicating things like QSDHCIProperties
is a great idea.  Of course the duplication was already there ("old
sdhci_t structure" in patch 7), so I don't think it should be a blocker,
but indeed there's a better way to do it.

> My concern is that this manual approach of defining machines complicates
> qtests.  This file will need to be modified by multiple people - each of
> them will be interested in a specific driver and not interested in the
> driver that other people have added.

This is exactly the opposite problem that we have now: when you write a
test you are interested typically only in one machine, and the
"if(x86)elseif(arm)" gets in the way.  (Also: it's also difficult to
split the ifs across files, i.e. it scales worse; and there's lots of
duplicated code across tests to do "for" loops of g_test_add, because
the ifs don't lend itself to "automatic" generation of test cases via
path walk).

The advantage is that (just like for OS vs. QEMU) the structure of this
file matches closely the board files in hw/arm/, which is something you
should already be familiar with if you're adding a new board to QEMU.

Paolo

Re: [Qemu-devel] [PATCH 0/7] Qtest driver framework

[Stefan Hajnoczi]

On Mon, Jul 09, 2018 at 11:11:29AM +0200, Emanuele Giuseppe Esposito wrote:
> This work is being done as Google Summer of Code 2018 project for QEMU,
> my mentors are Paolo Bonzini and Laurent Vivier.
> Additional infos on the project can be found at:
> https://wiki.qemu.org/Features/qtest_driver_framework

Thanks, I've finished reviewing this version.  It looks like a good
start!

The main challenge to me seems "how can we make tests simpler?".  The
presence of a new API and object model raises the bar for writing and
running tests.  I hope all qtests will use qgraph but if the complexity
is too high then qgraph may only be used in a few niche cases where
authors think it's worth abstracting machine types and others will
continue to write qtests without qgraph.

What are the next steps and do you have plans for making qgraph more
integral to libqos?

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v2 4/4] tests: Add centos VM testing

[Philippe Mathieu-Daudé]

Hi Fam,

On 07/11/2018 11:18 AM, Fam Zheng wrote:
> This one does docker testing in the VM. It is intended to replace the
> native docker testing on patchew testers.
> 
> Signed-off-by: Fam Zheng 
> ---
>  tests/vm/Makefile.include |  2 +-
>  tests/vm/centos   | 84 +++
>  2 files changed, 85 insertions(+), 1 deletion(-)
>  create mode 100755 tests/vm/centos
> 
> diff --git a/tests/vm/Makefile.include b/tests/vm/Makefile.include
> index 5daa2a3b73..e492cd73e5 100644
> --- a/tests/vm/Makefile.include
> +++ b/tests/vm/Makefile.include
> @@ -2,7 +2,7 @@
>  
>  .PHONY: vm-build-all
>  
> -IMAGES := ubuntu.i386 freebsd netbsd openbsd
> +IMAGES := ubuntu.i386 freebsd netbsd openbsd centos

You missed to add an entry to the vm-test rule.

>  IMAGE_FILES := $(patsubst %, tests/vm/%.img, $(IMAGES))
>  
>  .PRECIOUS: $(IMAGE_FILES)
> diff --git a/tests/vm/centos b/tests/vm/centos
> new file mode 100755
> index 00..afd560c564
> --- /dev/null
> +++ b/tests/vm/centos
> @@ -0,0 +1,84 @@
> +#!/usr/bin/env python
> +#
> +# CentOS image
> +#
> +# Copyright 2018 Red Hat Inc.
> +#
> +# Authors:
> +#  Fam Zheng 
> +#
> +# This code is licensed under the GPL version 2 or later.  See
> +# the COPYING file in the top-level directory.
> +#
> +
> +import os
> +import sys
> +import subprocess
> +import basevm
> +import time
> +
> +class CentosVM(basevm.BaseVM):
> +name = "centos"
> +BUILD_SCRIPT = """
> +set -e;
> +cd $(mktemp -d);
> +export SRC_ARCHIVE=/dev/vdb;
> +sudo chmod a+r $SRC_ARCHIVE;
> +tar -xf $SRC_ARCHIVE;
> +make docker-test-block@centos7 V={verbose} J={jobs};
> +make docker-test-quick@centos7 V={verbose} J={jobs};
> +make docker-test-mingw@fedora V={verbose} J={jobs};
> +"""
> +
> +def _gen_cloud_init_iso(self):
> +cidir = self._tmpdir
> +mdata = open(os.path.join(cidir, "meta-data"), "w")
> +mdata.writelines(["instance-id: centos-vm-0\n",
> +  "local-hostname: centos-guest\n"])
> +mdata.close()
> +udata = open(os.path.join(cidir, "user-data"), "w")
> +udata.writelines(["#cloud-config\n",
> +  "chpasswd:\n",
> +  "  list: |\n",
> +  "root:%s\n" % self.ROOT_PASS,
> +  "%s:%s\n" % (self.GUEST_USER, self.GUEST_PASS),
> +  "  expire: False\n",
> +  "users:\n",
> +  "  - name: %s\n" % self.GUEST_USER,
> +  "sudo: ALL=(ALL) NOPASSWD:ALL\n",
> +  "ssh-authorized-keys:\n",
> +  "- %s\n" % basevm.SSH_PUB_KEY,
> +  "  - name: root\n",
> +  "ssh-authorized-keys:\n",
> +  "- %s\n" % basevm.SSH_PUB_KEY,
> +  "locale: en_US.UTF-8\n"])
> +udata.close()
> +subprocess.check_call(["genisoimage", "-output", "cloud-init.iso",
> +   "-volid", "cidata", "-joliet", "-rock",
> +   "user-data", "meta-data"],
> +   cwd=cidir,
> +   stdin=self._devnull, stdout=self._stdout,
> +   stderr=self._stdout)
> +return os.path.join(cidir, "cloud-init.iso")
> +
> +def build_image(self, img):
> +cimg = 
> self._download_with_cache("https://cloud.centos.org/centos/7/images/CentOS-7-x86_64-GenericCloud-1802.qcow2.xz;)
> +img_tmp = img + ".tmp"
> +subprocess.check_call(["cp", "-f", cimg, img_tmp + ".xz"])
> +subprocess.check_call(["xz", "-df", img_tmp + ".xz"])
> +subprocess.check_call(["qemu-img", "resize", img_tmp, "50G"])
> +self.boot(img_tmp, extra_args = ["-cdrom", 
> self._gen_cloud_init_iso()])
> +self.wait_ssh()
> +self.ssh_root_check("touch /etc/cloud/cloud-init.disabled")
> +self.ssh_root_check("yum update -y")
> +self.ssh_root_check("yum install -y docker make git")
> +self.ssh_root_check("systemctl enable docker")
> +self.ssh_root("poweroff")
> +self.wait()
> +if os.path.exists(img):
> +os.remove(img)
> +os.rename(img_tmp, img)
> +return 0
> +
> +if __name__ == "__main__":
> +sys.exit(basevm.main(CentosVM))
> 

Re: [Qemu-devel] [PATCH 1/2] qdev: add HotplugHandler->post_plug() callback

[Igor Mammedov]

On Tue, 10 Jul 2018 16:50:36 +0100
Stefan Hajnoczi  wrote:

> The ->pre_plug() callback is invoked before the device is realized.  The
> ->plug() callback is invoked when the device is being realized but  
> before it is reset.
> 
> This patch adds a ->post_plug() callback which is invoked after the
> device has been reset.  This callback is needed by HotplugHandlers that
> need to wait until after ->reset().
it seems other plug handlers would be also affected by this issue
if monitor lock wouldn't block them when guest tried to processes
hotplug notification and trapped back on MMIO happily waiting for
device_add to release lock before proceeding.

It also seems wrong to call _plug handler on maybe partially
initialized device so perhaps we should first finish devices/children
realization then do reset and only after that call _plug() handler

something like following should fix the issue:

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index cf0db4b..a80372d 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -830,6 +830,18 @@ static void device_set_realized(Object *obj, bool value, 
Error **errp)
 goto fail;
 }
 
+QLIST_FOREACH(bus, >child_bus, sibling) {
+object_property_set_bool(OBJECT(bus), true, "realized",
+ _err);
+if (local_err != NULL) {
+goto child_realize_fail;
+}
+}
+
+if (dev->hotplugged) {
+device_reset(dev);
+}
+
 DEVICE_LISTENER_CALL(realize, Forward, dev);
 
 if (hotplug_ctrl) {
@@ -837,7 +849,7 @@ static void device_set_realized(Object *obj, bool value, 
Error **errp)
 }
 
 if (local_err != NULL) {
-goto post_realize_fail;
+goto child_realize_fail;
 }
 
 /*
@@ -852,20 +864,9 @@ static void device_set_realized(Object *obj, bool value, 
Error **errp)
dev->instance_id_alias,
dev->alias_required_for_version,
_err) < 0) {
-goto post_realize_fail;
-}
-}
-
-QLIST_FOREACH(bus, >child_bus, sibling) {
-object_property_set_bool(OBJECT(bus), true, "realized",
- _err);
-if (local_err != NULL) {
 goto child_realize_fail;
 }
 }
-if (dev->hotplugged) {
-device_reset(dev);
-}
 dev->pending_deleted_event = false;
 } else if (!value && dev->realized) {
 Error **local_errp = NULL;
@@ -902,7 +903,6 @@ child_realize_fail:
 vmstate_unregister(dev, qdev_get_vmsd(dev), dev);
 }
 
-post_realize_fail:
 g_free(dev->canonical_path);
 dev->canonical_path = NULL;
 if (dc->unrealize) {


> 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  include/hw/hotplug.h | 12 
>  hw/core/hotplug.c| 11 +++
>  hw/core/qdev.c   |  7 +++
>  3 files changed, 30 insertions(+)
> 
> diff --git a/include/hw/hotplug.h b/include/hw/hotplug.h
> index 1a0516a479..1e8b1053b6 100644
> --- a/include/hw/hotplug.h
> +++ b/include/hw/hotplug.h
> @@ -47,6 +47,8 @@ typedef void (*hotplug_fn)(HotplugHandler *plug_handler,
>   * @parent: Opaque parent interface.
>   * @pre_plug: pre plug callback called at start of device.realize(true)
>   * @plug: plug callback called at end of device.realize(true).
> + * @post_plug: post plug callback called after device.realize(true) and 
> device
> + * reset
>   * @unplug_request: unplug request callback.
>   *  Used as a means to initiate device unplug for devices 
> that
>   *  require asynchronous unplug handling.
> @@ -61,6 +63,7 @@ typedef struct HotplugHandlerClass {
>  /*  */
>  hotplug_fn pre_plug;
>  hotplug_fn plug;
> +hotplug_fn post_plug;
>  hotplug_fn unplug_request;
>  hotplug_fn unplug;
>  } HotplugHandlerClass;
> @@ -83,6 +86,15 @@ void hotplug_handler_pre_plug(HotplugHandler *plug_handler,
>DeviceState *plugged_dev,
>Error **errp);
>  
> +/**
> + * hotplug_handler_post_plug:
> + *
> + * Call #HotplugHandlerClass.post_plug callback of @plug_handler.
> + */
> +void hotplug_handler_post_plug(HotplugHandler *plug_handler,
> +   DeviceState *plugged_dev,
> +   Error **errp);
> +
>  /**
>   * hotplug_handler_unplug_request:
>   *
> diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c
> index 17ac986685..ab34c19461 100644
> --- a/hw/core/hotplug.c
> +++ b/hw/core/hotplug.c
> @@ -35,6 +35,17 @@ void hotplug_handler_plug(HotplugHandler *plug_handler,
>  }
>  }
>  
> +void hotplug_handler_post_plug(HotplugHandler *plug_handler,
> +   DeviceState *plugged_dev,
> +   

[Qemu-devel] [Bug 1772075] Re: Segmentation fault on aarch64 vm at powerdown

[M0Rf30]

In order not to upload a big image I can say that you can generate the image 
with this tool
https://github.com/M0Rf30/simonpi
the initrd used is in the arch linux arm boot partition generated by the 
previous referenced tool.

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1772075

Title:
  Segmentation fault on aarch64 vm at powerdown

Status in QEMU:
  New

Bug description:
  OS Arch Linux
  x86_64
  qemu version: 2.12

  cmdline:
  qemu-system-aarch64 -nographic -cpu cortex-a57 -m 2048 -M virt,gic_version=3 
-machine virtualization=true -bios /usr/share/ovmf/AARCH64/QEMU_EFI.fd -drive 
file=fat:rw:/opt/simonpiemu/kernels/rpi-3,if=none,format=raw,cache=none,id=hd0 
-device virtio-blk-device,drive=hd0 -drive 
file=/home/morfeo/.simonpi/sd-arch-rpi-3-qemu.img,if=none,format=raw,cache=none,id=hd1
 -device virtio-blk-device,drive=hd1 -kernel 
/opt/simonpiemu/kernels/rpi-3/Image -append "root=/dev/vda2 fstab=no 
rootfstype=ext4 rw console=ttyAMA0" -initrd 
/home/morfeo/.simonpi/rpi-3/boot/initramfs-linux.img -device 
virtio-net-device,mac=52:54:26:11:72:9b,netdev=net0 -netdev 
tap,id=net0,ifname=rasp-tap0,script=no,downscript=no

  error:

  qemu-system-aarch64: /build/qemu/src/qemu-2.12.0/block.c:3375:
  bdrv_close_all: Assertion `QTAILQ_EMPTY(_bdrv_states)' failed.

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1772075/+subscriptions

Re: [Qemu-devel] [PATCH 2/7] tests/qgraph: pci-pc driver and interface nodes

[Paolo Bonzini]

On 11/07/2018 16:49, Stefan Hajnoczi wrote:
> On Mon, Jul 09, 2018 at 11:11:31AM +0200, Emanuele Giuseppe Esposito wrote:
>> -QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc)
>> +static void *qpci_get_driver(void *obj, const char *interface)
>>  {
>> -QPCIBusPC *ret = g_new0(QPCIBusPC, 1);
>> +QPCIBusPC *qpci = obj;
>> +if (!g_strcmp0(interface, "pci-bus")) {
>> +return >bus;
>> +}
>> +printf("%s not present in pci-bus-pc", interface);
>> +abort();
>> +}
> 
> At this point I wonder if it makes sense to use the QEMU Object Model
> (QOM), which has interfaces and inheritance.  qgraph duplicates part of
> the object model.

Replying for Emanuele on this point since we didn't really go through
QOM yet; I'll let him answer the comments that are more related to the code.

QOM is much heavier weight than qgraph, and adds a lot more boilerplate:

- QOM properties interact with QAPI and bring in a lot of baggage;
qgraph would only use "child" properties to implement containment.

- QOM objects are long-lived, qgraph objects only last for the duration
of a single test.  qgraph doesn't need reference counting or complex
two-phase initialization like "realize" or "user_complete"

- QOM's hierarchy is shallow, but qgraph doesn't really need _any_
hierarchy.  Because it focuses on interface rather than hierarchy,
everything can be expressed simply through structs contained into one
another.

Consider that qgraph is 1/4th the size of QOM, and a large part of it is
the graph data structure that (as you said) would not be provided by QOM.

There are two things where using QOM would save a little bit of
duplicated concepts, namely the get_driver (get interface, what you
quote above) and get_device (get contained object) callbacks.  However,
it wouldn't simplify the code at all, because it would require the
introduction of separate instance and class structs.  We didn't want to
add all too much boilerplate for people that want to write qtest, as you
pointed out in the review of patch 4.

>> +void qpci_set_pc(QPCIBusPC *ret, QTestState *qts, QGuestAllocator *alloc)
> 
> It's not clear to me what the purpose of this function is - at least the
> name is a bit cryptic since it seems more like an initialization
> function than 'setting pc' on QPCIBusPC.  How about inlining this in
> qpci_init_pc() instead of keeping a separate function?

I agree about the naming.  Perhaps we can rename the existing
qpci_init_pc to qpci_pc_new, and qpci_set_pc to qpci_pc_init.

Would you prefer if the split was done in the patch that introduces the
user for qpci_set_pc (patch 5)?  We did it here because this patch
prepares qpci-pc

Thanks,

Paolo

>> +{
>>  assert(qts);
>>  
>>  ret->bus.pio_readb = qpci_pc_pio_readb;
>> @@ -147,11 +164,23 @@ QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator 
>> *alloc)
>>  ret->bus.mmio_alloc_ptr = 0xE000;
>>  ret->bus.mmio_limit = 0x1ULL;
>>  
>> +ret->obj.get_driver = qpci_get_driver;
>> +}
>> +
>> +QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc)
>> +{
>> +QPCIBusPC *ret = g_new0(QPCIBusPC, 1);
>> +qpci_set_pc(ret, qts, alloc);
>> +
>>  return >bus;
>>  }
>>  
>>  void qpci_free_pc(QPCIBus *bus)
>>  {
>> +if (!bus) {
>> +return;
>> +}
> 
> Why is this needed now?
> 
>> +
>>  QPCIBusPC *s = container_of(bus, QPCIBusPC, bus);
>>  
>>  g_free(s);
>> @@ -176,3 +205,11 @@ void qpci_unplug_acpi_device_test(const char *id, 
>> uint8_t slot)
>>  
>>  qmp_eventwait("DEVICE_DELETED");
>>  }
>> +
>> +static void qpci_pc(void)
>> +{
>> +qos_node_create_driver("pci-bus-pc", NULL);
>> +qos_node_produces("pci-bus-pc", "pci-bus");
> 
> In QOM pci-bus-pc would be a class, pci-bus would be an interface.  From
> a driver perspective it seems QOM can already do what is needed and the
> qgraph infrastructure isn't necessary.
> 
> Obviously the depth-first search *is* unique and not in QOM, although
> QOM does offer a tree namespace which can be used for looking up object
> instances and I guess this could be used to configure tests at runtime.
> 
> I'll think about this more as I read the rest of the patches.
> 
>> +}
>> +
>> +libqos_init(qpci_pc);
>> diff --git a/tests/libqos/pci-pc.h b/tests/libqos/pci-pc.h
>> index 491eeac756..ee381c5667 100644
>> --- a/tests/libqos/pci-pc.h
>> +++ b/tests/libqos/pci-pc.h
>> @@ -15,7 +15,15 @@
>>  
>>  #include "libqos/pci.h"
>>  #include "libqos/malloc.h"
>> +#include "qgraph.h"
>>  
>> +typedef struct QPCIBusPC {
>> +QOSGraphObject obj;
>> +QPCIBus bus;
>> +} QPCIBusPC;
> 
> Why does this need to be public?
> 
>> +
>> +void qpci_device_init(QPCIDevice *dev, QPCIBus *bus, int devfn);
> 
> Why does this need to be public?
> 
>> +void qpci_set_pc(QPCIBusPC *ret, QTestState *qts, QGuestAllocator *alloc);
> 
> Why does this need to be public?
> 
>>  QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc);
>>  void qpci_free_pc(QPCIBus 

Re: [Qemu-devel] [PATCH v2 2/4] tests/vm: Pass verbose flag into VM make commands

[Philippe Mathieu-Daudé]

On 07/11/2018 11:18 AM, Fam Zheng wrote:
> Our Makefile has:
> 
> vm-build-%: tests/vm/%.img
>   $(call quiet-command, \
>   $(SRC_PATH)/tests/vm/$* \
>   $(if $(V)$(DEBUG), --debug) \
>   $(if $(DEBUG), --interactive) \
> 
> the intention of which is to let the make command in VM have V=1 if
> V=1 is set. We pass this to the BUILD_SCRIPT format.
> 
> Signed-off-by: Fam Zheng 

Reviewed-by: Philippe Mathieu-Daudé 

> ---
>  tests/vm/basevm.py   | 3 ++-
>  tests/vm/freebsd | 4 ++--
>  tests/vm/netbsd  | 4 ++--
>  tests/vm/openbsd | 4 ++--
>  tests/vm/ubuntu.i386 | 4 ++--
>  5 files changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py
> index e5d6a328d5..25361c6b7d 100755
> --- a/tests/vm/basevm.py
> +++ b/tests/vm/basevm.py
> @@ -240,7 +240,8 @@ def main(vmcls):
>  vm.add_source_dir(args.build_qemu)
>  cmd = [vm.BUILD_SCRIPT.format(
> configure_opts = " ".join(argv),
> -   jobs=args.jobs)]
> +   jobs=args.jobs,
> +   verbose="1" if args.debug else "")]
>  else:
>  cmd = argv
>  img = args.image
> diff --git a/tests/vm/freebsd b/tests/vm/freebsd
> index 039dad8f69..b20612a6c9 100755
> --- a/tests/vm/freebsd
> +++ b/tests/vm/freebsd
> @@ -23,8 +23,8 @@ class FreeBSDVM(basevm.BaseVM):
>  cd $(mktemp -d /var/tmp/qemu-test.XX);
>  tar -xf /dev/vtbd1;
>  ./configure {configure_opts};
> -gmake -j{jobs};
> -gmake check;
> +gmake -j{jobs} V={verbose};
> +gmake check V={verbose};
>  """
>  
>  def build_image(self, img):
> diff --git a/tests/vm/netbsd b/tests/vm/netbsd
> index 3972d8b45c..3f9d34a208 100755
> --- a/tests/vm/netbsd
> +++ b/tests/vm/netbsd
> @@ -23,8 +23,8 @@ class NetBSDVM(basevm.BaseVM):
>  cd $(mktemp -d /var/tmp/qemu-test.XX);
>  tar -xf /dev/rld1a;
>  ./configure --python=python2.7 {configure_opts};
> -gmake -j{jobs};
> -gmake check;
> +gmake -j{jobs} V={verbose};
> +gmake check V={verbose};
>  """
>  
>  def build_image(self, img):
> diff --git a/tests/vm/openbsd b/tests/vm/openbsd
> index 6ae16d97fd..3285c1abde 100755
> --- a/tests/vm/openbsd
> +++ b/tests/vm/openbsd
> @@ -23,9 +23,9 @@ class OpenBSDVM(basevm.BaseVM):
>  cd $(mktemp -d /var/tmp/qemu-test.XX);
>  tar -xf /dev/rsd1c;
>  ./configure --cc=x86_64-unknown-openbsd6.1-gcc-4.9.4 
> --python=python2.7 {configure_opts};
> -gmake -j{jobs};
> +gmake -j{jobs} V={verbose};
>  # XXX: "gmake check" seems to always hang or fail
> -#gmake check;
> +#gmake check V={verbose};
>  """
>  
>  def build_image(self, img):
> diff --git a/tests/vm/ubuntu.i386 b/tests/vm/ubuntu.i386
> index fc319e0e6e..b4eaa0dedc 100755
> --- a/tests/vm/ubuntu.i386
> +++ b/tests/vm/ubuntu.i386
> @@ -25,8 +25,8 @@ class UbuntuX86VM(basevm.BaseVM):
>  sudo chmod a+r /dev/vdb;
>  tar -xf /dev/vdb;
>  ./configure {configure_opts};
> -make -j{jobs};
> -make check;
> +make -j{jobs} V={verbose};
> +make check V={verbose};
>  """
>  
>  def _gen_cloud_init_iso(self):
> 

Re: [Qemu-devel] [PATCH 7/7] tests/qgraph: sdhci test node

[Stefan Hajnoczi]

On Mon, Jul 09, 2018 at 11:11:36AM +0200, Emanuele Giuseppe Esposito wrote:
> +/**
> + * Old sdhci_t structure:

Do you intend to delete this comment before this series is merged?  It
seems like a TODO that doesn't need to be kept around.

> +qos_add_test("sdhci-test", "sdhci", test_machine);

How does this work for tests that need access to more than 1 device?
Can they request a driver instance via the API?


signature.asc
Description: PGP signature

Re: [Qemu-devel] [Qemu-arm] [PATCH 6/6] target/arm: Allow execution from small regions

[Philippe Mathieu-Daudé]

On 07/10/2018 01:00 PM, Peter Maydell wrote:
> Now that we have full support for small regions, including execution,
> we can remove the workarounds where we marked all small regions as
> non-executable for the M-profile MPU and SAU.
> 
> Signed-off-by: Peter Maydell 

Reviewed-by: Philippe Mathieu-Daudé 

> ---
>  target/arm/helper.c | 23 ---
>  1 file changed, 23 deletions(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index a2ac96084e7..ed96e6c02fb 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -9784,17 +9784,6 @@ static bool get_phys_addr_pmsav7(CPUARMState *env, 
> uint32_t address,
>  
>  fi->type = ARMFault_Permission;
>  fi->level = 1;
> -/*
> - * Core QEMU code can't handle execution from small pages yet, so
> - * don't try it. This way we'll get an MPU exception, rather than
> - * eventually causing QEMU to exit in get_page_addr_code().
> - */
> -if (*page_size < TARGET_PAGE_SIZE && (*prot & PAGE_EXEC)) {
> -qemu_log_mask(LOG_UNIMP,
> -  "MPU: No support for execution from regions "
> -  "smaller than 1K\n");
> -*prot &= ~PAGE_EXEC;
> -}
>  return !(*prot & (1 << access_type));
>  }
>  
> @@ -10014,18 +10003,6 @@ static bool pmsav8_mpu_lookup(CPUARMState *env, 
> uint32_t address,
>  
>  fi->type = ARMFault_Permission;
>  fi->level = 1;
> -/*
> - * Core QEMU code can't handle execution from small pages yet, so
> - * don't try it. This means any attempted execution will generate
> - * an MPU exception, rather than eventually causing QEMU to exit in
> - * get_page_addr_code().
> - */
> -if (*is_subpage && (*prot & PAGE_EXEC)) {
> -qemu_log_mask(LOG_UNIMP,
> -  "MPU: No support for execution from regions "
> -  "smaller than 1K\n");
> -*prot &= ~PAGE_EXEC;
> -}
>  return !(*prot & (1 << access_type));
>  }
>  
> 

Re: [Qemu-devel] [PATCH 6/7] tests/qgraph: gtest integration

[Stefan Hajnoczi]

On Mon, Jul 09, 2018 at 11:11:35AM +0200, Emanuele Giuseppe Esposito wrote:
> Add main executable that takes care of starting the framework, create the
> nodes, set the available drivers/machines, discover the path and run tests.

This is elegant, I like it.

> Signed-off-by: Emanuele Giuseppe Esposito 
> ---
>  tests/Makefile.include |   3 +
>  tests/qos-test.c   | 310 +
>  2 files changed, 313 insertions(+)
>  create mode 100644 tests/qos-test.c
> 
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index 706ac39ea5..e2db3e9d09 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -776,6 +776,9 @@ libqgraph-pc-obj-y += tests/libqos/raspi2-machine.o 
> tests/libqos/x86_64_pc-machi
>  check-unit-y += tests/test-qgraph$(EXESUF)
>  tests/test-qgraph$(EXESUF): tests/test-qgraph.o $(libqgraph-obj-y)
>  
> +check-qtest-pci-y += tests/qos-test$(EXESUF)
> +tests/qos-test$(EXESUF): tests/qos-test.o $(libqgraph-pc-obj-y)
> +
>  tests/qmp-test$(EXESUF): tests/qmp-test.o
>  tests/device-introspect-test$(EXESUF): tests/device-introspect-test.o
>  tests/rtc-test$(EXESUF): tests/rtc-test.o
> diff --git a/tests/qos-test.c b/tests/qos-test.c
> new file mode 100644
> index 00..f4748c44a4
> --- /dev/null
> +++ b/tests/qos-test.c
> @@ -0,0 +1,310 @@
> +/*
> + * libqos driver framework
> + *
> + * Copyright (c) 2018 Emanuele Giuseppe Esposito 
> 
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License version 2 as published by the Free Software Foundation.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see 
> 
> + */
> +
> +#include "qemu/osdep.h"
> +#include "libqtest.h"
> +#include "qapi/qmp/qdict.h"
> +#include "qapi/qmp/qstring.h"
> +#include "qapi/qmp/qlist.h"
> +#include "libqos/qgraph.h"
> +#include "libqos/qgraph_extra.h"
> +
> +/**
> + * create_machine_name(): appends the architecture to @name if
> + * @is_machine is valid.
> + */
> +static void create_machine_name(const char **name, bool is_machine)
> +{
> +const char *arch;
> +if (!is_machine) {
> +return;
> +}
> +arch = qtest_get_arch();
> +*name = g_strconcat(arch, "/", *name, NULL);
> +}
> +
> +/**
> + * destroy_machine_name(): frees the given @name if
> + * @is_machine is valid.
> + */
> +static void destroy_machine_name(const char *name, bool is_machine)
> +{
> +if (!is_machine) {
> +return;
> +}
> +g_free((char *)name);
> +}
> +
> +/**
> + * apply_to_qlist(): using QMP queries QEMU for a list of
> + * machines and devices available, and sets the respective node
> + * as TRUE. If a node is found, also all its produced and contained
> + * child are marked available.
> + *
> + * See qos_graph_node_set_availability() for more info
> + */
> +static void apply_to_qlist(QList *list, bool is_machine)
> +{
> +const QListEntry *p;
> +const char *name;
> +QDict *minfo;
> +QObject *qobj;
> +QString *qstr;
> +
> +for (p = qlist_first(list); p; p = qlist_next(p)) {
> +minfo = qobject_to(QDict, qlist_entry_obj(p));
> +g_assert(minfo);
> +qobj = qdict_get(minfo, "name");
> +g_assert(qobj);
> +qstr = qobject_to(QString, qobj);
> +g_assert(qstr);
> +name = qstring_get_str(qstr);
> +create_machine_name(, is_machine);
> +qos_graph_node_set_availability(name, TRUE);
> +destroy_machine_name(name, is_machine);
> +qobj = qdict_get(minfo, "alias");
> +if (qobj) {
> +g_assert(qobj);
> +qstr = qobject_to(QString, qobj);
> +g_assert(qstr);
> +name = qstring_get_str(qstr);
> +create_machine_name(, is_machine);
> +qos_graph_node_set_availability(name, TRUE);
> +destroy_machine_name(name, is_machine);
> +}
> +}
> +}
> +
> +/**
> + * qos_set_machines_devices_available(): sets availability of qgraph
> + * machines and devices.
> + *
> + * This function firstly starts QEMU with "-machine none" option,
> + * and then executes the QMP protocol asking for the list of devices
> + * and machines available.
> + *
> + * for each of these items, it looks up the corresponding qgraph node,
> + * setting it as available. The list currently returns all devices that
> + * are either machines or CONSUMED_BY other nodes.
> + * Therefore, in order to mark all other nodes, it recursively sets
> + * all its CONTAINS and PRODUCES child as available too.
> + */
> +static void 

Re: [Qemu-devel] [PATCH 4/7] tests/qgraph: arm/raspi2 machine node

[Stefan Hajnoczi]

On Mon, Jul 09, 2018 at 11:11:33AM +0200, Emanuele Giuseppe Esposito wrote:
> Add arm/raspi2 machine to the graph. This machine contains a generic-sdhci, so
> its constructor must take care of setting it properly when called.
> 
> Signed-off-by: Emanuele Giuseppe Esposito 
> ---
>  tests/Makefile.include|  3 +-
>  tests/libqos/raspi2-machine.c | 68 +++
>  2 files changed, 70 insertions(+), 1 deletion(-)
>  create mode 100644 tests/libqos/raspi2-machine.c
> 
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index acbf704a8a..de75a7394e 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -770,7 +770,8 @@ libqos-usb-obj-y = $(libqos-spapr-obj-y) 
> $(libqos-pc-obj-y) tests/libqos/usb.o
>  libqos-virtio-obj-y = $(libqos-spapr-obj-y) $(libqos-pc-obj-y) 
> tests/libqos/virtio.o tests/libqos/virtio-pci.o tests/libqos/virtio-mmio.o 
> tests/libqos/malloc-generic.o
>  
>  libqgraph-obj-y = tests/libqos/qgraph.o
> -libqgraph-pc-obj-y = $(libqos-pc-obj-y) tests/libqos/sdhci.o
> +libqgraph-pc-obj-y = $(libqos-pc-obj-y) $(libqgraph-obj-y) 
> tests/libqos/sdhci.o
> +libqgraph-pc-obj-y += tests/libqos/raspi2-machine.o
>  
>  check-unit-y += tests/test-qgraph$(EXESUF)
>  tests/test-qgraph$(EXESUF): tests/test-qgraph.o $(libqgraph-obj-y)
> diff --git a/tests/libqos/raspi2-machine.c b/tests/libqos/raspi2-machine.c
> new file mode 100644
> index 00..47f024076f
> --- /dev/null
> +++ b/tests/libqos/raspi2-machine.c
> @@ -0,0 +1,68 @@
> +/*
> + * libqos driver framework
> + *
> + * Copyright (c) 2018 Emanuele Giuseppe Esposito 
> 
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License version 2 as published by the Free Software Foundation.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see 
> 
> + */
> +
> +#include "qemu/osdep.h"
> +#include "libqtest.h"
> +#include "qgraph.h"
> +#include "sdhci.h"
> +
> +typedef struct QRaspi2Machine QRaspi2Machine;
> +
> +struct QRaspi2Machine {
> +QOSGraphObject obj;
> +QSDHCI_MemoryMapped sdhci;
> +};
> +
> +static void raspi2_destroy(QOSGraphObject *obj)
> +{
> +g_free(obj);
> +}
> +
> +static QOSGraphObject *raspi2_get_device(void *obj, const char *device)
> +{
> +QRaspi2Machine *machine = obj;
> +if (!g_strcmp0(device, "generic-sdhci")) {
> +return >sdhci.obj;
> +}
> +
> +printf("%s not present in arm/raspi2", device);
> +abort();
> +}
> +
> +static void *qos_create_machine_arm_raspi2(void)
> +{
> +QRaspi2Machine *machine = g_new0(QRaspi2Machine, 1);
> +
> +machine->obj.get_device = raspi2_get_device;
> +machine->obj.destructor = raspi2_destroy;
> +qos_create_sdhci_mm(>sdhci, 0x3f30, &(QSDHCIProperties) {
> +.version = 3,
> +.baseclock = 52,
> +.capab.sdma = false,
> +.capab.reg = 0x052134b4
> +});
> +return >obj;
> +}
> +
> +static void raspi2(void)
> +{
> +qos_node_create_machine("arm/raspi2", qos_create_machine_arm_raspi2);
> +qos_node_contains("arm/raspi2", "generic-sdhci");
> +}
> +
> +libqos_init(raspi2);

Hmm...I didn't realize it would be necessary to manually define each
machine type.  I thought qgraph would use qemu-system-x86_64
-machine/-device/info qtree to introspect QEMU and instantiate the
appropriate drivers.

My concern is that this manual approach of defining machines complicates
qtests.  This file will need to be modified by multiple people - each of
them will be interested in a specific driver and not interested in the
driver that other people have added.

This makes things more complicated where previous qtests simply had an
if (x86) { add_test(test_x86_foo); } else if (arm) {
add_test(test_arm_foo); } in their source file.  It's ugly but it's
obvious.

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH 1/7] tests: qgraph API for the qtest driver framework

[Paolo Bonzini]

On 11/07/2018 16:28, Stefan Hajnoczi wrote:
>> + * build_driver_cmd_line(): builds the command line for the driver
>> + * @node. The node name must be a valid qemu identifier, since it
>> + * will be used to build the command line.
>> + *
>> + * It is also possible to pass an optional @args that will be
>> + * concatenated to the command line.
>> + *
>> + * For drivers, prepend -device to the driver name.
>> + */
>> +static void build_driver_cmd_line(QOSGraphNode *node, const char *args)
> Why is this called "driver" instead of "device"?
> 

It's the command line that is needed for the driver to work; it can
include also e.g. a -netdev or -blockdev option, though the most common
case is to have just -device.

>> + *
>> + * QOSGraphObject also provides a destructor, used to deallocate the
>> + * after the test has been executed.
>> + */
>> +struct QOSGraphObject {
>> +/* for produces, returns void * */
>> +QOSGetDriver get_driver;
> 
> Unused?
> 
>> +/* for contains, returns a QOSGraphObject * */
>> +QOSGetDevice get_device;
> 
> Unused?

What is unused?

Thanks,

Paolo

Re: [Qemu-devel] [PATCH 2/7] tests/qgraph: pci-pc driver and interface nodes

[Stefan Hajnoczi]

On Mon, Jul 09, 2018 at 11:11:31AM +0200, Emanuele Giuseppe Esposito wrote:
> -QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc)
> +static void *qpci_get_driver(void *obj, const char *interface)
>  {
> -QPCIBusPC *ret = g_new0(QPCIBusPC, 1);
> +QPCIBusPC *qpci = obj;
> +if (!g_strcmp0(interface, "pci-bus")) {
> +return >bus;
> +}
> +printf("%s not present in pci-bus-pc", interface);
> +abort();
> +}

At this point I wonder if it makes sense to use the QEMU Object Model
(QOM), which has interfaces and inheritance.  qgraph duplicates part of
the object model.

> +void qpci_device_init(QPCIDevice *dev, QPCIBus *bus, int devfn)
> +{
> +if (!bus) {
> +return;
> +}

When does this happen and why?

> +dev->bus = bus;
> +dev->devfn = devfn;
> +
> +if (qpci_config_readw(dev, PCI_VENDOR_ID) == 0x) {
> +printf("PCI Device not found\n");
> +abort();
> +}
> +qpci_device_enable(dev);
> +}
> +
> +void qpci_set_pc(QPCIBusPC *ret, QTestState *qts, QGuestAllocator *alloc)

It's not clear to me what the purpose of this function is - at least the
name is a bit cryptic since it seems more like an initialization
function than 'setting pc' on QPCIBusPC.  How about inlining this in
qpci_init_pc() instead of keeping a separate function?

> +{
>  assert(qts);
>  
>  ret->bus.pio_readb = qpci_pc_pio_readb;
> @@ -147,11 +164,23 @@ QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator 
> *alloc)
>  ret->bus.mmio_alloc_ptr = 0xE000;
>  ret->bus.mmio_limit = 0x1ULL;
>  
> +ret->obj.get_driver = qpci_get_driver;
> +}
> +
> +QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc)
> +{
> +QPCIBusPC *ret = g_new0(QPCIBusPC, 1);
> +qpci_set_pc(ret, qts, alloc);
> +
>  return >bus;
>  }
>  
>  void qpci_free_pc(QPCIBus *bus)
>  {
> +if (!bus) {
> +return;
> +}

Why is this needed now?

> +
>  QPCIBusPC *s = container_of(bus, QPCIBusPC, bus);
>  
>  g_free(s);
> @@ -176,3 +205,11 @@ void qpci_unplug_acpi_device_test(const char *id, 
> uint8_t slot)
>  
>  qmp_eventwait("DEVICE_DELETED");
>  }
> +
> +static void qpci_pc(void)
> +{
> +qos_node_create_driver("pci-bus-pc", NULL);
> +qos_node_produces("pci-bus-pc", "pci-bus");

In QOM pci-bus-pc would be a class, pci-bus would be an interface.  From
a driver perspective it seems QOM can already do what is needed and the
qgraph infrastructure isn't necessary.

Obviously the depth-first search *is* unique and not in QOM, although
QOM does offer a tree namespace which can be used for looking up object
instances and I guess this could be used to configure tests at runtime.

I'll think about this more as I read the rest of the patches.

> +}
> +
> +libqos_init(qpci_pc);
> diff --git a/tests/libqos/pci-pc.h b/tests/libqos/pci-pc.h
> index 491eeac756..ee381c5667 100644
> --- a/tests/libqos/pci-pc.h
> +++ b/tests/libqos/pci-pc.h
> @@ -15,7 +15,15 @@
>  
>  #include "libqos/pci.h"
>  #include "libqos/malloc.h"
> +#include "qgraph.h"
>  
> +typedef struct QPCIBusPC {
> +QOSGraphObject obj;
> +QPCIBus bus;
> +} QPCIBusPC;

Why does this need to be public?

> +
> +void qpci_device_init(QPCIDevice *dev, QPCIBus *bus, int devfn);

Why does this need to be public?

> +void qpci_set_pc(QPCIBusPC *ret, QTestState *qts, QGuestAllocator *alloc);

Why does this need to be public?

>  QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc);
>  void qpci_free_pc(QPCIBus *bus);
>  
> diff --git a/tests/libqos/pci.c b/tests/libqos/pci.c
> index 0b73cb23d0..c51c186867 100644
> --- a/tests/libqos/pci.c
> +++ b/tests/libqos/pci.c
> @@ -15,6 +15,7 @@
>  
>  #include "hw/pci/pci_regs.h"
>  #include "qemu/host-utils.h"
> +#include "qgraph.h"
>  
>  void qpci_device_foreach(QPCIBus *bus, int vendor_id, int device_id,
>   void (*func)(QPCIDevice *dev, int devfn, void 
> *data),
> @@ -402,3 +403,10 @@ void qpci_plug_device_test(const char *driver, const 
> char *id,
>  qtest_qmp_device_add(driver, id, "'addr': '%d'%s%s", slot,
>   opts ? ", " : "", opts ? opts : "");
>  }
> +
> +static void qpci(void)
> +{
> +qos_node_create_interface("pci-bus");
> +}
> +
> +libqos_init(qpci);

Why does an interface need to be created?  The drivers declare which
interfaces they support?

I don't think this can be used to detect typoes in the driver's
qos_node_produces() call since there is no explicit control over the
order in which libqos_init() functions are called.  So the driver may
call qos_node_produces() before the qos_node_create_interface() is
called?


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH] seccomp: allow sched_setscheduler() with SCHED_IDLE policy

[Eduardo Otubo]

On 10/07/2018 - 16:55:57, Marc-André Lureau wrote:
> Current and upcoming mesa releases rely on a shader disk cash. It uses
> a thread job queue with low priority, set with
> sched_setscheduler(SCHED_IDLE). However, that syscall is rejected by
> the "resourcecontrol" seccomp qemu filter.
> 
> Since it should be safe to allow lowering thread priority, let's allow
> scheduling thread to idle policy.
> 
> Related to:
> https://bugzilla.redhat.com/show_bug.cgi?id=1594456
> 
> Signed-off-by: Marc-André Lureau 
> ---
>  qemu-seccomp.c | 12 ++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/qemu-seccomp.c b/qemu-seccomp.c
> index 148e4c6f24..9cd8eb9499 100644
> --- a/qemu-seccomp.c
> +++ b/qemu-seccomp.c
> @@ -34,6 +34,12 @@
>  struct QemuSeccompSyscall {
>  int32_t num;
>  uint8_t set;
> +uint8_t narg;
> +const struct scmp_arg_cmp *arg_cmp;
> +};
> +
> +const struct scmp_arg_cmp sched_setscheduler_arg[] = {
> +SCMP_A1(SCMP_CMP_NE, SCHED_IDLE)
>  };
>  
>  static const struct QemuSeccompSyscall blacklist[] = {
> @@ -92,7 +98,8 @@ static const struct QemuSeccompSyscall blacklist[] = {
>  { SCMP_SYS(setpriority),QEMU_SECCOMP_SET_RESOURCECTL },
>  { SCMP_SYS(sched_setparam), QEMU_SECCOMP_SET_RESOURCECTL },
>  { SCMP_SYS(sched_getparam), QEMU_SECCOMP_SET_RESOURCECTL },
> -{ SCMP_SYS(sched_setscheduler), QEMU_SECCOMP_SET_RESOURCECTL },
> +{ SCMP_SYS(sched_setscheduler), QEMU_SECCOMP_SET_RESOURCECTL,
> +  ARRAY_SIZE(sched_setscheduler_arg), sched_setscheduler_arg },
>  { SCMP_SYS(sched_getscheduler), QEMU_SECCOMP_SET_RESOURCECTL },
>  { SCMP_SYS(sched_setaffinity),  QEMU_SECCOMP_SET_RESOURCECTL },
>  { SCMP_SYS(sched_getaffinity),  QEMU_SECCOMP_SET_RESOURCECTL },
> @@ -118,7 +125,8 @@ static int seccomp_start(uint32_t seccomp_opts)
>  continue;
>  }
>  
> -rc = seccomp_rule_add(ctx, SCMP_ACT_KILL, blacklist[i].num, 0);
> +rc = seccomp_rule_add_array(ctx, SCMP_ACT_KILL, blacklist[i].num,
> +blacklist[i].narg, blacklist[i].arg_cmp);
>  if (rc < 0) {
>  goto seccomp_return;
>  }
> -- 
> 2.18.0.129.ge3331758f1
> 

Acked-by: Eduardo Otubo 

Patch looks safe enough for me. If everyone else is OK with this I'll send a
pull-request tomorrow morning.


signature.asc
Description: PGP signature

Re: [Qemu-devel] [Qemu-arm] [PATCH 5/6] accel/tcg: Return -1 for execution from MMIO regions in get_page_addr_code()

[Philippe Mathieu-Daudé]

On 07/10/2018 01:00 PM, Peter Maydell wrote:
> Now that all the callers can handle get_page_addr_code() returning -1,
> remove all the code which tries to handle execution from MMIO regions
> or small-MMU-region RAM areas. This will mean that we can correctly
> execute from these areas, rather than ending up either aborting QEMU
> or delivering an incorrect guest exception.
> 
> Signed-off-by: Peter Maydell 

Reviewed-by: Philippe Mathieu-Daudé 
Tested-by: Philippe Mathieu-Daudé 

> ---
>  accel/tcg/cputlb.c | 95 +-
>  1 file changed, 10 insertions(+), 85 deletions(-)
> 
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index c491703f15f..abb0225dc79 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -741,39 +741,6 @@ void tlb_set_page(CPUState *cpu, target_ulong vaddr,
>  prot, mmu_idx, size);
>  }
>  
> -static void report_bad_exec(CPUState *cpu, target_ulong addr)
> -{
> -/* Accidentally executing outside RAM or ROM is quite common for
> - * several user-error situations, so report it in a way that
> - * makes it clear that this isn't a QEMU bug and provide suggestions
> - * about what a user could do to fix things.
> - */
> -error_report("Trying to execute code outside RAM or ROM at 0x"
> - TARGET_FMT_lx, addr);
> -error_printf("This usually means one of the following happened:\n\n"
> - "(1) You told QEMU to execute a kernel for the wrong 
> machine "
> - "type, and it crashed on startup (eg trying to run a "
> - "raspberry pi kernel on a versatilepb QEMU machine)\n"
> - "(2) You didn't give QEMU a kernel or BIOS filename at all, 
> "
> - "and QEMU executed a ROM full of no-op instructions until "
> - "it fell off the end\n"
> - "(3) Your guest kernel has a bug and crashed by jumping "
> - "off into nowhere\n\n"
> - "This is almost always one of the first two, so check your "
> - "command line and that you are using the right type of 
> kernel "
> - "for this machine.\n"
> - "If you think option (3) is likely then you can try 
> debugging "
> - "your guest with the -d debug options; in particular "
> - "-d guest_errors will cause the log to include a dump of 
> the "
> - "guest register state at this point.\n\n"
> - "Execution cannot continue; stopping here.\n\n");
> -
> -/* Report also to the logs, with more detail including register dump */
> -qemu_log_mask(LOG_GUEST_ERROR, "qemu: fatal: Trying to execute code "
> -  "outside RAM or ROM at 0x" TARGET_FMT_lx "\n", addr);
> -log_cpu_state_mask(LOG_GUEST_ERROR, cpu, CPU_DUMP_FPU | CPU_DUMP_CCOP);
> -}
> -
>  static inline ram_addr_t qemu_ram_addr_from_host_nofail(void *ptr)
>  {
>  ram_addr_t ram_addr;
> @@ -963,7 +930,6 @@ tb_page_addr_t get_page_addr_code(CPUArchState *env, 
> target_ulong addr)
>  MemoryRegionSection *section;
>  CPUState *cpu = ENV_GET_CPU(env);
>  CPUIOTLBEntry *iotlbentry;
> -hwaddr physaddr, mr_offset;
>  
>  index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
>  mmu_idx = cpu_mmu_index(env, true);
> @@ -977,65 +943,24 @@ tb_page_addr_t get_page_addr_code(CPUArchState *env, 
> target_ulong addr)
>(TLB_RECHECK | TLB_INVALID_MASK)) == TLB_RECHECK)) {
>  /*
>   * This is a TLB_RECHECK access, where the MMU protection
> - * covers a smaller range than a target page, and we must
> - * repeat the MMU check here. This tlb_fill() call might
> - * longjump out if this access should cause a guest exception.
> - */
> -int index;
> -target_ulong tlb_addr;
> -
> -tlb_fill(cpu, addr, 0, MMU_INST_FETCH, mmu_idx, 0);
> -
> -index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
> -tlb_addr = env->tlb_table[mmu_idx][index].addr_code;
> -if (!(tlb_addr & ~(TARGET_PAGE_MASK | TLB_RECHECK))) {
> -/* RAM access. We can't handle this, so for now just stop */
> -cpu_abort(cpu, "Unable to handle guest executing from RAM within 
> "
> -  "a small MPU region at 0x" TARGET_FMT_lx, addr);
> -}
> -/*
> - * Fall through to handle IO accesses (which will almost certainly
> - * also result in failure)
> + * covers a smaller range than a target page. Return -1 to
> + * indicate that we cannot simply execute from RAM here;
> + * we will perform the necessary repeat of the MMU check
> + * when the "execute a single insn" code performs the
> + * load of the guest insn.
>   */
> +return -1;
>  }
>  
>  iotlbentry = >iotlb[mmu_idx][index];
>  section = 

Re: [Qemu-devel] [PATCH] docker: Install more packages in centos7

[Fam Zheng]

On Wed, 07/11 14:58, Fam Zheng wrote:
> This makes test-block work.
> 
> Signed-off-by: Fam Zheng 

Queued, thanks.

Re: [Qemu-devel] [PATCH 1/7] tests: qgraph API for the qtest driver framework

[Stefan Hajnoczi]

On Mon, Jul 09, 2018 at 11:11:30AM +0200, Emanuele Giuseppe Esposito wrote:
> +/* Graph Edge.*/
> +struct QOSGraphEdge {
> +QOSEdgeType type;
> +char *dest;
> +char *arg; /* just for CONTAIS and CONSUMED_BY */

CONTAINS?

> +/**
> + * remove_node(): removes a node @val from the nodes hash table.
> + * Note that node->name is not free'd since it will represent the
> + * hash table key
> + */
> +static void remove_node(void *val)

This is a confusing function name and doc comment.  It does not remove
the node from a hash table, it just frees it.

Please call this destroy_node().  The g_hash_table_new_full() argument
is GDestroyNotify value_destroy_func, so that would be consistent with
glib naming too.

> +{
> +QOSGraphNode *node = (QOSGraphNode *) val;

There is no need to cast void pointers in C.  Just "QOSGraphNode *node =
val;" will do.

> +/**
> + * build_driver_cmd_line(): builds the command line for the driver
> + * @node. The node name must be a valid qemu identifier, since it
> + * will be used to build the command line.
> + *
> + * It is also possible to pass an optional @args that will be
> + * concatenated to the command line.
> + *
> + * For drivers, prepend -device to the driver name.
> + */
> +static void build_driver_cmd_line(QOSGraphNode *node, const char *args)

Why is this called "driver" instead of "device"?

> +{
> +if (args) {
> +node->command_line = g_strconcat("-device ", node->name, ",",
> +  args, NULL);
> +} else {
> +node->command_line = g_strconcat("-device ", node->name, NULL);
> +}
> +}
> +
> +/**
> + * build_test_cmd_line(): builds the command line for the test
> + * @node. The node name need not to be a valid qemu identifier, since it
> + * will not be used to build the command line.
> + *
> + * It is also possible to pass an optional @args that will be
> + * used as additional command line.
> + */
> +static void build_test_cmd_line(QOSGraphNode *node, const char *args)
> +{
> +if (args) {
> +node->command_line = g_strdup(args);
> +} else {
> +node->command_line = NULL;
> +}

This is equivalent to:

  node->command_line = g_strdup(args);

https://developer.gnome.org/glib/stable/glib-String-Utility-Functions.html#g-strdup

> diff --git a/tests/libqos/qgraph.h b/tests/libqos/qgraph.h
> new file mode 100644
> index 00..54a1786c1e
> --- /dev/null
> +++ b/tests/libqos/qgraph.h
> @@ -0,0 +1,259 @@
> +/*
> + * libqos driver framework

The per-function doc comment are good.  Please also include higher-level
examples of how to use this.  At first glance all these functions are
overwhelming and it's not clear how to implement new tests, drivers,
interfaces, or machines.

See include/qom/object.h for inspiration.

> +/* edge types*/
> +enum QOSEdgeType {
> +CONTAINS,
> +PRODUCES,
> +CONSUMED_BY
> +};
> +
> +/* node types*/
> +enum QOSNodeType {
> +MACHINE,
> +DRIVER,
> +INTERFACE,
> +TEST
> +};

The QOSEdgeType and QOSNodeType enum constants use commonly-used names
in the global namespace.  Please prefix them since this is a header
file that will be included from many source files.

> +
> +/**
> + * Each driver, test or machine will have this as first field.
> + * Depending on the edge, the node will call the corresponding
> + * function when walking the path.
> + *
> + * QOSGraphObject also provides a destructor, used to deallocate the
> + * after the test has been executed.
> + */
> +struct QOSGraphObject {
> +/* for produces, returns void * */
> +QOSGetDriver get_driver;

Unused?

> +/* for contains, returns a QOSGraphObject * */
> +QOSGetDevice get_device;

Unused?

> diff --git a/tests/libqos/qgraph_extra.h b/tests/libqos/qgraph_extra.h
> new file mode 100644
> index 00..22850c0368
> --- /dev/null
> +++ b/tests/libqos/qgraph_extra.h
> @@ -0,0 +1,155 @@
> +/*
> + * libqos driver framework
> + *
> + * Copyright (c) 2018 Emanuele Giuseppe Esposito 
> 
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License version 2 as published by the Free Software Foundation.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see 
> 
> + */
> +
> +#ifndef QGRAPH_EXTRA_H
> +#define QGRAPH_EXTRA_H
> +
> +#include "qgraph.h"
> +#define PRINT_DEBUG 0

I would expect the #ifdef to be in the C file that uses it.  PRINT_DEBUG
is too generic a name for a .h file, it may cause namespace collisions.


signature.asc
Description: PGP signature

[Qemu-devel] [PATCH v2 4/4] tests: Add centos VM testing

[Fam Zheng]

This one does docker testing in the VM. It is intended to replace the
native docker testing on patchew testers.

Signed-off-by: Fam Zheng 
---
 tests/vm/Makefile.include |  2 +-
 tests/vm/centos   | 84 +++
 2 files changed, 85 insertions(+), 1 deletion(-)
 create mode 100755 tests/vm/centos

diff --git a/tests/vm/Makefile.include b/tests/vm/Makefile.include
index 5daa2a3b73..e492cd73e5 100644
--- a/tests/vm/Makefile.include
+++ b/tests/vm/Makefile.include
@@ -2,7 +2,7 @@
 
 .PHONY: vm-build-all
 
-IMAGES := ubuntu.i386 freebsd netbsd openbsd
+IMAGES := ubuntu.i386 freebsd netbsd openbsd centos
 IMAGE_FILES := $(patsubst %, tests/vm/%.img, $(IMAGES))
 
 .PRECIOUS: $(IMAGE_FILES)
diff --git a/tests/vm/centos b/tests/vm/centos
new file mode 100755
index 00..afd560c564
--- /dev/null
+++ b/tests/vm/centos
@@ -0,0 +1,84 @@
+#!/usr/bin/env python
+#
+# CentOS image
+#
+# Copyright 2018 Red Hat Inc.
+#
+# Authors:
+#  Fam Zheng 
+#
+# This code is licensed under the GPL version 2 or later.  See
+# the COPYING file in the top-level directory.
+#
+
+import os
+import sys
+import subprocess
+import basevm
+import time
+
+class CentosVM(basevm.BaseVM):
+name = "centos"
+BUILD_SCRIPT = """
+set -e;
+cd $(mktemp -d);
+export SRC_ARCHIVE=/dev/vdb;
+sudo chmod a+r $SRC_ARCHIVE;
+tar -xf $SRC_ARCHIVE;
+make docker-test-block@centos7 V={verbose} J={jobs};
+make docker-test-quick@centos7 V={verbose} J={jobs};
+make docker-test-mingw@fedora V={verbose} J={jobs};
+"""
+
+def _gen_cloud_init_iso(self):
+cidir = self._tmpdir
+mdata = open(os.path.join(cidir, "meta-data"), "w")
+mdata.writelines(["instance-id: centos-vm-0\n",
+  "local-hostname: centos-guest\n"])
+mdata.close()
+udata = open(os.path.join(cidir, "user-data"), "w")
+udata.writelines(["#cloud-config\n",
+  "chpasswd:\n",
+  "  list: |\n",
+  "root:%s\n" % self.ROOT_PASS,
+  "%s:%s\n" % (self.GUEST_USER, self.GUEST_PASS),
+  "  expire: False\n",
+  "users:\n",
+  "  - name: %s\n" % self.GUEST_USER,
+  "sudo: ALL=(ALL) NOPASSWD:ALL\n",
+  "ssh-authorized-keys:\n",
+  "- %s\n" % basevm.SSH_PUB_KEY,
+  "  - name: root\n",
+  "ssh-authorized-keys:\n",
+  "- %s\n" % basevm.SSH_PUB_KEY,
+  "locale: en_US.UTF-8\n"])
+udata.close()
+subprocess.check_call(["genisoimage", "-output", "cloud-init.iso",
+   "-volid", "cidata", "-joliet", "-rock",
+   "user-data", "meta-data"],
+   cwd=cidir,
+   stdin=self._devnull, stdout=self._stdout,
+   stderr=self._stdout)
+return os.path.join(cidir, "cloud-init.iso")
+
+def build_image(self, img):
+cimg = 
self._download_with_cache("https://cloud.centos.org/centos/7/images/CentOS-7-x86_64-GenericCloud-1802.qcow2.xz;)
+img_tmp = img + ".tmp"
+subprocess.check_call(["cp", "-f", cimg, img_tmp + ".xz"])
+subprocess.check_call(["xz", "-df", img_tmp + ".xz"])
+subprocess.check_call(["qemu-img", "resize", img_tmp, "50G"])
+self.boot(img_tmp, extra_args = ["-cdrom", self._gen_cloud_init_iso()])
+self.wait_ssh()
+self.ssh_root_check("touch /etc/cloud/cloud-init.disabled")
+self.ssh_root_check("yum update -y")
+self.ssh_root_check("yum install -y docker make git")
+self.ssh_root_check("systemctl enable docker")
+self.ssh_root("poweroff")
+self.wait()
+if os.path.exists(img):
+os.remove(img)
+os.rename(img_tmp, img)
+return 0
+
+if __name__ == "__main__":
+sys.exit(basevm.main(CentosVM))
-- 
2.17.1

[Qemu-devel] [PATCH v2 2/4] tests/vm: Pass verbose flag into VM make commands

[Fam Zheng]

Our Makefile has:

vm-build-%: tests/vm/%.img
$(call quiet-command, \
$(SRC_PATH)/tests/vm/$* \
$(if $(V)$(DEBUG), --debug) \
$(if $(DEBUG), --interactive) \

the intention of which is to let the make command in VM have V=1 if
V=1 is set. We pass this to the BUILD_SCRIPT format.

Signed-off-by: Fam Zheng 
---
 tests/vm/basevm.py   | 3 ++-
 tests/vm/freebsd | 4 ++--
 tests/vm/netbsd  | 4 ++--
 tests/vm/openbsd | 4 ++--
 tests/vm/ubuntu.i386 | 4 ++--
 5 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py
index e5d6a328d5..25361c6b7d 100755
--- a/tests/vm/basevm.py
+++ b/tests/vm/basevm.py
@@ -240,7 +240,8 @@ def main(vmcls):
 vm.add_source_dir(args.build_qemu)
 cmd = [vm.BUILD_SCRIPT.format(
configure_opts = " ".join(argv),
-   jobs=args.jobs)]
+   jobs=args.jobs,
+   verbose="1" if args.debug else "")]
 else:
 cmd = argv
 img = args.image
diff --git a/tests/vm/freebsd b/tests/vm/freebsd
index 039dad8f69..b20612a6c9 100755
--- a/tests/vm/freebsd
+++ b/tests/vm/freebsd
@@ -23,8 +23,8 @@ class FreeBSDVM(basevm.BaseVM):
 cd $(mktemp -d /var/tmp/qemu-test.XX);
 tar -xf /dev/vtbd1;
 ./configure {configure_opts};
-gmake -j{jobs};
-gmake check;
+gmake -j{jobs} V={verbose};
+gmake check V={verbose};
 """
 
 def build_image(self, img):
diff --git a/tests/vm/netbsd b/tests/vm/netbsd
index 3972d8b45c..3f9d34a208 100755
--- a/tests/vm/netbsd
+++ b/tests/vm/netbsd
@@ -23,8 +23,8 @@ class NetBSDVM(basevm.BaseVM):
 cd $(mktemp -d /var/tmp/qemu-test.XX);
 tar -xf /dev/rld1a;
 ./configure --python=python2.7 {configure_opts};
-gmake -j{jobs};
-gmake check;
+gmake -j{jobs} V={verbose};
+gmake check V={verbose};
 """
 
 def build_image(self, img):
diff --git a/tests/vm/openbsd b/tests/vm/openbsd
index 6ae16d97fd..3285c1abde 100755
--- a/tests/vm/openbsd
+++ b/tests/vm/openbsd
@@ -23,9 +23,9 @@ class OpenBSDVM(basevm.BaseVM):
 cd $(mktemp -d /var/tmp/qemu-test.XX);
 tar -xf /dev/rsd1c;
 ./configure --cc=x86_64-unknown-openbsd6.1-gcc-4.9.4 
--python=python2.7 {configure_opts};
-gmake -j{jobs};
+gmake -j{jobs} V={verbose};
 # XXX: "gmake check" seems to always hang or fail
-#gmake check;
+#gmake check V={verbose};
 """
 
 def build_image(self, img):
diff --git a/tests/vm/ubuntu.i386 b/tests/vm/ubuntu.i386
index fc319e0e6e..b4eaa0dedc 100755
--- a/tests/vm/ubuntu.i386
+++ b/tests/vm/ubuntu.i386
@@ -25,8 +25,8 @@ class UbuntuX86VM(basevm.BaseVM):
 sudo chmod a+r /dev/vdb;
 tar -xf /dev/vdb;
 ./configure {configure_opts};
-make -j{jobs};
-make check;
+make -j{jobs} V={verbose};
+make check V={verbose};
 """
 
 def _gen_cloud_init_iso(self):
-- 
2.17.1

[Qemu-devel] [PATCH v2 3/4] tests: Allow overriding archive path with SRC_ARCHIVE

[Fam Zheng]

In VM based tests, the source archive is created in host, we don't have
to run archive-source.sh again, as it complicates the Makefile and
scripts.

Signed-off-by: Fam Zheng 
---
 tests/docker/Makefile.include | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
index b2a7e761cc..f0525b91a7 100644
--- a/tests/docker/Makefile.include
+++ b/tests/docker/Makefile.include
@@ -27,8 +27,11 @@ DOCKER_SRC_COPY := $(BUILD_DIR)/docker-src.$(CUR_TIME)
 
 $(DOCKER_SRC_COPY):
@mkdir $@
-   $(call quiet-command, cd $(SRC_PATH) && scripts/archive-source.sh 
$@/qemu.tar, \
-   "GEN", "$@/qemu.tar")
+   $(if $(SRC_ARCHIVE), \
+   $(call quiet-command, cp "$(SRC_ARCHIVE)" $@/qemu.tar, \
+   "CP", "$@/qemu.tar"), \
+   $(call quiet-command, cd $(SRC_PATH) && 
scripts/archive-source.sh $@/qemu.tar, \
+   "GEN", "$@/qemu.tar"))
$(call quiet-command, cp $(SRC_PATH)/tests/docker/run $@/run, \
"COPY","RUNNER")
 
-- 
2.17.1

[Qemu-devel] [PATCH v2 0/4] Add a CentOS test image to run docker tests

[Fam Zheng]

v2: Drop archive-source.sh changes.
The new test depends on the iotests nbd fix I posted today to pass.

Docker testing on patchew has long suffered from 'make check' hangings. The
cleanness of VM testing is the cure. Now let's add a CentOS 7 image to run the
tests.  It's purely ad-hoc, but hopefully still easy to understand and use for
everyone.

The first patch makes passing source code from host to the container in VM
working, and is a nice clean up.

The second patch makes caches work, to speed up repetitive runs like on
patchew.

The last patch adds the new image that does the job. Two out of three docker
tests running on patchew.org are added to the image. I'll wait for Peter to fix
the 'docker-test-quick@centos6' hanging (oob test) before adding it too.

Fam

Fam Zheng (4):
  tests: Add an option for snapshot (default: off)
  tests/vm: Pass verbose flag into VM make commands
  tests: Allow overriding archive path with SRC_ARCHIVE
  tests: Add centos VM testing

 tests/docker/Makefile.include |  7 ++-
 tests/vm/Makefile.include |  2 +-
 tests/vm/basevm.py| 10 -
 tests/vm/centos   | 84 +++
 tests/vm/freebsd  |  4 +-
 tests/vm/netbsd   |  4 +-
 tests/vm/openbsd  |  4 +-
 tests/vm/ubuntu.i386  |  4 +-
 8 files changed, 106 insertions(+), 13 deletions(-)
 create mode 100755 tests/vm/centos

-- 
2.17.1

[Qemu-devel] [PATCH v2 1/4] tests: Add an option for snapshot (default: off)

[Fam Zheng]

Not using snapshot has the benefit of automatically persisting useful
test harnesses, such as docker images and ccache database. Although it
will lose some cleanness, it is imaginably useful for patchew.

Signed-off-by: Fam Zheng 
---
 tests/vm/basevm.py | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py
index 3643117816..e5d6a328d5 100755
--- a/tests/vm/basevm.py
+++ b/tests/vm/basevm.py
@@ -216,6 +216,8 @@ def parse_args(vm_name):
   help="build QEMU from source in guest")
 parser.add_option("--interactive", "-I", action="store_true",
   help="Interactively run command")
+parser.add_option("--snapshot", "-s", action="store_true",
+  help="run tests with a snapshot")
 parser.disable_interspersed_args()
 return parser.parse_args()
 
@@ -241,7 +243,10 @@ def main(vmcls):
jobs=args.jobs)]
 else:
 cmd = argv
-vm.boot(args.image + ",snapshot=on")
+img = args.image
+if args.snapshot:
+img += ",snapshot=on"
+vm.boot(img)
 vm.wait_ssh()
 except Exception as e:
 if isinstance(e, SystemExit) and e.code == 0:
-- 
2.17.1

Re: [Qemu-devel] [PATCH 0/7] Qtest driver framework

[Emanuele]

On 07/11/2018 04:00 PM, Stefan Hajnoczi wrote:


On Mon, Jul 09, 2018 at 11:11:29AM +0200, Emanuele Giuseppe Esposito wrote:

Basic framework steps are the following:
- All nodes and edges are created in their respective machine/driver/test files
- The framework starts QEMU and asks for a list of available drivers
   and machines

QEMU doesn't know about qtest drivers.  Does "drivers" mean "devices"
(i.e. qemu -device \?) and they have a 1:1 correspondence with qtest
drivers?

Yes, they are QEMU devices, it was a typo.
Not all qtest drivers are mapped with the QEMU devices, though. In fact, 
for now all drivers "contained" or "produced" by other nodes are not 
QEMU devices.


The others (machines and nodes that "consume"), are mapped to QEMU using 
the

{ "execute": "qom-list-types", "arguments" : { "implements" : "device" } }
for "consume" and
{ "execute": "query-machines" }
for machines.

With "consume" I mean in situation like "node X" ---> consumes --> "node 
Y",

"X" should be in the output of qom-list-types query.

The concept of qgraph makes sense.  Ease of use and documentation will
be critical to gain adoption.  The extra complexity makes it hard for
people writing and running tests, so it's important that it's clear
which tests will get run and why (or why not).

I am trying to document and makes it as easy and clean as possible :)

Re: [Qemu-devel] [PATCH 1/2] qdev: add HotplugHandler->post_plug() callback

[Igor Mammedov]

On Wed, 11 Jul 2018 15:32:12 +0200
Paolo Bonzini  wrote:

> On 11/07/2018 15:29, Stefan Hajnoczi wrote:
> >>  if (dev->hotplugged) {
> >>  device_reset(dev);
> >> +
> >> +if (hotplug_ctrl) {  
> > In the final patch I will move this out of if (dev->hotplugged) since
> > the other HotplugHandler callbacks are also invoked unconditionally.  
> 
> I'm not even sure why the reset is needed (removing it would also fix
> the bug!), and why it's only done on hotplug; however, it is probably
> there because it updates some bus-level state, so it's dangerous to
> remove it.
it might be also so that each device won't have to call reset manually from
their realize (5ab28c834) to initialize device into initial state.

> Paolo
> 
> >> +hotplug_handler_post_plug(hotplug_ctrl, dev, _err);
> >> +if (local_err != NULL) {
> >> +goto child_realize_fail;
> >> +}
> >> +}
> >>  }
> >>  dev->pending_deleted_event = false;  
> 

Re: [Qemu-devel] [PULL v2 18/32] qmp: Don't let JSON errors jump the queue

[Markus Armbruster]

Kevin Wolf  writes:

> Am 10.07.2018 um 16:02 hat Marc-André Lureau geschrieben:
>> Hi
>> 
>> On Tue, Jul 10, 2018 at 3:20 PM, Kevin Wolf  wrote:
>> > Am 03.07.2018 um 23:35 hat Markus Armbruster geschrieben:
>> >> handle_qmp_command() reports JSON syntax errors right away.  This is
>> >> wrong when OOB is enabled, because the errors can "jump the queue"
>> >> then.
>> >>
>> >> The previous commit fixed the same bug for semantic errors, by
>> >> delaying the checking until dispatch.  We can't delay the checking, so
>> >> delay the reporting.
>> >>
>> >> Signed-off-by: Markus Armbruster 
>> >> Reviewed-by: Eric Blake 
>> >> Message-Id: <20180703085358.13941-19-arm...@redhat.com>
>> >
>> > I'm observing a qemu crash in qemu-iotests 153 (which does however not
>> > seem to make the test case fail). git bisect points me to this patch.
>> >
>> > I'm getting output like this:
>> >
>> > *** Error in `/home/kwolf/source/qemu/tests/qemu-iotests/qemu': free(): 
>> > invalid pointer: 0x555f7870f7e0 ***
>> > === Backtrace: =
>> > /lib64/libc.so.6(+0x7cbac)[0x7fa9b29a2bac]
>> > /lib64/libc.so.6(+0x87a59)[0x7fa9b29ada59]
>> > /lib64/libc.so.6(cfree+0x16e)[0x7fa9b29b33be]
>> > /lib64/libglib-2.0.so.0(g_free+0xe)[0x7fa9ce462b4e]
>> > /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x6eb9dc)[0x555f76f489dc]
>> > /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x30ae4b)[0x555f76b67e4b]
>> > /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x311558)[0x555f76b6e558]
>> > /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x6e2d4e)[0x555f76f3fd4e]
>> > /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x6e5fa0)[0x555f76f42fa0]
>> > /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x6e2c2e)[0x555f76f3fc2e]
>> > /lib64/libglib-2.0.so.0(g_main_context_dispatch+0x157)[0x7fa9ce45d257]
>> > /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x6e526e)[0x555f76f4226e]
>> > /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x42349e)[0x555f76c8049e]
>> > /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x2c27ef)[0x555f76b1f7ef]
>> > /lib64/libc.so.6(__libc_start_main+0xea)[0x7fa9b294688a]
>> > /home/kwolf/source/qemu/tests/qemu-iotests/qemu(+0x2c5b8a)[0x555f76b22b8a]
>> >
>> > Interestingly, this doesn't want to produce a core dump for me, so no
>> > backtrace with usable function names here. But I assume that you can
>> > easily reproduce this yourself.
>> >
>> 
>> Looks like the double-free regression, you could try: "[PATCH]
>> monitor: fix double-free of request error"
>
> Thanks, that does fix it. Looks like it missed -rc0, though?

Yes.  I'll work on a pull request for -rc1.

Re: [Qemu-devel] [PATCH v2 2/2] memory: fix possible NULL pointer dereference

[Peter Maydell]

On 11 July 2018 at 14:47, Philippe Mathieu-Daudé  wrote:
> Hi Dima,
>
> On 07/11/2018 05:34 AM, Dima Stepanov wrote:
>> Gentle ping. CCing Paolo Bonzini.
>>
>> Regards, Dima.
>>
>> On Tue, Jun 19, 2018 at 05:12:16PM +0300, Dima Stepanov wrote:
>>> Ping.
>>>
>>> Regards, Dima.
>>>
>>> On Wed, Jun 13, 2018 at 11:19:55AM +0300, Dima Stepanov wrote:
 In the memory_region_do_invalidate_mmio_ptr() routine the section
 variable is intialized by the memory_region_find() call. The section.mr
 field can be set to NULL.

 Add the check for NULL before trying to drop a section.

 Signed-off-by: Dima Stepanov 
 ---
  memory.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

 diff --git a/memory.c b/memory.c
 index 3212acc..bb45248 100644
 --- a/memory.c
 +++ b/memory.c
 @@ -2712,7 +2712,7 @@ static void 
 memory_region_do_invalidate_mmio_ptr(CPUState *cpu,
  /* Reset dirty so this doesn't happen later. */
  cpu_physical_memory_test_and_clear_dirty(offset, size, 1);

 -if (section.mr != mr) {
 +if (section.mr && (section.mr != mr)) {
>
> section.mr can't be NULL here.
>
> You can give the static analyzer a hint using "assert(section.mr);"

Not in my view much point in messing with this code, though:
(a) it's broken and unusable as it stands (race conditions)
(b) it's obsoleted by the execute-from-mmio patchset
http://patchwork.ozlabs.org/cover/942090/ and if/when that
goes in it will all just get deleted.

thanks
-- PMM

Re: [Qemu-devel] [Qemu-arm] [PATCH 1/6] accel/tcg: Pass read access type through to io_readx()

[Philippe Mathieu-Daudé]

On 07/10/2018 01:00 PM, Peter Maydell wrote:
> The io_readx() function needs to know whether the load it is
> doing is an MMU_DATA_LOAD or an MMU_INST_FETCH, so that it
> can pass the right value to the cpu_transaction_failed()
> function. Plumb this information through from the softmmu
> code.
> 
> This is currently not often going to give the wrong answer,
> because usually instruction fetches go via get_page_addr_code().
> However once we switch over to handling execution from non-RAM by
> creating single-insn TBs, the path for an insn fetch to generate
> a bus error will be through cpu_ld*_code() and io_readx(),
> so without this change we will generate a d-side fault when we
> should generate an i-side fault.
> 
> We also have to pass the access type via a CPU struct global
> down to unassigned_mem_read(), for the benefit of the targets
> which still use the cpu_unassigned_access() hook (m68k, mips,
> sparc, xtensa).
> 
> Signed-off-by: Peter Maydell 

Reviewed-by: Philippe Mathieu-Daudé 

> ---
>  accel/tcg/softmmu_template.h | 11 +++
>  include/qom/cpu.h|  6 ++
>  accel/tcg/cputlb.c   |  5 +++--
>  memory.c |  3 ++-
>  4 files changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/accel/tcg/softmmu_template.h b/accel/tcg/softmmu_template.h
> index badbf148803..f060a693d41 100644
> --- a/accel/tcg/softmmu_template.h
> +++ b/accel/tcg/softmmu_template.h
> @@ -99,11 +99,12 @@ static inline DATA_TYPE glue(io_read, 
> SUFFIX)(CPUArchState *env,
>size_t mmu_idx, size_t index,
>target_ulong addr,
>uintptr_t retaddr,
> -  bool recheck)
> +  bool recheck,
> +  MMUAccessType access_type)
>  {
>  CPUIOTLBEntry *iotlbentry = >iotlb[mmu_idx][index];
>  return io_readx(env, iotlbentry, mmu_idx, addr, retaddr, recheck,
> -DATA_SIZE);
> +access_type, DATA_SIZE);
>  }
>  #endif
>  
> @@ -140,7 +141,8 @@ WORD_TYPE helper_le_ld_name(CPUArchState *env, 
> target_ulong addr,
>  /* ??? Note that the io helpers always read data in the target
> byte ordering.  We should push the LE/BE request down into io.  */
>  res = glue(io_read, SUFFIX)(env, mmu_idx, index, addr, retaddr,
> -tlb_addr & TLB_RECHECK);
> +tlb_addr & TLB_RECHECK,
> +READ_ACCESS_TYPE);
>  res = TGT_LE(res);
>  return res;
>  }
> @@ -207,7 +209,8 @@ WORD_TYPE helper_be_ld_name(CPUArchState *env, 
> target_ulong addr,
>  /* ??? Note that the io helpers always read data in the target
> byte ordering.  We should push the LE/BE request down into io.  */
>  res = glue(io_read, SUFFIX)(env, mmu_idx, index, addr, retaddr,
> -tlb_addr & TLB_RECHECK);
> +tlb_addr & TLB_RECHECK,
> +READ_ACCESS_TYPE);
>  res = TGT_BE(res);
>  return res;
>  }
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index bd796579ee4..ecf6ed556a9 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -386,6 +386,12 @@ struct CPUState {
>   */
>  uintptr_t mem_io_pc;
>  vaddr mem_io_vaddr;
> +/*
> + * This is only needed for the legacy cpu_unassigned_access() hook;
> + * when all targets using it have been converted to use
> + * cpu_transaction_failed() instead it can be removed.
> + */
> +MMUAccessType mem_io_access_type;
>  
>  int kvm_fd;
>  struct KVMState *kvm_state;
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index 20c147d6554..c491703f15f 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -789,7 +789,7 @@ static inline ram_addr_t 
> qemu_ram_addr_from_host_nofail(void *ptr)
>  static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
>   int mmu_idx,
>   target_ulong addr, uintptr_t retaddr,
> - bool recheck, int size)
> + bool recheck, MMUAccessType access_type, int size)
>  {
>  CPUState *cpu = ENV_GET_CPU(env);
>  hwaddr mr_offset;
> @@ -831,6 +831,7 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry 
> *iotlbentry,
>  }
>  
>  cpu->mem_io_vaddr = addr;
> +cpu->mem_io_access_type = access_type;
>  
>  if (mr->global_locking && !qemu_mutex_iothread_locked()) {
>  qemu_mutex_lock_iothread();
> @@ -843,7 +844,7 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry 
> *iotlbentry,
>  section->offset_within_address_space -
>  

Re: [Qemu-devel] [PATCH] docker: Install more packages in centos7

[Philippe Mathieu-Daudé]

On 07/11/2018 03:58 AM, Fam Zheng wrote:
> This makes test-block work.
> 
> Signed-off-by: Fam Zheng 

Reviewed-by: Philippe Mathieu-Daudé 

> ---
>  tests/docker/dockerfiles/centos7.docker | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tests/docker/dockerfiles/centos7.docker 
> b/tests/docker/dockerfiles/centos7.docker
> index 575de29a0a..83462b7205 100644
> --- a/tests/docker/dockerfiles/centos7.docker
> +++ b/tests/docker/dockerfiles/centos7.docker
> @@ -3,6 +3,7 @@ RUN yum install -y epel-release centos-release-xen
>  RUN yum -y update
>  ENV PACKAGES \
>  bison \
> +bzip2 \
>  bzip2-devel \
>  ccache \
>  csnappy-devel \
> @@ -12,10 +13,12 @@ ENV PACKAGES \
>  gettext \
>  git \
>  glib2-devel \
> +libaio-devel \
>  libepoxy-devel \
>  libfdt-devel \
>  librdmacm-devel \
>  lzo-devel \
> +nettle-devel \
>  make \
>  mesa-libEGL-devel \
>  mesa-libgbm-devel \
> 

Re: [Qemu-devel] [PATCH 0/7] Qtest driver framework

[Stefan Hajnoczi]

On Mon, Jul 09, 2018 at 11:11:29AM +0200, Emanuele Giuseppe Esposito wrote:
> Basic framework steps are the following:
> - All nodes and edges are created in their respective machine/driver/test 
> files
> - The framework starts QEMU and asks for a list of available drivers
>   and machines

QEMU doesn't know about qtest drivers.  Does "drivers" mean "devices"
(i.e. qemu -device \?) and they have a 1:1 correspondence with qtest
drivers?

The concept of qgraph makes sense.  Ease of use and documentation will
be critical to gain adoption.  The extra complexity makes it hard for
people writing and running tests, so it's important that it's clear
which tests will get run and why (or why not).


signature.asc
Description: PGP signature

[Qemu-devel] [Bug 1781211] [NEW] HAXM acceleration does not work at all.

[Dmitriy via Qemu-devel]

Public bug reported:

I have qemu windows build 2.12.90, haxm 7.2.0. Ubuntu, nor arch linux does not 
works when i turn on hax acceleration. Permanent kernel panics, black screen 
freezing and other crashes happens when i run qemu.
Qemu crashed with hax - when i ran it from iso. It crashed on already installed 
system - it's not matters. 

Versions:
archlinux-2018.07.01-x86_64
ubuntu-18.04-live-server-amd64.iso

I run qemu-system-x86_64.exe binary.

My CPU:
core i7 2600k

See screenshot

** Affects: qemu
 Importance: Undecided
 Status: New


** Tags: hax haxm windows

** Attachment added: "2018-07-11_15-49-15.png"
   
https://bugs.launchpad.net/bugs/1781211/+attachment/5162388/+files/2018-07-11_15-49-15.png

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1781211

Title:
  HAXM acceleration does not work at all.

Status in QEMU:
  New

Bug description:
  I have qemu windows build 2.12.90, haxm 7.2.0. Ubuntu, nor arch linux does 
not works when i turn on hax acceleration. Permanent kernel panics, black 
screen freezing and other crashes happens when i run qemu.
  Qemu crashed with hax - when i ran it from iso. It crashed on already 
installed system - it's not matters. 

  Versions:
  archlinux-2018.07.01-x86_64
  ubuntu-18.04-live-server-amd64.iso

  I run qemu-system-x86_64.exe binary.

  My CPU:
  core i7 2600k

  See screenshot

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1781211/+subscriptions

Re: [Qemu-devel] [PATCH] fix gdbserver_state pointer validation

[Philippe Mathieu-Daudé]

Hi Stephane,

On 07/11/2018 04:52 AM, stephane duverger wrote:
> To reach gdb_set_stop_cpu() with gdbserver_state == NULL, you previously
>> entered gdb_vm_state_change() with and use CPUState *cpu =
>> gdbserver_state->c_cpu = NULL deref, which shouldn't happen.
>> Also in gdb_set_stop_cpu() you finally call cpu_single_step(cpu=crap)
>> which then deref crap->singlestep_enabled.
>>
>> So I think this is not the correct fix.
>>
> 
> I think you are wrong. You can enter gdb_vm_state_change() only if it has
> been registered through qemu_add_vm_change_state_handler(). And this
> happens in gdbserver_start() which is called only when you start the
> gdbserver stub.
> 
> This is exactly what we don't want to do here: use qemu breakpoints and
> debug events forwarding without the need to enable a gdb stub.

Well, at least we agree the gdb stub code is not straightforward.

And apparently without seeing the bigger picture about how you are using
this, I am missing something.

> There might be an historical reason that vCPU debug events are always
> forwarded to the gdbserver (from cpu_handle_guest_debug()) but this
> should not be mandatory.
> 
> One could consider a check to the gdbserver state right before:
> 
> if (gdbserver_enabled())
> gdb_set_stop_cpu(cpu);
> 
> As found in other part of qemu code: kvm_enabled, hax_enabled, ...
> 
> 
>> Since this shouldn't happen, I'd add an assert(gdbserver_state) in
>> gdb_set_stop_cpu(), and clean gdb_vm_state_change()'s code flow to not
>> reach this.
>>
> 
> Correct if you previously add the gdbserver_enabled() check. Else this
> would abort the qemu instance each time a debug event is triggered
> and forwarded to a vm_change_state handler.
> 
  void gdb_set_stop_cpu(CPUState *cpu)
>  {
> +if (!gdbserver_state) {
> +return;
> +}
>  gdbserver_state->c_cpu = cpu;
>  gdbserver_state->g_cpu = cpu;
>>
>> I find it safer the opposite way, if (s) { s-> ... }
>>
> 
> Sincerely, this argument is subjective. If it's part of Qemu coding
> standard,
> i would agree. Again there is a lot of situations in the Qemu code where
> exit conditions are checked first and then lead to a return, preventing an
> unneeded level of indentation. Seriously, we are talking about a 2 lines
> function.

This is indeed a personal subjective suggestion, not part of QEMU Coding
standard. Rational is, if in the future someone needs to modify
gdb_set_stop_cpu(), it would be easier/safer for him.

No worries ;)

Regards,

Phil.



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [Bug 1719339] Re: serial8250: too much work for irq3

[Paul Gear]

** Tags added: canonical-is

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1719339

Title:
  serial8250: too much work for irq3

Status in QEMU:
  New

Bug description:
  It's know issue and sometimes mentioned since 2007. But it seems not
  fixed.

  http://lists.gnu.org/archive/html/qemu-devel/2008-02/msg00140.html
  https://bugzilla.redhat.com/show_bug.cgi?id=986761
  
http://old-list-archives.xenproject.org/archives/html/xen-devel/2009-02/msg00696.html

  I don't think fixes like increases PASS_LIMIT
  (/drivers/tty/serial/8250.c) or remove this annoying message
  (https://patchwork.kernel.org/patch/3920801/) is real fix. Some fix
  was proposed by H. Peter Anvin  https://lkml.org/lkml/2008/2/7/485.

  Can reproduce on Debian Strech host (Qemu 1:2.8+dfsg-6+deb9u2), Ubuntu
  16.04.2 LTS (Qemu 1:2.5+dfsg-5ubuntu10.15) also tried to use master
  branch (QEMU emulator version 2.10.50 (v2.10.0-766-ga43415ebfd-dirty))
  if we write a lot of message into console (dmesg or dd if=/dev/zero
  of=/dev/ttyS1).

  /usr/local/bin/qemu-system-x86_64 -name guest=ultra1,debug-threads=on
  -S -object
  secret,id=masterKey0,format=raw,file=/var/lib/libvirt/qemu/domain-27-ultra1
  /master-key.aes -machine pc-i440fx-2.8,accel=kvm,usb=off,dump-guest-
  core=off -cpu Skylake-
  
Client,ds=on,acpi=on,ss=on,ht=on,tm=on,pbe=on,dtes64=on,monitor=on,ds_cpl=on,vmx=on,smx=on,est=on,tm2=on,xtpr=on,pdcm=on,osxsave=on,tsc_adjust=on,clflushopt=on,pdpe1gb=on
  -m 4096 -realtime mlock=off -smp 4,sockets=1,cores=4,threads=1 -uuid
  4537ca29-73b2-40c3-9b43-666de182ba5f -display none -no-user-config
  -nodefaults -chardev
  
socket,id=charmonitor,path=/var/lib/libvirt/qemu/domain-27-ultra1/monitor.sock,server,nowait
  -mon chardev=charmonitor,id=monitor,mode=control -rtc
  base=utc,driftfix=slew -global kvm-pit.lost_tick_policy=delay -no-hpet
  -no-shutdown -global PIIX4_PM.disable_s3=1 -global
  PIIX4_PM.disable_s4=1 -boot strict=on -device ich9-usb-
  ehci1,id=usb,bus=pci.0,addr=0x8.0x7 -drive
  file=/home/dzagorui/csr/csr_disk.qcow2,format=qcow2,if=none,id=drive-
  ide0-0-0 -device ide-hd,bus=ide.0,unit=0,drive=drive-
  ide0-0-0,id=ide0-0-0,bootindex=1 -netdev tap,fd=26,id=hostnet0 -device
  e1000,netdev=hostnet0,id=net0,mac=52:54:00:a9:4c:86,bus=pci.0,addr=0x3
  -chardev
  socket,id=charserial0,host=127.0.0.1,port=4000,telnet,server,nowait
  -device isa-serial,chardev=charserial0,id=serial0 -chardev
  socket,id=charserial1,host=127.0.0.1,port=4001,telnet,server,nowait
  -device isa-serial,chardev=charserial1,id=serial1 -device virtio-
  balloon-pci,id=balloon0,bus=pci.0,addr=0x2 -msg timestamp=on

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1719339/+subscriptions

[Qemu-devel] [Bug 1719339] Re: serial8250: too much work for irq3

[Paul Gear]

I'm seeing this on AWS EC2 when there's (apparently) high logging volume
to the console, very similarly to
https://www.reddit.com/r/sysadmin/comments/6zuqad/mongodb_aws_ec2_serial8250_too_much_work_for_irq4/

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1719339

Title:
  serial8250: too much work for irq3

Status in QEMU:
  New

Bug description:
  It's know issue and sometimes mentioned since 2007. But it seems not
  fixed.

  http://lists.gnu.org/archive/html/qemu-devel/2008-02/msg00140.html
  https://bugzilla.redhat.com/show_bug.cgi?id=986761
  
http://old-list-archives.xenproject.org/archives/html/xen-devel/2009-02/msg00696.html

  I don't think fixes like increases PASS_LIMIT
  (/drivers/tty/serial/8250.c) or remove this annoying message
  (https://patchwork.kernel.org/patch/3920801/) is real fix. Some fix
  was proposed by H. Peter Anvin  https://lkml.org/lkml/2008/2/7/485.

  Can reproduce on Debian Strech host (Qemu 1:2.8+dfsg-6+deb9u2), Ubuntu
  16.04.2 LTS (Qemu 1:2.5+dfsg-5ubuntu10.15) also tried to use master
  branch (QEMU emulator version 2.10.50 (v2.10.0-766-ga43415ebfd-dirty))
  if we write a lot of message into console (dmesg or dd if=/dev/zero
  of=/dev/ttyS1).

  /usr/local/bin/qemu-system-x86_64 -name guest=ultra1,debug-threads=on
  -S -object
  secret,id=masterKey0,format=raw,file=/var/lib/libvirt/qemu/domain-27-ultra1
  /master-key.aes -machine pc-i440fx-2.8,accel=kvm,usb=off,dump-guest-
  core=off -cpu Skylake-
  
Client,ds=on,acpi=on,ss=on,ht=on,tm=on,pbe=on,dtes64=on,monitor=on,ds_cpl=on,vmx=on,smx=on,est=on,tm2=on,xtpr=on,pdcm=on,osxsave=on,tsc_adjust=on,clflushopt=on,pdpe1gb=on
  -m 4096 -realtime mlock=off -smp 4,sockets=1,cores=4,threads=1 -uuid
  4537ca29-73b2-40c3-9b43-666de182ba5f -display none -no-user-config
  -nodefaults -chardev
  
socket,id=charmonitor,path=/var/lib/libvirt/qemu/domain-27-ultra1/monitor.sock,server,nowait
  -mon chardev=charmonitor,id=monitor,mode=control -rtc
  base=utc,driftfix=slew -global kvm-pit.lost_tick_policy=delay -no-hpet
  -no-shutdown -global PIIX4_PM.disable_s3=1 -global
  PIIX4_PM.disable_s4=1 -boot strict=on -device ich9-usb-
  ehci1,id=usb,bus=pci.0,addr=0x8.0x7 -drive
  file=/home/dzagorui/csr/csr_disk.qcow2,format=qcow2,if=none,id=drive-
  ide0-0-0 -device ide-hd,bus=ide.0,unit=0,drive=drive-
  ide0-0-0,id=ide0-0-0,bootindex=1 -netdev tap,fd=26,id=hostnet0 -device
  e1000,netdev=hostnet0,id=net0,mac=52:54:00:a9:4c:86,bus=pci.0,addr=0x3
  -chardev
  socket,id=charserial0,host=127.0.0.1,port=4000,telnet,server,nowait
  -device isa-serial,chardev=charserial0,id=serial0 -chardev
  socket,id=charserial1,host=127.0.0.1,port=4001,telnet,server,nowait
  -device isa-serial,chardev=charserial1,id=serial1 -device virtio-
  balloon-pci,id=balloon0,bus=pci.0,addr=0x2 -msg timestamp=on

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1719339/+subscriptions