Re: [RFC PATCH 0/8] Convert avocado tests to normal Python unittests

[Thomas Huth]

On 16/07/2024 20.03, Paolo Bonzini wrote:



Il mar 16 lug 2024, 18:45 John Snow > ha scritto:


My only ask is that we keep the tests running in the custom venv
environment we set up at build time


Yes, they do, however pytest should also be added to pythondeps.toml if we 
go this way.


If we move to pytest, it's possible we can eliminate that funkiness,
which would be a win.


There is the pycotap dependency to produce TAP from pytest, but that's 
probably something small enough to be vendored.


The next version is only depending on pycotap now. I'm installing it in the 
venv there that we also install when running the old avocado tests. Not sure 
whether that's the best solution, though. Would it be OK to have it in 
python/wheels/ instead?


And also it depends on what 
the dependencies would be for the assets framework.

>

I'm also not so sure about recreating all of the framework that pulls vm
images on demand, that sounds like it'd be a lot of work, but maybe I'm
wrong about that.


Yep, that's the part that I am a bit more doubtful about.


As I'm mentioned elsewhere, the tests that really have a hard dependency on 
the Avocado framework are only the tests that use the cloud-init images via 
the LinuxTest class. That's currently onle these files:


- boot_linux.py
- hotplug_blk.py
- hotplug_cpu.py
- intel_iommu.py
- replay_linux.py
- smmu.py

I assume we could continue using avocado.utils for the cloud-init stuff 
there, and just run them via the meson test runner, too. I'll give it a try 
when I get some spare time.


 Thomas

Re: [PATCH 3/6] virtio-iommu: Free [host_]resv_ranges on unset_iommu_devices

[Eric Auger]

Hi Zhenzhong,

