Re: [RFC XEN PATCH v4 2/5] x86/pvh: Allow (un)map_pirq when caller isn't DOMID_SELF
On 2024/1/8 17:25, Jan Beulich wrote:
> On 08.01.2024 10:15, Chen, Jiqian wrote:
>> On 2024/1/8 16:47, Jan Beulich wrote:
>>> On 06.01.2024 01:46, Stefano Stabellini wrote:
On Fri, 5 Jan 2024, Jiqian Chen wrote:
> @@ -72,8 +73,30 @@ long hvm_physdev_op(int cmd,
> XEN_GUEST_HANDLE_PARAM(void) arg)
>
> switch ( cmd )
> {
> -case PHYSDEVOP_map_pirq:
> -case PHYSDEVOP_unmap_pirq:
> +case PHYSDEVOP_map_pirq: {
> +physdev_map_pirq_t map;
> +
> +if ( copy_from_guest(, arg, 1) != 0 )
> +return -EFAULT;
> +
> +if ( !has_pirq(currd) && map.domid == DOMID_SELF )
> +return -ENOSYS;
This looks OK to me although there is already another copy_from_guest in
do_physdev_op, but I don't see an easy way to make it better.
>>>
>>> How can double reads of hypercall args ever be okay? The new check clearly
>>> needs to be inserted in the code path where the structure is being read
>>> already anyway.
>> I also tried to add this check in PHYSDEVOP_map_pirq in physdev.c, but pv
>> has no flag X86_EMU_USE_PIRQ too.
>> If want to add it into physdev.c and combine Stefano's opinions, this check
>> may be like:
>>
>> diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c
>> index 47c4da0af7e1..c38d4d405726 100644
>> --- a/xen/arch/x86/physdev.c
>> +++ b/xen/arch/x86/physdev.c
>> @@ -303,11 +303,19 @@ ret_t do_physdev_op(int cmd,
>> XEN_GUEST_HANDLE_PARAM(void) arg)
>> case PHYSDEVOP_map_pirq: {
>> physdev_map_pirq_t map;
>> struct msi_info msi;
>> +struct domain *d;
>>
>> ret = -EFAULT;
>> if ( copy_from_guest(, arg, 1) != 0 )
>> break;
>>
>> +d = rcu_lock_domain_by_any_id(map.domid);
>> +if ( d == NULL )
>> +return -ESRCH;
>> +if ( !is_pv_domain(d) && !has_pirq(d) )
>> +return -ENOSYS;
>> +rcu_unlock_domain(d);
>> +
>> switch ( map.type )
>> {
>> case MAP_PIRQ_TYPE_MSI_SEG:
>
> Well, yes, perhaps kind of like that, but with rcu_unlock_domain() called
> on the error 2nd return path as well, and without abusing ENOSYS.
Ok, will call rcu_unlock_domain on 2nd error path, and change ENOSYS to
EOPNOTSUPP.
>
> Jan
--
Best regards,
Jiqian Chen.
[linux-5.4 test] 184281: regressions - FAIL
flight 184281 linux-5.4 real [real] http://logs.test-lab.xenproject.org/osstest/logs/184281/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-arm64-arm64-libvirt-raw 17 guest-start/debian.repeat fail REGR. vs. 184192 test-armhf-armhf-xl-multivcpu 18 guest-start/debian.repeat fail in 184275 REGR. vs. 184192 Tests which are failing intermittently (not blocking): test-amd64-amd64-xl-qemut-debianhvm-i386-xsm 12 debian-hvm-install fail in 184275 pass in 184281 test-armhf-armhf-xl-multivcpu 14 guest-start fail pass in 184275 test-armhf-armhf-xl-credit2 14 guest-startfail pass in 184275 test-armhf-armhf-xl 18 guest-start/debian.repeat fail pass in 184275 Regressions which are regarded as allowable (not blocking): test-armhf-armhf-xl-rtds 14 guest-start fail REGR. vs. 184192 Tests which did not succeed, but are not blocking: test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail blocked in 184192 test-armhf-armhf-xl-credit2 18 guest-start/debian.repeat fail in 184275 like 184192 test-armhf-armhf-xl-multivcpu 15 migrate-support-check fail in 184275 never pass test-armhf-armhf-xl-multivcpu 16 saverestore-support-check fail in 184275 never pass test-armhf-armhf-xl-credit2 15 migrate-support-check fail in 184275 never pass test-armhf-armhf-xl-credit2 16 saverestore-support-check fail in 184275 never pass test-armhf-armhf-xl-credit1 14 guest-start fail like 184192 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 184192 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 184192 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 184192 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 184192 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 184192 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 184192 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 184192 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 184192 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 184192 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 184192 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 184192 test-amd64-i386-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-i386-libvirt 15 migrate-support-checkfail never pass test-amd64-i386-xl-pvshim14 guest-start fail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-xl 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-amd64-i386-libvirt-raw 14 migrate-support-checkfail never pass test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail never pass test-arm64-arm64-xl-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 14 migrate-support-checkfail
[linux-linus test] 184283: regressions - FAIL
flight 184283 linux-linus real [real] http://logs.test-lab.xenproject.org/osstest/logs/184283/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-arm64-pvops 6 kernel-build fail REGR. vs. 184270 Tests which did not succeed, but are not blocking: test-arm64-arm64-examine 1 build-check(1) blocked n/a test-arm64-arm64-libvirt-raw 1 build-check(1) blocked n/a test-arm64-arm64-libvirt-xsm 1 build-check(1) blocked n/a test-arm64-arm64-xl 1 build-check(1) blocked n/a test-arm64-arm64-xl-credit1 1 build-check(1) blocked n/a test-arm64-arm64-xl-credit2 1 build-check(1) blocked n/a test-arm64-arm64-xl-thunderx 1 build-check(1) blocked n/a test-arm64-arm64-xl-vhd 1 build-check(1) blocked n/a test-arm64-arm64-xl-xsm 1 build-check(1) blocked n/a test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 184270 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 184270 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 184270 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 184270 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 184270 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 184270 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 184270 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 184270 test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qcow2 14 migrate-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-rtds 15 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit1 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-raw 14 migrate-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 15 migrate-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass version targeted for testing: linux5db8752c3b81bd33a549f6f812bab81e3bb61b20 baseline version: linux0dd3ee31125508cd67f7e7172247f05b7fd1753a Last test of basis 184270 2024-01-07 20:42:19 Z1 days Testing same since 184283 2024-01-08 20:10:43 Z0 days1 attempts People who touched revisions under test: Abhinav Singh Al Viro Alexander Mikhalitsyn Amir Goldstein Andrei Vagin Andrew Donnellan # ocxl Bagas Sanjaya Chandan Babu R Christian Brauner Christoph Hellwig Christophe JAILLET Chuck Lever Darrick J. Wong David Sterba Dmitry Antipov Dmitry Torokhov Edward Adam Davis Eric Farman # s390 Hao Ge Jan Harkes Jan Kara Jann Horn Jeff Layton Jens Axboe Jia Zhu Kent Overstreet Linus Torvalds Lukas Schauer Martin K. Petersen Mateusz Guzik Matthew Wilcox (Oracle) Miklos Szeredi Randy Dunlap Ryusuke Konishi Vegard Nossum Xin Yin Xu Yilun YangXin jobs: build-amd64-xsm pass build-arm64-xsm pass build-i386-xsm pass build-amd64 pass build-arm64 pass build-armhf pass build-i386 pass
[ovmf test] 184288: all pass - PUSHED
flight 184288 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/184288/ Perfect :-) All tests in this flight passed as required version targeted for testing: ovmf f2b074398ca0624206355524a1c5f653ff87876a baseline version: ovmf e7152e6186d31bc138fbd2592e07886005177aab Last test of basis 184276 2024-01-08 10:43:33 Z0 days Testing same since 184288 2024-01-09 03:12:49 Z0 days1 attempts People who touched revisions under test: Suqiang Ren jobs: build-amd64-xsm pass build-i386-xsm pass build-amd64 pass build-i386 pass build-amd64-libvirt pass build-i386-libvirt pass build-amd64-pvopspass build-i386-pvops pass test-amd64-amd64-xl-qemuu-ovmf-amd64 pass test-amd64-i386-xl-qemuu-ovmf-amd64 pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Pushing revision : To xenbits.xen.org:/home/xen/git/osstest/ovmf.git e7152e6186..f2b074398c f2b074398ca0624206355524a1c5f653ff87876a -> xen-tested-master
[xen-unstable test] 184278: regressions - FAIL
flight 184278 xen-unstable real [real] http://logs.test-lab.xenproject.org/osstest/logs/184278/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-arm64-xsm 6 xen-buildfail REGR. vs. 184271 Tests which did not succeed, but are not blocking: test-arm64-arm64-libvirt-xsm 1 build-check(1) blocked n/a test-arm64-arm64-xl-xsm 1 build-check(1) blocked n/a test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 184271 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 184271 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 184271 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 184271 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 184271 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 184271 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 184271 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 184271 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 184271 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 184271 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 184271 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 184271 test-amd64-i386-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-i386-libvirt 15 migrate-support-checkfail never pass test-amd64-i386-xl-pvshim14 guest-start fail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit1 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 16 saverestore-support-checkfail never pass test-amd64-i386-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-raw 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-xl-rtds 15 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail never pass version targeted for testing: xen 2467ab2a74463107132f7d929d2ff0e30a8b6fa3 baseline version: xen 5a3ace21f3d779b291a2d305824b2820d88de7f1 Last test of basis 184271 2024-01-08 01:53:47 Z0 days Testing same since 184278 2024-01-08 12:39:26 Z0 days1 attempts People who touched revisions under test: Jan Beulich Juergen Gross Julien Grall Oleksii Kurochko
Re: [PATCH v5 04/13] xen: extend domctl interface for cache coloring
On Mon, 8 Jan 2024, Carlo Nonato wrote: > Hi Jan, > > On Mon, Jan 8, 2024 at 5:40 PM Jan Beulich wrote: > > > > On 08.01.2024 17:31, Carlo Nonato wrote: > > > On Mon, Jan 8, 2024 at 4:32 PM Julien Grall wrote: > > >> On 08/01/2024 15:18, Carlo Nonato wrote: > > No. I am saying that that we should not be able to allow changing the > > colors after the memory has been allocated. To give an example, your > > current code would allow: > > > > 1) Setup the P2M pools or allocate RAM > > 2) Set the color > > > > This would render the coloring configuration moot. > > > > Whether we want to allow changing the coloring before hand is a > > different question and as I wrote earlier on, I don't mind if you want > > to forbid that. > > >>> > > >>> At the moment I'm relying on the toolstack in the sense that I know > > >>> that it > > >>> will set colors right after domain creation and before memory > > >>> allocation. > > >>> Calling alloc_domheap_pages() without a coloring configuration makes Xen > > >>> crash, so it's mandatory to have the configuration done before any > > >>> allocation. > > >>> I know that we shouldn't rely on the toolstack this much, but I didn't > > >>> find a better way. Given this assumption, looking for an already > > >>> existing > > >>> color configuration of a domain is sufficient to avoid what you are > > >>> saying. > > >>> > > >>> Is it possible to enforce such a constraint with domctl? > I mean to be > > >>> sure that this domctl will be called at a precise time. > > >> > > >> Yes. You can... > > >> > > >>> > > >>> Thanks. > > >>> > > > I don't know what to check that. > > > > You can check the size of the P2M pool (d->arch.paging.p2m_total_pages) > > is still 0. I think for RAM, you can check d->tot_pages == 0. > > >> > > >> ... reject the call if either of the two fields are not zero. > > > > > > What I'm saying is that Xen would crash before even reaching this point > > > if no > > > colors were provided. Let's say that the toolstack or whatever hypercall > > > user > > > isn't calling this domctl at all (or not at the right time), then the > > > domain > > > colored allocator would always return null pages since there are no > > > colors. > > > We would have a crash and your if (or mine) would be useless. > > > > Why is it that you can't simply allocated arbitrary memory if coloring > > information wasn't set up front? Aiui that'll be required anyway, as > > there shouldn't be a restriction that all domains have to use coloring. > > If coloring is enabled all domains are colored. It's one of our first > assumptions. We haven't developed something that works hybridly and supporting > that would require some rework. I think that's a good assumption and I wouldn't go in the direction of supporting a mix of colored and non-colored. To benefit from cache coloring, all domains need to be colored, otherwise a single non-colored domain could thrash the cache of everyone else.
[PATCH v3 33/46] hw/m68k/q800: use qemu_find_nic_info()
From: David Woodhouse
If a corresponding NIC configuration was found, it will have a MAC address
already assigned, so use that. Else, generate and assign a default one.
Using qemu_find_nic_info() is simpler than the alternative of using
qemu_configure_nic_device() and then having to fetch the "mac" property
as a string and convert it.
Signed-off-by: David Woodhouse
---
hw/m68k/q800.c | 29 -
1 file changed, 16 insertions(+), 13 deletions(-)
diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
index 83d1571d02..c9e7bdf197 100644
--- a/hw/m68k/q800.c
+++ b/hw/m68k/q800.c
@@ -48,6 +48,7 @@
#include "hw/display/macfb.h"
#include "hw/block/swim.h"
#include "net/net.h"
+#include "net/util.h"
#include "qapi/error.h"
#include "qemu/error-report.h"
#include "sysemu/qtest.h"
@@ -271,6 +272,8 @@ static void q800_machine_init(MachineState *machine)
BusState *adb_bus;
NubusBus *nubus;
DriveInfo *dinfo;
+NICInfo *nd;
+MACAddr mac;
uint8_t rng_seed[32];
linux_boot = (kernel_filename != NULL);
@@ -371,13 +374,6 @@ static void q800_machine_init(MachineState *machine)
/* MACSONIC */
-if (nb_nics > 1) {
-error_report("q800 can only have one ethernet interface");
-exit(1);
-}
-
-qemu_check_nic_model(_table[0], "dp83932");
-
/*
* MacSonic driver needs an Apple MAC address
* Valid prefix are:
@@ -387,14 +383,21 @@ static void q800_machine_init(MachineState *machine)
* 08:00:07 Apple
* (Q800 use the last one)
*/
-nd_table[0].macaddr.a[0] = 0x08;
-nd_table[0].macaddr.a[1] = 0x00;
-nd_table[0].macaddr.a[2] = 0x07;
-
object_initialize_child(OBJECT(machine), "dp8393x", >dp8393x,
TYPE_DP8393X);
dev = DEVICE(>dp8393x);
-qdev_set_nic_properties(dev, _table[0]);
+nd = qemu_find_nic_info(TYPE_DP8393X, true, "dp83932");
+if (nd) {
+qdev_set_nic_properties(dev, nd);
+memcpy(mac.a, nd->macaddr.a, sizeof(mac.a));
+} else {
+qemu_macaddr_default_if_unset();
+}
+mac.a[0] = 0x08;
+mac.a[1] = 0x00;
+mac.a[2] = 0x07;
+qdev_prop_set_macaddr(dev, "mac", mac.a);
+
qdev_prop_set_uint8(dev, "it_shift", 2);
qdev_prop_set_bit(dev, "big_endian", true);
object_property_set_link(OBJECT(dev), "dma_mr",
@@ -415,7 +418,7 @@ static void q800_machine_init(MachineState *machine)
prom = memory_region_get_ram_ptr(dp8393x_prom);
checksum = 0;
for (i = 0; i < 6; i++) {
-prom[i] = revbit8(nd_table[0].macaddr.a[i]);
+prom[i] = revbit8(mac.a[i]);
checksum ^= prom[i];
}
prom[7] = 0xff - checksum;
--
2.41.0
[PATCH v3 46/46] net: make nb_nics and nd_table[] static in net/net.c
From: David Woodhouse
Signed-off-by: David Woodhouse
---
include/net/net.h | 4
net/net.c | 3 +++
system/globals.c | 2 --
3 files changed, 3 insertions(+), 6 deletions(-)
diff --git a/include/net/net.h b/include/net/net.h
index 19fb82833c..766201c62c 100644
--- a/include/net/net.h
+++ b/include/net/net.h
@@ -247,10 +247,6 @@ struct NICInfo {
int nvectors;
};
-extern int nb_nics;
-extern NICInfo nd_table[MAX_NICS];
-extern const char *host_net_devices[];
-
/* from net.c */
extern NetClientStateList net_clients;
bool netdev_is_modern(const char *optstr);
diff --git a/net/net.c b/net/net.c
index 09ab0889f5..71cccb19da 100644
--- a/net/net.c
+++ b/net/net.c
@@ -77,6 +77,9 @@ static NetdevQueue nd_queue =
QSIMPLEQ_HEAD_INITIALIZER(nd_queue);
static GHashTable *nic_model_help;
+static int nb_nics;
+static NICInfo nd_table[MAX_NICS];
+
/***/
/* network device redirectors */
diff --git a/system/globals.c b/system/globals.c
index e83b5428d1..b6d4e72530 100644
--- a/system/globals.c
+++ b/system/globals.c
@@ -36,8 +36,6 @@ int display_opengl;
const char* keyboard_layout;
bool enable_mlock;
bool enable_cpu_pm;
-int nb_nics;
-NICInfo nd_table[MAX_NICS];
int autostart = 1;
int vga_interface_type = VGA_NONE;
bool vga_interface_created;
--
2.41.0
[PATCH v3 31/46] hw/net/etraxfs-eth: use qemu_configure_nic_device()
From: David Woodhouse
Signed-off-by: David Woodhouse
---
hw/cris/axis_dev88.c | 9 -
hw/net/etraxfs_eth.c | 5 ++---
include/hw/cris/etraxfs.h | 2 +-
3 files changed, 7 insertions(+), 9 deletions(-)
diff --git a/hw/cris/axis_dev88.c b/hw/cris/axis_dev88.c
index d82050d927..5556634921 100644
--- a/hw/cris/axis_dev88.c
+++ b/hw/cris/axis_dev88.c
@@ -308,15 +308,14 @@ void axisdev88_init(MachineState *machine)
/* Add the two ethernet blocks. */
dma_eth = g_malloc0(sizeof dma_eth[0] * 4); /* Allocate 4 channels. */
-etraxfs_eth_init(_table[0], 0x30034000, 1, _eth[0], _eth[1]);
-if (nb_nics > 1) {
-etraxfs_eth_init(_table[1], 0x30036000, 2, _eth[2],
_eth[3]);
-}
+etraxfs_eth_init(0x30034000, 1, _eth[0], _eth[1]);
/* The DMA Connector block is missing, hardwire things for now. */
etraxfs_dmac_connect_client(etraxfs_dmac, 0, _eth[0]);
etraxfs_dmac_connect_client(etraxfs_dmac, 1, _eth[1]);
-if (nb_nics > 1) {
+
+if (qemu_find_nic_info("etraxfs-eth", true, "fseth")) {
+etraxfs_eth_init(0x30036000, 2, _eth[2], _eth[3]);
etraxfs_dmac_connect_client(etraxfs_dmac, 6, _eth[2]);
etraxfs_dmac_connect_client(etraxfs_dmac, 7, _eth[3]);
}
diff --git a/hw/net/etraxfs_eth.c b/hw/net/etraxfs_eth.c
index ba57a978d1..5faf20c782 100644
--- a/hw/net/etraxfs_eth.c
+++ b/hw/net/etraxfs_eth.c
@@ -647,15 +647,14 @@ static void etraxfs_eth_class_init(ObjectClass *klass,
void *data)
/* Instantiate an ETRAXFS Ethernet MAC. */
DeviceState *
-etraxfs_eth_init(NICInfo *nd, hwaddr base, int phyaddr,
+etraxfs_eth_init(hwaddr base, int phyaddr,
struct etraxfs_dma_client *dma_out,
struct etraxfs_dma_client *dma_in)
{
DeviceState *dev;
-qemu_check_nic_model(nd, "fseth");
dev = qdev_new("etraxfs-eth");
-qdev_set_nic_properties(dev, nd);
+qemu_configure_nic_device(dev, true, "fseth");
qdev_prop_set_uint32(dev, "phyaddr", phyaddr);
/*
diff --git a/include/hw/cris/etraxfs.h b/include/hw/cris/etraxfs.h
index 467b529dc0..012c4e9974 100644
--- a/include/hw/cris/etraxfs.h
+++ b/include/hw/cris/etraxfs.h
@@ -31,7 +31,7 @@
#include "hw/sysbus.h"
#include "qapi/error.h"
-DeviceState *etraxfs_eth_init(NICInfo *nd, hwaddr base, int phyaddr,
+DeviceState *etraxfs_eth_init(hwaddr base, int phyaddr,
struct etraxfs_dma_client *dma_out,
struct etraxfs_dma_client *dma_in);
--
2.41.0
[PATCH v3 04/46] hw/pci: add pci_init_nic_devices(), pci_init_nic_in_slot()
From: David Woodhouse
The loop over nd_table[] to add PCI NICs is repeated in quite a few
places. Add a helper function to do it.
Some platforms also try to instantiate a specific model in a specific
slot, to match the real hardware. Add pci_init_nic_in_slot() for that
purpose.
Signed-off-by: David Woodhouse
Reviewed-by: Paul Durrant
---
hw/pci/pci.c | 45
include/hw/pci/pci.h | 4 +++-
2 files changed, 48 insertions(+), 1 deletion(-)
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 76080af580..5849606f66 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1925,6 +1925,51 @@ PCIDevice *pci_nic_init_nofail(NICInfo *nd, PCIBus
*rootbus,
return pci_dev;
}
+void pci_init_nic_devices(PCIBus *bus, const char *default_model)
+{
+qemu_create_nic_bus_devices(>qbus, TYPE_PCI_DEVICE, default_model,
+"virtio", "virtio-net-pci");
+}
+
+bool pci_init_nic_in_slot(PCIBus *rootbus, const char *model,
+ const char *alias, const char *devaddr)
+{
+NICInfo *nd = qemu_find_nic_info(model, true, alias);
+int dom, busnr, devfn;
+PCIDevice *pci_dev;
+unsigned slot;
+PCIBus *bus;
+
+if (!nd) {
+return false;
+}
+
+if (!devaddr || pci_parse_devaddr(devaddr, , , , NULL) < 0)
{
+error_report("Invalid PCI device address %s for device %s",
+ devaddr, model);
+exit(1);
+}
+
+if (dom != 0) {
+error_report("No support for non-zero PCI domains");
+exit(1);
+}
+
+devfn = PCI_DEVFN(slot, 0);
+
+bus = pci_find_bus_nr(rootbus, busnr);
+if (!bus) {
+error_report("Invalid PCI device address %s for device %s",
+ devaddr, model);
+exit(1);
+}
+
+pci_dev = pci_new(devfn, model);
+qdev_set_nic_properties(_dev->qdev, nd);
+pci_realize_and_unref(pci_dev, bus, _fatal);
+return true;
+}
+
PCIDevice *pci_vga_init(PCIBus *bus)
{
vga_interface_created = true;
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index fa6313aabc..6ff0b95a02 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -317,7 +317,9 @@ void pci_device_reset(PCIDevice *dev);
PCIDevice *pci_nic_init_nofail(NICInfo *nd, PCIBus *rootbus,
const char *default_model,
const char *default_devaddr);
-
+void pci_init_nic_devices(PCIBus *bus, const char *default_model);
+bool pci_init_nic_in_slot(PCIBus *rootbus, const char *default_model,
+ const char *alias, const char *devaddr);
PCIDevice *pci_vga_init(PCIBus *bus);
static inline PCIBus *pci_get_bus(const PCIDevice *dev)
--
2.41.0
[PATCH v3 01/46] net: add qemu_{configure,create}_nic_device(), qemu_find_nic_info()
From: David Woodhouse
Most code which directly accesses nd_table[] and nb_nics uses them for
one of two things. Either "I have created a NIC device and I'd like a
configuration for it", or "I will create a NIC device *if* there is a
configuration for it". With some variants on the theme around whether
they actually *check* if the model specified in the configuration is
the right one.
Provide functions which perform both of those, allowing platforms to
be a little more consistent and as a step towards making nd_table[]
and nb_nics private to the net code.
Also export the qemu_find_nic_info() helper, as some platforms have
special cases they need to handle.
Signed-off-by: David Woodhouse
Reviewed-by: Paul Durrant
---
include/net/net.h | 7 ++-
net/net.c | 51 +++
2 files changed, 57 insertions(+), 1 deletion(-)
diff --git a/include/net/net.h b/include/net/net.h
index ffbd2c8d56..25ea83fd12 100644
--- a/include/net/net.h
+++ b/include/net/net.h
@@ -207,7 +207,12 @@ int qemu_show_nic_models(const char *arg, const char
*const *models);
void qemu_check_nic_model(NICInfo *nd, const char *model);
int qemu_find_nic_model(NICInfo *nd, const char * const *models,
const char *default_model);
-
+NICInfo *qemu_find_nic_info(const char *typename, bool match_default,
+const char *alias);
+bool qemu_configure_nic_device(DeviceState *dev, bool match_default,
+ const char *alias);
+DeviceState *qemu_create_nic_device(const char *typename, bool match_default,
+const char *alias);
void print_net_client(Monitor *mon, NetClientState *nc);
void net_socket_rs_init(SocketReadState *rs,
SocketReadStateFinalize *finalize,
diff --git a/net/net.c b/net/net.c
index 0520bc1681..aeb7f573fc 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1087,6 +1087,57 @@ static int net_init_nic(const Netdev *netdev, const char
*name,
return idx;
}
+NICInfo *qemu_find_nic_info(const char *typename, bool match_default,
+const char *alias)
+{
+NICInfo *nd;
+int i;
+
+for (i = 0; i < nb_nics; i++) {
+nd = _table[i];
+
+if (!nd->used || nd->instantiated) {
+continue;
+}
+
+if ((match_default && !nd->model) || !g_strcmp0(nd->model, typename)
+|| (alias && !g_strcmp0(nd->model, alias))) {
+return nd;
+}
+}
+return NULL;
+}
+
+
+/* "I have created a device. Please configure it if you can" */
+bool qemu_configure_nic_device(DeviceState *dev, bool match_default,
+ const char *alias)
+{
+NICInfo *nd = qemu_find_nic_info(object_get_typename(OBJECT(dev)),
+ match_default, alias);
+
+if (nd) {
+qdev_set_nic_properties(dev, nd);
+return true;
+}
+return false;
+}
+
+/* "Please create a device, if you have a configuration for it" */
+DeviceState *qemu_create_nic_device(const char *typename, bool match_default,
+const char *alias)
+{
+NICInfo *nd = qemu_find_nic_info(typename, match_default, alias);
+DeviceState *dev;
+
+if (!nd) {
+return NULL;
+}
+
+dev = qdev_new(typename);
+qdev_set_nic_properties(dev, nd);
+return dev;
+}
static int (* const net_client_init_fun[NET_CLIENT_DRIVER__MAX])(
const Netdev *netdev,
--
2.41.0
[PATCH v3 41/46] hw/sparc/sun4m: use qemu_find_nic_info()
From: David Woodhouse
Obtain the MAC address from the NIC configuration if there is one, or
generate one explicitly so that it can be placed in the PROM.
Signed-off-by: David Woodhouse
---
hw/sparc/sun4m.c | 20 ++--
1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
index 550af01690..e782c8ec7a 100644
--- a/hw/sparc/sun4m.c
+++ b/hw/sparc/sun4m.c
@@ -299,13 +299,15 @@ static void *iommu_init(hwaddr addr, uint32_t version,
qemu_irq irq)
static void *sparc32_dma_init(hwaddr dma_base,
hwaddr esp_base, qemu_irq espdma_irq,
- hwaddr le_base, qemu_irq ledma_irq, NICInfo *nd)
+ hwaddr le_base, qemu_irq ledma_irq,
+ MACAddr *mac)
{
DeviceState *dma;
ESPDMADeviceState *espdma;
LEDMADeviceState *ledma;
SysBusESPState *esp;
SysBusPCNetState *lance;
+NICInfo *nd = qemu_find_nic_info("lance", true, NULL);
dma = qdev_new(TYPE_SPARC32_DMA);
espdma = SPARC32_ESPDMA_DEVICE(object_resolve_path_component(
@@ -320,7 +322,14 @@ static void *sparc32_dma_init(hwaddr dma_base,
lance = SYSBUS_PCNET(object_resolve_path_component(
OBJECT(ledma), "lance"));
-qdev_set_nic_properties(DEVICE(lance), nd);
+
+if (nd) {
+qdev_set_nic_properties(DEVICE(lance), nd);
+memcpy(mac->a, nd->macaddr.a, sizeof(mac->a));
+} else {
+qemu_macaddr_default_if_unset(mac);
+qdev_prop_set_macaddr(DEVICE(lance), "mac", mac->a);
+}
sysbus_realize_and_unref(SYS_BUS_DEVICE(dma), _fatal);
sysbus_mmio_map(SYS_BUS_DEVICE(dma), 0, dma_base);
@@ -823,7 +832,7 @@ static void sun4m_hw_init(MachineState *machine)
unsigned int smp_cpus = machine->smp.cpus;
unsigned int max_cpus = machine->smp.max_cpus;
HostMemoryBackend *ram_memdev = machine->memdev;
-NICInfo *nd = _table[0];
+MACAddr hostid;
if (machine->ram_size > hwdef->max_mem) {
error_report("Too much memory for this machine: %" PRId64 ","
@@ -884,10 +893,9 @@ static void sun4m_hw_init(MachineState *machine)
hwdef->iommu_pad_base, hwdef->iommu_pad_len);
}
-qemu_check_nic_model(nd, TYPE_LANCE);
sparc32_dma_init(hwdef->dma_base,
hwdef->esp_base, slavio_irq[18],
- hwdef->le_base, slavio_irq[16], nd);
+ hwdef->le_base, slavio_irq[16], );
if (graphic_depth != 8 && graphic_depth != 24) {
error_report("Unsupported depth: %d", graphic_depth);
@@ -1039,7 +1047,7 @@ static void sun4m_hw_init(MachineState *machine)
machine->initrd_filename,
machine->ram_size, _size);
-nvram_init(nvram, (uint8_t *)>macaddr, machine->kernel_cmdline,
+nvram_init(nvram, hostid.a, machine->kernel_cmdline,
machine->boot_config.order, machine->ram_size, kernel_size,
graphic_width, graphic_height, graphic_depth,
hwdef->nvram_machine_id, "Sun4m");
--
2.41.0
[PATCH v3 36/46] hw/mips/jazz: use qemu_find_nic_info()
From: David Woodhouse
Extract the MAC address from the NICInfo, or generate one explicitly if
there was no corresponding NIC configuration, to put it in the PROM.
Signed-off-by: David Woodhouse
---
hw/mips/jazz.c | 15 +++
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/hw/mips/jazz.c b/hw/mips/jazz.c
index 0d2348aa5a..5bf3e328db 100644
--- a/hw/mips/jazz.c
+++ b/hw/mips/jazz.c
@@ -119,15 +119,19 @@ static const MemoryRegionOps dma_dummy_ops = {
.endianness = DEVICE_NATIVE_ENDIAN,
};
-static void mips_jazz_init_net(NICInfo *nd, IOMMUMemoryRegion *rc4030_dma_mr,
+static void mips_jazz_init_net(IOMMUMemoryRegion *rc4030_dma_mr,
DeviceState *rc4030, MemoryRegion *dp8393x_prom)
{
DeviceState *dev;
SysBusDevice *sysbus;
int checksum, i;
uint8_t *prom;
+NICInfo *nd;
-qemu_check_nic_model(nd, "dp83932");
+nd = qemu_find_nic_info("dp8393x", true, "dp82932");
+if (!nd) {
+return;
+}
dev = qdev_new("dp8393x");
qdev_set_nic_properties(dev, nd);
@@ -324,12 +328,7 @@ static void mips_jazz_init(MachineState *machine,
}
/* Network controller */
-if (nb_nics == 1) {
-mips_jazz_init_net(_table[0], rc4030_dma_mr, rc4030, dp8393x_prom);
-} else if (nb_nics > 1) {
-error_report("This machine only supports one NIC");
-exit(1);
-}
+mips_jazz_init_net(rc4030_dma_mr, rc4030, dp8393x_prom);
/* SCSI adapter */
dev = qdev_new(TYPE_SYSBUS_ESP);
--
2.41.0
[PATCH v3 17/46] hw/ppc: use pci_init_nic_devices()
From: David Woodhouse
Signed-off-by: David Woodhouse
---
hw/ppc/e500.c | 4 +---
hw/ppc/mac_newworld.c | 4 +---
hw/ppc/mac_oldworld.c | 4 +---
hw/ppc/ppc440_bamboo.c | 14 +-
4 files changed, 8 insertions(+), 18 deletions(-)
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 566f1200dd..3bd12b54ab 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -1079,9 +1079,7 @@ void ppce500_init(MachineState *machine)
if (pci_bus) {
/* Register network interfaces. */
-for (i = 0; i < nb_nics; i++) {
-pci_nic_init_nofail(_table[i], pci_bus, mc->default_nic, NULL);
-}
+pci_init_nic_devices(pci_bus, mc->default_nic);
}
/* Register spinning region */
diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index 535710314a..b36dbaf2b6 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -444,9 +444,7 @@ static void ppc_core99_init(MachineState *machine)
graphic_depth = 15;
}
-for (i = 0; i < nb_nics; i++) {
-pci_nic_init_nofail(_table[i], pci_bus, mc->default_nic, NULL);
-}
+pci_init_nic_devices(pci_bus, mc->default_nic);
/* The NewWorld NVRAM is not located in the MacIO device */
if (kvm_enabled() && qemu_real_host_page_size() > 4096) {
diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
index 9acc7adfc9..1981d3d8f6 100644
--- a/hw/ppc/mac_oldworld.c
+++ b/hw/ppc/mac_oldworld.c
@@ -277,9 +277,7 @@ static void ppc_heathrow_init(MachineState *machine)
pci_vga_init(pci_bus);
-for (i = 0; i < nb_nics; i++) {
-pci_nic_init_nofail(_table[i], pci_bus, mc->default_nic, NULL);
-}
+pci_init_nic_devices(pci_bus, mc->default_nic);
/* MacIO IDE */
ide_drive_get(hd, ARRAY_SIZE(hd));
diff --git a/hw/ppc/ppc440_bamboo.c b/hw/ppc/ppc440_bamboo.c
index a189942de4..c75c3083e6 100644
--- a/hw/ppc/ppc440_bamboo.c
+++ b/hw/ppc/ppc440_bamboo.c
@@ -161,7 +161,6 @@ static void bamboo_init(MachineState *machine)
DeviceState *uicdev;
SysBusDevice *uicsbd;
int success;
-int i;
if (kvm_enabled()) {
error_report("machine %s does not support the KVM accelerator",
@@ -234,14 +233,11 @@ static void bamboo_init(MachineState *machine)
}
if (pcibus) {
-/* Register network interfaces. */
-for (i = 0; i < nb_nics; i++) {
-/*
- * There are no PCI NICs on the Bamboo board, but there are
- * PCI slots, so we can pick whatever default model we want.
- */
-pci_nic_init_nofail(_table[i], pcibus, mc->default_nic, NULL);
-}
+/*
+ * There are no PCI NICs on the Bamboo board, but there are
+ * PCI slots, so we can pick whatever default model we want.
+ */
+pci_init_nic_devices(pcibus, mc->default_nic);
}
/* Load kernel. */
--
2.41.0
[PATCH v3 19/46] hw/sparc64/sun4u: use pci_init_nic_devices()
From: David Woodhouse
The first sunhme NIC gets placed a function 1 on slot 1 of PCI bus A,
and the rest are dynamically assigned on PCI bus B.
Previously, any PCI NIC would get the special treatment purely by
virtue of being first in the list.
Signed-off-by: David Woodhouse
---
hw/sparc64/sun4u.c | 27 ---
1 file changed, 8 insertions(+), 19 deletions(-)
diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
index 24d53bf5fd..eda9b58a21 100644
--- a/hw/sparc64/sun4u.c
+++ b/hw/sparc64/sun4u.c
@@ -639,29 +639,18 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
memset(, 0, sizeof(MACAddr));
onboard_nic = false;
-for (i = 0; i < nb_nics; i++) {
-PCIBus *bus;
-nd = _table[i];
-
-if (!nd->model || strcmp(nd->model, mc->default_nic) == 0) {
-if (!onboard_nic) {
-pci_dev = pci_new_multifunction(PCI_DEVFN(1, 1),
mc->default_nic);
-bus = pci_busA;
-memcpy(, >macaddr.a, sizeof(MACAddr));
-onboard_nic = true;
-} else {
-pci_dev = pci_new(-1, mc->default_nic);
-bus = pci_busB;
-}
-} else {
-pci_dev = pci_new(-1, nd->model);
-bus = pci_busB;
-}
+nd = qemu_find_nic_info(mc->default_nic, true, NULL);
+if (nd) {
+pci_dev = pci_new_multifunction(PCI_DEVFN(1, 1), mc->default_nic);
dev = _dev->qdev;
qdev_set_nic_properties(dev, nd);
-pci_realize_and_unref(pci_dev, bus, _fatal);
+pci_realize_and_unref(pci_dev, pci_busA, _fatal);
+
+memcpy(, >macaddr.a, sizeof(MACAddr));
+onboard_nic = true;
}
+pci_init_nic_devices(pci_busB, mc->default_nic);
/* If we don't have an onboard NIC, grab a default MAC address so that
* we have a valid machine id */
--
2.41.0
[PATCH v3 44/46] hw/pci: remove pci_nic_init_nofail()
From: David Woodhouse
This function is no longer used.
Signed-off-by: David Woodhouse
---
hw/pci/pci.c | 72
include/hw/pci/pci.h | 3 --
2 files changed, 75 deletions(-)
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 5849606f66..449abfb182 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1853,78 +1853,6 @@ const pci_class_desc *get_class_desc(int class)
return desc;
}
-/* Initialize a PCI NIC. */
-PCIDevice *pci_nic_init_nofail(NICInfo *nd, PCIBus *rootbus,
- const char *default_model,
- const char *default_devaddr)
-{
-const char *devaddr = nd->devaddr ? nd->devaddr : default_devaddr;
-GPtrArray *pci_nic_models;
-PCIBus *bus;
-PCIDevice *pci_dev;
-DeviceState *dev;
-int devfn;
-int i;
-int dom, busnr;
-unsigned slot;
-
-if (nd->model && !strcmp(nd->model, "virtio")) {
-g_free(nd->model);
-nd->model = g_strdup("virtio-net-pci");
-}
-
-pci_nic_models = qemu_get_nic_models(TYPE_PCI_DEVICE);
-
-if (qemu_show_nic_models(nd->model, (const char **)pci_nic_models->pdata))
{
-exit(0);
-}
-
-i = qemu_find_nic_model(nd, (const char **)pci_nic_models->pdata,
-default_model);
-if (i < 0) {
-exit(1);
-}
-
-if (!rootbus) {
-error_report("No primary PCI bus");
-exit(1);
-}
-
-assert(!rootbus->parent_dev);
-
-if (!devaddr) {
-devfn = -1;
-busnr = 0;
-} else {
-if (pci_parse_devaddr(devaddr, , , , NULL) < 0) {
-error_report("Invalid PCI device address %s for device %s",
- devaddr, nd->model);
-exit(1);
-}
-
-if (dom != 0) {
-error_report("No support for non-zero PCI domains");
-exit(1);
-}
-
-devfn = PCI_DEVFN(slot, 0);
-}
-
-bus = pci_find_bus_nr(rootbus, busnr);
-if (!bus) {
-error_report("Invalid PCI device address %s for device %s",
- devaddr, nd->model);
-exit(1);
-}
-
-pci_dev = pci_new(devfn, nd->model);
-dev = _dev->qdev;
-qdev_set_nic_properties(dev, nd);
-pci_realize_and_unref(pci_dev, bus, _fatal);
-g_ptr_array_free(pci_nic_models, true);
-return pci_dev;
-}
-
void pci_init_nic_devices(PCIBus *bus, const char *default_model)
{
qemu_create_nic_bus_devices(>qbus, TYPE_PCI_DEVICE, default_model,
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 6ff0b95a02..eaa3fc99d8 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -314,9 +314,6 @@ void pci_device_set_intx_routing_notifier(PCIDevice *dev,
PCIINTxRoutingNotifier notifier);
void pci_device_reset(PCIDevice *dev);
-PCIDevice *pci_nic_init_nofail(NICInfo *nd, PCIBus *rootbus,
- const char *default_model,
- const char *default_devaddr);
void pci_init_nic_devices(PCIBus *bus, const char *default_model);
bool pci_init_nic_in_slot(PCIBus *rootbus, const char *default_model,
const char *alias, const char *devaddr);
--
2.41.0
[PATCH v3 12/46] hw/mips/fuloong2e: use pci_init_nic_devices()
From: David Woodhouse
The previous behaviour was: *if* the first NIC specified on the command
line was an RTL8139 (or unspecified model) then it gets assigned to PCI
slot 7, which is where the Fuloong board had an RTL8139. All other
devices (including the first, if it was specified a anything other then
an rtl8319) get dynamically assigned on the bus.
The new behaviour is subtly different: If the first NIC was given a
specific model *other* than rtl8139, and a subsequent NIC was not,
then the rtl8139 (or unspecified) NIC will go to slot 7 and the rest
will be dynamically assigned.
Signed-off-by: David Woodhouse
---
hw/mips/fuloong2e.c | 16 +++-
1 file changed, 3 insertions(+), 13 deletions(-)
diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
index 97b2c8ed8e..a45aac368c 100644
--- a/hw/mips/fuloong2e.c
+++ b/hw/mips/fuloong2e.c
@@ -201,19 +201,9 @@ static void main_cpu_reset(void *opaque)
/* Network support */
static void network_init(PCIBus *pci_bus)
{
-int i;
-
-for (i = 0; i < nb_nics; i++) {
-NICInfo *nd = _table[i];
-const char *default_devaddr = NULL;
-
-if (i == 0 && (!nd->model || strcmp(nd->model, "rtl8139") == 0)) {
-/* The Fuloong board has a RTL8139 card using PCI SLOT 7 */
-default_devaddr = "07";
-}
-
-pci_nic_init_nofail(nd, pci_bus, "rtl8139", default_devaddr);
-}
+/* The Fuloong board has a RTL8139 card using PCI SLOT 7 */
+pci_init_nic_in_slot(pci_bus, "rtl8139", NULL, "07");
+pci_init_nic_devices(pci_bus, "rtl8139");
}
static void mips_fuloong2e_init(MachineState *machine)
--
2.41.0
[PATCH v3 28/46] hw/arm/npcm7xx: use qemu_configure_nic_device, allow emc0/emc1 as aliases
From: David Woodhouse
Also update the test to specify which device to attach the test socket
to, and remove the comment lamenting the fact that we can't do so.
Signed-off-by: David Woodhouse
---
hw/arm/npcm7xx.c | 16 +---
tests/qtest/npcm7xx_emc-test.c | 18 --
2 files changed, 13 insertions(+), 21 deletions(-)
diff --git a/hw/arm/npcm7xx.c b/hw/arm/npcm7xx.c
index 15ff21d047..ee395864e4 100644
--- a/hw/arm/npcm7xx.c
+++ b/hw/arm/npcm7xx.c
@@ -655,8 +655,9 @@ static void npcm7xx_realize(DeviceState *dev, Error **errp)
/*
* EMC Modules. Cannot fail.
- * The mapping of the device to its netdev backend works as follows:
- * emc[i] = nd_table[i]
+ * Use the available NIC configurations in order, allowing 'emc0' and
+ * 'emc1' to by used as aliases for the model= parameter to override.
+ *
* This works around the inability to specify the netdev property for the
* emc device: it's not pluggable and thus the -device option can't be
* used.
@@ -664,12 +665,13 @@ static void npcm7xx_realize(DeviceState *dev, Error
**errp)
QEMU_BUILD_BUG_ON(ARRAY_SIZE(npcm7xx_emc_addr) != ARRAY_SIZE(s->emc));
QEMU_BUILD_BUG_ON(ARRAY_SIZE(s->emc) != 2);
for (i = 0; i < ARRAY_SIZE(s->emc); i++) {
-s->emc[i].emc_num = i;
SysBusDevice *sbd = SYS_BUS_DEVICE(>emc[i]);
-if (nd_table[i].used) {
-qemu_check_nic_model(_table[i], TYPE_NPCM7XX_EMC);
-qdev_set_nic_properties(DEVICE(sbd), _table[i]);
-}
+char alias[6];
+
+s->emc[i].emc_num = i;
+snprintf(alias, sizeof(alias), "emc%u", i);
+qemu_configure_nic_device(DEVICE(sbd), true, alias);
+
/*
* The device exists regardless of whether it's connected to a QEMU
* netdev backend. So always instantiate it even if there is no
diff --git a/tests/qtest/npcm7xx_emc-test.c b/tests/qtest/npcm7xx_emc-test.c
index b046f1d76a..f7646fae2c 100644
--- a/tests/qtest/npcm7xx_emc-test.c
+++ b/tests/qtest/npcm7xx_emc-test.c
@@ -225,21 +225,11 @@ static int *packet_test_init(int module_num, GString
*cmd_line)
g_assert_cmpint(ret, != , -1);
/*
- * KISS and use -nic. We specify two nics (both emc{0,1}) because there's
- * currently no way to specify only emc1: The driver implicitly relies on
- * emc[i] == nd_table[i].
+ * KISS and use -nic. The driver accepts 'emc0' and 'emc1' as aliases
+ * in the 'model' field to specify the device to match.
*/
-if (module_num == 0) {
-g_string_append_printf(cmd_line,
- " -nic socket,fd=%d,model=" TYPE_NPCM7XX_EMC " "
- " -nic user,model=" TYPE_NPCM7XX_EMC " ",
- test_sockets[1]);
-} else {
-g_string_append_printf(cmd_line,
- " -nic user,model=" TYPE_NPCM7XX_EMC " "
- " -nic socket,fd=%d,model=" TYPE_NPCM7XX_EMC "
",
- test_sockets[1]);
-}
+g_string_append_printf(cmd_line, " -nic socket,fd=%d,model=emc%d ",
+ test_sockets[1], module_num);
g_test_queue_destroy(packet_test_clear, test_sockets);
return test_sockets;
--
2.41.0
[PATCH v3 08/46] hw/arm/sbsa-ref: use pci_init_nic_devices()
From: David Woodhouse
Signed-off-by: David Woodhouse
Reviewed-by: Leif Lindholm
---
hw/arm/sbsa-ref.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
index 477dca0637..f0171176ea 100644
--- a/hw/arm/sbsa-ref.c
+++ b/hw/arm/sbsa-ref.c
@@ -671,9 +671,7 @@ static void create_pcie(SBSAMachineState *sms)
pci = PCI_HOST_BRIDGE(dev);
if (pci->bus) {
-for (i = 0; i < nb_nics; i++) {
-pci_nic_init_nofail(_table[i], pci->bus, mc->default_nic, NULL);
-}
+pci_init_nic_devices(pci->bus, mc->default_nic);
}
pci_create_simple(pci->bus, -1, "bochs-display");
--
2.41.0
[PATCH v3 05/46] hw/i386/pc: use qemu_get_nic_info() and pci_init_nic_devices()
From: David Woodhouse
Eliminate direct access to nd_table[] and nb_nics by processing the the
Xen and ISA NICs first and then calling pci_init_nic_devices() for the
rest.
Signed-off-by: David Woodhouse
Reviewed-by: Paul Durrant
---
hw/i386/pc.c| 26 --
include/hw/net/ne2000-isa.h | 2 --
2 files changed, 16 insertions(+), 12 deletions(-)
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 496498df3a..d80c536d88 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -658,8 +658,11 @@ static void pc_init_ne2k_isa(ISABus *bus, NICInfo *nd)
{
static int nb_ne2k = 0;
-if (nb_ne2k == NE2000_NB_MAX)
+if (nb_ne2k == NE2000_NB_MAX) {
+error_setg(_fatal,
+ "maximum number of ISA NE2000 devices exceeded");
return;
+}
isa_ne2000_init(bus, ne2000_io[nb_ne2k],
ne2000_irq[nb_ne2k], nd);
nb_ne2k++;
@@ -1297,23 +1300,26 @@ void pc_nic_init(PCMachineClass *pcmc, ISABus *isa_bus,
PCIBus *pci_bus,
BusState *xen_bus)
{
MachineClass *mc = MACHINE_CLASS(pcmc);
-int i;
+bool default_is_ne2k = g_str_equal(mc->default_nic, TYPE_ISA_NE2000);
+NICInfo *nd;
rom_set_order_override(FW_CFG_ORDER_OVERRIDE_NIC);
-for (i = 0; i < nb_nics; i++) {
-NICInfo *nd = _table[i];
-const char *model = nd->model ? nd->model : mc->default_nic;
-if (xen_bus && (!nd->model || g_str_equal(model, "xen-net-device"))) {
+if (xen_bus) {
+while (nc = qemu_find_nic_info("xen-net-device", true, NULL)) {
DeviceState *dev = qdev_new("xen-net-device");
qdev_set_nic_properties(dev, nd);
qdev_realize_and_unref(dev, xen_bus, _fatal);
-} else if (g_str_equal(model, "ne2k_isa")) {
-pc_init_ne2k_isa(isa_bus, nd);
-} else {
-pci_nic_init_nofail(nd, pci_bus, model, NULL);
}
}
+
+while ((nd = qemu_find_nic_info(TYPE_ISA_NE2000, default_is_ne2k, NULL))) {
+pc_init_ne2k_isa(isa_bus, nd);
+}
+
+/* Anything remaining should be a PCI NIC */
+pci_init_nic_devices(pci_bus, mc->default_nic);
+
rom_reset_order_override();
}
diff --git a/include/hw/net/ne2000-isa.h b/include/hw/net/ne2000-isa.h
index af59ee0b02..73bae10ad1 100644
--- a/include/hw/net/ne2000-isa.h
+++ b/include/hw/net/ne2000-isa.h
@@ -22,8 +22,6 @@ static inline ISADevice *isa_ne2000_init(ISABus *bus, int
base, int irq,
{
ISADevice *d;
-qemu_check_nic_model(nd, "ne2k_isa");
-
d = isa_try_new(TYPE_ISA_NE2000);
if (d) {
DeviceState *dev = DEVICE(d);
--
2.41.0
[PATCH v3 25/46] hw/net/smc91c111: use qemu_configure_nic_device()
From: David Woodhouse
Some callers instantiate the device unconditionally, others will do so only
if there is a NICInfo to go with it. This appears to be fairly random, but
preserve the existing behaviour for now.
Signed-off-by: David Woodhouse
---
hw/arm/gumstix.c | 6 ++
hw/arm/integratorcp.c | 5 +++--
hw/arm/mainstone.c | 3 +--
hw/arm/realview.c | 25 ++---
hw/arm/versatilepb.c | 15 ---
hw/net/smc91c111.c | 5 ++---
include/hw/net/smc91c111.h | 2 +-
7 files changed, 23 insertions(+), 38 deletions(-)
diff --git a/hw/arm/gumstix.c b/hw/arm/gumstix.c
index 2ca4140c9f..f58c4da7f9 100644
--- a/hw/arm/gumstix.c
+++ b/hw/arm/gumstix.c
@@ -74,8 +74,7 @@ static void connex_init(MachineState *machine)
FLASH_SECTOR_SIZE, 2, 0, 0, 0, 0, 0);
/* Interrupt line of NIC is connected to GPIO line 36 */
-smc91c111_init(_table[0], 0x04000300,
-qdev_get_gpio_in(cpu->gpio, 36));
+smc91c111_init(0x04000300, qdev_get_gpio_in(cpu->gpio, 36));
}
static void verdex_init(MachineState *machine)
@@ -98,8 +97,7 @@ static void verdex_init(MachineState *machine)
FLASH_SECTOR_SIZE, 2, 0, 0, 0, 0, 0);
/* Interrupt line of NIC is connected to GPIO line 99 */
-smc91c111_init(_table[0], 0x04000300,
-qdev_get_gpio_in(cpu->gpio, 99));
+smc91c111_init(0x04000300, qdev_get_gpio_in(cpu->gpio, 99));
}
static void connex_class_init(ObjectClass *oc, void *data)
diff --git a/hw/arm/integratorcp.c b/hw/arm/integratorcp.c
index 1830e1d785..c56a2c1353 100644
--- a/hw/arm/integratorcp.c
+++ b/hw/arm/integratorcp.c
@@ -666,8 +666,9 @@ static void integratorcp_init(MachineState *machine)
sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0x1d00);
sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic[25]);
-if (nd_table[0].used)
-smc91c111_init(_table[0], 0xc800, pic[27]);
+if (qemu_find_nic_info("smc91c111", true, NULL)) {
+smc91c111_init(0xc800, pic[27]);
+}
sysbus_create_simple("pl110", 0xc000, pic[22]);
diff --git a/hw/arm/mainstone.c b/hw/arm/mainstone.c
index 68329c4617..84dbb6e525 100644
--- a/hw/arm/mainstone.c
+++ b/hw/arm/mainstone.c
@@ -153,8 +153,7 @@ static void mainstone_common_init(MachineState *machine,
qdev_get_gpio_in(mst_irq, S1_IRQ),
qdev_get_gpio_in(mst_irq, S1_CD_IRQ));
-smc91c111_init(_table[0], MST_ETH_PHYS,
-qdev_get_gpio_in(mst_irq, ETHERNET_IRQ));
+smc91c111_init(MST_ETH_PHYS, qdev_get_gpio_in(mst_irq, ETHERNET_IRQ));
mainstone_binfo.board_id = arm_id;
arm_load_kernel(mpu->cpu, machine, _binfo);
diff --git a/hw/arm/realview.c b/hw/arm/realview.c
index 132217b2ed..6e7529d98f 100644
--- a/hw/arm/realview.c
+++ b/hw/arm/realview.c
@@ -89,7 +89,6 @@ static void realview_init(MachineState *machine,
I2CBus *i2c;
int n;
unsigned int smp_cpus = machine->smp.cpus;
-int done_nic = 0;
qemu_irq cpu_irq[4];
int is_mpcore = 0;
int is_pb = 0;
@@ -295,24 +294,20 @@ static void realview_init(MachineState *machine,
n--;
}
}
-for(n = 0; n < nb_nics; n++) {
-nd = _table[n];
-
-if (!done_nic && (!nd->model ||
-strcmp(nd->model, is_pb ? "lan9118" : "smc91c111") == 0)) {
-if (is_pb) {
-lan9118_init(nd, 0x4e00, pic[28]);
-} else {
-smc91c111_init(nd, 0x4e00, pic[28]);
-}
-done_nic = 1;
+
+nd = qemu_find_nic_info(is_pb ? "lan9118" : "smc91c111", true, NULL);
+if (nd) {
+if (is_pb) {
+lan9118_init(nd, 0x4e00, pic[28]);
} else {
-if (pci_bus) {
-pci_nic_init_nofail(nd, pci_bus, "rtl8139", NULL);
-}
+smc91c111_init(0x4e00, pic[28]);
}
}
+if (pci_bus) {
+pci_init_nic_devices(pci_bus, "rtl8139");
+}
+
dev = sysbus_create_simple(TYPE_ARM_SBCON_I2C, 0x10002000, NULL);
i2c = (I2CBus *)qdev_get_child_bus(dev, "i2c");
i2c_slave_create_simple(i2c, "ds1338", 0x68);
diff --git a/hw/arm/versatilepb.c b/hw/arm/versatilepb.c
index 4b2257787b..0517a65601 100644
--- a/hw/arm/versatilepb.c
+++ b/hw/arm/versatilepb.c
@@ -192,10 +192,8 @@ static void versatile_init(MachineState *machine, int
board_id)
SysBusDevice *busdev;
DeviceState *pl041;
PCIBus *pci_bus;
-NICInfo *nd;
I2CBus *i2c;
int n;
-int done_smc = 0;
DriveInfo *dinfo;
if (machine->ram_size > 0x1000) {
@@ -263,16 +261,11 @@ static void versatile_init(MachineState *machine, int
board_id)
sysbus_connect_irq(busdev, 3, sic[30]);
pci_bus = (PCIBus *)qdev_get_child_bus(dev, "pci");
-for(n = 0; n < nb_nics; n++) {
-nd = _table[n];
-
-if (!done_smc && (!nd->model ||
[PATCH v3 23/46] hw/arm/exynos4: use qemu_create_nic_device()
From: David Woodhouse
Signed-off-by: David Woodhouse
---
hw/arm/exynos4_boards.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/hw/arm/exynos4_boards.c b/hw/arm/exynos4_boards.c
index b0e13eb4f0..003992189b 100644
--- a/hw/arm/exynos4_boards.c
+++ b/hw/arm/exynos4_boards.c
@@ -76,10 +76,8 @@ static void lan9215_init(uint32_t base, qemu_irq irq)
SysBusDevice *s;
/* This should be a 9215 but the 9118 is close enough */
-if (nd_table[0].used) {
-qemu_check_nic_model(_table[0], "lan9118");
-dev = qdev_new(TYPE_LAN9118);
-qdev_set_nic_properties(dev, _table[0]);
+dev = qemu_create_nic_device(TYPE_LAN9118, true, NULL);
+if (dev) {
qdev_prop_set_uint32(dev, "mode_16bit", 1);
s = SYS_BUS_DEVICE(dev);
sysbus_realize_and_unref(s, _fatal);
--
2.41.0
[PATCH v3 32/46] hw/m68k/mcf5208: use qemu_create_nic_device()
From: David Woodhouse
Signed-off-by: David Woodhouse
---
hw/m68k/mcf5208.c | 19 ++-
1 file changed, 6 insertions(+), 13 deletions(-)
diff --git a/hw/m68k/mcf5208.c b/hw/m68k/mcf5208.c
index d22d8536db..0cfb806c20 100644
--- a/hw/m68k/mcf5208.c
+++ b/hw/m68k/mcf5208.c
@@ -206,16 +206,16 @@ static void mcf5208_sys_init(MemoryRegion *address_space,
qemu_irq *pic)
}
}
-static void mcf_fec_init(MemoryRegion *sysmem, NICInfo *nd, hwaddr base,
- qemu_irq *irqs)
+static void mcf_fec_init(MemoryRegion *sysmem, hwaddr base, qemu_irq *irqs)
{
DeviceState *dev;
SysBusDevice *s;
int i;
-qemu_check_nic_model(nd, TYPE_MCF_FEC_NET);
-dev = qdev_new(TYPE_MCF_FEC_NET);
-qdev_set_nic_properties(dev, nd);
+dev = qemu_create_nic_device(TYPE_MCF_FEC_NET, true, NULL);
+if (!dev) {
+return;
+}
s = SYS_BUS_DEVICE(dev);
sysbus_realize_and_unref(s, _fatal);
@@ -267,14 +267,7 @@ static void mcf5208evb_init(MachineState *machine)
mcf5208_sys_init(address_space_mem, pic);
-if (nb_nics > 1) {
-error_report("Too many NICs");
-exit(1);
-}
-if (nd_table[0].used) {
-mcf_fec_init(address_space_mem, _table[0],
- 0xfc03, pic + 36);
-}
+mcf_fec_init(address_space_mem, 0xfc03, pic + 36);
g_free(pic);
--
2.41.0
[PATCH v3 15/46] hw/ppc/prep: use pci_init_nic_devices()
From: David Woodhouse
Previously, the first PCI NIC would be placed in PCI slot 3 and the rest
would be dynamically assigned. Even if the user overrode the default NIC
type and made it something other than PCNet.
Now, the first PCNet NIC (that is, anything not explicitly specified
to be anything different) will go to slot 3 even if it isn't the first
NIC specified on the commnd line. And anything else will be dynamically
assigned.
Signed-off-by: David Woodhouse
---
hw/ppc/prep.c | 8 +++-
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
index 137276bcb9..1a6cd05c61 100644
--- a/hw/ppc/prep.c
+++ b/hw/ppc/prep.c
@@ -241,7 +241,6 @@ static void ibm_40p_init(MachineState *machine)
ISADevice *isa_dev;
ISABus *isa_bus;
void *fw_cfg;
-int i;
uint32_t kernel_base = 0, initrd_base = 0;
long kernel_size = 0, initrd_size = 0;
char boot_device;
@@ -336,10 +335,9 @@ static void ibm_40p_init(MachineState *machine)
/* XXX: s3-trio at PCI_DEVFN(2, 0) */
pci_vga_init(pci_bus);
-for (i = 0; i < nb_nics; i++) {
-pci_nic_init_nofail(_table[i], pci_bus, mc->default_nic,
-i == 0 ? "3" : NULL);
-}
+/* First PCNET device at PCI_DEVFN(3, 0) */
+pci_init_nic_in_slot(pci_bus, mc->default_nic, NULL, "3");
+pci_init_nic_devices(pci_bus, mc->default_nic);
}
/* Prepare firmware configuration for OpenBIOS */
--
2.41.0
[PATCH v3 03/46] net: add qemu_create_nic_bus_devices()
From: David Woodhouse
This will instantiate any NICs which live on a given bus type. Each bus
is allowed *one* substitution (for PCI it's virtio → virtio-net-pci, for
Xen it's xen → xen-net-device; no point in overengineering it unless we
actually want more).
Signed-off-by: David Woodhouse
Reviewed-by: Paul Durrant
---
include/net/net.h | 3 +++
net/net.c | 53 +++
2 files changed, 56 insertions(+)
diff --git a/include/net/net.h b/include/net/net.h
index 25ea83fd12..31e63d1f0d 100644
--- a/include/net/net.h
+++ b/include/net/net.h
@@ -213,6 +213,9 @@ bool qemu_configure_nic_device(DeviceState *dev, bool
match_default,
const char *alias);
DeviceState *qemu_create_nic_device(const char *typename, bool match_default,
const char *alias);
+void qemu_create_nic_bus_devices(BusState *bus, const char *parent_type,
+ const char *default_model,
+ const char *alias, const char *alias_target);
void print_net_client(Monitor *mon, NetClientState *nc);
void net_socket_rs_init(SocketReadState *rs,
SocketReadStateFinalize *finalize,
diff --git a/net/net.c b/net/net.c
index 962904eaef..4651b3f443 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1223,6 +1223,59 @@ DeviceState *qemu_create_nic_device(const char
*typename, bool match_default,
return dev;
}
+void qemu_create_nic_bus_devices(BusState *bus, const char *parent_type,
+ const char *default_model,
+ const char *alias, const char *alias_target)
+{
+GPtrArray *nic_models = qemu_get_nic_models(parent_type);
+const char *model;
+DeviceState *dev;
+NICInfo *nd;
+int i;
+
+if (nic_model_help) {
+if (alias_target) {
+add_nic_model_help(alias_target, alias);
+}
+for (i = 0; i < nic_models->len - 1; i++) {
+add_nic_model_help(nic_models->pdata[i], NULL);
+}
+}
+
+/* Drop the NULL terminator which would make g_str_equal() unhappy */
+nic_models->len--;
+
+for (i = 0; i < nb_nics; i++) {
+nd = _table[i];
+
+if (!nd->used || nd->instantiated) {
+continue;
+}
+
+model = nd->model ? nd->model : default_model;
+if (!model) {
+continue;
+}
+
+/* Each bus type is allowed *one* substitution */
+if (g_str_equal(model, alias)) {
+model = alias_target;
+}
+
+if (!g_ptr_array_find_with_equal_func(nic_models, model,
+ g_str_equal, NULL)) {
+/* This NIC does not live on this bus. */
+continue;
+}
+
+dev = qdev_new(model);
+qdev_set_nic_properties(dev, nd);
+qdev_realize_and_unref(dev, bus, _fatal);
+}
+
+g_ptr_array_free(nic_models, true);
+}
+
static int (* const net_client_init_fun[NET_CLIENT_DRIVER__MAX])(
const Netdev *netdev,
const char *name,
--
2.41.0
[PATCH v3 02/46] net: report list of available models according to platform
From: David Woodhouse
By noting the models for which a configuration was requested, we can give
the user an accurate list of which NIC models were actually available on
the platform/configuration that was otherwise chosen.
Signed-off-by: David Woodhouse
Reviewed-by: Paul Durrant
---
net/net.c | 94 +++
1 file changed, 94 insertions(+)
diff --git a/net/net.c b/net/net.c
index aeb7f573fc..962904eaef 100644
--- a/net/net.c
+++ b/net/net.c
@@ -75,6 +75,8 @@ typedef QSIMPLEQ_HEAD(, NetdevQueueEntry) NetdevQueue;
static NetdevQueue nd_queue = QSIMPLEQ_HEAD_INITIALIZER(nd_queue);
+static GHashTable *nic_model_help;
+
/***/
/* network device redirectors */
@@ -1087,12 +1089,94 @@ static int net_init_nic(const Netdev *netdev, const
char *name,
return idx;
}
+static gboolean add_nic_result(gpointer key, gpointer value, gpointer
user_data)
+{
+GPtrArray *results = user_data;
+GPtrArray *alias_list = value;
+const char *model = key;
+char *result;
+
+if (!alias_list) {
+result = g_strdup(model);
+} else {
+GString *result_str = g_string_new(model);
+int i;
+
+g_string_append(result_str, " (aka ");
+for (i = 0; i < alias_list->len; i++) {
+if (i) {
+g_string_append(result_str, ", ");
+}
+g_string_append(result_str, alias_list->pdata[i]);
+}
+g_string_append(result_str, ")");
+result = result_str->str;
+g_string_free(result_str, false);
+g_ptr_array_unref(alias_list);
+}
+g_ptr_array_add(results, result);
+return true;
+}
+
+static int model_cmp(char **a, char **b)
+{
+return strcmp(*a, *b);
+}
+
+static void show_nic_models(void)
+{
+GPtrArray *results = g_ptr_array_new();
+int i;
+
+g_hash_table_foreach_remove(nic_model_help, add_nic_result, results);
+g_ptr_array_sort(results, (GCompareFunc)model_cmp);
+
+printf("Available NIC models for this configuration:\n");
+for (i = 0 ; i < results->len; i++) {
+printf("%s\n", (char *)results->pdata[i]);
+}
+g_hash_table_unref(nic_model_help);
+nic_model_help = NULL;
+}
+
+static void add_nic_model_help(const char *model, const char *alias)
+{
+GPtrArray *alias_list = NULL;
+
+if (g_hash_table_lookup_extended(nic_model_help, model, NULL,
+ (gpointer *)_list)) {
+/* Already exists, no alias to add: return */
+if (!alias) {
+return;
+}
+if (alias_list) {
+/* Check if this alias is already in the list. Add if not. */
+if (!g_ptr_array_find_with_equal_func(alias_list, alias,
+ g_str_equal, NULL)) {
+g_ptr_array_add(alias_list, g_strdup(alias));
+}
+return;
+}
+}
+/* Either this model wasn't in the list already, or a first alias added */
+if (alias) {
+alias_list = g_ptr_array_new();
+g_ptr_array_set_free_func(alias_list, g_free);
+g_ptr_array_add(alias_list, g_strdup(alias));
+}
+g_hash_table_replace(nic_model_help, g_strdup(model), alias_list);
+}
+
NICInfo *qemu_find_nic_info(const char *typename, bool match_default,
const char *alias)
{
NICInfo *nd;
int i;
+if (nic_model_help) {
+add_nic_model_help(typename, alias);
+}
+
for (i = 0; i < nb_nics; i++) {
nd = _table[i];
@@ -1606,6 +1690,10 @@ void net_check_clients(void)
NetClientState *nc;
int i;
+if (nic_model_help) {
+show_nic_models();
+exit(0);
+}
net_hub_check_clients();
QTAILQ_FOREACH(nc, _clients, next) {
@@ -1685,6 +1773,12 @@ static int net_param_nic(void *dummy, QemuOpts *opts,
Error **errp)
memset(ni, 0, sizeof(*ni));
ni->model = qemu_opt_get_del(opts, "model");
+if (!nic_model_help && !g_strcmp0(ni->model, "help")) {
+nic_model_help = g_hash_table_new_full(g_str_hash, g_str_equal,
+ g_free, NULL);
+return 0;
+}
+
/* Create an ID if the user did not specify one */
nd_id = g_strdup(qemu_opts_id(opts));
if (!nd_id) {
--
2.41.0
[PATCH v3 45/46] net: remove qemu_show_nic_models(), qemu_find_nic_model()
From: David Woodhouse
These old functions can be removed now too. Let net_param_nic() print
the full set of network devices directly, and also make it note that a
list more specific to this platform/config will be available by using
'-nic model=help' instead.
Signed-off-by: David Woodhouse
---
include/net/net.h | 3 ---
net/net.c | 39 ++-
2 files changed, 6 insertions(+), 36 deletions(-)
diff --git a/include/net/net.h b/include/net/net.h
index 1be8b40074..19fb82833c 100644
--- a/include/net/net.h
+++ b/include/net/net.h
@@ -203,9 +203,6 @@ void qemu_set_vnet_hdr_len(NetClientState *nc, int len);
int qemu_set_vnet_le(NetClientState *nc, bool is_le);
int qemu_set_vnet_be(NetClientState *nc, bool is_be);
void qemu_macaddr_default_if_unset(MACAddr *macaddr);
-int qemu_show_nic_models(const char *arg, const char *const *models);
-int qemu_find_nic_model(NICInfo *nd, const char * const *models,
-const char *default_model);
NICInfo *qemu_find_nic_info(const char *typename, bool match_default,
const char *alias);
bool qemu_configure_nic_device(DeviceState *dev, bool match_default,
diff --git a/net/net.c b/net/net.c
index ffd4b42d5a..09ab0889f5 100644
--- a/net/net.c
+++ b/net/net.c
@@ -977,38 +977,6 @@ GPtrArray *qemu_get_nic_models(const char *device_type)
return nic_models;
}
-int qemu_show_nic_models(const char *arg, const char *const *models)
-{
-int i;
-
-if (!arg || !is_help_option(arg)) {
-return 0;
-}
-
-printf("Available NIC models:\n");
-for (i = 0 ; models[i]; i++) {
-printf("%s\n", models[i]);
-}
-return 1;
-}
-
-int qemu_find_nic_model(NICInfo *nd, const char * const *models,
-const char *default_model)
-{
-int i;
-
-if (!nd->model)
-nd->model = g_strdup(default_model);
-
-for (i = 0 ; models[i]; i++) {
-if (strcmp(nd->model, models[i]) == 0)
-return i;
-}
-
-error_report("Unsupported NIC model: %s", nd->model);
-return -1;
-}
-
static int net_init_nic(const Netdev *netdev, const char *name,
NetClientState *peer, Error **errp)
{
@@ -1791,9 +1759,14 @@ static int net_param_nic(void *dummy, QemuOpts *opts,
Error **errp)
}
if (is_help_option(type)) {
GPtrArray *nic_models = qemu_get_nic_models(TYPE_DEVICE);
+int i;
show_netdevs();
printf("\n");
-qemu_show_nic_models(type, (const char **)nic_models->pdata);
+printf("Supported NIC models "
+ "(use -nic model=help for a filtered list):\n");
+for (i = 0 ; nic_models->pdata[i]; i++) {
+printf("%s\n", (char *)nic_models->pdata[i]);
+}
g_ptr_array_free(nic_models, true);
exit(0);
}
--
2.41.0
[PATCH v3 37/46] hw/net/lasi_i82596: use qemu_configure_nic_device()
From: David Woodhouse
Signed-off-by: David Woodhouse
---
hw/net/lasi_i82596.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/hw/net/lasi_i82596.c b/hw/net/lasi_i82596.c
index 6a3147fe2d..2bb4f2c4ca 100644
--- a/hw/net/lasi_i82596.c
+++ b/hw/net/lasi_i82596.c
@@ -125,11 +125,10 @@ SysBusI82596State *lasi_82596_init(MemoryRegion
*addr_space,
static const MACAddr HP_MAC = {
.a = { 0x08, 0x00, 0x09, 0xef, 0x34, 0xf6 } };
-qemu_check_nic_model(_table[0], TYPE_LASI_82596);
dev = qdev_new(TYPE_LASI_82596);
s = SYSBUS_I82596(dev);
s->state.irq = lan_irq;
-qdev_set_nic_properties(dev, _table[0]);
+qemu_configure_nic_device(dev, true, NULL);
sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), _fatal);
s->state.conf.macaddr = HP_MAC; /* set HP MAC prefix */
--
2.41.0
[PATCH v3 09/46] hw/arm/virt: use pci_init_nic_devices()
From: David Woodhouse
Signed-off-by: David Woodhouse
---
hw/arm/virt.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 2793121cb4..8cf9237001 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1454,9 +1454,7 @@ static void create_pcie(VirtMachineState *vms)
pci->bypass_iommu = vms->default_bus_bypass_iommu;
vms->bus = pci->bus;
if (vms->bus) {
-for (i = 0; i < nb_nics; i++) {
-pci_nic_init_nofail(_table[i], pci->bus, mc->default_nic, NULL);
-}
+pci_init_nic_devices(pci->bus, mc->default_nic);
}
nodename = vms->pciehb_nodename = g_strdup_printf("/pcie@%" PRIx64, base);
--
2.41.0
[PATCH v3 00/46] Rework matching of network devices to -nic options
Most platforms iterating directly over the nd_table[] are doing one of
two things. Either they are creating the NIC for their platform and want
to find a matching -nic configuration for it, if such exists. Or they
are only going to create that platform NIC if a matching config *does*
exist.
All of those can be converted to the new qemu_configure_nic_device()
and qemu_create_nic_device() functions. The latter of which will call
qdev_new() to create the device (and apply the config) if a matching
NIC config does exist for it. The existing behaviour of each platform
has been preserved for now, despite it being apparently fairly random.
PCI and indeed XenBus can use a qemu_create_nic_bus_devices() which will
create all NICs that live on a given bus type. That covers most
platforms, but some PCI platforms do something special with the first
NIC of a given type, placing it in the slot where it would have been on
the real hardware. There were various inconsistencies in the way the
platforms did so, and whether they cared what model the NIC was. Those
subtle behavioural changes I *have* allowed to change, and now the
pci_init_nic_slot() function will pick the first NIC that the user
specified which isn't explicitly *not* the default type, and put that
in the specified slot.
The tests for npcm7xx used to lament that they had to instantiate both
NICs even when they wanted to test only the second, because there was
no way to specify which hardware devices gets which configuration. I
made that untrue, by allowing 'emc0' and 'emc1' aliases, and fixed up
the test accordingly.
There are one or two special cases which want to do special things with
the MAC address of the primary NIC, to set up a system identification
(or force it to use an Apple OUI, in the case of m68k/q400). All those
work out relatively cleanly too.
And I can clean up the ugly patch which fixed up the Xen network device
handling, and replace it with a simple call to the new
qemu_create_nic_bus_devices() function.
I suspect that we can remove the pci_init_nic_devices() from platform
code and just do it later, except for platforms which *care* which
PCI bus the dynamic devices go on (is that just sun4u which puts its
primary NIC onto a different bus?).
Finally, while we're at it, clean up -nic model=help to only print
the device models which are actually usable on the given platform
rather than just listing them *all*.
And now we can make nd_table[] and nb_nics static in net/net.c because
nothing else has any business poking at them directly.
v3: Rebase to 8.2
v2: Some build fixes after better coverage testing, revert the Xen fix
in this series because I'm putting the less invasive hack into 8.2
(hopefully).
David Woodhouse (46):
net: add qemu_{configure,create}_nic_device(), qemu_find_nic_info()
net: report list of available models according to platform
net: add qemu_create_nic_bus_devices()
hw/pci: add pci_init_nic_devices(), pci_init_nic_in_slot()
hw/i386/pc: use qemu_get_nic_info() and pci_init_nic_devices()
hw/xen: use qemu_create_nic_bus_devices() to instantiate Xen NICs
hw/alpha/dp264: use pci_init_nic_devices()
hw/arm/sbsa-ref: use pci_init_nic_devices()
hw/arm/virt: use pci_init_nic_devices()
hw/hppa: use pci_init_nic_devices()
hw/loongarch: use pci_init_nic_devices()
hw/mips/fuloong2e: use pci_init_nic_devices()
hw/mips/malta: use pci_init_nic_devices()
hw/mips/loongson3_virt: use pci_init_nic_devices()
hw/ppc/prep: use pci_init_nic_devices()
hw/ppc/spapr: use qemu_get_nic_info() and pci_init_nic_devices()
hw/ppc: use pci_init_nic_devices()
hw/sh4/r2d: use pci_init_nic_devices()
hw/sparc64/sun4u: use pci_init_nic_devices()
hw/xtensa/virt: use pci_init_nic_devices()
hw/arm/allwinner: use qemu_configure_nic_device()
hw/arm/aspeed: use qemu_configure_nic_device()
hw/arm/exynos4: use qemu_create_nic_device()
hw/arm/fsl: use qemu_configure_nic_device()
hw/net/smc91c111: use qemu_configure_nic_device()
hw/net/lan9118: use qemu_configure_nic_device()
hw/arm/highbank: use qemu_create_nic_device()
hw/arm/npcm7xx: use qemu_configure_nic_device, allow emc0/emc1 as aliases
hw/arm/stellaris: use qemu_find_nic_info()
hw/arm: use qemu_configure_nic_device()
hw/net/etraxfs-eth: use qemu_configure_nic_device()
hw/m68k/mcf5208: use qemu_create_nic_device()
hw/m68k/q800: use qemu_find_nic_info()
hw/microblaze: use qemu_configure_nic_device()
hw/mips/mipssim: use qemu_create_nic_device()
hw/mips/jazz: use qemu_find_nic_info()
hw/net/lasi_i82596: use qemu_configure_nic_device()
hw/openrisc/openrisc_sim: use qemu_create_nic_device()
hw/riscv: use qemu_configure_nic_device()
hw/s390x/s390-virtio-ccw: use qemu_create_nic_device()
hw/sparc/sun4m: use qemu_find_nic_info()
[PATCH v3 26/46] hw/net/lan9118: use qemu_configure_nic_device()
From: David Woodhouse
Some callers instantiate the device unconditionally, others will do so only
if there is a NICInfo to go with it. This appears to be fairly random, but
preseve the existing behaviour for now.
Signed-off-by: David Woodhouse
---
hw/arm/kzm.c | 4 ++--
hw/arm/mps2.c| 2 +-
hw/arm/realview.c| 6 ++
hw/arm/vexpress.c| 4 ++--
hw/net/lan9118.c | 5 ++---
include/hw/net/lan9118.h | 2 +-
6 files changed, 10 insertions(+), 13 deletions(-)
diff --git a/hw/arm/kzm.c b/hw/arm/kzm.c
index 9be91ebeaa..2ccd6f8a76 100644
--- a/hw/arm/kzm.c
+++ b/hw/arm/kzm.c
@@ -113,8 +113,8 @@ static void kzm_init(MachineState *machine)
alias_offset += ram[i].size;
}
-if (nd_table[0].used) {
-lan9118_init(_table[0], KZM_LAN9118_ADDR,
+if (qemu_find_nic_info("lan9118", true, NULL)) {
+lan9118_init(KZM_LAN9118_ADDR,
qdev_get_gpio_in(DEVICE(>soc.avic), 52));
}
diff --git a/hw/arm/mps2.c b/hw/arm/mps2.c
index bd873cc5de..50919ee46d 100644
--- a/hw/arm/mps2.c
+++ b/hw/arm/mps2.c
@@ -456,7 +456,7 @@ static void mps2_common_init(MachineState *machine)
/* In hardware this is a LAN9220; the LAN9118 is software compatible
* except that it doesn't support the checksum-offload feature.
*/
-lan9118_init(_table[0], mmc->ethernet_base,
+lan9118_init(mmc->ethernet_base,
qdev_get_gpio_in(armv7m,
mmc->fpga_type == FPGA_AN511 ? 47 : 13));
diff --git a/hw/arm/realview.c b/hw/arm/realview.c
index 6e7529d98f..1f88da3702 100644
--- a/hw/arm/realview.c
+++ b/hw/arm/realview.c
@@ -84,7 +84,6 @@ static void realview_init(MachineState *machine,
SysBusDevice *busdev;
qemu_irq pic[64];
PCIBus *pci_bus = NULL;
-NICInfo *nd;
DriveInfo *dinfo;
I2CBus *i2c;
int n;
@@ -295,10 +294,9 @@ static void realview_init(MachineState *machine,
}
}
-nd = qemu_find_nic_info(is_pb ? "lan9118" : "smc91c111", true, NULL);
-if (nd) {
+if (qemu_find_nic_info(is_pb ? "lan9118" : "smc91c111", true, NULL)) {
if (is_pb) {
-lan9118_init(nd, 0x4e00, pic[28]);
+lan9118_init(0x4e00, pic[28]);
} else {
smc91c111_init(0x4e00, pic[28]);
}
diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
index fd981f4c33..1e02e0965f 100644
--- a/hw/arm/vexpress.c
+++ b/hw/arm/vexpress.c
@@ -679,8 +679,8 @@ static void vexpress_common_init(MachineState *machine)
memory_region_add_subregion(sysmem, map[VE_VIDEORAM], >vram);
/* 0x4e00 LAN9118 Ethernet */
-if (nd_table[0].used) {
-lan9118_init(_table[0], map[VE_ETHERNET], pic[15]);
+if (qemu_find_nic_info("lan9118", true, NULL)) {
+lan9118_init(map[VE_ETHERNET], pic[15]);
}
/* VE_USB: not modelled */
diff --git a/hw/net/lan9118.c b/hw/net/lan9118.c
index 598dd79e17..47ff25b441 100644
--- a/hw/net/lan9118.c
+++ b/hw/net/lan9118.c
@@ -1408,14 +1408,13 @@ static void lan9118_register_types(void)
/* Legacy helper function. Should go away when machine config files are
implemented. */
-void lan9118_init(NICInfo *nd, uint32_t base, qemu_irq irq)
+void lan9118_init(uint32_t base, qemu_irq irq)
{
DeviceState *dev;
SysBusDevice *s;
-qemu_check_nic_model(nd, "lan9118");
dev = qdev_new(TYPE_LAN9118);
-qdev_set_nic_properties(dev, nd);
+qemu_configure_nic_device(dev, true, NULL);
s = SYS_BUS_DEVICE(dev);
sysbus_realize_and_unref(s, _fatal);
sysbus_mmio_map(s, 0, base);
diff --git a/include/hw/net/lan9118.h b/include/hw/net/lan9118.h
index 3d0c67f339..4bf9da7a63 100644
--- a/include/hw/net/lan9118.h
+++ b/include/hw/net/lan9118.h
@@ -15,6 +15,6 @@
#define TYPE_LAN9118 "lan9118"
-void lan9118_init(NICInfo *, uint32_t, qemu_irq);
+void lan9118_init(uint32_t, qemu_irq);
#endif
--
2.41.0
[PATCH v3 40/46] hw/s390x/s390-virtio-ccw: use qemu_create_nic_device()
From: David Woodhouse
Signed-off-by: David Woodhouse
---
hw/s390x/s390-virtio-ccw.c | 11 ++-
1 file changed, 2 insertions(+), 9 deletions(-)
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 1169e20b94..202c378131 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -221,16 +221,9 @@ static void s390_init_ipl_dev(const char *kernel_filename,
static void s390_create_virtio_net(BusState *bus, const char *name)
{
-int i;
-
-for (i = 0; i < nb_nics; i++) {
-NICInfo *nd = _table[i];
-DeviceState *dev;
-
-qemu_check_nic_model(nd, "virtio");
+DeviceState *dev;
-dev = qdev_new(name);
-qdev_set_nic_properties(dev, nd);
+while ((dev = qemu_create_nic_device(name, true, "virtio"))) {
qdev_realize_and_unref(dev, bus, _fatal);
}
}
--
2.41.0
[PATCH v3 06/46] hw/xen: use qemu_create_nic_bus_devices() to instantiate Xen NICs
From: David Woodhouse
When instantiating XenBus itself, for each NIC which is configured with
either the model unspecified, or set to to "xen" or "xen-net-device",
create a corresponding xen-net-device for it.
Now we can revert the previous more hackish version which relied on the
platform code explicitly registering the NICs on its own XenBus, having
returned the BusState* from xen_bus_init() itself.
This also fixes the setup for Xen PV guests, which was previously broken
in various ways and never actually managed to peer with the netdev.
Signed-off-by: David Woodhouse
Reviewed-by: Paul Durrant
---
hw/i386/pc.c| 13 ++---
hw/i386/pc_piix.c | 2 +-
hw/i386/pc_q35.c| 2 +-
hw/xen/xen-bus.c| 6 --
hw/xen/xen_devconfig.c | 25 -
hw/xenpv/xen_machine_pv.c | 9 -
include/hw/i386/pc.h| 4 +---
include/hw/xen/xen-bus.h| 2 +-
include/hw/xen/xen-legacy-backend.h | 1 -
9 files changed, 10 insertions(+), 54 deletions(-)
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index d80c536d88..3d5e29ce22 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1268,7 +1268,7 @@ void pc_basic_device_init(struct PCMachineState *pcms,
if (pcms->bus) {
pci_create_simple(pcms->bus, -1, "xen-platform");
}
-pcms->xenbus = xen_bus_init();
+xen_bus_init();
xen_be_init();
}
#endif
@@ -1296,8 +1296,7 @@ void pc_basic_device_init(struct PCMachineState *pcms,
pcms->vmport != ON_OFF_AUTO_ON);
}
-void pc_nic_init(PCMachineClass *pcmc, ISABus *isa_bus, PCIBus *pci_bus,
- BusState *xen_bus)
+void pc_nic_init(PCMachineClass *pcmc, ISABus *isa_bus, PCIBus *pci_bus)
{
MachineClass *mc = MACHINE_CLASS(pcmc);
bool default_is_ne2k = g_str_equal(mc->default_nic, TYPE_ISA_NE2000);
@@ -1305,14 +1304,6 @@ void pc_nic_init(PCMachineClass *pcmc, ISABus *isa_bus,
PCIBus *pci_bus,
rom_set_order_override(FW_CFG_ORDER_OVERRIDE_NIC);
-if (xen_bus) {
-while (nc = qemu_find_nic_info("xen-net-device", true, NULL)) {
-DeviceState *dev = qdev_new("xen-net-device");
-qdev_set_nic_properties(dev, nd);
-qdev_realize_and_unref(dev, xen_bus, _fatal);
-}
-}
-
while ((nd = qemu_find_nic_info(TYPE_ISA_NE2000, default_is_ne2k, NULL))) {
pc_init_ne2k_isa(isa_bus, nd);
}
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 042c13cdbc..aaf2c2521f 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -342,7 +342,7 @@ static void pc_init1(MachineState *machine,
pc_basic_device_init(pcms, isa_bus, x86ms->gsi, rtc_state, true,
0x4);
-pc_nic_init(pcmc, isa_bus, pci_bus, pcms->xenbus);
+pc_nic_init(pcmc, isa_bus, pci_bus);
if (pcmc->pci_enabled) {
pc_cmos_init(pcms, idebus[0], idebus[1], rtc_state);
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index f43d5142b8..7ca3f465e0 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -340,7 +340,7 @@ static void pc_q35_init(MachineState *machine)
/* the rest devices to which pci devfn is automatically assigned */
pc_vga_init(isa_bus, host_bus);
-pc_nic_init(pcmc, isa_bus, host_bus, pcms->xenbus);
+pc_nic_init(pcmc, isa_bus, host_bus);
if (machine->nvdimms_state->is_enabled) {
nvdimm_init_acpi_state(machine->nvdimms_state, system_io,
diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
index 4973e7d9c9..fb82cc33e4 100644
--- a/hw/xen/xen-bus.c
+++ b/hw/xen/xen-bus.c
@@ -19,6 +19,7 @@
#include "qapi/error.h"
#include "qapi/qmp/qdict.h"
#include "sysemu/sysemu.h"
+#include "net/net.h"
#include "trace.h"
static char *xen_device_get_backend_path(XenDevice *xendev)
@@ -1133,7 +1134,7 @@ static void xen_register_types(void)
type_init(xen_register_types)
-BusState *xen_bus_init(void)
+void xen_bus_init(void)
{
DeviceState *dev = qdev_new(TYPE_XEN_BRIDGE);
BusState *bus = qbus_new(TYPE_XEN_BUS, dev, NULL);
@@ -1141,5 +1142,6 @@ BusState *xen_bus_init(void)
sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), _fatal);
qbus_set_bus_hotplug_handler(bus);
-return bus;
+qemu_create_nic_bus_devices(bus, TYPE_XEN_DEVICE, "xen-net-device",
+"xen", "xen-net-device");
}
diff --git a/hw/xen/xen_devconfig.c b/hw/xen/xen_devconfig.c
index 3f77c675c6..2150869f60 100644
--- a/hw/xen/xen_devconfig.c
+++ b/hw/xen/xen_devconfig.c
@@ -46,31 +46,6 @@ static int xen_config_dev_all(char *fe, char *be)
/* - */
-int xen_config_dev_nic(NICInfo *nic)
-{
-char fe[256], be[256];
-char mac[20];
-int vlan_id = -1;
-
-net_hub_id_for_client(nic->netdev, _id);
-snprintf(mac, sizeof(mac), "%02x:%02x:%02x:%02x:%02x:%02x",
- nic->macaddr.a[0],
[PATCH v3 22/46] hw/arm/aspeed: use qemu_configure_nic_device()
From: David Woodhouse
Signed-off-by: David Woodhouse
---
hw/arm/aspeed.c | 9 -
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index cc59176563..bed5e4f40b 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -356,7 +356,6 @@ static void aspeed_machine_init(MachineState *machine)
AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(machine);
AspeedSoCClass *sc;
int i;
-NICInfo *nd = _table[0];
bmc->soc = ASPEED_SOC(object_new(amc->soc_name));
object_property_add_child(OBJECT(machine), "soc", OBJECT(bmc->soc));
@@ -371,10 +370,10 @@ static void aspeed_machine_init(MachineState *machine)
_fatal);
for (i = 0; i < sc->macs_num; i++) {
-if ((amc->macs_mask & (1 << i)) && nd->used) {
-qemu_check_nic_model(nd, TYPE_FTGMAC100);
-qdev_set_nic_properties(DEVICE(>soc->ftgmac100[i]), nd);
-nd++;
+if ((amc->macs_mask & (1 << i)) &&
+!qemu_configure_nic_device(DEVICE(>soc->ftgmac100[i]),
+ true, NULL)) {
+break; /* No configs left; stop asking */
}
}
--
2.41.0
[PATCH v3 16/46] hw/ppc/spapr: use qemu_get_nic_info() and pci_init_nic_devices()
From: David Woodhouse
Avoid directly referencing nd_table[] by first instantiating any
spapr-vlan devices using a qemu_get_nic_info() loop, then calling
pci_init_nic_devices() to do the rest.
No functional change intended.
Signed-off-by: David Woodhouse
---
hw/ppc/spapr.c | 18 +-
1 file changed, 5 insertions(+), 13 deletions(-)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 4997aa4f1d..37604e7caf 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2796,6 +2796,7 @@ static void spapr_machine_init(MachineState *machine)
MemoryRegion *sysmem = get_system_memory();
long load_limit, fw_size;
Error *resize_hpt_err = NULL;
+NICInfo *nd;
if (!filename) {
error_report("Could not find LPAR firmware '%s'", bios_name);
@@ -2996,21 +2997,12 @@ static void spapr_machine_init(MachineState *machine)
phb = spapr_create_default_phb();
-for (i = 0; i < nb_nics; i++) {
-NICInfo *nd = _table[i];
-
-if (!nd->model) {
-nd->model = g_strdup("spapr-vlan");
-}
-
-if (g_str_equal(nd->model, "spapr-vlan") ||
-g_str_equal(nd->model, "ibmveth")) {
-spapr_vlan_create(spapr->vio_bus, nd);
-} else {
-pci_nic_init_nofail(_table[i], phb->bus, nd->model, NULL);
-}
+while ((nd = qemu_find_nic_info("spapr-vlan", true, "ibmveth"))) {
+spapr_vlan_create(spapr->vio_bus, nd);
}
+pci_init_nic_devices(phb->bus, NULL);
+
for (i = 0; i <= drive_get_max_bus(IF_SCSI); i++) {
spapr_vscsi_create(spapr->vio_bus);
}
--
2.41.0
[PATCH v3 38/46] hw/openrisc/openrisc_sim: use qemu_create_nic_device()
From: David Woodhouse
Signed-off-by: David Woodhouse
---
hw/openrisc/openrisc_sim.c | 18 +-
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/hw/openrisc/openrisc_sim.c b/hw/openrisc/openrisc_sim.c
index 35da123aef..bffd6f721f 100644
--- a/hw/openrisc/openrisc_sim.c
+++ b/hw/openrisc/openrisc_sim.c
@@ -170,7 +170,7 @@ static void openrisc_create_fdt(Or1ksimState *state,
static void openrisc_sim_net_init(Or1ksimState *state, hwaddr base, hwaddr
size,
int num_cpus, OpenRISCCPU *cpus[],
- int irq_pin, NICInfo *nd)
+ int irq_pin)
{
void *fdt = state->fdt;
DeviceState *dev;
@@ -178,8 +178,10 @@ static void openrisc_sim_net_init(Or1ksimState *state,
hwaddr base, hwaddr size,
char *nodename;
int i;
-dev = qdev_new("open_eth");
-qdev_set_nic_properties(dev, nd);
+dev = qemu_create_nic_device("open_eth", true, NULL);
+if (!dev) {
+return;
+}
s = SYS_BUS_DEVICE(dev);
sysbus_realize_and_unref(s, _fatal);
@@ -313,12 +315,10 @@ static void openrisc_sim_init(MachineState *machine)
openrisc_create_fdt(state, or1ksim_memmap, smp_cpus, machine->ram_size,
machine->kernel_cmdline);
-if (nd_table[0].used) {
-openrisc_sim_net_init(state, or1ksim_memmap[OR1KSIM_ETHOC].base,
- or1ksim_memmap[OR1KSIM_ETHOC].size,
- smp_cpus, cpus,
- OR1KSIM_ETHOC_IRQ, nd_table);
-}
+openrisc_sim_net_init(state, or1ksim_memmap[OR1KSIM_ETHOC].base,
+ or1ksim_memmap[OR1KSIM_ETHOC].size,
+ smp_cpus, cpus,
+ OR1KSIM_ETHOC_IRQ);
if (smp_cpus > 1) {
openrisc_sim_ompic_init(state, or1ksim_memmap[OR1KSIM_OMPIC].base,
--
2.41.0
[PATCH v3 42/46] hw/xtensa/xtfpga: use qemu_create_nic_device()
From: David Woodhouse
Signed-off-by: David Woodhouse
---
hw/xtensa/xtfpga.c | 13 ++---
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/hw/xtensa/xtfpga.c b/hw/xtensa/xtfpga.c
index fbad1c83a3..f49e6591dc 100644
--- a/hw/xtensa/xtfpga.c
+++ b/hw/xtensa/xtfpga.c
@@ -141,14 +141,16 @@ static void xtfpga_net_init(MemoryRegion *address_space,
hwaddr base,
hwaddr descriptors,
hwaddr buffers,
-qemu_irq irq, NICInfo *nd)
+qemu_irq irq)
{
DeviceState *dev;
SysBusDevice *s;
MemoryRegion *ram;
-dev = qdev_new("open_eth");
-qdev_set_nic_properties(dev, nd);
+dev = qemu_create_nic_device("open_eth", true, NULL);
+if (!dev) {
+return;
+}
s = SYS_BUS_DEVICE(dev);
sysbus_realize_and_unref(s, _fatal);
@@ -301,10 +303,7 @@ static void xtfpga_init(const XtfpgaBoardDesc *board,
MachineState *machine)
memory_region_add_subregion(system_memory, board->io[1], io);
}
xtfpga_fpga_init(system_io, 0x0d02, freq);
-if (nd_table[0].used) {
-xtfpga_net_init(system_io, 0x0d03, 0x0d030400, 0x0d80,
-extints[1], nd_table);
-}
+xtfpga_net_init(system_io, 0x0d03, 0x0d030400, 0x0d80, extints[1]);
serial_mm_init(system_io, 0x0d050020, 2, extints[0],
115200, serial_hd(0), DEVICE_NATIVE_ENDIAN);
--
2.41.0
[PATCH v3 21/46] hw/arm/allwinner: use qemu_configure_nic_device()
From: David Woodhouse
Signed-off-by: David Woodhouse
---
hw/arm/allwinner-a10.c | 6 +-
hw/arm/allwinner-h3.c | 6 +-
hw/arm/allwinner-r40.c | 27 ++-
3 files changed, 4 insertions(+), 35 deletions(-)
diff --git a/hw/arm/allwinner-a10.c b/hw/arm/allwinner-a10.c
index b0ea3f7f66..57f52871ec 100644
--- a/hw/arm/allwinner-a10.c
+++ b/hw/arm/allwinner-a10.c
@@ -142,11 +142,7 @@ static void aw_a10_realize(DeviceState *dev, Error **errp)
sysbus_realize(SYS_BUS_DEVICE(>dramc), _fatal);
sysbus_mmio_map(SYS_BUS_DEVICE(>dramc), 0, AW_A10_DRAMC_BASE);
-/* FIXME use qdev NIC properties instead of nd_table[] */
-if (nd_table[0].used) {
-qemu_check_nic_model(_table[0], TYPE_AW_EMAC);
-qdev_set_nic_properties(DEVICE(>emac), _table[0]);
-}
+qemu_configure_nic_device(DEVICE(>emac), true, NULL);
if (!sysbus_realize(SYS_BUS_DEVICE(>emac), errp)) {
return;
}
diff --git a/hw/arm/allwinner-h3.c b/hw/arm/allwinner-h3.c
index f05afddf7e..4f102ad082 100644
--- a/hw/arm/allwinner-h3.c
+++ b/hw/arm/allwinner-h3.c
@@ -369,11 +369,7 @@ static void allwinner_h3_realize(DeviceState *dev, Error
**errp)
"sd-bus");
/* EMAC */
-/* FIXME use qdev NIC properties instead of nd_table[] */
-if (nd_table[0].used) {
-qemu_check_nic_model(_table[0], TYPE_AW_SUN8I_EMAC);
-qdev_set_nic_properties(DEVICE(>emac), _table[0]);
-}
+qemu_configure_nic_device(DEVICE(>emac), true, NULL);
object_property_set_link(OBJECT(>emac), "dma-memory",
OBJECT(get_system_memory()), _fatal);
sysbus_realize(SYS_BUS_DEVICE(>emac), _fatal);
diff --git a/hw/arm/allwinner-r40.c b/hw/arm/allwinner-r40.c
index a0d367c60d..4d5661b014 100644
--- a/hw/arm/allwinner-r40.c
+++ b/hw/arm/allwinner-r40.c
@@ -294,7 +294,6 @@ static void allwinner_r40_init(Object *obj)
static void allwinner_r40_realize(DeviceState *dev, Error **errp)
{
-const char *r40_nic_models[] = { "gmac", "emac", NULL };
AwR40State *s = AW_R40(dev);
/* CPUs */
@@ -454,31 +453,8 @@ static void allwinner_r40_realize(DeviceState *dev, Error
**errp)
sysbus_mmio_map(SYS_BUS_DEVICE(>dramc), 2,
s->memmap[AW_R40_DEV_DRAMPHY]);
-/* nic support gmac and emac */
-for (int i = 0; i < ARRAY_SIZE(r40_nic_models) - 1; i++) {
-NICInfo *nic = _table[i];
-
-if (!nic->used) {
-continue;
-}
-if (qemu_show_nic_models(nic->model, r40_nic_models)) {
-exit(0);
-}
-
-switch (qemu_find_nic_model(nic, r40_nic_models, r40_nic_models[0])) {
-case 0: /* gmac */
-qdev_set_nic_properties(DEVICE(>gmac), nic);
-break;
-case 1: /* emac */
-qdev_set_nic_properties(DEVICE(>emac), nic);
-break;
-default:
-exit(1);
-break;
-}
-}
-
/* GMAC */
+qemu_configure_nic_device(DEVICE(>gmac), true, "gmac");
object_property_set_link(OBJECT(>gmac), "dma-memory",
OBJECT(get_system_memory()),
_fatal);
sysbus_realize(SYS_BUS_DEVICE(>gmac), _fatal);
@@ -487,6 +463,7 @@ static void allwinner_r40_realize(DeviceState *dev, Error
**errp)
qdev_get_gpio_in(DEVICE(>gic), AW_R40_GIC_SPI_GMAC));
/* EMAC */
+qemu_configure_nic_device(DEVICE(>emac), true, "emac");
sysbus_realize(SYS_BUS_DEVICE(>emac), _fatal);
sysbus_mmio_map(SYS_BUS_DEVICE(>emac), 0, s->memmap[AW_R40_DEV_EMAC]);
sysbus_connect_irq(SYS_BUS_DEVICE(>emac), 0,
--
2.41.0
[PATCH v3 07/46] hw/alpha/dp264: use pci_init_nic_devices()
From: David Woodhouse
Signed-off-by: David Woodhouse
---
hw/alpha/dp264.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
index 03495e1e60..52a1fa310b 100644
--- a/hw/alpha/dp264.c
+++ b/hw/alpha/dp264.c
@@ -124,9 +124,7 @@ static void clipper_init(MachineState *machine)
pci_vga_init(pci_bus);
/* Network setup. e1000 is good enough, failing Tulip support. */
-for (i = 0; i < nb_nics; i++) {
-pci_nic_init_nofail(_table[i], pci_bus, mc->default_nic, NULL);
-}
+pci_init_nic_devices(pci_bus, mc->default_nic);
/* Super I/O */
isa_create_simple(isa_bus, TYPE_SMC37C669_SUPERIO);
--
2.41.0
[PATCH v3 10/46] hw/hppa: use pci_init_nic_devices()
From: David Woodhouse
Signed-off-by: David Woodhouse
---
hw/hppa/machine.c | 7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
index c8da7c18d5..19d477105e 100644
--- a/hw/hppa/machine.c
+++ b/hw/hppa/machine.c
@@ -338,7 +338,6 @@ static void machine_HP_common_init_tail(MachineState
*machine, PCIBus *pci_bus,
uint64_t kernel_entry = 0, kernel_low, kernel_high;
MemoryRegion *addr_space = get_system_memory();
MemoryRegion *rom_region;
-long i;
unsigned int smp_cpus = machine->smp.cpus;
SysBusDevice *s;
@@ -362,10 +361,8 @@ static void machine_HP_common_init_tail(MachineState
*machine, PCIBus *pci_bus,
qdev_get_gpio_in(lasi_dev, LASI_IRQ_LAN_HPA));
}
-for (i = 0; i < nb_nics; i++) {
-if (!enable_lasi_lan()) {
-pci_nic_init_nofail(_table[i], pci_bus, mc->default_nic, NULL);
-}
+if (!enable_lasi_lan()) {
+pci_init_nic_devices(pci_bus, mc->default_nic);
}
/* BMC board: HP Powerbar SP2 Diva (with console only) */
--
2.41.0
[PATCH v3 24/46] hw/arm/fsl: use qemu_configure_nic_device()
From: David Woodhouse
Signed-off-by: David Woodhouse
---
hw/arm/fsl-imx25.c | 2 +-
hw/arm/fsl-imx6.c | 2 +-
hw/arm/fsl-imx6ul.c | 2 +-
hw/arm/fsl-imx7.c | 2 +-
4 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/hw/arm/fsl-imx25.c b/hw/arm/fsl-imx25.c
index 9d2fb75a68..a24fa7b443 100644
--- a/hw/arm/fsl-imx25.c
+++ b/hw/arm/fsl-imx25.c
@@ -170,7 +170,7 @@ static void fsl_imx25_realize(DeviceState *dev, Error
**errp)
object_property_set_uint(OBJECT(>fec), "phy-num", s->phy_num,
_abort);
-qdev_set_nic_properties(DEVICE(>fec), _table[0]);
+qemu_configure_nic_device(DEVICE(>fec), true, NULL);
if (!sysbus_realize(SYS_BUS_DEVICE(>fec), errp)) {
return;
diff --git a/hw/arm/fsl-imx6.c b/hw/arm/fsl-imx6.c
index b2153022c0..02f3024090 100644
--- a/hw/arm/fsl-imx6.c
+++ b/hw/arm/fsl-imx6.c
@@ -380,7 +380,7 @@ static void fsl_imx6_realize(DeviceState *dev, Error **errp)
object_property_set_uint(OBJECT(>eth), "phy-num", s->phy_num,
_abort);
-qdev_set_nic_properties(DEVICE(>eth), _table[0]);
+qemu_configure_nic_device(DEVICE(>eth), true, NULL);
if (!sysbus_realize(SYS_BUS_DEVICE(>eth), errp)) {
return;
}
diff --git a/hw/arm/fsl-imx6ul.c b/hw/arm/fsl-imx6ul.c
index e37b69a5e1..ca3dd439ec 100644
--- a/hw/arm/fsl-imx6ul.c
+++ b/hw/arm/fsl-imx6ul.c
@@ -442,7 +442,7 @@ static void fsl_imx6ul_realize(DeviceState *dev, Error
**errp)
s->phy_num[i], _abort);
object_property_set_uint(OBJECT(>eth[i]), "tx-ring-num",
FSL_IMX6UL_ETH_NUM_TX_RINGS, _abort);
-qdev_set_nic_properties(DEVICE(>eth[i]), _table[i]);
+qemu_configure_nic_device(DEVICE(>eth[i]), true, NULL);
sysbus_realize(SYS_BUS_DEVICE(>eth[i]), _abort);
sysbus_mmio_map(SYS_BUS_DEVICE(>eth[i]), 0,
diff --git a/hw/arm/fsl-imx7.c b/hw/arm/fsl-imx7.c
index 474cfdc87c..1acbe065db 100644
--- a/hw/arm/fsl-imx7.c
+++ b/hw/arm/fsl-imx7.c
@@ -446,7 +446,7 @@ static void fsl_imx7_realize(DeviceState *dev, Error **errp)
s->phy_num[i], _abort);
object_property_set_uint(OBJECT(>eth[i]), "tx-ring-num",
FSL_IMX7_ETH_NUM_TX_RINGS, _abort);
-qdev_set_nic_properties(DEVICE(>eth[i]), _table[i]);
+qemu_configure_nic_device(DEVICE(>eth[i]), true, NULL);
sysbus_realize(SYS_BUS_DEVICE(>eth[i]), _abort);
sysbus_mmio_map(SYS_BUS_DEVICE(>eth[i]), 0, FSL_IMX7_ENETn_ADDR[i]);
--
2.41.0
[PATCH v3 39/46] hw/riscv: use qemu_configure_nic_device()
From: David Woodhouse
Signed-off-by: David Woodhouse
---
hw/riscv/microchip_pfsoc.c | 14 ++
hw/riscv/sifive_u.c| 7 +--
2 files changed, 3 insertions(+), 18 deletions(-)
diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
index b775aa8946..7725dfbde5 100644
--- a/hw/riscv/microchip_pfsoc.c
+++ b/hw/riscv/microchip_pfsoc.c
@@ -202,7 +202,6 @@ static void microchip_pfsoc_soc_realize(DeviceState *dev,
Error **errp)
MemoryRegion *envm_data = g_new(MemoryRegion, 1);
MemoryRegion *qspi_xip_mem = g_new(MemoryRegion, 1);
char *plic_hart_config;
-NICInfo *nd;
int i;
sysbus_realize(SYS_BUS_DEVICE(>e_cpus), _abort);
@@ -411,17 +410,8 @@ static void microchip_pfsoc_soc_realize(DeviceState *dev,
Error **errp)
memmap[MICROCHIP_PFSOC_USB].size);
/* GEMs */
-
-nd = _table[0];
-if (nd->used) {
-qemu_check_nic_model(nd, TYPE_CADENCE_GEM);
-qdev_set_nic_properties(DEVICE(>gem0), nd);
-}
-nd = _table[1];
-if (nd->used) {
-qemu_check_nic_model(nd, TYPE_CADENCE_GEM);
-qdev_set_nic_properties(DEVICE(>gem1), nd);
-}
+qemu_configure_nic_device(DEVICE(>gem0), true, NULL);
+qemu_configure_nic_device(DEVICE(>gem1), true, NULL);
object_property_set_int(OBJECT(>gem0), "revision", GEM_REVISION, errp);
object_property_set_int(OBJECT(>gem0), "phy-addr", 8, errp);
diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index ec76dce6c9..5207ec1fa5 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -789,7 +789,6 @@ static void sifive_u_soc_realize(DeviceState *dev, Error
**errp)
MemoryRegion *l2lim_mem = g_new(MemoryRegion, 1);
char *plic_hart_config;
int i, j;
-NICInfo *nd = _table[0];
qdev_prop_set_uint32(DEVICE(>u_cpus), "num-harts", ms->smp.cpus - 1);
qdev_prop_set_uint32(DEVICE(>u_cpus), "hartid-base", 1);
@@ -893,11 +892,7 @@ static void sifive_u_soc_realize(DeviceState *dev, Error
**errp)
}
sysbus_mmio_map(SYS_BUS_DEVICE(>otp), 0, memmap[SIFIVE_U_DEV_OTP].base);
-/* FIXME use qdev NIC properties instead of nd_table[] */
-if (nd->used) {
-qemu_check_nic_model(nd, TYPE_CADENCE_GEM);
-qdev_set_nic_properties(DEVICE(>gem), nd);
-}
+qemu_configure_nic_device(DEVICE(>gem), true, NULL);
object_property_set_int(OBJECT(>gem), "revision", GEM_REVISION,
_abort);
if (!sysbus_realize(SYS_BUS_DEVICE(>gem), errp)) {
--
2.41.0
[PATCH v3 14/46] hw/mips/loongson3_virt: use pci_init_nic_devices()
From: David Woodhouse
Signed-off-by: David Woodhouse
---
hw/mips/loongson3_virt.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/hw/mips/loongson3_virt.c b/hw/mips/loongson3_virt.c
index 33eae01eca..caedde2df0 100644
--- a/hw/mips/loongson3_virt.c
+++ b/hw/mips/loongson3_virt.c
@@ -451,9 +451,7 @@ static inline void loongson3_virt_devices_init(MachineState
*machine,
usb_create_simple(usb_bus_find(-1), "usb-tablet");
}
-for (i = 0; i < nb_nics; i++) {
-pci_nic_init_nofail(_table[i], pci_bus, mc->default_nic, NULL);
-}
+pci_init_nic_devices(pci_bus, mc->default_nic);
}
static void mips_loongson3_virt_init(MachineState *machine)
--
2.41.0
[PATCH v3 34/46] hw/microblaze: use qemu_configure_nic_device()
From: David Woodhouse
Signed-off-by: David Woodhouse
---
hw/microblaze/petalogix_ml605_mmu.c | 3 +--
hw/microblaze/petalogix_s3adsp1800_mmu.c | 3 +--
2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/hw/microblaze/petalogix_ml605_mmu.c
b/hw/microblaze/petalogix_ml605_mmu.c
index fb7889cf67..0f5fabc32e 100644
--- a/hw/microblaze/petalogix_ml605_mmu.c
+++ b/hw/microblaze/petalogix_ml605_mmu.c
@@ -133,7 +133,6 @@ petalogix_ml605_init(MachineState *machine)
sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, irq[TIMER_IRQ]);
/* axi ethernet and dma initialization. */
-qemu_check_nic_model(_table[0], "xlnx.axi-ethernet");
eth0 = qdev_new("xlnx.axi-ethernet");
dma = qdev_new("xlnx.axi-dma");
@@ -145,7 +144,7 @@ petalogix_ml605_init(MachineState *machine)
"axistream-connected-target", NULL);
cs = object_property_get_link(OBJECT(dma),
"axistream-control-connected-target", NULL);
-qdev_set_nic_properties(eth0, _table[0]);
+qemu_configure_nic_device(eth0, true, NULL);
qdev_prop_set_uint32(eth0, "rxmem", 0x1000);
qdev_prop_set_uint32(eth0, "txmem", 0x1000);
object_property_set_link(OBJECT(eth0), "axistream-connected", ds,
diff --git a/hw/microblaze/petalogix_s3adsp1800_mmu.c
b/hw/microblaze/petalogix_s3adsp1800_mmu.c
index 505639c298..dad46bd7f9 100644
--- a/hw/microblaze/petalogix_s3adsp1800_mmu.c
+++ b/hw/microblaze/petalogix_s3adsp1800_mmu.c
@@ -114,9 +114,8 @@ petalogix_s3adsp1800_init(MachineState *machine)
sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, TIMER_BASEADDR);
sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, irq[TIMER_IRQ]);
-qemu_check_nic_model(_table[0], "xlnx.xps-ethernetlite");
dev = qdev_new("xlnx.xps-ethernetlite");
-qdev_set_nic_properties(dev, _table[0]);
+qemu_configure_nic_device(dev, true, NULL);
qdev_prop_set_uint32(dev, "tx-ping-pong", 0);
qdev_prop_set_uint32(dev, "rx-ping-pong", 0);
sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), _fatal);
--
2.41.0
[PATCH v3 27/46] hw/arm/highbank: use qemu_create_nic_device()
From: David Woodhouse
Signed-off-by: David Woodhouse
---
hw/arm/highbank.c | 12 +---
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
index c21e18d08f..6a0e20e58d 100644
--- a/hw/arm/highbank.c
+++ b/hw/arm/highbank.c
@@ -296,19 +296,17 @@ static void calxeda_init(MachineState *machine, enum
cxmachines machine_id)
sysbus_create_simple(TYPE_SYSBUS_AHCI, 0xffe08000, pic[83]);
-if (nd_table[0].used) {
-qemu_check_nic_model(_table[0], "xgmac");
-dev = qdev_new("xgmac");
-qdev_set_nic_properties(dev, _table[0]);
+dev = qemu_create_nic_device("xgmac", true, NULL);
+if (dev) {
sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), _fatal);
sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0xfff5);
sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic[77]);
sysbus_connect_irq(SYS_BUS_DEVICE(dev), 1, pic[78]);
sysbus_connect_irq(SYS_BUS_DEVICE(dev), 2, pic[79]);
+}
-qemu_check_nic_model(_table[1], "xgmac");
-dev = qdev_new("xgmac");
-qdev_set_nic_properties(dev, _table[1]);
+dev = qemu_create_nic_device("xgmac", true, NULL);
+if (dev) {
sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), _fatal);
sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0xfff51000);
sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic[80]);
--
2.41.0
[PATCH v3 43/46] net: remove qemu_check_nic_model()
From: David Woodhouse
Signed-off-by: David Woodhouse
---
include/net/net.h | 1 -
net/net.c | 13 -
2 files changed, 14 deletions(-)
diff --git a/include/net/net.h b/include/net/net.h
index 31e63d1f0d..1be8b40074 100644
--- a/include/net/net.h
+++ b/include/net/net.h
@@ -204,7 +204,6 @@ int qemu_set_vnet_le(NetClientState *nc, bool is_le);
int qemu_set_vnet_be(NetClientState *nc, bool is_be);
void qemu_macaddr_default_if_unset(MACAddr *macaddr);
int qemu_show_nic_models(const char *arg, const char *const *models);
-void qemu_check_nic_model(NICInfo *nd, const char *model);
int qemu_find_nic_model(NICInfo *nd, const char * const *models,
const char *default_model);
NICInfo *qemu_find_nic_info(const char *typename, bool match_default,
diff --git a/net/net.c b/net/net.c
index 4651b3f443..ffd4b42d5a 100644
--- a/net/net.c
+++ b/net/net.c
@@ -992,19 +992,6 @@ int qemu_show_nic_models(const char *arg, const char
*const *models)
return 1;
}
-void qemu_check_nic_model(NICInfo *nd, const char *model)
-{
-const char *models[2];
-
-models[0] = model;
-models[1] = NULL;
-
-if (qemu_show_nic_models(nd->model, models))
-exit(0);
-if (qemu_find_nic_model(nd, models, model) < 0)
-exit(1);
-}
-
int qemu_find_nic_model(NICInfo *nd, const char * const *models,
const char *default_model)
{
--
2.41.0
[PATCH v3 29/46] hw/arm/stellaris: use qemu_find_nic_info()
From: David Woodhouse
Rather than just using qemu_configure_nic_device(), populate the MAC
address in the system-registers device by peeking at the NICInfo before
it's assigned to the device.
Generate the MAC address early, if there is no matching -nic option.
Otherwise the MAC address wouldn't be generated until net_client_init1()
runs.
Signed-off-by: David Woodhouse
---
hw/arm/stellaris.c | 30 ++
1 file changed, 22 insertions(+), 8 deletions(-)
diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
index 729a8bf569..e8e1c83413 100644
--- a/hw/arm/stellaris.c
+++ b/hw/arm/stellaris.c
@@ -1027,7 +1027,8 @@ static void stellaris_init(MachineState *ms,
stellaris_board_info *board)
DeviceState *ssys_dev;
int i;
int j;
-const uint8_t *macaddr;
+NICInfo *nd;
+MACAddr mac;
MemoryRegion *sram = g_new(MemoryRegion, 1);
MemoryRegion *flash = g_new(MemoryRegion, 1);
@@ -1050,12 +1051,22 @@ static void stellaris_init(MachineState *ms,
stellaris_board_info *board)
* need its sysclk output.
*/
ssys_dev = qdev_new(TYPE_STELLARIS_SYS);
-/* Most devices come preprogrammed with a MAC address in the user data. */
-macaddr = nd_table[0].macaddr.a;
+
+/*
+ * Most devices come preprogrammed with a MAC address in the user data.
+ * Generate a MAC address now, if there isn't a matching -nic for it.
+ */
+nd = qemu_find_nic_info("stellaris_enet", true, "stellaris");
+if (nd) {
+memcpy(mac.a, nd->macaddr.a, sizeof(mac.a));
+} else {
+qemu_macaddr_default_if_unset();
+}
+
qdev_prop_set_uint32(ssys_dev, "user0",
- macaddr[0] | (macaddr[1] << 8) | (macaddr[2] << 16));
+ mac.a[0] | (mac.a[1] << 8) | (mac.a[2] << 16));
qdev_prop_set_uint32(ssys_dev, "user1",
- macaddr[3] | (macaddr[4] << 8) | (macaddr[5] << 16));
+ mac.a[3] | (mac.a[4] << 8) | (mac.a[5] << 16));
qdev_prop_set_uint32(ssys_dev, "did0", board->did0);
qdev_prop_set_uint32(ssys_dev, "did1", board->did1);
qdev_prop_set_uint32(ssys_dev, "dc0", board->dc0);
@@ -1267,10 +1278,13 @@ static void stellaris_init(MachineState *ms,
stellaris_board_info *board)
if (board->dc4 & (1 << 28)) {
DeviceState *enet;
-qemu_check_nic_model(_table[0], "stellaris");
-
enet = qdev_new("stellaris_enet");
-qdev_set_nic_properties(enet, _table[0]);
+if (nd) {
+qdev_set_nic_properties(enet, nd);
+} else {
+qdev_prop_set_macaddr(enet, "mac", mac.a);
+}
+
sysbus_realize_and_unref(SYS_BUS_DEVICE(enet), _fatal);
sysbus_mmio_map(SYS_BUS_DEVICE(enet), 0, 0x40048000);
sysbus_connect_irq(SYS_BUS_DEVICE(enet), 0, qdev_get_gpio_in(nvic,
42));
--
2.41.0
[PATCH v3 30/46] hw/arm: use qemu_configure_nic_device()
From: David Woodhouse
Signed-off-by: David Woodhouse
---
hw/arm/mps2-tz.c | 8 ++--
hw/arm/msf2-soc.c| 6 +-
hw/arm/musicpal.c| 3 +--
hw/arm/xilinx_zynq.c | 11 ---
hw/arm/xlnx-versal.c | 7 +--
hw/arm/xlnx-zynqmp.c | 8 +---
6 files changed, 10 insertions(+), 33 deletions(-)
diff --git a/hw/arm/mps2-tz.c b/hw/arm/mps2-tz.c
index 5d8cdc1a4c..a2d18afd79 100644
--- a/hw/arm/mps2-tz.c
+++ b/hw/arm/mps2-tz.c
@@ -503,14 +503,12 @@ static MemoryRegion *make_eth_dev(MPS2TZMachineState
*mms, void *opaque,
const PPCExtraData *extradata)
{
SysBusDevice *s;
-NICInfo *nd = _table[0];
/* In hardware this is a LAN9220; the LAN9118 is software compatible
* except that it doesn't support the checksum-offload feature.
*/
-qemu_check_nic_model(nd, "lan9118");
mms->lan9118 = qdev_new(TYPE_LAN9118);
-qdev_set_nic_properties(mms->lan9118, nd);
+qemu_configure_nic_device(mms->lan9118, true, NULL);
s = SYS_BUS_DEVICE(mms->lan9118);
sysbus_realize_and_unref(s, _fatal);
@@ -528,7 +526,6 @@ static MemoryRegion *make_eth_usb(MPS2TZMachineState *mms,
void *opaque,
* irqs[] is the ethernet IRQ.
*/
SysBusDevice *s;
-NICInfo *nd = _table[0];
memory_region_init(>eth_usb_container, OBJECT(mms),
"mps2-tz-eth-usb-container", 0x20);
@@ -537,9 +534,8 @@ static MemoryRegion *make_eth_usb(MPS2TZMachineState *mms,
void *opaque,
* In hardware this is a LAN9220; the LAN9118 is software compatible
* except that it doesn't support the checksum-offload feature.
*/
-qemu_check_nic_model(nd, "lan9118");
mms->lan9118 = qdev_new(TYPE_LAN9118);
-qdev_set_nic_properties(mms->lan9118, nd);
+qemu_configure_nic_device(mms->lan9118, true, NULL);
s = SYS_BUS_DEVICE(mms->lan9118);
sysbus_realize_and_unref(s, _fatal);
diff --git a/hw/arm/msf2-soc.c b/hw/arm/msf2-soc.c
index b5fe9f364d..35bf1d64e1 100644
--- a/hw/arm/msf2-soc.c
+++ b/hw/arm/msf2-soc.c
@@ -197,12 +197,8 @@ static void m2sxxx_soc_realize(DeviceState *dev_soc, Error
**errp)
g_free(bus_name);
}
-/* FIXME use qdev NIC properties instead of nd_table[] */
-if (nd_table[0].used) {
-qemu_check_nic_model(_table[0], TYPE_MSS_EMAC);
-qdev_set_nic_properties(DEVICE(>emac), _table[0]);
-}
dev = DEVICE(>emac);
+qemu_configure_nic_device(dev, true, NULL);
object_property_set_link(OBJECT(>emac), "ahb-bus",
OBJECT(get_system_memory()), _abort);
if (!sysbus_realize(SYS_BUS_DEVICE(>emac), errp)) {
diff --git a/hw/arm/musicpal.c b/hw/arm/musicpal.c
index 3200c9f68a..8781e99d27 100644
--- a/hw/arm/musicpal.c
+++ b/hw/arm/musicpal.c
@@ -1286,9 +1286,8 @@ static void musicpal_init(MachineState *machine)
}
sysbus_create_simple(TYPE_MV88W8618_FLASHCFG, MP_FLASHCFG_BASE, NULL);
-qemu_check_nic_model(_table[0], "mv88w8618");
dev = qdev_new(TYPE_MV88W8618_ETH);
-qdev_set_nic_properties(dev, _table[0]);
+qemu_configure_nic_device(dev, true, "mv88w8618");
object_property_set_link(OBJECT(dev), "dma-memory",
OBJECT(get_system_memory()), _fatal);
sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), _fatal);
diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index dbb9793aa1..73a6472b91 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -108,16 +108,13 @@ static void zynq_write_board_setup(ARMCPU *cpu,
static struct arm_boot_info zynq_binfo = {};
-static void gem_init(NICInfo *nd, uint32_t base, qemu_irq irq)
+static void gem_init(uint32_t base, qemu_irq irq)
{
DeviceState *dev;
SysBusDevice *s;
dev = qdev_new(TYPE_CADENCE_GEM);
-if (nd->used) {
-qemu_check_nic_model(nd, TYPE_CADENCE_GEM);
-qdev_set_nic_properties(dev, nd);
-}
+qemu_configure_nic_device(dev, true, NULL);
object_property_set_int(OBJECT(dev), "phy-addr", 7, _abort);
s = SYS_BUS_DEVICE(dev);
sysbus_realize_and_unref(s, _fatal);
@@ -279,8 +276,8 @@ static void zynq_init(MachineState *machine)
sysbus_create_varargs("cadence_ttc", 0xF8002000,
pic[69-IRQ_OFFSET], pic[70-IRQ_OFFSET], pic[71-IRQ_OFFSET], NULL);
-gem_init(_table[0], 0xE000B000, pic[54-IRQ_OFFSET]);
-gem_init(_table[1], 0xE000C000, pic[77-IRQ_OFFSET]);
+gem_init(0xE000B000, pic[54 - IRQ_OFFSET]);
+gem_init(0xE000C000, pic[77 - IRQ_OFFSET]);
for (n = 0; n < 2; n++) {
int hci_irq = n ? 79 : 56;
diff --git a/hw/arm/xlnx-versal.c b/hw/arm/xlnx-versal.c
index 9600551c44..01965ddf99 100644
--- a/hw/arm/xlnx-versal.c
+++ b/hw/arm/xlnx-versal.c
@@ -254,18 +254,13 @@ static void versal_create_gems(Versal *s, qemu_irq *pic)
static const int irqs[] = { VERSAL_GEM0_IRQ_0, VERSAL_GEM1_IRQ_0};
static const uint64_t addrs[] = { MM_GEM0, MM_GEM1 };
char *name =
[PATCH v3 35/46] hw/mips/mipssim: use qemu_create_nic_device()
From: David Woodhouse
The MIPS SIM platform instantiates its NIC only if a corresponding
configuration exists for it. Use qemu_create_nic_device() function for
that.
Signed-off-by: David Woodhouse
---
hw/mips/mipssim.c | 13 +++--
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/hw/mips/mipssim.c b/hw/mips/mipssim.c
index 01e323904d..16af31648e 100644
--- a/hw/mips/mipssim.c
+++ b/hw/mips/mipssim.c
@@ -118,13 +118,15 @@ static void main_cpu_reset(void *opaque)
}
}
-static void mipsnet_init(int base, qemu_irq irq, NICInfo *nd)
+static void mipsnet_init(int base, qemu_irq irq)
{
DeviceState *dev;
SysBusDevice *s;
-dev = qdev_new("mipsnet");
-qdev_set_nic_properties(dev, nd);
+dev = qemu_create_nic_device("mipsnet", true, NULL);
+if (!dev) {
+return;
+}
s = SYS_BUS_DEVICE(dev);
sysbus_realize_and_unref(s, _fatal);
@@ -225,9 +227,8 @@ mips_mipssim_init(MachineState *machine)
sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0));
}
-if (nd_table[0].used)
-/* MIPSnet uses the MIPS CPU INT0, which is interrupt 2. */
-mipsnet_init(0x4200, env->irq[2], _table[0]);
+/* MIPSnet uses the MIPS CPU INT0, which is interrupt 2. */
+mipsnet_init(0x4200, env->irq[2]);
}
static void mips_mipssim_machine_init(MachineClass *mc)
--
2.41.0
[PATCH v3 11/46] hw/loongarch: use pci_init_nic_devices()
From: David Woodhouse
Signed-off-by: David Woodhouse
---
hw/loongarch/virt.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
index 4b7dc67a2d..c48804ac38 100644
--- a/hw/loongarch/virt.c
+++ b/hw/loongarch/virt.c
@@ -504,9 +504,7 @@ static void loongarch_devices_init(DeviceState *pch_pic,
LoongArchMachineState *
fdt_add_uart_node(lams);
/* Network init */
-for (i = 0; i < nb_nics; i++) {
-pci_nic_init_nofail(_table[i], pci_bus, mc->default_nic, NULL);
-}
+pci_init_nic_devices(pci_bus, mc->default_nic);
/*
* There are some invalid guest memory access.
--
2.41.0
[PATCH v3 13/46] hw/mips/malta: use pci_init_nic_devices()
From: David Woodhouse
The Malta board setup code would previously place the first NIC into PCI
slot 11 if was a PCNet card, and the rest (including the first if it was
anything other than a PCNet card) would be dynamically assigned.
Now it will place any PCNet NIC into slot 11, and then anything else will
be dynamically assigned.
Signed-off-by: David Woodhouse
---
hw/mips/malta.c | 15 +++
1 file changed, 3 insertions(+), 12 deletions(-)
diff --git a/hw/mips/malta.c b/hw/mips/malta.c
index d22bb1edef..af74008c82 100644
--- a/hw/mips/malta.c
+++ b/hw/mips/malta.c
@@ -612,18 +612,9 @@ static MaltaFPGAState *malta_fpga_init(MemoryRegion
*address_space,
/* Network support */
static void network_init(PCIBus *pci_bus)
{
-int i;
-
-for (i = 0; i < nb_nics; i++) {
-NICInfo *nd = _table[i];
-const char *default_devaddr = NULL;
-
-if (i == 0 && (!nd->model || strcmp(nd->model, "pcnet") == 0))
-/* The malta board has a PCNet card using PCI SLOT 11 */
-default_devaddr = "0b";
-
-pci_nic_init_nofail(nd, pci_bus, "pcnet", default_devaddr);
-}
+/* The malta board has a PCNet card using PCI SLOT 11 */
+pci_init_nic_in_slot(pci_bus, "pcnet", NULL, "0b");
+pci_init_nic_devices(pci_bus, "pcnet");
}
static void bl_setup_gt64120_jump_kernel(void **p, uint64_t run_addr,
--
2.41.0
[PATCH v3 20/46] hw/xtensa/virt: use pci_init_nic_devices()
From: David Woodhouse
Signed-off-by: David Woodhouse
---
hw/xtensa/virt.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/hw/xtensa/virt.c b/hw/xtensa/virt.c
index a6cf646e99..5310a88861 100644
--- a/hw/xtensa/virt.c
+++ b/hw/xtensa/virt.c
@@ -102,9 +102,7 @@ static void create_pcie(MachineState *ms, CPUXtensaState
*env, int irq_base,
pci = PCI_HOST_BRIDGE(dev);
if (pci->bus) {
-for (i = 0; i < nb_nics; i++) {
-pci_nic_init_nofail(_table[i], pci->bus, mc->default_nic, NULL);
-}
+pci_init_nic_devices(pci->bus, mc->default_nic);
}
}
--
2.41.0
[PATCH v3 18/46] hw/sh4/r2d: use pci_init_nic_devices()
From: David Woodhouse Previously, the first PCI NIC would be assigned to slot 2 even if the user override the model and made it something other than an rtl8139 which is the default. Everything else would be dynamically assigned. Now, the first rtl8139 gets slot 2 and everything else is dynamic. Signed-off-by: David Woodhouse Reviewed-by: Yoshinori Sato --- hw/sh4/r2d.c | 6 ++ 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/hw/sh4/r2d.c b/hw/sh4/r2d.c index 4944994e9c..e9f316a6ce 100644 --- a/hw/sh4/r2d.c +++ b/hw/sh4/r2d.c @@ -240,7 +240,6 @@ static void r2d_init(MachineState *machine) MemoryRegion *sdram = g_new(MemoryRegion, 1); qemu_irq *irq; DriveInfo *dinfo; -int i; DeviceState *dev; SysBusDevice *busdev; MemoryRegion *address_space_mem = get_system_memory(); @@ -309,9 +308,8 @@ static void r2d_init(MachineState *machine) 0x555, 0x2aa, 0); /* NIC: rtl8139 on-board, and 2 slots. */ -for (i = 0; i < nb_nics; i++) -pci_nic_init_nofail(_table[i], pci_bus, -mc->default_nic, i == 0 ? "2" : NULL); +pci_init_nic_in_slot(pci_bus, mc->default_nic, NULL, "2"); +pci_init_nic_devices(pci_bus, mc->default_nic); /* USB keyboard */ usb_create_simple(usb_bus_find(-1), "usb-kbd"); -- 2.41.0
[linux-5.4 test] 184275: regressions - FAIL
flight 184275 linux-5.4 real [real] flight 184280 linux-5.4 real-retest [real] http://logs.test-lab.xenproject.org/osstest/logs/184275/ http://logs.test-lab.xenproject.org/osstest/logs/184280/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-armhf-armhf-xl-multivcpu 18 guest-start/debian.repeat fail REGR. vs. 184192 test-arm64-arm64-libvirt-raw 17 guest-start/debian.repeat fail REGR. vs. 184192 Tests which are failing intermittently (not blocking): test-armhf-armhf-xl-multivcpu 14 guest-start fail in 184280 pass in 184275 test-amd64-amd64-xl-qemut-debianhvm-i386-xsm 12 debian-hvm-install fail pass in 184280-retest Regressions which are regarded as allowable (not blocking): test-armhf-armhf-xl-rtds 14 guest-start fail REGR. vs. 184192 Tests which did not succeed, but are not blocking: test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail blocked in 184192 test-armhf-armhf-xl-credit1 14 guest-start fail like 184192 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 184192 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 184192 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 184192 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 184192 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 184192 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 184192 test-armhf-armhf-xl-credit2 18 guest-start/debian.repeatfail like 184192 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 184192 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 184192 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 184192 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 184192 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 184192 test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-xl 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-i386-libvirt 15 migrate-support-checkfail never pass test-amd64-i386-xl-pvshim14 guest-start fail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-support-checkfail never pass test-amd64-i386-libvirt-raw 14 migrate-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 15 migrate-support-checkfail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail never pass test-arm64-arm64-xl-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-raw 14 migrate-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 14 migrate-support-checkfail never pass
Re: [PATCH v2] NUMA: no need for asm/numa.h when !NUMA
Hi Jan, On 1/8/24 5:30 AM, Jan Beulich wrote: > There's no point in every architecture carrying the same stubs for the > case when NUMA isn't enabled (or even supported). Move all of that to > xen/numa.h; replace explicit uses of asm/numa.h in common code. Make > inclusion of asm/numa.h dependent upon NUMA=y. > > Drop the no longer applicable "implement NUMA support" comments - in a > !NUMA section this simply makes no sense. > > Signed-off-by: Jan Beulich Acked-by: Shawn Anastasio Thanks, Shawn
Re: Clang-format configuration discussion - pt 2
> I found the way we dealt with MISRA rules quite helpful. We had a weekly meeting to discuss some of the rules and then the outcome was posted on the ML. Maybe we should do the same here? Any other suggestion how to move? >>> >>> I have mixed feelings with meetings like the Misra ones. That's probably >>> unavoidable because of it being a goal to move fast. I'm not sure the >>> same applies here. >> >> I think in this situation is less about moving fast but more about making a >> closure of the 3 years+ discussion about the coding style. > > Exactly. The meeting is useful to find alignment in a more fruitful way. > > We don't have many MISRA rules left to discuss anyway. We could discuss > the codestyle changes after MISRA or in parallel. Hi Stefano, this is a very good idea, discussing that in a meeting would ease a lot the explanation for the clang-format configurable. Cheers, Luca
Re: [PATCH v5 08/13] xen/page_alloc: introduce preserved page flags macro
On 02.01.2024 10:51, Carlo Nonato wrote: > PGC_static and PGC_extra are flags that needs to be preserved when assigning > a page. Define a new macro that groups those flags and use it instead of > or'ing every time. > > The new macro is used also in free_heap_pages() allowing future commits to > extended it with other flags that must stop merging, as it now works for > PGC_static. PGC_extra is no harm here since it's only ever being set on > allocated pages. Is it? I can't see where free_domheap_pages() would clear it before calling free_heap_pages(). Or wait, that may happen in mark_page_free(), but then PGC_static would be cleared there, too. I must be missing something. > --- a/xen/common/page_alloc.c > +++ b/xen/common/page_alloc.c > @@ -158,6 +158,8 @@ > #define PGC_static 0 > #endif > > +#define preserved_flags (PGC_extra | PGC_static) I think this wants to (a) have a PGC_ prefix and (b) as a #define be all capitals. > @@ -1504,7 +1506,7 @@ static void free_heap_pages( > /* Merge with predecessor block? */ > if ( !mfn_valid(page_to_mfn(predecessor)) || > !page_state_is(predecessor, free) || > - (predecessor->count_info & PGC_static) || > + (predecessor->count_info & preserved_flags) || > (PFN_ORDER(predecessor) != order) || > (page_to_nid(predecessor) != node) ) > break; > @@ -1528,7 +1530,7 @@ static void free_heap_pages( > /* Merge with successor block? */ > if ( !mfn_valid(page_to_mfn(successor)) || > !page_state_is(successor, free) || > - (successor->count_info & PGC_static) || > + (successor->count_info & preserved_flags) || > (PFN_ORDER(successor) != order) || > (page_to_nid(successor) != node) ) > break; Irrespective of the comment at the top, this looks like an abuse of the new constant: There's nothing inherently making preserved flags also suppress merging (assuming it was properly checked that both sided have the same flags set/clear). Jan
Re: [PATCH v5 01/13] xen/common: add cache coloring common code
On 02.01.2024 10:51, Carlo Nonato wrote: > This commit adds the Last Level Cache (LLC) coloring common header, Kconfig > options and functions. Since this is an arch specific feature, actual > implementation is postponed to later patches and Kconfig options are placed > under xen/arch. As a general remark / nit: "This commit", "this patch", or alike aren't well suited for descriptions. > --- a/xen/arch/Kconfig > +++ b/xen/arch/Kconfig > @@ -31,3 +31,19 @@ config NR_NUMA_NODES > associated with multiple-nodes management. It is the upper bound of > the number of NUMA nodes that the scheduler, memory allocation and > other NUMA-aware components can handle. > + > +config LLC_COLORING > + bool "Last Level Cache (LLC) coloring" if EXPERT > + depends on HAS_LLC_COLORING > + > +config NR_LLC_COLORS > + int "Maximum number of LLC colors" > + default 128 What if I set to value to 0? Or to an unreasonably large one? You don't bound the value range at all. > --- a/xen/common/domain.c > +++ b/xen/common/domain.c > @@ -7,6 +7,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -1144,6 +1145,9 @@ static void cf_check complete_domain_destroy(struct > rcu_head *head) > struct vcpu *v; > int i; > > +if ( is_domain_llc_colored(d) ) > +domain_llc_coloring_free(d); Would be nice if the freeing function could be called unconditionally, being a no-op for non-colored domains. Further - is it really necessary to do this freeing this late? > --- a/xen/common/keyhandler.c > +++ b/xen/common/keyhandler.c > @@ -6,6 +6,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -307,6 +308,9 @@ static void cf_check dump_domains(unsigned char key) > > arch_dump_domain_info(d); > > +if ( is_domain_llc_colored(d) ) > +domain_dump_llc_colors(d); I'm less concerned of the conditional here, but along the lines of the comment above, it could of course again be the function that simply is a no-op for non-colored domains. > --- /dev/null > +++ b/xen/include/xen/llc-coloring.h > @@ -0,0 +1,46 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Last Level Cache (LLC) coloring common header > + * > + * Copyright (C) 2022 Xilinx Inc. > + * > + * Authors: > + *Carlo Nonato > + */ > +#ifndef __COLORING_H__ > +#define __COLORING_H__ > + > +#include > +#include > + > +#ifdef CONFIG_HAS_LLC_COLORING Why does this matter here? IOW why ... > +#include > + > +#ifdef CONFIG_LLC_COLORING ... is it not just this which is checked? > +extern bool llc_coloring_enabled; > +#define llc_coloring_enabled (llc_coloring_enabled) > +#endif > + > +#endif > + > +#ifndef llc_coloring_enabled > +#define llc_coloring_enabled (false) > +#endif +1 to the question Julien has raised here. > --- a/xen/include/xen/sched.h > +++ b/xen/include/xen/sched.h > @@ -626,6 +626,11 @@ struct domain > > /* Holding CDF_* constant. Internal flags for domain creation. */ > unsigned int cdf; > + > +#ifdef CONFIG_LLC_COLORING > +unsigned int *llc_colors; Can the color values change over the lifetime of a domain? If not, it may be prudent to have this be pointer-to-const. Jan > +unsigned int num_llc_colors; > +#endif > }; > > static inline struct page_list_head *page_to_list(
Re: [PATCH v5 04/13] xen: extend domctl interface for cache coloring
Hi Jan, On Mon, Jan 8, 2024 at 5:40 PM Jan Beulich wrote: > > On 08.01.2024 17:31, Carlo Nonato wrote: > > On Mon, Jan 8, 2024 at 4:32 PM Julien Grall wrote: > >> On 08/01/2024 15:18, Carlo Nonato wrote: > No. I am saying that that we should not be able to allow changing the > colors after the memory has been allocated. To give an example, your > current code would allow: > > 1) Setup the P2M pools or allocate RAM > 2) Set the color > > This would render the coloring configuration moot. > > Whether we want to allow changing the coloring before hand is a > different question and as I wrote earlier on, I don't mind if you want > to forbid that. > >>> > >>> At the moment I'm relying on the toolstack in the sense that I know that > >>> it > >>> will set colors right after domain creation and before memory allocation. > >>> Calling alloc_domheap_pages() without a coloring configuration makes Xen > >>> crash, so it's mandatory to have the configuration done before any > >>> allocation. > >>> I know that we shouldn't rely on the toolstack this much, but I didn't > >>> find a better way. Given this assumption, looking for an already existing > >>> color configuration of a domain is sufficient to avoid what you are > >>> saying. > >>> > >>> Is it possible to enforce such a constraint with domctl? > I mean to be > >>> sure that this domctl will be called at a precise time. > >> > >> Yes. You can... > >> > >>> > >>> Thanks. > >>> > > I don't know what to check that. > > You can check the size of the P2M pool (d->arch.paging.p2m_total_pages) > is still 0. I think for RAM, you can check d->tot_pages == 0. > >> > >> ... reject the call if either of the two fields are not zero. > > > > What I'm saying is that Xen would crash before even reaching this point if > > no > > colors were provided. Let's say that the toolstack or whatever hypercall > > user > > isn't calling this domctl at all (or not at the right time), then the domain > > colored allocator would always return null pages since there are no colors. > > We would have a crash and your if (or mine) would be useless. > > Why is it that you can't simply allocated arbitrary memory if coloring > information wasn't set up front? Aiui that'll be required anyway, as > there shouldn't be a restriction that all domains have to use coloring. If coloring is enabled all domains are colored. It's one of our first assumptions. We haven't developed something that works hybridly and supporting that would require some rework. > Jan
Re: [PATCH v5 04/13] xen: extend domctl interface for cache coloring
Hi Carlo, On 08/01/2024 16:31, Carlo Nonato wrote: On Mon, Jan 8, 2024 at 4:32 PM Julien Grall wrote: Hi, On 08/01/2024 15:18, Carlo Nonato wrote: No. I am saying that that we should not be able to allow changing the colors after the memory has been allocated. To give an example, your current code would allow: 1) Setup the P2M pools or allocate RAM 2) Set the color This would render the coloring configuration moot. Whether we want to allow changing the coloring before hand is a different question and as I wrote earlier on, I don't mind if you want to forbid that. At the moment I'm relying on the toolstack in the sense that I know that it will set colors right after domain creation and before memory allocation. Calling alloc_domheap_pages() without a coloring configuration makes Xen crash, so it's mandatory to have the configuration done before any allocation. I know that we shouldn't rely on the toolstack this much, but I didn't find a better way. Given this assumption, looking for an already existing color configuration of a domain is sufficient to avoid what you are saying. Is it possible to enforce such a constraint with domctl? > I mean to be sure that this domctl will be called at a precise time. Yes. You can... Thanks. I don't know what to check that. You can check the size of the P2M pool (d->arch.paging.p2m_total_pages) is still 0. I think for RAM, you can check d->tot_pages == 0. ... reject the call if either of the two fields are not zero. What I'm saying is that Xen would crash before even reaching this point if no colors were provided. Let's say that the toolstack or whatever hypercall user isn't calling this domctl at all (or not at the right time), then the domain colored allocator would always return null pages since there are no colors. We would have a crash and your if (or mine) would be useless. Really? I can believe that NULL may be returned but a crash... If that's the case, then this should be fixed. Let's say that now the domctl is called at the right time (no p2m, no tot_pages, no colors) then we can set the colors and everything works. From this point other calls to this domctl would be skipped because of your if which is equivalent to mine (checking for colors existence). So I misunderstood the implementation of is_domain_llc_colored(). I would have thought it was based on whether the domain has colors. But instead, it is based on whether coloring is enabled globally. So I agree that the checks are not necessary today. But if is_domain_llc_colored() is changed, then this may not be correct anymore. So I think there are some clarifications required. If the 'd' will never matter, then probably is_domain_llc_colored() should be removed. If not, then we want to add the check in the domctl (or at minimum document it). Also bringing in checks on p2m would require arch specific code which I was trying to avoid. Fair enough. But the first step is to make the sure the code is correct and Xen doesn't crash. We can then discuss about avoiding arch specific code. BTW, all your LLC code is implemented in arch/arm. So if it is really intended to contain zero arch specific code, then shouldn't it be implemented in common/? Cheers, -- Julien Grall
[xtf test] 184279: all pass - PUSHED
flight 184279 xtf real [real] http://logs.test-lab.xenproject.org/osstest/logs/184279/ Perfect :-) All tests in this flight passed as required version targeted for testing: xtf 2eed9f51c67a9e5d29ffd4ffeee50710489aad23 baseline version: xtf a5bd8d9e5d5c7b729d6d6122900d28f7a00aa6c0 Last test of basis 184277 2024-01-08 11:42:46 Z0 days Testing same since 184279 2024-01-08 15:13:02 Z0 days1 attempts People who touched revisions under test: Andrew Cooper jobs: build-amd64-xtf pass build-amd64 pass build-amd64-pvopspass test-xtf-amd64-amd64-1 pass test-xtf-amd64-amd64-2 pass test-xtf-amd64-amd64-3 pass test-xtf-amd64-amd64-4 pass test-xtf-amd64-amd64-5 pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Pushing revision : To xenbits.xen.org:/home/xen/git/xtf.git a5bd8d9..2eed9f5 2eed9f51c67a9e5d29ffd4ffeee50710489aad23 -> xen-tested-master
Re: [PATCH v5 04/13] xen: extend domctl interface for cache coloring
On 08.01.2024 17:31, Carlo Nonato wrote: > On Mon, Jan 8, 2024 at 4:32 PM Julien Grall wrote: >> On 08/01/2024 15:18, Carlo Nonato wrote: No. I am saying that that we should not be able to allow changing the colors after the memory has been allocated. To give an example, your current code would allow: 1) Setup the P2M pools or allocate RAM 2) Set the color This would render the coloring configuration moot. Whether we want to allow changing the coloring before hand is a different question and as I wrote earlier on, I don't mind if you want to forbid that. >>> >>> At the moment I'm relying on the toolstack in the sense that I know that it >>> will set colors right after domain creation and before memory allocation. >>> Calling alloc_domheap_pages() without a coloring configuration makes Xen >>> crash, so it's mandatory to have the configuration done before any >>> allocation. >>> I know that we shouldn't rely on the toolstack this much, but I didn't >>> find a better way. Given this assumption, looking for an already existing >>> color configuration of a domain is sufficient to avoid what you are saying. >>> >>> Is it possible to enforce such a constraint with domctl? > I mean to be >>> sure that this domctl will be called at a precise time. >> >> Yes. You can... >> >>> >>> Thanks. >>> > I don't know what to check that. You can check the size of the P2M pool (d->arch.paging.p2m_total_pages) is still 0. I think for RAM, you can check d->tot_pages == 0. >> >> ... reject the call if either of the two fields are not zero. > > What I'm saying is that Xen would crash before even reaching this point if no > colors were provided. Let's say that the toolstack or whatever hypercall user > isn't calling this domctl at all (or not at the right time), then the domain > colored allocator would always return null pages since there are no colors. > We would have a crash and your if (or mine) would be useless. Why is it that you can't simply allocated arbitrary memory if coloring information wasn't set up front? Aiui that'll be required anyway, as there shouldn't be a restriction that all domains have to use coloring. Jan
[PULL 5/6] Replace "iothread lock" with "BQL" in comments
The term "iothread lock" is obsolete. The APIs use Big QEMU Lock (BQL)
in their names. Update the code comments to use "BQL" instead of
"iothread lock".
Signed-off-by: Stefan Hajnoczi
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Paul Durrant
Reviewed-by: Akihiko Odaki
Reviewed-by: Cédric Le Goater
Reviewed-by: Harsh Prateek Bora
Message-id: 20240102153529.486531-5-stefa...@redhat.com
Signed-off-by: Stefan Hajnoczi
---
docs/devel/reset.rst | 2 +-
hw/display/qxl.h | 2 +-
include/exec/cpu-common.h| 2 +-
include/exec/memory.h| 4 ++--
include/exec/ramblock.h | 2 +-
include/migration/register.h | 8
target/arm/internals.h | 4 ++--
accel/tcg/cputlb.c | 4 ++--
accel/tcg/tcg-accel-ops-icount.c | 2 +-
hw/remote/mpqemu-link.c | 2 +-
migration/block-dirty-bitmap.c | 10 +-
migration/block.c| 22 +++---
migration/colo.c | 2 +-
migration/migration.c| 2 +-
migration/ram.c | 4 ++--
system/physmem.c | 6 +++---
target/arm/helper.c | 2 +-
ui/spice-core.c | 2 +-
util/rcu.c | 2 +-
audio/coreaudio.m| 4 ++--
ui/cocoa.m | 6 +++---
21 files changed, 47 insertions(+), 47 deletions(-)
diff --git a/docs/devel/reset.rst b/docs/devel/reset.rst
index 38ed1790f7..d4e79718ba 100644
--- a/docs/devel/reset.rst
+++ b/docs/devel/reset.rst
@@ -19,7 +19,7 @@ Triggering reset
This section documents the APIs which "users" of a resettable object should use
to control it. All resettable control functions must be called while holding
-the iothread lock.
+the BQL.
You can apply a reset to an object using ``resettable_assert_reset()``. You
need
to call ``resettable_release_reset()`` to release the object from reset. To
diff --git a/hw/display/qxl.h b/hw/display/qxl.h
index fdac14edad..e0a85a5ca4 100644
--- a/hw/display/qxl.h
+++ b/hw/display/qxl.h
@@ -159,7 +159,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(PCIQXLDevice, PCI_QXL)
*
* Use with care; by the time this function returns, the returned pointer is
* not protected by RCU anymore. If the caller is not within an RCU critical
- * section and does not hold the iothread lock, it must have other means of
+ * section and does not hold the BQL, it must have other means of
* protecting the pointer, such as a reference to the region that includes
* the incoming ram_addr_t.
*
diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
index 41115d8919..fef3138d29 100644
--- a/include/exec/cpu-common.h
+++ b/include/exec/cpu-common.h
@@ -92,7 +92,7 @@ RAMBlock *qemu_ram_block_by_name(const char *name);
*
* By the time this function returns, the returned pointer is not protected
* by RCU anymore. If the caller is not within an RCU critical section and
- * does not hold the iothread lock, it must have other means of protecting the
+ * does not hold the BQL, it must have other means of protecting the
* pointer, such as a reference to the memory region that owns the RAMBlock.
*/
RAMBlock *qemu_ram_block_from_host(void *ptr, bool round_offset,
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 48c11ca743..177be23db7 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1982,7 +1982,7 @@ int memory_region_get_fd(MemoryRegion *mr);
*
* Use with care; by the time this function returns, the returned pointer is
* not protected by RCU anymore. If the caller is not within an RCU critical
- * section and does not hold the iothread lock, it must have other means of
+ * section and does not hold the BQL, it must have other means of
* protecting the pointer, such as a reference to the region that includes
* the incoming ram_addr_t.
*
@@ -1999,7 +1999,7 @@ MemoryRegion *memory_region_from_host(void *ptr,
ram_addr_t *offset);
*
* Use with care; by the time this function returns, the returned pointer is
* not protected by RCU anymore. If the caller is not within an RCU critical
- * section and does not hold the iothread lock, it must have other means of
+ * section and does not hold the BQL, it must have other means of
* protecting the pointer, such as a reference to the region that includes
* the incoming ram_addr_t.
*
diff --git a/include/exec/ramblock.h b/include/exec/ramblock.h
index 69c6a53902..3eb79723c6 100644
--- a/include/exec/ramblock.h
+++ b/include/exec/ramblock.h
@@ -34,7 +34,7 @@ struct RAMBlock {
ram_addr_t max_length;
void (*resized)(const char*, uint64_t length, void *host);
uint32_t flags;
-/* Protected by iothread lock. */
+/* Protected by the BQL. */
char idstr[256];
/* RCU-enabled, writes protected by the ramlist lock */
QLIST_ENTRY(RAMBlock) next;
diff --git a/include/migration/register.h b/include/migration/register.h
index
[PULL 6/6] Rename "QEMU global mutex" to "BQL" in comments and docs
The term "QEMU global mutex" is identical to the more widely used Big
QEMU Lock ("BQL"). Update the code comments and documentation to use
"BQL" instead of "QEMU global mutex".
Signed-off-by: Stefan Hajnoczi
Acked-by: Markus Armbruster
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Paul Durrant
Reviewed-by: Akihiko Odaki
Reviewed-by: Cédric Le Goater
Reviewed-by: Harsh Prateek Bora
Message-id: 20240102153529.486531-6-stefa...@redhat.com
Signed-off-by: Stefan Hajnoczi
---
docs/devel/multi-thread-tcg.rst | 7 +++
docs/devel/qapi-code-gen.rst | 2 +-
docs/devel/replay.rst | 2 +-
docs/devel/multiple-iothreads.txt | 14 +++---
include/block/blockjob.h | 6 +++---
include/io/task.h | 2 +-
include/qemu/coroutine-core.h | 2 +-
include/qemu/coroutine.h | 2 +-
hw/block/dataplane/virtio-blk.c | 8
hw/block/virtio-blk.c | 2 +-
hw/scsi/virtio-scsi-dataplane.c | 6 +++---
net/tap.c | 2 +-
12 files changed, 27 insertions(+), 28 deletions(-)
diff --git a/docs/devel/multi-thread-tcg.rst b/docs/devel/multi-thread-tcg.rst
index c9541a7b20..7302c3bf53 100644
--- a/docs/devel/multi-thread-tcg.rst
+++ b/docs/devel/multi-thread-tcg.rst
@@ -226,10 +226,9 @@ instruction. This could be a future optimisation.
Emulated hardware state
---
-Currently thanks to KVM work any access to IO memory is automatically
-protected by the global iothread mutex, also known as the BQL (Big
-QEMU Lock). Any IO region that doesn't use global mutex is expected to
-do its own locking.
+Currently thanks to KVM work any access to IO memory is automatically protected
+by the BQL (Big QEMU Lock). Any IO region that doesn't use the BQL is expected
+to do its own locking.
However IO memory isn't the only way emulated hardware state can be
modified. Some architectures have model specific registers that
diff --git a/docs/devel/qapi-code-gen.rst b/docs/devel/qapi-code-gen.rst
index 7f78183cd4..ea8228518c 100644
--- a/docs/devel/qapi-code-gen.rst
+++ b/docs/devel/qapi-code-gen.rst
@@ -594,7 +594,7 @@ blocking the guest and other background operations.
Coroutine safety can be hard to prove, similar to thread safety. Common
pitfalls are:
-- The global mutex isn't held across ``qemu_coroutine_yield()``, so
+- The BQL isn't held across ``qemu_coroutine_yield()``, so
operations that used to assume that they execute atomically may have
to be more careful to protect against changes in the global state.
diff --git a/docs/devel/replay.rst b/docs/devel/replay.rst
index 0244be8b9c..effd856f0c 100644
--- a/docs/devel/replay.rst
+++ b/docs/devel/replay.rst
@@ -184,7 +184,7 @@ modes.
Reading and writing requests are created by CPU thread of QEMU. Later these
requests proceed to block layer which creates "bottom halves". Bottom
halves consist of callback and its parameters. They are processed when
-main loop locks the global mutex. These locks are not synchronized with
+main loop locks the BQL. These locks are not synchronized with
replaying process because main loop also processes the events that do not
affect the virtual machine state (like user interaction with monitor).
diff --git a/docs/devel/multiple-iothreads.txt
b/docs/devel/multiple-iothreads.txt
index 4865196bde..de85767b12 100644
--- a/docs/devel/multiple-iothreads.txt
+++ b/docs/devel/multiple-iothreads.txt
@@ -5,7 +5,7 @@ the COPYING file in the top-level directory.
This document explains the IOThread feature and how to write code that runs
-outside the QEMU global mutex.
+outside the BQL.
The main loop and IOThreads
---
@@ -29,13 +29,13 @@ scalability bottleneck on hosts with many CPUs. Work can
be spread across
several IOThreads instead of just one main loop. When set up correctly this
can improve I/O latency and reduce jitter seen by the guest.
-The main loop is also deeply associated with the QEMU global mutex, which is a
-scalability bottleneck in itself. vCPU threads and the main loop use the QEMU
-global mutex to serialize execution of QEMU code. This mutex is necessary
-because a lot of QEMU's code historically was not thread-safe.
+The main loop is also deeply associated with the BQL, which is a
+scalability bottleneck in itself. vCPU threads and the main loop use the BQL
+to serialize execution of QEMU code. This mutex is necessary because a lot of
+QEMU's code historically was not thread-safe.
The fact that all I/O processing is done in a single main loop and that the
-QEMU global mutex is contended by all vCPU threads and the main loop explain
+BQL is contended by all vCPU threads and the main loop explain
why it is desirable to place work into IOThreads.
The experimental virtio-blk data-plane implementation has been benchmarked and
@@ -66,7 +66,7 @@ There are several old APIs that use the main loop AioContext:
Since they implicitly work on the main loop
[PULL 3/6] qemu/main-loop: rename QEMU_IOTHREAD_LOCK_GUARD to BQL_LOCK_GUARD
The name "iothread" is overloaded. Use the term Big QEMU Lock (BQL)
instead, it is already widely used and unambiguous.
Signed-off-by: Stefan Hajnoczi
Reviewed-by: Paul Durrant
Acked-by: David Woodhouse
Reviewed-by: Cédric Le Goater
Acked-by: Ilya Leoshkevich
Reviewed-by: Harsh Prateek Bora
Reviewed-by: Akihiko Odaki
Message-id: 20240102153529.486531-3-stefa...@redhat.com
Signed-off-by: Stefan Hajnoczi
---
include/qemu/main-loop.h | 19 +--
hw/i386/kvm/xen_evtchn.c | 14 +++---
hw/i386/kvm/xen_gnttab.c | 2 +-
hw/mips/mips_int.c| 2 +-
hw/ppc/ppc.c | 2 +-
target/i386/kvm/xen-emu.c | 2 +-
target/ppc/excp_helper.c | 2 +-
target/ppc/helper_regs.c | 2 +-
target/riscv/cpu_helper.c | 4 ++--
9 files changed, 24 insertions(+), 25 deletions(-)
diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
index 72ebc0cb3a..c26ad2a029 100644
--- a/include/qemu/main-loop.h
+++ b/include/qemu/main-loop.h
@@ -343,33 +343,32 @@ void bql_lock_impl(const char *file, int line);
void bql_unlock(void);
/**
- * QEMU_IOTHREAD_LOCK_GUARD
+ * BQL_LOCK_GUARD
*
* Wrap a block of code in a conditional bql_{lock,unlock}.
*/
-typedef struct IOThreadLockAuto IOThreadLockAuto;
+typedef struct BQLLockAuto BQLLockAuto;
-static inline IOThreadLockAuto *qemu_iothread_auto_lock(const char *file,
-int line)
+static inline BQLLockAuto *bql_auto_lock(const char *file, int line)
{
if (bql_locked()) {
return NULL;
}
bql_lock_impl(file, line);
/* Anything non-NULL causes the cleanup function to be called */
-return (IOThreadLockAuto *)(uintptr_t)1;
+return (BQLLockAuto *)(uintptr_t)1;
}
-static inline void qemu_iothread_auto_unlock(IOThreadLockAuto *l)
+static inline void bql_auto_unlock(BQLLockAuto *l)
{
bql_unlock();
}
-G_DEFINE_AUTOPTR_CLEANUP_FUNC(IOThreadLockAuto, qemu_iothread_auto_unlock)
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(BQLLockAuto, bql_auto_unlock)
-#define QEMU_IOTHREAD_LOCK_GUARD() \
-g_autoptr(IOThreadLockAuto) _iothread_lock_auto __attribute__((unused)) \
-= qemu_iothread_auto_lock(__FILE__, __LINE__)
+#define BQL_LOCK_GUARD() \
+g_autoptr(BQLLockAuto) _bql_lock_auto __attribute__((unused)) \
+= bql_auto_lock(__FILE__, __LINE__)
/*
* qemu_cond_wait_iothread: Wait on condition for the main loop mutex
diff --git a/hw/i386/kvm/xen_evtchn.c b/hw/i386/kvm/xen_evtchn.c
index 4a835a1010..0171ef6d59 100644
--- a/hw/i386/kvm/xen_evtchn.c
+++ b/hw/i386/kvm/xen_evtchn.c
@@ -1127,7 +1127,7 @@ int xen_evtchn_reset_op(struct evtchn_reset *reset)
return -ESRCH;
}
-QEMU_IOTHREAD_LOCK_GUARD();
+BQL_LOCK_GUARD();
return xen_evtchn_soft_reset();
}
@@ -1145,7 +1145,7 @@ int xen_evtchn_close_op(struct evtchn_close *close)
return -EINVAL;
}
-QEMU_IOTHREAD_LOCK_GUARD();
+BQL_LOCK_GUARD();
qemu_mutex_lock(>port_lock);
ret = close_port(s, close->port, _kvm_routes);
@@ -1272,7 +1272,7 @@ int xen_evtchn_bind_pirq_op(struct evtchn_bind_pirq *pirq)
return -EINVAL;
}
-QEMU_IOTHREAD_LOCK_GUARD();
+BQL_LOCK_GUARD();
if (s->pirq[pirq->pirq].port) {
return -EBUSY;
@@ -1824,7 +1824,7 @@ int xen_physdev_map_pirq(struct physdev_map_pirq *map)
return -ENOTSUP;
}
-QEMU_IOTHREAD_LOCK_GUARD();
+BQL_LOCK_GUARD();
QEMU_LOCK_GUARD(>port_lock);
if (map->domid != DOMID_SELF && map->domid != xen_domid) {
@@ -1884,7 +1884,7 @@ int xen_physdev_unmap_pirq(struct physdev_unmap_pirq
*unmap)
return -EINVAL;
}
-QEMU_IOTHREAD_LOCK_GUARD();
+BQL_LOCK_GUARD();
qemu_mutex_lock(>port_lock);
if (!pirq_inuse(s, pirq)) {
@@ -1924,7 +1924,7 @@ int xen_physdev_eoi_pirq(struct physdev_eoi *eoi)
return -ENOTSUP;
}
-QEMU_IOTHREAD_LOCK_GUARD();
+BQL_LOCK_GUARD();
QEMU_LOCK_GUARD(>port_lock);
if (!pirq_inuse(s, pirq)) {
@@ -1956,7 +1956,7 @@ int xen_physdev_query_pirq(struct
physdev_irq_status_query *query)
return -ENOTSUP;
}
-QEMU_IOTHREAD_LOCK_GUARD();
+BQL_LOCK_GUARD();
QEMU_LOCK_GUARD(>port_lock);
if (!pirq_inuse(s, pirq)) {
diff --git a/hw/i386/kvm/xen_gnttab.c b/hw/i386/kvm/xen_gnttab.c
index a0cc30f619..245e4b15db 100644
--- a/hw/i386/kvm/xen_gnttab.c
+++ b/hw/i386/kvm/xen_gnttab.c
@@ -176,7 +176,7 @@ int xen_gnttab_map_page(uint64_t idx, uint64_t gfn)
return -EINVAL;
}
-QEMU_IOTHREAD_LOCK_GUARD();
+BQL_LOCK_GUARD();
QEMU_LOCK_GUARD(>gnt_lock);
xen_overlay_do_map_page(>gnt_aliases[idx], gpa);
diff --git a/hw/mips/mips_int.c b/hw/mips/mips_int.c
index 6c32e466a3..eef2fd2cd1 100644
--- a/hw/mips/mips_int.c
+++ b/hw/mips/mips_int.c
@@ -36,7 +36,7 @@ static void cpu_mips_irq_request(void *opaque, int irq, int
level)
return;
}
-QEMU_IOTHREAD_LOCK_GUARD();
+
[PULL 4/6] qemu/main-loop: rename qemu_cond_wait_iothread() to qemu_cond_wait_bql()
The name "iothread" is overloaded. Use the term Big QEMU Lock (BQL)
instead, it is already widely used and unambiguous.
Signed-off-by: Stefan Hajnoczi
Reviewed-by: Cédric Le Goater
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Paul Durrant
Reviewed-by: Harsh Prateek Bora
Reviewed-by: Akihiko Odaki
Message-id: 20240102153529.486531-4-stefa...@redhat.com
Signed-off-by: Stefan Hajnoczi
---
include/qemu/main-loop.h | 10 +-
accel/tcg/tcg-accel-ops-rr.c | 4 ++--
hw/display/virtio-gpu.c | 2 +-
hw/ppc/spapr_events.c | 2 +-
system/cpu-throttle.c | 2 +-
system/cpus.c | 4 ++--
target/i386/nvmm/nvmm-accel-ops.c | 2 +-
target/i386/whpx/whpx-accel-ops.c | 2 +-
8 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
index c26ad2a029..5764db157c 100644
--- a/include/qemu/main-loop.h
+++ b/include/qemu/main-loop.h
@@ -371,17 +371,17 @@ G_DEFINE_AUTOPTR_CLEANUP_FUNC(BQLLockAuto,
bql_auto_unlock)
= bql_auto_lock(__FILE__, __LINE__)
/*
- * qemu_cond_wait_iothread: Wait on condition for the main loop mutex
+ * qemu_cond_wait_bql: Wait on condition for the Big QEMU Lock (BQL)
*
- * This function atomically releases the main loop mutex and causes
+ * This function atomically releases the Big QEMU Lock (BQL) and causes
* the calling thread to block on the condition.
*/
-void qemu_cond_wait_iothread(QemuCond *cond);
+void qemu_cond_wait_bql(QemuCond *cond);
/*
- * qemu_cond_timedwait_iothread: like the previous, but with timeout
+ * qemu_cond_timedwait_bql: like the previous, but with timeout
*/
-void qemu_cond_timedwait_iothread(QemuCond *cond, int ms);
+void qemu_cond_timedwait_bql(QemuCond *cond, int ms);
/* internal interfaces */
diff --git a/accel/tcg/tcg-accel-ops-rr.c b/accel/tcg/tcg-accel-ops-rr.c
index c4ea372a3f..5794e5a9ce 100644
--- a/accel/tcg/tcg-accel-ops-rr.c
+++ b/accel/tcg/tcg-accel-ops-rr.c
@@ -111,7 +111,7 @@ static void rr_wait_io_event(void)
while (all_cpu_threads_idle()) {
rr_stop_kick_timer();
-qemu_cond_wait_iothread(first_cpu->halt_cond);
+qemu_cond_wait_bql(first_cpu->halt_cond);
}
rr_start_kick_timer();
@@ -198,7 +198,7 @@ static void *rr_cpu_thread_fn(void *arg)
/* wait for initial kick-off after machine start */
while (first_cpu->stopped) {
-qemu_cond_wait_iothread(first_cpu->halt_cond);
+qemu_cond_wait_bql(first_cpu->halt_cond);
/* process any pending work */
CPU_FOREACH(cpu) {
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index bae1c2a803..f8a675eb30 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -1512,7 +1512,7 @@ void virtio_gpu_reset(VirtIODevice *vdev)
g->reset_finished = false;
qemu_bh_schedule(g->reset_bh);
while (!g->reset_finished) {
-qemu_cond_wait_iothread(>reset_cond);
+qemu_cond_wait_bql(>reset_cond);
}
} else {
virtio_gpu_reset_bh(g);
diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
index deb4641505..cb0587 100644
--- a/hw/ppc/spapr_events.c
+++ b/hw/ppc/spapr_events.c
@@ -899,7 +899,7 @@ void spapr_mce_req_event(PowerPCCPU *cpu, bool recovered)
}
return;
}
-qemu_cond_wait_iothread(>fwnmi_machine_check_interlock_cond);
+qemu_cond_wait_bql(>fwnmi_machine_check_interlock_cond);
if (spapr->fwnmi_machine_check_addr == -1) {
/*
* If the machine was reset while waiting for the interlock,
diff --git a/system/cpu-throttle.c b/system/cpu-throttle.c
index 786a9a5639..c951a6c65e 100644
--- a/system/cpu-throttle.c
+++ b/system/cpu-throttle.c
@@ -54,7 +54,7 @@ static void cpu_throttle_thread(CPUState *cpu,
run_on_cpu_data opaque)
endtime_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME) + sleeptime_ns;
while (sleeptime_ns > 0 && !cpu->stop) {
if (sleeptime_ns > SCALE_MS) {
-qemu_cond_timedwait_iothread(cpu->halt_cond,
+qemu_cond_timedwait_bql(cpu->halt_cond,
sleeptime_ns / SCALE_MS);
} else {
bql_unlock();
diff --git a/system/cpus.c b/system/cpus.c
index 1ede629f1f..68d161d96b 100644
--- a/system/cpus.c
+++ b/system/cpus.c
@@ -533,12 +533,12 @@ void bql_unlock(void)
qemu_mutex_unlock();
}
-void qemu_cond_wait_iothread(QemuCond *cond)
+void qemu_cond_wait_bql(QemuCond *cond)
{
qemu_cond_wait(cond, );
}
-void qemu_cond_timedwait_iothread(QemuCond *cond, int ms)
+void qemu_cond_timedwait_bql(QemuCond *cond, int ms)
{
qemu_cond_timedwait(cond, , ms);
}
diff --git a/target/i386/nvmm/nvmm-accel-ops.c
b/target/i386/nvmm/nvmm-accel-ops.c
index f9d5e9a37a..6b2bfd9b9c 100644
--- a/target/i386/nvmm/nvmm-accel-ops.c
+++ b/target/i386/nvmm/nvmm-accel-ops.c
@@ -48,7 +48,7 @@ static void
[PULL 2/6] system/cpus: rename qemu_mutex_lock_iothread() to bql_lock()
The Big QEMU Lock (BQL) has many names and they are confusing. The actual QemuMutex variable is called qemu_global_mutex but it's commonly referred to as the BQL in discussions and some code comments. The locking APIs, however, are called qemu_mutex_lock_iothread() and qemu_mutex_unlock_iothread(). The "iothread" name is historic and comes from when the main thread was split into into KVM vcpu threads and the "iothread" (now called the main loop thread). I have contributed to the confusion myself by introducing a separate --object iothread, a separate concept unrelated to the BQL. The "iothread" name is no longer appropriate for the BQL. Rename the locking APIs to: - void bql_lock(void) - void bql_unlock(void) - bool bql_locked(void) There are more APIs with "iothread" in their names. Subsequent patches will rename them. There are also comments and documentation that will be updated in later patches. Signed-off-by: Stefan Hajnoczi Reviewed-by: Paul Durrant Acked-by: Fabiano Rosas Acked-by: David Woodhouse Reviewed-by: Cédric Le Goater Acked-by: Peter Xu Acked-by: Eric Farman Reviewed-by: Harsh Prateek Bora Acked-by: Hyman Huang Reviewed-by: Akihiko Odaki Message-id: 20240102153529.486531-2-stefa...@redhat.com Signed-off-by: Stefan Hajnoczi --- include/block/aio-wait.h | 2 +- include/qemu/main-loop.h | 39 + include/qemu/thread.h| 2 +- accel/accel-blocker.c| 10 +-- accel/dummy-cpus.c | 8 +- accel/hvf/hvf-accel-ops.c| 4 +- accel/kvm/kvm-accel-ops.c| 4 +- accel/kvm/kvm-all.c | 22 ++--- accel/tcg/cpu-exec.c | 26 +++--- accel/tcg/cputlb.c | 16 ++-- accel/tcg/tcg-accel-ops-icount.c | 4 +- accel/tcg/tcg-accel-ops-mttcg.c | 12 +-- accel/tcg/tcg-accel-ops-rr.c | 14 ++-- accel/tcg/tcg-accel-ops.c| 2 +- accel/tcg/translate-all.c| 2 +- cpu-common.c | 4 +- dump/dump.c | 4 +- hw/core/cpu-common.c | 6 +- hw/i386/intel_iommu.c| 6 +- hw/i386/kvm/xen_evtchn.c | 16 ++-- hw/i386/kvm/xen_overlay.c| 2 +- hw/i386/kvm/xen_xenstore.c | 2 +- hw/intc/arm_gicv3_cpuif.c| 2 +- hw/intc/s390_flic.c | 18 ++-- hw/misc/edu.c| 4 +- hw/misc/imx6_src.c | 2 +- hw/misc/imx7_src.c | 2 +- hw/net/xen_nic.c | 8 +- hw/ppc/pegasos2.c| 2 +- hw/ppc/ppc.c | 4 +- hw/ppc/spapr.c | 2 +- hw/ppc/spapr_rng.c | 4 +- hw/ppc/spapr_softmmu.c | 4 +- hw/remote/mpqemu-link.c | 20 ++--- hw/remote/vfio-user-obj.c| 2 +- hw/s390x/s390-skeys.c| 2 +- migration/block-dirty-bitmap.c | 4 +- migration/block.c| 16 ++-- migration/colo.c | 60 +++--- migration/dirtyrate.c| 12 +-- migration/migration.c| 52 ++-- migration/ram.c | 12 +-- replay/replay-internal.c | 2 +- semihosting/console.c| 8 +- stubs/iothread-lock.c| 6 +- system/cpu-throttle.c| 4 +- system/cpus.c| 51 ++-- system/dirtylimit.c | 4 +- system/memory.c | 2 +- system/physmem.c | 8 +- system/runstate.c| 2 +- system/watchpoint.c | 4 +- target/arm/arm-powerctl.c| 14 ++-- target/arm/helper.c | 4 +- target/arm/hvf/hvf.c | 8 +- target/arm/kvm.c | 8 +- target/arm/ptw.c | 6 +- target/arm/tcg/helper-a64.c | 8 +- target/arm/tcg/m_helper.c| 6 +- target/arm/tcg/op_helper.c | 24 +++--- target/arm/tcg/psci.c| 2 +- target/hppa/int_helper.c | 8 +- target/i386/hvf/hvf.c| 6 +- target/i386/kvm/hyperv.c | 4 +- target/i386/kvm/kvm.c| 28 +++ target/i386/kvm/xen-emu.c| 14 ++-- target/i386/nvmm/nvmm-accel-ops.c| 4 +- target/i386/nvmm/nvmm-all.c | 20 ++--- target/i386/tcg/sysemu/fpu_helper.c | 6 +- target/i386/tcg/sysemu/misc_helper.c | 4 +- target/i386/whpx/whpx-accel-ops.c| 4 +- target/i386/whpx/whpx-all.c | 24 +++--- target/loongarch/tcg/csr_helper.c| 4 +- target/mips/kvm.c| 4 +- target/mips/tcg/sysemu/cp0_helper.c | 4 +- target/openrisc/sys_helper.c | 16 ++-- target/ppc/excp_helper.c | 12 +--
[PULL 1/6] iothread: Remove unused Error** argument in aio_context_set_aio_params
From: Philippe Mathieu-Daudé
aio_context_set_aio_params() doesn't use its undocumented
Error** argument. Remove it to simplify.
Note this removes a use of "unchecked Error**" in
iothread_set_aio_context_params().
Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Markus Armbruster
Signed-off-by: Stefan Hajnoczi
Message-ID: <20231120171806.19361-1-phi...@linaro.org>
---
include/block/aio.h | 3 +--
iothread.c | 3 +--
util/aio-posix.c| 3 +--
util/aio-win32.c| 3 +--
util/main-loop.c| 5 +
5 files changed, 5 insertions(+), 12 deletions(-)
diff --git a/include/block/aio.h b/include/block/aio.h
index af05512a7d..c802a392e5 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -699,8 +699,7 @@ void aio_context_set_poll_params(AioContext *ctx, int64_t
max_ns,
* @max_batch: maximum number of requests in a batch, 0 means that the
* engine will use its default
*/
-void aio_context_set_aio_params(AioContext *ctx, int64_t max_batch,
-Error **errp);
+void aio_context_set_aio_params(AioContext *ctx, int64_t max_batch);
/**
* aio_context_set_thread_pool_params:
diff --git a/iothread.c b/iothread.c
index b753286414..6c1fc8c856 100644
--- a/iothread.c
+++ b/iothread.c
@@ -170,8 +170,7 @@ static void iothread_set_aio_context_params(EventLoopBase
*base, Error **errp)
}
aio_context_set_aio_params(iothread->ctx,
- iothread->parent_obj.aio_max_batch,
- errp);
+ iothread->parent_obj.aio_max_batch);
aio_context_set_thread_pool_params(iothread->ctx, base->thread_pool_min,
base->thread_pool_max, errp);
diff --git a/util/aio-posix.c b/util/aio-posix.c
index 7f2c99729d..266c9dd35f 100644
--- a/util/aio-posix.c
+++ b/util/aio-posix.c
@@ -777,8 +777,7 @@ void aio_context_set_poll_params(AioContext *ctx, int64_t
max_ns,
aio_notify(ctx);
}
-void aio_context_set_aio_params(AioContext *ctx, int64_t max_batch,
-Error **errp)
+void aio_context_set_aio_params(AioContext *ctx, int64_t max_batch)
{
/*
* No thread synchronization here, it doesn't matter if an incorrect value
diff --git a/util/aio-win32.c b/util/aio-win32.c
index 948ef47a4d..d144f9391f 100644
--- a/util/aio-win32.c
+++ b/util/aio-win32.c
@@ -438,7 +438,6 @@ void aio_context_set_poll_params(AioContext *ctx, int64_t
max_ns,
}
}
-void aio_context_set_aio_params(AioContext *ctx, int64_t max_batch,
-Error **errp)
+void aio_context_set_aio_params(AioContext *ctx, int64_t max_batch)
{
}
diff --git a/util/main-loop.c b/util/main-loop.c
index 797b640c41..63b4cda84a 100644
--- a/util/main-loop.c
+++ b/util/main-loop.c
@@ -192,10 +192,7 @@ static void main_loop_update_params(EventLoopBase *base,
Error **errp)
return;
}
-aio_context_set_aio_params(qemu_aio_context, base->aio_max_batch, errp);
-if (*errp) {
-return;
-}
+aio_context_set_aio_params(qemu_aio_context, base->aio_max_batch);
aio_context_set_thread_pool_params(qemu_aio_context, base->thread_pool_min,
base->thread_pool_max, errp);
--
2.43.0
[PULL 0/6] Block patches
The following changes since commit ffd454c67e38cc6df792733ebc5d967eee28ac0d: Merge tag 'pull-vfio-20240107' of https://github.com/legoater/qemu into staging (2024-01-08 10:28:42 +) are available in the Git repository at: https://gitlab.com/stefanha/qemu.git tags/block-pull-request for you to fetch changes up to 0b2675c473f68f13bc5ca1dd1c43ce421542e7b8: Rename "QEMU global mutex" to "BQL" in comments and docs (2024-01-08 10:45:43 -0500) Pull request Philippe Mathieu-Daudé (1): iothread: Remove unused Error** argument in aio_context_set_aio_params Stefan Hajnoczi (5): system/cpus: rename qemu_mutex_lock_iothread() to bql_lock() qemu/main-loop: rename QEMU_IOTHREAD_LOCK_GUARD to BQL_LOCK_GUARD qemu/main-loop: rename qemu_cond_wait_iothread() to qemu_cond_wait_bql() Replace "iothread lock" with "BQL" in comments Rename "QEMU global mutex" to "BQL" in comments and docs docs/devel/multi-thread-tcg.rst | 7 +- docs/devel/qapi-code-gen.rst | 2 +- docs/devel/replay.rst| 2 +- docs/devel/reset.rst | 2 +- docs/devel/multiple-iothreads.txt| 14 ++-- hw/display/qxl.h | 2 +- include/block/aio-wait.h | 2 +- include/block/aio.h | 3 +- include/block/blockjob.h | 6 +- include/exec/cpu-common.h| 2 +- include/exec/memory.h| 4 +- include/exec/ramblock.h | 2 +- include/io/task.h| 2 +- include/migration/register.h | 8 +- include/qemu/coroutine-core.h| 2 +- include/qemu/coroutine.h | 2 +- include/qemu/main-loop.h | 68 --- include/qemu/thread.h| 2 +- target/arm/internals.h | 4 +- accel/accel-blocker.c| 10 +-- accel/dummy-cpus.c | 8 +- accel/hvf/hvf-accel-ops.c| 4 +- accel/kvm/kvm-accel-ops.c| 4 +- accel/kvm/kvm-all.c | 22 ++--- accel/tcg/cpu-exec.c | 26 +++--- accel/tcg/cputlb.c | 20 ++--- accel/tcg/tcg-accel-ops-icount.c | 6 +- accel/tcg/tcg-accel-ops-mttcg.c | 12 +-- accel/tcg/tcg-accel-ops-rr.c | 18 ++-- accel/tcg/tcg-accel-ops.c| 2 +- accel/tcg/translate-all.c| 2 +- cpu-common.c | 4 +- dump/dump.c | 4 +- hw/block/dataplane/virtio-blk.c | 8 +- hw/block/virtio-blk.c| 2 +- hw/core/cpu-common.c | 6 +- hw/display/virtio-gpu.c | 2 +- hw/i386/intel_iommu.c| 6 +- hw/i386/kvm/xen_evtchn.c | 30 +++ hw/i386/kvm/xen_gnttab.c | 2 +- hw/i386/kvm/xen_overlay.c| 2 +- hw/i386/kvm/xen_xenstore.c | 2 +- hw/intc/arm_gicv3_cpuif.c| 2 +- hw/intc/s390_flic.c | 18 ++-- hw/mips/mips_int.c | 2 +- hw/misc/edu.c| 4 +- hw/misc/imx6_src.c | 2 +- hw/misc/imx7_src.c | 2 +- hw/net/xen_nic.c | 8 +- hw/ppc/pegasos2.c| 2 +- hw/ppc/ppc.c | 6 +- hw/ppc/spapr.c | 2 +- hw/ppc/spapr_events.c| 2 +- hw/ppc/spapr_rng.c | 4 +- hw/ppc/spapr_softmmu.c | 4 +- hw/remote/mpqemu-link.c | 22 ++--- hw/remote/vfio-user-obj.c| 2 +- hw/s390x/s390-skeys.c| 2 +- hw/scsi/virtio-scsi-dataplane.c | 6 +- iothread.c | 3 +- migration/block-dirty-bitmap.c | 14 ++-- migration/block.c| 38 - migration/colo.c | 62 +++--- migration/dirtyrate.c| 12 +-- migration/migration.c| 54 ++-- migration/ram.c | 16 ++-- net/tap.c| 2 +- replay/replay-internal.c | 2 +- semihosting/console.c| 8 +- stubs/iothread-lock.c| 6 +- system/cpu-throttle.c| 6 +- system/cpus.c| 55 +++-- system/dirtylimit.c | 4 +- system/memory.c | 2 +- system/physmem.c | 14 ++-- system/runstate.c| 2 +- system/watchpoint.c | 4 +- target/arm/arm-powerctl.c| 14 ++-- target/arm/helper.c | 6 +- target/arm/hvf/hvf.c | 8 +- target/arm/kvm.c | 8 +- target/arm/ptw.c | 6 +-
Re: [PATCH v3 0/5] Make Big QEMU Lock naming consistent
On Tue, Jan 02, 2024 at 10:35:24AM -0500, Stefan Hajnoczi wrote:
> v3:
> - Rebase
> - Define bql_lock() macro on a single line [Akihiko Odaki]
> v2:
> - Rename APIs bql_*() [PeterX]
> - Spell out "Big QEMU Lock (BQL)" in doc comments [PeterX]
> - Rename "iolock" variables in hw/remote/mpqemu-link.c [Harsh]
> - Fix bql_auto_lock() indentation in Patch 2 [Ilya]
> - "with BQL taken" -> "with the BQL taken" [Philippe]
> - "under BQL" -> "under the BQL" [Philippe]
>
> The Big QEMU Lock ("BQL") has two other names: "iothread lock" and "QEMU
> global
> mutex". The term "iothread lock" is easily confused with the unrelated
> --object
> iothread (iothread.c).
>
> This series updates the code and documentation to consistently use "BQL". This
> makes the code easier to understand.
>
> Stefan Hajnoczi (5):
> system/cpus: rename qemu_mutex_lock_iothread() to bql_lock()
> qemu/main-loop: rename QEMU_IOTHREAD_LOCK_GUARD to BQL_LOCK_GUARD
> qemu/main-loop: rename qemu_cond_wait_iothread() to
> qemu_cond_wait_bql()
> Replace "iothread lock" with "BQL" in comments
> Rename "QEMU global mutex" to "BQL" in comments and docs
>
> docs/devel/multi-thread-tcg.rst | 7 +-
> docs/devel/qapi-code-gen.rst | 2 +-
> docs/devel/replay.rst| 2 +-
> docs/devel/reset.rst | 2 +-
> docs/devel/multiple-iothreads.txt| 14 ++--
> hw/display/qxl.h | 2 +-
> include/block/aio-wait.h | 2 +-
> include/block/blockjob.h | 6 +-
> include/exec/cpu-common.h| 2 +-
> include/exec/memory.h| 4 +-
> include/exec/ramblock.h | 2 +-
> include/io/task.h| 2 +-
> include/migration/register.h | 8 +-
> include/qemu/coroutine-core.h| 2 +-
> include/qemu/coroutine.h | 2 +-
> include/qemu/main-loop.h | 68 ---
> include/qemu/thread.h| 2 +-
> target/arm/internals.h | 4 +-
> accel/accel-blocker.c| 10 +--
> accel/dummy-cpus.c | 8 +-
> accel/hvf/hvf-accel-ops.c| 4 +-
> accel/kvm/kvm-accel-ops.c| 4 +-
> accel/kvm/kvm-all.c | 22 ++---
> accel/tcg/cpu-exec.c | 26 +++---
> accel/tcg/cputlb.c | 20 ++---
> accel/tcg/tcg-accel-ops-icount.c | 6 +-
> accel/tcg/tcg-accel-ops-mttcg.c | 12 +--
> accel/tcg/tcg-accel-ops-rr.c | 18 ++--
> accel/tcg/tcg-accel-ops.c| 2 +-
> accel/tcg/translate-all.c| 2 +-
> cpu-common.c | 4 +-
> dump/dump.c | 4 +-
> hw/block/dataplane/virtio-blk.c | 8 +-
> hw/block/virtio-blk.c| 2 +-
> hw/core/cpu-common.c | 6 +-
> hw/display/virtio-gpu.c | 2 +-
> hw/i386/intel_iommu.c| 6 +-
> hw/i386/kvm/xen_evtchn.c | 30 +++
> hw/i386/kvm/xen_gnttab.c | 2 +-
> hw/i386/kvm/xen_overlay.c| 2 +-
> hw/i386/kvm/xen_xenstore.c | 2 +-
> hw/intc/arm_gicv3_cpuif.c| 2 +-
> hw/intc/s390_flic.c | 18 ++--
> hw/mips/mips_int.c | 2 +-
> hw/misc/edu.c| 4 +-
> hw/misc/imx6_src.c | 2 +-
> hw/misc/imx7_src.c | 2 +-
> hw/net/xen_nic.c | 8 +-
> hw/ppc/pegasos2.c| 2 +-
> hw/ppc/ppc.c | 6 +-
> hw/ppc/spapr.c | 2 +-
> hw/ppc/spapr_events.c| 2 +-
> hw/ppc/spapr_rng.c | 4 +-
> hw/ppc/spapr_softmmu.c | 4 +-
> hw/remote/mpqemu-link.c | 22 ++---
> hw/remote/vfio-user-obj.c| 2 +-
> hw/s390x/s390-skeys.c| 2 +-
> hw/scsi/virtio-scsi-dataplane.c | 6 +-
> migration/block-dirty-bitmap.c | 14 ++--
> migration/block.c| 38 -
> migration/colo.c | 62 +++---
> migration/dirtyrate.c| 12 +--
> migration/migration.c| 54 ++--
> migration/ram.c | 16 ++--
> net/tap.c| 2 +-
> replay/replay-internal.c | 2 +-
> semihosting/console.c| 8 +-
> stubs/iothread-lock.c| 6 +-
> system/cpu-throttle.c| 6 +-
> system/cpus.c| 55 +++--
> system/dirtylimit.c | 4 +-
> system/memory.c | 2 +-
> system/physmem.c | 14 ++--
> system/runstate.c| 2 +-
> system/watchpoint.c | 4 +-
> target/arm/arm-powerctl.c| 14 ++--
> target/arm/helper.c |
Re: [PATCH v5 04/13] xen: extend domctl interface for cache coloring
Hi Julien, On Mon, Jan 8, 2024 at 4:32 PM Julien Grall wrote: > > Hi, > > On 08/01/2024 15:18, Carlo Nonato wrote: > >> No. I am saying that that we should not be able to allow changing the > >> colors after the memory has been allocated. To give an example, your > >> current code would allow: > >> > >> 1) Setup the P2M pools or allocate RAM > >> 2) Set the color > >> > >> This would render the coloring configuration moot. > >> > >> Whether we want to allow changing the coloring before hand is a > >> different question and as I wrote earlier on, I don't mind if you want > >> to forbid that. > > > > At the moment I'm relying on the toolstack in the sense that I know that it > > will set colors right after domain creation and before memory allocation. > > Calling alloc_domheap_pages() without a coloring configuration makes Xen > > crash, so it's mandatory to have the configuration done before any > > allocation. > > I know that we shouldn't rely on the toolstack this much, but I didn't > > find a better way. Given this assumption, looking for an already existing > > color configuration of a domain is sufficient to avoid what you are saying. > > > > Is it possible to enforce such a constraint with domctl? > I mean to be > > sure that this domctl will be called at a precise time. > > Yes. You can... > > > > > Thanks. > > > >>> I don't know what to check that. > >> > >> You can check the size of the P2M pool (d->arch.paging.p2m_total_pages) > >> is still 0. I think for RAM, you can check d->tot_pages == 0. > > ... reject the call if either of the two fields are not zero. What I'm saying is that Xen would crash before even reaching this point if no colors were provided. Let's say that the toolstack or whatever hypercall user isn't calling this domctl at all (or not at the right time), then the domain colored allocator would always return null pages since there are no colors. We would have a crash and your if (or mine) would be useless. Let's say that now the domctl is called at the right time (no p2m, no tot_pages, no colors) then we can set the colors and everything works. >From this point other calls to this domctl would be skipped because of your if which is equivalent to mine (checking for colors existence). Also bringing in checks on p2m would require arch specific code which I was trying to avoid. Thanks. > Cheers, > > -- > Julien Grall
Re: [PATCH v5 04/13] xen: extend domctl interface for cache coloring
Hi, On 08/01/2024 15:18, Carlo Nonato wrote: No. I am saying that that we should not be able to allow changing the colors after the memory has been allocated. To give an example, your current code would allow: 1) Setup the P2M pools or allocate RAM 2) Set the color This would render the coloring configuration moot. Whether we want to allow changing the coloring before hand is a different question and as I wrote earlier on, I don't mind if you want to forbid that. At the moment I'm relying on the toolstack in the sense that I know that it will set colors right after domain creation and before memory allocation. Calling alloc_domheap_pages() without a coloring configuration makes Xen crash, so it's mandatory to have the configuration done before any allocation. I know that we shouldn't rely on the toolstack this much, but I didn't find a better way. Given this assumption, looking for an already existing color configuration of a domain is sufficient to avoid what you are saying. Is it possible to enforce such a constraint with domctl? > I mean to be sure that this domctl will be called at a precise time. Yes. You can... Thanks. I don't know what to check that. You can check the size of the P2M pool (d->arch.paging.p2m_total_pages) is still 0. I think for RAM, you can check d->tot_pages == 0. ... reject the call if either of the two fields are not zero. Cheers, -- Julien Grall
Re: [PATCH v2] NUMA: limit first_valid_mfn exposure
Hi Jan, On 08/01/2024 15:03, Jan Beulich wrote: On 08.01.2024 15:57, Julien Grall wrote: Hi Jan, On 08/01/2024 14:47, Jan Beulich wrote: On 08.01.2024 15:13, Julien Grall wrote: Hi Jan, On 08/01/2024 11:43, Jan Beulich wrote: On 08.01.2024 12:37, Julien Grall wrote: On 08/01/2024 11:31, Jan Beulich wrote: Address the TODO regarding first_valid_mfn by making the variable static when NUMA=y, thus also addressing a Misra C:2012 rule 8.4 concern (on x86). Signed-off-by: Jan Beulich --- Julien suggests something like STATIC_IF(CONFIG_NUMA) unsigned long first_valid_mfn; but I view this as non-scalable (or at least I can't see how to implement such in a scalabale way) and hence undesirable to introduce. I don't really see the scalability problem. Can you explain a bit more? Well, when seeing your original suggestion, I first considered it quite reasonable. But when thinking how to implement it, I couldn't see what #define STATIC_IF(cfg) should expand to. That's simply because a macro body cannot itself have pre-processor directives. Hence all I could think of was #ifdef CONFIG_NUMA # define static_if_CONFIG_NUMA static #else # define static_if_CONFIG_NUMA #endif #define STATIC_IF(cfg) static_if_ ## cfg And I think it is easy to see how this wouldn't scale across CONFIG_xyz. Plus that that point STATIC_IF() itself would be pretty much redundant. But maybe I'm simply overlooking the obvious ... You can use the same trick as for IS_ENABLED. The code below will select static or nothing: #define static_enabled(cfg) _static_enabled(cfg) #define _static_enabled(value) __static_enabled(__ARG_PLACEHOLDER_##value) #define __static_enabled(arg1_or_junk) ___static_enabled(arg1_or_junk static,) #define ___static_enabled(__ignored, val, ...) val #define STATIC_IF(option) static_enabled(option) I have tested both with CONFIG_NUMA and !CONFIG_NUMA to confirm the visibility of the variable will be correct. Hmm, okay. Then my 2nd scalability concern, in another dimension: What if static-ness ends up depending on two (or more) CONFIG_*? Do you have any concrete example where this would be useful? If not, then I suggest to go with this solution and we can cross the bridge when we have an example. We don't have to solve everything at once and at least with the approach I proposed we can start to use STATIC_IF() (or EXTERN_IF) a bit more often without open-coding it. Well. IS_ENABLED() is okay in this regard because you can combine multiple of them (with && or ||). The same isn't true here (afaict). Indeed it is not. But I can't think of any place where it would be useful at the moment. After all I could equally well say that as long as we don't have a sufficient number of such examples, but just one, not introducing a special construct is going to be okay for the time being. The thing is I expect more place in the code requiring to use STATIC_IF() (or EXTERN_IF()). So it is not clear to me why we would delay solving the open-coding... In fact, this is a matter of taste, but the cost (less readable) vs benefit (not exposing) is not really worth it to me here. Mostly, because I don't entirely see the problem of keeping first_valid_mfn exposed to everyone. This balance would change if we introduce STATIC_IF(). Anyway, I will let other share their opinions. Cheers, -- Julien Grall
Re: [PATCH v5 04/13] xen: extend domctl interface for cache coloring
Hi Julien
On Mon, Jan 8, 2024 at 12:36 PM Julien Grall wrote:
>
> Hi Carlo,
>
> On 08/01/2024 11:19, Carlo Nonato wrote:
> > Hi Julien,
> >
> > On Mon, Jan 8, 2024 at 12:01 PM Julien Grall wrote:
> >>
> >> Hi Carlo,
> >>
> >> On 08/01/2024 10:27, Carlo Nonato wrote:
> >>> On Fri, Jan 5, 2024 at 6:26 PM Julien Grall wrote:
> On 02/01/2024 09:51, Carlo Nonato wrote:
> > This commit updates the domctl interface to allow the user to set cache
> > coloring configurations from the toolstack.
> > It also implements the functionality for arm64.
> >
> > Based on original work from: Luca Miccio
> >
> > Signed-off-by: Carlo Nonato
> > Signed-off-by: Marco Solieri
> > ---
> > v5:
> > - added a new hypercall to set colors
> > - uint for the guest handle
> > v4:
> > - updated XEN_DOMCTL_INTERFACE_VERSION
> > ---
> > xen/arch/arm/llc-coloring.c| 17 +
> > xen/common/domctl.c| 11 +++
> > xen/include/public/domctl.h| 10 +-
> > xen/include/xen/llc-coloring.h | 3 +++
> > 4 files changed, 40 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/arch/arm/llc-coloring.c b/xen/arch/arm/llc-coloring.c
> > index 5ce58aba70..a08614ec36 100644
> > --- a/xen/arch/arm/llc-coloring.c
> > +++ b/xen/arch/arm/llc-coloring.c
> > @@ -9,6 +9,7 @@
> > *Carlo Nonato
> > */
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -278,6 +279,22 @@ int dom0_set_llc_colors(struct domain *d)
> > return domain_check_colors(d);
> > }
> >
> > +int domain_set_llc_colors_domctl(struct domain *d,
> > + const struct
> > xen_domctl_set_llc_colors *config)
> > +{
> > +if ( d->num_llc_colors )
> > +return -EEXIST;
> > +
> > +if ( domain_alloc_colors(d, config->num_llc_colors) )
>
> domain_alloc_colors() doesn't sanity check config->num_llc_colors before
> allocating the array. You want a check the size before so we would not
> try to allocate an arbitrary amount of memory.
>
> > +return -ENOMEM;
> > +
> > +if ( copy_from_guest(d->llc_colors, config->llc_colors,
> > + config->num_llc_colors) )
> > +return -EFAULT;
> > +
> > +return domain_check_colors(d);
> > +}
> > +
> > /*
> > * Local variables:
> > * mode: C
> > diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> > index f5a71ee5f7..b6867d0602 100644
> > --- a/xen/common/domctl.c
> > +++ b/xen/common/domctl.c
> > @@ -8,6 +8,7 @@
> >
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -858,6 +859,16 @@ long
> > do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> > __HYPERVISOR_domctl, "h", u_domctl);
> > break;
> >
> > +case XEN_DOMCTL_set_llc_colors:
> > +if ( !llc_coloring_enabled )
> > +break;
> > +
> > +ret = domain_set_llc_colors_domctl(d, >u.set_llc_colors);
> > +if ( ret == -EEXIST )
> > +printk(XENLOG_ERR
> > + "Can't set LLC colors on an already created
> > domain\n");
>
> To me, the message doesn't match the check in
> domain_set_llc_colors_domctl(). But I think you want to check that no
> memory was yet allocated to the domain. Otherwise, you coloring will be
> wrong.
>
> Also, it is a bit unclear why you print a message for -EEXIST but not
> the others. In this instance, I would consider to print nothing at all.
> >>>
> >>> The problem here is that we don't support recoloring. When a domain is
> >>> created it receives a coloring configuration and it can't change. If this
> >>> hypercall is called twice I have to stop the second time somehow.
> >> Looking at your check what you prevent is a toolstack updating the array
> >> twice. But that would be ok (/!\ I am not saying we should allow it) so
> >> long no memory has been allocated to the domain.
> >>
> >> But I also consider we would re-color once we started to allocate memory
> >> for the domain (either RAM or P2M). This seems to be missed out in your
> >> check.
> >
> > So you want to be able to change colors if no memory has yet been allocated?
>
> No. I am saying that that we should not be able to allow changing the
> colors after the memory has been allocated. To give an example, your
> current code would allow:
>
>1) Setup the P2M pools or allocate RAM
>2) Set the color
>
> This would render the coloring configuration moot.
>
> Whether we want to allow changing the coloring before hand is a
> different question
Re: [RFC XEN PATCH v4 4/5] domctl: Use gsi to grant/revoke irq permission
On Mon, Jan 08, 2024 at 09:55:26AM +0100, Jan Beulich wrote:
> On 06.01.2024 02:08, Stefano Stabellini wrote:
> > On Fri, 5 Jan 2024, Jiqian Chen wrote:
> >> --- a/tools/libs/light/libxl_pci.c
> >> +++ b/tools/libs/light/libxl_pci.c
> >> @@ -1418,6 +1418,7 @@ static void pci_add_dm_done(libxl__egc *egc,
> >> unsigned long long start, end, flags, size;
> >> int irq, i;
> >> int r;
> >> +int gsi;
> >> uint32_t flag = XEN_DOMCTL_DEV_RDM_RELAXED;
> >> uint32_t domainid = domid;
> >> bool isstubdom = libxl_is_stubdom(ctx, domid, );
> >> @@ -1486,6 +1487,7 @@ static void pci_add_dm_done(libxl__egc *egc,
> >> goto out_no_irq;
> >> }
> >> if ((fscanf(f, "%u", ) == 1) && irq) {
> >> +gsi = irq;
> >
> > A question for Roger and Jan: are we always guaranteed that gsi == irq
> > (also in the PV case)?
>
> Iirc for IO-APIC based IRQs that's always the case;
I think that's always the case on Linux, because it calls
PHYSDEVOP_map_pirq with index == pirq (see Linux
pci_xen_initial_domain()). But other OSes could possibly make the
call with pirq == -1 and get a randomly allocated pirq for GSIs.
IOW: I don't think the pirq field in xen_domctl_irq_permission can be
altered like proposed here to switch from passing a pirq to a GSI. A
new hypercall should be introduced that either is GSI specific, or
contains a type field in order to specify the namespace the field
targets.
Thanks, Roger.
Re: [PATCH v2] NUMA: limit first_valid_mfn exposure
On 08.01.2024 15:57, Julien Grall wrote: > Hi Jan, > > On 08/01/2024 14:47, Jan Beulich wrote: >> On 08.01.2024 15:13, Julien Grall wrote: >>> Hi Jan, >>> >>> On 08/01/2024 11:43, Jan Beulich wrote: On 08.01.2024 12:37, Julien Grall wrote: > On 08/01/2024 11:31, Jan Beulich wrote: >> Address the TODO regarding first_valid_mfn by making the variable static >> when NUMA=y, thus also addressing a Misra C:2012 rule 8.4 concern (on >> x86). >> >> Signed-off-by: Jan Beulich >> --- >> Julien suggests something like >> >> STATIC_IF(CONFIG_NUMA) unsigned long first_valid_mfn; >> >> but I view this as non-scalable (or at least I can't see how to >> implement such in a scalabale way) and hence undesirable to introduce. > > I don't really see the scalability problem. Can you explain a bit more? Well, when seeing your original suggestion, I first considered it quite reasonable. But when thinking how to implement it, I couldn't see what #define STATIC_IF(cfg) should expand to. That's simply because a macro body cannot itself have pre-processor directives. Hence all I could think of was #ifdef CONFIG_NUMA # define static_if_CONFIG_NUMA static #else # define static_if_CONFIG_NUMA #endif #define STATIC_IF(cfg) static_if_ ## cfg And I think it is easy to see how this wouldn't scale across CONFIG_xyz. Plus that that point STATIC_IF() itself would be pretty much redundant. But maybe I'm simply overlooking the obvious ... >>> >>> You can use the same trick as for IS_ENABLED. The code below will select >>> static or nothing: >>> >>> #define static_enabled(cfg) _static_enabled(cfg) >>> #define _static_enabled(value) __static_enabled(__ARG_PLACEHOLDER_##value) >>> #define __static_enabled(arg1_or_junk) ___static_enabled(arg1_or_junk >>> static,) >>> #define ___static_enabled(__ignored, val, ...) val >>> >>> #define STATIC_IF(option) static_enabled(option) >>> >>> I have tested both with CONFIG_NUMA and !CONFIG_NUMA to confirm the >>> visibility of the variable will be correct. >> >> Hmm, okay. Then my 2nd scalability concern, in another dimension: What >> if static-ness ends up depending on two (or more) CONFIG_*? > > Do you have any concrete example where this would be useful? If not, > then I suggest to go with this solution and we can cross the bridge when > we have an example. > > We don't have to solve everything at once and at least with the approach > I proposed we can start to use STATIC_IF() (or EXTERN_IF) a bit more > often without open-coding it. Well. IS_ENABLED() is okay in this regard because you can combine multiple of them (with && or ||). The same isn't true here (afaict). After all I could equally well say that as long as we don't have a sufficient number of such examples, but just one, not introducing a special construct is going to be okay for the time being. Jan
Re: [PATCH v3 07/34] xen/asm-generic: introdure nospec.h
On Thu, 2024-01-04 at 12:06 +0100, Jan Beulich wrote: > On 22.12.2023 16:12, Oleksii Kurochko wrote: > > The header is similar between Arm, PPC, and RISC-V, > > so it has been moved to asm-generic. > > > > Signed-off-by: Oleksii Kurochko > > Acked-by: Jan Beulich > > A word may want saying though on ... > > > --- a/xen/arch/arm/include/asm/nospec.h > > +++ b/xen/include/asm-generic/nospec.h > > @@ -1,8 +1,8 @@ > > -/* SPDX-License-Identifier: GPL-2.0 */ > > -/* Copyright 2018 Amazon.com, Inc. or its affiliates. All Rights > > Reserved. */ > > +/* SPDX-License-Identifier: GPL-2.0-only */ > > +#ifndef _ASM_GENERIC_NOSPEC_H > > +#define _ASM_GENERIC_NOSPEC_H > > ... the removal of the copyright line. It was automatically decided by Git to do it this way. I'll try to make it generate a new file instead of moving/renaming Arm's nospec.h. ~ Oleksii
Re: Xen 4.19 release schedule proposal
On 08.01.2024 15:37, Oleksii wrote: > On Thu, 2024-01-04 at 13:52 +0100, Jan Beulich wrote: >> On 02.01.2024 17:59, Oleksii wrote: >>> I'd like to propose the release schedule for Xen 4.19. >>> >>> Based on the previous release schedules [1] and [2], it seems the >>> next >>> release date should be on Wednesday, July 10, 2024: >>> >>> ** Proposed option: Wed Jul 10, 2024 ** >>> (+9 months from Xen 4.18 release) >> >> Hmm, aren't we intending to be on a 8 month cadence? > Considering that in July, there will be the Xen Developer Summit, we > can aim for an 8-month cadence. July? Iirc I read June in the announcement. > However, in the Xen release document, there was mention of a discussion > [1] about cadence: > "With 18 months of full support and 36 months of security support, the > number of concurrent supported releases will be the same with either 8 > or 9 months release cycles, so I have chosen an 8-month cycle for now." > > I interpreted this as either an 8 or 9-month cycle, and it's not > strict. If there's a strict requirement for a specific duration, I'll > resend the Release Schedule Proposal. I'm not sure about "strict". Yet ... > [1] https://lists.xen.org/archives/html/xen-devel/2018-07/msg02240.html ... this very mail worked out how overlap with larger holiday ranges could be minimized not only for a single release, but for any as long as the cadence is followed. Iirc this works out better with 8 months (as kind of to be expected, as then there are only 3 variants, whereas with 9 months it would be 4 of them). Just to clarify, personally I'm fine with 9 months or even longer, but it seemed to me that we had settled on 8. Jan
Re: [PATCH v2] NUMA: limit first_valid_mfn exposure
Hi Jan, On 08/01/2024 14:47, Jan Beulich wrote: On 08.01.2024 15:13, Julien Grall wrote: Hi Jan, On 08/01/2024 11:43, Jan Beulich wrote: On 08.01.2024 12:37, Julien Grall wrote: On 08/01/2024 11:31, Jan Beulich wrote: Address the TODO regarding first_valid_mfn by making the variable static when NUMA=y, thus also addressing a Misra C:2012 rule 8.4 concern (on x86). Signed-off-by: Jan Beulich --- Julien suggests something like STATIC_IF(CONFIG_NUMA) unsigned long first_valid_mfn; but I view this as non-scalable (or at least I can't see how to implement such in a scalabale way) and hence undesirable to introduce. I don't really see the scalability problem. Can you explain a bit more? Well, when seeing your original suggestion, I first considered it quite reasonable. But when thinking how to implement it, I couldn't see what #define STATIC_IF(cfg) should expand to. That's simply because a macro body cannot itself have pre-processor directives. Hence all I could think of was #ifdef CONFIG_NUMA # define static_if_CONFIG_NUMA static #else # define static_if_CONFIG_NUMA #endif #define STATIC_IF(cfg) static_if_ ## cfg And I think it is easy to see how this wouldn't scale across CONFIG_xyz. Plus that that point STATIC_IF() itself would be pretty much redundant. But maybe I'm simply overlooking the obvious ... You can use the same trick as for IS_ENABLED. The code below will select static or nothing: #define static_enabled(cfg) _static_enabled(cfg) #define _static_enabled(value) __static_enabled(__ARG_PLACEHOLDER_##value) #define __static_enabled(arg1_or_junk) ___static_enabled(arg1_or_junk static,) #define ___static_enabled(__ignored, val, ...) val #define STATIC_IF(option) static_enabled(option) I have tested both with CONFIG_NUMA and !CONFIG_NUMA to confirm the visibility of the variable will be correct. Hmm, okay. Then my 2nd scalability concern, in another dimension: What if static-ness ends up depending on two (or more) CONFIG_*? Do you have any concrete example where this would be useful? If not, then I suggest to go with this solution and we can cross the bridge when we have an example. We don't have to solve everything at once and at least with the approach I proposed we can start to use STATIC_IF() (or EXTERN_IF) a bit more often without open-coding it. Cheers, -- Julien Grall
Re: [PATCH v3 06/34] xen: avoid generation of empty asm/iommu.h
On Thu, 2024-01-04 at 12:04 +0100, Jan Beulich wrote:
> On 22.12.2023 16:12, Oleksii Kurochko wrote:
> > Signed-off-by: Oleksii Kurochko
>
> The change looks okay-ish, but again needs a description: You want to
> explain why you use the absolute minimum of the scopes the two (or,
> in principle, possibly more) #ifdef-s cover.
Sure, I'll add a description to the commit message.
~ Oleksii
> > --- a/xen/include/xen/iommu.h
> > +++ b/xen/include/xen/iommu.h
> > @@ -337,7 +337,9 @@ extern int
> > iommu_add_extra_reserved_device_memory(unsigned long start,
> > extern int iommu_get_extra_reserved_device_memory(iommu_grdm_t
> > *func,
> > void *ctxt);
> >
> > +#ifdef CONFIG_HAS_PASSTHROUGH
> > #include
> > +#endif
> >
> > #ifndef iommu_call
> > # define iommu_call(ops, fn, args...) ((ops)->fn(args))
> > @@ -345,7 +347,9 @@ extern int
> > iommu_get_extra_reserved_device_memory(iommu_grdm_t *func,
> > #endif
> >
> > struct domain_iommu {
> > +#ifdef CONFIG_HAS_PASSTHROUGH
> > struct arch_iommu arch;
> > +#endif
> >
> > /* iommu_ops */
> > const struct iommu_ops *platform_ops;
>
Re: [PATCH v2] NUMA: limit first_valid_mfn exposure
On 08.01.2024 15:13, Julien Grall wrote: > Hi Jan, > > On 08/01/2024 11:43, Jan Beulich wrote: >> On 08.01.2024 12:37, Julien Grall wrote: >>> On 08/01/2024 11:31, Jan Beulich wrote: Address the TODO regarding first_valid_mfn by making the variable static when NUMA=y, thus also addressing a Misra C:2012 rule 8.4 concern (on x86). Signed-off-by: Jan Beulich --- Julien suggests something like STATIC_IF(CONFIG_NUMA) unsigned long first_valid_mfn; but I view this as non-scalable (or at least I can't see how to implement such in a scalabale way) and hence undesirable to introduce. >>> >>> I don't really see the scalability problem. Can you explain a bit more? >> >> Well, when seeing your original suggestion, I first considered it quite >> reasonable. But when thinking how to implement it, I couldn't see what >> >> #define STATIC_IF(cfg) >> >> should expand to. That's simply because a macro body cannot itself have >> pre-processor directives. Hence all I could think of was >> >> #ifdef CONFIG_NUMA >> # define static_if_CONFIG_NUMA static >> #else >> # define static_if_CONFIG_NUMA >> #endif >> #define STATIC_IF(cfg) static_if_ ## cfg >> >> And I think it is easy to see how this wouldn't scale across CONFIG_xyz. >> Plus that that point STATIC_IF() itself would be pretty much redundant. >> But maybe I'm simply overlooking the obvious ... > > You can use the same trick as for IS_ENABLED. The code below will select > static or nothing: > > #define static_enabled(cfg) _static_enabled(cfg) > #define _static_enabled(value) __static_enabled(__ARG_PLACEHOLDER_##value) > #define __static_enabled(arg1_or_junk) ___static_enabled(arg1_or_junk > static,) > #define ___static_enabled(__ignored, val, ...) val > > #define STATIC_IF(option) static_enabled(option) > > I have tested both with CONFIG_NUMA and !CONFIG_NUMA to confirm the > visibility of the variable will be correct. Hmm, okay. Then my 2nd scalability concern, in another dimension: What if static-ness ends up depending on two (or more) CONFIG_*? Jan
Re: [PATCH v3 03/34] xen: add support in public/hvm/save.h for PPC and RISC-V
On Thu, 2024-01-04 at 12:01 +0100, Jan Beulich wrote: > On 22.12.2023 16:12, Oleksii Kurochko wrote: > > Signed-off-by: Oleksii Kurochko > > Since you change how PPC is handled, this can't go without > description (i.e. > justification). I thought adding a comment in the code would be sufficient. I can include something similar in the commit message as well. ~ Oleksii > > > --- a/xen/include/public/hvm/save.h > > +++ b/xen/include/public/hvm/save.h > > @@ -89,8 +89,8 @@ DECLARE_HVM_SAVE_TYPE(END, 0, struct > > hvm_save_end); > > #include "../arch-x86/hvm/save.h" > > #elif defined(__arm__) || defined(__aarch64__) > > #include "../arch-arm/hvm/save.h" > > -#elif defined(__powerpc64__) > > -#include "../arch-ppc.h" > > +#elif defined(__powerpc64__) || defined(__riscv) > > +/* no specific header to include */ > > #else > > #error "unsupported architecture" > > #endif >
Re: [XEN RFC] x86/uaccess: remove __{put,get}_user_bad()
On 08.01.2024 15:01, Federico Serafini wrote: > Additionally, looking at violations of 16.3 on X86 [1], > I think we should also consider generate_exception(), > ASSERT_UNREACHABLE() and PARSE_ERR_RET() as allowed terminals > for a switch-clause, do you agree? No, and iirc this was discussed before. ASSERT_UNREACHABLE() is a debug-build-only construct, so unsuitable. The other two aren't global constructs, and hence shouldn't be deviated globally. generate_exception() at least ends in a goto anyway, so why would it need special treatment? Jan
Re: Xen 4.19 release schedule proposal
On Thu, 2024-01-04 at 13:52 +0100, Jan Beulich wrote: > On 02.01.2024 17:59, Oleksii wrote: > > I'd like to propose the release schedule for Xen 4.19. > > > > Based on the previous release schedules [1] and [2], it seems the > > next > > release date should be on Wednesday, July 10, 2024: > > > > ** Proposed option: Wed Jul 10, 2024 ** > > (+9 months from Xen 4.18 release) > > Hmm, aren't we intending to be on a 8 month cadence? Considering that in July, there will be the Xen Developer Summit, we can aim for an 8-month cadence. However, in the Xen release document, there was mention of a discussion [1] about cadence: "With 18 months of full support and 36 months of security support, the number of concurrent supported releases will be the same with either 8 or 9 months release cycles, so I have chosen an 8-month cycle for now." I interpreted this as either an 8 or 9-month cycle, and it's not strict. If there's a strict requirement for a specific duration, I'll resend the Release Schedule Proposal. [1] https://lists.xen.org/archives/html/xen-devel/2018-07/msg02240.html ~ Oleksii
[xtf test] 184277: all pass - PUSHED
flight 184277 xtf real [real] http://logs.test-lab.xenproject.org/osstest/logs/184277/ Perfect :-) All tests in this flight passed as required version targeted for testing: xtf a5bd8d9e5d5c7b729d6d6122900d28f7a00aa6c0 baseline version: xtf 0a58a1471eb5f692700c0fd026ff96969b6ad7b0 Last test of basis 184247 2024-01-03 20:42:38 Z4 days Testing same since 184277 2024-01-08 11:42:46 Z0 days1 attempts People who touched revisions under test: Andrew Cooper Bernhard Kaindl jobs: build-amd64-xtf pass build-amd64 pass build-amd64-pvopspass test-xtf-amd64-amd64-1 pass test-xtf-amd64-amd64-2 pass test-xtf-amd64-amd64-3 pass test-xtf-amd64-amd64-4 pass test-xtf-amd64-amd64-5 pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Pushing revision : To xenbits.xen.org:/home/xen/git/xtf.git 0a58a14..a5bd8d9 a5bd8d9e5d5c7b729d6d6122900d28f7a00aa6c0 -> xen-tested-master
Re: [PATCH v2] NUMA: limit first_valid_mfn exposure
Hi Jan, On 08/01/2024 11:43, Jan Beulich wrote: On 08.01.2024 12:37, Julien Grall wrote: On 08/01/2024 11:31, Jan Beulich wrote: Address the TODO regarding first_valid_mfn by making the variable static when NUMA=y, thus also addressing a Misra C:2012 rule 8.4 concern (on x86). Signed-off-by: Jan Beulich --- Julien suggests something like STATIC_IF(CONFIG_NUMA) unsigned long first_valid_mfn; but I view this as non-scalable (or at least I can't see how to implement such in a scalabale way) and hence undesirable to introduce. I don't really see the scalability problem. Can you explain a bit more? Well, when seeing your original suggestion, I first considered it quite reasonable. But when thinking how to implement it, I couldn't see what #define STATIC_IF(cfg) should expand to. That's simply because a macro body cannot itself have pre-processor directives. Hence all I could think of was #ifdef CONFIG_NUMA # define static_if_CONFIG_NUMA static #else # define static_if_CONFIG_NUMA #endif #define STATIC_IF(cfg) static_if_ ## cfg And I think it is easy to see how this wouldn't scale across CONFIG_xyz. Plus that that point STATIC_IF() itself would be pretty much redundant. But maybe I'm simply overlooking the obvious ... You can use the same trick as for IS_ENABLED. The code below will select static or nothing: #define static_enabled(cfg) _static_enabled(cfg) #define _static_enabled(value) __static_enabled(__ARG_PLACEHOLDER_##value) #define __static_enabled(arg1_or_junk) ___static_enabled(arg1_or_junk static,) #define ___static_enabled(__ignored, val, ...) val #define STATIC_IF(option) static_enabled(option) I have tested both with CONFIG_NUMA and !CONFIG_NUMA to confirm the visibility of the variable will be correct. Cheers, -- Julien Grall
Re: [XEN RFC] x86/uaccess: remove __{put,get}_user_bad()
On 08/01/24 12:36, Jan Beulich wrote:
On 08.01.2024 12:16, Federico Serafini wrote:
On 08/01/24 09:02, Jan Beulich wrote:
On 05.01.2024 17:19, Federico Serafini wrote:
Hello everyone,
On 21/12/23 13:41, Jan Beulich wrote:
On 21.12.2023 13:01, Nicola Vetrini wrote:
Hi Andrew,
On 2023-12-21 12:03, Andrew Cooper wrote:
On 21/12/2023 10:58 am, Jan Beulich wrote:
On 21.12.2023 11:53, Federico Serafini wrote:
Remove declarations of __put_user_bad() and __get_user_bad()
since they have no definition.
Replace their uses with a break statement to address violations of
MISRA C:2012 Rule 16.3 ("An unconditional `break' statement shall
terminate every switch-clause").
No functional change.
Signed-off-by: Federico Serafini
---
Several violations of Rule 16.3 come from uses of macros
get_unsafe_size() and put_unsafe_size().
Looking at the macro definitions I found __get_user_bad() and
__put_user_bad().
I was wondering if instead of just adding the break statement I can
also remove
such functions which seem to not have a definition.
No, you can't. Try introducing a caller which "accidentally" uses the
wrong size. Without your change you'll observe the build failing (in
a somewhat obscure way, but still), while with your change bad code
will silently be generated.
The construct here is deliberate. It's a build time assertion that bad
sizes aren't used.
__bitop_bad_size() and __xsm_action_mismatch_detected() are the same
pattern in other areas of code too, with the latter being more explicit
because of how it's wrapped by LINKER_BUG_ON().
It is slightly horrible, and not the most obvious construct for
newcomers. If there's an alternative way to get a build assertion, we
could consider switching to a new pattern.
would you be in favour of a solution with a BUILD_BUG_ON in the default
branch followed by a break?
default:
BUILD_BUG_ON(!size || size >=8 || (size & (size - 1)));
break;
I don't think this would compile - BUILD_BUG_ON() wants a compile-time
constant passed.
What do you think about adding the following macro to compiler.h:
#define static_assert_unreachable(identifier) \
asm("unreachable " #identifier " reached")
It expands to an invalid assembly instruction that will lead to a
customizable error message generated by the assembler instead of the
linker (anticipating the error detection).
The use of this macro will indicate a program point considered
unreachable (and as such removed) by the static analysis performed by
the compiler, even at an optimization level -O0.
An example of use is in the default case of put_unsafe_size():
default: static_assert_unreachable(default);
In case a wrong size will be used, the following message will be
generated:
./arch/x86/include/asm/uaccess.h: Assembler messages:
./arch/x86/include/asm/uaccess.h:257: Error: no such instruction:
`unreachable default reached'
Nice idea. To take it one step further, why not simply use the .error
assembler directive then?
It seems good.
Note that adopting the macro and discussing its definition are two
separate things:
I think we can all agree on the fact that the use of such macro improves
readability, so I would suggest its adoption.
Whereas for its definition, if you don't like the invalid asm
instruction, we could discuss for a different solution, for example,
the following is something similar to what you are doing now:
#define static_assert_unreachable(identifier) \
extern void identifier(void); \
identifier()
Note also that the problem of the missing break statement (that violates
Rule 16.3) is still present, it could be addressed by adding the break
or deviating for such special cases, do you have any preferences?
Amend the new macro's expansion by unreachable()?
It would work only if we also add macro unreachable() to the allowed
statements that can terminate a switch-clause.
Isn't this, or something substantially similar, necessary anyway, to
avoid ...
I'll take this opportunity to clarify the Rule 16.3 and the deviation
system of ECLAIR for this rule (adding Julien in CC, he might be
interested in this).
The rationale of 16.3 is the avoidance of unintentional fall through.
To do this, the rule says to put an unconditional break statement at
the end of every switch-clause.
Nothing is said about the semantics of the code within the
switch-clause, e.g., the rule does not take into account if the fall
through cannot happen because the code returns in every feasible path.
The reason behind this is to keep the rule as simple as possible and
above all, keep the rule to be decidable.
such "break" then violating the "no unreachable code" rule?
For such cases of Rule 2.1 ("A project shall not contain unreachable
code.") there is already a deviation documented in deviations.rst:
"The compiler implementation guarantees that the unreachable code is
removed. Constant expressions and unreachable branches of if and switch
statements are
[ovmf test] 184276: all pass - PUSHED
flight 184276 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/184276/ Perfect :-) All tests in this flight passed as required version targeted for testing: ovmf e7152e6186d31bc138fbd2592e07886005177aab baseline version: ovmf c3d865a4c21d91f2e338a91d0c8b1eaf205575b5 Last test of basis 184273 2024-01-08 08:14:18 Z0 days Testing same since 184276 2024-01-08 10:43:33 Z0 days1 attempts People who touched revisions under test: Hou, Wenxing Wenxing Hou jobs: build-amd64-xsm pass build-i386-xsm pass build-amd64 pass build-i386 pass build-amd64-libvirt pass build-i386-libvirt pass build-amd64-pvopspass build-i386-pvops pass test-amd64-amd64-xl-qemuu-ovmf-amd64 pass test-amd64-i386-xl-qemuu-ovmf-amd64 pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Pushing revision : To xenbits.xen.org:/home/xen/git/osstest/ovmf.git c3d865a4c2..e7152e6186 e7152e6186d31bc138fbd2592e07886005177aab -> xen-tested-master
[xen-unstable-smoke test] 184274: tolerable all pass - PUSHED
flight 184274 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/184274/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass version targeted for testing: xen 2467ab2a74463107132f7d929d2ff0e30a8b6fa3 baseline version: xen 5a3ace21f3d779b291a2d305824b2820d88de7f1 Last test of basis 184260 2024-01-06 01:02:12 Z2 days Testing same since 184274 2024-01-08 10:05:30 Z0 days1 attempts People who touched revisions under test: Jan Beulich Juergen Gross Julien Grall Oleksii Kurochko Shawn Anastasio Tamas K Lengyel jobs: build-arm64-xsm pass build-amd64 pass build-armhf pass build-amd64-libvirt pass test-armhf-armhf-xl pass test-arm64-arm64-xl-xsm pass test-amd64-amd64-xl-qemuu-debianhvm-amd64pass test-amd64-amd64-libvirt pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Pushing revision : To xenbits.xen.org:/home/xen/git/xen.git 5a3ace21f3..2467ab2a74 2467ab2a74463107132f7d929d2ff0e30a8b6fa3 -> smoke
Re: [PATCH] hw/i386/pc_piix: Make piix_intx_routing_notifier_xen() more device independent
On 8/1/24 00:16, Bernhard Beschow wrote:
This is a follow-up on commit 89965db43cce "hw/isa/piix3: Avoid Xen-specific
variant of piix3_write_config()" which introduced
piix_intx_routing_notifier_xen(). This function is implemented in board code but
accesses the PCI configuration space of the PIIX ISA function to determine the
PCI interrupt routes. Avoid this by reusing pci_device_route_intx_to_irq() which
makes piix_intx_routing_notifier_xen() more device-agnostic.
One remaining improvement would be making piix_intx_routing_notifier_xen()
agnostic towards the number of PCI interrupt routes and move it to xen-hvm.
This might be useful for possible Q35 Xen efforts but remains a future exercise
for now.
Signed-off-by: Bernhard Beschow
---
hw/i386/pc_piix.c | 9 +++--
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 042c13cdbc..abfcfe4d2b 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -92,13 +92,10 @@ static void piix_intx_routing_notifier_xen(PCIDevice *dev)
{
int i;
-/* Scan for updates to PCI link routes (0x60-0x63). */
+/* Scan for updates to PCI link routes. */
for (i = 0; i < PIIX_NUM_PIRQS; i++) {
-uint8_t v = dev->config_read(dev, PIIX_PIRQCA + i, 1);
-if (v & 0x80) {
-v = 0;
-}
-v &= 0xf;
+const PCIINTxRoute route = pci_device_route_intx_to_irq(dev, i);
This indeed dispatch to piix_route_intx_pin_to_irq().
Reviewed-by: Philippe Mathieu-Daudé
+const uint8_t v = route.mode == PCI_INTX_ENABLED ? route.irq : 0;
xen_set_pci_link_route(i, v);
}
}
Re: [PATCH v2] xen/gntdev: Fix the abuse of underlying struct page in DMA-buf import
On Sun, 7 Jan 2024 at 11:35, Oleksandr Tyshchenko wrote:
>
> From: Oleksandr Tyshchenko
>
> DO NOT access the underlying struct page of an sg table exported
> by DMA-buf in dmabuf_imp_to_refs(), this is not allowed.
> Please see drivers/dma-buf/dma-buf.c:mangle_sg_table() for details.
>
> Fortunately, here (for special Xen device) we can avoid using
> pages and calculate gfns directly from dma addresses provided by
> the sg table.
>
> Suggested-by: Daniel Vetter
> Signed-off-by: Oleksandr Tyshchenko
> Acked-by: Christian König
> Reviewed-by: Stefano Stabellini
> ---
> Please note, I didn't manage to test the patch against the latest master
> branch
> on real HW (patch was only build tested there). Patch was tested on Arm64
> guests using Linux v5.10.41 from vendor's BSP, this is the environment where
> running this use-case is possible and to which I have an access (Xen PV
> display
> with zero-copy and backend domain as a buffer provider - be-alloc=1, so
> dma-buf
> import part was involved). A little bit old, but the dma-buf import code
> in gntdev-dmabuf.c hasn't been changed much since that time, all context
> remains allmost the same according to my code inspection.
>
> v2:
>- add R-b and A-b
>- fix build warning noticed by kernel test robot by initializing
> "ret" in case of error
>
> https://lore.kernel.org/oe-kbuild-all/202401062122.it6zvlg0-...@intel.com/
> ---
> ---
> drivers/xen/gntdev-dmabuf.c | 44 -
> 1 file changed, 19 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/xen/gntdev-dmabuf.c b/drivers/xen/gntdev-dmabuf.c
> index 4440e626b797..272c0ab01ef5 100644
> --- a/drivers/xen/gntdev-dmabuf.c
> +++ b/drivers/xen/gntdev-dmabuf.c
> @@ -11,6 +11,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -50,7 +51,7 @@ struct gntdev_dmabuf {
>
> /* Number of pages this buffer has. */
> int nr_pages;
> - /* Pages of this buffer. */
> + /* Pages of this buffer (only for dma-buf export). */
> struct page **pages;
> };
>
> @@ -484,7 +485,7 @@ static int dmabuf_exp_from_refs(struct gntdev_priv *priv,
> int flags,
> /* DMA buffer import support. */
>
> static int
> -dmabuf_imp_grant_foreign_access(struct page **pages, u32 *refs,
> +dmabuf_imp_grant_foreign_access(unsigned long *gfns, u32 *refs,
> int count, int domid)
> {
> grant_ref_t priv_gref_head;
> @@ -507,7 +508,7 @@ dmabuf_imp_grant_foreign_access(struct page **pages, u32
> *refs,
> }
>
> gnttab_grant_foreign_access_ref(cur_ref, domid,
> - xen_page_to_gfn(pages[i]), 0);
> + gfns[i], 0);
> refs[i] = cur_ref;
> }
>
> @@ -529,7 +530,6 @@ static void dmabuf_imp_end_foreign_access(u32 *refs, int
> count)
>
> static void dmabuf_imp_free_storage(struct gntdev_dmabuf *gntdev_dmabuf)
> {
> - kfree(gntdev_dmabuf->pages);
> kfree(gntdev_dmabuf->u.imp.refs);
> kfree(gntdev_dmabuf);
> }
> @@ -549,12 +549,6 @@ static struct gntdev_dmabuf
> *dmabuf_imp_alloc_storage(int count)
> if (!gntdev_dmabuf->u.imp.refs)
> goto fail;
>
> - gntdev_dmabuf->pages = kcalloc(count,
> - sizeof(gntdev_dmabuf->pages[0]),
> - GFP_KERNEL);
> - if (!gntdev_dmabuf->pages)
> - goto fail;
> -
> gntdev_dmabuf->nr_pages = count;
>
> for (i = 0; i < count; i++)
> @@ -576,7 +570,8 @@ dmabuf_imp_to_refs(struct gntdev_dmabuf_priv *priv,
> struct device *dev,
> struct dma_buf *dma_buf;
> struct dma_buf_attachment *attach;
> struct sg_table *sgt;
> - struct sg_page_iter sg_iter;
> + struct sg_dma_page_iter sg_iter;
> + unsigned long *gfns;
> int i;
>
> dma_buf = dma_buf_get(fd);
> @@ -624,26 +619,25 @@ dmabuf_imp_to_refs(struct gntdev_dmabuf_priv *priv,
> struct device *dev,
>
> gntdev_dmabuf->u.imp.sgt = sgt;
>
> - /* Now convert sgt to array of pages and check for page validity. */
> + gfns = kcalloc(count, sizeof(*gfns), GFP_KERNEL);
> + if (!gfns) {
> + ret = ERR_PTR(-ENOMEM);
> + goto fail_unmap;
> + }
> +
> + /* Now convert sgt to array of gfns without accessing underlying
> pages. */
> i = 0;
> - for_each_sgtable_page(sgt, _iter, 0) {
> - struct page *page = sg_page_iter_page(_iter);
> - /*
> -* Check if page is valid: this can happen if we are given
> -* a page from VRAM or other resources which are not backed
> -* by a struct page.
> -*/
> - if (!pfn_valid(page_to_pfn(page))) {
> - ret = ERR_PTR(-EINVAL);
> -
Re: [PATCH v5 03/13] xen/arm: add Dom0 cache coloring support
On Mon, Jan 8, 2024 at 12:44 PM Julien Grall wrote: > > > > On 08/01/2024 11:04, Carlo Nonato wrote: > > Hi Julien, > > > > On Mon, Jan 8, 2024 at 11:25 AM Julien Grall wrote: > >> > >> Hi Carlo, > >> > >> On 08/01/2024 10:06, Carlo Nonato wrote: > One of the reason is at least in the dom0less case, you will override > the value afterwards. > >>> > >>> In that case I need to allocate the array before parsing the string. > >>> I allocate a full array then the string is parsed and the actual size is > >>> found > >>> at the end of this phase. Knowing the actual size would require two > >>> parsing > >>> stages. Yes I'm wasting a bit of memory by oversizing the array here. Is > >>> it > >>> a problem? > >> > >> While wasting memory is indeed not nice. This wasn't the main reason of > >> this comment. > >> > >> The reason is that you seem to set d->num_lcc_colors will but will never > >> be read before it gets overwritten. Looking again at the code, you are > >> also assuming parse_colors() will always take an array of nr_colors. > > > > Ok, I think I understood, but that happens only in dom0less case because > > d->num_llc_colors is overwritten after parsing. In other cases it's ok to > > set > > it there. Anyway I can move the assignment out of the function if that is > > clearer. > > > >> It would be better if parse_colors() takes the maximum size of the array > >> in parameter. This would harden the code and it makes more sense for > >> domain_alloc_colors() to set d->num_lcc_colors. > > > > I don't understand this. parse_colors() must take only arrays of nr_colors > > size (the global, maximum number of colors), otherwise the parsed string > > config could exceed the array size. Since we don't know in advance the real > > size before parsing, I think it's better to pass only arrays that are > > already > > allocated with the maximum size. > > My concern is there is a disconnect. From the code, it is not obvious at > all that parse_colors() only want to accept an array of nr_colors. If > you pass an extra argument (or re-use the one you pass) for the array > size and use within the code, then it makes more obvious that your array > is always the correct size. > > At least to me, this is a good practice in C to always pass the array > and its size together (other language have that embedded). But I can > appreciate this is not view like that for everyone. The minimum would be > to document this requirement in a comment Ok got it. Thanks for the explanation. > > Doing as you said I would still pass nr_colors as the maximum size, but that > > would be strange since the global would still be accessible. > > I don't really see the problem here. Your code doesn't need to use the > global variable. > > > If domain_alloc_colors() setting d->num_llc_colors is so confusing, > > I will just move the assignment after the function call. > > > >> Also, I just noticed you have a global variable named nr_colors and the > >> function parse_colors() takes an argument called *num_colors. This is > >> quite confusing, can we have better name? > >> > >> Maybe rename nr_colors to nr_global_colors and and num_colors to > >> nr_array_colors? > > > > I agree with the fact that naming is confusing. I would opt for > > max_nr_colors > > for the global. > > I am fine with that. > > Cheers, > > -- > Julien Grall
Re: [PATCH v5 03/13] xen/arm: add Dom0 cache coloring support
On 08/01/2024 11:04, Carlo Nonato wrote: Hi Julien, On Mon, Jan 8, 2024 at 11:25 AM Julien Grall wrote: Hi Carlo, On 08/01/2024 10:06, Carlo Nonato wrote: One of the reason is at least in the dom0less case, you will override the value afterwards. In that case I need to allocate the array before parsing the string. I allocate a full array then the string is parsed and the actual size is found at the end of this phase. Knowing the actual size would require two parsing stages. Yes I'm wasting a bit of memory by oversizing the array here. Is it a problem? While wasting memory is indeed not nice. This wasn't the main reason of this comment. The reason is that you seem to set d->num_lcc_colors will but will never be read before it gets overwritten. Looking again at the code, you are also assuming parse_colors() will always take an array of nr_colors. Ok, I think I understood, but that happens only in dom0less case because d->num_llc_colors is overwritten after parsing. In other cases it's ok to set it there. Anyway I can move the assignment out of the function if that is clearer. It would be better if parse_colors() takes the maximum size of the array in parameter. This would harden the code and it makes more sense for domain_alloc_colors() to set d->num_lcc_colors. I don't understand this. parse_colors() must take only arrays of nr_colors size (the global, maximum number of colors), otherwise the parsed string config could exceed the array size. Since we don't know in advance the real size before parsing, I think it's better to pass only arrays that are already allocated with the maximum size. My concern is there is a disconnect. From the code, it is not obvious at all that parse_colors() only want to accept an array of nr_colors. If you pass an extra argument (or re-use the one you pass) for the array size and use within the code, then it makes more obvious that your array is always the correct size. At least to me, this is a good practice in C to always pass the array and its size together (other language have that embedded). But I can appreciate this is not view like that for everyone. The minimum would be to document this requirement in a comment Doing as you said I would still pass nr_colors as the maximum size, but that would be strange since the global would still be accessible. I don't really see the problem here. Your code doesn't need to use the global variable. If domain_alloc_colors() setting d->num_llc_colors is so confusing, I will just move the assignment after the function call. Also, I just noticed you have a global variable named nr_colors and the function parse_colors() takes an argument called *num_colors. This is quite confusing, can we have better name? Maybe rename nr_colors to nr_global_colors and and num_colors to nr_array_colors? I agree with the fact that naming is confusing. I would opt for max_nr_colors for the global. I am fine with that. Cheers, -- Julien Grall
Re: [PATCH v2] NUMA: limit first_valid_mfn exposure
On 08.01.2024 12:37, Julien Grall wrote: > On 08/01/2024 11:31, Jan Beulich wrote: >> Address the TODO regarding first_valid_mfn by making the variable static >> when NUMA=y, thus also addressing a Misra C:2012 rule 8.4 concern (on >> x86). >> >> Signed-off-by: Jan Beulich >> --- >> Julien suggests something like >> >> STATIC_IF(CONFIG_NUMA) unsigned long first_valid_mfn; >> >> but I view this as non-scalable (or at least I can't see how to >> implement such in a scalabale way) and hence undesirable to introduce. > > I don't really see the scalability problem. Can you explain a bit more? Well, when seeing your original suggestion, I first considered it quite reasonable. But when thinking how to implement it, I couldn't see what #define STATIC_IF(cfg) should expand to. That's simply because a macro body cannot itself have pre-processor directives. Hence all I could think of was #ifdef CONFIG_NUMA # define static_if_CONFIG_NUMA static #else # define static_if_CONFIG_NUMA #endif #define STATIC_IF(cfg) static_if_ ## cfg And I think it is easy to see how this wouldn't scale across CONFIG_xyz. Plus that that point STATIC_IF() itself would be pretty much redundant. But maybe I'm simply overlooking the obvious ... Jan
Re: [PATCH v2] NUMA: limit first_valid_mfn exposure
Hi Jan, On 08/01/2024 11:31, Jan Beulich wrote: Address the TODO regarding first_valid_mfn by making the variable static when NUMA=y, thus also addressing a Misra C:2012 rule 8.4 concern (on x86). Signed-off-by: Jan Beulich --- Julien suggests something like STATIC_IF(CONFIG_NUMA) unsigned long first_valid_mfn; but I view this as non-scalable (or at least I can't see how to implement such in a scalabale way) and hence undesirable to introduce. I don't really see the scalability problem. Can you explain a bit more? Cheers, -- Julien Grall
Re: [XEN RFC] x86/uaccess: remove __{put,get}_user_bad()
On 08.01.2024 12:16, Federico Serafini wrote:
> On 08/01/24 09:02, Jan Beulich wrote:
>> On 05.01.2024 17:19, Federico Serafini wrote:
>>> Hello everyone,
>>>
>>> On 21/12/23 13:41, Jan Beulich wrote:
On 21.12.2023 13:01, Nicola Vetrini wrote:
> Hi Andrew,
>
> On 2023-12-21 12:03, Andrew Cooper wrote:
>> On 21/12/2023 10:58 am, Jan Beulich wrote:
>>> On 21.12.2023 11:53, Federico Serafini wrote:
Remove declarations of __put_user_bad() and __get_user_bad()
since they have no definition.
Replace their uses with a break statement to address violations of
MISRA C:2012 Rule 16.3 ("An unconditional `break' statement shall
terminate every switch-clause").
No functional change.
Signed-off-by: Federico Serafini
---
Several violations of Rule 16.3 come from uses of macros
get_unsafe_size() and put_unsafe_size().
Looking at the macro definitions I found __get_user_bad() and
__put_user_bad().
I was wondering if instead of just adding the break statement I can
also remove
such functions which seem to not have a definition.
>>> No, you can't. Try introducing a caller which "accidentally" uses the
>>> wrong size. Without your change you'll observe the build failing (in
>>> a somewhat obscure way, but still), while with your change bad code
>>> will silently be generated.
>>
>> The construct here is deliberate. It's a build time assertion that bad
>> sizes aren't used.
>>
>> __bitop_bad_size() and __xsm_action_mismatch_detected() are the same
>> pattern in other areas of code too, with the latter being more explicit
>> because of how it's wrapped by LINKER_BUG_ON().
>>
>>
>> It is slightly horrible, and not the most obvious construct for
>> newcomers. If there's an alternative way to get a build assertion, we
>> could consider switching to a new pattern.
>
> would you be in favour of a solution with a BUILD_BUG_ON in the default
> branch followed by a break?
>
> default:
>BUILD_BUG_ON(!size || size >=8 || (size & (size - 1)));
>break;
I don't think this would compile - BUILD_BUG_ON() wants a compile-time
constant passed.
>>>
>>> What do you think about adding the following macro to compiler.h:
>>>
>>> #define static_assert_unreachable(identifier) \
>>> asm("unreachable " #identifier " reached")
>>>
>>> It expands to an invalid assembly instruction that will lead to a
>>> customizable error message generated by the assembler instead of the
>>> linker (anticipating the error detection).
>>>
>>> The use of this macro will indicate a program point considered
>>> unreachable (and as such removed) by the static analysis performed by
>>> the compiler, even at an optimization level -O0.
>>>
>>> An example of use is in the default case of put_unsafe_size():
>>>
>>> default: static_assert_unreachable(default);
>>>
>>> In case a wrong size will be used, the following message will be
>>> generated:
>>>
>>> ./arch/x86/include/asm/uaccess.h: Assembler messages:
>>> ./arch/x86/include/asm/uaccess.h:257: Error: no such instruction:
>>> `unreachable default reached'
>>
>> Nice idea. To take it one step further, why not simply use the .error
>> assembler directive then?
>
> It seems good.
>
>>
>>> Note that adopting the macro and discussing its definition are two
>>> separate things:
>>> I think we can all agree on the fact that the use of such macro improves
>>> readability, so I would suggest its adoption.
>>> Whereas for its definition, if you don't like the invalid asm
>>> instruction, we could discuss for a different solution, for example,
>>> the following is something similar to what you are doing now:
>>>
>>> #define static_assert_unreachable(identifier) \
>>> extern void identifier(void); \
>>> identifier()
>>>
>>>
>>> Note also that the problem of the missing break statement (that violates
>>> Rule 16.3) is still present, it could be addressed by adding the break
>>> or deviating for such special cases, do you have any preferences?
>>
>> Amend the new macro's expansion by unreachable()?
>
> It would work only if we also add macro unreachable() to the allowed
> statements that can terminate a switch-clause.
Isn't this, or something substantially similar, necessary anyway, to
avoid ...
> I'll take this opportunity to clarify the Rule 16.3 and the deviation
> system of ECLAIR for this rule (adding Julien in CC, he might be
> interested in this).
> The rationale of 16.3 is the avoidance of unintentional fall through.
> To do this, the rule says to put an unconditional break statement at
> the end of every switch-clause.
>
> Nothing is said about the semantics of the code within the
> switch-clause, e.g., the rule does not take into account if the fall
> through cannot happen because the code returns in every
Re: [PATCH v5 04/13] xen: extend domctl interface for cache coloring
Hi Carlo,
On 08/01/2024 11:19, Carlo Nonato wrote:
Hi Julien,
On Mon, Jan 8, 2024 at 12:01 PM Julien Grall wrote:
Hi Carlo,
On 08/01/2024 10:27, Carlo Nonato wrote:
On Fri, Jan 5, 2024 at 6:26 PM Julien Grall wrote:
On 02/01/2024 09:51, Carlo Nonato wrote:
This commit updates the domctl interface to allow the user to set cache
coloring configurations from the toolstack.
It also implements the functionality for arm64.
Based on original work from: Luca Miccio
Signed-off-by: Carlo Nonato
Signed-off-by: Marco Solieri
---
v5:
- added a new hypercall to set colors
- uint for the guest handle
v4:
- updated XEN_DOMCTL_INTERFACE_VERSION
---
xen/arch/arm/llc-coloring.c| 17 +
xen/common/domctl.c| 11 +++
xen/include/public/domctl.h| 10 +-
xen/include/xen/llc-coloring.h | 3 +++
4 files changed, 40 insertions(+), 1 deletion(-)
diff --git a/xen/arch/arm/llc-coloring.c b/xen/arch/arm/llc-coloring.c
index 5ce58aba70..a08614ec36 100644
--- a/xen/arch/arm/llc-coloring.c
+++ b/xen/arch/arm/llc-coloring.c
@@ -9,6 +9,7 @@
*Carlo Nonato
*/
#include
+#include
#include
#include
#include
@@ -278,6 +279,22 @@ int dom0_set_llc_colors(struct domain *d)
return domain_check_colors(d);
}
+int domain_set_llc_colors_domctl(struct domain *d,
+ const struct xen_domctl_set_llc_colors
*config)
+{
+if ( d->num_llc_colors )
+return -EEXIST;
+
+if ( domain_alloc_colors(d, config->num_llc_colors) )
domain_alloc_colors() doesn't sanity check config->num_llc_colors before
allocating the array. You want a check the size before so we would not
try to allocate an arbitrary amount of memory.
+return -ENOMEM;
+
+if ( copy_from_guest(d->llc_colors, config->llc_colors,
+ config->num_llc_colors) )
+return -EFAULT;
+
+return domain_check_colors(d);
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index f5a71ee5f7..b6867d0602 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -8,6 +8,7 @@
#include
#include
+#include
#include
#include
#include
@@ -858,6 +859,16 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t)
u_domctl)
__HYPERVISOR_domctl, "h", u_domctl);
break;
+case XEN_DOMCTL_set_llc_colors:
+if ( !llc_coloring_enabled )
+break;
+
+ret = domain_set_llc_colors_domctl(d, >u.set_llc_colors);
+if ( ret == -EEXIST )
+printk(XENLOG_ERR
+ "Can't set LLC colors on an already created domain\n");
To me, the message doesn't match the check in
domain_set_llc_colors_domctl(). But I think you want to check that no
memory was yet allocated to the domain. Otherwise, you coloring will be
wrong.
Also, it is a bit unclear why you print a message for -EEXIST but not
the others. In this instance, I would consider to print nothing at all.
The problem here is that we don't support recoloring. When a domain is
created it receives a coloring configuration and it can't change. If this
hypercall is called twice I have to stop the second time somehow.
Looking at your check what you prevent is a toolstack updating the array
twice. But that would be ok (/!\ I am not saying we should allow it) so
long no memory has been allocated to the domain.
But I also consider we would re-color once we started to allocate memory
for the domain (either RAM or P2M). This seems to be missed out in your
check.
So you want to be able to change colors if no memory has yet been allocated?
No. I am saying that that we should not be able to allow changing the
colors after the memory has been allocated. To give an example, your
current code would allow:
1) Setup the P2M pools or allocate RAM
2) Set the color
This would render the coloring configuration moot.
Whether we want to allow changing the coloring before hand is a
different question and as I wrote earlier on, I don't mind if you want
to forbid that.
I don't know what to check that.
You can check the size of the P2M pool (d->arch.paging.p2m_total_pages)
is still 0. I think for RAM, you can check d->tot_pages == 0.
Cheers,
--
Julien Grall
[PATCH v2] NUMA: limit first_valid_mfn exposure
Address the TODO regarding first_valid_mfn by making the variable static
when NUMA=y, thus also addressing a Misra C:2012 rule 8.4 concern (on
x86).
Signed-off-by: Jan Beulich
---
Julien suggests something like
STATIC_IF(CONFIG_NUMA) unsigned long first_valid_mfn;
but I view this as non-scalable (or at least I can't see how to
implement such in a scalabale way) and hence undesirable to introduce.
---
v2: New, split off.
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -254,12 +254,13 @@ static PAGE_LIST_HEAD(page_broken_list);
* BOOT-TIME ALLOCATOR
*/
+#ifndef CONFIG_NUMA
/*
* first_valid_mfn is exported because it is used when !CONFIG_NUMA.
- *
- * TODO: Consider if we can conditionally export first_valid_mfn based
- * on whether NUMA is selected.
*/
+#else
+static
+#endif
mfn_t first_valid_mfn = INVALID_MFN_INITIALIZER;
struct bootmem_region {
[PATCH v2] NUMA: no need for asm/numa.h when !NUMA
There's no point in every architecture carrying the same stubs for the
case when NUMA isn't enabled (or even supported). Move all of that to
xen/numa.h; replace explicit uses of asm/numa.h in common code. Make
inclusion of asm/numa.h dependent upon NUMA=y.
Drop the no longer applicable "implement NUMA support" comments - in a
!NUMA section this simply makes no sense.
Signed-off-by: Jan Beulich
---
v2: Split off first_valid_mfn specific change. Re-base.
--- a/xen/arch/arm/include/asm/numa.h
+++ /dev/null
@@ -1,31 +0,0 @@
-#ifndef __ARCH_ARM_NUMA_H
-#define __ARCH_ARM_NUMA_H
-
-#include
-
-typedef u8 nodeid_t;
-
-#ifndef CONFIG_NUMA
-
-/* Fake one node for now. See also node_online_map. */
-#define cpu_to_node(cpu) 0
-#define node_to_cpumask(node) (cpu_online_map)
-
-/* XXX: implement NUMA support */
-#define node_spanned_pages(nid) (max_page - mfn_x(first_valid_mfn))
-#define node_start_pfn(nid) (mfn_x(first_valid_mfn))
-#define __node_distance(a, b) (20)
-
-#endif
-
-#define arch_want_default_dmazone() (false)
-
-#endif /* __ARCH_ARM_NUMA_H */
-/*
- * Local variables:
- * mode: C
- * c-file-style: "BSD"
- * c-basic-offset: 4
- * indent-tabs-mode: nil
- * End:
- */
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -42,7 +42,7 @@ integer_param("maxcpus", max_cpus);
/* CPU logical map: map xen cpuid to an MPIDR */
register_t __cpu_logical_map[NR_CPUS] = { [0 ... NR_CPUS-1] = MPIDR_INVALID };
-/* Fake one node for now. See also asm/numa.h */
+/* Fake one node for now. See also xen/numa.h */
nodemask_t __read_mostly node_online_map = { { [0] = 1UL } };
/* Xen stack for bringing up the first CPU. */
--- a/xen/arch/ppc/include/asm/numa.h
+++ /dev/null
@@ -1,20 +0,0 @@
-#ifndef __ASM_PPC_NUMA_H__
-#define __ASM_PPC_NUMA_H__
-
-#include
-#include
-
-typedef uint8_t nodeid_t;
-
-/* Fake one node for now. See also node_online_map. */
-#define cpu_to_node(cpu) 0
-#define node_to_cpumask(node) (cpu_online_map)
-
-/* XXX: implement NUMA support */
-#define node_spanned_pages(nid) (max_page - mfn_x(first_valid_mfn))
-#define node_start_pfn(nid) (mfn_x(first_valid_mfn))
-#define __node_distance(a, b) (20)
-
-#define arch_want_default_dmazone() (false)
-
-#endif /* __ASM_PPC_NUMA_H__ */
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -138,7 +138,6 @@
#include
#include
-#include
#include
#include
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -22,8 +22,8 @@
#include
#include
#include
-#include
#include
+#include
#include
#include
#include
--- a/xen/include/xen/domain.h
+++ b/xen/include/xen/domain.h
@@ -2,6 +2,7 @@
#ifndef __XEN_DOMAIN_H__
#define __XEN_DOMAIN_H__
+#include
#include
#include
@@ -12,7 +13,6 @@ struct guest_area {
};
#include
-#include
typedef union {
struct vcpu_guest_context *nat;
--- a/xen/include/xen/numa.h
+++ b/xen/include/xen/numa.h
@@ -2,7 +2,13 @@
#define _XEN_NUMA_H
#include
+
+#ifdef CONFIG_NUMA
+#include
#include
+#else
+typedef uint8_t nodeid_t;
+#endif
#define NUMA_NO_NODE 0xFF
#define NUMA_NO_DISTANCE 0xFF
@@ -108,8 +114,18 @@ extern void numa_set_processor_nodes_par
#else
+/* Fake one node for now. See also node_online_map. */
+#define cpu_to_node(cpu) 0
+#define node_to_cpumask(node) cpu_online_map
+
+#define arch_want_default_dmazone() false
+
extern mfn_t first_valid_mfn;
+#define node_spanned_pages(nid) (max_page - mfn_x(first_valid_mfn))
+#define node_start_pfn(nid) mfn_x(first_valid_mfn)
+#define __node_distance(a, b) 20
+
static inline nodeid_t mfn_to_nid(mfn_t mfn)
{
return 0;
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -11,10 +11,10 @@
#include
#include
#include
+#include
#include
#include
#include
-#include
/*
* The PCI interface treats multi-function devices as independent
Re: [PATCH v5 11/13] Revert "xen/arm: Remove unused BOOT_RELOC_VIRT_START"
Hi Julien, On Fri, Jan 5, 2024 at 7:20 PM Julien Grall wrote: > > Hi Carlo, > > On 02/01/2024 09:51, Carlo Nonato wrote: > > This reverts commit 0c18fb76323bfb13615b6f13c98767face2d8097 (not clean). > > > > This is not a clean revert since the rework of the memory layout, but it is > > sufficiently similar to a clean one. > > The only difference is that the BOOT_RELOC_VIRT_START must match the new > > layout. > > > > Cache coloring support for Xen needs to relocate Xen code and data in a new > > colored physical space. The BOOT_RELOC_VIRT_START will be used as the > > virtual > > base address for a temporary mapping to this new space. > > > > Signed-off-by: Carlo Nonato > > Signed-off-by: Marco Solieri > > --- > > xen/arch/arm/include/asm/mmu/layout.h | 2 ++ > > xen/arch/arm/mmu/setup.c | 1 + > > 2 files changed, 3 insertions(+) > > > > diff --git a/xen/arch/arm/include/asm/mmu/layout.h > > b/xen/arch/arm/include/asm/mmu/layout.h > > index eac7eef885..30031f74d9 100644 > > --- a/xen/arch/arm/include/asm/mmu/layout.h > > +++ b/xen/arch/arm/include/asm/mmu/layout.h > > @@ -74,6 +74,8 @@ > > #define BOOT_FDT_VIRT_START (FIXMAP_VIRT_START + FIXMAP_VIRT_SIZE) > > #define BOOT_FDT_VIRT_SIZE _AT(vaddr_t, MB(4)) > > > > +#define BOOT_RELOC_VIRT_START (BOOT_FDT_VIRT_START + BOOT_FDT_VIRT_SIZE) > > This new addition wants to be documented in the layout comment in a few > lines above. Also, the area you are using is 2MB whereas Xen can now be > up to 8MB. > > Secondly, you want to add a BOOTRELOC_VIRT_SIZE with a check in > build_assertions() making sure that this is at least as big as > XEN_VIRT_SIZE. > > Overall, I am not sure this is really a revert at this point. The idea > is the same, but you are defining BOOT_FDT_VIRT_START differently. > > To me it feels like it belong to the first patch where you will use it. > And that would be ok to mention in the commit message that the idea was > borrowed from a previously reverted commit. Ok, nice. > > + > > #ifdef CONFIG_LIVEPATCH > > #define LIVEPATCH_VMAP_START(BOOT_FDT_VIRT_START + BOOT_FDT_VIRT_SIZE) > > #define LIVEPATCH_VMAP_SIZE_AT(vaddr_t, MB(2)) > > diff --git a/xen/arch/arm/mmu/setup.c b/xen/arch/arm/mmu/setup.c > > index d5264e51bc..37b6d230ad 100644 > > --- a/xen/arch/arm/mmu/setup.c > > +++ b/xen/arch/arm/mmu/setup.c > > @@ -69,6 +69,7 @@ static void __init __maybe_unused build_assertions(void) > > /* 2MB aligned regions */ > > BUILD_BUG_ON(XEN_VIRT_START & ~SECOND_MASK); > > BUILD_BUG_ON(FIXMAP_ADDR(0) & ~SECOND_MASK); > > +BUILD_BUG_ON(BOOT_RELOC_VIRT_START & ~SECOND_MASK); > > /* 1GB aligned regions */ > > #ifdef CONFIG_ARM_32 > > BUILD_BUG_ON(XENHEAP_VIRT_START & ~FIRST_MASK); > > -- > Julien Grall Thanks.