On 7/17/24 05:06, Duan, Zhenzhong wrote:
>
>> -Original Message-
>> From: Eric Auger 
>> Subject: [PATCH 3/6] virtio-iommu: Free [host_]resv_ranges on
>> unset_iommu_devices
>>
>> We are currently missing the deallocation of the [host_]resv_regions
>> in case of hot unplug. Also to make things more simple let's rule
>> out the case where multiple HostIOMMUDevices would be aliased and
>> attached to the same IOMMUDevice. This allows to remove the handling
>> of conflicting Host reserved regions. Anyway this is not properly
>> supported at guest kernel level. On hotunplug the reserved regions
>> are reset to the ones set by virtio-iommu property.
>>
>> Signed-off-by: Eric Auger 
>> ---
>> hw/virtio/virtio-iommu.c | 62 ++--
>> 1 file changed, 28 insertions(+), 34 deletions(-)
>>
>> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
>> index 2c54c0d976..2de41ab412 100644
>> --- a/hw/virtio/virtio-iommu.c
>> +++ b/hw/virtio/virtio-iommu.c
>> @@ -538,8 +538,6 @@ static int
>> virtio_iommu_set_host_iova_ranges(VirtIOIOMMU *s, PCIBus *bus,
>> {
>> IOMMUPciBus *sbus = g_hash_table_lookup(s->as_by_busptr, bus);
>> IOMMUDevice *sdev;
>> -GList *current_ranges;
>> -GList *l, *tmp, *new_ranges = NULL;
>> int ret = -EINVAL;
>>
>> if (!sbus) {
>> @@ -553,33 +551,10 @@ static int
>> virtio_iommu_set_host_iova_ranges(VirtIOIOMMU *s, PCIBus *bus,
>> return ret;
>> }
>>
>> -current_ranges = sdev->host_resv_ranges;
>> -
>> -/* check that each new resv region is included in an existing one */
>> if (sdev->host_resv_ranges) {
>> -range_inverse_array(iova_ranges,
>> -&new_ranges,
>> -0, UINT64_MAX);
>> -
>> -for (tmp = new_ranges; tmp; tmp = tmp->next) {
>> -Range *newr = (Range *)tmp->data;
>> -bool included = false;
>> -
>> -for (l = current_ranges; l; l = l->next) {
>> -Range * r = (Range *)l->data;
>> -
>> -if (range_contains_range(r, newr)) {
>> -included = true;
>> -break;
>> -}
>> -}
>> -if (!included) {
>> -goto error;
>> -}
>> -}
>> -/* all new reserved ranges are included in existing ones */
>> -ret = 0;
>> -goto out;
>> +error_setg(errp, "%s virtio-iommu does not support aliased BDF",
>> +   __func__);
>> +return ret;
>> }
>>
>> range_inverse_array(iova_ranges,
>> @@ -588,14 +563,31 @@ static int
>> virtio_iommu_set_host_iova_ranges(VirtIOIOMMU *s, PCIBus *bus,
>> rebuild_resv_regions(sdev);
>>
>> return 0;
>> -error:
>> -error_setg(errp, "%s Conflicting host reserved ranges set!",
>> -   __func__);
>> -out:
>> -g_list_free_full(new_ranges, g_free);
>> -return ret;
>> }
>>
>> +static void virtio_iommu_unset_host_iova_ranges(VirtIOIOMMU *s,
>> PCIBus *bus,
>> +int devfn)
>> +{
>> +IOMMUPciBus *sbus = g_hash_table_lookup(s->as_by_busptr, bus);
>> +IOMMUDevice *sdev;
>> +
>> +if (!sbus) {
>> +return;
>> +}
>> +
>> +sdev = sbus->pbdev[devfn];
>> +if (!sdev) {
>> +return;
>> +}
>> +
>> +g_list_free_full(g_steal_pointer(&sdev->host_resv_ranges), g_free);
>> +g_list_free_full(sdev->resv_regions, g_free);
>> +sdev->host_resv_ranges = NULL;
>> +sdev->resv_regions = NULL;
>> +add_prop_resv_regions(sdev);
> Is this necessary? rebuild_resv_regions() will do that again.
My goal was to reset the state that existed before the

virtio_iommu_set_host_iova_ranges() was called. prop resv regions were 
originally added in virtio_iommu_find_add_as. 
The next device to be hotplugged at this aliased bdf is not necessarily a VFIO 
device (may be a virtio one), in which case we would miss the prop resv regions.

>
> Other than that, for the whole series,
>
> Reviewed-by: Zhenzhong Duan 

Thanks!

Eric
>
> Thanks
> Zhenzhong
>
>> +}
>> +
>> +
>> static bool check_page_size_mask(VirtIOIOMMU *viommu, uint64_t
>> new_mask,
>>  Error **errp)
>> {
>> @@ -704,6 +696,8 @@ virtio_iommu_unset_iommu_device(PCIBus *bus,
>> void *opaque, int devfn)
>> if (!hiod) {
>> return;
>> }
>> +virtio_iommu_unset_host_iova_ranges(viommu, hiod->aliased_bus,
>> +hiod->aliased_devfn);
>>
>> g_hash_table_remove(viommu->host_iommu_devices, &key);
>> }
>> --
>> 2.41.0

Re: [RFC PATCH 0/8] Convert avocado tests to normal Python unittests

[Paolo Bonzini]

On Wed, Jul 17, 2024 at 9:32 AM Thomas Huth  wrote:
> > There is the pycotap dependency to produce TAP from pytest, but that's
> > probably something small enough to be vendored.
>
> The next version is only depending on pycotap now. I'm installing it in the
> venv there that we also install when running the old avocado tests. Not sure
> whether that's the best solution, though. Would it be OK to have it in
> python/wheels/ instead?

Yes, and you can probably move it to the same group as meson; it's
ridiculously small (5k) and it is indeed used _with_ meson. Then you
don't need any change in "configure".

Paolo

Re: [PATCH v2 6/9] qapi: convert "Example" sections without titles

[Markus Armbruster]

John Snow  writes:

> Use the no-option form of ".. qmp-example::" to convert any Examples
> that do not have any form of caption or explanation whatsoever. Note
> that in a few cases, example sections are split into two or more
> separate example blocks. This is only done stylistically to create a
> delineation between two or more logically independent examples.
>
> See commit-3: "docs/qapidoc: create qmp-example directive", for a
>   detailed explanation of this custom directive syntax.
>
> See commit+3: "qapi: remove "Example" doc section" for a detailed
>   explanation of why.
>
> Note: an empty "TODO" line was added to announce-self to keep the
> example from floating up into the body; this will be addressed more
> rigorously in the new qapidoc generator.
>
> Signed-off-by: John Snow 

[...]

> diff --git a/qapi/run-state.json b/qapi/run-state.json
> index 4d40c88876c..ab7116680b3 100644
> --- a/qapi/run-state.json
> +++ b/qapi/run-state.json

[...]

> @@ -469,7 +469,7 @@
>  #
>  # Since: 9.1
>  #
> -# Example:
> +# ..qmp-example::

Lacks a space, output is messed up.  Can fix in my tree.

>  #
>  # <- { "event": "GUEST_PVSHUTDOWN",
>  #  "timestamp": { "seconds": 1648245259, "microseconds": 893771 } }

[...]

R-by stands.

Re: [PATCH] Manpage: Update description of 'user=username' for '-run-with'

[Boqiao Fu]

thanks for your helping

Best,
Boqiao

On Tue, Jul 16, 2024 at 5:00 PM Paolo Bonzini  wrote:

> > Manpage: the description of '-runs' didn't show this parameter will use
> > setuid, so the customer might get confused when 'elevateprivileges=deny'
> is
> > used. Since '-runas' is going to be deprecated and replaced by this
> > parameter in the coming qemu9.1, add the message here.
>
> Queued, thanks.  I modified the patch a bit to explain how setgid and
> setgroups are used in addition to setuid:
>
> diff --git a/qemu-options.hx b/qemu-options.hx
> index ad6521ef5e7..694fa37f284 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -5024,8 +5024,11 @@ SRST
>  in combination with -runas.
>
>  ``user=username`` or ``user=uid:gid`` can be used to drop root
> privileges
> -by switching to the specified user (via username) or user and group
> -(via uid:gid) immediately before starting guest execution.
> +before starting guest execution. QEMU will use the ``setuid`` and
> ``setgid``
> +system calls to switch to the specified identity.  Note that the
> +``user=username`` syntax will also apply the full set of supplementary
> +groups for the user, whereas the ``user=uid:gid`` will use only the
> +``gid`` group.
>
> Paolo
>
>

Re: [PATCH v2 9/9] qapi: remove "Example" doc section

[Markus Armbruster]

John Snow  writes:

> Fully eliminate the "Example" sections in QAPI doc blocks now that they
> have all been converted to arbitrary rST syntax using the
> ".. qmp-example::" directive. Update tests to match.
>
> Migrating to the new syntax
> ---
>
> The old "Example:" or "Examples:" section syntax is now caught as an
> error, but "Example::" is stil permitted as explicit rST syntax for an
> un-lexed, generic preformatted text block.
>
> ('Example' is not special in this case, any sentence that ends with "::"
> will start an indented code block in rST.)
>
> Arbitrary rST for Examples is now possible, but it's strongly
> recommended that documentation authors use the ".. qmp-example::"
> directive for consistent visual formatting in rendered HTML docs. The
> ":title:" directive option may be used to add extra information into the
> title bar for the example. The ":annotated:" option can be used to write
> arbitrary rST instead, with nested "::" blocks applying QMP formatting
> where desired.
>
> Other choices available are ".. code-block:: QMP" which will not create
> an "Example:" box, or the short-form "::" code-block syntax which will
> not apply QMP highlighting when used outside of the qmp-example
> directive.
>
> Why?
> 
>
> This patch has several benefits:
>
> 1. Example sections can now be written more arbitrarily, mixing
>explanatory paragraphs and code blocks however desired.
>
> 2. Example sections can now use fully arbitrary rST.
>
> 3. All code blocks are now lexed and validated as QMP; increasing
>usability of the docs and ensuring validity of example snippets.
>
>(To some extent - This patch only gaurantees it lexes correctly, not
>that it's valid under the JSON or QMP grammars. It will catch most
>small mistakes, however.)
>
> 4. Each qmp-example can be titled or annotated independently without
>bypassing the QMP lexer/validator.
>
>(i.e. code blocks are now for *code* only, so we don't have to
>sacrifice exposition for having lexically valid examples.)
>
> NOTE: As with the "Notes" conversion (d461c279737), this patch (and the
>   three preceding) may change the rendering order for Examples in
>   the current generator. The forthcoming qapidoc rewrite will fix
>   this by always generating documentation in source order.
>
> Signed-off-by: John Snow 

Reviewed-by: Markus Armbruster 

RE: [PATCH 3/6] virtio-iommu: Free [host_]resv_ranges on unset_iommu_devices

[Duan, Zhenzhong]

>-Original Message-
>From: Eric Auger 
>Subject: Re: [PATCH 3/6] virtio-iommu: Free [host_]resv_ranges on
>unset_iommu_devices
>
>Hi Zhenzhong,
>
>On 7/17/24 05:06, Duan, Zhenzhong wrote:
>>
>>> -Original Message-
>>> From: Eric Auger 
>>> Subject: [PATCH 3/6] virtio-iommu: Free [host_]resv_ranges on
>>> unset_iommu_devices
>>>
>>> We are currently missing the deallocation of the [host_]resv_regions
>>> in case of hot unplug. Also to make things more simple let's rule
>>> out the case where multiple HostIOMMUDevices would be aliased and
>>> attached to the same IOMMUDevice. This allows to remove the handling
>>> of conflicting Host reserved regions. Anyway this is not properly
>>> supported at guest kernel level. On hotunplug the reserved regions
>>> are reset to the ones set by virtio-iommu property.
>>>
>>> Signed-off-by: Eric Auger 
>>> ---
>>> hw/virtio/virtio-iommu.c | 62 ++--
>>> 1 file changed, 28 insertions(+), 34 deletions(-)
>>>
>>> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
>>> index 2c54c0d976..2de41ab412 100644
>>> --- a/hw/virtio/virtio-iommu.c
>>> +++ b/hw/virtio/virtio-iommu.c
>>> @@ -538,8 +538,6 @@ static int
>>> virtio_iommu_set_host_iova_ranges(VirtIOIOMMU *s, PCIBus *bus,
>>> {
>>> IOMMUPciBus *sbus = g_hash_table_lookup(s->as_by_busptr, bus);
>>> IOMMUDevice *sdev;
>>> -GList *current_ranges;
>>> -GList *l, *tmp, *new_ranges = NULL;
>>> int ret = -EINVAL;
>>>
>>> if (!sbus) {
>>> @@ -553,33 +551,10 @@ static int
>>> virtio_iommu_set_host_iova_ranges(VirtIOIOMMU *s, PCIBus *bus,
>>> return ret;
>>> }
>>>
>>> -current_ranges = sdev->host_resv_ranges;
>>> -
>>> -/* check that each new resv region is included in an existing one */
>>> if (sdev->host_resv_ranges) {
>>> -range_inverse_array(iova_ranges,
>>> -&new_ranges,
>>> -0, UINT64_MAX);
>>> -
>>> -for (tmp = new_ranges; tmp; tmp = tmp->next) {
>>> -Range *newr = (Range *)tmp->data;
>>> -bool included = false;
>>> -
>>> -for (l = current_ranges; l; l = l->next) {
>>> -Range * r = (Range *)l->data;
>>> -
>>> -if (range_contains_range(r, newr)) {
>>> -included = true;
>>> -break;
>>> -}
>>> -}
>>> -if (!included) {
>>> -goto error;
>>> -}
>>> -}
>>> -/* all new reserved ranges are included in existing ones */
>>> -ret = 0;
>>> -goto out;
>>> +error_setg(errp, "%s virtio-iommu does not support aliased BDF",
>>> +   __func__);
>>> +return ret;
>>> }
>>>
>>> range_inverse_array(iova_ranges,
>>> @@ -588,14 +563,31 @@ static int
>>> virtio_iommu_set_host_iova_ranges(VirtIOIOMMU *s, PCIBus *bus,
>>> rebuild_resv_regions(sdev);
>>>
>>> return 0;
>>> -error:
>>> -error_setg(errp, "%s Conflicting host reserved ranges set!",
>>> -   __func__);
>>> -out:
>>> -g_list_free_full(new_ranges, g_free);
>>> -return ret;
>>> }
>>>
>>> +static void virtio_iommu_unset_host_iova_ranges(VirtIOIOMMU *s,
>>> PCIBus *bus,
>>> +int devfn)
>>> +{
>>> +IOMMUPciBus *sbus = g_hash_table_lookup(s->as_by_busptr, bus);
>>> +IOMMUDevice *sdev;
>>> +
>>> +if (!sbus) {
>>> +return;
>>> +}
>>> +
>>> +sdev = sbus->pbdev[devfn];
>>> +if (!sdev) {
>>> +return;
>>> +}
>>> +
>>> +g_list_free_full(g_steal_pointer(&sdev->host_resv_ranges), g_free);
>>> +g_list_free_full(sdev->resv_regions, g_free);
>>> +sdev->host_resv_ranges = NULL;
>>> +sdev->resv_regions = NULL;
>>> +add_prop_resv_regions(sdev);
>> Is this necessary? rebuild_resv_regions() will do that again.
>My goal was to reset the state that existed before the
>
>virtio_iommu_set_host_iova_ranges() was called. prop resv regions were
>originally added in virtio_iommu_find_add_as.
>The next device to be hotplugged at this aliased bdf is not necessarily a VFIO
>device (may be a virtio one), in which case we would miss the prop resv
>regions.

Yeah, you are right, we must have it.

Thanks
Zhenzhong

>
>>
>> Other than that, for the whole series,
>>
>> Reviewed-by: Zhenzhong Duan 
>
>Thanks!
>
>Eric
>>
>> Thanks
>> Zhenzhong
>>
>>> +}
>>> +
>>> +
>>> static bool check_page_size_mask(VirtIOIOMMU *viommu, uint64_t
>>> new_mask,
>>>  Error **errp)
>>> {
>>> @@ -704,6 +696,8 @@ virtio_iommu_unset_iommu_device(PCIBus
>*bus,
>>> void *opaque, int devfn)
>>> if (!hiod) {
>>> return;
>>> }
>>> +virtio_iommu_unset_host_iova_ranges(viommu, hiod->aliased_bus,
>>> +hiod->aliased_devfn);
>>>
>>> g_hash_table_remove(viommu->host_iommu_devices, &key);
>>> }
>>> --
>>> 2.41.0

Re: [PATCH v4] mc146818rtc: add a way to generate RTC interrupts via QMP

[Daniil Tatianin]

On 5/29/24 6:27 PM, Philippe Mathieu-Daudé wrote:


On 29/5/24 16:34, Markus Armbruster wrote:

Daniil Tatianin  writes:


On 5/29/24 4:39 PM, Philippe Mathieu-Daudé wrote:


On 29/5/24 14:43, Daniil Tatianin wrote:

On 5/29/24 3:36 PM, Philippe Mathieu-Daudé wrote:


On 29/5/24 14:03, Markus Armbruster wrote:

Daniil Tatianin  writes:

This can be used to force-synchronize the time in guest after a 
long

stop-cont pause, which can be useful for serverless-type workload.

Also add a comment to highlight the fact that this (and one 
other QMP

command) only works for the MC146818 RTC controller.

Acked-by: Philippe Mathieu-Daudé 
Signed-off-by: Daniil Tatianin 
---

Changes since v0:
- Rename to rtc-inject-irq to match other similar API
- Add a comment to highlight that this only works for the I386 RTC

Changes since v1:
- Added a description below the QMP command to explain how it 
can be

    used and what it does.

Changes since v2:
- Add a 'broadcast' suffix.
- Change the comments to explain the flags we're setting.
- Change the command description to fix styling & explain that 
it's a broadcast command.


Changes since v3:
- Fix checkpatch complaints about usage of C99 comments

---
   hw/rtc/mc146818rtc.c | 20 
   include/hw/rtc/mc146818rtc.h |  1 +
   qapi/misc-target.json    | 19 +++
   3 files changed, 40 insertions(+)




diff --git a/qapi/misc-target.json b/qapi/misc-target.json
index 4e0a6492a9..7d388a3753 100644
--- a/qapi/misc-target.json
+++ b/qapi/misc-target.json
@@ -19,6 +19,25 @@
   { 'command': 'rtc-reset-reinjection',
 'if': 'TARGET_I386' }
   +##
+# @rtc-inject-irq-broadcast:
+#
+# Inject an RTC interrupt for all existing RTCs on the system.
+# The interrupt forces the guest to synchronize the time with 
RTC.
+# This is useful after a long stop-cont pause, which is common 
for

+# serverless-type workload.


In previous version you said:

   > This isn't really related to migration though. Serverless is 
based

   > on constantly stopping and resuming the VM on e.g. every HTTP
   > request to an endpoint.

Which made some sense. Maybe mention HTTP? And point to that use 
case

(possibly with QMP commands) in the commit description?


Hmm, maybe it would be helpful for people who don't know what 
serverless means.


How about:
  This is useful after a long stop-const pause, which is 
common for serverless-type workloads,
  e.g. stopping/resuming the VM on every HTTP request to an 
endpoint, which might involve
  a long pause in between the requests, causing time drift in 
the guest.


Please help me understand your workflow. Your management layer call
@stop and @cont QMP commands, is that right?


Yes, that is correct.


@cont will emit a @RESUME event.

If we could listen to QAPI events from C code, we could have the
mc146818rtc device automatically sync on VM resume, and no need for
this async command.


Perhaps? I'm not sure how that would be implemented, but let's see 
what Markus has to say.


You can't listen on an event in QEMU itself.  You can only hook into the
place that generates the event.


Apparently "qemu/notify.h" could be use for QAPI events (currently
only used by migration). Big change, to be discussed later.


The RESUME event is sent from vm_prepare_start() in system/cpus.c.


Good spot, it is where we call synchronize_pre_resume() for vCPUs,
which is exactly what Daniil wants for RTC devices.

I'd rather we call here rtc_synchronize_pre_resume(), which would
mostly be qmp_rtc_inject_irq_broadcast() content, without using QMP
at all.

But for back-compat we need some CLI option "sync-rtc-on-resume"
default to false. Preferably a mc146818rtc property to KISS.

That would solve Daniil problem and make Markus/myself happier.


So I started looking into this, and I'm a bit unsure about what we want 
this API to look like. What I mean is there isn't a generic RTC 
abstraction in QEMU, likewise there isn't an "RTC" global variable you 
can easily use to hook up some sort of API or ops-like functions like 
cpu_accel does.


One simple solution I'm seeing is making an mc146818-specific API like 
mc146818rtc_synchronize_pre_resume(), and call that directly wrapped 
inside an ifdef CONFIG_MC146818RTC inside system/cpus.c. We can then 
check the sync-on-resume property inside of that helper and optionally 
just return from it if it's not set.


Any objections on this approach? Is there a better way to do this?

Thanks!

Paolo, any objection?

Regards,

Phil.

Re: [RFC 0/4] Added Interrupt controller emulation for loongarch kvm

[Cornelia Huck]

On Wed, Jul 17 2024, Xianglai Li  wrote:

> Before this, the interrupt controller simulation has been completed
> in the user mode program. In order to reduce the loss caused by frequent
> switching of the virtual machine monitor from kernel mode to user mode
> when the guest accesses the interrupt controller, we add the interrupt
> controller simulation in kvm.
>
> In qemu side implementation is simple, just make a new IPI EXTIOI PCH KVM
> related several classes, And the interface to access kvm related data is
> implemented.
>
> Most of the simulation work of the interrupt controller is done in kvm.
> Because KVM the changes have not been the Linux community acceptance,
> the patches of this series will have RFC label until KVM patch into the 
> community.
>
> For the implementation of kvm simulation, refer to the following documents.
>
> IPI simulation implementation reference:
> https://github.com/loongson/LoongArch-Documentation/tree/main/docs/Loongson-3A5000-usermanual-EN/inter-processor-interrupts-and-communication
>
> EXTIOI simulation implementation reference:
> https://github.com/loongson/LoongArch-Documentation/tree/main/docs/Loongson-3A5000-usermanual-EN/io-interrupts/extended-io-interrupts
>
> PCH-PIC simulation implementation reference:
> https://github.com/loongson/LoongArch-Documentation/blob/main/docs/Loongson-7A1000-usermanual-EN/interrupt-controller.adoc
>
> For PCH-MSI, we used irqfd mechanism to send the interrupt signal
> generated by user state to kernel state and then to EXTIOI without
> maintaining PCH-MSI state in kernel state.
>
> You can easily get the code from the link below:
> the kernel:
> https://github.com/lixianglai/linux
> the branch is: interrupt
>
> the qemu:
> https://github.com/lixianglai/qemu
> the branch is: interrupt
>
> Please note that the code above is regularly updated based on community
> reviews.
>
> Cc: Paolo Bonzini  
> Cc: Song Gao  
> Cc: Huacai Chen  
> Cc: Jiaxun Yang  
> Cc: "Michael S. Tsirkin"  
> Cc: Cornelia Huck  
> Cc: k...@vger.kernel.org 
> Cc: Bibo Mao  
>
> Xianglai Li (4):
>   hw/loongarch: Add KVM IPI device support
>   hw/loongarch: Add KVM extioi device support
>   hw/loongarch: Add KVM pch pic device support
>   hw/loongarch: Add KVM pch msi device support
>
>  hw/intc/Kconfig |  12 ++
>  hw/intc/loongarch_extioi_kvm.c  | 141 +++
>  hw/intc/loongarch_ipi_kvm.c | 207 
>  hw/intc/loongarch_pch_msi.c |  42 --
>  hw/intc/loongarch_pch_pic.c |  20 ++-
>  hw/intc/loongarch_pch_pic_kvm.c | 189 +
>  hw/intc/meson.build |   3 +
>  hw/loongarch/virt.c | 141 ---
>  include/hw/intc/loongarch_extioi.h  |  34 -
>  include/hw/intc/loongarch_pch_msi.h |   2 +-
>  include/hw/intc/loongarch_pch_pic.h |  51 ++-
>  include/hw/intc/loongson_ipi.h  |  22 +++
>  include/hw/loongarch/virt.h |  15 ++
>  linux-headers/asm-loongarch/kvm.h   |   7 +
>  linux-headers/linux/kvm.h   |   6 +

Please split out any headers changes into a separate patch -- just put
them into a placeholder patch at the beginning of the series as long as
the changes are not yet upstream (and replace that with a full headers
sync later.)

>  15 files changed, 823 insertions(+), 69 deletions(-)
>  create mode 100644 hw/intc/loongarch_extioi_kvm.c
>  create mode 100644 hw/intc/loongarch_ipi_kvm.c
>  create mode 100644 hw/intc/loongarch_pch_pic_kvm.c

Re: [PATCH v1 00/11] Convert avocado tests to normal Python unittests

[Thomas Huth]

On 16/07/2024 19.03, Thomas Huth wrote:

On 16/07/2024 18.51, Daniel P. Berrangé wrote:

On Tue, Jul 16, 2024 at 01:26:03PM +0200, Thomas Huth wrote:

...

So instead of trying to update the python-based test suite in QEMU
to a newer version of Avocado, we should maybe try to better integrate
it with the meson test runner instead. Indeed most tests work quite
nicely without the Avocado framework already, as you can see with
this patch series - it does not convert all tests, just a subset so
far, but this already proves that many tests only need small modifi-
cations to work without Avocado.

...

Now if you want to try out these patches: Apply the patches, then
recompile and then run:

  make check-functional

You can also run single targets e.g. with:

  make check-functional-ppc

You can also run the tests without any test runner now by
setting the PYTHONPATH environment variable to the "python" folder
of your source tree, and by specifying the build directory via
QEMU_BUILD_ROOT (if autodetection fails) and by specifying the
QEMU binary via QEMU_TEST_QEMU_BINARY. For example:

  export PYTHONPATH=$HOME/qemu/python
  export QEMU_TEST_QEMU_BINARY=qemu-system-x86_64
  export PYTHONPATH=$HOME/qemu/build
  ~/qemu/tests/functional/test_virtio_version.py


For the whole series as is

  Tested-by: Daniel P. Berrangé 

as it does what you claim it does here when I tried it.


Thanks!


The logs of the tests can be found in the build directory under
tests/functional/ - console log and general logs will
be put in separate files there.


As an example, one dir name appears to be:

   __main__.MemAddrCheck.test_phybits_ok_pentium_pae

I'd rather prefer it if the dir name matched the test script
file name - in this case test_mem_addr_space.py, as I don't
want to have to lookup which class names were defined inside
each test script. We could drop the "test_" prefix from the
method name too

IOW, could we make this dir name be:

   test_mem_addr_space.phybits_ok_pentium_pae


I can try to change that, indeed ... but the boilerplate code will increase 
a little bit, I guess, since I cannot simply rely on the unittest.id() 
function in that case anymore...


After looking at this for a while, I think it's maybe best to ditch the idea 
of making the .py files directly runnable and run the tests via a simple 
pycotap runner instead. Then you get proper module names:


$ pyvenv/bin/python3 -m pycotap test_virtio_version
TAP version 13
ok 1 test_virtio_version.VirtioVersionCheck.test_conventional_devs
ok 2 test_virtio_version.VirtioVersionCheck.test_modern_only_devs
1..2

 Thomas

[ANNOUNCE] QEMU 9.0.2 Stable released

[Michael Tokarev]

Hi everyone,

The QEMU v9.0.2 stable release is now available.

You can grab the tarball from our download page here:

  https://www.qemu.org/download/#source

  https://download.qemu.org/qemu-9.0.2.tar.xz
  https://download.qemu.org/qemu-9.0.2.tar.xz.sig (signature)

v9.0.2 is now tagged in the official qemu.git repository, and the
stable-9.0 branch has been updated accordingly:

  https://gitlab.com/qemu-project/qemu/-/commits/stable-9.0

There are 27 changes since the previous v9.0.1 release, including
a fix for CVE-2024-4467 (qemu-img info command lack of input validation).

Thank you everyone who has been involved and helped with the stable series!

/mjt

Changelog (stable-9.0-hash master-hash Author Name: Commmit-Subject):

5ebde3b5c0 Michael Tokarev:
 Update version for 9.0.2 release
e0d660aeea 3936bbdf9a Vincent Fu:
 hw/nvme: fix number of PIDs for FDP RUH update
e4a9b44f7a e389929d19 Markus Armbruster:
 sphinx/qapidoc: Fix to generate doc for explicit, unboxed arguments
837864aa6c a0124e333e Maxim Mikityanskiy:
 char-stdio: Restore blocking mode of stdout on exit
8c86d8aa6c 7aa6492401 Stefano Garzarella:
 virtio: remove virtio_tswap16s() call in vring_packed_event_read()
c13615f78f a113d041e8 Cindy Lu:
 virtio-pci: Fix the failure process in kvm_virtio_pci_vector_use_one()
b4efc4ce2c a71d9dfbf6 Richard Henderson:
 tcg/optimize: Fix TCG_COND_TST* simplification of setcond2
5be2bb40e3 7ead946998 Kevin Wolf:
 block: Parse filenames only when explicitly requested
8c022d8af6 7e1110664e Kevin Wolf:
 iotests/270: Don't store data-file with json: prefix in image
0bbe8f9b12 2eb42a728d Kevin Wolf:
 iotests/244: Don't store data-file with protocol in image
312ca4065b bd385a5298 Kevin Wolf:
 qcow2: Don't open data_file with BDRV_O_NO_IO
68473fdd22 e68dcbb079 Daniel P. Berrang=C3=A9:
 tests: add testing of parameter=3D1 for SMP topology
a4fd014e33 9d7950edb0 Daniel P. Berrang=C3=A9:
 hw/core: allow parameter=3D1 for SMP topology on any machine
10f230bd61 7619129f0d Richard Henderson:
 target/arm: Fix FJCVTZS vs flush-to-zero
10b9e0c546 76bccf3cb9 Richard Henderson:
 target/arm: Fix VCMLA Dd, Dn, Dm[idx]
50a8a6b4d6 903916f0a0 Chuang Xu:
 i386/cpu: fixup number of addressable IDs for processor cores in the physi=
cal package
c048a5 641b1efe01 Thomas Huth:
 tests: Update our CI to use CentOS Stream 9 instead of 8
d7a4a38a03 6d3279655a Fabiano Rosas:
 migration: Fix file migration with fdset
658fb89bdc 521d7fb3eb Richard Henderson:
 tcg/loongarch64: Fix tcg_out_movi vs some pcrel pointers
c8fdbb5bab 6b4965373e Cl=C3=A9ment Chigot:
 target/sparc: use signed denominator in sdiv helper
0556f5fc13 54b2792102 Ilya Leoshkevich:
 linux-user: Make TARGET_NR_setgroups affect only the current thread
7ee955223e 3b279f73fa Anton Johansson:
 accel/tcg: Fix typo causing tb->page_addr[1] to not be recorded
37f037cb69 b1cf266c82 Gerd Hoffmann:
 stdvga: fix screen blanking
1608a7f81f a276ec8e26 Philippe Mathieu-Daud=C3=A9:
 hw/audio/virtio-snd: Always use little endian audio format
35e5ce5bd6 719c6819ed Stefan Hajnoczi:
 Revert "monitor: use aio_co_reschedule_self()"
0d90c36d9c 77bf310084 Dongwon Kim:
 ui/gtk: Draw guest frame at refresh cycle
09f36a1f3f 2c3e4e2de6 Alexey Dobriyan:
 virtio-net: drop too short packets early
db0a21257e 3973615e7f Mark Cave-Ayland:
 target/i386: fix size of EBP writeback in gen_enter()

[ANNOUNCE] QEMU 8.2.6 Stable released

[Michael Tokarev]

Hi everyone,

The QEMU v8.2.6 stable release is now available.

You can grab the tarball from our download page here:

  https://www.qemu.org/download/#source

  https://download.qemu.org/qemu-8.2.6.tar.xz
  https://download.qemu.org/qemu-8.2.6.tar.xz.sig (signature)

v8.2.6 is now tagged in the official qemu.git repository, and the
stable-8.2 branch has been updated accordingly:

  https://gitlab.com/qemu-project/qemu/-/commits/stable-8.2

There are 23 changes since the previous v8.2.5 release, including
a fix for CVE-2024-4467 (qemu-img info command lack of input validation).
This is supposed to be the last release in 8.2.x series.

Thank you everyone who has been involved and helped with the stable series!

/mjt

Changelog (stable-8.2-hash master-hash Author Name: Commmit-Subject):

46300ebc38 Michael Tokarev:
 Update version for 8.2.6 release
57d9378af9 3936bbdf9a Vincent Fu:
 hw/nvme: fix number of PIDs for FDP RUH update
55b151b6a6 e389929d19 Markus Armbruster:
 sphinx/qapidoc: Fix to generate doc for explicit, unboxed arguments
8f7bb1266f a0124e333e Maxim Mikityanskiy:
 char-stdio: Restore blocking mode of stdout on exit
b932f9fbd4 7aa6492401 Stefano Garzarella:
 virtio: remove virtio_tswap16s() call in vring_packed_event_read()
0d2c267638 a113d041e8 Cindy Lu:
 virtio-pci: Fix the failure process in kvm_virtio_pci_vector_use_one()
aea89f4179 7ead946998 Kevin Wolf:
 block: Parse filenames only when explicitly requested
46fdbe667d 7e1110664e Kevin Wolf:
 iotests/270: Don't store data-file with json: prefix in image
6a2774e8ae 2eb42a728d Kevin Wolf:
 iotests/244: Don't store data-file with protocol in image
d7e7f342c6 bd385a5298 Kevin Wolf:
 qcow2: Don't open data_file with BDRV_O_NO_IO
38fb9d1edc 7619129f0d Richard Henderson:
 target/arm: Fix FJCVTZS vs flush-to-zero
8c56d9f61a 76bccf3cb9 Richard Henderson:
 target/arm: Fix VCMLA Dd, Dn, Dm[idx]
40e04161b3 903916f0a0 Chuang Xu:
 i386/cpu: fixup number of addressable IDs for processor cores in the physi=
cal package
df0e72dc86 641b1efe01 Thomas Huth:
 tests: Update our CI to use CentOS Stream 9 instead of 8
5d9f2461b4 6d3279655a Fabiano Rosas:
 migration: Fix file migration with fdset
1229d60714 521d7fb3eb Richard Henderson:
 tcg/loongarch64: Fix tcg_out_movi vs some pcrel pointers
f4564fc8d2 6b4965373e Cl=C3=A9ment Chigot:
 target/sparc: use signed denominator in sdiv helper
1649e9559b 54b2792102 Ilya Leoshkevich:
 linux-user: Make TARGET_NR_setgroups affect only the current thread
40682cfcea 3b279f73fa Anton Johansson:
 accel/tcg: Fix typo causing tb->page_addr[1] to not be recorded
d08c1fd6db b1cf266c82 Gerd Hoffmann:
 stdvga: fix screen blanking
1798f38242 a276ec8e26 Philippe Mathieu-Daud=C3=A9:
 hw/audio/virtio-snd: Always use little endian audio format
fdbeeb454c 77bf310084 Dongwon Kim:
 ui/gtk: Draw guest frame at refresh cycle
fa275f2211 2c3e4e2de6 Alexey Dobriyan:
 virtio-net: drop too short packets early
3e09472893 3973615e7f Mark Cave-Ayland:
 target/i386: fix size of EBP writeback in gen_enter()

Re: [RFC PATCH v4] ptp: Add vDSO-style vmclock support

[David Woodhouse]

On Tue, 2024-07-16 at 13:54 +0200, Peter Hilber wrote:
> On 08.07.24 11:27, David Woodhouse wrote:
> > +
> > +   /*
> > +    * Time according to time_type field above.
> > +    */
> > +   uint64_t time_sec;  /* Seconds since time_type epoch */
> > +   uint64_t time_frac_sec; /* (seconds >> 64) */
> > +   uint64_t time_esterror_picosec; /* (± picoseconds) */
> > +   uint64_t time_maxerror_picosec; /* (± picoseconds) */
> 
> Is this unsigned or signed?

The field itself is unsigned, as it provides the absolute value of the
error (which can be in either direction). Probably better just to drop
the ± from the comment.

Julien is now back from vacation and I'm expecting to see his opinion
on whether we can change that to nanoseconds for consistency.


smime.p7s
Description: S/MIME cryptographic signature

Re: [RFC PATCH v4] ptp: Add vDSO-style vmclock support

[David Woodhouse]

On Tue, 2024-07-16 at 15:20 +0200, Peter Hilber wrote:
> On 16.07.24 14:32, David Woodhouse wrote:
> > On 16 July 2024 12:54:52 BST, Peter Hilber  
> > wrote:
> > > On 11.07.24 09:50, David Woodhouse wrote:
> > > > On Thu, 2024-07-11 at 09:25 +0200, Peter Hilber wrote:
> > > > > 
> > > > > IMHO this phrasing is better, since it directly refers to the state 
> > > > > of the
> > > > > structure.
> > > > 
> > > > Thanks. I'll update it.
> > > > 
> > > > > AFAIU if there would be abnormal delays in store buffers, causing some
> > > > > driver to still see the old clock for some time, the monotonicity 
> > > > > could be
> > > > > violated:
> > > > > 
> > > > > 1. device writes new, much slower clock to store buffer
> > > > > 2. some time passes
> > > > > 3. driver reads old, much faster clock
> > > > > 4. device writes store buffer to cache
> > > > > 5. driver reads new, much slower clock
> > > > > 
> > > > > But I hope such delays do not occur.
> > > > 
> > > > For the case of the hypervisor←→guest interface this should be handled
> > > > by the use of memory barriers and the seqcount lock.
> > > > 
> > > > The guest driver reads the seqcount, performs a read memory barrier,
> > > > then reads the contents of the structure. Then performs *another* read
> > > > memory barrier, and checks the seqcount hasn't changed:
> > > > https://git.infradead.org/?p=users/dwmw2/linux.git;a=blob;f=drivers/ptp/ptp_vmclock.c;hb=vmclock#l351
> > > > 
> > > > The converse happens with write barriers on the hypervisor side:
> > > > https://git.infradead.org/?p=users/dwmw2/qemu.git;a=blob;f=hw/acpi/vmclock.c;hb=vmclock#l68
> > > 
> > > My point is that, looking at the above steps 1. - 5.:
> > > 
> > > 3. read HW counter, smp_rmb, read seqcount
> > > 4. store seqcount, smp_wmb, stores, smp_wmb, store seqcount become 
> > > effective
> > > 5. read seqcount, smp_rmb, read HW counter
> > > 
> > > AFAIU this would still be a theoretical problem suggesting the use of
> > > stronger barriers.
> > 
> > This seems like a bug on the guest side. The HW counter needs to be read 
> > *within* the (paired, matching) seqcount reads, not before or after.
> > 
> > 
> 
> There would be paired reads:
> 
> 1. device writes new, much slower clock to store buffer
> 2. some time passes
> 3. read seqcount, smp_rmb, ..., read HW counter, smp_rmb, read seqcount
> 4. store seqcount, smp_wmb, stores, smp_wmb, store seqcount all become
>    effective only now
> 5. read seqcount, smp_rmb, read HW counter, ..., smp_rmb, read seqcount
> 
> I just omitted the parts which do not necessarily need to happen close to
> 4. for the monotonicity to be violated. My point is that 1. could become
> visible to other cores long after it happened on the local core (during
> 4.).

Oh, I see. That would be a bug on the device side then. And as you say,
it could be fixed by using the appropriate barriers. Or my alternative
of just documenting "Don't Do That Then".



smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH] qapi/qom: Document feature unstable of @x-vfio-user-server

[Markus Armbruster]

Markus Armbruster  writes:

> Commit 8f9a9259d32c added ObjectType member @x-vfio-user-server with
> feature unstable, but neglected to explain why it is unstable.  Do
> that now.
>
> Fixes: 8f9a9259d32c (vfio-user: define vfio-user-server object)
> Cc: Elena Ufimtseva 
> Cc: John G Johnson 
> Cc: Jagannathan Raman 
> Cc: qemu-sta...@nongnu.org
> Signed-off-by: Markus Armbruster 
> ---
>  qapi/qom.json | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/qapi/qom.json b/qapi/qom.json
> index 8bd299265e..3c0f8c633d 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -1012,7 +1012,8 @@
>  #
>  # Features:
>  #
> -# @unstable: Member @x-remote-object is experimental.
> +# @unstable: Members @x-remote-object and @x-vfio-user-server are
> +# experimental.

Second line needs to be indented.  Fixed in my tree.

>  #
>  # Since: 6.0
>  ##

Queued.

Re: [RFC PATCH] gdbstub: Re-factor gdb command extensions

[Alex Bennée]

Richard Henderson  writes:

> On 7/17/24 02:55, Alex Bennée wrote:
>>> Are you expecting the same GdbCmdParseEntry object to be registered
>>> multiple times?  Can we fix that at a higher level?
>> Its basically a hack to deal with the fact everything is tied to the
>> CPUObject so we register everything multiple times. We could do a if
>> (!registerd) register() dance but I guess I'm thinking forward to a
>> hydrogenous future but I guess we'd need to do more work then anyway.
>
> Any chance we could move it all to the CPUClass?

We would have to move quite a bunch of stuff, including the system
register creation code. I don't think that's a re-factor I want to
attempt quite so close to soft-freeze.

>
>
> r~

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v1 00/11] Convert avocado tests to normal Python unittests

[Daniel P . Berrangé]

On Wed, Jul 17, 2024 at 10:04:19AM +0200, Thomas Huth wrote:
> On 16/07/2024 19.03, Thomas Huth wrote:
> > On 16/07/2024 18.51, Daniel P. Berrangé wrote:
> > > On Tue, Jul 16, 2024 at 01:26:03PM +0200, Thomas Huth wrote:
> > ...
> > > > So instead of trying to update the python-based test suite in QEMU
> > > > to a newer version of Avocado, we should maybe try to better integrate
> > > > it with the meson test runner instead. Indeed most tests work quite
> > > > nicely without the Avocado framework already, as you can see with
> > > > this patch series - it does not convert all tests, just a subset so
> > > > far, but this already proves that many tests only need small modifi-
> > > > cations to work without Avocado.
> > ...
> > > > Now if you want to try out these patches: Apply the patches, then
> > > > recompile and then run:
> > > > 
> > > >   make check-functional
> > > > 
> > > > You can also run single targets e.g. with:
> > > > 
> > > >   make check-functional-ppc
> > > > 
> > > > You can also run the tests without any test runner now by
> > > > setting the PYTHONPATH environment variable to the "python" folder
> > > > of your source tree, and by specifying the build directory via
> > > > QEMU_BUILD_ROOT (if autodetection fails) and by specifying the
> > > > QEMU binary via QEMU_TEST_QEMU_BINARY. For example:
> > > > 
> > > >   export PYTHONPATH=$HOME/qemu/python
> > > >   export QEMU_TEST_QEMU_BINARY=qemu-system-x86_64
> > > >   export PYTHONPATH=$HOME/qemu/build
> > > >   ~/qemu/tests/functional/test_virtio_version.py
> > > 
> > > For the whole series as is
> > > 
> > >   Tested-by: Daniel P. Berrangé 
> > > 
> > > as it does what you claim it does here when I tried it.
> > 
> > Thanks!
> > 
> > > > The logs of the tests can be found in the build directory under
> > > > tests/functional/ - console log and general logs will
> > > > be put in separate files there.
> > > 
> > > As an example, one dir name appears to be:
> > > 
> > >    __main__.MemAddrCheck.test_phybits_ok_pentium_pae
> > > 
> > > I'd rather prefer it if the dir name matched the test script
> > > file name - in this case test_mem_addr_space.py, as I don't
> > > want to have to lookup which class names were defined inside
> > > each test script. We could drop the "test_" prefix from the
> > > method name too
> > > 
> > > IOW, could we make this dir name be:
> > > 
> > >    test_mem_addr_space.phybits_ok_pentium_pae
> > 
> > I can try to change that, indeed ... but the boilerplate code will
> > increase a little bit, I guess, since I cannot simply rely on the
> > unittest.id() function in that case anymore...
> 
> After looking at this for a while, I think it's maybe best to ditch the idea
> of making the .py files directly runnable and run the tests via a simple
> pycotap runner instead. Then you get proper module names:

I'd really not want to loose that. To me, eliminating the test harness
entirely when debugging is the single biggest improvement of this new
approach, especially when I want to 'strace' the test without
extraneous processes.

> $ pyvenv/bin/python3 -m pycotap test_virtio_version
> TAP version 13
> ok 1 test_virtio_version.VirtioVersionCheck.test_conventional_devs
> ok 2 test_virtio_version.VirtioVersionCheck.test_modern_only_devs
> 1..

With the following change, you get the same output with direct
execution, by making argv look the same as you'd get when
running your pycotap example.

diff --git a/tests/functional/qemu_test/__init__.py 
b/tests/functional/qemu_test/__init__.py
index cc49fd4c94..3a3e65252d 100644
--- a/tests/functional/qemu_test/__init__.py
+++ b/tests/functional/qemu_test/__init__.py
@@ -266,7 +266,10 @@ def fetch_asset(self, url, asset_hash):
 def main():
 tr = pycotap.TAPTestRunner(message_log = pycotap.LogMode.LogToError,
test_output_log = 
pycotap.LogMode.LogToError)
-unittest.main(testRunner = tr)
+import sys
+import os.path
+path = os.path.basename(sys.argv[0])[:-3]
+unittest.main(module = None, testRunner = tr, argv=["__dummy__", path])
 
 
 class QemuSystemTest(QemuBaseTest):

With regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [PATCH v1 00/11] Convert avocado tests to normal Python unittests

[Thomas Huth]

On 17/07/2024 10.37, Daniel P. Berrangé wrote:

On Wed, Jul 17, 2024 at 10:04:19AM +0200, Thomas Huth wrote:

On 16/07/2024 19.03, Thomas Huth wrote:

On 16/07/2024 18.51, Daniel P. Berrangé wrote:

On Tue, Jul 16, 2024 at 01:26:03PM +0200, Thomas Huth wrote:

...

So instead of trying to update the python-based test suite in QEMU
to a newer version of Avocado, we should maybe try to better integrate
it with the meson test runner instead. Indeed most tests work quite
nicely without the Avocado framework already, as you can see with
this patch series - it does not convert all tests, just a subset so
far, but this already proves that many tests only need small modifi-
cations to work without Avocado.

...

Now if you want to try out these patches: Apply the patches, then
recompile and then run:

   make check-functional

You can also run single targets e.g. with:

   make check-functional-ppc

You can also run the tests without any test runner now by
setting the PYTHONPATH environment variable to the "python" folder
of your source tree, and by specifying the build directory via
QEMU_BUILD_ROOT (if autodetection fails) and by specifying the
QEMU binary via QEMU_TEST_QEMU_BINARY. For example:

   export PYTHONPATH=$HOME/qemu/python
   export QEMU_TEST_QEMU_BINARY=qemu-system-x86_64
   export PYTHONPATH=$HOME/qemu/build
   ~/qemu/tests/functional/test_virtio_version.py


For the whole series as is

   Tested-by: Daniel P. Berrangé 

as it does what you claim it does here when I tried it.


Thanks!


The logs of the tests can be found in the build directory under
tests/functional/ - console log and general logs will
be put in separate files there.


As an example, one dir name appears to be:

    __main__.MemAddrCheck.test_phybits_ok_pentium_pae

I'd rather prefer it if the dir name matched the test script
file name - in this case test_mem_addr_space.py, as I don't
want to have to lookup which class names were defined inside
each test script. We could drop the "test_" prefix from the
method name too

IOW, could we make this dir name be:

    test_mem_addr_space.phybits_ok_pentium_pae


I can try to change that, indeed ... but the boilerplate code will
increase a little bit, I guess, since I cannot simply rely on the
unittest.id() function in that case anymore...


After looking at this for a while, I think it's maybe best to ditch the idea
of making the .py files directly runnable and run the tests via a simple
pycotap runner instead. Then you get proper module names:


I'd really not want to loose that. To me, eliminating the test harness
entirely when debugging is the single biggest improvement of this new
approach, especially when I want to 'strace' the test without
extraneous processes.


$ pyvenv/bin/python3 -m pycotap test_virtio_version
TAP version 13
ok 1 test_virtio_version.VirtioVersionCheck.test_conventional_devs
ok 2 test_virtio_version.VirtioVersionCheck.test_modern_only_devs
1..


With the following change, you get the same output with direct
execution, by making argv look the same as you'd get when
running your pycotap example.

diff --git a/tests/functional/qemu_test/__init__.py 
b/tests/functional/qemu_test/__init__.py
index cc49fd4c94..3a3e65252d 100644
--- a/tests/functional/qemu_test/__init__.py
+++ b/tests/functional/qemu_test/__init__.py
@@ -266,7 +266,10 @@ def fetch_asset(self, url, asset_hash):
  def main():
  tr = pycotap.TAPTestRunner(message_log = pycotap.LogMode.LogToError,
 test_output_log = 
pycotap.LogMode.LogToError)
-unittest.main(testRunner = tr)
+import sys
+import os.path
+path = os.path.basename(sys.argv[0])[:-3]
+unittest.main(module = None, testRunner = tr, argv=["__dummy__", path])


Sweet, thank you very much, looks like this will do the job!

 Thomas

Re: [PATCH 0/2] Postcopy migration and vhost-user errors

[Michael S. Tsirkin]

On Tue, Jul 16, 2024 at 06:02:50PM -0400, Peter Xu wrote:
> On Tue, Jul 16, 2024 at 03:44:54PM +0530, Prasad Pandit wrote:
> > Hello Peter,
> > 
> > On Mon, 15 Jul 2024 at 19:10, Peter Xu  wrote:
> > > IMHO it's better we debug and fix all the issues before merging this one,
> > > otherwise we may overlook something.
> > 
> > * Well we don't know where the issue is, not sure where the fix may go
> > in, ex. if the issue turns out to be how virsh(1) invokes
> > migrate-postcopy, fix may go in virsh(1). Patches in this series
> > anyway don't help to fix the migration convergence issue, so they
> > could be reviewed independently I guess.
> 
> I still think we should find a complete solution before merging anything,
> because I'm not 100% confident the issue to be further investigated is
> irrelevant to this patch.
> 
> No strong opinions, I'll leave that to Michael to decide.
> 
> > 
> > > You could pass over the patch to whoever going to debug this, so it will 
> > > be included in the whole set to be
> > > posted when the bug is completely fixed.
> > 
> > * Yes, this patch series is linked there.
> > 
> > > The protocol should have no restriction on the thread model of a 
> > > front-end.
> > > It only describes the wire protocol.
> > >
> > > IIUC the protocol was designed to be serialized by nature (where there's 
> > > no
> > > request ID, so we can't match reply to any of the previous response), then
> > > the front-end can manage the threads well to serialize all the requests,
> > > like using this rwlock.
> > 
> > * I see, okay. The simple protocol definition seems to indicate that
> > it is meant for one front-end/back-end pair. If we are dividing the
> > front-end across multiple threads, maybe we need a document to
> > describe those threads and how they work, at least for the QEMU
> > (front-end) side. Because the back-end could be a non-QEMU process, we
> > can not do much there. (just thinking)
> 
> IMHO that's not part of the protocol but impl details, so the current doc
> looks all fine to me.
> 
> Thanks,
> 
> -- 
> Peter Xu


I just want to understand how we managed to have two threads
talking in parallel. BQL is normally enough, which path
manages to invoke vhost-user with BQL not taken?
Just check BQL taken on each vhost user invocation and
you will figure it out.


-- 
MST

[PATCH v2] pci-bridge: avoid linking a single downstream port more than once

[Yao Xingtao via]

Since the downstream port is not checked, two slots can be linked to
a single port. However, this can prevent the driver from detecting the
device properly.

It is necessary to ensure that a downstream port is not linked more than
once.

Links: 
https://lore.kernel.org/qemu-devel/oszpr01mb6453bc61d2ff4035f18084ef8d...@oszpr01mb6453.jpnprd01.prod.outlook.com
Signed-off-by: Yao Xingtao 

---
V1[1] -> V2:
 - Move downstream port check forward

[1] 
https://lore.kernel.org/qemu-devel/20240704033834.3362-1-yaoxt.f...@fujitsu.com
---
 hw/pci-bridge/cxl_downstream.c | 5 +
 hw/pci-bridge/pcie_root_port.c | 5 +
 hw/pci-bridge/xio3130_downstream.c | 5 +
 3 files changed, 15 insertions(+)

diff --git a/hw/pci-bridge/cxl_downstream.c b/hw/pci-bridge/cxl_downstream.c
index 742da07a015a..af81ddfeec13 100644
--- a/hw/pci-bridge/cxl_downstream.c
+++ b/hw/pci-bridge/cxl_downstream.c
@@ -142,6 +142,11 @@ static void cxl_dsp_realize(PCIDevice *d, Error **errp)
 MemoryRegion *component_bar = &cregs->component_registers;
 int rc;
 
+if (pcie_find_port_by_pn(pci_get_bus(d), p->port) != NULL) {
+error_setg(errp, "Can't link port, error %d", -EBUSY);
+return;
+}
+
 pci_bridge_initfn(d, TYPE_PCIE_BUS);
 pcie_port_init_reg(d);
 
diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
index 09a34786bc62..a540204bda27 100644
--- a/hw/pci-bridge/pcie_root_port.c
+++ b/hw/pci-bridge/pcie_root_port.c
@@ -67,6 +67,11 @@ static void rp_realize(PCIDevice *d, Error **errp)
 PCIERootPortClass *rpc = PCIE_ROOT_PORT_GET_CLASS(d);
 int rc;
 
+if (pcie_find_port_by_pn(pci_get_bus(d), p->port) != NULL) {
+error_setg(errp, "Can't link port, error %d", -EBUSY);
+return;
+}
+
 pci_config_set_interrupt_pin(d->config, 1);
 if (d->cap_present & QEMU_PCIE_CAP_CXL) {
 pci_bridge_initfn(d, TYPE_CXL_BUS);
diff --git a/hw/pci-bridge/xio3130_downstream.c 
b/hw/pci-bridge/xio3130_downstream.c
index 907d5105b019..63f6baa615fd 100644
--- a/hw/pci-bridge/xio3130_downstream.c
+++ b/hw/pci-bridge/xio3130_downstream.c
@@ -69,6 +69,11 @@ static void xio3130_downstream_realize(PCIDevice *d, Error 
**errp)
 PCIESlot *s = PCIE_SLOT(d);
 int rc;
 
+if (pcie_find_port_by_pn(pci_get_bus(d), p->port) != NULL) {
+error_setg(errp, "Can't link port, error %d", -EBUSY);
+return;
+}
+
 pci_bridge_initfn(d, TYPE_PCIE_BUS);
 pcie_port_init_reg(d);
 
-- 
2.37.3

Re: Re: [PATCH v9 09/10] hw/nvme: add reservation protocal command

[卢长奇]

Hi,


Thank you for your support.
1. I will add a guide on how to get a simple test at next patch.
2. I think hostid is stored locally like cntlid, but I can't
find a way to get the host ID. Is it correct to get it through
qmp_query_uuid() method?

Using spdk as target will not fail, but it will show 0 at hostid
at present.

On 2024/7/15 18:09, Klaus Jensen wrote:
> On Jul 12 10:36, Changqi Lu wrote:
>> Add reservation acquire, reservation register,
>> reservation release and reservation report commands
>> in the nvme device layer.
>>
>> By introducing these commands, this enables the nvme
>> device to perform reservation-related tasks, including
>> querying keys, querying reservation status, registering
>> reservation keys, initiating and releasing reservations,
>> as well as clearing and preempting reservations held by
>> other keys.
>>
>> These commands are crucial for management and control of
>> shared storage resources in a persistent manner.
>> Signed-off-by: Changqi Lu
>> Signed-off-by: zhenwei pi
>> Acked-by: Klaus Jensen
>> ---
>> hw/nvme/ctrl.c | 318 +++
>> hw/nvme/nvme.h | 4 +
>> include/block/nvme.h | 37 +
>> 3 files changed, 359 insertions(+)
>>
>
> This looks good to me, but two comments.
>
> 1. Do you have a small guide on how to get a simple test environment
> up for this?
>
> 2. Can you touch on the justification for not supporting the remaining
> mandatory features required when indicating Reservation support?
>
> hw/nvme *does* compromise on mandatory features from time to time
> when it makes sense, so I'm not saying that not having the log
> pages, notifications and so on is a deal breaker, I'm just
> interested in the justification and/or motivation.
>
> For instance, I think the SPDK reserve test will fail on this due
> to lack of Host Identifier Feature support.

Re: [PATCH v4 07/12] vfio/{iommufd,container}: Initialize HostIOMMUDeviceCaps during attach_device()

[Joao Martins]

On 17/07/2024 03:05, Duan, Zhenzhong wrote:
> 
> 
>> -Original Message-
>> From: Joao Martins 
>> Subject: [PATCH v4 07/12] vfio/{iommufd,container}: Initialize
>> HostIOMMUDeviceCaps during attach_device()
>>
>> Fetch IOMMU hw raw caps behind the device and thus move the
>> HostIOMMUDevice::realize() to be done during the attach of the device. It
>> allows it to cache the information obtained from IOMMU_GET_HW_INFO
>> from
>> iommufd early on. However, while legacy HostIOMMUDevice caps
>> always return true and doesn't have dependency on other things, the
>> IOMMUFD
>> backend requires the iommufd FD to be connected and having a devid to be
>> able to query capabilities. Hence when exactly is HostIOMMUDevice
>> initialized inside backend ::attach_device() implementation is backend
>> specific.
>>
>> This is in preparation to fetch parse hw capabilities and understand if
>> dirty tracking is supported by device backing IOMMU without necessarily
>> duplicating the amount of calls we do to IOMMU_GET_HW_INFO.
>>
>> Suggested-by: Cédric Le Goater 
>> Signed-off-by: Joao Martins 
>> ---
>> include/sysemu/host_iommu_device.h |  1 +
>> hw/vfio/common.c   | 16 ++--
>> hw/vfio/container.c|  6 ++
>> hw/vfio/iommufd.c  |  7 +++
>> 4 files changed, 20 insertions(+), 10 deletions(-)
>>
>> diff --git a/include/sysemu/host_iommu_device.h
>> b/include/sysemu/host_iommu_device.h
>> index 20e77cf54568..b1e5f4b8ac3e 100644
>> --- a/include/sysemu/host_iommu_device.h
>> +++ b/include/sysemu/host_iommu_device.h
>> @@ -24,6 +24,7 @@
>>  */
>> typedef struct HostIOMMUDeviceCaps {
>> uint32_t type;
>> +uint64_t hw_caps;
>> } HostIOMMUDeviceCaps;
>>
>> #define TYPE_HOST_IOMMU_DEVICE "host-iommu-device"
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index b0beed44116e..cc14f0e3fe24 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -1544,7 +1544,7 @@ bool vfio_attach_device(char *name, VFIODevice
>> *vbasedev,
>> {
>> const VFIOIOMMUClass *ops =
>>
>> VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_LEGACY));
>> -HostIOMMUDevice *hiod;
>> +HostIOMMUDevice *hiod = NULL;
> 
> No need to NULL it?
> 
/me nods

>>
>> if (vbasedev->iommufd) {
>> ops =
>> VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_IOMMUF
>> D));
>> @@ -1552,21 +1552,17 @@ bool vfio_attach_device(char *name,
>> VFIODevice *vbasedev,
>>
>> assert(ops);
>>
>> -if (!ops->attach_device(name, vbasedev, as, errp)) {
>> -return false;
>> -}
>>
>> -if (vbasedev->mdev) {
>> -return true;
>> +if (!vbasedev->mdev) {
>> +hiod = HOST_IOMMU_DEVICE(object_new(ops->hiod_typename));
>> +vbasedev->hiod = hiod;
>> }
>>
>> -hiod = HOST_IOMMU_DEVICE(object_new(ops->hiod_typename));
>> -if (!HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod, vbasedev,
>> errp)) {
>> +if (!ops->attach_device(name, vbasedev, as, errp)) {
>> object_unref(hiod);
>> -ops->detach_device(vbasedev);
>> +vbasedev->hiod = NULL;
>> return false;
>> }
>> -vbasedev->hiod = hiod;
>>
>> return true;
>> }
>> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
>> index c27f448ba26e..29da261bbf3e 100644
>> --- a/hw/vfio/container.c
>> +++ b/hw/vfio/container.c
>> @@ -907,6 +907,7 @@ static bool vfio_legacy_attach_device(const char
>> *name, VFIODevice *vbasedev,
>>   AddressSpace *as, Error **errp)
>> {
>> int groupid = vfio_device_groupid(vbasedev, errp);
>> +HostIOMMUDevice *hiod = vbasedev->hiod;
> 
> Hiod is used only once in this func, may be use vbasedev->hiod directly?
> 

The problem is more of how the line below (...)
> 
>> VFIODevice *vbasedev_iter;
>> VFIOGroup *group;
>> VFIOContainerBase *bcontainer;
>> @@ -917,6 +918,11 @@ static bool vfio_legacy_attach_device(const char
>> *name, VFIODevice *vbasedev,
>>
>> trace_vfio_attach_device(vbasedev->name, groupid);
>>
>> +if (hiod &&
>> +!HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod, vbasedev,
>> errp)) {
>> +return false;
>> +}
>> +

(...) would look like like really long. And I would end up deref-ing 3 times.

But with the helper function that Cedric suggests might easy to accomodate your
comment.

>> group = vfio_get_group(groupid, as, errp);
>> if (!group) {
>> return false;
>> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
>> index 873c919e319c..d34dc88231ec 100644
>> --- a/hw/vfio/iommufd.c
>> +++ b/hw/vfio/iommufd.c
>> @@ -384,6 +384,7 @@ static bool iommufd_cdev_attach(const char *name,
>> VFIODevice *vbasedev,
>> Error *err = NULL;
>> const VFIOIOMMUClass *iommufd_vioc =
>>
>> VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_IOMMUF
>> D));
>> +HostIOMMUDevice *hiod = vbasedev->hiod;
> 
> Same here.
> 
>>
>> if (vbasedev->fd < 0) {
>> devfd = iommufd_cdev_getfd(vbasedev->sysfs

Re: [PATCH v4 05/12] vfio/iommufd: Introduce auto domain creation

[Joao Martins]

On 17/07/2024 03:18, Duan, Zhenzhong wrote:
> 
> 
>> -Original Message-
>> From: Joao Martins 
>> Subject: [PATCH v4 05/12] vfio/iommufd: Introduce auto domain creation
>>
>> There's generally two modes of operation for IOMMUFD:
>>
>> * The simple user API which intends to perform relatively simple things
>> with IOMMUs e.g. DPDK. It generally creates an IOAS and attach to VFIO
>> and mainly performs IOAS_MAP and UNMAP.
>>
>> * The native IOMMUFD API where you have fine grained control of the
>> IOMMU domain and model it accordingly. This is where most new feature
>> are being steered to.
>>
>> For dirty tracking 2) is required, as it needs to ensure that
>> the stage-2/parent IOMMU domain will only attach devices
>> that support dirty tracking (so far it is all homogeneous in x86, likely
>> not the case for smmuv3). Such invariant on dirty tracking provides a
>> useful guarantee to VMMs that will refuse incompatible device
>> attachments for IOMMU domains.
>>
>> Dirty tracking insurance is enforced via HWPT_ALLOC, which is
>> responsible for creating an IOMMU domain. This is contrast to the
>> 'simple API' where the IOMMU domain is created by IOMMUFD
>> automatically
>> when it attaches to VFIO (usually referred as autodomains) but it has
>> the needed handling for mdevs.
>>
>> To support dirty tracking with the advanced IOMMUFD API, it needs
>> similar logic, where IOMMU domains are created and devices attached to
>> compatible domains. Essentially mimmicing kernel
>> iommufd_device_auto_get_domain(). With mdevs given there's no IOMMU
>> domain
>> it falls back to IOAS attach.
>>
>> The auto domain logic allows different IOMMU domains to be created when
>> DMA dirty tracking is not desired (and VF can provide it), and others where
>> it is. Here is not used in this way here given how VFIODevice migration
>> state is initialized after the device attachment. But such mixed mode of
>> IOMMU dirty tracking + device dirty tracking is an improvement that can
>> be added on. Keep the 'all of nothing' of type1 approach that we have
>> been using so far between container vs device dirty tracking.
>>
>> Signed-off-by: Joao Martins 
>> ---
>> include/hw/vfio/vfio-common.h |  9 
>> include/sysemu/iommufd.h  |  5 +++
>> backends/iommufd.c| 30 +
>> hw/vfio/iommufd.c | 82
>> +++
>> backends/trace-events |  1 +
>> 5 files changed, 127 insertions(+)
>>
>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-
>> common.h
>> index 7419466bca92..2dd468ce3c02 100644
>> --- a/include/hw/vfio/vfio-common.h
>> +++ b/include/hw/vfio/vfio-common.h
>> @@ -95,10 +95,17 @@ typedef struct VFIOHostDMAWindow {
>>
>> typedef struct IOMMUFDBackend IOMMUFDBackend;
>>
>> +typedef struct VFIOIOASHwpt {
>> +uint32_t hwpt_id;
>> +QLIST_HEAD(, VFIODevice) device_list;
>> +QLIST_ENTRY(VFIOIOASHwpt) next;
>> +} VFIOIOASHwpt;
>> +
>> typedef struct VFIOIOMMUFDContainer {
>> VFIOContainerBase bcontainer;
>> IOMMUFDBackend *be;
>> uint32_t ioas_id;
>> +QLIST_HEAD(, VFIOIOASHwpt) hwpt_list;
>> } VFIOIOMMUFDContainer;
>>
>> OBJECT_DECLARE_SIMPLE_TYPE(VFIOIOMMUFDContainer,
>> VFIO_IOMMU_IOMMUFD);
>> @@ -135,6 +142,8 @@ typedef struct VFIODevice {
>> HostIOMMUDevice *hiod;
>> int devid;
>> IOMMUFDBackend *iommufd;
>> +VFIOIOASHwpt *hwpt;
>> +QLIST_ENTRY(VFIODevice) hwpt_next;
>> } VFIODevice;
>>
>> struct VFIODeviceOps {
>> diff --git a/include/sysemu/iommufd.h b/include/sysemu/iommufd.h
>> index 57d502a1c79a..e917e7591d05 100644
>> --- a/include/sysemu/iommufd.h
>> +++ b/include/sysemu/iommufd.h
>> @@ -50,6 +50,11 @@ int
>> iommufd_backend_unmap_dma(IOMMUFDBackend *be, uint32_t ioas_id,
>> bool iommufd_backend_get_device_info(IOMMUFDBackend *be, uint32_t
>> devid,
>>  uint32_t *type, void *data, uint32_t 
>> len,
>>  uint64_t *caps, Error **errp);
>> +bool iommufd_backend_alloc_hwpt(IOMMUFDBackend *be, uint32_t
>> dev_id,
>> +uint32_t pt_id, uint32_t flags,
>> +uint32_t data_type, uint32_t data_len,
>> +void *data_ptr, uint32_t *out_hwpt,
>> +Error **errp);
>>
>> #define TYPE_HOST_IOMMU_DEVICE_IOMMUFD
>> TYPE_HOST_IOMMU_DEVICE "-iommufd"
>> #endif
>> diff --git a/backends/iommufd.c b/backends/iommufd.c
>> index 2b3d51af26d2..5d3dfa917415 100644
>> --- a/backends/iommufd.c
>> +++ b/backends/iommufd.c
>> @@ -208,6 +208,36 @@ int
>> iommufd_backend_unmap_dma(IOMMUFDBackend *be, uint32_t ioas_id,
>> return ret;
>> }
>>
>> +bool iommufd_backend_alloc_hwpt(IOMMUFDBackend *be, uint32_t
>> dev_id,
>> +uint32_t pt_id, uint32_t flags,
>> +uint32_t data_type, uint32_t data_len,
>> +void *data_ptr, uint32_t *out_hwpt,

Re: [PATCH v4 05/12] vfio/iommufd: Introduce auto domain creation

[Joao Martins]

On 17/07/2024 03:52, Duan, Zhenzhong wrote:
> 
> 
>> -Original Message-
>> From: Joao Martins 
>> Subject: Re: [PATCH v4 05/12] vfio/iommufd: Introduce auto domain
>> creation
>>
>> On 16/07/2024 17:44, Joao Martins wrote:
>>> On 16/07/2024 17:04, Eric Auger wrote:
 Hi Joao,

 On 7/12/24 13:46, Joao Martins wrote:
> There's generally two modes of operation for IOMMUFD:
>
> * The simple user API which intends to perform relatively simple things
> with IOMMUs e.g. DPDK. It generally creates an IOAS and attach to VFIO

 It generally creates? can you explicit what is "it"

>>> 'It' here refers to the process/API-user
>>>
 I am confused by this automatic terminology again (not your fault). the
>> doc says:
 "

   *

 Automatic domain - refers to an iommu domain created automatically
 when attaching a device to an IOAS object. This is compatible to the
 semantics of VFIO type1.

   *

 Manual domain - refers to an iommu domain designated by the user as
 the target pagetable to be attached to by a device. Though currently
 there are no uAPIs to directly create such domain, the datastructure
 and algorithms are ready for handling that use case.

 "


 in 1) the device is attached to the ioas id (using the auto domain if I am
>> not wrong)
 Here you attach to an hwpt id. Isn't it a manual domain?

>>>
>>> Correct.
>>>
>>> The 'auto domains' generally refers to the kernel-equivalent own
>> automatic
>>> attaching to a new pagetable.
>>>
>>> Here I call 'auto domains' in the userspace version too because we are
>> doing the
>>> exact same but from userspace, using the manual API in IOMMUFD.
>>>
> and mainly performs IOAS_MAP and UNMAP.
>
> * The native IOMMUFD API where you have fine grained control of the
> IOMMU domain and model it accordingly. This is where most new
>> feature
> are being steered to.
>
> For dirty tracking 2) is required, as it needs to ensure that
> the stage-2/parent IOMMU domain will only attach devices
> that support dirty tracking (so far it is all homogeneous in x86, likely
> not the case for smmuv3). Such invariant on dirty tracking provides a
> useful guarantee to VMMs that will refuse incompatible device
> attachments for IOMMU domains.
>
> Dirty tracking insurance is enforced via HWPT_ALLOC, which is
> responsible for creating an IOMMU domain. This is contrast to the
> 'simple API' where the IOMMU domain is created by IOMMUFD
>> automatically
> when it attaches to VFIO (usually referred as autodomains) but it has
> the needed handling for mdevs.
>
> To support dirty tracking with the advanced IOMMUFD API, it needs
> similar logic, where IOMMU domains are created and devices attached
>> to
> compatible domains. Essentially mimmicing kernel
> iommufd_device_auto_get_domain(). With mdevs given there's no
>> IOMMU domain
> it falls back to IOAS attach.
>
> The auto domain logic allows different IOMMU domains to be created
>> when
> DMA dirty tracking is not desired (and VF can provide it), and others
>> where
> it is. Here is not used in this way here given how VFIODevice migration

 Here is not used in this way here ?

>>>
>>> I meant, 'Here it is not used in this way given (...)'
>>>
> state is initialized after the device attachment. But such mixed mode of
> IOMMU dirty tracking + device dirty tracking is an improvement that
>> can
> be added on. Keep the 'all of nothing' of type1 approach that we have
> been using so far between container vs device dirty tracking.
>
> Signed-off-by: Joao Martins 
> ---
>  include/hw/vfio/vfio-common.h |  9 
>  include/sysemu/iommufd.h  |  5 +++
>  backends/iommufd.c| 30 +
>  hw/vfio/iommufd.c | 82
>> +++
>  backends/trace-events |  1 +
>  5 files changed, 127 insertions(+)
>
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-
>> common.h
> index 7419466bca92..2dd468ce3c02 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -95,10 +95,17 @@ typedef struct VFIOHostDMAWindow {
>
>  typedef struct IOMMUFDBackend IOMMUFDBackend;
>
> +typedef struct VFIOIOASHwpt {
> +uint32_t hwpt_id;
> +QLIST_HEAD(, VFIODevice) device_list;
> +QLIST_ENTRY(VFIOIOASHwpt) next;
> +} VFIOIOASHwpt;
> +
>  typedef struct VFIOIOMMUFDContainer {
>  VFIOContainerBase bcontainer;
>  IOMMUFDBackend *be;
>  uint32_t ioas_id;
> +QLIST_HEAD(, VFIOIOASHwpt) hwpt_list;
>  } VFIOIOMMUFDContainer;
>
>  OBJECT_DECLARE_SIMPLE_TYPE(VFIOIOMMUFDContainer,
>> VFIO_IOMMU_IOMMUFD);
> @@ -135,6 +142,8 @@ typedef 

Re: [PATCH v4 09/12] vfio/iommufd: Implement VFIOIOMMUClass::set_dirty_tracking support

[Joao Martins]

On 17/07/2024 03:24, Duan, Zhenzhong wrote:
> 
> 
>> -Original Message-
>> From: Joao Martins 
>> Subject: [PATCH v4 09/12] vfio/iommufd: Implement
>> VFIOIOMMUClass::set_dirty_tracking support
>>
>> ioctl(iommufd, IOMMU_HWPT_SET_DIRTY_TRACKING, arg) is the UAPI that
>> enables or disables dirty page tracking. It is used if the hwpt
>> has been created with dirty tracking supported domain (stored in
>> hwpt::flags) and it is called on the whole list of iommu domains
>> it is are tracking. On failure it rolls it back.
>>
>> The checking of hwpt::flags is introduced here as a second user
>> and thus consolidate such check into a helper function
>> iommufd_hwpt_dirty_tracking().
>>
>> Signed-off-by: Joao Martins 
>> ---
>> include/sysemu/iommufd.h |  3 +++
>> backends/iommufd.c   | 23 +++
>> hw/vfio/iommufd.c| 39
>> ++-
>> backends/trace-events|  1 +
>> 4 files changed, 65 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/sysemu/iommufd.h b/include/sysemu/iommufd.h
>> index e917e7591d05..7416d9219703 100644
>> --- a/include/sysemu/iommufd.h
>> +++ b/include/sysemu/iommufd.h
>> @@ -55,6 +55,9 @@ bool
>> iommufd_backend_alloc_hwpt(IOMMUFDBackend *be, uint32_t dev_id,
>> uint32_t data_type, uint32_t data_len,
>> void *data_ptr, uint32_t *out_hwpt,
>> Error **errp);
>> +bool iommufd_backend_set_dirty_tracking(IOMMUFDBackend *be,
>> uint32_t hwpt_id,
>> +bool start, Error **errp);
>>
>> #define TYPE_HOST_IOMMU_DEVICE_IOMMUFD
>> TYPE_HOST_IOMMU_DEVICE "-iommufd"
>> +
>> #endif
>> diff --git a/backends/iommufd.c b/backends/iommufd.c
>> index 41a9dec3b2c5..239f0976e0ad 100644
>> --- a/backends/iommufd.c
>> +++ b/backends/iommufd.c
>> @@ -239,6 +239,29 @@ bool
>> iommufd_backend_alloc_hwpt(IOMMUFDBackend *be, uint32_t dev_id,
>> return true;
>> }
>>
>> +bool iommufd_backend_set_dirty_tracking(IOMMUFDBackend *be,
>> +uint32_t hwpt_id, bool start,
>> +Error **errp)
>> +{
>> +int ret;
>> +struct iommu_hwpt_set_dirty_tracking set_dirty = {
>> +.size = sizeof(set_dirty),
>> +.hwpt_id = hwpt_id,
>> +.flags = !start ? 0 : IOMMU_HWPT_DIRTY_TRACKING_ENABLE,
>> +};
>> +
>> +ret = ioctl(be->fd, IOMMU_HWPT_SET_DIRTY_TRACKING, &set_dirty);
>> +trace_iommufd_backend_set_dirty(be->fd, hwpt_id, start, ret ? errno :
>> 0);
>> +if (ret) {
>> +error_setg_errno(errp, errno,
>> + "IOMMU_HWPT_SET_DIRTY_TRACKING(hwpt_id %u) failed",
>> + hwpt_id);
>> +return false;
>> +}
>> +
>> +return true;
>> +}
>> +
>> bool iommufd_backend_get_device_info(IOMMUFDBackend *be, uint32_t
>> devid,
>>  uint32_t *type, void *data, uint32_t 
>> len,
>>  uint64_t *caps, Error **errp)
>> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
>> index edc8f97d8f3d..da678315faeb 100644
>> --- a/hw/vfio/iommufd.c
>> +++ b/hw/vfio/iommufd.c
>> @@ -110,6 +110,42 @@ static void
>> iommufd_cdev_unbind_and_disconnect(VFIODevice *vbasedev)
>> iommufd_backend_disconnect(vbasedev->iommufd);
>> }
>>
>> +static bool iommufd_hwpt_dirty_tracking(VFIOIOASHwpt *hwpt)
>> +{
>> +return hwpt->hwpt_flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING;
>> +}
>> +
>> +static int iommufd_set_dirty_page_tracking(const VFIOContainerBase
>> *bcontainer,
>> +   bool start, Error **errp)
>> +{
>> +const VFIOIOMMUFDContainer *container =
>> +container_of(bcontainer, VFIOIOMMUFDContainer, bcontainer);
>> +VFIOIOASHwpt *hwpt;
>> +
>> +QLIST_FOREACH(hwpt, &container->hwpt_list, next) {
>> +if (!iommufd_hwpt_dirty_tracking(hwpt)) {
>> +continue;
>> +}
> 
> So the devices under an hwpt that doesn't support dirty tracking are bypassed.
> Then how to track dirty pages coming from those devices?
> 

We don't support 'mixed mode' dirty tracking right now even before this series.
I plan on lifting that restriction as a follow up. So far I was thinking that to
make sure migration is blocked if neither VF nor IOMMU VF dirty tracking are
supported.

The reason is that the migration initialization of the VFIODevice needs to be
adjusted to be able to understand all the constraints that the IOMMU dirty
tracking is not requested when VF dirty tracking is in use, and vice-versa. Thus
making this check a lot more representative of the features it is using.

Re: [PATCH v3] osdep: add a qemu_close_all_open_fd() helper

[Clément Léger]

On 16/07/2024 16:46, Philippe Mathieu-Daudé wrote:
> Hi Clément,
> 
> On 16/7/24 16:39, Clément Léger wrote:
>> Since commit 03e471c41d8b ("qemu_init: increase NOFILE soft limit on
>> POSIX"), the maximum number of file descriptors that can be opened are
>> raised to nofile.rlim_max. On recent debian distro, this yield a maximum
>> of 1073741816 file descriptors. Now, when forking to start
>> qemu-bridge-helper, this actually calls close() on the full possible file
>> descriptor range (more precisely [3 - sysconf(_SC_OPEN_MAX)]) which
>> takes a considerable amount of time. In order to reduce that time,
>> factorize existing code to close all open files descriptors in a new
>> qemu_close_all_open_fd() function. This function uses various methods
>> to close all the open file descriptors ranging from the most efficient
>> one to the least one. It also accepts an ordered array of file
>> descriptors that should not be closed since this is required by the
>> callers that calls it after forking.
>>
>> Signed-off-by: Clément Léger 
>>
>> 
>>
>> v3:
>>   - Use STD*_FILENO defines instead of raw values
>>   - Fix indentation of close_all_fds_after_fork()
>>   - Check for nksip in fallback code
>>   - Check for path starting with a '.' in qemu_close_all_open_fd_proc()
>>   - Use unsigned for cur_skip
>>   - Move ifdefs inside close_fds functions rather than redefining them
>>   - Remove uneeded 'if(nskip)' test
>>   - Add comments to close_range version
>>   - Reduce range of skip fd as we find them in
>>   - v2:
>> https://lore.kernel.org/qemu-devel/20240618111704.63092-1-cle...@rivosinc.com/
>>
>> v2:
>>   - Factorize async_teardown.c close_fds implementation as well as
>> tap.c ones
>>   - Apply checkpatch
>>   - v1:
>> https://lore.kernel.org/qemu-devel/20240617162520.4045016-1-cle...@rivosinc.com/
>>
>> ---
>>   include/qemu/osdep.h    |   8 +++
>>   net/tap.c   |  33 +-
>>   system/async-teardown.c |  37 +---
>>   util/osdep.c    | 129 
>>   4 files changed, 157 insertions(+), 50 deletions(-)
>>
>> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
>> index 191916f38e..43a035d756 100644
>> --- a/include/qemu/osdep.h
>> +++ b/include/qemu/osdep.h
>> @@ -757,6 +757,14 @@ static inline void qemu_reset_optind(void)
>>     int qemu_fdatasync(int fd);
>>   +/**
>> + * Close all open file descriptors except the ones supplied in the
>> @skip array
>> + *
>> + * @skip: ordered array of distinct file descriptors that should not
>> be closed
>> + * @nskip: number of entries in the @skip array.
>> + */
> 
> Can we mention @skip is optional, and @nskip can be 0?
> 
> Also I'd assert(skip != NULL || nskip == 0).

Yes sure, I'll add that.

Thanks,

Clément

Re: [PATCH v3] osdep: add a qemu_close_all_open_fd() helper

[Clément Léger]

On 16/07/2024 17:52, Richard Henderson wrote:
> On 7/17/24 00:39, Clément Léger wrote:
>> +    /* Restrict the range as we found fds matching
>> start/end */
>> +    if (i == skip_start)
>> +    skip_start++;
>> +    else if (i == skip_end)
>> +    skip_end--;
> 
> Need braces.

Sorry for that, I'll run checkpatch before sending v4.

Thanks,

Clément

> 
> Otherwise,
> Reviewed-by: Richard Henderson 
> 
> 
> 
> r~

Re: [PATCH 04/11] tests/functional: Add python-based tests to the meson build system

[Thomas Huth]

On 16/07/2024 17.15, Fabiano Rosas wrote:

Thomas Huth  writes:


Integrate the new python-based test framework with the meson build
system. Since these tests now require the pycotap module, make
sure that it gets installed in the venv.

The changes to the meson.build files are partly based on an earlier
patch by Ani Sinha (but heavily modified by Thomas Huth e.g. to use
pycotap for running the tests instead).

Signed-off-by: Thomas Huth 


Just one touch-up below.

Reviewed-by: Fabiano Rosas 


---
  pythondeps.toml  |  3 +-
  tests/Makefile.include   | 18 -
  tests/functional/meson.build | 75 
  tests/meson.build|  1 +
  4 files changed, 95 insertions(+), 2 deletions(-)
  create mode 100644 tests/functional/meson.build

diff --git a/pythondeps.toml b/pythondeps.toml
index f6e590fdd8..c018b4d74a 100644
--- a/pythondeps.toml
+++ b/pythondeps.toml
@@ -26,9 +26,10 @@ meson = { accepted = ">=1.1.0", installed = "1.2.3", canary = 
"meson" }
  sphinx = { accepted = ">=3.4.3", installed = "5.3.0", canary = "sphinx-build" 
}
  sphinx_rtd_theme = { accepted = ">=0.5", installed = "1.1.1" }
  
-[avocado]

+[tests]
  # Note that qemu.git/python/ is always implicitly installed.
  # Prefer an LTS version when updating the accepted versions of
  # avocado-framework, for example right now the limit is 92.x.
  avocado-framework = { accepted = "(>=88.1, <93.0)", installed = "88.1", canary = 
"avocado" }
  pycdlib = { accepted = ">=1.11.0" }
+pycotap = { accepted = ">=1.1.0" }
diff --git a/tests/Makefile.include b/tests/Makefile.include
index d39d5dd6a4..2bdf607977 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -9,6 +9,8 @@ check-help:
@echo "Individual test suites:"
@echo " $(MAKE) check-qtest-TARGET Run qtest tests for given target"
@echo " $(MAKE) check-qtestRun qtest tests"
+   @echo " $(MAKE) check-functional   Run python-based functional 
tests"
+   @echo " $(MAKE) check-functional-TARG  Run functional tests for
a given target"
@echo " $(MAKE) check-unit Run qobject tests"
@echo " $(MAKE) check-qapi-schema  Run QAPI schema tests"
@echo " $(MAKE) check-blockRun block tests"
@@ -111,7 +113,7 @@ quiet-venv-pip = $(quiet-@)$(call quiet-command-run, \
  
  $(TESTS_VENV_TOKEN): $(SRC_PATH)/pythondeps.toml

$(call quiet-venv-pip,install -e "$(SRC_PATH)/python/")
-   $(MKVENV_ENSUREGROUP) $< avocado
+   $(MKVENV_ENSUREGROUP) $< tests
$(call quiet-command, touch $@)
  
  $(TESTS_RESULTS_DIR):

@@ -152,6 +154,20 @@ check-acceptance-deprecated-warning:
  
  check-acceptance: check-acceptance-deprecated-warning | check-avocado
  
+# Make sure that pycotap is installed before running any functional tests:

+ifneq ($(filter check-func%,$(MAKECMDGOALS))$(filter check,$(MAKECMDGOALS)),)
+do-meson-check: check-venv
+endif
+
+FUNCTIONAL_TARGETS=$(patsubst %-softmmu,check-functional-%, $(filter 
%-softmmu,$(TARGETS)))
+.PHONY: $(FUNCTIONAL_TARGETS)
+$(FUNCTIONAL_TARGETS):
+   @make SPEED=thorough $(subst -functional,-func,$@)
+
+.PHONY: check-functional
+check-functional:
+   @make SPEED=thorough check-func check-func-quick


I think these^ two should use $(MAKE) instead:

make[1]: warning: jobserver unavailable: using -j1.  Add '+' to parent
make rule.


Right, thanks! I'll change it in the next version.

 Thomas

Re: [PATCH v2 0/4] Reconstruct loongson ipi driver

[maobibo]

On 2024/7/16 下午2:40, Philippe Mathieu-Daudé wrote:

On 16/7/24 03:29, maobibo wrote:



On 2024/7/16 上午9:04, maobibo wrote:



On 2024/7/15 下午11:17, Philippe Mathieu-Daudé wrote:

On 4/7/24 05:37, Bibo Mao wrote:

Now loongson ipi and loongarch ipi share the same code with different
macro, loongson ipi has its separate function such mmio region,
loongarch ipi has other requirement such as irqchip in kernel.

Interrupt irqchip has strong relationship with architecture, since
it sends irq to vcpu and interfaces to get irqchip register is also
architecture specific.

Here like other architectures, base class TYPE_LOONGSON_IPI_COMMON
is added, it comes from loongson ipi mostly. And it defined four 
abstract
interfaces which can be used for MIPS 3A4000 and Loongarch 3A5000 
machine,

also can be used for 3A5000 irqchip in kernel mode soon.

Also Loongarch ipi and loongson ipi device are added here, it inherits
from base class TYPE_LOONGSON_IPI_COMMON. Loongarch ipi is tested,
loongson ipi device only passes to compile and make check, it is not
tested.

Bibo Mao (4):
   hw/intc/loongson_ipi_common: Add loongson ipi common class
   hw/intc/loongarch_ipi: Add loongarch ipi support
   hw/loongarch/virt: Replace loongson ipi with loongarch ipi
   hw/intc/loongson_ipi: reconstruct driver inherit from common class


I'll try to respin a clearer v3.
I am ok with it since it solve the problem, and it is suitable for 
9.1 release. Only that in the long time we hope that intc emulation 
driver has common base class + tcg/kvm driver, similar with other 
architecture.



Sorry for the confusion, I had thought it was another topic.

Thanks for pointing out the problem and welcome the v3 version.


Please do not post v3, let me post it.

Hi Philippe,

QEMU 9.1 is coming to soft frozen stage, do you have enough time working 
on it?  Is it ok to use bugfix patch for 9.1 release version?

https://lore.kernel.org/all/20240627125819.62779-2-phi...@linaro.org/

After 9.1 is released, there will be enough time for patch v3.

Regards
Bibo, Mao

Re: [PATCH v4 05/12] vfio/iommufd: Introduce auto domain creation

[Cédric Le Goater]

On 7/17/24 11:09, Joao Martins wrote:

On 17/07/2024 03:52, Duan, Zhenzhong wrote:




-Original Message-
From: Joao Martins 
Subject: Re: [PATCH v4 05/12] vfio/iommufd: Introduce auto domain
creation

On 16/07/2024 17:44, Joao Martins wrote:

On 16/07/2024 17:04, Eric Auger wrote:

Hi Joao,

On 7/12/24 13:46, Joao Martins wrote:

There's generally two modes of operation for IOMMUFD:

* The simple user API which intends to perform relatively simple things
with IOMMUs e.g. DPDK. It generally creates an IOAS and attach to VFIO


It generally creates? can you explicit what is "it"


'It' here refers to the process/API-user


I am confused by this automatic terminology again (not your fault). the

doc says:

"

   *

 Automatic domain - refers to an iommu domain created automatically
 when attaching a device to an IOAS object. This is compatible to the
 semantics of VFIO type1.

   *

 Manual domain - refers to an iommu domain designated by the user as
 the target pagetable to be attached to by a device. Though currently
 there are no uAPIs to directly create such domain, the datastructure
 and algorithms are ready for handling that use case.

"


in 1) the device is attached to the ioas id (using the auto domain if I am

not wrong)

Here you attach to an hwpt id. Isn't it a manual domain?



Correct.

The 'auto domains' generally refers to the kernel-equivalent own

automatic

attaching to a new pagetable.

Here I call 'auto domains' in the userspace version too because we are

doing the

exact same but from userspace, using the manual API in IOMMUFD.


and mainly performs IOAS_MAP and UNMAP.

* The native IOMMUFD API where you have fine grained control of the
IOMMU domain and model it accordingly. This is where most new

feature

are being steered to.

For dirty tracking 2) is required, as it needs to ensure that
the stage-2/parent IOMMU domain will only attach devices
that support dirty tracking (so far it is all homogeneous in x86, likely
not the case for smmuv3). Such invariant on dirty tracking provides a
useful guarantee to VMMs that will refuse incompatible device
attachments for IOMMU domains.

Dirty tracking insurance is enforced via HWPT_ALLOC, which is
responsible for creating an IOMMU domain. This is contrast to the
'simple API' where the IOMMU domain is created by IOMMUFD

automatically

when it attaches to VFIO (usually referred as autodomains) but it has
the needed handling for mdevs.

To support dirty tracking with the advanced IOMMUFD API, it needs
similar logic, where IOMMU domains are created and devices attached

to

compatible domains. Essentially mimmicing kernel
iommufd_device_auto_get_domain(). With mdevs given there's no

IOMMU domain

it falls back to IOAS attach.

The auto domain logic allows different IOMMU domains to be created

when

DMA dirty tracking is not desired (and VF can provide it), and others

where

it is. Here is not used in this way here given how VFIODevice migration


Here is not used in this way here ?



I meant, 'Here it is not used in this way given (...)'


state is initialized after the device attachment. But such mixed mode of
IOMMU dirty tracking + device dirty tracking is an improvement that

can

be added on. Keep the 'all of nothing' of type1 approach that we have
been using so far between container vs device dirty tracking.

Signed-off-by: Joao Martins 
---
  include/hw/vfio/vfio-common.h |  9 
  include/sysemu/iommufd.h  |  5 +++
  backends/iommufd.c| 30 +
  hw/vfio/iommufd.c | 82

+++

  backends/trace-events |  1 +
  5 files changed, 127 insertions(+)

diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-

common.h

index 7419466bca92..2dd468ce3c02 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -95,10 +95,17 @@ typedef struct VFIOHostDMAWindow {

  typedef struct IOMMUFDBackend IOMMUFDBackend;

+typedef struct VFIOIOASHwpt {
+uint32_t hwpt_id;
+QLIST_HEAD(, VFIODevice) device_list;
+QLIST_ENTRY(VFIOIOASHwpt) next;
+} VFIOIOASHwpt;
+
  typedef struct VFIOIOMMUFDContainer {
  VFIOContainerBase bcontainer;
  IOMMUFDBackend *be;
  uint32_t ioas_id;
+QLIST_HEAD(, VFIOIOASHwpt) hwpt_list;
  } VFIOIOMMUFDContainer;

  OBJECT_DECLARE_SIMPLE_TYPE(VFIOIOMMUFDContainer,

VFIO_IOMMU_IOMMUFD);

@@ -135,6 +142,8 @@ typedef struct VFIODevice {
  HostIOMMUDevice *hiod;
  int devid;
  IOMMUFDBackend *iommufd;
+VFIOIOASHwpt *hwpt;
+QLIST_ENTRY(VFIODevice) hwpt_next;
  } VFIODevice;

  struct VFIODeviceOps {
diff --git a/include/sysemu/iommufd.h b/include/sysemu/iommufd.h
index 57d502a1c79a..e917e7591d05 100644
--- a/include/sysemu/iommufd.h
+++ b/include/sysemu/iommufd.h
@@ -50,6 +50,11 @@ int

iommufd_backend_unmap_dma(IOMMUFDBackend *be, uint32_t ioas_id,

  bool iommufd_backend_get_device_info(IOMMUFDBackend *be,

uint32_t devid,

 

Re: [PATCH v4 05/12] vfio/iommufd: Introduce auto domain creation

[Joao Martins]

On 17/07/2024 10:28, Cédric Le Goater wrote:
>>> @@ -224,6 +300,11 @@ static void
 iommufd_cdev_detach_container(VFIODevice *vbasedev,
>>>   {
>>>   Error *err = NULL;
>>>
>>> +    if (vbasedev->hwpt) {
>>> +    iommufd_cdev_autodomains_put(vbasedev, container);
>>> +    return;
>> Where do we detach the device from the hwpt?
>>
> In iommufd_backend_free_id() for auto domains
>

 to clarify here I meant *userspace* auto domains

 *kernel* auto domains (mdev) goes via DETACH_IOMMUFD_PT
>>>
>>> If the device is still attached to the hwpt, will iommufd_backend_free_id()
>>> succeed?
>>> Have you tried the hot unplug?
>>>
>>
>> I have but I didn't see any errors. But I will check again for v5 as it could
>> also be my oversight.
>>
>> I was thinking about Eric's remark overnight and I think what I am doing is 
>> not
>> correct regardless of the above.
>>
>> I should be calling DETACH_IOMMUFD_PT pairing with ATTACH_IOMMUFD_PT, and the
>> iommufd_backend_free_id() is to drop the final reference pairing with
>> alloc_hwpt() when the device list is empty i.e. when there's no more devices 
>> in
>> that vdev::hwpt.
>>
>> DETACH_IOMMUFD_PT decrement the hwpt refcount and it doesn't differentiate
>> between auto domains vs manual domains.
>>
>> The code is already there anyhow it just has the order of
>> iommufd_cdev_autodomains_put vs detach invocation reversed; I'll fix that for
>> next version.
> 
> While at it, could you please move these routines :
> 
>   iommufd_cdev_detach_ioas_hwpt
>   iommufd_cdev_attach_ioas_hwpt
>  
> under backends/iommufd.c ? I think that's where they belong.

OK

Re: [PATCH v4 11/12] vfio/migration: Don't block migration device dirty tracking is unsupported

[Joao Martins]

On 17/07/2024 03:38, Duan, Zhenzhong wrote:
> 
> 
>> -Original Message-
>> From: Joao Martins 
>> Subject: [PATCH v4 11/12] vfio/migration: Don't block migration device dirty
>> tracking is unsupported
>>
>> By default VFIO migration is set to auto, which will support live
>> migration if the migration capability is set *and* also dirty page
>> tracking is supported.
>>
>> For testing purposes one can force enable without dirty page tracking
>> via enable-migration=on, but that option is generally left for testing
>> purposes.
>>
>> So starting with IOMMU dirty tracking it can use to acomodate the lack of
>> VF dirty page tracking allowing us to minimize the VF requirements for
>> migration and thus enabling migration by default for those too.
>>
>> Signed-off-by: Joao Martins 
>> ---
>> hw/vfio/migration.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
>> index 34d4be2ce1b1..ce3d1b6e9a25 100644
>> --- a/hw/vfio/migration.c
>> +++ b/hw/vfio/migration.c
>> @@ -1036,7 +1036,8 @@ bool vfio_migration_realize(VFIODevice
>> *vbasedev, Error **errp)
>> return !vfio_block_migration(vbasedev, err, errp);
>> }
>>
>> -if (!vbasedev->dirty_pages_supported) {
>> +if (!vbasedev->dirty_pages_supported &&
>> +!vbasedev->bcontainer->dirty_pages_supported) {
>> if (vbasedev->enable_migration == ON_OFF_AUTO_AUTO) {
>> error_setg(&err,
>>"%s: VFIO device doesn't support device dirty 
>> tracking",
> 
> I'm not sure if this message needs to be updated, " VFIO device doesn't 
> support device and IOMMU dirty tracking"
> 
> Same for the below:
> 
> warn_report("%s: VFIO device doesn't support device dirty tracking"


Ah yes, good catch. Additionally I think I should check device hwpt rather than
container::dirty_pages_supported i.e.

if (!vbasedev->dirty_pages_supported &&
(vbasedev->hwpt && !iommufd_hwpt_dirty_tracking(vbasedev->hwpt)))

This makes sure that migration is blocked with more accuracy

[PATCH] scripts/checkpatch: emit a warning if an imported file is touched

[Stefano Garzarella]

If a file imported from Linux is touched, emit a warning and suggest
using scripts/update-linux-headers.sh

Signed-off-by: Stefano Garzarella 
---
 scripts/checkpatch.pl | 14 --
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index ff373a7083..b0e8266fa2 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1374,6 +1374,7 @@ sub process {
my $in_header_lines = $file ? 0 : 1;
my $in_commit_log = 0;  #Scanning lines before patch
my $reported_maintainer_file = 0;
+   my $reported_imported_file = 0;
my $non_utf8_charset = 0;
 
our @report = ();
@@ -1673,8 +1674,17 @@ sub process {
 # ignore non-hunk lines and lines being removed
next if (!$hunk_line || $line =~ /^-/);
 
-# ignore files that are being periodically imported from Linux
-   next if ($realfile =~ 
/^(linux-headers|include\/standard-headers)\//);
+# ignore files that are being periodically imported from Linux and emit a 
warning
+   if ($realfile =~ 
/^(linux-headers|include\/standard-headers)\//) {
+   if (!$reported_imported_file) {
+   $reported_imported_file = 1;
+   WARN("added, moved or deleted file(s) " .
+"imported from Linux, are you using " .
+"scripts/update-linux-headers.sh?\n" .
+$herecurr);
+   }
+   next;
+   }
 
 #trailing whitespace
if ($line =~ /^\+.*\015/) {
-- 
2.45.2

RE: [PATCH v4 05/12] vfio/iommufd: Introduce auto domain creation

[Duan, Zhenzhong]

>-Original Message-
>From: Joao Martins 
>Subject: Re: [PATCH v4 05/12] vfio/iommufd: Introduce auto domain
>creation
>
>On 17/07/2024 03:52, Duan, Zhenzhong wrote:
>>
>>
>>> -Original Message-
>>> From: Joao Martins 
>>> Subject: Re: [PATCH v4 05/12] vfio/iommufd: Introduce auto domain
>>> creation
>>>
>>> On 16/07/2024 17:44, Joao Martins wrote:
 On 16/07/2024 17:04, Eric Auger wrote:
> Hi Joao,
>
> On 7/12/24 13:46, Joao Martins wrote:
>> There's generally two modes of operation for IOMMUFD:
>>
>> * The simple user API which intends to perform relatively simple
>things
>> with IOMMUs e.g. DPDK. It generally creates an IOAS and attach to
>VFIO
>
> It generally creates? can you explicit what is "it"
>
 'It' here refers to the process/API-user

> I am confused by this automatic terminology again (not your fault). the
>>> doc says:
> "
>
>   *
>
> Automatic domain - refers to an iommu domain created
>automatically
> when attaching a device to an IOAS object. This is compatible to the
> semantics of VFIO type1.
>
>   *
>
> Manual domain - refers to an iommu domain designated by the user
>as
> the target pagetable to be attached to by a device. Though currently
> there are no uAPIs to directly create such domain, the datastructure
> and algorithms are ready for handling that use case.
>
> "
>
>
> in 1) the device is attached to the ioas id (using the auto domain if I am
>>> not wrong)
> Here you attach to an hwpt id. Isn't it a manual domain?
>

 Correct.

 The 'auto domains' generally refers to the kernel-equivalent own
>>> automatic
 attaching to a new pagetable.

 Here I call 'auto domains' in the userspace version too because we are
>>> doing the
 exact same but from userspace, using the manual API in IOMMUFD.

>> and mainly performs IOAS_MAP and UNMAP.
>>
>> * The native IOMMUFD API where you have fine grained control of
>the
>> IOMMU domain and model it accordingly. This is where most new
>>> feature
>> are being steered to.
>>
>> For dirty tracking 2) is required, as it needs to ensure that
>> the stage-2/parent IOMMU domain will only attach devices
>> that support dirty tracking (so far it is all homogeneous in x86, likely
>> not the case for smmuv3). Such invariant on dirty tracking provides a
>> useful guarantee to VMMs that will refuse incompatible device
>> attachments for IOMMU domains.
>>
>> Dirty tracking insurance is enforced via HWPT_ALLOC, which is
>> responsible for creating an IOMMU domain. This is contrast to the
>> 'simple API' where the IOMMU domain is created by IOMMUFD
>>> automatically
>> when it attaches to VFIO (usually referred as autodomains) but it has
>> the needed handling for mdevs.
>>
>> To support dirty tracking with the advanced IOMMUFD API, it needs
>> similar logic, where IOMMU domains are created and devices
>attached
>>> to
>> compatible domains. Essentially mimmicing kernel
>> iommufd_device_auto_get_domain(). With mdevs given there's no
>>> IOMMU domain
>> it falls back to IOAS attach.
>>
>> The auto domain logic allows different IOMMU domains to be created
>>> when
>> DMA dirty tracking is not desired (and VF can provide it), and others
>>> where
>> it is. Here is not used in this way here given how VFIODevice
>migration
>
> Here is not used in this way here ?
>

 I meant, 'Here it is not used in this way given (...)'

>> state is initialized after the device attachment. But such mixed mode
>of
>> IOMMU dirty tracking + device dirty tracking is an improvement that
>>> can
>> be added on. Keep the 'all of nothing' of type1 approach that we have
>> been using so far between container vs device dirty tracking.
>>
>> Signed-off-by: Joao Martins 
>> ---
>>  include/hw/vfio/vfio-common.h |  9 
>>  include/sysemu/iommufd.h  |  5 +++
>>  backends/iommufd.c| 30 +
>>  hw/vfio/iommufd.c | 82
>>> +++
>>  backends/trace-events |  1 +
>>  5 files changed, 127 insertions(+)
>>
>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-
>>> common.h
>> index 7419466bca92..2dd468ce3c02 100644
>> --- a/include/hw/vfio/vfio-common.h
>> +++ b/include/hw/vfio/vfio-common.h
>> @@ -95,10 +95,17 @@ typedef struct VFIOHostDMAWindow {
>>
>>  typedef struct IOMMUFDBackend IOMMUFDBackend;
>>
>> +typedef struct VFIOIOASHwpt {
>> +uint32_t hwpt_id;
>> +QLIST_HEAD(, VFIODevice) device_list;
>> +QLIST_ENTRY(VFIOIOASHwpt) next;
>> +} VFIOIOASHwpt;
>> +
>>  typedef struct VFIOIOMMUFDContainer {
>>  VFIOContainerBase bconta

Re: [PATCH 01/17] target/arm: Use tcg_gen_extract2_i64 for EXT

[Philippe Mathieu-Daudé]

On 17/7/24 08:08, Richard Henderson wrote:

The extract2 tcg op performs the same operation
as the do_ext64 function.

Signed-off-by: Richard Henderson 
---
  target/arm/tcg/translate-a64.c | 23 +++
  1 file changed, 3 insertions(+), 20 deletions(-)


Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH] scripts/checkpatch: emit a warning if an imported file is touched

[Daniel P . Berrangé]

On Wed, Jul 17, 2024 at 11:37:52AM +0200, Stefano Garzarella wrote:
> If a file imported from Linux is touched, emit a warning and suggest
> using scripts/update-linux-headers.sh
> 
> Signed-off-by: Stefano Garzarella 
> ---
>  scripts/checkpatch.pl | 14 --
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index ff373a7083..b0e8266fa2 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1374,6 +1374,7 @@ sub process {
>   my $in_header_lines = $file ? 0 : 1;
>   my $in_commit_log = 0;  #Scanning lines before patch
>   my $reported_maintainer_file = 0;
> + my $reported_imported_file = 0;
>   my $non_utf8_charset = 0;
>  
>   our @report = ();
> @@ -1673,8 +1674,17 @@ sub process {
>  # ignore non-hunk lines and lines being removed
>   next if (!$hunk_line || $line =~ /^-/);
>  
> -# ignore files that are being periodically imported from Linux
> - next if ($realfile =~ 
> /^(linux-headers|include\/standard-headers)\//);
> +# ignore files that are being periodically imported from Linux and emit a 
> warning
> + if ($realfile =~ 
> /^(linux-headers|include\/standard-headers)\//) {
> + if (!$reported_imported_file) {
> + $reported_imported_file = 1;
> + WARN("added, moved or deleted file(s) " .
> +  "imported from Linux, are you using " .
> +  "scripts/update-linux-headers.sh?\n" .
> +  $herecurr);

This is a good hint, but can we add a further check that is a fatal error,
if the headers are changed in the same commit as non-header changes. When
importing headers, they should only ever be in a self-contained patch
with nothing else touched.

> + }
> + next;
> + }

With regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [PATCH v4 05/12] vfio/iommufd: Introduce auto domain creation

[Joao Martins]

On 17/07/2024 10:48, Duan, Zhenzhong wrote:
> 
> 
>> -Original Message-
>> From: Joao Martins 
>> Subject: Re: [PATCH v4 05/12] vfio/iommufd: Introduce auto domain
>> creation
>>
>> On 17/07/2024 03:52, Duan, Zhenzhong wrote:
>>>
>>>
 -Original Message-
 From: Joao Martins 
 Subject: Re: [PATCH v4 05/12] vfio/iommufd: Introduce auto domain
 creation

 On 16/07/2024 17:44, Joao Martins wrote:
> On 16/07/2024 17:04, Eric Auger wrote:
>> Hi Joao,
>>
>> On 7/12/24 13:46, Joao Martins wrote:
>>> There's generally two modes of operation for IOMMUFD:
>>>
>>> * The simple user API which intends to perform relatively simple
>> things
>>> with IOMMUs e.g. DPDK. It generally creates an IOAS and attach to
>> VFIO
>>
>> It generally creates? can you explicit what is "it"
>>
> 'It' here refers to the process/API-user
>
>> I am confused by this automatic terminology again (not your fault). the
 doc says:
>> "
>>
>>   *
>>
>> Automatic domain - refers to an iommu domain created
>> automatically
>> when attaching a device to an IOAS object. This is compatible to the
>> semantics of VFIO type1.
>>
>>   *
>>
>> Manual domain - refers to an iommu domain designated by the user
>> as
>> the target pagetable to be attached to by a device. Though currently
>> there are no uAPIs to directly create such domain, the datastructure
>> and algorithms are ready for handling that use case.
>>
>> "
>>
>>
>> in 1) the device is attached to the ioas id (using the auto domain if I 
>> am
 not wrong)
>> Here you attach to an hwpt id. Isn't it a manual domain?
>>
>
> Correct.
>
> The 'auto domains' generally refers to the kernel-equivalent own
 automatic
> attaching to a new pagetable.
>
> Here I call 'auto domains' in the userspace version too because we are
 doing the
> exact same but from userspace, using the manual API in IOMMUFD.
>
>>> and mainly performs IOAS_MAP and UNMAP.
>>>
>>> * The native IOMMUFD API where you have fine grained control of
>> the
>>> IOMMU domain and model it accordingly. This is where most new
 feature
>>> are being steered to.
>>>
>>> For dirty tracking 2) is required, as it needs to ensure that
>>> the stage-2/parent IOMMU domain will only attach devices
>>> that support dirty tracking (so far it is all homogeneous in x86, likely
>>> not the case for smmuv3). Such invariant on dirty tracking provides a
>>> useful guarantee to VMMs that will refuse incompatible device
>>> attachments for IOMMU domains.
>>>
>>> Dirty tracking insurance is enforced via HWPT_ALLOC, which is
>>> responsible for creating an IOMMU domain. This is contrast to the
>>> 'simple API' where the IOMMU domain is created by IOMMUFD
 automatically
>>> when it attaches to VFIO (usually referred as autodomains) but it has
>>> the needed handling for mdevs.
>>>
>>> To support dirty tracking with the advanced IOMMUFD API, it needs
>>> similar logic, where IOMMU domains are created and devices
>> attached
 to
>>> compatible domains. Essentially mimmicing kernel
>>> iommufd_device_auto_get_domain(). With mdevs given there's no
 IOMMU domain
>>> it falls back to IOAS attach.
>>>
>>> The auto domain logic allows different IOMMU domains to be created
 when
>>> DMA dirty tracking is not desired (and VF can provide it), and others
 where
>>> it is. Here is not used in this way here given how VFIODevice
>> migration
>>
>> Here is not used in this way here ?
>>
>
> I meant, 'Here it is not used in this way given (...)'
>
>>> state is initialized after the device attachment. But such mixed mode
>> of
>>> IOMMU dirty tracking + device dirty tracking is an improvement that
 can
>>> be added on. Keep the 'all of nothing' of type1 approach that we have
>>> been using so far between container vs device dirty tracking.
>>>
>>> Signed-off-by: Joao Martins 
>>> ---
>>>  include/hw/vfio/vfio-common.h |  9 
>>>  include/sysemu/iommufd.h  |  5 +++
>>>  backends/iommufd.c| 30 +
>>>  hw/vfio/iommufd.c | 82
 +++
>>>  backends/trace-events |  1 +
>>>  5 files changed, 127 insertions(+)
>>>
>>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-
 common.h
>>> index 7419466bca92..2dd468ce3c02 100644
>>> --- a/include/hw/vfio/vfio-common.h
>>> +++ b/include/hw/vfio/vfio-common.h
>>> @@ -95,10 +95,17 @@ typedef struct VFIOHostDMAWindow {
>>>
>>>  typedef struct IOMMUFDBackend IOMMUFDBackend;
>>>
>>> +typedef struct VFIOIOASHwpt {
>>> +uint32_t hwpt_id;
>>>

Re: [PATCH 02/17] target/arm: Convert EXT to decodetree

[Philippe Mathieu-Daudé]

On 17/7/24 08:08, Richard Henderson wrote:

Signed-off-by: Richard Henderson 
---
  target/arm/tcg/translate-a64.c | 121 +
  target/arm/tcg/a64.decode  |   5 ++
  2 files changed, 53 insertions(+), 73 deletions(-)


Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH 03/17] target/arm: Convert TBL, TBX to decodetree

[Philippe Mathieu-Daudé]

On 17/7/24 08:08, Richard Henderson wrote:

Signed-off-by: Richard Henderson 
---
  target/arm/tcg/translate-a64.c | 47 ++
  target/arm/tcg/a64.decode  |  4 +++
  2 files changed, 18 insertions(+), 33 deletions(-)


Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH] scripts/checkpatch: emit a warning if an imported file is touched

[Cornelia Huck]

On Wed, Jul 17 2024, Stefano Garzarella  wrote:

> If a file imported from Linux is touched, emit a warning and suggest
> using scripts/update-linux-headers.sh
>
> Signed-off-by: Stefano Garzarella 
> ---
>  scripts/checkpatch.pl | 14 --
>  1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index ff373a7083..b0e8266fa2 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1374,6 +1374,7 @@ sub process {
>   my $in_header_lines = $file ? 0 : 1;
>   my $in_commit_log = 0;  #Scanning lines before patch
>   my $reported_maintainer_file = 0;
> + my $reported_imported_file = 0;
>   my $non_utf8_charset = 0;
>  
>   our @report = ();
> @@ -1673,8 +1674,17 @@ sub process {
>  # ignore non-hunk lines and lines being removed
>   next if (!$hunk_line || $line =~ /^-/);
>  
> -# ignore files that are being periodically imported from Linux
> - next if ($realfile =~ 
> /^(linux-headers|include\/standard-headers)\//);
> +# ignore files that are being periodically imported from Linux and emit a 
> warning
> + if ($realfile =~ 
> /^(linux-headers|include\/standard-headers)\//) {
> + if (!$reported_imported_file) {
> + $reported_imported_file = 1;
> + WARN("added, moved or deleted file(s) " .
> +  "imported from Linux, are you using " .
> +  "scripts/update-linux-headers.sh?\n" .
> +  $herecurr);
> + }
> + next;
> + }

Thanks, that looks useful -- just two comments (sorry, my perl-fu is
low):
- Is there a way to check that this is a proper linux headers update?
  We'd have to rely on heuristics, but OTOH, we also usually want a
  headers update to use a certain format ($SUBJECT containing "headers
  update", patch description pointing to the version this update was
  done against.) Not sure if it is worth actually trying to figure this
  out.
- A common issue is headers changes mixed in with other code changes,
  which should not happen -- can we check for that as well and advise
  to either do a headers update, or use a placeholder patch?

>  
>  #trailing whitespace
>   if ($line =~ /^\+.*\015/) {

Re: [PATCH 08/17] target/arm: Convert FMOVI (scalar, immediate) to decodetree

[Philippe Mathieu-Daudé]

On 17/7/24 08:08, Richard Henderson wrote:

Signed-off-by: Richard Henderson 
---
  target/arm/tcg/translate-a64.c | 74 --
  target/arm/tcg/a64.decode  |  4 ++
  2 files changed, 30 insertions(+), 48 deletions(-)


Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH 11/17] target/arm: Fix whitespace near gen_srshr64_i64

[Philippe Mathieu-Daudé]

On 17/7/24 08:08, Richard Henderson wrote:

Signed-off-by: Richard Henderson 
---
  target/arm/tcg/gengvec.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)


Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH 17/17] target/arm: Push tcg_rnd into handle_shri_with_rndacc

[Philippe Mathieu-Daudé]

On 17/7/24 08:09, Richard Henderson wrote:

We always pass the same value for round; compute it
within common code.

Signed-off-by: Richard Henderson 
---
  target/arm/tcg/translate-a64.c | 32 ++--
  1 file changed, 6 insertions(+), 26 deletions(-)


Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v2 2/8] aspeed: Load eMMC first boot area as a boot rom

[Philippe Mathieu-Daudé]

On 17/7/24 08:30, Cédric Le Goater wrote:

From: Cédric Le Goater 

The first boot area partition (64K) of the eMMC device should contain
an initial boot loader (u-boot SPL). Load it as a ROM only if an eMMC
device is available to boot from but no flash device is.

Signed-off-by: Cédric Le Goater 
Reviewed-by: Andrew Jeffery 
Tested-by: Andrew Jeffery 
---
  hw/arm/aspeed.c | 8 +---
  1 file changed, 5 insertions(+), 3 deletions(-)


Reviewed-by: Philippe Mathieu-Daudé 

RE: [PATCH v4 05/12] vfio/iommufd: Introduce auto domain creation

[Duan, Zhenzhong]

>-Original Message-
>From: Joao Martins 
>Subject: Re: [PATCH v4 05/12] vfio/iommufd: Introduce auto domain
>creation
>
>On 17/07/2024 03:18, Duan, Zhenzhong wrote:
>>
>>
>>> -Original Message-
>>> From: Joao Martins 
>>> Subject: [PATCH v4 05/12] vfio/iommufd: Introduce auto domain
>creation
>>>
>>> There's generally two modes of operation for IOMMUFD:
>>>
>>> * The simple user API which intends to perform relatively simple things
>>> with IOMMUs e.g. DPDK. It generally creates an IOAS and attach to VFIO
>>> and mainly performs IOAS_MAP and UNMAP.
>>>
>>> * The native IOMMUFD API where you have fine grained control of the
>>> IOMMU domain and model it accordingly. This is where most new feature
>>> are being steered to.
>>>
>>> For dirty tracking 2) is required, as it needs to ensure that
>>> the stage-2/parent IOMMU domain will only attach devices
>>> that support dirty tracking (so far it is all homogeneous in x86, likely
>>> not the case for smmuv3). Such invariant on dirty tracking provides a
>>> useful guarantee to VMMs that will refuse incompatible device
>>> attachments for IOMMU domains.
>>>
>>> Dirty tracking insurance is enforced via HWPT_ALLOC, which is
>>> responsible for creating an IOMMU domain. This is contrast to the
>>> 'simple API' where the IOMMU domain is created by IOMMUFD
>>> automatically
>>> when it attaches to VFIO (usually referred as autodomains) but it has
>>> the needed handling for mdevs.
>>>
>>> To support dirty tracking with the advanced IOMMUFD API, it needs
>>> similar logic, where IOMMU domains are created and devices attached to
>>> compatible domains. Essentially mimmicing kernel
>>> iommufd_device_auto_get_domain(). With mdevs given there's no
>IOMMU
>>> domain
>>> it falls back to IOAS attach.
>>>
>>> The auto domain logic allows different IOMMU domains to be created
>when
>>> DMA dirty tracking is not desired (and VF can provide it), and others
>where
>>> it is. Here is not used in this way here given how VFIODevice migration
>>> state is initialized after the device attachment. But such mixed mode of
>>> IOMMU dirty tracking + device dirty tracking is an improvement that can
>>> be added on. Keep the 'all of nothing' of type1 approach that we have
>>> been using so far between container vs device dirty tracking.
>>>
>>> Signed-off-by: Joao Martins 
>>> ---
>>> include/hw/vfio/vfio-common.h |  9 
>>> include/sysemu/iommufd.h  |  5 +++
>>> backends/iommufd.c| 30 +
>>> hw/vfio/iommufd.c | 82
>>> +++
>>> backends/trace-events |  1 +
>>> 5 files changed, 127 insertions(+)
>>>
>>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-
>>> common.h
>>> index 7419466bca92..2dd468ce3c02 100644
>>> --- a/include/hw/vfio/vfio-common.h
>>> +++ b/include/hw/vfio/vfio-common.h
>>> @@ -95,10 +95,17 @@ typedef struct VFIOHostDMAWindow {
>>>
>>> typedef struct IOMMUFDBackend IOMMUFDBackend;
>>>
>>> +typedef struct VFIOIOASHwpt {
>>> +uint32_t hwpt_id;
>>> +QLIST_HEAD(, VFIODevice) device_list;
>>> +QLIST_ENTRY(VFIOIOASHwpt) next;
>>> +} VFIOIOASHwpt;
>>> +
>>> typedef struct VFIOIOMMUFDContainer {
>>> VFIOContainerBase bcontainer;
>>> IOMMUFDBackend *be;
>>> uint32_t ioas_id;
>>> +QLIST_HEAD(, VFIOIOASHwpt) hwpt_list;
>>> } VFIOIOMMUFDContainer;
>>>
>>> OBJECT_DECLARE_SIMPLE_TYPE(VFIOIOMMUFDContainer,
>>> VFIO_IOMMU_IOMMUFD);
>>> @@ -135,6 +142,8 @@ typedef struct VFIODevice {
>>> HostIOMMUDevice *hiod;
>>> int devid;
>>> IOMMUFDBackend *iommufd;
>>> +VFIOIOASHwpt *hwpt;
>>> +QLIST_ENTRY(VFIODevice) hwpt_next;
>>> } VFIODevice;
>>>
>>> struct VFIODeviceOps {
>>> diff --git a/include/sysemu/iommufd.h b/include/sysemu/iommufd.h
>>> index 57d502a1c79a..e917e7591d05 100644
>>> --- a/include/sysemu/iommufd.h
>>> +++ b/include/sysemu/iommufd.h
>>> @@ -50,6 +50,11 @@ int
>>> iommufd_backend_unmap_dma(IOMMUFDBackend *be, uint32_t
>ioas_id,
>>> bool iommufd_backend_get_device_info(IOMMUFDBackend *be,
>uint32_t
>>> devid,
>>>  uint32_t *type, void *data, uint32_t 
>>> len,
>>>  uint64_t *caps, Error **errp);
>>> +bool iommufd_backend_alloc_hwpt(IOMMUFDBackend *be, uint32_t
>>> dev_id,
>>> +uint32_t pt_id, uint32_t flags,
>>> +uint32_t data_type, uint32_t data_len,
>>> +void *data_ptr, uint32_t *out_hwpt,
>>> +Error **errp);
>>>
>>> #define TYPE_HOST_IOMMU_DEVICE_IOMMUFD
>>> TYPE_HOST_IOMMU_DEVICE "-iommufd"
>>> #endif
>>> diff --git a/backends/iommufd.c b/backends/iommufd.c
>>> index 2b3d51af26d2..5d3dfa917415 100644
>>> --- a/backends/iommufd.c
>>> +++ b/backends/iommufd.c
>>> @@ -208,6 +208,36 @@ int
>>> iommufd_backend_unmap_dma(IOMMUFDBackend *be, uint32_t
>ioas_id,
>>> return ret;
>>> }
>>>
>>> +bool iommufd_backend_alloc_hwpt(

Re: [PATCH v2 0/8] aspeed: Add boot from eMMC support (AST2600)

[Philippe Mathieu-Daudé]

On 17/7/24 08:30, Cédric Le Goater wrote:


Cédric Le Goater (8):
   aspeed: Change type of eMMC device
   aspeed: Load eMMC first boot area as a boot rom
   aspeed/scu: Add boot-from-eMMC HW strapping bit for AST2600 SoC
   aspeed: Introduce a AspeedSoCClass 'boot_from_emmc' handler
   aspeed: Tune eMMC device properties to reflect HW strapping
   aspeed: Add boot-from-eMMC HW strapping bit to rainier-bmc machine
   aspeed: Introduce a 'hw_strap1' machine attribute
   aspeed: Introduce a 'boot-emmc' machine option


Series:
Tested-by: Philippe Mathieu-Daudé 

Re: [PATCH v2 5/8] aspeed: Tune eMMC device properties to reflect HW strapping

[Philippe Mathieu-Daudé]

On 17/7/24 08:30, Cédric Le Goater wrote:

From: Cédric Le Goater 

When the boot-from-eMMC HW strapping bit is set, use the 'boot-config'
property to set the boot config register to boot from the first boot
area partition of the eMMC device. Also set the boot partition size
of the device.

Signed-off-by: Cédric Le Goater 
Reviewed-by: Andrew Jeffery 
Tested-by: Andrew Jeffery 
---
  hw/arm/aspeed.c | 17 +
  1 file changed, 13 insertions(+), 4 deletions(-)


Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v2] pci-bridge: avoid linking a single downstream port more than once

[Philippe Mathieu-Daudé]

Hi Yao,

On 17/7/24 10:56, Yao Xingtao via wrote:

Since the downstream port is not checked, two slots can be linked to
a single port. However, this can prevent the driver from detecting the
device properly.

It is necessary to ensure that a downstream port is not linked more than
once.

Links: 
https://lore.kernel.org/qemu-devel/oszpr01mb6453bc61d2ff4035f18084ef8d...@oszpr01mb6453.jpnprd01.prod.outlook.com
Signed-off-by: Yao Xingtao 

---
V1[1] -> V2:
  - Move downstream port check forward

[1] 
https://lore.kernel.org/qemu-devel/20240704033834.3362-1-yaoxt.f...@fujitsu.com
---
  hw/pci-bridge/cxl_downstream.c | 5 +
  hw/pci-bridge/pcie_root_port.c | 5 +
  hw/pci-bridge/xio3130_downstream.c | 5 +
  3 files changed, 15 insertions(+)

diff --git a/hw/pci-bridge/cxl_downstream.c b/hw/pci-bridge/cxl_downstream.c
index 742da07a015a..af81ddfeec13 100644
--- a/hw/pci-bridge/cxl_downstream.c
+++ b/hw/pci-bridge/cxl_downstream.c
@@ -142,6 +142,11 @@ static void cxl_dsp_realize(PCIDevice *d, Error **errp)
  MemoryRegion *component_bar = &cregs->component_registers;
  int rc;
  
+if (pcie_find_port_by_pn(pci_get_bus(d), p->port) != NULL) {

+error_setg(errp, "Can't link port, error %d", -EBUSY);
+return;


Could pcie_cap_slot_init() be a good place to check for that?

Otherwise IMHO we should add a helper in "hw/pci/pcie.h" and
call it here, not duplicate this code in each model.


+}
+
  pci_bridge_initfn(d, TYPE_PCIE_BUS);
  pcie_port_init_reg(d);

Re: [PATCH v2 0/2] system: Enable the device aliases for or1k, too

[Philippe Mathieu-Daudé]

Philippe Mathieu-Daudé (1):
   system: Sort QEMU_ARCH_VIRTIO_PCI definition

Thomas Huth (1):
   system: Enable the device aliases for or1k, too


Thanks, series queued.

Re: [PATCH v2] qga/commands-posix: Make ga_wait_child() return boolean

[Philippe Mathieu-Daudé]

Hi Konstantin,

If there are no other patches on your guest-agent queue,
I can take this patch in my next pull request.

Regards,

Phil.

On 16/7/24 18:23, Zhao Liu wrote:

Make ga_wait_child() return boolean and check the returned boolean
in ga_run_command() instead of dereferencing @errp.

Cc: Michael Roth 
Cc: Konstantin Kostiuk 
Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Zhao Liu 
---
v2:
  * Added Phil's r/b.
  * Used Phil's polished words.
---
  qga/commands-posix.c | 8 
  1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 7f05996495a2..64bb0be94479 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -59,7 +59,7 @@
  #endif
  #endif
  
-static void ga_wait_child(pid_t pid, int *status, Error **errp)

+static bool ga_wait_child(pid_t pid, int *status, Error **errp)
  {
  pid_t rpid;
  
@@ -70,10 +70,11 @@ static void ga_wait_child(pid_t pid, int *status, Error **errp)

  if (rpid == -1) {
  error_setg_errno(errp, errno, "failed to wait for child (pid: %d)",
   pid);
-return;
+return false;
  }
  
  g_assert(rpid == pid);

+return true;
  }
  
  static ssize_t ga_pipe_read_str(int fd[2], char **str)

@@ -178,8 +179,7 @@ static int ga_run_command(const char *argv[], const char 
*in_str,
  goto out;
  }
  
-ga_wait_child(pid, &status, errp);

-if (*errp) {
+if (!ga_wait_child(pid, &status, errp)) {
  goto out;
  }
  

Re: [PATCH v2] target/s390x: filter deprecated properties based on model expansion type

[David Hildenbrand]

On 16.07.24 19:32, Collin Walling wrote:

As s390 CPU models progress and deprecated properties are dropped
outright, it will be cumbersome for management apps to query the host
for a comprehensive list of deprecated properties that will need to be
disabled on older models. To remedy this, the query-cpu-model-expansion
output now behaves by filtering deprecated properties based on the
expansion type instead of filtering based off of the model's full set
of features:

When reporting a static CPU model, only show deprecated properties that
are a subset of the model's enabled features.

When reporting a full CPU model, show the entire list of deprecated
properties regardless if they are supported on the model.

Suggested-by: Jiri Denemark 
Signed-off-by: Collin Walling 
---

Changelog:

 v2
 - Changed commit message
 - Added documentation reflecting this change
 - Made code changes that more accurately filter the deprecated
 properties based on expansion type.  This change makes it
 so that the deprecated-properties reported for a static model
 expansion are a subset of the model's properties instead of
 the model's full-definition properties.

 For example:

 Previously, the z900 static model would report 'bpb' in the
 list of deprecated-properties.  However, this prop is *not*
 a part of the model's feature set, leading to some inaccuracy
 (albeit harmless).

 Now, this feature will not show during a static expansion.
 It will, however, show up in a full expansion (along with
 the rest of the list: 'csske', 'te', 'cte').

@David, I've elected to respectully forgo adding your ack-by on this
iteration since I have changed the code (and therefore the behavior)
between this version and the previous in case you do not agree with
these adjustments.

---
  qapi/machine-target.json |  8 ++--
  target/s390x/cpu_models_sysemu.c | 16 +---
  2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/qapi/machine-target.json b/qapi/machine-target.json
index a8d9ec87f5..d151504f25 100644
--- a/qapi/machine-target.json
+++ b/qapi/machine-target.json
@@ -21,8 +21,12 @@
  # @props: a dictionary of QOM properties to be applied
  #
  # @deprecated-props: a list of properties that are flagged as deprecated
-# by the CPU vendor.  These props are a subset of the full model's
-# definition list of properties. (since 9.1)
+# by the CPU vendor.  (since 9.1).
+#
+# .. note:: Since 9.1, the list of deprecated props were always a subset
+#of the model's full-definition list of properites. Now, this list is


s/properites/properties/


+#populated with the model's enabled property set when delta changes
+#are applied. All deprecated properties are reported otherwise.
  #
  # Since: 2.8
  ##


Acked-by: David Hildenbrand 

--
Cheers,

David / dhildenb

Re: [PATCH 0/5] qapi: Doc comment cleanups

[Markus Armbruster]

Queued.

Re: [PATCH 04/14] qapi: add a 'command-features' pragma

[Daniel P . Berrangé]

On Tue, Jul 16, 2024 at 08:08:42PM +0200, Markus Armbruster wrote:
> Sorry for the delay; too many distractions, and I needed a good think.
> 
> Daniel P. Berrangé  writes:
> 
> > On Fri, Jul 12, 2024 at 10:50:54AM +0200, Markus Armbruster wrote:
> >> Daniel P. Berrangé  writes:
> >> 
> >> > On Fri, Jul 12, 2024 at 10:07:34AM +0200, Markus Armbruster wrote:
> >> >> Daniel P. Berrangé  writes:
> >> >> 
> >> >> > The 'command-features' pragma allows for defining additional
> >> >> > special features that are unique to a particular QAPI schema
> >> >> > instance and its implementation.
> >> >> 
> >> >> So far, we have special features (predefined, known to the generator and
> >> >> treated specially), and normal features (user-defined, not known to the
> >> >> generator).  You create a new kind in between: user-defined, not known
> >> >> to the generator, yet treated specially (I guess?).  Hmm.
> >> >> 
> >> >> Could you at least hint at indented use here?  What special treatment do
> >> >> you have in mind?
> >> >
> >> > Essentially, these features are a way to attach metadata to commands that
> >> > the server side impl can later query. This eliminates the need to 
> >> > hardcode
> >> > lists of commands, such as in QGA which hardcodes a list of commands 
> >> > which
> >> > are safe to use when filesystems are frozen. This is illustrated later in
> >> > this series.
> >> 
> >> Please update docs/devel/qapi-code-gen.rst section "Pragma directives",
> >> and maybe section "Features".
> 
> Second thoughts; see below.
> 
> >> I'm not sure conflating the new kind of feature with existing special
> >> features is a good idea.  I need to review more of the series before I
> >> can make up my mind.
> >
> > I originally implemented a completely separate 'tags' concept in the
> > QAPI parser, before deciding I was just re-inventing 'features' for
> > no obvious benefit.
> >
> > The other nice thing about using features is that these are exposed
> > in the schema and docs. With the 'fsfreeze' restriction in code,
> > there's no formal docs of what commands are allowed when frozen, and
> > this is also not exposed in QAPI schema to apps. Using 'features'
> > we get all that as standard.
> 
> When you need to tack a mark to one or more things for whatever purpose
> *and* expose it to QMP clients, then features make sense.  This is the
> case here.
> 
> Initially, features were strictly an external interface annotation, and
> were not meant to be used within QEMU.  All features were user-defined.
> 
> This changed when I created configurable policy for deprecated and
> unstable management interfaces: the policy engine needs to check for
> features 'deprecated' and 'unstable'.  Since the policy engine is partly
> implemented in generated code, these two features need to be baked into
> the generator.  This makes them special.
> 
> You need less than that: a predicate "does  have " for
> certain features, ideally without baking them into the generator.
> 
> The command registry already tracks each command's special features for
> use by the policy engine.  Obvious idea: also track the features you
> want to pass to the predicate.
> 
> Your series adds tracking for exactly the features you need:
> 
> * Enumerate them in the schema with new pragma command-features
> 
>   Missing: documentation for the pragma.
> 
> * Generate an extension QapiSpecialFeatureCustom of existing enum
>   QapiSpecialFeature, which is not generated.  The latter is in
>   qapi/util.h, the former in ${prefix}qapi-init-commands.h.
> 
> * Mark these features special for commands only, so existing registry
>   machinery tracks them.  Do *not* make them special elsewhere, because
>   that would break things.
> 
>   Feels like a hack.  Minor trap: if you use the same feature in
>   multiple schemas, multiple generated headers will define the same enum
>   constant, possibly with different values.  If you manage to include
>   the wrong header *and* the value differs there, you'll likely lose
>   hair.
> 
> * Missing: tests.
> 
> I think we can avoid supplying most of the missing bits.  The main QAPI
> schema uses five features: deprecated, unstable,
> allow-write-only-overlay, dynamic-auto-read-only, fdset.  The QGA QAPI
> schema uses four, namely the four you add in this series.  Why not track
> all features, and dispense with the pragma?  Like this:
> 
> * Change type of feature bitsets to uint64_t (it's unsigned now).
> 
> * Error out if a schema has more than 64 features.
> 
> * Move enum QapiSpecialFeature into a new generated header.
> 
> * Generate a member for each feature, not just the two predefined ones.
> 
> * Pass all command features to the registry, not just the special ones.
> 
> * Recommended: do the same elsewhere, i.e. replace
>   gen_special_features() by gen_features().
> 
> Thoughts?

So basically the code would always have access to all features, and
we would have no notion of "special" features any more.

I'm happy to give that a

Re: [PATCH v2 0/9] qapi: convert example sections to qmp-example rST directives

[Markus Armbruster]

John Snow  writes:

> This patchset focuses on converting example sections to rST directives
> using a new `.. qmp-example::` directive.

Queued, thanks!

[PULL 07/14] docs/qapidoc: factor out do_parse()

[Markus Armbruster]

From: John Snow 

Factor out the compatibility parser helper into a base class, so it can
be shared by other directives.

Signed-off-by: John Snow 
Reviewed-by: Markus Armbruster 
Message-ID: <20240717021312.606116-3-js...@redhat.com>
Signed-off-by: Markus Armbruster 
---
 docs/sphinx/qapidoc.py | 32 +++-
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index 62b39833ca..b3be82998a 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -481,7 +481,25 @@ def visit_module(self, name):
 super().visit_module(name)
 
 
-class QAPIDocDirective(Directive):
+class NestedDirective(Directive):
+def run(self):
+raise NotImplementedError
+
+def do_parse(self, rstlist, node):
+"""
+Parse rST source lines and add them to the specified node
+
+Take the list of rST source lines rstlist, parse them as
+rST, and add the resulting docutils nodes as children of node.
+The nodes are parsed in a way that allows them to include
+subheadings (titles) without confusing the rendering of
+anything else.
+"""
+with switch_source_input(self.state, rstlist):
+nested_parse_with_titles(self.state, rstlist, node)
+
+
+class QAPIDocDirective(NestedDirective):
 """Extract documentation from the specified QAPI .json file"""
 
 required_argument = 1
@@ -519,18 +537,6 @@ def run(self):
 # so they are displayed nicely to the user
 raise ExtensionError(str(err)) from err
 
-def do_parse(self, rstlist, node):
-"""Parse rST source lines and add them to the specified node
-
-Take the list of rST source lines rstlist, parse them as
-rST, and add the resulting docutils nodes as children of node.
-The nodes are parsed in a way that allows them to include
-subheadings (titles) without confusing the rendering of
-anything else.
-"""
-with switch_source_input(self.state, rstlist):
-nested_parse_with_titles(self.state, rstlist, node)
-
 
 def setup(app):
 """Register qapi-doc directive with Sphinx"""
-- 
2.45.0

[PULL 02/14] qapi/pci: Clean up documentation around PciDeviceClass

[Markus Armbruster]

PciDeviceInfo's doc comment has a note on PciDeviceClass member @desc.
Since the note applies always, not just within PciDeviceInfo, merge it
into PciDeviceClass's description of member @desc.

Signed-off-by: Markus Armbruster 
Message-ID: <2024072228.2140606-2-arm...@redhat.com>
Reviewed-by: Michael S. Tsirkin 
Reviewed-by: John Snow 
---
 qapi/pci.json | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/qapi/pci.json b/qapi/pci.json
index 8287d15dd0..97179920fb 100644
--- a/qapi/pci.json
+++ b/qapi/pci.json
@@ -93,7 +93,8 @@
 #
 # Information about the Class of a PCI device
 #
-# @desc: a string description of the device's class
+# @desc: a string description of the device's class (not stable, and
+# should only be treated as informational)
 #
 # @class: the class code of the device
 #
@@ -146,9 +147,6 @@
 #
 # @regions: a list of the PCI I/O regions associated with the device
 #
-# .. note:: The contents of @class_info.desc are not stable and should
-#only be treated as informational.
-#
 # Since: 0.14
 ##
 { 'struct': 'PciDeviceInfo',
-- 
2.45.0

[PULL 09/14] docs/qapidoc: add QMP highlighting to annotated qmp-example blocks

[Markus Armbruster]

From: John Snow 

For any code literal blocks inside of a qmp-example directive, apply and
enforce the QMP lexer/highlighter to those blocks.

This way, you won't need to write:

```
.. qmp-example::
   :annotated:

   Blah blah

   .. code-block:: QMP

  -> { "lorem": "ipsum" }
```

But instead, simply:

```
.. qmp-example::
   :annotated:

   Blah blah::

 -> { "lorem": "ipsum" }
```

Once the directive block is exited, whatever the previous default
highlight language was will be restored; localizing the forced QMP
lexing to exclusively this directive.

Note, if the default language is *already* QMP, this directive will not
generate and restore redundant highlight configuration nodes. We may
well decide that the default language ought to be QMP for any QAPI
reference pages, but this way the directive behaves consistently no
matter where it is used.

Signed-off-by: John Snow 
Acked-by: Markus Armbruster 
Message-ID: <20240717021312.606116-5-js...@redhat.com>
Signed-off-by: Markus Armbruster 
---
 docs/sphinx/qapidoc.py | 53 ++
 1 file changed, 49 insertions(+), 4 deletions(-)

diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index 11defcfa3f..738b2450fb 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -26,6 +26,7 @@
 
 import os
 import re
+import sys
 import textwrap
 from typing import List
 
@@ -36,6 +37,7 @@
 from qapi.gen import QAPISchemaVisitor
 from qapi.schema import QAPISchema
 
+from sphinx import addnodes
 from sphinx.directives.code import CodeBlock
 from sphinx.errors import ExtensionError
 from sphinx.util.docutils import switch_source_input
@@ -545,10 +547,10 @@ class QMPExample(CodeBlock, NestedDirective):
 Custom admonition for QMP code examples.
 
 When the :annotated: option is present, the body of this directive
-is parsed as normal rST instead. Code blocks must be explicitly
-written by the user, but this allows for intermingling explanatory
-paragraphs with arbitrary rST syntax and code blocks for more
-involved examples.
+is parsed as normal rST, but with any '::' code blocks set to use
+the QMP lexer. Code blocks must be explicitly written by the user,
+but this allows for intermingling explanatory paragraphs with
+arbitrary rST syntax and code blocks for more involved examples.
 
 When :annotated: is absent, the directive body is treated as a
 simple standalone QMP code block literal.
@@ -562,6 +564,33 @@ class QMPExample(CodeBlock, NestedDirective):
 "title": directives.unchanged,
 }
 
+def _highlightlang(self) -> addnodes.highlightlang:
+"""Return the current highlightlang setting for the document"""
+node = None
+doc = self.state.document
+
+if hasattr(doc, "findall"):
+# docutils >= 0.18.1
+for node in doc.findall(addnodes.highlightlang):
+pass
+else:
+for elem in doc.traverse():
+if isinstance(elem, addnodes.highlightlang):
+node = elem
+
+if node:
+return node
+
+# No explicit directive found, use defaults
+node = addnodes.highlightlang(
+lang=self.env.config.highlight_language,
+force=False,
+# Yes, Sphinx uses this value to effectively disable line
+# numbers and not 0 or None or -1 or something. ¯\_(ツ)_/¯
+linenothreshold=sys.maxsize,
+)
+return node
+
 def admonition_wrap(self, *content) -> List[nodes.Node]:
 title = "Example:"
 if "title" in self.options:
@@ -576,8 +605,24 @@ def admonition_wrap(self, *content) -> List[nodes.Node]:
 return [admon]
 
 def run_annotated(self) -> List[nodes.Node]:
+lang_node = self._highlightlang()
+
 content_node: nodes.Element = nodes.section()
+
+# Configure QMP highlighting for "::" blocks, if needed
+if lang_node["lang"] != "QMP":
+content_node += addnodes.highlightlang(
+lang="QMP",
+force=False,  # "True" ignores lexing errors
+linenothreshold=lang_node["linenothreshold"],
+)
+
 self.do_parse(self.content, content_node)
+
+# Restore prior language highlighting, if needed
+if lang_node["lang"] != "QMP":
+content_node += addnodes.highlightlang(**lang_node.attributes)
+
 return content_node.children
 
 def run(self) -> List[nodes.Node]:
-- 
2.45.0

[PULL 13/14] qapi: convert "Example" sections with longer prose

[Markus Armbruster]

From: John Snow 

These examples require longer explanations or have explanations that
require markup to look reasonable when rendered and so use the longer
form of the ".. qmp-example::" directive.

By using the :annotated: option, the content in the example block is
assumed *not* to be a code block literal and is instead parsed as normal
rST - with the exception that any code literal blocks after `::` will
assumed to be a QMP code literal block.

Note: There's one title-less conversion in this patch that comes along
for the ride because it's part of a larger "Examples" block that was
better to convert all at once.

See commit-5: "docs/qapidoc: create qmp-example directive", for a
  detailed explanation of this custom directive syntax.

See commit+1: "qapi: remove "Example" doc section" for a detailed
  explanation of why.

Signed-off-by: John Snow 
Reviewed-by: Markus Armbruster 
Message-ID: <20240717021312.606116-9-js...@redhat.com>
Signed-off-by: Markus Armbruster 
---
 qapi/block.json | 30 +++---
 qapi/machine.json   | 30 --
 qapi/migration.json |  7 +--
 qapi/virtio.json| 24 ++--
 4 files changed, 62 insertions(+), 29 deletions(-)

diff --git a/qapi/block.json b/qapi/block.json
index 5ddd061e96..ce9490a367 100644
--- a/qapi/block.json
+++ b/qapi/block.json
@@ -545,31 +545,37 @@
 #
 # Since: 4.0
 #
-# Example:
+# .. qmp-example::
+#:annotated:
 #
-# Set new histograms for all io types with intervals
-# [0, 10), [10, 50), [50, 100), [100, +inf):
+#Set new histograms for all io types with intervals
+#[0, 10), [10, 50), [50, 100), [100, +inf)::
 #
 # -> { "execute": "block-latency-histogram-set",
 #  "arguments": { "id": "drive0",
 # "boundaries": [10, 50, 100] } }
 # <- { "return": {} }
 #
-# Example:
+# .. qmp-example::
+#:annotated:
 #
-# Set new histogram only for write, other histograms will remain
-# not changed (or not created):
+#Set new histogram only for write, other histograms will remain
+#not changed (or not created)::
 #
 # -> { "execute": "block-latency-histogram-set",
 #  "arguments": { "id": "drive0",
 # "boundaries-write": [10, 50, 100] } }
 # <- { "return": {} }
 #
-# Example:
+# .. qmp-example::
+#:annotated:
 #
-# Set new histograms with the following intervals:
-#   read, flush: [0, 10), [10, 50), [50, 100), [100, +inf)
-#   write: [0, 1000), [1000, 5000), [5000, +inf)
+#Set new histograms with the following intervals:
+#
+#- read, flush: [0, 10), [10, 50), [50, 100), [100, +inf)
+#- write: [0, 1000), [1000, 5000), [5000, +inf)
+#
+#::
 #
 # -> { "execute": "block-latency-histogram-set",
 #  "arguments": { "id": "drive0",
@@ -578,7 +584,9 @@
 # <- { "return": {} }
 #
 # .. qmp-example::
-#:title: Remove all latency histograms
+#:annotated:
+#
+#Remove all latency histograms::
 #
 # -> { "execute": "block-latency-histogram-set",
 #  "arguments": { "id": "drive0" } }
diff --git a/qapi/machine.json b/qapi/machine.json
index 193b2b7c16..f9ea6b3e97 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1045,10 +1045,11 @@
 #
 # Since: 2.7
 #
-# Examples:
+# .. qmp-example::
+#:annotated:
 #
-# For pseries machine type started with -smp 2,cores=2,maxcpus=4
-# -cpu POWER8:
+#For pseries machine type started with
+#``-smp 2,cores=2,maxcpus=4 -cpu POWER8``::
 #
 # -> { "execute": "query-hotpluggable-cpus" }
 # <- {"return": [
@@ -1058,7 +1059,10 @@
 #"vcpus-count": 1, "qom-path": "/machine/unattached/device[0]"}
 #]}
 #
-# For pc machine type started with -smp 1,maxcpus=2:
+# .. qmp-example::
+#:annotated:
+#
+#For pc machine type started with ``-smp 1,maxcpus=2``::
 #
 # -> { "execute": "query-hotpluggable-cpus" }
 # <- {"return": [
@@ -1073,8 +1077,11 @@
 #  }
 #]}
 #
-# For s390x-virtio-ccw machine type started with -smp 1,maxcpus=2
-# -cpu qemu (Since: 2.11):
+# .. qmp-example::
+#:annotated:
+#
+#For s390x-virtio-ccw machine type started with
+#``-smp 1,maxcpus=2 -cpu qemu`` (Since: 2.11)::
 #
 # -> { "execute": "query-hotpluggable-cpus" }
 # <- {"return": [
@@ -1128,12 +1135,15 @@
 #
 # Since: 0.14
 #
-# Example:
+# .. qmp-example::
+#:annotated:
 #
-# -> { "execute": "balloon", "arguments": { "value": 536870912 } }
-# <- { "return": {} }
+#::
 #
-# With a 2.5GiB guest this command inflated the ballon to 3GiB.
+#  -> { "execute": "balloon", "arguments": { "value": 536870912 } }
+#  <- { "return": {} }
+#
+#With a 2.5GiB guest this command inflated the ballon to 3GiB.
 ##
 { 'command': 'balloon', 'data': {'value': 'int'} }
 
diff --git a/qapi/migration.json b/qapi/migration.json
index 74968a1417..073b67c052 100644
--- a/qapi/migration.json
+++ b/q

[PULL 04/14] qapi/machine: Clarify query-uuid value when none has been specified

[Markus Armbruster]

When no UUID has been specified, query-uuid returns

{"UUID": "----"}

The doc comment calls this "a null UUID", which I find less than
clear.  RFC 9562 calls it "the nil UUID (all zeroes)", so use that
instead.

Signed-off-by: Markus Armbruster 
Message-ID: <2024072228.2140606-4-arm...@redhat.com>
Reviewed-by: Michael S. Tsirkin 
Reviewed-by: John Snow 
[Wording improved, commit message adjusted]
Reviewed-by: Philippe Mathieu-Daudé 
---
 qapi/machine.json | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/qapi/machine.json b/qapi/machine.json
index 50ff102d56..eb2fd01e95 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -305,9 +305,8 @@
 #
 # Since: 0.14
 #
-# .. note:: If no UUID was specified for the guest, a null UUID is
-#returned.
-#
+# .. note:: If no UUID was specified for the guest, the nil UUID (all
+#zeroes) is returned.
 ##
 { 'struct': 'UuidInfo', 'data': {'UUID': 'str'} }
 
-- 
2.45.0

[PULL 12/14] qapi: convert "Example" sections with titles

[Markus Armbruster]

From: John Snow 

When an Example section has a brief explanation, convert it to a
qmp-example:: section using the :title: option.

Rule of thumb: If the title can fit on a single line and requires no rST
markup, it's a good candidate for using the :title: option of
qmp-example.

In this patch, trailing punctuation is removed from the title section
for consistent headline aesthetics. In just one case, specifics of the
example are removed to make the title read better.

See commit-4: "docs/qapidoc: create qmp-example directive", for a
  detailed explanation of this custom directive syntax.

See commit+2: "qapi: remove "Example" doc section" for a detailed
  explanation of why.

Signed-off-by: John Snow 
Reviewed-by: Markus Armbruster 
Message-ID: <20240717021312.606116-8-js...@redhat.com>
Signed-off-by: Markus Armbruster 
---
 qapi/block-core.json | 24 
 qapi/block.json  | 13 ++---
 qapi/migration.json  | 25 ++---
 qapi/qom.json|  8 
 qapi/ui.json | 11 ++-
 qapi/virtio.json | 19 ++-
 6 files changed, 52 insertions(+), 48 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 84a020aa9e..f400b334c8 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -5881,9 +5881,8 @@
 #
 # Since: 2.7
 #
-# Examples:
-#
-# 1. Add a new node to a quorum
+# .. qmp-example::
+#:title: Add a new node to a quorum
 #
 # -> { "execute": "blockdev-add",
 #  "arguments": {
@@ -5897,7 +5896,8 @@
 # "node": "new_node" } }
 # <- { "return": {} }
 #
-# 2. Delete a quorum's node
+# .. qmp-example::
+#:title: Delete a quorum's node
 #
 # -> { "execute": "x-blockdev-change",
 #  "arguments": { "parent": "disk1",
@@ -5933,16 +5933,16 @@
 #
 # Since: 2.12
 #
-# Examples:
-#
-# 1. Move a node into an IOThread
+# .. qmp-example::
+#:title: Move a node into an IOThread
 #
 # -> { "execute": "x-blockdev-set-iothread",
 #  "arguments": { "node-name": "disk1",
 # "iothread": "iothread0" } }
 # <- { "return": {} }
 #
-# 2. Move a node into the main loop
+# .. qmp-example::
+#:title: Move a node into the main loop
 #
 # -> { "execute": "x-blockdev-set-iothread",
 #  "arguments": { "node-name": "disk1",
@@ -6018,16 +6018,16 @@
 #
 # Since: 2.0
 #
-# Examples:
-#
-# 1. Read operation
+# .. qmp-example::
+#:title: Read operation
 #
 # <- { "event": "QUORUM_REPORT_BAD",
 #  "data": { "node-name": "node0", "sector-num": 345435, 
"sectors-count": 5,
 #"type": "read" },
 #  "timestamp": { "seconds": 1344522075, "microseconds": 745528 } }
 #
-# 2. Flush operation
+# .. qmp-example::
+#:title: Flush operation
 #
 # <- { "event": "QUORUM_REPORT_BAD",
 #  "data": { "node-name": "node0", "sector-num": 0, "sectors-count": 
2097120,
diff --git a/qapi/block.json b/qapi/block.json
index c8e52bc2d2..5ddd061e96 100644
--- a/qapi/block.json
+++ b/qapi/block.json
@@ -342,9 +342,8 @@
 #
 # Since: 2.5
 #
-# Examples:
-#
-# 1. Change a removable medium
+# .. qmp-example::
+#:title: Change a removable medium
 #
 # -> { "execute": "blockdev-change-medium",
 #  "arguments": { "id": "ide0-1-0",
@@ -352,7 +351,8 @@
 # "format": "raw" } }
 # <- { "return": {} }
 #
-# 2. Load a read-only medium into a writable drive
+# .. qmp-example::
+#:title: Load a read-only medium into a writable drive
 #
 # -> { "execute": "blockdev-change-medium",
 #  "arguments": { "id": "floppyA",
@@ -577,9 +577,8 @@
 # "boundaries-write": [1000, 5000] } }
 # <- { "return": {} }
 #
-# Example:
-#
-# Remove all latency histograms:
+# .. qmp-example::
+#:title: Remove all latency histograms
 #
 # -> { "execute": "block-latency-histogram-set",
 #  "arguments": { "id": "drive0" } }
diff --git a/qapi/migration.json b/qapi/migration.json
index 3c65720238..74968a1417 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -287,14 +287,14 @@
 #
 # Since: 0.14
 #
-# Examples:
-#
-# 1. Before the first migration
+# .. qmp-example::
+#:title: Before the first migration
 #
 # -> { "execute": "query-migrate" }
 # <- { "return": {} }
 #
-# 2. Migration is done and has succeeded
+# .. qmp-example::
+#:title: Migration is done and has succeeded
 #
 # -> { "execute": "query-migrate" }
 # <- { "return": {
@@ -314,12 +314,14 @@
 #  }
 #}
 #
-# 3. Migration is done and has failed
+# .. qmp-example::
+#:title: Migration is done and has failed
 #
 # -> { "execute": "query-migrate" }
 # <- { "return": { "status": "failed" } }
 #
-# 4. Migration is being performed:
+# .. qmp-example::
+#:title: Migration is being performed
 #
 # -> { "execute": "query-migrate" }
 # <

[PULL 14/14] qapi: remove "Example" doc section

[Markus Armbruster]

From: John Snow 

Fully eliminate the "Example" sections in QAPI doc blocks now that they
have all been converted to arbitrary rST syntax using the
".. qmp-example::" directive. Update tests to match.

Migrating to the new syntax
---

The old "Example:" or "Examples:" section syntax is now caught as an
error, but "Example::" is stil permitted as explicit rST syntax for an
un-lexed, generic preformatted text block.

('Example' is not special in this case, any sentence that ends with "::"
will start an indented code block in rST.)

Arbitrary rST for Examples is now possible, but it's strongly
recommended that documentation authors use the ".. qmp-example::"
directive for consistent visual formatting in rendered HTML docs. The
":title:" directive option may be used to add extra information into the
title bar for the example. The ":annotated:" option can be used to write
arbitrary rST instead, with nested "::" blocks applying QMP formatting
where desired.

Other choices available are ".. code-block:: QMP" which will not create
an "Example:" box, or the short-form "::" code-block syntax which will
not apply QMP highlighting when used outside of the qmp-example
directive.

Why?


This patch has several benefits:

1. Example sections can now be written more arbitrarily, mixing
   explanatory paragraphs and code blocks however desired.

2. Example sections can now use fully arbitrary rST.

3. All code blocks are now lexed and validated as QMP; increasing
   usability of the docs and ensuring validity of example snippets.

   (To some extent - This patch only gaurantees it lexes correctly, not
   that it's valid under the JSON or QMP grammars. It will catch most
   small mistakes, however.)

4. Each qmp-example can be titled or annotated independently without
   bypassing the QMP lexer/validator.

   (i.e. code blocks are now for *code* only, so we don't have to
   sacrifice exposition for having lexically valid examples.)

NOTE: As with the "Notes" conversion (d461c279737), this patch (and the
  three preceding) may change the rendering order for Examples in
  the current generator. The forthcoming qapidoc rewrite will fix
  this by always generating documentation in source order.

Signed-off-by: John Snow 
Message-ID: <20240717021312.606116-10-js...@redhat.com>
Reviewed-by: Markus Armbruster 
Signed-off-by: Markus Armbruster 
---
 docs/devel/qapi-code-gen.rst| 58 -
 scripts/qapi/parser.py  | 10 +-
 tests/qapi-schema/doc-good.json | 19 +++
 tests/qapi-schema/doc-good.out  | 26 ++-
 tests/qapi-schema/doc-good.txt  | 23 ++---
 5 files changed, 98 insertions(+), 38 deletions(-)

diff --git a/docs/devel/qapi-code-gen.rst b/docs/devel/qapi-code-gen.rst
index ae97b335cb..583207a8ec 100644
--- a/docs/devel/qapi-code-gen.rst
+++ b/docs/devel/qapi-code-gen.rst
@@ -899,7 +899,7 @@ Documentation markup
 
 
 Documentation comments can use most rST markup.  In particular,
-a ``::`` literal block can be used for examples::
+a ``::`` literal block can be used for pre-formatted text::
 
 # ::
 #
@@ -995,8 +995,8 @@ line "Features:", like this::
   # @feature: Description text
 
 A tagged section begins with a paragraph that starts with one of the
-following words: "Since:", "Example:"/"Examples:", "Returns:",
-"Errors:", "TODO:".  It ends with the start of a new section.
+following words: "Since:", "Returns:", "Errors:", "TODO:".  It ends with
+the start of a new section.
 
 The second and subsequent lines of tagged sections must be indented
 like this::
@@ -1020,13 +1020,53 @@ detailing a relevant error condition. For example::
 A "Since: x.y.z" tagged section lists the release that introduced the
 definition.
 
-An "Example" or "Examples" section is rendered entirely
-as literal fixed-width text.  "TODO" sections are not rendered at all
-(they are for developers, not users of QMP).  In other sections, the
-text is formatted, and rST markup can be used.
+"TODO" sections are not rendered (they are for developers, not users of
+QMP).  In other sections, the text is formatted, and rST markup can be
+used.
+
+QMP Examples can be added by using the ``.. qmp-example::``
+directive. In its simplest form, this can be used to contain a single
+QMP code block which accepts standard JSON syntax with additional server
+directionality indicators (``->`` and ``<-``), and elisions (``...``).
+
+Optionally, a plaintext title may be provided by using the ``:title:``
+directive option. If the title is omitted, the example title will
+default to "Example:".
+
+A simple QMP example::
+
+  # .. qmp-example::
+  #:title: Using query-block
+  #
+  #-> { "execute": "query-block" }
+  #<- { ... }
+
+More complex or multi-step examples where exposition is needed before or
+between QMP code blocks can be created by using the ``:annotated:``
+directive option. When using this option, nested QMP code blo

[PULL 08/14] docs/qapidoc: create qmp-example directive

[Markus Armbruster]

From: John Snow 

This is a directive that creates a syntactic sugar for creating
"Example" boxes very similar to the ones already used in the bitmaps.rst
document, please see e.g.
https://www.qemu.org/docs/master/interop/bitmaps.html#creation-block-dirty-bitmap-add

In its simplest form, when a custom title is not needed or wanted, and
the example body is *solely* a QMP example:

```
.. qmp-example::

   {body}
```

is syntactic sugar for:

```
.. admonition:: Example:

   .. code-block:: QMP

  {body}
```

When a custom, plaintext title that describes the example is desired,
this form:

```
.. qmp-example::
   :title: Defrobnification

   {body}
```

Is syntactic sugar for:

```
.. admonition:: Example: Defrobnification

   .. code-block:: QMP

  {body}
```

Lastly, when Examples are multi-step processes that require non-QMP
exposition, have lengthy titles, or otherwise involve prose with rST
markup (lists, cross-references, etc), the most complex form:

```
.. qmp-example::
   :annotated:

   This example shows how to use `foo-command`::

 {body}

   For more information, please see `frobnozz`.
```

Is desugared to:

```
.. admonition:: Example:

   This example shows how to use `foo-command`::

 {body}

   For more information, please see `frobnozz`.
```

Note that :annotated: and :title: options can be combined together, if
desired.

The primary benefit here being documentation source consistently using
the same directive for all forms of examples to ensure consistent visual
styling, and ensuring all relevant prose is visually grouped alongside
the code literal block.

Note that as of this commit, the code-block rST syntax "::" does not
apply QMP highlighting; you would need to use ".. code-block:: QMP". The
very next commit changes this behavior to assume all "::" code blocks
within this directive are QMP blocks.

Signed-off-by: John Snow 
Acked-by: Markus Armbruster 
Message-ID: <20240717021312.606116-4-js...@redhat.com>
Signed-off-by: Markus Armbruster 
---
 docs/sphinx/qapidoc.py | 55 ++
 1 file changed, 55 insertions(+)

diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index b3be82998a..11defcfa3f 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -27,6 +27,7 @@
 import os
 import re
 import textwrap
+from typing import List
 
 from docutils import nodes
 from docutils.parsers.rst import Directive, directives
@@ -35,6 +36,7 @@
 from qapi.gen import QAPISchemaVisitor
 from qapi.schema import QAPISchema
 
+from sphinx.directives.code import CodeBlock
 from sphinx.errors import ExtensionError
 from sphinx.util.docutils import switch_source_input
 from sphinx.util.nodes import nested_parse_with_titles
@@ -538,10 +540,63 @@ def run(self):
 raise ExtensionError(str(err)) from err
 
 
+class QMPExample(CodeBlock, NestedDirective):
+"""
+Custom admonition for QMP code examples.
+
+When the :annotated: option is present, the body of this directive
+is parsed as normal rST instead. Code blocks must be explicitly
+written by the user, but this allows for intermingling explanatory
+paragraphs with arbitrary rST syntax and code blocks for more
+involved examples.
+
+When :annotated: is absent, the directive body is treated as a
+simple standalone QMP code block literal.
+"""
+
+required_argument = 0
+optional_arguments = 0
+has_content = True
+option_spec = {
+"annotated": directives.flag,
+"title": directives.unchanged,
+}
+
+def admonition_wrap(self, *content) -> List[nodes.Node]:
+title = "Example:"
+if "title" in self.options:
+title = f"{title} {self.options['title']}"
+
+admon = nodes.admonition(
+"",
+nodes.title("", title),
+*content,
+classes=["admonition", "admonition-example"],
+)
+return [admon]
+
+def run_annotated(self) -> List[nodes.Node]:
+content_node: nodes.Element = nodes.section()
+self.do_parse(self.content, content_node)
+return content_node.children
+
+def run(self) -> List[nodes.Node]:
+annotated = "annotated" in self.options
+
+if annotated:
+content_nodes = self.run_annotated()
+else:
+self.arguments = ["QMP"]
+content_nodes = super().run()
+
+return self.admonition_wrap(*content_nodes)
+
+
 def setup(app):
 """Register qapi-doc directive with Sphinx"""
 app.add_config_value("qapidoc_srctree", None, "env")
 app.add_directive("qapi-doc", QAPIDocDirective)
+app.add_directive("qmp-example", QMPExample)
 
 return {
 "version": __version__,
-- 
2.45.0

[PULL 05/14] qapi/sockets: Move deprecation note out of SocketAddress doc comment

[Markus Armbruster]

Doc comments are reference documentation for users of QMP.
SocketAddress's doc comment contains a deprecation note advising
developers to use SocketAddress for new code.  Irrelevant for users of
QMP.  Move the note out of the doc comment.

Signed-off-by: Markus Armbruster 
Message-ID: <2024072228.2140606-5-arm...@redhat.com>
Reviewed-by: John Snow 
Reviewed-by: Philippe Mathieu-Daudé 
---
 qapi/sockets.json | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/qapi/sockets.json b/qapi/sockets.json
index 4d78d2ccb7..e76fdb9925 100644
--- a/qapi/sockets.json
+++ b/qapi/sockets.json
@@ -179,10 +179,6 @@
 #
 # @type: Transport type
 #
-# .. note:: This type is deprecated in favor of SocketAddress.  The
-#difference between SocketAddressLegacy and SocketAddress is that
-#the latter has fewer ``{}`` on the wire.
-#
 # Since: 1.3
 ##
 { 'union': 'SocketAddressLegacy',
@@ -193,6 +189,9 @@
 'unix': 'UnixSocketAddressWrapper',
 'vsock': 'VsockSocketAddressWrapper',
 'fd': 'FdSocketAddressWrapper' } }
+# Note: This type is deprecated in favor of SocketAddress.  The
+# difference between SocketAddressLegacy and SocketAddress is that the
+# latter has fewer ``{}`` on the wire.
 
 ##
 # @SocketAddressType:
-- 
2.45.0

[PULL 00/14] QAPI patches patches for 2024-07-17

[Markus Armbruster]

The following changes since commit e2f346aa98646e84eabe0256f89d08e89b1837cf:

  Merge tag 'sdmmc-20240716' of https://github.com/philmd/qemu into staging 
(2024-07-17 07:59:31 +1000)

are available in the Git repository at:

  https://repo.or.cz/qemu/armbru.git tags/pull-qapi-2024-07-17

for you to fetch changes up to 3c5f6114d9ffc70bc9b1a7cc072a911f966d:

  qapi: remove "Example" doc section (2024-07-17 10:20:54 +0200)


QAPI patches patches for 2024-07-17


Harmonie Snow (1):
  docs/sphinx: add CSS styling for qmp-example directive

John Snow (7):
  docs/qapidoc: factor out do_parse()
  docs/qapidoc: create qmp-example directive
  docs/qapidoc: add QMP highlighting to annotated qmp-example blocks
  qapi: convert "Example" sections without titles
  qapi: convert "Example" sections with titles
  qapi: convert "Example" sections with longer prose
  qapi: remove "Example" doc section

Markus Armbruster (6):
  qapi/qom: Document feature unstable of @x-vfio-user-server
  qapi/pci: Clean up documentation around PciDeviceClass
  qapi/machine: Clean up documentation around CpuInstanceProperties
  qapi/machine: Clarify query-uuid value when none has been specified
  qapi/sockets: Move deprecation note out of SocketAddress doc comment
  qapi/ui: Drop note on naming of SpiceQueryMouseMode

 docs/devel/qapi-code-gen.rst   |  58 ---
 docs/sphinx-static/theme_overrides.css |  49 +
 docs/sphinx/qapidoc.py | 128 ++---
 qapi/acpi.json |   4 +-
 qapi/block-core.json   |  88 ---
 qapi/block.json|  57 ---
 qapi/char.json |  24 ---
 qapi/control.json  |   8 +--
 qapi/dump.json |   8 +--
 qapi/machine-target.json   |   2 +-
 qapi/machine.json  |  86 --
 qapi/migration.json|  90 ---
 qapi/misc-target.json  |  22 +++---
 qapi/misc.json |  32 -
 qapi/net.json  |  22 +++---
 qapi/pci.json  |   8 +--
 qapi/qdev.json |  10 +--
 qapi/qom.json  |  19 ++---
 qapi/replay.json   |   8 +--
 qapi/rocker.json   |   8 +--
 qapi/run-state.json|  32 -
 qapi/sockets.json  |   7 +-
 qapi/tpm.json  |   6 +-
 qapi/trace.json|   4 +-
 qapi/transaction.json  |   2 +-
 qapi/ui.json   |  47 ++--
 qapi/vfio.json |   2 +-
 qapi/virtio.json   |  45 +++-
 qapi/yank.json |   4 +-
 scripts/qapi/parser.py |  10 ++-
 tests/qapi-schema/doc-good.json|  19 +++--
 tests/qapi-schema/doc-good.out |  26 ---
 tests/qapi-schema/doc-good.txt |  23 +++---
 33 files changed, 610 insertions(+), 348 deletions(-)

-- 
2.45.0

Re: [PATCH 3/3] semihosting: Restrict to TCG

[Philippe Mathieu-Daudé]

On 19/6/24 09:49, Paolo Bonzini wrote:

On Wed, Jun 12, 2024 at 3:13 PM Philippe Mathieu-Daudé
 wrote:

Building qemu-system-mips configured with --without-default-devices:

Undefined symbols for architecture arm64:
"_qemu_semihosting_console_write", referenced from:
_mips_semihosting in target_mips_tcg_sysemu_mips-semi.c.o
"_semihost_sys_close", referenced from:
_mips_semihosting in target_mips_tcg_sysemu_mips-semi.c.o
"_uaccess_strlen_user", referenced from:
_mips_semihosting in target_mips_tcg_sysemu_mips-semi.c.o
...

So this one has to use "select".

Similarly m68k:

Undefined symbols for architecture arm64:
"_semihost_sys_close", referenced from:
_do_m68k_semihosting in target_m68k_m68k-semi.c.o
...





The file to be stubbed would have
to be target/m68k/m68k-semi.c (for do_m68k_semihosting) and
target/mips/tcg/sysemu/mips-semi.c (for mips_semihosting), not the
common code.


Yes! Thank you, I'm was to blind to realize that...

[PULL 10/14] docs/sphinx: add CSS styling for qmp-example directive

[Markus Armbruster]

From: Harmonie Snow 

Add CSS styling for qmp-example directives to increase readability and
consistently style all example blocks.

Signed-off-by: Harmonie Snow 
Signed-off-by: John Snow 
Acked-by: Markus Armbruster 
Message-ID: <20240717021312.606116-6-js...@redhat.com>
Signed-off-by: Markus Armbruster 
---
 docs/sphinx-static/theme_overrides.css | 49 ++
 1 file changed, 49 insertions(+)

diff --git a/docs/sphinx-static/theme_overrides.css 
b/docs/sphinx-static/theme_overrides.css
index c70ef95128..965ecac54f 100644
--- a/docs/sphinx-static/theme_overrides.css
+++ b/docs/sphinx-static/theme_overrides.css
@@ -87,6 +87,55 @@ div[class^="highlight"] pre {
 padding-bottom: 1px;
 }
 
+/* qmp-example directive styling */
+
+.rst-content .admonition-example {
+/* do not apply the standard admonition background */
+background-color: transparent;
+border: solid #ffd2ed 1px;
+}
+
+.rst-content .admonition-example > .admonition-title:before {
+content: "▷";
+}
+
+.rst-content .admonition-example > .admonition-title {
+background-color: #5980a6;
+}
+
+.rst-content .admonition-example > div[class^="highlight"] {
+/* make code boxes take up the full width of the admonition w/o margin */
+margin-left: -12px;
+margin-right: -12px;
+
+border-top: 1px solid #ffd2ed;
+border-bottom: 1px solid #ffd2ed;
+border-left: 0px;
+border-right: 0px;
+}
+
+.rst-content .admonition-example > div[class^="highlight"]:nth-child(2) {
+/* If a code box is the second element in an example admonition,
+ * it is the first child after the title. let it sit flush against
+ * the title. */
+margin-top: -12px;
+border-top: 0px;
+}
+
+.rst-content .admonition-example > div[class^="highlight"]:last-child {
+/* If a code box is the final element in an example admonition, don't
+ * render margin below it; let it sit flush with the end of the
+ * admonition box */
+margin-bottom: -12px;
+border-bottom: 0px;
+}
+
+.rst-content .admonition-example .highlight {
+background-color: #fffafd;
+}
+
+/* end qmp-example styling */
+
 @media screen {
 
 /* content column
-- 
2.45.0

[PULL 11/14] qapi: convert "Example" sections without titles

[Markus Armbruster]

From: John Snow 

Use the no-option form of ".. qmp-example::" to convert any Examples
that do not have any form of caption or explanation whatsoever. Note
that in a few cases, example sections are split into two or more
separate example blocks. This is only done stylistically to create a
delineation between two or more logically independent examples.

See commit-3: "docs/qapidoc: create qmp-example directive", for a
  detailed explanation of this custom directive syntax.

See commit+3: "qapi: remove "Example" doc section" for a detailed
  explanation of why.

Note: an empty "TODO" line was added to announce-self to keep the
example from floating up into the body; this will be addressed more
rigorously in the new qapidoc generator.

Signed-off-by: John Snow 
Message-ID: <20240717021312.606116-7-js...@redhat.com>
Reviewed-by: Markus Armbruster 
[Markup fixed in one place]
Signed-off-by: Markus Armbruster 
---
 qapi/acpi.json   |  4 +--
 qapi/block-core.json | 64 +---
 qapi/block.json  | 18 ++-
 qapi/char.json   | 24 +--
 qapi/control.json|  8 ++---
 qapi/dump.json   |  8 ++---
 qapi/machine-target.json |  2 +-
 qapi/machine.json| 38 
 qapi/migration.json  | 58 ++--
 qapi/misc-target.json| 22 +++---
 qapi/misc.json   | 32 ++--
 qapi/net.json| 22 --
 qapi/pci.json|  2 +-
 qapi/qdev.json   | 10 ---
 qapi/qom.json|  8 ++---
 qapi/replay.json |  8 ++---
 qapi/rocker.json |  8 ++---
 qapi/run-state.json  | 32 ++--
 qapi/tpm.json|  6 ++--
 qapi/trace.json  |  4 +--
 qapi/transaction.json|  2 +-
 qapi/ui.json | 34 ++---
 qapi/vfio.json   |  2 +-
 qapi/virtio.json |  2 +-
 qapi/yank.json   |  4 +--
 25 files changed, 219 insertions(+), 203 deletions(-)

diff --git a/qapi/acpi.json b/qapi/acpi.json
index aa4dbe5794..045dab6228 100644
--- a/qapi/acpi.json
+++ b/qapi/acpi.json
@@ -111,7 +111,7 @@
 #
 # Since: 2.1
 #
-# Example:
+# .. qmp-example::
 #
 # -> { "execute": "query-acpi-ospm-status" }
 # <- { "return": [ { "device": "d1", "slot": "0", "slot-type": "DIMM", 
"source": 1, "status": 0},
@@ -131,7 +131,7 @@
 #
 # Since: 2.1
 #
-# Example:
+# .. qmp-example::
 #
 # <- { "event": "ACPI_DEVICE_OST",
 #  "data": { "info": { "device": "d1", "slot": "0",
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 096bdbe0aa..84a020aa9e 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -764,7 +764,7 @@
 #
 # Since: 0.14
 #
-# Example:
+# .. qmp-example::
 #
 # -> { "execute": "query-block" }
 # <- {
@@ -1168,7 +1168,7 @@
 #
 # Since: 0.14
 #
-# Example:
+# .. qmp-example::
 #
 # -> { "execute": "query-blockstats" }
 # <- {
@@ -1461,7 +1461,7 @@
 #
 # Since: 0.14
 #
-# Example:
+# .. qmp-example::
 #
 # -> { "execute": "block_resize",
 #  "arguments": { "device": "scratch", "size": 1073741824 } }
@@ -1680,7 +1680,7 @@
 #
 # Since: 0.14
 #
-# Example:
+# .. qmp-example::
 #
 # -> { "execute": "blockdev-snapshot-sync",
 #  "arguments": { "device": "ide-hd0",
@@ -1711,7 +1711,7 @@
 #
 # Since: 2.5
 #
-# Example:
+# .. qmp-example::
 #
 # -> { "execute": "blockdev-add",
 #  "arguments": { "driver": "qcow2",
@@ -1857,7 +1857,7 @@
 #
 # Since: 1.3
 #
-# Example:
+# .. qmp-example::
 #
 # -> { "execute": "block-commit",
 #  "arguments": { "device": "virtio0",
@@ -1895,7 +1895,7 @@
 #
 # Since: 1.6
 #
-# Example:
+# .. qmp-example::
 #
 # -> { "execute": "drive-backup",
 #  "arguments": { "device": "drive0",
@@ -1921,7 +1921,7 @@
 #
 # Since: 2.3
 #
-# Example:
+# .. qmp-example::
 #
 # -> { "execute": "blockdev-backup",
 #  "arguments": { "device": "src-id",
@@ -1945,7 +1945,7 @@
 #
 # Since: 2.0
 #
-# Example:
+# .. qmp-example::
 #
 # -> { "execute": "query-named-block-nodes" }
 # <- { "return": [ { "ro":false,
@@ -2126,7 +2126,7 @@
 #
 # Since: 1.3
 #
-# Example:
+# .. qmp-example::
 #
 # -> { "execute": "drive-mirror",
 #  "arguments": { "device": "ide-hd0",
@@ -2303,7 +2303,7 @@
 #
 # Since: 2.4
 #
-# Example:
+# .. qmp-example::
 #
 # -> { "execute": "block-dirty-bitmap-add",
 #  "arguments": { "node": "drive0", "name": "bitmap0" } }
@@ -2327,7 +2327,7 @@
 #
 # Since: 2.4
 #
-# Example:
+# .. qmp-example::
 #
 # -> { "execute": "block-dirty-bitmap-remove",
 #  "arguments": { "node": "drive0", "name": "bitmap0" } }
@@ -2350,7 +2350,7 @@
 #
 # Since: 2.4
 #
-# Example:
+# .. qmp-example::
 #
 # -> { "execute": "block-dirty-bitmap-clear",
 #  "arguments": { "node": "drive0", "name": "bitmap0" } }
@@ -2371,7 +2371,7 @@
 #
 # Since: 4.0
 #
-# Example:
+# ..

[PULL 06/14] qapi/ui: Drop note on naming of SpiceQueryMouseMode

[Markus Armbruster]

Doc comments are reference documentation for users of QMP.
SpiceQueryMouseMode's doc comment contains a note explaining why it's
not named SpiceMouseMode: spice/enums.h has it already.  Irrelevant
for users of QMP; delete the note.

Signed-off-by: Markus Armbruster 
Message-ID: <2024072228.2140606-6-arm...@redhat.com>
Reviewed-by: John Snow 
---
 qapi/ui.json | 2 --
 1 file changed, 2 deletions(-)

diff --git a/qapi/ui.json b/qapi/ui.json
index 5bcccbfc93..df089869a5 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -273,8 +273,6 @@
 # @unknown: No information is available about mouse mode used by the
 # spice server.
 #
-# .. note:: spice/enums.h has a SpiceMouseMode already, hence the name.
-#
 # Since: 1.1
 ##
 { 'enum': 'SpiceQueryMouseMode',
-- 
2.45.0

[PULL 01/14] qapi/qom: Document feature unstable of @x-vfio-user-server

[Markus Armbruster]

Commit 8f9a9259d32c added ObjectType member @x-vfio-user-server with
feature unstable, but neglected to explain why it is unstable.  Do
that now.

Fixes: 8f9a9259d32c (vfio-user: define vfio-user-server object)
Cc: Elena Ufimtseva 
Cc: John G Johnson 
Cc: Jagannathan Raman 
Cc: qemu-sta...@nongnu.org
Signed-off-by: Markus Armbruster 
Message-ID: <20240703095310.1242102-1-arm...@redhat.com>
Reviewed-by: John Snow 
[Indentation fixed]
---
 qapi/qom.json | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/qapi/qom.json b/qapi/qom.json
index 8e75a419c3..3e52aec8fb 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -1024,7 +1024,8 @@
 #
 # Features:
 #
-# @unstable: Member @x-remote-object is experimental.
+# @unstable: Members @x-remote-object and @x-vfio-user-server are
+# experimental.
 #
 # Since: 6.0
 ##
-- 
2.45.0

[PULL 03/14] qapi/machine: Clean up documentation around CpuInstanceProperties

[Markus Armbruster]

CpuInstanceProperties' doc comment describes its members as properties
to be passed to device_add when hot-plugging a CPU.

This was in fact the initial use of this type, with
query-hotpluggable-cpus: letting management applications find out what
properties need to be passed with device_add to hot-plug a CPU.

We've since added other uses: set-numa-node (commit 419fcdec3c1 and
f3be67812c2), and query-cpus-fast (commit ce74ee3dea6).  These are not
about device-add.

query-hotpluggable-cpus uses CpuInstanceProperties within
HotpluggableCPU.  Lift the documentation related to device-add from
CpuInstanceProperties to HotpluggableCPU.

Signed-off-by: Markus Armbruster 
Message-ID: <2024072228.2140606-3-arm...@redhat.com>
Reviewed-by: Michael S. Tsirkin 
Reviewed-by: John Snow 
---
 qapi/machine.json | 13 ++---
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/qapi/machine.json b/qapi/machine.json
index f15ad1b43e..50ff102d56 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -960,9 +960,7 @@
 ##
 # @CpuInstanceProperties:
 #
-# List of properties to be used for hotplugging a CPU instance, it
-# should be passed by management with device_add command when a CPU is
-# being hotplugged.
+# Properties identifying a CPU.
 #
 # Which members are optional and which mandatory depends on the
 # architecture and board.
@@ -996,9 +994,6 @@
 #
 # @thread-id: thread number within the core the CPU  belongs to
 #
-# .. note:: Management should be prepared to pass through additional
-#properties with device_add.
-#
 # Since: 2.7
 ##
 { 'struct': 'CpuInstanceProperties',
@@ -1020,7 +1015,8 @@
 #
 # @type: CPU object type for usage with device_add command
 #
-# @props: list of properties to be used for hotplugging CPU
+# @props: list of properties to pass for hotplugging a CPU with
+# device_add
 #
 # @vcpus-count: number of logical VCPU threads @HotpluggableCPU
 # provides
@@ -1028,6 +1024,9 @@
 # @qom-path: link to existing CPU object if CPU is present or omitted
 # if CPU is not present.
 #
+# .. note:: Management should be prepared to pass through additional
+#properties with device_add.
+#
 # Since: 2.7
 ##
 { 'struct': 'HotpluggableCPU',
-- 
2.45.0

[PATCH v4 0/8] semihosting: Restrict to TCG

[Philippe Mathieu-Daudé]

v4: Address Paolo's comment
v3: Address Anton's comment
v2: Address Paolo's comment

Semihosting currently uses the TCG probe_access API,
so it is pointless to have it in the binary when TCG
isn't.

It could be implemented for other accelerators, but
work need to be done. Meanwhile, do not enable it
unless TCG is available.

Philippe Mathieu-Daudé (8):
  semihosting: Include missing 'gdbstub/syscalls.h' header
  target/m68k: Add semihosting stub
  target/mips: Add semihosting stub
  target/m68k: Restrict semihosting to TCG
  target/mips: Restrict semihosting to TCG
  target/riscv: Restrict semihosting to TCG
  target/xtensa: Restrict semihosting to TCG
  semihosting: Restrict to TCG

 include/semihosting/syscalls.h|  2 ++
 target/m68k/semihosting-stub.c| 15 +++
 target/mips/tcg/sysemu/semihosting-stub.c | 15 +++
 semihosting/Kconfig   |  1 +
 target/m68k/Kconfig   |  2 +-
 target/m68k/meson.build   |  5 -
 target/mips/Kconfig   |  2 +-
 target/mips/tcg/sysemu/meson.build|  6 --
 target/riscv/Kconfig  |  4 ++--
 target/xtensa/Kconfig |  2 +-
 10 files changed, 46 insertions(+), 8 deletions(-)
 create mode 100644 target/m68k/semihosting-stub.c
 create mode 100644 target/mips/tcg/sysemu/semihosting-stub.c

-- 
2.41.0

[PATCH v4 8/8] semihosting: Restrict to TCG

[Philippe Mathieu-Daudé]

Semihosting currently uses the TCG probe_access API.
It is pointless to have it in the binary when TCG isn't.

Signed-off-by: Philippe Mathieu-Daudé 
Reviewed-by: Richard Henderson 
---
 semihosting/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/semihosting/Kconfig b/semihosting/Kconfig
index eaf3a20ef5..fbe6ac87f9 100644
--- a/semihosting/Kconfig
+++ b/semihosting/Kconfig
@@ -1,6 +1,7 @@
 
 config SEMIHOSTING
bool
+   depends on TCG
 
 config ARM_COMPATIBLE_SEMIHOSTING
bool
-- 
2.41.0

[PATCH v4 5/8] target/mips: Restrict semihosting to TCG

[Philippe Mathieu-Daudé]

Semihosting currently uses the TCG probe_access API. To prepare for
encoding the TCG dependency in Kconfig, do not enable it unless TCG
is available.

Suggested-by: Paolo Bonzini 
Signed-off-by: Philippe Mathieu-Daudé 
Reviewed-by: Anton Johansson 
---
 target/mips/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/mips/Kconfig b/target/mips/Kconfig
index eb19c94c7d..876048b150 100644
--- a/target/mips/Kconfig
+++ b/target/mips/Kconfig
@@ -1,6 +1,6 @@
 config MIPS
 bool
-select SEMIHOSTING
+imply SEMIHOSTING if TCG
 
 config MIPS64
 bool
-- 
2.41.0

[PATCH v4 6/8] target/riscv: Restrict semihosting to TCG

[Philippe Mathieu-Daudé]

Semihosting currently uses the TCG probe_access API. To prepare for
encoding the TCG dependency in Kconfig, do not enable it unless TCG
is available.

Suggested-by: Paolo Bonzini 
Signed-off-by: Philippe Mathieu-Daudé 
Reviewed-by: Anton Johansson 
---
 target/riscv/Kconfig | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/riscv/Kconfig b/target/riscv/Kconfig
index 5f30df22f2..c332616d36 100644
--- a/target/riscv/Kconfig
+++ b/target/riscv/Kconfig
@@ -1,9 +1,9 @@
 config RISCV32
 bool
-select ARM_COMPATIBLE_SEMIHOSTING # for do_common_semihosting()
+imply ARM_COMPATIBLE_SEMIHOSTING if TCG
 select DEVICE_TREE # needed by boot.c
 
 config RISCV64
 bool
-select ARM_COMPATIBLE_SEMIHOSTING # for do_common_semihosting()
+imply ARM_COMPATIBLE_SEMIHOSTING if TCG
 select DEVICE_TREE # needed by boot.c
-- 
2.41.0

[PATCH v4 3/8] target/mips: Add semihosting stub

[Philippe Mathieu-Daudé]

Since the SEMIHOSTING feature is optional, we need
a stub to link when it is disabled.

Signed-off-by: Philippe Mathieu-Daudé 
---
 target/mips/tcg/sysemu/semihosting-stub.c | 15 +++
 target/mips/tcg/sysemu/meson.build|  6 --
 2 files changed, 19 insertions(+), 2 deletions(-)
 create mode 100644 target/mips/tcg/sysemu/semihosting-stub.c

diff --git a/target/mips/tcg/sysemu/semihosting-stub.c 
b/target/mips/tcg/sysemu/semihosting-stub.c
new file mode 100644
index 00..7ae27d746f
--- /dev/null
+++ b/target/mips/tcg/sysemu/semihosting-stub.c
@@ -0,0 +1,15 @@
+/*
+ *  MIPS semihosting stub
+ *
+ * SPDX-FileContributor: Philippe Mathieu-Daudé 
+ * SPDX-FileCopyrightText: 2024 Linaro Ltd.
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "internal.h"
+
+void mips_semihosting(CPUMIPSState *env)
+{
+g_assert_not_reached();
+}
diff --git a/target/mips/tcg/sysemu/meson.build 
b/target/mips/tcg/sysemu/meson.build
index ec665a4b1e..911341ac37 100644
--- a/target/mips/tcg/sysemu/meson.build
+++ b/target/mips/tcg/sysemu/meson.build
@@ -1,10 +1,12 @@
 mips_system_ss.add(files(
   'cp0_helper.c',
-  'mips-semi.c',
   'special_helper.c',
   'tlb_helper.c',
 ))
-
+mips_system_ss.add(when: ['CONFIG_SEMIHOSTING'],
+  if_true: files('mips-semi.c'),
+  if_false: files('semihosting-stub.c')
+)
 mips_system_ss.add(when: 'TARGET_MIPS64', if_true: files(
   'lcsr_helper.c',
 ))
-- 
2.41.0

[PATCH v4 4/8] target/m68k: Restrict semihosting to TCG

[Philippe Mathieu-Daudé]

The semihosting feature depends on TCG (due to the probe_access
API access). Although TCG is the single accelerator currently
available for the m68k target, use the Kconfig "imply" directive
which is more correct (if we were to support a different accel).

Reported-by: Anton Johansson 
Signed-off-by: Philippe Mathieu-Daudé 
---
 target/m68k/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/m68k/Kconfig b/target/m68k/Kconfig
index 9eae71486f..23aae24ebe 100644
--- a/target/m68k/Kconfig
+++ b/target/m68k/Kconfig
@@ -1,3 +1,3 @@
 config M68K
 bool
-select SEMIHOSTING
+imply SEMIHOSTING if TCG
-- 
2.41.0

[PATCH v4 1/8] semihosting: Include missing 'gdbstub/syscalls.h' header

[Philippe Mathieu-Daudé]

"semihosting/syscalls.h" requires definitions from
"gdbstub/syscalls.h", include it in order to avoid:

  include/semihosting/syscalls.h:23:38: error: unknown type name 
'gdb_syscall_complete_cb'
  void semihost_sys_open(CPUState *cs, gdb_syscall_complete_cb complete,
   ^

Signed-off-by: Philippe Mathieu-Daudé 
---
 include/semihosting/syscalls.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/semihosting/syscalls.h b/include/semihosting/syscalls.h
index 3a5ec229eb..b5937c619a 100644
--- a/include/semihosting/syscalls.h
+++ b/include/semihosting/syscalls.h
@@ -9,6 +9,8 @@
 #ifndef SEMIHOSTING_SYSCALLS_H
 #define SEMIHOSTING_SYSCALLS_H
 
+#include "gdbstub/syscalls.h"
+
 /*
  * Argument loading from the guest is performed by the caller;
  * results are returned via the 'complete' callback.
-- 
2.41.0

[PATCH v4 7/8] target/xtensa: Restrict semihosting to TCG

[Philippe Mathieu-Daudé]

The semihosting feature depends on TCG (due to the probe_access
API access). Although TCG is the single accelerator currently
available for the xtensa target, use the Kconfig "imply" directive
which is more correct (if we were to support a different accel).

Reported-by: Anton Johansson 
Signed-off-by: Philippe Mathieu-Daudé 
---
 target/xtensa/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/xtensa/Kconfig b/target/xtensa/Kconfig
index 5e46049262..e8c2598c4d 100644
--- a/target/xtensa/Kconfig
+++ b/target/xtensa/Kconfig
@@ -1,3 +1,3 @@
 config XTENSA
 bool
-select SEMIHOSTING
+imply SEMIHOSTING if TCG
-- 
2.41.0

[PATCH v4 2/8] target/m68k: Add semihosting stub

[Philippe Mathieu-Daudé]

Since the SEMIHOSTING feature is optional, we need
a stub to link when it is disabled.

Suggested-by: Paolo Bonzini 
Signed-off-by: Philippe Mathieu-Daudé 
---
 target/m68k/semihosting-stub.c | 15 +++
 target/m68k/meson.build|  5 -
 2 files changed, 19 insertions(+), 1 deletion(-)
 create mode 100644 target/m68k/semihosting-stub.c

diff --git a/target/m68k/semihosting-stub.c b/target/m68k/semihosting-stub.c
new file mode 100644
index 00..d6a5965e29
--- /dev/null
+++ b/target/m68k/semihosting-stub.c
@@ -0,0 +1,15 @@
+/*
+ *  m68k/ColdFire semihosting stub
+ *
+ * SPDX-FileContributor: Philippe Mathieu-Daudé 
+ * SPDX-FileCopyrightText: 2024 Linaro Ltd.
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "cpu.h"
+
+void do_m68k_semihosting(CPUM68KState *env, int nr)
+{
+g_assert_not_reached();
+}
diff --git a/target/m68k/meson.build b/target/m68k/meson.build
index 8d3f9ce288..4d213daaf6 100644
--- a/target/m68k/meson.build
+++ b/target/m68k/meson.build
@@ -11,9 +11,12 @@ m68k_ss.add(files(
 
 m68k_system_ss = ss.source_set()
 m68k_system_ss.add(files(
-  'm68k-semi.c',
   'monitor.c'
 ))
+m68k_system_ss.add(when: ['CONFIG_SEMIHOSTING'],
+  if_true: files('m68k-semi.c'),
+  if_false: files('semihosting-stub.c')
+)
 
 target_arch += {'m68k': m68k_ss}
 target_system_arch += {'m68k': m68k_system_ss}
-- 
2.41.0

[PATCH 08/26] hw/display/apple-gfx: Adds migration blocker

[Phil Dennis-Jordan]

Although the underlying ParavirtualizedGraphics.framework does support
state (de-)serialisation, this is currently not yet integrated. We
therefore add a migration blocker for now.

Signed-off-by: Phil Dennis-Jordan 
---
 hw/display/apple-gfx.m | 16 
 1 file changed, 16 insertions(+)

diff --git a/hw/display/apple-gfx.m b/hw/display/apple-gfx.m
index 39aba8d143..c97bd40cb5 100644
--- a/hw/display/apple-gfx.m
+++ b/hw/display/apple-gfx.m
@@ -24,6 +24,8 @@
 #include "sysemu/cpus.h"
 #include "ui/console.h"
 #include "monitor/monitor.h"
+#include "qapi/error.h"
+#include "migration/blocker.h"
 #import 
 
 #define TYPE_APPLE_GFX  "apple-gfx"
@@ -109,6 +111,8 @@ -(void)mmioWriteAtOffset:(size_t) offset 
value:(uint32_t)value;
 
 OBJECT_DECLARE_SIMPLE_TYPE(AppleGFXState, APPLE_GFX)
 
+static Error *apple_gfx_mig_blocker;
+
 static AppleGFXTask *apple_gfx_new_task(AppleGFXState *s, uint64_t len)
 {
 void *base = APPLE_GFX_BASE_VA;
@@ -362,6 +366,8 @@ static void apple_gfx_reset(DeviceState *d)
 static void apple_gfx_init(Object *obj)
 {
 AppleGFXState *s = APPLE_GFX(obj);
+Error *local_err = NULL;
+int r;
 
 memory_region_init_io(&s->iomem_gfx, obj, &apple_gfx_ops, s, 
TYPE_APPLE_GFX, 0x4000);
 memory_region_init_io(&s->iomem_iosfc, obj, &apple_iosfc_ops, s, 
TYPE_APPLE_GFX, 0x1);
@@ -369,6 +375,16 @@ static void apple_gfx_init(Object *obj)
 sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->iomem_iosfc);
 sysbus_init_irq(SYS_BUS_DEVICE(s), &s->irq_gfx);
 sysbus_init_irq(SYS_BUS_DEVICE(s), &s->irq_iosfc);
+
+/* TODO: PVG framework supports serialising device state: integrate it! */
+if (apple_gfx_mig_blocker == NULL) {
+error_setg(&apple_gfx_mig_blocker,
+  "Migration state blocked by apple-gfx display device");
+r = migrate_add_blocker(&apple_gfx_mig_blocker, &local_err);
+if (r < 0) {
+error_report_err(local_err);
+}
+}
 }
 
 static void apple_gfx_realize(DeviceState *dev, Error **errp)
-- 
2.39.3 (Apple Git-146)

[PATCH 15/26] hw/display/apple-gfx: Separates generic & vmapple-specific functionality

[Phil Dennis-Jordan]

The apple-gfx paravirtualised graphics device can be used in a
MMIO mode with the 'vmapple' arm64 machine model, but also as
a PCI device, especially on x86-64 VMs. There are some
significant differences between these implementations, but
even more shared functionality.

This change prepares for a PCI based implementation by
splitting out the vmapple-specific code into a separate
code file.

Signed-off-by: Phil Dennis-Jordan 
---
 hw/display/Kconfig |   4 +
 hw/display/apple-gfx-vmapple.m | 194 +
 hw/display/apple-gfx.h |  47 +++
 hw/display/apple-gfx.m | 221 +++--
 hw/display/meson.build |   3 +-
 hw/display/trace-events|   2 +
 6 files changed, 266 insertions(+), 205 deletions(-)
 create mode 100644 hw/display/apple-gfx-vmapple.m
 create mode 100644 hw/display/apple-gfx.h

diff --git a/hw/display/Kconfig b/hw/display/Kconfig
index 13cd256c06..e3d10bf6ff 100644
--- a/hw/display/Kconfig
+++ b/hw/display/Kconfig
@@ -146,4 +146,8 @@ config DM163
 
 config MAC_PVG
 bool
+default y
 
+config MAC_PVG_VMAPPLE
+bool
+depends on MAC_PVG
diff --git a/hw/display/apple-gfx-vmapple.m b/hw/display/apple-gfx-vmapple.m
new file mode 100644
index 00..6af8b7a292
--- /dev/null
+++ b/hw/display/apple-gfx-vmapple.m
@@ -0,0 +1,194 @@
+#include "apple-gfx.h"
+#include "monitor/monitor.h"
+#include "hw/sysbus.h"
+#include "hw/irq.h"
+#include "trace.h"
+#import 
+
+_Static_assert(__aarch64__, "");
+
+/*
+ * ParavirtualizedGraphics.Framework only ships header files for the x86
+ * variant which does not include IOSFC descriptors and host devices. We add
+ * their definitions here so that we can also work with the ARM version.
+ */
+typedef bool(^IOSFCRaiseInterrupt)(uint32_t vector);
+typedef bool(^IOSFCUnmapMemory)(
+void *a, void *b, void *c, void *d, void *e, void *f);
+typedef bool(^IOSFCMapMemory)(
+uint64_t phys, uint64_t len, bool ro, void **va, void *e, void *f);
+
+@interface PGDeviceDescriptor (IOSurfaceMapper)
+@property (readwrite, nonatomic) bool usingIOSurfaceMapper;
+@end
+
+@interface PGIOSurfaceHostDeviceDescriptor : NSObject
+-(PGIOSurfaceHostDeviceDescriptor *)init;
+@property (readwrite, nonatomic, copy, nullable) IOSFCMapMemory mapMemory;
+@property (readwrite, nonatomic, copy, nullable) IOSFCUnmapMemory unmapMemory;
+@property (readwrite, nonatomic, copy, nullable) IOSFCRaiseInterrupt 
raiseInterrupt;
+@end
+
+@interface PGIOSurfaceHostDevice : NSObject
+-(instancetype)initWithDescriptor:(PGIOSurfaceHostDeviceDescriptor *) desc;
+-(uint32_t)mmioReadAtOffset:(size_t) offset;
+-(void)mmioWriteAtOffset:(size_t) offset value:(uint32_t)value;
+@end
+
+typedef struct AppleGFXVmappleState {
+SysBusDevice parent_obj;
+
+AppleGFXState common;
+
+qemu_irq irq_gfx;
+qemu_irq irq_iosfc;
+MemoryRegion iomem_iosfc;
+PGIOSurfaceHostDevice *pgiosfc;
+} AppleGFXVmappleState;
+
+OBJECT_DECLARE_SIMPLE_TYPE(AppleGFXVmappleState, APPLE_GFX_VMAPPLE)
+
+
+static uint64_t apple_iosfc_read(void *opaque, hwaddr offset, unsigned size)
+{
+AppleGFXVmappleState *s = opaque;
+uint64_t res = 0;
+
+bql_unlock();
+res = [s->pgiosfc mmioReadAtOffset:offset];
+bql_lock();
+
+trace_apple_iosfc_read(offset, res);
+
+return res;
+}
+
+static void apple_iosfc_write(
+void *opaque, hwaddr offset, uint64_t val, unsigned size)
+{
+AppleGFXVmappleState *s = opaque;
+
+trace_apple_iosfc_write(offset, val);
+
+[s->pgiosfc mmioWriteAtOffset:offset value:val];
+}
+
+static const MemoryRegionOps apple_iosfc_ops = {
+.read = apple_iosfc_read,
+.write = apple_iosfc_write,
+.endianness = DEVICE_LITTLE_ENDIAN,
+.valid = {
+.min_access_size = 4,
+.max_access_size = 8,
+},
+.impl = {
+.min_access_size = 4,
+.max_access_size = 8,
+},
+};
+
+static PGIOSurfaceHostDevice *apple_gfx_prepare_iosurface_host_device(
+AppleGFXVmappleState *s)
+{
+PGIOSurfaceHostDeviceDescriptor *iosfc_desc =
+[PGIOSurfaceHostDeviceDescriptor new];
+PGIOSurfaceHostDevice *iosfc_host_dev = nil;
+
+iosfc_desc.mapMemory =
+^(uint64_t phys, uint64_t len, bool ro, void **va, void *e, void *f) {
+trace_apple_iosfc_map_memory(phys, len, ro, va, e, f);
+MemoryRegion *tmp_mr;
+*va = gpa2hva(&tmp_mr, phys, len, NULL);
+return (bool)true;
+};
+
+iosfc_desc.unmapMemory =
+^(void *a, void *b, void *c, void *d, void *e, void *f) {
+trace_apple_iosfc_unmap_memory(a, b, c, d, e, f);
+return (bool)true;
+};
+
+iosfc_desc.raiseInterrupt = ^(uint32_t vector) {
+trace_apple_iosfc_raise_irq(vector);
+bool locked = bql_locked();
+if (!locked) {
+bql_lock();
+}
+qemu_irq_pulse(s->irq_iosfc);
+if (!locked) {
+bql_unlock();
+}
+retu

[PATCH v2 7/8] hw/display/apple-gfx: Adds configurable mode list

[Phil Dennis-Jordan]

This change adds a property 'display_modes' on the graphics device
which permits specifying a list of display modes. (screen resolution
and refresh rate)

PCI variant of apple-gfx only for the moment.

Signed-off-by: Phil Dennis-Jordan 
---
 hw/display/apple-gfx-pci.m |  43 ++-
 hw/display/apple-gfx.h |  17 -
 hw/display/apple-gfx.m | 151 ++---
 3 files changed, 198 insertions(+), 13 deletions(-)

diff --git a/hw/display/apple-gfx-pci.m b/hw/display/apple-gfx-pci.m
index b3311e736c..942fba16a2 100644
--- a/hw/display/apple-gfx-pci.m
+++ b/hw/display/apple-gfx-pci.m
@@ -1,6 +1,7 @@
 #include "apple-gfx.h"
 #include "hw/pci/pci_device.h"
 #include "hw/pci/msi.h"
+#include "hw/qdev-properties.h"
 #include "qapi/error.h"
 #include "trace.h"
 #import 
@@ -86,6 +87,46 @@ static void apple_gfx_pci_reset(DeviceState *dev)
 [s->common.pgdev reset];
 }
 
+static void apple_gfx_pci_get_display_modes(Object *obj, Visitor *v,
+const char *name, void *opaque,
+Error **errp)
+{
+Property *prop = opaque;
+AppleGFXDisplayModeList *mode_list = object_field_prop_ptr(obj, prop);
+
+apple_gfx_get_display_modes(mode_list, v, name, errp);
+}
+
+static void apple_gfx_pci_set_display_modes(Object *obj, Visitor *v,
+const char *name, void *opaque,
+Error **errp)
+{
+Property *prop = opaque;
+AppleGFXDisplayModeList *mode_list = object_field_prop_ptr(obj, prop);
+
+apple_gfx_set_display_modes(mode_list, v, name, errp);
+}
+
+const PropertyInfo apple_gfx_pci_prop_display_modes = {
+.name  = "display_modes",
+.description =
+"Colon-separated list of display modes; "
+"x@; the first mode is considered "
+"'native'. Example: 3840x2160@60:2560x1440@60:1920x1080@60",
+.get   = apple_gfx_pci_get_display_modes,
+.set   = apple_gfx_pci_set_display_modes,
+};
+
+#define DEFINE_PROP_DISPLAY_MODES(_name, _state, _field) \
+DEFINE_PROP(_name, _state, _field, apple_gfx_pci_prop_display_modes, \
+AppleGFXDisplayModeList)
+
+static Property apple_gfx_pci_properties[] = {
+DEFINE_PROP_DISPLAY_MODES("display-modes", AppleGFXPCIState,
+  common.display_modes),
+DEFINE_PROP_END_OF_LIST(),
+};
+
 static void apple_gfx_pci_class_init(ObjectClass *klass, void *data)
 {
 DeviceClass *dc = DEVICE_CLASS(klass);
@@ -101,7 +142,7 @@ static void apple_gfx_pci_class_init(ObjectClass *klass, 
void *data)
 pci->class_id = PCI_CLASS_DISPLAY_OTHER;
 pci->realize = apple_gfx_pci_realize;
 
-// TODO: Property for setting mode list
+device_class_set_props(dc, apple_gfx_pci_properties);
 }
 
 static TypeInfo apple_gfx_pci_types[] = {
diff --git a/hw/display/apple-gfx.h b/hw/display/apple-gfx.h
index 995ecf7f4a..baad4a9865 100644
--- a/hw/display/apple-gfx.h
+++ b/hw/display/apple-gfx.h
@@ -5,14 +5,28 @@
 #define TYPE_APPLE_GFX_PCI  "apple-gfx-pci"
 
 #include "qemu/typedefs.h"
+#include "qemu/osdep.h"
 
 typedef struct AppleGFXState AppleGFXState;
 
+typedef struct AppleGFXDisplayMode {
+uint16_t width_px;
+uint16_t height_px;
+uint16_t refresh_rate_hz;
+} AppleGFXDisplayMode;
+
+typedef struct AppleGFXDisplayModeList {
+GArray *modes;
+} AppleGFXDisplayModeList;
+
 void apple_gfx_common_init(Object *obj, AppleGFXState *s, const char* 
obj_name);
+void apple_gfx_get_display_modes(AppleGFXDisplayModeList *mode_list, Visitor 
*v,
+ const char *name, Error **errp);
+void apple_gfx_set_display_modes(AppleGFXDisplayModeList *mode_list, Visitor 
*v,
+ const char *name, Error **errp);
 
 #ifdef __OBJC__
 
-#include "qemu/osdep.h"
 #include "exec/memory.h"
 #include "ui/surface.h"
 #include 
@@ -38,6 +52,7 @@ struct AppleGFXState {
 bool new_frame;
 bool cursor_show;
 QEMUCursor *cursor;
+AppleGFXDisplayModeList display_modes;
 
 dispatch_queue_t render_queue;
 /* The following fields should only be accessed from render_queue: */
diff --git a/hw/display/apple-gfx.m b/hw/display/apple-gfx.m
index 018db8bf19..de6dac9e04 100644
--- a/hw/display/apple-gfx.m
+++ b/hw/display/apple-gfx.m
@@ -16,6 +16,9 @@
 #include "trace.h"
 #include "qemu-main.h"
 #include "qemu/main-loop.h"
+#include "qemu/cutils.h"
+#include "qapi/visitor.h"
+#include "qapi/error.h"
 #include "ui/console.h"
 #include "monitor/monitor.h"
 #include "qapi/error.h"
@@ -23,9 +26,10 @@
 #include 
 #import 
 
-static const PGDisplayCoord_t apple_gfx_modes[] = {
-{ .x = 1440, .y = 1080 },
-{ .x = 1280, .y = 1024 },
+static const AppleGFXDisplayMode apple_gfx_default_modes[] = {
+{ 1920, 1080, 60 },
+{ 1440, 1080, 60 },
+{ 1280, 1024, 60 },
 };
 
 typedef struct PGTask_s { // Name matches forward declaration in PG header
@@ -27

[PATCH 07/26] hw/display/apple-gfx: Makes set_mode thread & memory safe

[Phil Dennis-Jordan]

When the set_mode callback was invoked outside of the BQL, there
could be a race condition swapping out the resized render target
texture and VRAM. set_mode may be called inside or out of the
BQL depending on context (reentrant from a MMIO write or not)
so we need to check locking state first.

Signed-off-by: Phil Dennis-Jordan 
---
 hw/display/apple-gfx.m | 54 +++---
 1 file changed, 40 insertions(+), 14 deletions(-)

diff --git a/hw/display/apple-gfx.m b/hw/display/apple-gfx.m
index b10c060d9a..39aba8d143 100644
--- a/hw/display/apple-gfx.m
+++ b/hw/display/apple-gfx.m
@@ -290,34 +290,60 @@ static void update_cursor(AppleGFXState *s)
 
 static void set_mode(AppleGFXState *s, uint32_t width, uint32_t height)
 {
-void *vram = g_malloc0(width * height * 4);
+void *vram = NULL;
 void *old_vram = s->vram;
 DisplaySurface *surface;
 MTLTextureDescriptor *textureDescriptor;
-id old_texture = s->texture;
+id old_texture = nil;
+id texture = nil;
+bool locking_required = false;
 
+locking_required = !bql_locked();
+if (locking_required) {
+bql_lock();
+}
 if (s->surface &&
 width == surface_width(s->surface) &&
 height == surface_height(s->surface)) {
+if (locking_required) {
+bql_unlock();
+}
 return;
 }
+if (locking_required) {
+bql_unlock();
+}
+
+vram = g_malloc0(width * height * 4);
 surface = qemu_create_displaysurface_from(width, height, 
PIXMAN_LE_a8r8g8b8,
   width * 4, vram);
-s->surface = surface;
-dpy_gfx_replace_surface(s->con, surface);
-s->vram = vram;
-g_free(old_vram);
 
-textureDescriptor = [MTLTextureDescriptor 
texture2DDescriptorWithPixelFormat:MTLPixelFormatBGRA8Unorm
-  width:width
-  height:height
-  mipmapped:NO];
-textureDescriptor.usage = s->pgdisp.minimumTextureUsage;
-s->texture = [s->mtl newTextureWithDescriptor:textureDescriptor];
+@autoreleasepool {
+textureDescriptor =
+[MTLTextureDescriptor
+texture2DDescriptorWithPixelFormat:MTLPixelFormatBGRA8Unorm
+ width:width
+height:height
+ mipmapped:NO];
+textureDescriptor.usage = s->pgdisp.minimumTextureUsage;
+texture = [s->mtl newTextureWithDescriptor:textureDescriptor];
+}
 
-if (old_texture) {
-[old_texture release];
+if (locking_required) {
+bql_lock();
+}
+old_vram = s->vram;
+s->vram = vram;
+s->surface = surface;
+dpy_gfx_replace_surface(s->con, surface);
+old_texture = s->texture;
+s->texture = texture;
+if (locking_required) {
+bql_unlock();
 }
+
+g_free(old_vram);
+[old_texture release];
 }
 
 static void create_fb(AppleGFXState *s)
-- 
2.39.3 (Apple Git-146)

[PATCH 24/26] hw/display/apple-gfx: Adds configurable mode list

[Phil Dennis-Jordan]

This change adds a property 'display_modes' on the graphics device
which permits specifying a list of display modes. (screen resolution
and refresh rate)

PCI variant of apple-gfx only for the moment.

Signed-off-by: Phil Dennis-Jordan 
---
 hw/display/apple-gfx-pci.m |  43 ++-
 hw/display/apple-gfx.h |  17 -
 hw/display/apple-gfx.m | 151 ++---
 3 files changed, 198 insertions(+), 13 deletions(-)

diff --git a/hw/display/apple-gfx-pci.m b/hw/display/apple-gfx-pci.m
index bdbab35eed..a8205093ab 100644
--- a/hw/display/apple-gfx-pci.m
+++ b/hw/display/apple-gfx-pci.m
@@ -1,6 +1,7 @@
 #include "apple-gfx.h"
 #include "hw/pci/pci_device.h"
 #include "hw/pci/msi.h"
+#include "hw/qdev-properties.h"
 #include "qapi/error.h"
 #include "trace.h"
 #import 
@@ -90,6 +91,46 @@ static void apple_gfx_pci_reset(DeviceState *dev)
 }
 }
 
+static void apple_gfx_pci_get_display_modes(Object *obj, Visitor *v,
+const char *name, void *opaque,
+Error **errp)
+{
+Property *prop = opaque;
+AppleGFXDisplayModeList *mode_list = object_field_prop_ptr(obj, prop);
+
+apple_gfx_get_display_modes(mode_list, v, name, errp);
+}
+
+static void apple_gfx_pci_set_display_modes(Object *obj, Visitor *v,
+const char *name, void *opaque,
+Error **errp)
+{
+Property *prop = opaque;
+AppleGFXDisplayModeList *mode_list = object_field_prop_ptr(obj, prop);
+
+apple_gfx_set_display_modes(mode_list, v, name, errp);
+}
+
+const PropertyInfo apple_gfx_pci_prop_display_modes = {
+.name  = "display_modes",
+.description =
+"Colon-separated list of display modes; "
+"x@; the first mode is considered "
+"'native'. Example: 3840x2160@60:2560x1440@60:1920x1080@60",
+.get   = apple_gfx_pci_get_display_modes,
+.set   = apple_gfx_pci_set_display_modes,
+};
+
+#define DEFINE_PROP_DISPLAY_MODES(_name, _state, _field) \
+DEFINE_PROP(_name, _state, _field, apple_gfx_pci_prop_display_modes, \
+AppleGFXDisplayModeList)
+
+static Property apple_gfx_pci_properties[] = {
+DEFINE_PROP_DISPLAY_MODES("display-modes", AppleGFXPCIState,
+  common.display_modes),
+DEFINE_PROP_END_OF_LIST(),
+};
+
 static void apple_gfx_pci_class_init(ObjectClass *klass, void *data)
 {
 DeviceClass *dc = DEVICE_CLASS(klass);
@@ -105,7 +146,7 @@ static void apple_gfx_pci_class_init(ObjectClass *klass, 
void *data)
 pci->class_id = PCI_CLASS_DISPLAY_OTHER;
 pci->realize = apple_gfx_pci_realize;
 
-// TODO: Property for setting mode list
+device_class_set_props(dc, apple_gfx_pci_properties);
 }
 
 static TypeInfo apple_gfx_pci_types[] = {
diff --git a/hw/display/apple-gfx.h b/hw/display/apple-gfx.h
index 995ecf7f4a..baad4a9865 100644
--- a/hw/display/apple-gfx.h
+++ b/hw/display/apple-gfx.h
@@ -5,14 +5,28 @@
 #define TYPE_APPLE_GFX_PCI  "apple-gfx-pci"
 
 #include "qemu/typedefs.h"
+#include "qemu/osdep.h"
 
 typedef struct AppleGFXState AppleGFXState;
 
+typedef struct AppleGFXDisplayMode {
+uint16_t width_px;
+uint16_t height_px;
+uint16_t refresh_rate_hz;
+} AppleGFXDisplayMode;
+
+typedef struct AppleGFXDisplayModeList {
+GArray *modes;
+} AppleGFXDisplayModeList;
+
 void apple_gfx_common_init(Object *obj, AppleGFXState *s, const char* 
obj_name);
+void apple_gfx_get_display_modes(AppleGFXDisplayModeList *mode_list, Visitor 
*v,
+ const char *name, Error **errp);
+void apple_gfx_set_display_modes(AppleGFXDisplayModeList *mode_list, Visitor 
*v,
+ const char *name, Error **errp);
 
 #ifdef __OBJC__
 
-#include "qemu/osdep.h"
 #include "exec/memory.h"
 #include "ui/surface.h"
 #include 
@@ -38,6 +52,7 @@ struct AppleGFXState {
 bool new_frame;
 bool cursor_show;
 QEMUCursor *cursor;
+AppleGFXDisplayModeList display_modes;
 
 dispatch_queue_t render_queue;
 /* The following fields should only be accessed from render_queue: */
diff --git a/hw/display/apple-gfx.m b/hw/display/apple-gfx.m
index 6c92f2579b..e0ad784022 100644
--- a/hw/display/apple-gfx.m
+++ b/hw/display/apple-gfx.m
@@ -16,6 +16,9 @@
 #include "trace.h"
 #include "qemu-main.h"
 #include "qemu/main-loop.h"
+#include "qemu/cutils.h"
+#include "qapi/visitor.h"
+#include "qapi/error.h"
 #include "ui/console.h"
 #include "monitor/monitor.h"
 #include "qapi/error.h"
@@ -23,9 +26,10 @@
 #include 
 #import 
 
-static const PGDisplayCoord_t apple_gfx_modes[] = {
-{ .x = 1440, .y = 1080 },
-{ .x = 1280, .y = 1024 },
+static const AppleGFXDisplayMode apple_gfx_default_modes[] = {
+{ 1920, 1080, 60 },
+{ 1440, 1080, 60 },
+{ 1280, 1024, 60 },
 };
 
 typedef struct PGTask_s { // Name matches forward declaration in PG header
@@ -298,7 +302,6 @@ static vo

[PATCH 23/26] hw/display/apple-gfx: Host GPU picking improvements

[Phil Dennis-Jordan]

During startup of the PV graphics device, we need to specify the
host GPU to use for PV acceleration of the guest's graphics
operations.

On a host system, this is trivial: pick the only one. The
MTLCreateSystemDefaultDevice() function will do the right thing
in this case.

It gets a little more complicated on systems with more than one
GPU; first and foremost, this applies to x86-64 MacBook Pros
with 15/16" displays. However, with eGPUs, in theory any x86-64
Mac can gain one or more additional GPUs. In these cases, the
default is often not ideal - usually, discrete GPUs are selected.
In my tests, performance tends to be best with iGPUs, however,
and they are usually also best in terms of energy consumption.

Ideally, we will want to allow the user to manually select a GPU
if they so choose. In this patch, I am interested in picking a
sensible default. Instead of the built-in default logic, it is
now:

 1. Select a GPU with unified memory (iGPU)
 2. If (1) fails, select a GPU that is built-in, not an eGPU.
 3. If (2) fails, fall back to system default.

Signed-off-by: Phil Dennis-Jordan 
---
 hw/display/apple-gfx.m | 28 +++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/hw/display/apple-gfx.m b/hw/display/apple-gfx.m
index cbcbaf0006..6c92f2579b 100644
--- a/hw/display/apple-gfx.m
+++ b/hw/display/apple-gfx.m
@@ -502,6 +502,32 @@ static void 
apple_gfx_register_task_mapping_handlers(AppleGFXState *s,
 return mode_array;
 }
 
+static id copy_suitable_metal_device(void)
+{
+id dev = nil;
+NSArray> *devs = MTLCopyAllDevices();
+
+/* Prefer a unified memory GPU. Failing that, pick a non-removable GPU. */
+for (size_t i = 0; i < devs.count; ++i) {
+if (devs[i].hasUnifiedMemory) {
+dev = devs[i];
+break;
+}
+if (!devs[i].removable) {
+dev = devs[i];
+}
+}
+
+if (dev != nil) {
+[dev retain];
+} else {
+dev = MTLCreateSystemDefaultDevice();
+}
+[devs release];
+
+return dev;
+}
+
 void apple_gfx_common_realize(AppleGFXState *s, PGDeviceDescriptor *desc)
 {
 PGDisplayDescriptor *disp_desc = nil;
@@ -509,7 +535,7 @@ void apple_gfx_common_realize(AppleGFXState *s, 
PGDeviceDescriptor *desc)
 QTAILQ_INIT(&s->tasks);
 s->render_queue = dispatch_queue_create("apple-gfx.render",
 DISPATCH_QUEUE_SERIAL);
-s->mtl = MTLCreateSystemDefaultDevice();
+s->mtl = copy_suitable_metal_device();
 s->mtl_queue = [s->mtl newCommandQueue];
 
 desc.device = s->mtl;
-- 
2.39.3 (Apple Git-146)

[PATCH 01/26] hw/vmapple/apple-gfx: Introduce ParavirtualizedGraphics.Framework support

[Phil Dennis-Jordan]

From: Alexander Graf 

MacOS provides a framework (library) that allows any vmm to implement a
paravirtualized 3d graphics passthrough to the host metal stack called
ParavirtualizedGraphics.Framework (PVG). The library abstracts away
almost every aspect of the paravirtualized device model and only provides
and receives callbacks on MMIO access as well as to share memory address
space between the VM and PVG.

This patch implements a QEMU device that drives PVG for the VMApple
variant of it.

Signed-off-by: Alexander Graf 

Cherry-pick/rebase conflict fixes:
Signed-off-by: Phil Dennis-Jordan 
---
 hw/vmapple/Kconfig  |   4 +
 hw/vmapple/apple-gfx.m  | 578 
 hw/vmapple/meson.build  |   1 +
 hw/vmapple/trace-events |  23 ++
 meson.build |   4 +
 5 files changed, 610 insertions(+)
 create mode 100644 hw/vmapple/Kconfig
 create mode 100644 hw/vmapple/apple-gfx.m
 create mode 100644 hw/vmapple/meson.build
 create mode 100644 hw/vmapple/trace-events

diff --git a/hw/vmapple/Kconfig b/hw/vmapple/Kconfig
new file mode 100644
index 00..5835790a31
--- /dev/null
+++ b/hw/vmapple/Kconfig
@@ -0,0 +1,4 @@
+
+config VMAPPLE_PVG
+bool
+
diff --git a/hw/vmapple/apple-gfx.m b/hw/vmapple/apple-gfx.m
new file mode 100644
index 00..f75da5c610
--- /dev/null
+++ b/hw/vmapple/apple-gfx.m
@@ -0,0 +1,578 @@
+/*
+ * QEMU Apple ParavirtualizedGraphics.framework device
+ *
+ * Copyright © 2023 Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ * ParavirtualizedGraphics.framework is a set of libraries that macOS provides
+ * which implements 3d graphics passthrough to the host as well as a
+ * proprietary guest communication channel to drive it. This device model
+ * implements support to drive that library from within QEMU.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/irq.h"
+#include "migration/vmstate.h"
+#include "qemu/log.h"
+#include "qemu/module.h"
+#include "trace.h"
+#include "hw/sysbus.h"
+#include "hw/pci/msi.h"
+#include "crypto/hash.h"
+#include "sysemu/cpus.h"
+#include "ui/console.h"
+#include "monitor/monitor.h"
+#import 
+
+#define TYPE_APPLE_GFX  "apple-gfx"
+
+#define MAX_MRS 512
+
+static const PGDisplayCoord_t apple_gfx_modes[] = {
+{ .x = 1440, .y = 1080 },
+{ .x = 1280, .y = 1024 },
+};
+
+/*
+ * We have to map PVG memory into our address space. Use the one below
+ * as base start address. In normal linker setups it points to a free
+ * memory range.
+ */
+#define APPLE_GFX_BASE_VA ((void *)(uintptr_t)0x5000UL)
+
+/*
+ * ParavirtualizedGraphics.Framework only ships header files for the x86
+ * variant which does not include IOSFC descriptors and host devices. We add
+ * their definitions here so that we can also work with the ARM version.
+ */
+typedef bool(^IOSFCRaiseInterrupt)(uint32_t vector);
+typedef bool(^IOSFCUnmapMemory)(void *a, void *b, void *c, void *d, void *e, 
void *f);
+typedef bool(^IOSFCMapMemory)(uint64_t phys, uint64_t len, bool ro, void **va, 
void *e, void *f);
+
+@interface PGDeviceDescriptorExt : PGDeviceDescriptor
+@property (readwrite, nonatomic) bool usingIOSurfaceMapper;
+@end
+
+@interface PGIOSurfaceHostDeviceDescriptor : NSObject
+-(PGIOSurfaceHostDeviceDescriptor *)init;
+@property (readwrite, nonatomic, copy, nullable) IOSFCMapMemory mapMemory;
+@property (readwrite, nonatomic, copy, nullable) IOSFCUnmapMemory unmapMemory;
+@property (readwrite, nonatomic, copy, nullable) IOSFCRaiseInterrupt 
raiseInterrupt;
+@end
+
+@interface PGIOSurfaceHostDevice : NSObject
+-(void)initWithDescriptor:(PGIOSurfaceHostDeviceDescriptor *) desc;
+-(uint32_t)mmioReadAtOffset:(size_t) offset;
+-(void)mmioWriteAtOffset:(size_t) offset value:(uint32_t)value;
+@end
+
+typedef struct AppleGFXMR {
+QTAILQ_ENTRY(AppleGFXMR) node;
+hwaddr pa;
+void *va;
+uint64_t len;
+} AppleGFXMR;
+
+typedef QTAILQ_HEAD(, AppleGFXMR) AppleGFXMRList;
+
+typedef struct AppleGFXTask {
+QTAILQ_ENTRY(AppleGFXTask) node;
+void *mem;
+uint64_t len;
+} AppleGFXTask;
+
+typedef QTAILQ_HEAD(, AppleGFXTask) AppleGFXTaskList;
+
+typedef struct AppleGFXState {
+/* Private */
+SysBusDevice parent_obj;
+
+/* Public */
+qemu_irq irq_gfx;
+qemu_irq irq_iosfc;
+MemoryRegion iomem_gfx;
+MemoryRegion iomem_iosfc;
+id pgdev;
+id pgdisp;
+PGIOSurfaceHostDevice *pgiosfc;
+AppleGFXMRList mrs;
+AppleGFXTaskList tasks;
+QemuConsole *con;
+void *vram;
+id mtl;
+id texture;
+bool handles_frames;
+bool new_frame;
+bool cursor_show;
+DisplaySurface *surface;
+QEMUCursor *cursor;
+} AppleGFXState;
+
+
+OBJECT_DECLARE_SIMPLE_TYPE(AppleGFXState, APPLE_GFX)
+
+static AppleGFXTask *apple_gfx_new_task(AppleGFXState *s, uint64_t len)
+{
+void *base = APPLE_GFX_BASE_VA;
+AppleGFXTask *task;
+
+QTA

[PATCH 04/26] hw/display/apple-gfx: uses DEFINE_TYPES macro

[Phil Dennis-Jordan]

Switches the device definition to the more modern macro variants.

Signed-off-by: Phil Dennis-Jordan 
---
 hw/display/apple-gfx.m | 23 +--
 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/hw/display/apple-gfx.m b/hw/display/apple-gfx.m
index 3b437e2519..87bcdcd98e 100644
--- a/hw/display/apple-gfx.m
+++ b/hw/display/apple-gfx.m
@@ -86,10 +86,8 @@ -(void)mmioWriteAtOffset:(size_t) offset 
value:(uint32_t)value;
 typedef QTAILQ_HEAD(, AppleGFXTask) AppleGFXTaskList;
 
 typedef struct AppleGFXState {
-/* Private */
 SysBusDevice parent_obj;
 
-/* Public */
 qemu_irq irq_gfx;
 qemu_irq irq_iosfc;
 MemoryRegion iomem_gfx;
@@ -562,17 +560,14 @@ static void apple_gfx_class_init(ObjectClass *klass, void 
*data)
 dc->realize = apple_gfx_realize;
 }
 
-static TypeInfo apple_gfx_info = {
-.name  = TYPE_APPLE_GFX,
-.parent= TYPE_SYS_BUS_DEVICE,
-.instance_size = sizeof(AppleGFXState),
-.class_init= apple_gfx_class_init,
-.instance_init = apple_gfx_init,
+static TypeInfo apple_gfx_types[] = {
+{
+.name  = TYPE_APPLE_GFX,
+.parent= TYPE_SYS_BUS_DEVICE,
+.instance_size = sizeof(AppleGFXState),
+.class_init= apple_gfx_class_init,
+.instance_init = apple_gfx_init,
+}
 };
 
-static void apple_gfx_register_types(void)
-{
-type_register_static(&apple_gfx_info);
-}
-
-type_init(apple_gfx_register_types)
+DEFINE_TYPES(apple_gfx_types)
-- 
2.39.3 (Apple Git-146)

[PATCH 20/26] hw/display/apple-gfx: Fixes cursor hotspot handling

[Phil Dennis-Jordan]

The ParavirtualizedGraphics framework provides the cursor's
hotspot, this change actually passes that information through to
Qemu's cursor handling.

This change also seizes the opportunity to make other cursor
related code conform to coding standards.

Signed-off-by: Phil Dennis-Jordan 
---
 hw/display/apple-gfx.m | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/display/apple-gfx.m b/hw/display/apple-gfx.m
index 437294d0fb..bc9722b420 100644
--- a/hw/display/apple-gfx.m
+++ b/hw/display/apple-gfx.m
@@ -223,7 +223,8 @@ static void apple_gfx_fb_update_display(void *opaque)
 
 static void update_cursor(AppleGFXState *s)
 {
-dpy_mouse_set(s->con, s->pgdisp.cursorPosition.x, 
s->pgdisp.cursorPosition.y, s->cursor_show);
+dpy_mouse_set(s->con, s->pgdisp.cursorPosition.x,
+  s->pgdisp.cursorPosition.y, s->cursor_show);
 
 /* Need to render manually if cursor is not natively supported */
 if (!dpy_cursor_define_supported(s->con)) {
@@ -423,7 +424,8 @@ static void 
apple_gfx_register_task_mapping_handlers(AppleGFXState *s,
 trace_apple_gfx_mode_change(sizeInPixels.x, sizeInPixels.y);
 set_mode(s, sizeInPixels.x, sizeInPixels.y);
 };
-disp_desc.cursorGlyphHandler = ^(NSBitmapImageRep *glyph, PGDisplayCoord_t 
hotSpot) {
+disp_desc.cursorGlyphHandler = ^(NSBitmapImageRep *glyph,
+ PGDisplayCoord_t hotSpot) {
 uint32_t bpp = glyph.bitsPerPixel;
 uint64_t width = glyph.pixelsWide;
 uint64_t height = glyph.pixelsHigh;
@@ -434,6 +436,8 @@ static void 
apple_gfx_register_task_mapping_handlers(AppleGFXState *s,
 cursor_unref(s->cursor);
 }
 s->cursor = cursor_alloc(width, height);
+s->cursor->hot_x = hotSpot.x;
+s->cursor->hot_y = hotSpot.y;
 
 /* TODO handle different bpp */
 if (bpp == 32) {
-- 
2.39.3 (Apple Git-146)

[PATCH 06/26] hw/display/apple-gfx: Removes dead/superfluous code

[Phil Dennis-Jordan]

This removes a few chunks of code that do nothing useful.

 * MAX_MRS was not used anywhere.
 * The MMIO offset switch block had only a default case.
 * Generating mip maps for the texture is a waste of time as the
   texture is only used for read-out to a system memory buffer.
 * The trace range callbacks are used only when the device is
   handled by the (2D-only) UEFI guest driver, not by the macOS
   guest driver, hence the lack of observed use in the vmapple
   machine which does not use UEFI. Making these no-ops could
   potentially be harmful, whereas leaving them unimplemented
   is explicitly supported according to the framework header
   docs.

Signed-off-by: Phil Dennis-Jordan 
---
 hw/display/apple-gfx.m | 17 +
 1 file changed, 1 insertion(+), 16 deletions(-)

diff --git a/hw/display/apple-gfx.m b/hw/display/apple-gfx.m
index 8b0459f969..b10c060d9a 100644
--- a/hw/display/apple-gfx.m
+++ b/hw/display/apple-gfx.m
@@ -28,8 +28,6 @@
 
 #define TYPE_APPLE_GFX  "apple-gfx"
 
-#define MAX_MRS 512
-
 static const PGDisplayCoord_t apple_gfx_modes[] = {
 { .x = 1440, .y = 1080 },
 { .x = 1280, .y = 1024 },
@@ -149,11 +147,7 @@ static uint64_t apple_gfx_read(void *opaque, hwaddr 
offset, unsigned size)
 AppleGFXState *s = opaque;
 uint64_t res = 0;
 
-switch (offset) {
-default:
-res = [s->pgdev mmioReadAtOffset:offset];
-break;
-}
+res = [s->pgdev mmioReadAtOffset:offset];
 
 trace_apple_gfx_read(offset, res);
 
@@ -248,7 +242,6 @@ static void apple_gfx_fb_update_display(void *opaque)
 }
 
 id blitCommandEncoder = [mipmapCommandBuffer 
blitCommandEncoder];
-[blitCommandEncoder generateMipmapsForTexture:s->texture];
 [blitCommandEncoder endEncoding];
 [mipmapCommandBuffer commit];
 [mipmapCommandBuffer waitUntilCompleted];
@@ -460,14 +453,6 @@ static void apple_gfx_realize(DeviceState *dev, Error 
**errp)
 }
 };
 
-desc.addTraceRange = ^(PGPhysicalMemoryRange_t * _Nonnull range, 
PGTraceRangeHandler _Nonnull handler) {
-/* Never saw this called. Return a bogus pointer so we catch access. */
-return (PGTraceRange_t *)(void *)(uintptr_t)0x4242;
-};
-
-desc.removeTraceRange = ^(PGTraceRange_t * _Nonnull range) {
-/* Never saw this called. Nothing to do. */
-};
 s->pgdev = PGNewDeviceWithDescriptor(desc);
 
 [disp_desc init];
-- 
2.39.3 (Apple Git-146)

[PATCH 25/26] MAINTAINERS: Add myself as maintainer for apple-gfx, reviewer for HVF

[Phil Dennis-Jordan]

I'm happy to take responsibility for the macOS PV graphics code. As
HVF patches don't seem to get much attention at the moment, I'm also
adding myself as designated reviewer for HVF and x86 HVF to try and
improve that.

I anticipate that the resulting workload should be covered by the
funding I'm receiving for improving Qemu in combination with macOS. As
of right now this runs out at the end of 2024; I expect the workload on
apple-gfx should be relatively minor and manageable in my spare time
beyond that. I may have to remove myself from more general HVF duties
once the contract runs out if it's more than I can manage.

Signed-off-by: Phil Dennis-Jordan 
---
 MAINTAINERS | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 7d9811458c..b870bd6ad5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -511,6 +511,7 @@ F: target/arm/hvf/
 X86 HVF CPUs
 M: Cameron Esfahani 
 M: Roman Bolshakov 
+R: Phil Dennis-Jordan 
 W: https://wiki.qemu.org/Features/HVF
 S: Maintained
 F: target/i386/hvf/
@@ -518,6 +519,7 @@ F: target/i386/hvf/
 HVF
 M: Cameron Esfahani 
 M: Roman Bolshakov 
+R: Phil Dennis-Jordan 
 W: https://wiki.qemu.org/Features/HVF
 S: Maintained
 F: accel/hvf/
@@ -2624,6 +2626,11 @@ F: hw/display/edid*
 F: include/hw/display/edid.h
 F: qemu-edid.c
 
+macOS PV Graphics (apple-gfx)
+M: Phil Dennis-Jordan 
+S: Maintained
+F: hw/display/apple-gfx*
+
 PIIX4 South Bridge (i82371AB)
 M: Hervé Poussineau 
 M: Philippe Mathieu-Daudé 
-- 
2.39.3 (Apple Git-146)

[PATCH 03/26] hw/display/apple-gfx: Moved from hw/vmapple/

[Phil Dennis-Jordan]

The macOS PV graphics device is useful outside the (as yet incomplete)
vmapple machine type, so this patch moves it to hw/display.

Signed-off-by: Phil Dennis-Jordan 
---
 hw/display/Kconfig  |  4 
 hw/{vmapple => display}/apple-gfx.m |  0
 hw/display/meson.build  |  1 +
 hw/display/trace-events | 23 +++
 hw/vmapple/Kconfig  |  4 
 hw/vmapple/meson.build  |  1 -
 hw/vmapple/trace-events | 23 ---
 7 files changed, 28 insertions(+), 28 deletions(-)
 rename hw/{vmapple => display}/apple-gfx.m (100%)
 delete mode 100644 hw/vmapple/Kconfig
 delete mode 100644 hw/vmapple/meson.build
 delete mode 100644 hw/vmapple/trace-events

diff --git a/hw/display/Kconfig b/hw/display/Kconfig
index a4552c8ed7..13cd256c06 100644
--- a/hw/display/Kconfig
+++ b/hw/display/Kconfig
@@ -143,3 +143,7 @@ config XLNX_DISPLAYPORT
 
 config DM163
 bool
+
+config MAC_PVG
+bool
+
diff --git a/hw/vmapple/apple-gfx.m b/hw/display/apple-gfx.m
similarity index 100%
rename from hw/vmapple/apple-gfx.m
rename to hw/display/apple-gfx.m
diff --git a/hw/display/meson.build b/hw/display/meson.build
index 7db05eace9..713786bd07 100644
--- a/hw/display/meson.build
+++ b/hw/display/meson.build
@@ -65,6 +65,7 @@ system_ss.add(when: 'CONFIG_ARTIST', if_true: 
files('artist.c'))
 
 system_ss.add(when: 'CONFIG_ATI_VGA', if_true: [files('ati.c', 'ati_2d.c', 
'ati_dbg.c'), pixman])
 
+system_ss.add(when: 'CONFIG_MAC_PVG', if_true: [files('apple-gfx.m'), pvg, 
metal])
 
 if config_all_devices.has_key('CONFIG_VIRTIO_GPU')
   virtio_gpu_ss = ss.source_set()
diff --git a/hw/display/trace-events b/hw/display/trace-events
index 781f8a3320..98e1a3a3a0 100644
--- a/hw/display/trace-events
+++ b/hw/display/trace-events
@@ -191,3 +191,26 @@ dm163_bits_ppi(unsigned dest_width) "dest_width : %u"
 dm163_leds(int led, uint32_t value) "led %d: 0x%x"
 dm163_channels(int channel, uint8_t value) "channel %d: 0x%x"
 dm163_refresh_rate(uint32_t rr) "refresh rate %d"
+
+# apple-gfx.m
+apple_gfx_read(uint64_t offset, uint64_t res) "offset=0x%"PRIx64" 
res=0x%"PRIx64
+apple_gfx_write(uint64_t offset, uint64_t val) "offset=0x%"PRIx64" 
val=0x%"PRIx64
+apple_gfx_create_task(uint32_t vm_size, void *va) "vm_size=0x%x base_addr=%p"
+apple_gfx_destroy_task(void *task) "task=%p"
+apple_gfx_map_memory(void *task, uint32_t range_count, uint64_t 
virtual_offset, uint32_t read_only) "task=%p range_count=0x%x 
virtual_offset=0x%"PRIx64" read_only=%d"
+apple_gfx_map_memory_range(uint32_t i, uint64_t phys_addr, uint64_t phys_len, 
void *va) "[%d] phys_addr=0x%"PRIx64" phys_len=0x%"PRIx64" va=%p"
+apple_gfx_remap(uint64_t retval, uint64_t source, uint64_t target) 
"retval=%"PRId64" source=0x%"PRIx64" target=0x%"PRIx64
+apple_gfx_unmap_memory(void *task, uint64_t virtual_offset, uint64_t length) 
"task=%p virtual_offset=0x%"PRIx64" length=0x%"PRIx64
+apple_gfx_read_memory(uint64_t phys_address, uint64_t length, void *dst) 
"phys_addr=0x%"PRIx64" length=0x%"PRIx64" dest=%p"
+apple_gfx_raise_irq(uint32_t vector) "vector=0x%x"
+apple_gfx_new_frame(void) ""
+apple_gfx_mode_change(uint64_t x, uint64_t y) "x=%"PRId64" y=%"PRId64
+apple_gfx_cursor_set(uint32_t bpp, uint64_t width, uint64_t height) "bpp=%d 
width=%"PRId64" height=0x%"PRId64
+apple_gfx_cursor_show(uint32_t show) "show=%d"
+apple_gfx_cursor_move(void) ""
+apple_iosfc_read(uint64_t offset, uint64_t res) "offset=0x%"PRIx64" 
res=0x%"PRIx64
+apple_iosfc_write(uint64_t offset, uint64_t val) "offset=0x%"PRIx64" 
val=0x%"PRIx64
+apple_iosfc_map_memory(uint64_t phys, uint64_t len, uint32_t ro, void *va, 
void *e, void *f) "phys=0x%"PRIx64" len=0x%"PRIx64" ro=%d va=%p e=%p f=%p"
+apple_iosfc_unmap_memory(void *a, void *b, void *c, void *d, void *e, void *f) 
"a=%p b=%p c=%p d=%p e=%p f=%p"
+apple_iosfc_raise_irq(uint32_t vector) "vector=0x%x"
+
diff --git a/hw/vmapple/Kconfig b/hw/vmapple/Kconfig
deleted file mode 100644
index 5835790a31..00
--- a/hw/vmapple/Kconfig
+++ /dev/null
@@ -1,4 +0,0 @@
-
-config VMAPPLE_PVG
-bool
-
diff --git a/hw/vmapple/meson.build b/hw/vmapple/meson.build
deleted file mode 100644
index f9ab2194d9..00
--- a/hw/vmapple/meson.build
+++ /dev/null
@@ -1 +0,0 @@
-system_ss.add(when: 'CONFIG_VMAPPLE_PVG',  if_true: [files('apple-gfx.m'), 
pvg, metal])
diff --git a/hw/vmapple/trace-events b/hw/vmapple/trace-events
deleted file mode 100644
index 497f64064b..00
--- a/hw/vmapple/trace-events
+++ /dev/null
@@ -1,23 +0,0 @@
-# See docs/devel/tracing.rst for syntax documentation.
-
-# apple-gfx.m
-apple_gfx_read(uint64_t offset, uint64_t res) "offset=0x%"PRIx64" 
res=0x%"PRIx64
-apple_gfx_write(uint64_t offset, uint64_t val) "offset=0x%"PRIx64" 
val=0x%"PRIx64
-apple_gfx_create_task(uint32_t vm_size, void *va) "vm_size=0x%x base_addr=%p"
-apple_gfx_destroy_task(void *task) "task=%p"
-apple_gfx_map_memory(void *task, uint32_t range_count, uint64_t 
virtual_offset, uint32_

[PATCH 19/26] ui/cocoa: Adds non-app runloop on main thread mode

[Phil Dennis-Jordan]

Various system frameworks on macOS and other Apple platforms
require a main runloop to be processing events on the process’s
main thread. The Cocoa UI’s requirement to run the process as a
Cocoa application automatically enables this runloop, but it
can be useful to have the runloop handling events even without
the Cocoa UI active.

This change adds a non-app runloop mode to the cocoa_main
function. This can be requested by other code, while the Cocoa UI
additionally enables app mode. This arrangement ensures there is
only one qemu_main function switcheroo, and the Cocoa UI’s app
mode requirement and other subsystems’ runloop requests don’t
conflict with each other.

The main runloop is required for the AppleGFX PV graphics device,
so the runloop request call has been added to its initialisation.

Signed-off-by: Phil Dennis-Jordan 
---
 hw/display/apple-gfx.m |  3 +++
 include/qemu-main.h|  2 ++
 ui/cocoa.m | 15 +--
 3 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/hw/display/apple-gfx.m b/hw/display/apple-gfx.m
index 5855d1d7f5..437294d0fb 100644
--- a/hw/display/apple-gfx.m
+++ b/hw/display/apple-gfx.m
@@ -14,6 +14,7 @@
 
 #include "apple-gfx.h"
 #include "trace.h"
+#include "qemu-main.h"
 #include "qemu/main-loop.h"
 #include "ui/console.h"
 #include "monitor/monitor.h"
@@ -309,6 +310,8 @@ void apple_gfx_common_init(Object *obj, AppleGFXState *s, 
const char* obj_name)
 error_report_err(local_err);
 }
 }
+
+cocoa_enable_runloop_on_main_thread();
 }
 
 static void apple_gfx_register_task_mapping_handlers(AppleGFXState *s,
diff --git a/include/qemu-main.h b/include/qemu-main.h
index 940960a7db..da4516e69e 100644
--- a/include/qemu-main.h
+++ b/include/qemu-main.h
@@ -8,4 +8,6 @@
 int qemu_default_main(void);
 extern int (*qemu_main)(void);
 
+void cocoa_enable_runloop_on_main_thread(void);
+
 #endif /* QEMU_MAIN_H */
diff --git a/ui/cocoa.m b/ui/cocoa.m
index 2935247cdd..ccef2aa93c 100644
--- a/ui/cocoa.m
+++ b/ui/cocoa.m
@@ -1941,6 +1941,7 @@ static void cocoa_clipboard_request(QemuClipboardInfo 
*info,
 exit(status);
 }
 
+static bool run_as_cocoa_app = false;
 static int cocoa_main(void)
 {
 QemuThread thread;
@@ -1953,7 +1954,11 @@ static int cocoa_main(void)
 
 // Start the main event loop
 COCOA_DEBUG("Main thread: entering OSX run loop\n");
-[NSApp run];
+if (run_as_cocoa_app) {
+[NSApp run];
+} else {
+CFRunLoopRun();
+}
 COCOA_DEBUG("Main thread: left OSX run loop, which should never happen\n");
 
 abort();
@@ -2012,13 +2017,19 @@ static void cocoa_refresh(DisplayChangeListener *dcl)
 [pool release];
 }
 
+void cocoa_enable_runloop_on_main_thread(void)
+{
+qemu_main = cocoa_main;
+}
+
 static void cocoa_display_init(DisplayState *ds, DisplayOptions *opts)
 {
 NSAutoreleasePool * pool = [[NSAutoreleasePool alloc] init];
 
 COCOA_DEBUG("qemu_cocoa: cocoa_display_init\n");
 
-qemu_main = cocoa_main;
+run_as_cocoa_app = true;
+cocoa_enable_runloop_on_main_thread();
 
 // Pull this console process up to being a fully-fledged graphical
 // app with a menubar and Dock icon
-- 
2.39.3 (Apple Git-146)

[PATCH 26/26] hw/display/apple-gfx: Removes UI pointer support check

[Phil Dennis-Jordan]

The dpy_cursor_define_supported() check is destined for removal
when the last UI frontend without cursor support (cocoa) gains
cursor integration. This patch is required to reconcile with
that change.

Signed-off-by: Phil Dennis-Jordan 
---
 hw/display/apple-gfx.m | 24 
 1 file changed, 24 deletions(-)

diff --git a/hw/display/apple-gfx.m b/hw/display/apple-gfx.m
index e0ad784022..a4789be275 100644
--- a/hw/display/apple-gfx.m
+++ b/hw/display/apple-gfx.m
@@ -175,25 +175,6 @@ static void apple_gfx_render_frame_completed(AppleGFXState 
*s, void *vram,
 g_free(vram);
 } else {
 copy_mtl_texture_to_surface_mem(texture, vram);
-
-/* Need to render cursor manually if not supported by backend */
-if (!dpy_cursor_define_supported(s->con) && s->cursor && 
s->cursor_show) {
-pixman_image_t *image =
-pixman_image_create_bits(PIXMAN_a8r8g8b8,
- s->cursor->width,
- s->cursor->height,
- (uint32_t *)s->cursor->data,
- s->cursor->width * 4);
-
-pixman_image_composite(PIXMAN_OP_OVER,
-   image, NULL, s->surface->image,
-   0, 0, 0, 0, s->pgdisp.cursorPosition.x,
-   s->pgdisp.cursorPosition.y, 
s->cursor->width,
-   s->cursor->height);
-
-pixman_image_unref(image);
-}
-
 if (s->gfx_update_requested) {
 s->gfx_update_requested = false;
 dpy_gfx_update_full(s->con);
@@ -234,11 +215,6 @@ static void update_cursor(AppleGFXState *s)
 {
 dpy_mouse_set(s->con, s->pgdisp.cursorPosition.x,
   s->pgdisp.cursorPosition.y, s->cursor_show);
-
-/* Need to render manually if cursor is not natively supported */
-if (!dpy_cursor_define_supported(s->con)) {
-s->new_frame = true;
-}
 }
 
 static void set_mode(AppleGFXState *s, uint32_t width, uint32_t height)
-- 
2.39.3 (Apple Git-146)

[PATCH 13/26] hw/display/apple-gfx: Defines PGTask_s struct instead of casting

[Phil Dennis-Jordan]

The struct PGTask_s is only forward-declared by the macOS
ParavirtualizedGraphics.framework and treated as opaque, which
means client code can safely provide its own definition.

This change does exactly that, renaming struct AppleGFXTask to
PGTask_s, but keeping the original typedef name. The upshot are
improved type safety and fewer casts.

Signed-off-by: Phil Dennis-Jordan 
---
 hw/display/apple-gfx.m | 20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/hw/display/apple-gfx.m b/hw/display/apple-gfx.m
index a23f731ddc..39e33ed999 100644
--- a/hw/display/apple-gfx.m
+++ b/hw/display/apple-gfx.m
@@ -62,13 +62,13 @@ -(uint32_t)mmioReadAtOffset:(size_t) offset;
 -(void)mmioWriteAtOffset:(size_t) offset value:(uint32_t)value;
 @end
 
-typedef struct AppleGFXTask {
-QTAILQ_ENTRY(AppleGFXTask) node;
+typedef struct PGTask_s { // Name matches forward declaration in PG header
+QTAILQ_ENTRY(PGTask_s) node;
 mach_vm_address_t address;
 uint64_t len;
 } AppleGFXTask;
 
-typedef QTAILQ_HEAD(, AppleGFXTask) AppleGFXTaskList;
+typedef QTAILQ_HEAD(, PGTask_s) AppleGFXTaskList;
 
 typedef struct AppleGFXState {
 SysBusDevice parent_obj;
@@ -384,19 +384,19 @@ static void apple_gfx_realize(DeviceState *dev, Error 
**errp)
 AppleGFXTask *task = apple_gfx_new_task(s, vmSize);
 *baseAddress = (void*)task->address;
 trace_apple_gfx_create_task(vmSize, *baseAddress);
-return (PGTask_t *)task;
+return task;
 };
 
-desc.destroyTask = ^(PGTask_t * _Nonnull _task) {
-AppleGFXTask *task = (AppleGFXTask *)_task;
+desc.destroyTask = ^(AppleGFXTask * _Nonnull task) {
 trace_apple_gfx_destroy_task(task);
 QTAILQ_REMOVE(&s->tasks, task, node);
 mach_vm_deallocate(mach_task_self(), task->address, task->len);
 g_free(task);
 };
 
-desc.mapMemory = ^(PGTask_t * _Nonnull _task, uint32_t rangeCount, 
uint64_t virtualOffset, bool readOnly, PGPhysicalMemoryRange_t * _Nonnull 
ranges) {
-AppleGFXTask *task = (AppleGFXTask*)_task;
+desc.mapMemory = ^(AppleGFXTask * _Nonnull task, uint32_t rangeCount,
+   uint64_t virtualOffset, bool readOnly,
+   PGPhysicalMemoryRange_t * _Nonnull ranges) {
 kern_return_t r;
 mach_vm_address_t target, source;
 trace_apple_gfx_map_memory(task, rangeCount, virtualOffset, readOnly);
@@ -433,8 +433,8 @@ static void apple_gfx_realize(DeviceState *dev, Error 
**errp)
 return (bool)true;
 };
 
-desc.unmapMemory = ^(PGTask_t * _Nonnull _task, uint64_t virtualOffset, 
uint64_t length) {
-AppleGFXTask *task = (AppleGFXTask *)_task;
+desc.unmapMemory = ^(AppleGFXTask * _Nonnull task, uint64_t virtualOffset,
+ uint64_t length) {
 kern_return_t r;
 mach_vm_address_t range_address;
 
-- 
2.39.3 (Apple Git-146)

[PATCH 18/26] hw/display/apple-gfx: Adds PCI implementation

[Phil Dennis-Jordan]

This change wires up the PCI variant of the paravirtualised
graphics device, mainly useful for x86-64 macOS guests, implemented
by macOS's ParavirtualizedGraphics.framework. It builds on the shared
code previously split from the vmapple MMIO variant of the device.

Signed-off-by: Phil Dennis-Jordan 
---
 hw/display/Kconfig |   5 ++
 hw/display/apple-gfx-pci.m | 125 +
 hw/display/meson.build |   1 +
 3 files changed, 131 insertions(+)
 create mode 100644 hw/display/apple-gfx-pci.m

diff --git a/hw/display/Kconfig b/hw/display/Kconfig
index e3d10bf6ff..8a78a60670 100644
--- a/hw/display/Kconfig
+++ b/hw/display/Kconfig
@@ -151,3 +151,8 @@ config MAC_PVG
 config MAC_PVG_VMAPPLE
 bool
 depends on MAC_PVG
+
+config MAC_PVG_PCI
+bool
+depends on MAC_PVG && PCI
+default y if PCI_DEVICES
diff --git a/hw/display/apple-gfx-pci.m b/hw/display/apple-gfx-pci.m
new file mode 100644
index 00..bdbab35eed
--- /dev/null
+++ b/hw/display/apple-gfx-pci.m
@@ -0,0 +1,125 @@
+#include "apple-gfx.h"
+#include "hw/pci/pci_device.h"
+#include "hw/pci/msi.h"
+#include "qapi/error.h"
+#include "trace.h"
+#import 
+
+typedef struct AppleGFXPCIState {
+PCIDevice parent_obj;
+
+AppleGFXState common;
+} AppleGFXPCIState;
+
+OBJECT_DECLARE_SIMPLE_TYPE(AppleGFXPCIState, APPLE_GFX_PCI)
+
+static const char* apple_gfx_pci_option_rom_path = NULL;
+
+static void apple_gfx_init_option_rom_path(void)
+{
+NSURL *option_rom_url = PGCopyOptionROMURL();
+const char *option_rom_path = option_rom_url.fileSystemRepresentation;
+if (option_rom_url.fileURL && option_rom_path != NULL) {
+apple_gfx_pci_option_rom_path = g_strdup(option_rom_path);
+}
+[option_rom_url release];
+}
+
+static void apple_gfx_pci_init(Object *obj)
+{
+AppleGFXPCIState *s = APPLE_GFX_PCI(obj);
+
+if (!apple_gfx_pci_option_rom_path) {
+/* Done on device not class init to avoid -daemonize ObjC fork crash */
+PCIDeviceClass *pci = PCI_DEVICE_CLASS(object_get_class(obj));
+apple_gfx_init_option_rom_path();
+pci->romfile = apple_gfx_pci_option_rom_path;
+}
+
+apple_gfx_common_init(obj, &s->common, TYPE_APPLE_GFX_PCI);
+}
+
+static void apple_gfx_pci_interrupt(PCIDevice *dev, AppleGFXPCIState *s,
+uint32_t vector)
+{
+bool msi_ok;
+trace_apple_gfx_raise_irq(vector);
+
+msi_ok = msi_enabled(dev);
+if (msi_ok) {
+msi_notify(dev, vector);
+}
+}
+
+static void apple_gfx_pci_realize(PCIDevice *dev, Error **errp)
+{
+AppleGFXPCIState *s = APPLE_GFX_PCI(dev);
+Error *err = NULL;
+int ret;
+
+pci_register_bar(dev, PG_PCI_BAR_MMIO,
+ PCI_BASE_ADDRESS_SPACE_MEMORY, &s->common.iomem_gfx);
+
+ret = msi_init(dev, 0x0 /* config offset; 0 = find space */,
+   PG_PCI_MAX_MSI_VECTORS, true /* msi64bit */,
+   false /*msi_per_vector_mask*/, &err);
+if (ret != 0) {
+error_propagate(errp, err);
+return;
+}
+
+@autoreleasepool {
+PGDeviceDescriptor *desc = [PGDeviceDescriptor new];
+desc.raiseInterrupt = ^(uint32_t vector) {
+apple_gfx_pci_interrupt(dev, s, vector);
+};
+
+apple_gfx_common_realize(&s->common, desc);
+[desc release];
+desc = nil;
+}
+}
+
+static void apple_gfx_pci_reset(DeviceState *dev)
+{
+AppleGFXPCIState *s = APPLE_GFX_PCI(dev);
+if (@available(macOS 12,*)) {
+[s->common.pgdev reset];
+} else {
+// TODO: tear down and bring back up
+}
+}
+
+static void apple_gfx_pci_class_init(ObjectClass *klass, void *data)
+{
+DeviceClass *dc = DEVICE_CLASS(klass);
+PCIDeviceClass *pci = PCI_DEVICE_CLASS(klass);
+
+dc->reset = apple_gfx_pci_reset;
+dc->desc = "macOS Paravirtualized Graphics PCI Display Controller";
+dc->hotpluggable = false;
+set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
+
+pci->vendor_id = PG_PCI_VENDOR_ID;
+pci->device_id = PG_PCI_DEVICE_ID;
+pci->class_id = PCI_CLASS_DISPLAY_OTHER;
+pci->realize = apple_gfx_pci_realize;
+
+// TODO: Property for setting mode list
+}
+
+static TypeInfo apple_gfx_pci_types[] = {
+{
+.name  = TYPE_APPLE_GFX_PCI,
+.parent= TYPE_PCI_DEVICE,
+.instance_size = sizeof(AppleGFXPCIState),
+.class_init= apple_gfx_pci_class_init,
+.instance_init = apple_gfx_pci_init,
+.interfaces = (InterfaceInfo[]) {
+{ INTERFACE_PCIE_DEVICE },
+{ },
+},
+}
+};
+DEFINE_TYPES(apple_gfx_pci_types)
+
diff --git a/hw/display/meson.build b/hw/display/meson.build
index 70d855749c..ceb7bb0761 100644
--- a/hw/display/meson.build
+++ b/hw/display/meson.build
@@ -67,6 +67,7 @@ system_ss.add(when: 'CONFIG_ATI_VGA', if_true: 
[files('ati.c', 'ati_2d.c', 'ati_
 
 system_ss.add(when: 'CONFIG_MAC_PVG', if_true: [files(

[PATCH 17/26] hw/display/apple-gfx: Asynchronous rendering and graphics update

[Phil Dennis-Jordan]

This change avoids doing expensive rendering while holding the BQL.
Rendering with the lock held is not only inefficient, it can also
cause deadlocks when the PV Graphics framework’s encode... method
causes a (synchronous) call to a callback, which in turn tries to
acquire the BQL.

Signed-off-by: Phil Dennis-Jordan 
---
 hw/display/apple-gfx.h |  15 +++-
 hw/display/apple-gfx.m | 193 +++--
 2 files changed, 138 insertions(+), 70 deletions(-)

diff --git a/hw/display/apple-gfx.h b/hw/display/apple-gfx.h
index fa7fea6368..9d6d40795e 100644
--- a/hw/display/apple-gfx.h
+++ b/hw/display/apple-gfx.h
@@ -15,12 +15,14 @@ void apple_gfx_common_init(Object *obj, AppleGFXState *s, 
const char* obj_name);
 #include "qemu/osdep.h"
 #include "exec/memory.h"
 #include "ui/surface.h"
+#include 
 
 @class PGDeviceDescriptor;
 @protocol PGDevice;
 @protocol PGDisplay;
 @protocol MTLDevice;
 @protocol MTLTexture;
+@protocol MTLCommandQueue;
 
 typedef QTAILQ_HEAD(, PGTask_s) AppleGFXTaskList;
 
@@ -30,14 +32,21 @@ struct AppleGFXState {
 id pgdisp;
 AppleGFXTaskList tasks;
 QemuConsole *con;
-void *vram;
 id mtl;
-id texture;
+id mtl_queue;
 bool handles_frames;
 bool new_frame;
 bool cursor_show;
-DisplaySurface *surface;
 QEMUCursor *cursor;
+
+dispatch_queue_t render_queue;
+/* The following fields should only be accessed from render_queue: */
+bool gfx_update_requested;
+bool new_frame_ready;
+int32_t pending_frames;
+void *vram;
+DisplaySurface *surface;
+id texture;
 };
 
 void apple_gfx_common_realize(AppleGFXState *s, PGDeviceDescriptor *desc);
diff --git a/hw/display/apple-gfx.m b/hw/display/apple-gfx.m
index 48463e5a1f..5855d1d7f5 100644
--- a/hw/display/apple-gfx.m
+++ b/hw/display/apple-gfx.m
@@ -35,6 +35,9 @@
 
 static Error *apple_gfx_mig_blocker;
 
+static void apple_gfx_render_frame_completed(AppleGFXState *s, void *vram,
+ id texture);
+
 static AppleGFXTask *apple_gfx_new_task(AppleGFXState *s, uint64_t len)
 {
 mach_vm_address_t task_mem;
@@ -105,41 +108,63 @@ static void apple_gfx_write(void *opaque, hwaddr offset, 
uint64_t val,
 },
 };
 
-static void apple_gfx_fb_update_display(void *opaque)
+static void apple_gfx_render_new_frame(AppleGFXState *s)
 {
-AppleGFXState *s = opaque;
-
-if (!s->new_frame || !s->handles_frames) {
+BOOL r;
+void *vram = s->vram;
+uint32_t width = surface_width(s->surface);
+uint32_t height = surface_height(s->surface);
+MTLRegion region = MTLRegionMake2D(0, 0, width, height);
+id command_buffer = [s->mtl_queue commandBuffer];
+id texture = s->texture;
+r = [s->pgdisp encodeCurrentFrameToCommandBuffer:command_buffer
+ texture:texture
+  region:region];
+if (!r) {
 return;
 }
+[texture retain];
+
+[command_buffer retain];
+[command_buffer addCompletedHandler:
+^(id cb)
+{
+dispatch_async(s->render_queue, ^{
+apple_gfx_render_frame_completed(s, vram, texture);
+[texture release];
+});
+[command_buffer release];
+}];
+[command_buffer commit];
+}
 
-@autoreleasepool {
-s->new_frame = false;
-
-BOOL r;
-uint32_t width = surface_width(s->surface);
-uint32_t height = surface_height(s->surface);
-MTLRegion region = MTLRegionMake2D(0, 0, width, height);
-id commandQueue = [s->mtl newCommandQueue];
-id mipmapCommandBuffer = [commandQueue 
commandBuffer];
-
-r = [s->pgdisp encodeCurrentFrameToCommandBuffer:mipmapCommandBuffer
- texture:s->texture
-  region:region];
+static void copy_mtl_texture_to_surface_mem(id texture, void *vram)
+{
+/* TODO: Skip this entirely on a pure Metal or headless/guest-only
+ * rendering path, else use a blit command encoder? Needs careful
+ * (double?) buffering design. */
+size_t width = texture.width, height = texture.height;
+MTLRegion region = MTLRegionMake2D(0, 0, width, height);
+[texture getBytes:vram
+  bytesPerRow:(width * 4)
+bytesPerImage:(width * height * 4)
+   fromRegion:region
+  mipmapLevel:0
+slice:0];
+}
 
-if (r != YES) {
-return;
-}
+static void apple_gfx_render_frame_completed(AppleGFXState *s, void *vram,
+ id texture)
+{
+--s->pending_frames;
+assert(s->pending_frames >= 0);
 
-id blitCommandEncoder = [mipmapCommandBuffer 
blitCommandEncoder];
-[blitCommandEncoder endEncoding];
-[mipmapCommandBuffer commit];
-[mipmapCommandBuffer waitUntilCompleted];
-[s->texture getBytes:s

[PATCH 21/26] hw/display/apple-gfx: Implements texture syncing for non-UMA GPUs

[Phil Dennis-Jordan]

Renderable Metal textures are handled differently depending on
whether the GPU uses a unified memory architecture (no physical
distinction between VRAM and system RAM, CPU and GPU share the
memory bus) or not. (Traditional discrete GPU with its own VRAM)

In the discrete GPU case, textures must be explicitly
synchronised to the CPU or the GPU before use after being
modified by the other. In this case, we sync after the PV
graphics framework has rendered the next frame into the
texture using the GPU so that we can read out its contents using
the CPU. This fixes the issue where the guest screen stayed
black on AMD Radeon GPUs.

Signed-off-by: Phil Dennis-Jordan 
---
 hw/display/apple-gfx.h |  1 +
 hw/display/apple-gfx.m | 10 +-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/hw/display/apple-gfx.h b/hw/display/apple-gfx.h
index 9d6d40795e..995ecf7f4a 100644
--- a/hw/display/apple-gfx.h
+++ b/hw/display/apple-gfx.h
@@ -43,6 +43,7 @@ struct AppleGFXState {
 /* The following fields should only be accessed from render_queue: */
 bool gfx_update_requested;
 bool new_frame_ready;
+bool using_managed_texture_storage;
 int32_t pending_frames;
 void *vram;
 DisplaySurface *surface;
diff --git a/hw/display/apple-gfx.m b/hw/display/apple-gfx.m
index bc9722b420..801ae4ad51 100644
--- a/hw/display/apple-gfx.m
+++ b/hw/display/apple-gfx.m
@@ -125,7 +125,12 @@ static void apple_gfx_render_new_frame(AppleGFXState *s)
 return;
 }
 [texture retain];
-
+if (s->using_managed_texture_storage) {
+/* "Managed" textures exist in both VRAM and RAM and must be synced. */
+id blit = [command_buffer blitCommandEncoder];
+[blit synchronizeResource:texture];
+[blit endEncoding];
+}
 [command_buffer retain];
 [command_buffer addCompletedHandler:
 ^(id cb)
@@ -268,6 +273,9 @@ static void set_mode(AppleGFXState *s, uint32_t width, 
uint32_t height)
 texture = [s->mtl newTextureWithDescriptor:textureDescriptor];
 }
 
+s->using_managed_texture_storage =
+(texture.storageMode == MTLStorageModeManaged);
+
 dispatch_sync(s->render_queue,
 ^{
 id old_texture = nil;
-- 
2.39.3 (Apple Git-146)

[PATCH v2 2/8] hw/display/apple-gfx: Adds PCI implementation

[Phil Dennis-Jordan]

This change wires up the PCI variant of the paravirtualised
graphics device, mainly useful for x86-64 macOS guests, implemented
by macOS's ParavirtualizedGraphics.framework. It builds on code
shared with the vmapple/mmio variant of the PVG device.

Signed-off-by: Phil Dennis-Jordan 
---
 hw/display/Kconfig |   5 ++
 hw/display/apple-gfx-pci.m | 121 +
 hw/display/meson.build |   1 +
 3 files changed, 127 insertions(+)
 create mode 100644 hw/display/apple-gfx-pci.m

diff --git a/hw/display/Kconfig b/hw/display/Kconfig
index e3d10bf6ff..8a78a60670 100644
--- a/hw/display/Kconfig
+++ b/hw/display/Kconfig
@@ -151,3 +151,8 @@ config MAC_PVG
 config MAC_PVG_VMAPPLE
 bool
 depends on MAC_PVG
+
+config MAC_PVG_PCI
+bool
+depends on MAC_PVG && PCI
+default y if PCI_DEVICES
diff --git a/hw/display/apple-gfx-pci.m b/hw/display/apple-gfx-pci.m
new file mode 100644
index 00..b3311e736c
--- /dev/null
+++ b/hw/display/apple-gfx-pci.m
@@ -0,0 +1,121 @@
+#include "apple-gfx.h"
+#include "hw/pci/pci_device.h"
+#include "hw/pci/msi.h"
+#include "qapi/error.h"
+#include "trace.h"
+#import 
+
+typedef struct AppleGFXPCIState {
+PCIDevice parent_obj;
+
+AppleGFXState common;
+} AppleGFXPCIState;
+
+OBJECT_DECLARE_SIMPLE_TYPE(AppleGFXPCIState, APPLE_GFX_PCI)
+
+static const char* apple_gfx_pci_option_rom_path = NULL;
+
+static void apple_gfx_init_option_rom_path(void)
+{
+NSURL *option_rom_url = PGCopyOptionROMURL();
+const char *option_rom_path = option_rom_url.fileSystemRepresentation;
+if (option_rom_url.fileURL && option_rom_path != NULL) {
+apple_gfx_pci_option_rom_path = g_strdup(option_rom_path);
+}
+[option_rom_url release];
+}
+
+static void apple_gfx_pci_init(Object *obj)
+{
+AppleGFXPCIState *s = APPLE_GFX_PCI(obj);
+
+if (!apple_gfx_pci_option_rom_path) {
+/* Done on device not class init to avoid -daemonize ObjC fork crash */
+PCIDeviceClass *pci = PCI_DEVICE_CLASS(object_get_class(obj));
+apple_gfx_init_option_rom_path();
+pci->romfile = apple_gfx_pci_option_rom_path;
+}
+
+apple_gfx_common_init(obj, &s->common, TYPE_APPLE_GFX_PCI);
+}
+
+static void apple_gfx_pci_interrupt(PCIDevice *dev, AppleGFXPCIState *s,
+uint32_t vector)
+{
+bool msi_ok;
+trace_apple_gfx_raise_irq(vector);
+
+msi_ok = msi_enabled(dev);
+if (msi_ok) {
+msi_notify(dev, vector);
+}
+}
+
+static void apple_gfx_pci_realize(PCIDevice *dev, Error **errp)
+{
+AppleGFXPCIState *s = APPLE_GFX_PCI(dev);
+Error *err = NULL;
+int ret;
+
+pci_register_bar(dev, PG_PCI_BAR_MMIO,
+ PCI_BASE_ADDRESS_SPACE_MEMORY, &s->common.iomem_gfx);
+
+ret = msi_init(dev, 0x0 /* config offset; 0 = find space */,
+   PG_PCI_MAX_MSI_VECTORS, true /* msi64bit */,
+   false /*msi_per_vector_mask*/, &err);
+if (ret != 0) {
+error_propagate(errp, err);
+return;
+}
+
+@autoreleasepool {
+PGDeviceDescriptor *desc = [PGDeviceDescriptor new];
+desc.raiseInterrupt = ^(uint32_t vector) {
+apple_gfx_pci_interrupt(dev, s, vector);
+};
+
+apple_gfx_common_realize(&s->common, desc);
+[desc release];
+desc = nil;
+}
+}
+
+static void apple_gfx_pci_reset(DeviceState *dev)
+{
+AppleGFXPCIState *s = APPLE_GFX_PCI(dev);
+[s->common.pgdev reset];
+}
+
+static void apple_gfx_pci_class_init(ObjectClass *klass, void *data)
+{
+DeviceClass *dc = DEVICE_CLASS(klass);
+PCIDeviceClass *pci = PCI_DEVICE_CLASS(klass);
+
+dc->reset = apple_gfx_pci_reset;
+dc->desc = "macOS Paravirtualized Graphics PCI Display Controller";
+dc->hotpluggable = false;
+set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
+
+pci->vendor_id = PG_PCI_VENDOR_ID;
+pci->device_id = PG_PCI_DEVICE_ID;
+pci->class_id = PCI_CLASS_DISPLAY_OTHER;
+pci->realize = apple_gfx_pci_realize;
+
+// TODO: Property for setting mode list
+}
+
+static TypeInfo apple_gfx_pci_types[] = {
+{
+.name  = TYPE_APPLE_GFX_PCI,
+.parent= TYPE_PCI_DEVICE,
+.instance_size = sizeof(AppleGFXPCIState),
+.class_init= apple_gfx_pci_class_init,
+.instance_init = apple_gfx_pci_init,
+.interfaces = (InterfaceInfo[]) {
+{ INTERFACE_PCIE_DEVICE },
+{ },
+},
+}
+};
+DEFINE_TYPES(apple_gfx_pci_types)
+
diff --git a/hw/display/meson.build b/hw/display/meson.build
index 70d855749c..ceb7bb0761 100644
--- a/hw/display/meson.build
+++ b/hw/display/meson.build
@@ -67,6 +67,7 @@ system_ss.add(when: 'CONFIG_ATI_VGA', if_true: 
[files('ati.c', 'ati_2d.c', 'ati_
 
 system_ss.add(when: 'CONFIG_MAC_PVG', if_true: [files('apple-gfx.m'), 
pvg, metal])
 system_ss.add(when: 'CONFIG_MAC_PVG_VMAPPLE', if_true: 
[files('apple-gfx-vmapple.m'), pvg, 

[PATCH 02/26] hw/vmapple/apple-gfx: BQL renaming update

[Phil Dennis-Jordan]

Since the original apple-gfx code was written, the BQL functions have
been renamed. This change updates the apple-gfx code to use the new
names.

Signed-off-by: Phil Dennis-Jordan 
---
 hw/vmapple/apple-gfx.m | 28 ++--
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/hw/vmapple/apple-gfx.m b/hw/vmapple/apple-gfx.m
index f75da5c610..3b437e2519 100644
--- a/hw/vmapple/apple-gfx.m
+++ b/hw/vmapple/apple-gfx.m
@@ -168,9 +168,9 @@ static void apple_gfx_write(void *opaque, hwaddr offset, 
uint64_t val, unsigned
 
 trace_apple_gfx_write(offset, val);
 
-qemu_mutex_unlock_iothread();
+bql_unlock();
 [s->pgdev mmioWriteAtOffset:offset value:val];
-qemu_mutex_lock_iothread();
+bql_lock();
 }
 
 static const MemoryRegionOps apple_gfx_ops = {
@@ -192,9 +192,9 @@ static uint64_t apple_iosfc_read(void *opaque, hwaddr 
offset, unsigned size)
 AppleGFXState *s = opaque;
 uint64_t res = 0;
 
-qemu_mutex_unlock_iothread();
+bql_unlock();
 res = [s->pgiosfc mmioReadAtOffset:offset];
-qemu_mutex_lock_iothread();
+bql_lock();
 
 trace_apple_iosfc_read(offset, res);
 
@@ -396,7 +396,7 @@ static void apple_gfx_realize(DeviceState *dev, Error 
**errp)
 PGPhysicalMemoryRange_t *range = &ranges[i];
 MemoryRegion *tmp_mr;
 /* TODO: Bounds checks? r/o? */
-qemu_mutex_lock_iothread();
+bql_lock();
 AppleGFXMR *mr = apple_gfx_mapMemory(s, task, virtualOffset,
  range->physicalAddress,
  range->physicalLength);
@@ -416,7 +416,7 @@ static void apple_gfx_realize(DeviceState *dev, Error 
**errp)
 trace_apple_gfx_remap(retval, source, target);
 g_assert(retval == KERN_SUCCESS);
 
-qemu_mutex_unlock_iothread();
+bql_unlock();
 
 virtualOffset += mr->len;
 }
@@ -428,7 +428,7 @@ static void apple_gfx_realize(DeviceState *dev, Error 
**errp)
 AppleGFXMR *mr, *next;
 
 trace_apple_gfx_unmap_memory(task, virtualOffset, length);
-qemu_mutex_lock_iothread();
+bql_lock();
 QTAILQ_FOREACH_SAFE(mr, &s->mrs, node, next) {
 if (mr->va >= (task->mem + virtualOffset) &&
 (mr->va + mr->len) <= (task->mem + virtualOffset + length)) {
@@ -438,7 +438,7 @@ static void apple_gfx_realize(DeviceState *dev, Error 
**errp)
 g_free(mr);
 }
 }
-qemu_mutex_unlock_iothread();
+bql_unlock();
 return (bool)true;
 };
 
@@ -452,13 +452,13 @@ static void apple_gfx_realize(DeviceState *dev, Error 
**errp)
 bool locked;
 
 trace_apple_gfx_raise_irq(vector);
-locked = qemu_mutex_iothread_locked();
+locked = bql_locked();
 if (!locked) {
-qemu_mutex_lock_iothread();
+bql_lock();
 }
 qemu_irq_pulse(s->irq_gfx);
 if (!locked) {
-qemu_mutex_unlock_iothread();
+bql_unlock();
 }
 };
 
@@ -534,13 +534,13 @@ static void apple_gfx_realize(DeviceState *dev, Error 
**errp)
 
 iosfc_desc.raiseInterrupt = ^(uint32_t vector) {
 trace_apple_iosfc_raise_irq(vector);
-bool locked = qemu_mutex_iothread_locked();
+bool locked = bql_locked();
 if (!locked) {
-qemu_mutex_lock_iothread();
+bql_lock();
 }
 qemu_irq_pulse(s->irq_iosfc);
 if (!locked) {
-qemu_mutex_unlock_iothread();
+bql_unlock();
 }
 return (bool)true;
 };
-- 
2.39.3 (Apple Git-146)

[PATCH 05/26] hw/display/apple-gfx: native -> little endian memory ops

[Phil Dennis-Jordan]

This is considered best practice.

Signed-off-by: Phil Dennis-Jordan 
---
 hw/display/apple-gfx.m | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/display/apple-gfx.m b/hw/display/apple-gfx.m
index 87bcdcd98e..8b0459f969 100644
--- a/hw/display/apple-gfx.m
+++ b/hw/display/apple-gfx.m
@@ -174,7 +174,7 @@ static void apple_gfx_write(void *opaque, hwaddr offset, 
uint64_t val, unsigned
 static const MemoryRegionOps apple_gfx_ops = {
 .read = apple_gfx_read,
 .write = apple_gfx_write,
-.endianness = DEVICE_NATIVE_ENDIAN,
+.endianness = DEVICE_LITTLE_ENDIAN,
 .valid = {
 .min_access_size = 4,
 .max_access_size = 8,
-- 
2.39.3 (Apple Git-146)