Re: [PATCH v2 11/17] xen/hypfs: add getsize() and findentry() callbacks to hypfs_funcs
On 02.12.2020 16:51, Jürgen Groß wrote: > On 02.12.20 16:42, Jan Beulich wrote: >> On 01.12.2020 09:21, Juergen Gross wrote: >>> Add a getsize() function pointer to struct hypfs_funcs for being able >>> to have dynamically filled entries without the need to take the hypfs >>> lock each time the contents are being generated. >>> >>> For directories add a findentry callback to the vector and modify >>> hypfs_get_entry_rel() to use it. >>> >>> Signed-off-by: Juergen Gross >> >> Reviewed-by: Jan Beulich >> with maybe one further small adjustment: >> >>> @@ -176,15 +188,41 @@ static int hypfs_get_path_user(char *buf, >>> return 0; >>> } >>> >>> +struct hypfs_entry *hypfs_leaf_findentry(const struct hypfs_entry_dir *dir, >>> + const char *name, >>> + unsigned int name_len) >>> +{ >>> +return ERR_PTR(-ENOENT); >>> +} >> >> ENOENT seems odd to me here. There looks to be no counterpart to >> EISDIR, so maybe ENODATA, EACCES, or EPERM? > > Hmm, why? > > In case I have /a/b and I'm looking for /a/b/c ENOENT seems to be just > fine? > > Or I could add ENOTDIR. Oh, there actually is supposed to be such an entry, but public/errno.h is simply missing it. Yes - ENOTDIR is what I was thinking of when saying "there looks to be no counterpart to EISDIR". Jan
Re: [PATCH] x86/IRQ: bump max number of guests for a shared IRQ to 31
On 02.12.2020 17:34, Igor Druzhinin wrote: > On 02/12/2020 15:21, Jan Beulich wrote: >> On 02.12.2020 15:53, Igor Druzhinin wrote: >>> On 02/12/2020 09:25, Jan Beulich wrote: Instead I'm wondering whether this wouldn't better be a Kconfig setting (or even command line controllable). There don't look to be any restrictions on the precise value chosen (i.e. 2**n-1 like is the case for old and new values here, for whatever reason), so a simple permitted range of like 4...64 would seem fine to specify. Whether the default then would want to be 8 (close to the current 7) or higher (around the actually observed maximum) is a different question. >>> >>> I'm in favor of a command line argument here - it would be much less trouble >>> if a higher limit was suddenly necessary in the field. The default IMO >>> should definitely be higher than 8 - I'd stick with number 32 which to me >>> should cover our real world scenarios and apply some headroom for the >>> future. >> >> Well, I'm concerned of the extra memory overhead. Every IRQ, >> sharable or not, will get the extra slots allocated with the >> current scheme. Perhaps a prereq change then would be to only >> allocate multi-guest arrays for sharable IRQs, effectively >> shrinking the overhead in particular for all MSI ones? > > That's one way to improve overall system scalability but in that area > there is certainly much bigger fish to fry elsewhere. With 32 elements in the > array we get 200 bytes of overhead per structure, with 16 it's just 72 extra > bytes which in the unattainable worst case scenario of every single vector > taken > in 512 CPU machine would only account for several MB of overhead. I'm generally unhappy with this way of thinking, as this is what has been leading to unnecessary growth of all sorts of software and its needs of resources. Yes, there surely are larger gains to be had elsewhere, but that's imo still no excuse to grow memory allocations "blindly" despite it being clear that in a fair share of cases a fair part of the allocated memory won't be used. This said, ... > I'd start with dynamic array allocation first and setting the limit to 16 that > should be enough for now. And then if that default value needs to be raised > we can consider further improvements. ... I'm puzzled by this plan of yours, because unless I'm misunderstanding dynamic array allocation is what I've been asking for, effectively. Now that we have xmalloc_flex_struct(), this should even be relatively straightforward, i.e. in particular with no need to open code complex expressions. Jan
Re: [PATCH v2 1/2] x86/IRQ: make max number of guests for a shared IRQ configurable
On 03.12.2020 02:58, Igor Druzhinin wrote: > --- a/docs/misc/xen-command-line.pandoc > +++ b/docs/misc/xen-command-line.pandoc > @@ -1641,6 +1641,15 @@ This option is ignored in **pv-shim** mode. > ### nr_irqs (x86) > > `= ` > > +### irq_max_guests (x86) As a rule of thumb, new options want to use - instead of _ as separators. There was a respective discussion quite some time ago. My patch to treat - and _ equally when parsing options wasn't liked by Andrew ... > @@ -435,6 +439,9 @@ int __init init_irq_data(void) > for ( ; irq < nr_irqs; irq++ ) > irq_to_desc(irq)->irq = irq; > > +if ( !irq_max_guests || irq_max_guests > 255) The 255 is a consequence of the struct field being u8, aiui? There should be a direct connection between the two, i.e. when changing the underlying property preferably only one place should require touching (possible e.g. by converting to a bit field with its width established via a #define), or comments on either side should point at the other one. Also as a nit, there's a blank missing ahead of the closing paren. > @@ -1564,7 +1570,8 @@ int pirq_guest_bind(struct vcpu *v, struct pirq *pirq, > int will_share) > if ( newaction == NULL ) > { > spin_unlock_irq(&desc->lock); > -if ( (newaction = xmalloc(irq_guest_action_t)) != NULL && > +if ( (newaction = xmalloc_bytes(sizeof(irq_guest_action_t) + > + irq_max_guests * sizeof(action->guest[0]))) != NULL && As said in the (later) reply to v1, please try to make use of xmalloc_flex_struct() here. > @@ -1633,11 +1640,11 @@ int pirq_guest_bind(struct vcpu *v, struct pirq > *pirq, int will_share) > goto retry; > } > > -if ( action->nr_guests == IRQ_MAX_GUESTS ) > +if ( action->nr_guests == irq_max_guests ) > { > printk(XENLOG_G_INFO "Cannot bind IRQ%d to dom%d. " > - "Already at max share.\n", > - pirq->pirq, v->domain->domain_id); > + "Already at max share %u, increase with irq_max_guests= > option.\n", > + pirq->pirq, v->domain->domain_id, irq_max_guests); If you already need to largely redo this printk(), please - switch to %pd at the same time, - don't have the format string extend across multiple lines, - preferably avoid to use full stops (we don't use any in the vast majority of log messages), e.g. by making it "cannot bind IRQ%d to %pd, already at max share %u (increase with irq-max-guests= option)", if you want to stay close to the original wording (I think this could be expressed more compactly as well). Jan
Re: [PATCH v2 09/17] xen/hypfs: move per-node function pointers into a dedicated struct
On 02.12.20 16:36, Jan Beulich wrote: On 01.12.2020 09:21, Juergen Gross wrote: static int hypfs_write(struct hypfs_entry *entry, XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned long ulen) As a tangent, is there a reason these write functions don't take handles of "const void"? (I realize this likely is nothing that wants addressing right here.) Uh, this is harder than I thought. guest_handle_cast() doesn't handle const guest handle types currently: hypfs.c:447:58: error: unknown type name ‘const_void’; did you mean ‘const’? ret = hypfs_write(entry, guest_handle_cast(arg3, const_void), arg4); ^ /home/gross/xen/unstable/xen/include/xen/guest_access.h:26:5: note: in definition of macro ‘guest_handle_cast’ type *_x = (hnd).p; \ ^~~~ Currently my ideas would be to either: - add a new macro for constifying a guest handle (type -> const_type) - add a new macro for casting a guest handle to a const_type - add typedefs for the const_* types (typedef const x const_x) - open code the cast Or am I missing an existing variant? Juergen OpenPGP_0xB0DE9DD628BF132F.asc Description: application/pgp-keys OpenPGP_signature Description: OpenPGP digital signature
Re: [PATCH v2 2/2] x86/IRQ: allocate guest array of max size only for shareable IRQs
On 03.12.2020 02:58, Igor Druzhinin wrote: > @@ -1540,6 +1540,7 @@ int pirq_guest_bind(struct vcpu *v, struct pirq *pirq, > int will_share) > unsigned intirq; > struct irq_desc *desc; > irq_guest_action_t *action, *newaction = NULL; > +unsigned intmax_nr_guests = will_share ? irq_max_guests : 1; > int rc = 0; > > WARN_ON(!spin_is_locked(&v->domain->event_lock)); > @@ -1571,7 +1572,7 @@ int pirq_guest_bind(struct vcpu *v, struct pirq *pirq, > int will_share) > { > spin_unlock_irq(&desc->lock); > if ( (newaction = xmalloc_bytes(sizeof(irq_guest_action_t) + > - irq_max_guests * sizeof(action->guest[0]))) != NULL && > + max_nr_guests * sizeof(action->guest[0]))) != NULL && > zalloc_cpumask_var(&newaction->cpu_eoi_map) ) > goto retry; > xfree(newaction); > @@ -1640,7 +1641,7 @@ int pirq_guest_bind(struct vcpu *v, struct pirq *pirq, > int will_share) > goto retry; > } > > -if ( action->nr_guests == irq_max_guests ) > +if ( action->nr_guests == max_nr_guests ) > { > printk(XENLOG_G_INFO "Cannot bind IRQ%d to dom%d. " > "Already at max share %u, increase with irq_max_guests= > option.\n", Just as a minor remark - I don't think this last hunk is necessary, as the !will_share case won't make it here unless action->nr_guests is still zero. At which point the need for the new local variable would also disappear. But I'm not going to insist, as there may be the view that the code is more clear this way. However, if clarity (in the sense of "obviously correct") is the goal, then I think using >= instead of == would now become preferable. Jan
Re: [PATCH v2 1/2] x86/IRQ: make max number of guests for a shared IRQ configurable
On 03.12.2020 02:58, Igor Druzhinin wrote: > --- a/docs/misc/xen-command-line.pandoc > +++ b/docs/misc/xen-command-line.pandoc > @@ -1641,6 +1641,15 @@ This option is ignored in **pv-shim** mode. > ### nr_irqs (x86) > > `= ` > > +### irq_max_guests (x86) > +> `= ` > + > +> Default: `16` > + > +Maximum number of guests IRQ could be shared between, i.e. a limit on > +the number of guests it is possible to start each having assigned a device > +sharing a common interrupt line. Accepts values between 1 and 255. Reading through this again, could "IRQ" be expanded to "any individual IRQ" or some such? Jan
[linux-linus test] 157164: regressions - trouble: broken/fail/pass
flight 157164 linux-linus real [real] http://logs.test-lab.xenproject.org/osstest/logs/157164/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-arm64-arm64-xl-xsm broken test-amd64-i386-qemut-rhel6hvm-intel 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-qemuu-ws16-amd64 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-xsm7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-qemut-debianhvm-amd64 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-qemuu-debianhvm-amd64-shadow 7 xen-install fail REGR. vs. 152332 test-amd64-i386-qemuu-rhel6hvm-intel 7 xen-install fail REGR. vs. 152332 test-amd64-i386-libvirt 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-qemuu-debianhvm-i386-xsm 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-qemut-ws16-amd64 7 xen-install fail REGR. vs. 152332 test-amd64-coresched-i386-xl 7 xen-install fail REGR. vs. 152332 test-amd64-i386-qemut-rhel6hvm-amd 7 xen-installfail REGR. vs. 152332 test-amd64-i386-qemuu-rhel6hvm-amd 7 xen-installfail REGR. vs. 152332 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 7 xen-install fail REGR. vs. 152332 test-amd64-i386-libvirt-xsm 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-qemuu-debianhvm-amd64 7 xen-install fail REGR. vs. 152332 test-amd64-i386-pair 10 xen-install/src_host fail REGR. vs. 152332 test-amd64-i386-pair 11 xen-install/dst_host fail REGR. vs. 152332 test-amd64-i386-examine 6 xen-install fail REGR. vs. 152332 test-amd64-i386-freebsd10-amd64 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-raw7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-pvshim 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-qemut-debianhvm-i386-xsm 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-shadow 7 xen-install fail REGR. vs. 152332 test-amd64-i386-freebsd10-i386 7 xen-installfail REGR. vs. 152332 test-amd64-i386-xl-qemuu-ovmf-amd64 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-qemut-win7-amd64 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-qemuu-win7-amd64 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-qemut-stubdom-debianhvm-amd64-xsm 7 xen-install fail REGR. vs. 152332 test-amd64-i386-libvirt-pair 10 xen-install/src_host fail REGR. vs. 152332 test-amd64-i386-libvirt-pair 11 xen-install/dst_host fail REGR. vs. 152332 test-arm64-arm64-xl 8 xen-boot fail REGR. vs. 152332 test-arm64-arm64-xl-credit1 8 xen-boot fail REGR. vs. 152332 test-arm64-arm64-examine 13 examine-iommufail REGR. vs. 152332 test-amd64-amd64-amd64-pvgrub 20 guest-stop fail REGR. vs. 152332 test-amd64-amd64-i386-pvgrub 20 guest-stop fail REGR. vs. 152332 test-amd64-amd64-examine 4 memdisk-try-append fail REGR. vs. 152332 test-amd64-i386-xl-qemuu-dmrestrict-amd64-dmrestrict 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl7 xen-install fail REGR. vs. 152332 test-arm64-arm64-xl-credit2 8 xen-boot fail REGR. vs. 152332 test-arm64-arm64-xl-seattle 8 xen-boot fail REGR. vs. 152332 Tests which did not succeed, but are not blocking: test-arm64-arm64-xl-xsm 5 host-install(5) broken blocked in 152332 test-arm64-arm64-libvirt-xsm 11 leak-check/basis(11)fail blocked in 152332 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 152332 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 152332 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 152332 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 152332 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 152332 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 152332 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 152332 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-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail 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-armhf-armhf-xl-cubietruck 15 migrate-support-checkfail never pass
Re: [PATCH v2 09/17] xen/hypfs: move per-node function pointers into a dedicated struct
On 03.12.2020 09:47, Jürgen Groß wrote: > On 02.12.20 16:36, Jan Beulich wrote: >> On 01.12.2020 09:21, Juergen Gross wrote: >>> static int hypfs_write(struct hypfs_entry *entry, >>> XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned long >>> ulen) >> >> As a tangent, is there a reason these write functions don't take >> handles of "const void"? (I realize this likely is nothing that >> wants addressing right here.) > > Uh, this is harder than I thought. > > guest_handle_cast() doesn't handle const guest handle types currently: > > hypfs.c:447:58: error: unknown type name ‘const_void’; did you mean ‘const’? > ret = hypfs_write(entry, guest_handle_cast(arg3, const_void), > arg4); >^ > /home/gross/xen/unstable/xen/include/xen/guest_access.h:26:5: note: in > definition of macro ‘guest_handle_cast’ > type *_x = (hnd).p; \ > ^~~~ > > Currently my ideas would be to either: > > - add a new macro for constifying a guest handle (type -> const_type) > - add a new macro for casting a guest handle to a const_type > - add typedefs for the const_* types (typedef const x const_x) > - open code the cast > > Or am I missing an existing variant? I don't think you are. Both of your first two suggestions look good to me - ultimately we may want to have both anyway, eventually. For its (presumed) type safety I may have a slight preference for option 1, albeit afaics guest_handle_cast() doesn't allow conversion between arbitrary types either (only to/from void). It's quite unfortunate that this requires an explicit cast in the first place, but what do you do. Jan
[libvirt test] 157171: regressions - FAIL
flight 157171 libvirt real [real] http://logs.test-lab.xenproject.org/osstest/logs/157171/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-armhf-libvirt 6 libvirt-buildfail REGR. vs. 151777 build-amd64-libvirt 6 libvirt-buildfail REGR. vs. 151777 build-i386-libvirt6 libvirt-buildfail REGR. vs. 151777 build-arm64-libvirt 6 libvirt-buildfail REGR. vs. 151777 Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-pair 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-vhd 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-xsm 1 build-check(1) blocked n/a test-amd64-i386-libvirt 1 build-check(1) blocked n/a test-amd64-i386-libvirt-pair 1 build-check(1) blocked n/a test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 1 build-check(1) blocked n/a test-amd64-i386-libvirt-xsm 1 build-check(1) blocked n/a test-arm64-arm64-libvirt 1 build-check(1) blocked n/a test-arm64-arm64-libvirt-qcow2 1 build-check(1) blocked n/a test-arm64-arm64-libvirt-xsm 1 build-check(1) blocked n/a test-armhf-armhf-libvirt 1 build-check(1) blocked n/a test-armhf-armhf-libvirt-raw 1 build-check(1) blocked n/a version targeted for testing: libvirt 5f6a7618993b31c1ded6e5ad650a607a5c93f6e2 baseline version: libvirt 2c846fa6bcc11929c9fb857a22430fb9945654ad Last test of basis 151777 2020-07-10 04:19:19 Z 146 days Failing since151818 2020-07-11 04:18:52 Z 145 days 140 attempts Testing same since 157171 2020-12-03 04:19:14 Z0 days1 attempts People who touched revisions under test: Adolfo Jayme Barrientos Aleksandr Alekseev Andika Triwidada Andrea Bolognani Balázs Meskó Barrett Schonefeld Bastien Orivel Bihong Yu Binfeng Wu Boris Fiuczynski Brian Turek Christian Ehrhardt Christian Schoenebeck Cole Robinson Collin Walling Cornelia Huck Côme Borsoi Daniel Henrique Barboza Daniel Letai Daniel P. Berrange Daniel P. Berrangé Erik Skultety Fabian Freyer Fangge Jin Fedora Weblate Translation Guoyi Tu Göran Uddeborg Halil Pasic Han Han Hao Wang Ian Wienand Jamie Strandboge Jamie Strandboge Jean-Baptiste Holcroft Jianan Gao Jim Fehlig Jin Yan Jiri Denemark John Ferlan Jonathan Watt Jonathon Jongsma Julio Faracco Ján Tomko Kashyap Chamarthy Kevin Locke Laine Stump Liao Pingfang Lin Ma Lin Ma Lin Ma Marc Hartmayer Marc-André Lureau Marek Marczykowski-Górecki Markus Schade Martin Kletzander Masayoshi Mizuma Matt Coleman Matt Coleman Mauro Matteo Cascella Michal Privoznik Michał Smyk Milo Casagrande Neal Gompa Nico Pache Nikolay Shirokovskiy Olaf Hering Olesya Gerasimenko Orion Poplawski Patrick Magauran Paulo de Rezende Pinatti Pavel Hrdina Peter Krempa Pino Toscano Pino Toscano Piotr Drąg Prathamesh Chavan Ricky Tigg Roman Bogorodskiy Roman Bolshakov Ryan Gahagan Ryan Schmidt Sam Hartman Scott Shambarger Sebastian Mitterle Shaojun Yang Simon Gaiser Stefan Bader Stefan Berger Szymon Scholz Thomas Huth Tim Wiederhake Tomáš Golembiovský Tuguoyi Wang Xin Weblate Yang Hang Yanqiu Zhang Yi Li Yi Wang Yuri Chornoivan Zheng Chuan zhenwei pi Zhenyu Zheng 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 build-amd64-libvirt fail build-arm64-libvirt fail build-armhf-libvirt fail build-i386-libvirt fail build-amd64-pvopspass build-arm64-pvopspass build-armhf-pvopspass build-i386-pvops pass test-amd64-amd6
Re: [PATCH 1/2] include: don't use asm/page.h from common headers
On 02.12.2020 18:14, Julien Grall wrote: > Hi Jan, > > On 02/12/2020 14:49, Jan Beulich wrote: >> Doing so limits what can be done in (in particular included by) this per- >> arch header. Abstract out page shift/size related #define-s, which is all >> the repsecitve headers care about. Extend the replacement / removal to > > s/repsecitve/respective/ > >> some x86 headers as well; some others now need to include page.h (and >> they really should have before). >> >> Arm's VADDR_BITS gets restricted to 32-bit, as its current value is >> clearly wrong for 64-bit, but the constant also isn't used anywhere >> right now (i.e. the #define could also be dropped altogether). > > Whoops. Thankfully this is not used. > >> >> I wasn't sure about Arm's use of vaddr_t in PAGE_OFFSET(), and hence I >> kept it and provided a way to override the #define in the common header. > > vaddr_t is defined to 32-bit for arm32 or 64-bit for arm64. So I think > it would be fine to use the generic PAGE_OFFSET() implementation. Will switch. >> --- /dev/null >> +++ b/xen/include/asm-arm/page-shift.h > > The name of the file looks a bit odd given that *_BITS are also defined > in it. So how about renaming to page-size.h? I was initially meaning to use that name, but these headers specifically don't define any sizes - *_BITS are still shift values, at least in a way. If the current name isn't liked, my next best suggestion would then be page-bits.h. >> @@ -0,0 +1,15 @@ >> +#ifndef __ARM_PAGE_SHIFT_H__ >> +#define __ARM_PAGE_SHIFT_H__ >> + >> +#define PAGE_SHIFT 12 >> + >> +#define PAGE_OFFSET(ptr)((vaddr_t)(ptr) & ~PAGE_MASK) >> + >> +#ifdef CONFIG_ARM_64 >> +#define PADDR_BITS 48 > > Shouldn't we define VADDR_BITS here? See the description - it's unused anyway. I'm fine any of the three possible ways: 1) keep as is in v1 2) drop altogether 3) also #define for 64-bit (but then you need to tell me whether 64 is the right value to use, or what the correct one would be) > But I wonder whether VADDR_BITS > should be defined as sizeof(vaddr_t) * 8. > > This would require to include asm/types.h. Which I'd specifically like to avoid. Plus use of sizeof() also precludes the use of respective #define-s in #if-s. >> --- a/xen/include/asm-x86/desc.h >> +++ b/xen/include/asm-x86/desc.h >> @@ -1,6 +1,8 @@ >> #ifndef __ARCH_DESC_H >> #define __ARCH_DESC_H >> >> +#include > > May I ask why you are including and not here? Because of DECLARE_PER_CPU(l1_pgentry_t, gdt_l1e); and DECLARE_PER_CPU(l1_pgentry_t, compat_gdt_l1e); at least (didn't check further). Jan
Re: [PATCH v2 11/17] xen/hypfs: add getsize() and findentry() callbacks to hypfs_funcs
On 03.12.20 09:12, Jan Beulich wrote: On 02.12.2020 16:51, Jürgen Groß wrote: On 02.12.20 16:42, Jan Beulich wrote: On 01.12.2020 09:21, Juergen Gross wrote: Add a getsize() function pointer to struct hypfs_funcs for being able to have dynamically filled entries without the need to take the hypfs lock each time the contents are being generated. For directories add a findentry callback to the vector and modify hypfs_get_entry_rel() to use it. Signed-off-by: Juergen Gross Reviewed-by: Jan Beulich with maybe one further small adjustment: @@ -176,15 +188,41 @@ static int hypfs_get_path_user(char *buf, return 0; } +struct hypfs_entry *hypfs_leaf_findentry(const struct hypfs_entry_dir *dir, + const char *name, + unsigned int name_len) +{ +return ERR_PTR(-ENOENT); +} ENOENT seems odd to me here. There looks to be no counterpart to EISDIR, so maybe ENODATA, EACCES, or EPERM? Hmm, why? In case I have /a/b and I'm looking for /a/b/c ENOENT seems to be just fine? Or I could add ENOTDIR. Oh, there actually is supposed to be such an entry, but public/errno.h is simply missing it. Yes - ENOTDIR is what I was thinking of when saying "there looks to be no counterpart to EISDIR". Okay, I'll add ENOTDIR and set it here. Juergen OpenPGP_0xB0DE9DD628BF132F.asc Description: application/pgp-keys OpenPGP_signature Description: OpenPGP digital signature
Re: [PATCH 2/2] mm: split out mfn_t / gfn_t / pfn_t definitions and helpers
On 02.12.2020 18:35, Julien Grall wrote: > On 02/12/2020 14:50, Jan Beulich wrote: >> xen/mm.h has heavy dependencies, while in a number of cases only these >> type definitions are needed. This separation then also allows pulling in >> these definitions when including xen/mm.h would cause cyclic >> dependencies. >> >> Replace xen/mm.h inclusion where possible in include/xen/. (In >> xen/iommu.h also take the opportunity and correct the few remaining >> sorting issues.) >> >> Signed-off-by: Jan Beulich >> >> --- a/xen/arch/x86/acpi/power.c >> +++ b/xen/arch/x86/acpi/power.c >> @@ -10,7 +10,6 @@ >>* Slimmed with Xen specific support. >>*/ >> >> -#include > > This seems to be unrelated of this work. Well spotted, but the answer really is "yes and no". My first attempt at fixing build issues from this and similar asm/io.h inclusions was to remove such unnecessary ones. But this didn't work out - I had to fix the header instead. If you think this extra cleanup really does any harm here, I can drop it. But I'd prefer to keep it. >> --- /dev/null >> +++ b/xen/include/xen/frame-num.h > > It would feel more natural to me if the file is named mm-types.h. Indeed I was first meaning to use this name (not the least because I don't particularly like the one chosen, but I also couldn't think of a better one). However, then things like struct page_info would imo also belong there (more precisely in asm/mm-types.h to be included from here), which is specifically something I want to avoid. Yes, eventually we may (I'm inclined to even say "will") want such a header, but I still want to keep these even more fundamental types in a separate one. Otherwise we'll again end up with files including mm-types.h just because of needing e.g. gfn_t for a function declaration. (Note that the same isn't the case for struct page_info, which can simply be forward declared.) Jan
Re: [PATCH v3 1/5] evtchn: drop acquiring of per-channel lock from send_guest_{global,vcpu}_virq()
On 02.12.2020 20:03, Julien Grall wrote: > On 23/11/2020 13:28, Jan Beulich wrote: >> The per-vCPU virq_lock, which is being held anyway, together with there >> not being any call to evtchn_port_set_pending() when v->virq_to_evtchn[] >> is zero, provide sufficient guarantees. > > I agree that the per-vCPU virq_lock is going to be sufficient, however > dropping the lock also means the event channel locking is more complex > to understand (the long comment that was added proves it). > > In fact, the locking in the event channel code was already proven to be > quite fragile, therefore I think this patch is not worth the risk. I agree this is a very reasonable position to take. I probably would even have remained silent if in the meantime the spin_lock()s there hadn't changed to read_trylock()s. I really think we want to limit this unusual locking model to where we strictly need it. And this change eliminates 50% of them, with the added benefit of making the paths more lightweight. Jan
Re: [PATCH v2 09/17] xen/hypfs: move per-node function pointers into a dedicated struct
On 03.12.20 10:12, Jan Beulich wrote: On 03.12.2020 09:47, Jürgen Groß wrote: On 02.12.20 16:36, Jan Beulich wrote: On 01.12.2020 09:21, Juergen Gross wrote: static int hypfs_write(struct hypfs_entry *entry, XEN_GUEST_HANDLE_PARAM(void) uaddr, unsigned long ulen) As a tangent, is there a reason these write functions don't take handles of "const void"? (I realize this likely is nothing that wants addressing right here.) Uh, this is harder than I thought. guest_handle_cast() doesn't handle const guest handle types currently: hypfs.c:447:58: error: unknown type name ‘const_void’; did you mean ‘const’? ret = hypfs_write(entry, guest_handle_cast(arg3, const_void), arg4); ^ /home/gross/xen/unstable/xen/include/xen/guest_access.h:26:5: note: in definition of macro ‘guest_handle_cast’ type *_x = (hnd).p; \ ^~~~ Currently my ideas would be to either: - add a new macro for constifying a guest handle (type -> const_type) - add a new macro for casting a guest handle to a const_type - add typedefs for the const_* types (typedef const x const_x) - open code the cast Or am I missing an existing variant? I don't think you are. Both of your first two suggestions look good to me - ultimately we may want to have both anyway, eventually. For its (presumed) type safety I may have a slight preference for option 1, albeit afaics guest_handle_cast() doesn't allow conversion between arbitrary types either (only to/from void). It's quite unfortunate that this requires an explicit cast in the first place, but what do you do. Right. I'm going with variant 2, as variant 1 is not really easy to achieve without specifying the basic type as a macro parameter - which will basically be variant 2. Juergen OpenPGP_0xB0DE9DD628BF132F.asc Description: application/pgp-keys OpenPGP_signature Description: OpenPGP digital signature
Re: [PATCH v3 5/5] evtchn: don't call Xen consumer callback with per-channel lock held
On 02.12.2020 22:10, Julien Grall wrote: > On 23/11/2020 13:30, Jan Beulich wrote: >> While there don't look to be any problems with this right now, the lock >> order implications from holding the lock can be very difficult to follow >> (and may be easy to violate unknowingly). The present callbacks don't >> (and no such callback should) have any need for the lock to be held. >> >> However, vm_event_disable() frees the structures used by respective >> callbacks and isn't otherwise synchronized with invocations of these >> callbacks, so maintain a count of in-progress calls, for evtchn_close() >> to wait to drop to zero before freeing the port (and dropping the lock). > > AFAICT, this callback is not the only place where the synchronization is > missing in the VM event code. > > For instance, vm_event_put_request() can also race against > vm_event_disable(). > > So shouldn't we handle this issue properly in VM event? I suppose that's a question to the VM event folks rather than me? >> --- >> Should we make this accounting optional, to be requested through a new >> parameter to alloc_unbound_xen_event_channel(), or derived from other >> than the default callback being requested? > > Aside the VM event, do you see any value for the other caller? No (albeit I'm not entirely certain about vpl011_notification()'s needs), hence the consideration. It's unnecessary overhead in those cases. >> @@ -781,9 +786,15 @@ int evtchn_send(struct domain *ld, unsig >> rport = lchn->u.interdomain.remote_port; >> rchn = evtchn_from_port(rd, rport); >> if ( consumer_is_xen(rchn) ) >> +{ >> +/* Don't keep holding the lock for the call below. */ >> +atomic_inc(&rchn->u.interdomain.active_calls); >> +evtchn_read_unlock(lchn); >> xen_notification_fn(rchn)(rd->vcpu[rchn->notify_vcpu_id], >> rport); >> -else >> -evtchn_port_set_pending(rd, rchn->notify_vcpu_id, rchn); > > atomic_dec() doesn't contain any memory barrier, so we will want one > between xen_notification_fn() and atomic_dec() to avoid re-ordering. Oh, indeed. But smp_mb() is too heavy handed here - x86 doesn't really need any barrier, yet would gain a full MFENCE that way. Actually - looks like I forgot we gained smp_mb__before_atomic() a little over half a year ago. Jan
[PATCH] x86/vmap: handle superpages in vmap_to_mfn()
From: Hongyan Xia There is simply no guarantee that vmap won't return superpages to the caller. It can happen if the list of MFNs are contiguous, or we simply have a large granularity. Although rare, if such things do happen, we will simply hit BUG_ON() and crash. Introduce xen_map_to_mfn() to translate any mapped Xen address to mfn regardless of page size, and wrap vmap_to_mfn() around it. Signed-off-by: Hongyan Xia --- Changed in v2: - const pl*e - introduce xen_map_to_mfn(). - goto to a single exit path. - ASSERT_UNREACHABLE instead of ASSERT. --- xen/arch/x86/mm.c | 54 ++ xen/include/asm-x86/mm.h | 1 + xen/include/asm-x86/page.h | 2 +- 3 files changed, 56 insertions(+), 1 deletion(-) diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index 5a50339284c7..53cc5c6de2e6 100644 --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -5194,6 +5194,60 @@ l1_pgentry_t *virt_to_xen_l1e(unsigned long v) } \ } while ( false ) +/* Translate mapped Xen address to MFN. */ +mfn_t xen_map_to_mfn(unsigned long va) +{ +#define CHECK_MAPPED(cond_) \ +if ( !(cond_) ) \ +{ \ +ASSERT_UNREACHABLE(); \ +ret = INVALID_MFN; \ +goto out; \ +} \ + +bool locking = system_state > SYS_STATE_boot; +unsigned int l2_offset = l2_table_offset(va); +unsigned int l1_offset = l1_table_offset(va); +const l3_pgentry_t *pl3e = virt_to_xen_l3e(va); +const l2_pgentry_t *pl2e = NULL; +const l1_pgentry_t *pl1e = NULL; +struct page_info *l3page; +mfn_t ret; + +L3T_INIT(l3page); +CHECK_MAPPED(pl3e) +l3page = virt_to_page(pl3e); +L3T_LOCK(l3page); + +CHECK_MAPPED(l3e_get_flags(*pl3e) & _PAGE_PRESENT) +if ( l3e_get_flags(*pl3e) & _PAGE_PSE ) +{ +ret = mfn_add(l3e_get_mfn(*pl3e), + (l2_offset << PAGETABLE_ORDER) + l1_offset); +goto out; +} + +pl2e = map_l2t_from_l3e(*pl3e) + l2_offset; +CHECK_MAPPED(l2e_get_flags(*pl2e) & _PAGE_PRESENT) +if ( l2e_get_flags(*pl2e) & _PAGE_PSE ) +{ +ret = mfn_add(l2e_get_mfn(*pl2e), l1_offset); +goto out; +} + +pl1e = map_l1t_from_l2e(*pl2e) + l1_offset; +CHECK_MAPPED(l1e_get_flags(*pl1e) & _PAGE_PRESENT) +ret = l1e_get_mfn(*pl1e); + +#undef CHECK_MAPPED + out: +L3T_UNLOCK(l3page); +unmap_domain_page(pl1e); +unmap_domain_page(pl2e); +unmap_domain_page(pl3e); +return ret; +} + int map_pages_to_xen( unsigned long virt, mfn_t mfn, diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h index deeba75a1cbb..2fc8eeaf7aad 100644 --- a/xen/include/asm-x86/mm.h +++ b/xen/include/asm-x86/mm.h @@ -578,6 +578,7 @@ mfn_t alloc_xen_pagetable_new(void); void free_xen_pagetable_new(mfn_t mfn); l1_pgentry_t *virt_to_xen_l1e(unsigned long v); +mfn_t xen_map_to_mfn(unsigned long va); int __sync_local_execstate(void); diff --git a/xen/include/asm-x86/page.h b/xen/include/asm-x86/page.h index 7a771baf7cb3..886adf17a40c 100644 --- a/xen/include/asm-x86/page.h +++ b/xen/include/asm-x86/page.h @@ -291,7 +291,7 @@ void copy_page_sse2(void *, const void *); #define pfn_to_paddr(pfn) __pfn_to_paddr(pfn) #define paddr_to_pfn(pa)__paddr_to_pfn(pa) #define paddr_to_pdx(pa)pfn_to_pdx(paddr_to_pfn(pa)) -#define vmap_to_mfn(va) l1e_get_mfn(*virt_to_xen_l1e((unsigned long)(va))) +#define vmap_to_mfn(va) xen_map_to_mfn((unsigned long)va) #define vmap_to_page(va)mfn_to_page(vmap_to_mfn(va)) #endif /* !defined(__ASSEMBLY__) */ -- 2.17.1
Re: [PATCH] x86/vmap: handle superpages in vmap_to_mfn()
Apologies. Missing v2 in the title.
Re: [PATCH v2 2/8] xen/arm: revert atomic operation related command-queue insertion patch
Hello Julien, Thanks for reviewing the code. > On 2 Dec 2020, at 1:44 pm, Julien Grall wrote: > > Hi Rahul, > > On 26/11/2020 17:02, Rahul Singh wrote: >> Linux SMMUv3 code implements the commands-queue insertion based on >> atomic operations implemented in Linux. Atomic functions used by the >> commands-queue insertion is not implemented in XEN therefore revert the >> patch that implemented the commands-queue insertion based on atomic >> operations. > > This commit message explains why we revert but not the consequence of the > revert. Can outline if there are any and why they are fine? Ok let me try to add more detail. > > I am also interested to have a list of *must* have for the driver to be out > of the tech preview. Ok let me add more informing in the commit message in next version of the patch. Regards, Rahul > > Cheers, > > -- > Julien Grall
Re: [PATCH 1/2] include: don't use asm/page.h from common headers
On 03/12/2020 09:27, Jan Beulich wrote: >>> --- /dev/null >>> +++ b/xen/include/asm-arm/page-shift.h >> The name of the file looks a bit odd given that *_BITS are also defined >> in it. So how about renaming to page-size.h? > I was initially meaning to use that name, but these headers > specifically don't define any sizes - *_BITS are still shift > values, at least in a way. If the current name isn't liked, my > next best suggestion would then be page-bits.h. Pick a generic name, or it will bitrot quickly, and it really wants to be the same across architectures. The real issue is that page.h contains far too much stuff now, in both architectures. Longterm, we want to split apart the architectural pagetable definitions, from the Xen APIs to manipulate them, which will simplify the asm include hierarchy as well. I'd go with page-bits.h, or just a plain pagetable.h. ~Andrew
[ovmf test] 157167: all pass - PUSHED
flight 157167 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/157167/ Perfect :-) All tests in this flight passed as required version targeted for testing: ovmf 7c4ab1c2ef60a4690177d2361f8dd44d7d7df7f8 baseline version: ovmf 9fb629edd75e1ae1e7f4e85b0876107a7180899b Last test of basis 157117 2020-11-30 18:12:47 Z2 days Testing same since 157167 2020-12-02 23:40:53 Z0 days1 attempts People who touched revisions under test: Guo Dong 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 9fb629edd7..7c4ab1c2ef 7c4ab1c2ef60a4690177d2361f8dd44d7d7df7f8 -> xen-tested-master
[PATCH v5 1/4] domctl: introduce a new domain create flag, XEN_DOMCTL_CDF_evtchn_fifo, ...
From: Paul Durrant ...to control the visibility of the FIFO event channel operations (EVTCHNOP_init_control, EVTCHNOP_expand_array, and EVTCHNOP_set_priority) to the guest. These operations were added to the public header in commit d2d50c2f308f ("evtchn: add FIFO-based event channel ABI") and the first implementation appeared in the two subsequent commits: edc8872aeb4a ("evtchn: implement EVTCHNOP_set_priority and add the set_priority hook") and 88910061ec61 ("evtchn: add FIFO-based event channel hypercalls and port ops"). Prior to that, a guest issuing those operations would receive a return value of -ENOSYS (not implemented) from Xen. Guests aware of the FIFO operations but running on an older (pre-4.4) Xen would fall back to using the 2-level event channel interface upon seeing this return value. Unfortunately the uncontrolable appearance of these new operations in Xen 4.4 onwards has implications for hibernation of some Linux guests. During resume from hibernation, there are two kernels involved: the "boot" kernel and the "resume" kernel. The guest boot kernel may default to use FIFO operations and instruct Xen via EVTCHNOP_init_control to switch from 2-level to FIFO. On the other hand, the resume kernel keeps assuming 2-level, because it was hibernated on a version of Xen that did not support the FIFO operations. To maintain compatibility it is necessary to make Xen behave as it did before the new operations were added and hence the code in this patch ensures that, if XEN_DOMCTL_CDF_evtchn_fifo is not set, the FIFO event channel operations will again result in -ENOSYS being returned to the guest. This patch also adds an extra log line into the 'e' key handler output to call out which event channel ABI is in use by a domain. NOTE: To maintain current behavior, until a tool-stack option is added to control the flag, it is unconditionally set for all domains. A subsequent patch will introduce such tool-stack control. Signed-off-by: Paul Durrant Signed-off-by: Eslam Elnikety --- Cc: Ian Jackson Cc: Wei Liu Cc: Anthony PERARD Cc: Andrew Cooper Cc: George Dunlap Cc: Jan Beulich Cc: Julien Grall Cc: Stefano Stabellini Cc: Christian Lindig Cc: David Scott Cc: Volodymyr Babchuk Cc: "Roger Pau Monné" v5: - Flip the sense of the flag from disabling to enabling, as requested by Andrew v4: - New in v4 --- tools/libs/light/libxl_create.c | 1 + tools/ocaml/libs/xc/xenctrl.ml | 1 + tools/ocaml/libs/xc/xenctrl.mli | 1 + xen/arch/arm/domain.c | 3 ++- xen/arch/arm/domain_build.c | 3 ++- xen/arch/arm/setup.c| 3 ++- xen/arch/x86/setup.c| 3 ++- xen/common/domain.c | 2 +- xen/common/event_channel.c | 24 +--- xen/include/public/domctl.h | 4 +++- 10 files changed, 36 insertions(+), 9 deletions(-) diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c index 321a13e519b5..3ca9f00d6d83 100644 --- a/tools/libs/light/libxl_create.c +++ b/tools/libs/light/libxl_create.c @@ -607,6 +607,7 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config, .max_evtchn_port = b_info->event_channels, .max_grant_frames = b_info->max_grant_frames, .max_maptrack_frames = b_info->max_maptrack_frames, +.flags = XEN_DOMCTL_CDF_evtchn_fifo, }; if (info->type != LIBXL_DOMAIN_TYPE_PV) { diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml index e878699b0a1a..fa5c7b7eb0a2 100644 --- a/tools/ocaml/libs/xc/xenctrl.ml +++ b/tools/ocaml/libs/xc/xenctrl.ml @@ -65,6 +65,7 @@ type domain_create_flag = | CDF_XS_DOMAIN | CDF_IOMMU | CDF_NESTED_VIRT + | CDF_EVTCHN_FIFO type domain_create_iommu_opts = | IOMMU_NO_SHAREPT diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli index e64907df8e7e..a872002d90cc 100644 --- a/tools/ocaml/libs/xc/xenctrl.mli +++ b/tools/ocaml/libs/xc/xenctrl.mli @@ -58,6 +58,7 @@ type domain_create_flag = | CDF_XS_DOMAIN | CDF_IOMMU | CDF_NESTED_VIRT + | CDF_EVTCHN_FIFO type domain_create_iommu_opts = | IOMMU_NO_SHAREPT diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index 18cafcdda7b1..59f947370053 100644 --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -622,7 +622,8 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config) unsigned int max_vcpus; /* HVM and HAP must be set. IOMMU may or may not be */ -if ( (config->flags & ~XEN_DOMCTL_CDF_iommu) != +if ( (config->flags & + ~(XEN_DOMCTL_CDF_iommu | XEN_DOMCTL_CDF_evtchn_fifo) != (XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap) ) { dprintk(XENLOG_INFO, "Unsupported configuration %#x\n", diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index e824ba34b012..13d1e79f1463 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_bui
[PATCH v5 0/4] Xen ABI feature control
From: Paul Durrant This series was previously called "evtchn: Introduce a per-guest knob to control FIFO ABI". It is been extensively re-worked and extended to cover another ABI feature. Paul Durrant (4): domctl: introduce a new domain create flag, XEN_DOMCTL_CDF_evtchn_fifo, ... domctl: introduce a new domain create flag, XEN_DOMCTL_CDF_evtchn_upcall, ... libxl: introduce a 'libxl_xen_abi_features' enumeration... xl: introduce a 'xen-abi-features' option... docs/man/xl.cfg.5.pod.in | 50 tools/include/libxl.h| 10 +++ tools/libs/light/libxl_arm.c | 22 +- tools/libs/light/libxl_create.c | 31 tools/libs/light/libxl_types.idl | 7 + tools/libs/light/libxl_x86.c | 17 ++- tools/ocaml/libs/xc/xenctrl.ml | 2 ++ tools/ocaml/libs/xc/xenctrl.mli | 2 ++ tools/xl/xl_parse.c | 50 ++-- xen/arch/arm/domain.c| 3 +- xen/arch/arm/domain_build.c | 3 +- xen/arch/arm/setup.c | 3 +- xen/arch/x86/domain.c| 8 + xen/arch/x86/hvm/hvm.c | 3 ++ xen/arch/x86/setup.c | 4 ++- xen/common/domain.c | 3 +- xen/common/event_channel.c | 24 +-- xen/include/public/domctl.h | 6 +++- 18 files changed, 229 insertions(+), 19 deletions(-) --- Cc: Andrew Cooper Cc: Anthony PERARD Cc: Christian Lindig Cc: David Scott Cc: George Dunlap Cc: Ian Jackson Cc: Jan Beulich Cc: Julien Grall Cc: "Roger Pau Monné" Cc: Stefano Stabellini Cc: Volodymyr Babchuk Cc: Wei Liu -- 2.20.1
[PATCH v5 4/4] xl: introduce a 'xen-abi-features' option...
From: Paul Durrant ... to control which features of the Xen ABI are enabled in 'libxl_domain_build_info', and hence exposed to the guest. Signed-off-by: Paul Durrant --- Cc: Ian Jackson Cc: Wei Liu Cc: Anthony PERARD v5: - New in v5 --- docs/man/xl.cfg.5.pod.in | 50 tools/xl/xl_parse.c | 50 ++-- 2 files changed, 98 insertions(+), 2 deletions(-) diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in index 3f0f8de1e988..b42ab8ba9f60 100644 --- a/docs/man/xl.cfg.5.pod.in +++ b/docs/man/xl.cfg.5.pod.in @@ -1649,6 +1649,56 @@ This feature is a B. =back +=item B + +The features of the Xen ABI exposed to the guest. The following features +may be specified: + +=over 4 + +=item B + +A new event channel ABI was introduced in Xen 4.4. Moving a guest from an +earlier Xen to Xen 4.4 onwards may expose bugs in the guest support for +this ABI. Disabling this feature hides the ABI from the guest and hence +may be used as a workaround for such bugs. + +The festure is enabled by default. + +=item B + +B. A new hypercall to specify per-VCPU interrupt vectors to use +for event channel upcalls in HVM guests was added in Xen 4.6. Moving a guest +from an earlier Xen to Xen 4.6 onwards may expose bugs in the guest support +for this hypercall. Disabling this feature hides the hypercall from the +guest and hence may be used as a workaround for such bugs. + +The festure is enabled by default for B guests. Note that it is +considered an error to enable this feature for B or B guests. + +=item B + +This is a special value that enables all available features. + +=back + +Features can be disabled by prefixing the name with '!'. So, for example, +to enable all features except B, specify: + +=over 4 + +B + +=back + +Or, to simply enable default features except B, specify: + +=over 4 + +B + +=back + =back =head2 Paravirtualised (PV) Guest Specific Options diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c index cae8eb679c5a..566e09f938f4 100644 --- a/tools/xl/xl_parse.c +++ b/tools/xl/xl_parse.c @@ -1216,8 +1216,9 @@ void parse_config_data(const char *config_source, XLU_ConfigList *cpus, *vbds, *nics, *pcis, *cvfbs, *cpuids, *vtpms, *usbctrls, *usbdevs, *p9devs, *vdispls, *pvcallsifs_devs; XLU_ConfigList *channels, *ioports, *irqs, *iomem, *viridian, *dtdevs, - *mca_caps; -int num_ioports, num_irqs, num_iomem, num_cpus, num_viridian, num_mca_caps; + *mca_caps, *features; +int num_ioports, num_irqs, num_iomem, num_cpus, num_viridian, num_mca_caps, +num_features; int pci_power_mgmt = 0; int pci_msitranslate = 0; int pci_permissive = 0; @@ -2737,6 +2738,51 @@ skip_usbdev: xlu_cfg_get_defbool(config, "xend_suspend_evtchn_compat", &c_info->xend_suspend_evtchn_compat, 0); +switch (xlu_cfg_get_list(config, "xen_abi_features", + &features, &num_features, 1)) +{ +case 0: /* Success */ +if (num_features) { +libxl_bitmap_alloc(ctx, &b_info->feature_enable, + LIBXL_BUILDINFO_FEATURE_ENABLE_DISABLE_WIDTH); +libxl_bitmap_alloc(ctx, &b_info->feature_disable, + LIBXL_BUILDINFO_FEATURE_ENABLE_DISABLE_WIDTH); +} +for (i = 0; i < num_features; i++) { +if (strcmp(buf, "all") == 0) +libxl_bitmap_set_any(&b_info->feature_enable); +else { +libxl_bitmap *s = &b_info->feature_enable; +libxl_bitmap *r = &b_info->feature_disable; +libxl_xen_abi_feature f; + +buf = xlu_cfg_get_listitem(features, i); + +if (*buf == '!') { +s = &b_info->feature_disable; +r = &b_info->feature_enable; +buf++; +} + +e = libxl_xen_abi_feature_from_string(buf, &f); +if (e) { +fprintf(stderr, +"xl: Unknown Xen ABI feature '%s'\n", +buf); +exit(-ERROR_FAIL); +} + +libxl_bitmap_set(s, f); +libxl_bitmap_reset(r, f); +} +} +break; +case ESRCH: break; /* Option not present */ +default: +fprintf(stderr,"xl: Unable to parse Xen ABI features.\n"); +exit(-ERROR_FAIL); +} + xlu_cfg_destroy(config); } -- 2.20.1
[PATCH v5 2/4] domctl: introduce a new domain create flag, XEN_DOMCTL_CDF_evtchn_upcall, ...
From: Paul Durrant ...to control the visibility of the per-vCPU upcall feature for HVM guests. Commit 04447f4453c0 ("x86/hvm: add per-vcpu evtchn upcalls") added a mechanism by which x86 HVM guests can register a vector for each vCPU which will be used by Xen to signal event channels on that vCPU. This facility (an HVMOP hypercall) appeared in a uncontrolled fashion which has implications for the behaviour of OS when moving from an older Xen to a newer Xen. For instance, the OS may be aware of the per-vCPU upcall feature but support for it is buggy. In this case the OS will function perfectly well on the older Xen, but fail (in a potentially non-obvious way) on the newer Xen. To maintain compatibility it is necessary to make Xen behave as it did before the new hypercall was added and hence the code in this patch ensures that, if XEN_DOMCTL_CDF_evtchn_upcall is not set, the hypercall will again result in -ENOSYS being returned to the guest. NOTE: To maintain current behavior, until a tool-stack option is added to control the flag, it is unconditionally set for x86 HVM domains. A subsequent patch will introduce such tool-stack control. Signed-off-by: Paul Durrant --- Cc: Ian Jackson Cc: Wei Liu Cc: Anthony PERARD Cc: Andrew Cooper Cc: George Dunlap Cc: Jan Beulich Cc: Julien Grall Cc: Stefano Stabellini Cc: Christian Lindig Cc: David Scott Cc: "Roger Pau Monné" v5: - New in v5 --- tools/libs/light/libxl_x86.c| 7 ++- tools/ocaml/libs/xc/xenctrl.ml | 1 + tools/ocaml/libs/xc/xenctrl.mli | 1 + xen/arch/x86/domain.c | 8 xen/arch/x86/hvm/hvm.c | 3 +++ xen/arch/x86/setup.c| 1 + xen/common/domain.c | 3 ++- xen/include/public/domctl.h | 4 +++- 8 files changed, 25 insertions(+), 3 deletions(-) diff --git a/tools/libs/light/libxl_x86.c b/tools/libs/light/libxl_x86.c index 86d272999d67..f7217b422404 100644 --- a/tools/libs/light/libxl_x86.c +++ b/tools/libs/light/libxl_x86.c @@ -5,7 +5,12 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc, libxl_domain_config *d_config, struct xen_domctl_createdomain *config) { -switch(d_config->c_info.type) { +libxl_domain_create_info *info = &d_config->c_info; + +if (info->type == LIBXL_DOMAIN_TYPE_HVM) +config->flags |= XEN_DOMCTL_CDF_evtchn_upcall; + +switch(info->type) { case LIBXL_DOMAIN_TYPE_HVM: config->arch.emulation_flags = (XEN_X86_EMU_ALL & ~XEN_X86_EMU_VPCI); break; diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml index fa5c7b7eb0a2..04284c364108 100644 --- a/tools/ocaml/libs/xc/xenctrl.ml +++ b/tools/ocaml/libs/xc/xenctrl.ml @@ -66,6 +66,7 @@ type domain_create_flag = | CDF_IOMMU | CDF_NESTED_VIRT | CDF_EVTCHN_FIFO + | CDF_EVTCHN_UPCALL type domain_create_iommu_opts = | IOMMU_NO_SHAREPT diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli index a872002d90cc..e40759464ae5 100644 --- a/tools/ocaml/libs/xc/xenctrl.mli +++ b/tools/ocaml/libs/xc/xenctrl.mli @@ -59,6 +59,7 @@ type domain_create_flag = | CDF_IOMMU | CDF_NESTED_VIRT | CDF_EVTCHN_FIFO + | CDF_EVTCHN_UPCALL type domain_create_iommu_opts = | IOMMU_NO_SHAREPT diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c index 1b894d0124d7..e7f83880219d 100644 --- a/xen/arch/x86/domain.c +++ b/xen/arch/x86/domain.c @@ -662,11 +662,19 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config) } if ( !hvm ) +{ +if ( config->flags & XEN_DOMCTL_CDF_evtchn_upcall ) +{ +dprintk(XENLOG_INFO, "Per-vCPU event channel vector only supported for HVM guests\n"); +return -EINVAL; +} + /* * It is only meaningful for XEN_DOMCTL_CDF_oos_off to be clear * for HVM guests. */ config->flags |= XEN_DOMCTL_CDF_oos_off; +} if ( nested_virt && !hap ) { diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 54e32e4fe85c..7ffc42a7282e 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -4037,6 +4037,9 @@ static int hvmop_set_evtchn_upcall_vector( struct domain *d = current->domain; struct vcpu *v; +if ( !(d->options & XEN_DOMCTL_CDF_evtchn_upcall) ) +return -ENOSYS; + if ( !is_hvm_domain(d) ) return -EINVAL; diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index e558241c73da..3ff9a25eede6 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -751,6 +751,7 @@ static struct domain *__init create_dom0(const module_t *image, if ( opt_dom0_pvh ) { dom0_cfg.flags |= (XEN_DOMCTL_CDF_hvm | + XEN_DOMCTL_CDF_evtchn_upcall | ((hvm_hap_supported() && !opt_dom0_shadow) ? XEN_DOMCTL_CDF_hap
[PATCH v5 3/4] libxl: introduce a 'libxl_xen_abi_features' enumeration...
From: Paul Durrant ... and bitmaps to enable or disable fetaures. This patch adds a new 'libxl_xen_abi_features' enumeration into the IDL which specifies features of the Xen ABI which may be optionally enabled or disabled via new 'feature_enable' and 'feature_disable' bitaps added into 'libxl_domain_build_info'. The initially defined features are enabled by default (for relevant architectures) and so the corresponding flags in 'struct xen_domctl_createdomain' are set if they are missing from 'disable_features' rather than if they are present in 'enable_features'. Checks are, however, added to make sure that features are not specifically enabled in cases where they are not supported. NOTE: A subsequent patch will add an option into xl.cfg(5) to control whether Xen ABI features are enabled or disabled. Signed-off-by: Paul Durrant --- Cc: Ian Jackson Cc: Wei Liu Cc: Anthony PERARD v5: - New in v5 --- tools/include/libxl.h| 10 ++ tools/libs/light/libxl_arm.c | 22 +++--- tools/libs/light/libxl_create.c | 32 +++- tools/libs/light/libxl_types.idl | 7 +++ tools/libs/light/libxl_x86.c | 14 -- 5 files changed, 75 insertions(+), 10 deletions(-) diff --git a/tools/include/libxl.h b/tools/include/libxl.h index eaffccb30f37..b328a5621e6f 100644 --- a/tools/include/libxl.h +++ b/tools/include/libxl.h @@ -451,6 +451,16 @@ */ #define LIBXL_HAVE_VIRIDIAN_EX_PROCESSOR_MASKS 1 +/* + * LIBXL_HAVE_BUILDINFO_XEN_ABI_FEATURE indicates that the + * libxl_xen_abi_feature enumeration is defined and that + * libxl_domain_build_info has feature_enable and _disable bitmaps + * of the specified width. These bitmaps are used to enable or disable + * features of the Xen ABI (enumerated by the new type) for a domain. + */ +#define LIBXL_HAVE_BUILDINFO_XEN_ABI_FEATURE 1 +#define LIBXL_BUILDINFO_FEATURE_ENABLE_DISABLE_WIDTH 64 + /* * libxl ABI compatibility * diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c index 66e8a065fe67..69676340a661 100644 --- a/tools/libs/light/libxl_arm.c +++ b/tools/libs/light/libxl_arm.c @@ -28,19 +28,27 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc, unsigned int i; uint32_t vuart_irq; bool vuart_enabled = false; +libxl_domain_build_info *b_info = &d_config->b_info; +libxl_xen_abi_feature f = LIBXL_XEN_ABI_FEATURE_EVTCHN_UPCALL; + +if (libxl_bitmap_test(&b_info->feature_enable, f)) { +LOG(ERROR, "unsupported Xen ABI feature '%s'", +libxl_xen_abi_feature_to_string(f)); +return ERROR_FAIL; +} /* * If pl011 vuart is enabled then increment the nr_spis to allow allocation * of SPI VIRQ for pl011. */ -if (d_config->b_info.arch_arm.vuart == LIBXL_VUART_TYPE_SBSA_UART) { +if (b_info->arch_arm.vuart == LIBXL_VUART_TYPE_SBSA_UART) { nr_spis += (GUEST_VPL011_SPI - 32) + 1; vuart_irq = GUEST_VPL011_SPI; vuart_enabled = true; } -for (i = 0; i < d_config->b_info.num_irqs; i++) { -uint32_t irq = d_config->b_info.irqs[i]; +for (i = 0; i < b_info->num_irqs; i++) { +uint32_t irq = b_info->irqs[i]; uint32_t spi; /* @@ -72,7 +80,7 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc, config->arch.nr_spis = nr_spis; LOG(DEBUG, " - Allocate %u SPIs", nr_spis); -switch (d_config->b_info.arch_arm.gic_version) { +switch (b_info->arch_arm.gic_version) { case LIBXL_GIC_VERSION_DEFAULT: config->arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE; break; @@ -84,11 +92,11 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc, break; default: LOG(ERROR, "Unknown GIC version %d", -d_config->b_info.arch_arm.gic_version); +b_info->arch_arm.gic_version); return ERROR_FAIL; } -switch (d_config->b_info.tee) { +switch (b_info->tee) { case LIBXL_TEE_TYPE_NONE: config->arch.tee_type = XEN_DOMCTL_CONFIG_TEE_NONE; break; @@ -97,7 +105,7 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc, break; default: LOG(ERROR, "Unknown TEE type %d", -d_config->b_info.tee); +b_info->tee); return ERROR_FAIL; } diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c index 3ca9f00d6d83..8cf7fd5f6d1b 100644 --- a/tools/libs/light/libxl_create.c +++ b/tools/libs/light/libxl_create.c @@ -587,6 +587,7 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config, struct xs_permissions noperm[1]; xs_transaction_t t = 0; libxl_vminfo *vm_list; +libxl_xen_abi_feature f; /* convenience aliases */ libxl_domain_create_info *info = &d_config->c_info; @@ -607,9 +608,38 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config, .max_evtchn_port = b_info->event_channels,
Re: [PATCH v2 3/8] xen/arm: revert patch related to XArray
Hello Julien, > On 2 Dec 2020, at 1:46 pm, Julien Grall wrote: > > Hi Rahul, > > On 26/11/2020 17:02, Rahul Singh wrote: >> XArray is not implemented in XEN revert the patch that introduce the >> XArray code in SMMUv3 driver. > > Similar to the atomic revert, you are explaining why the revert but not the > consequences. I think this is quite important to have them outline in the > commit message as it looks like it means the SMMU driver would not scale. Ok I will add. > >> Once XArray is implemented in XEN this patch can be added in XEN. > > What's the plan for that? As of now there is no plan for Xarray from our side. Do we have requirement for Xarray implementation in XEN ? Regards, Rahul > > Cheers, > > -- > Julien Grall
Re: [PATCH v2 4/8] xen/arm: Remove support for MSI on SMMUv3
Hello Julien, > On 2 Dec 2020, at 2:11 pm, Julien Grall wrote: > > Hi Rahul, > > On 02/12/2020 13:12, Rahul Singh wrote: >> Hello Stefano, >>> On 2 Dec 2020, at 12:40 am, Stefano Stabellini >>> wrote: >>> >>> On Tue, 1 Dec 2020, Stefano Stabellini wrote: On Thu, 26 Nov 2020, Rahul Singh wrote: > XEN does not support MSI on ARM platforms, therefore remove the MSI > support from SMMUv3 driver. > > Signed-off-by: Rahul Singh I wonder if it makes sense to #ifdef CONFIG_MSI this code instead of removing it completely. >>> >>> One more thought: could this patch be achieved by reverting >>> 166bdbd23161160f2abcea70621adba179050bee ? If this patch could be done >>> by a couple of revert, it would be great to say it in the commit >>> message. >>> >> Ok will add in next version. >>> In the past, we tried to keep the entire file as textually similar to the original Linux driver as possible to make it easier to backport features and fixes. So, in this case we would probably not even used an #ifdef but maybe something like: if (/* msi_enabled */ 0) return; at the beginning of arm_smmu_setup_msis. However, that strategy didn't actually work very well because backports have proven difficult to do anyway. So at that point we might as well at least have clean code in Xen and do the changes properly. > > It was difficult because Linux decided to rework how IOMMU drivers works. I > agree the risk is still there and therefore clean code would be better with > some caveats (see below). > >> Main reason to remove the feature/code that is not usable in XEN is to have >> a clean code. > > I agree that short term this feature will not be usable. However, I think we > need to think about {medium, long}-term plan to avoid extra effort in the > future because the driver evolve in a way making the revert of revert > impossible. > > Therefore I would prefer to keep both the MSI and PCI ATS present as they are > going to be useful/necessary on some platforms. It doesn't matter that they > don't work because the driver will be in tech preview. Ok. As Stefano also agreed the same I will keep the PC ATS and MSI functionality in next version. Regards, Rahul > > Cheers, > > -- > Julien Grall
RE: [PATCH v4 01/23] xl / libxl: s/pcidev/pci and remove DEFINE_DEVICE_TYPE_STRUCT_X
> -Original Message- > From: Oleksandr Andrushchenko > Sent: 01 December 2020 12:33 > To: Paul Durrant ; xen-devel@lists.xenproject.org > Cc: Paul Durrant ; Ian Jackson ; > Wei Liu ; > Anthony PERARD > Subject: Re: [PATCH v4 01/23] xl / libxl: s/pcidev/pci and remove > DEFINE_DEVICE_TYPE_STRUCT_X > > Hi, Paul! > > On 11/24/20 10:01 AM, Paul Durrant wrote: > > From: Paul Durrant > > > > The seemingly arbitrary use of 'pci' and 'pcidev' in the code in libxl_pci.c > > is confusing and also compromises use of some macros used for other device > > types. Indeed it seems that DEFINE_DEVICE_TYPE_STRUCT_X exists solely > > because > > of this duality. > > > > This patch purges use of 'pcidev' from the libxl code, allowing evaluation > > of > > DEFINE_DEVICE_TYPE_STRUCT_X to be replaced with DEFINE_DEVICE_TYPE_STRUCT, > > hence allowing removal of the former. > > > > For consistency the xl and libs/util code is also modified, but in this case > > it is purely cosmetic. > > > > NOTE: Some of the more gross formatting errors (such as lack of spaces after > >keywords) that came into context have been fixed in libxl_pci.c. > > > > Signed-off-by: Paul Durrant > > --- > > Cc: Ian Jackson > > Cc: Wei Liu > > Cc: Anthony PERARD > > --- > > tools/include/libxl.h | 17 +- > > tools/libs/light/libxl_create.c | 6 +- > > tools/libs/light/libxl_dm.c | 18 +- > > tools/libs/light/libxl_internal.h | 45 ++- > > tools/libs/light/libxl_pci.c | 582 > > +++--- > > tools/libs/light/libxl_types.idl | 2 +- > > tools/libs/util/libxlu_pci.c | 36 +-- > > tools/xl/xl_parse.c | 28 +- > > tools/xl/xl_pci.c | 68 ++--- > > tools/xl/xl_sxp.c | 12 +- > > 10 files changed, 409 insertions(+), 405 deletions(-) > > > > diff --git a/tools/include/libxl.h b/tools/include/libxl.h > > index 1ea5b4f446..fbe4c81ba5 100644 > > --- a/tools/include/libxl.h > > +++ b/tools/include/libxl.h > > @@ -445,6 +445,13 @@ > > #define LIBXL_HAVE_DISK_SAFE_REMOVE 1 > > > [snip] > > -/* Scan through /sys/.../pciback/slots looking for pcidev's BDF */ > > -static int pciback_dev_has_slot(libxl__gc *gc, libxl_device_pci *pcidev) > > +/* Scan through /sys/.../pciback/slots looking for pci's BDF */ > > +static int pciback_dev_has_slot(libxl__gc *gc, libxl_device_pci *pci) > > { > > FILE *f; > > int rc = 0; > > @@ -635,11 +635,11 @@ static int pciback_dev_has_slot(libxl__gc *gc, > > libxl_device_pci *pcidev) > > return ERROR_FAIL; > > } > > > > -while(fscanf(f, "%x:%x:%x.%d\n", &dom, &bus, &dev, &func)==4) { > > -if(dom == pcidev->domain > > - && bus == pcidev->bus > > - && dev == pcidev->dev > > - && func == pcidev->func) { > > +while (fscanf(f, "%x:%x:%x.%d\n", &dom, &bus, &dev, &func)==4) { > So, then you can probably put spaces around "4" if touching this line Oh yes. Will do. > > +if (dom == pci->domain > > +&& bus == pci->bus > > +&& dev == pci->dev > > +&& func == pci->func) { > > rc = 1; > > goto out; > > } > > @@ -649,7 +649,7 @@ out: > > return rc; > > } > > > > Reviewed-by: Oleksandr Andrushchenko Thanks, Paul
Re: [PATCH v5 0/4] Xen ABI feature control
On 03.12.20 13:41, Paul Durrant wrote: From: Paul Durrant This series was previously called "evtchn: Introduce a per-guest knob to control FIFO ABI". It is been extensively re-worked and extended to cover another ABI feature. Paul Durrant (4): domctl: introduce a new domain create flag, XEN_DOMCTL_CDF_evtchn_fifo, ... domctl: introduce a new domain create flag, XEN_DOMCTL_CDF_evtchn_upcall, ... libxl: introduce a 'libxl_xen_abi_features' enumeration... xl: introduce a 'xen-abi-features' option... docs/man/xl.cfg.5.pod.in | 50 tools/include/libxl.h| 10 +++ tools/libs/light/libxl_arm.c | 22 +- tools/libs/light/libxl_create.c | 31 tools/libs/light/libxl_types.idl | 7 + tools/libs/light/libxl_x86.c | 17 ++- tools/ocaml/libs/xc/xenctrl.ml | 2 ++ tools/ocaml/libs/xc/xenctrl.mli | 2 ++ tools/xl/xl_parse.c | 50 ++-- xen/arch/arm/domain.c| 3 +- xen/arch/arm/domain_build.c | 3 +- xen/arch/arm/setup.c | 3 +- xen/arch/x86/domain.c| 8 + xen/arch/x86/hvm/hvm.c | 3 ++ xen/arch/x86/setup.c | 4 ++- xen/common/domain.c | 3 +- xen/common/event_channel.c | 24 +-- xen/include/public/domctl.h | 6 +++- 18 files changed, 229 insertions(+), 19 deletions(-) --- Cc: Andrew Cooper Cc: Anthony PERARD Cc: Christian Lindig Cc: David Scott Cc: George Dunlap Cc: Ian Jackson Cc: Jan Beulich Cc: Julien Grall Cc: "Roger Pau Monné" Cc: Stefano Stabellini Cc: Volodymyr Babchuk Cc: Wei Liu Do we want to add a create flag for each such feature, or would it be better to set options like those via hypfs? It would be fairly easy to ad dynamic hypfs paths, e.g.: /domain//abi-features/evtchn-fifo /domain//abi-features/evtchn-upcall which would have boolean type and could be set as long as the domain hasn't been started. xl support could even be rather generic, without the need to add coding to xl for each new feature. This is no objection to this series, but just an idea how to avoid extending the use of unstable interfaces. Thoughts? Juergen OpenPGP_0xB0DE9DD628BF132F.asc Description: application/pgp-keys OpenPGP_signature Description: OpenPGP digital signature
RE: [PATCH v4 03/23] libxl: Make sure devices added by pci-attach are reflected in the config
> -Original Message- > From: Oleksandr Andrushchenko > Sent: 01 December 2020 13:12 > To: Paul Durrant ; xen-devel@lists.xenproject.org > Cc: Paul Durrant ; Ian Jackson ; > Wei Liu ; > Anthony PERARD > Subject: Re: [PATCH v4 03/23] libxl: Make sure devices added by pci-attach > are reflected in the config > > Hi, Paul! > > On 11/24/20 10:01 AM, Paul Durrant wrote: > > From: Paul Durrant > > > > Currently libxl__device_pci_add_xenstore() is broken in that does not > > update the domain's configuration for the first device added (which causes > > creation of the overall backend area in xenstore). This can be easily > > observed > > by running 'xl list -l' after adding a single device: the device will be > > missing. > > > > This patch fixes the problem and adds a DEBUG log line to allow easy > > verification that the domain configuration is being modified. Also, the use > > of libxl__device_generic_add() is dropped as it leads to a confusing > > situation > > where only partial backend information is written under the xenstore > > '/libxl' path. For LIBXL__DEVICE_KIND_PCI devices the only definitive > > information in xenstore is under '/local/domain/0/backend' (the '0' being > > hard-coded). > > > > NOTE: This patch includes a whitespace in add_pcis_done(). > > > > Signed-off-by: Paul Durrant > > --- > > Cc: Ian Jackson > > Cc: Wei Liu > > Cc: Anthony PERARD > > > > v2: > > - Avoid having two completely different ways of adding devices into > > xenstore > > > > v3: > > - Revert some changes form v2 as there is confusion over use of the libxl > > and backend xenstore paths which needs to be fixed > > --- > > tools/libs/light/libxl_pci.c | 87 > > +++- > > 1 file changed, 45 insertions(+), 42 deletions(-) > > > > diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c > > index 9d44b28f0a..da01c77ba2 100644 > > --- a/tools/libs/light/libxl_pci.c > > +++ b/tools/libs/light/libxl_pci.c > > @@ -79,39 +79,55 @@ static void libxl__device_from_pci(libxl__gc *gc, > > uint32_t domid, > > device->kind = LIBXL__DEVICE_KIND_PCI; > > } > > > > -static int libxl__create_pci_backend(libxl__gc *gc, uint32_t domid, > > - const libxl_device_pci *pci, > > - int num) > > +static void libxl__create_pci_backend(libxl__gc *gc, xs_transaction_t t, > > + uint32_t domid, const > > libxl_device_pci *pci) > > { > > -flexarray_t *front = NULL; > > -flexarray_t *back = NULL; > > -libxl__device device; > > -int i; > > +libxl_ctx *ctx = libxl__gc_owner(gc); > > +flexarray_t *front, *back; > > +char *fe_path, *be_path; > > +struct xs_permissions fe_perms[2], be_perms[2]; > > + > > +LOGD(DEBUG, domid, "Creating pci backend"); > > > > front = flexarray_make(gc, 16, 1); > > back = flexarray_make(gc, 16, 1); > > > > -LOGD(DEBUG, domid, "Creating pci backend"); > > - > > -/* add pci device */ > > -libxl__device_from_pci(gc, domid, pci, &device); > > +fe_path = libxl__domain_device_frontend_path(gc, domid, 0, > > + LIBXL__DEVICE_KIND_PCI); > > +be_path = libxl__domain_device_backend_path(gc, 0, domid, 0, > > +LIBXL__DEVICE_KIND_PCI); > > > > +flexarray_append_pair(back, "frontend", fe_path); > > flexarray_append_pair(back, "frontend-id", GCSPRINTF("%d", domid)); > > -flexarray_append_pair(back, "online", "1"); > > +flexarray_append_pair(back, "online", GCSPRINTF("%d", 1)); > > flexarray_append_pair(back, "state", GCSPRINTF("%d", > > XenbusStateInitialising)); > > flexarray_append_pair(back, "domain", libxl__domid_to_name(gc, > > domid)); > > > > -for (i = 0; i < num; i++, pci++) > > -libxl_create_pci_backend_device(gc, back, i, pci); > > +be_perms[0].id = 0; > > There was a discussion [1] on PCI on ARM and one of the question was that it > is possible > > that we have the pci backend running in a late hardware domain/driver domain, > which may > > not be Domain-0. Do you think we can avoid using 0 here and get some clue of > the domain > > from "*backend=domain-id"? If not set it will return Domain-0's ID and won't > break anything* Not sure what you're asking for since... > > *Thank you,* > > *Oleksandr > * > > > +be_perms[0].perms = XS_PERM_NONE; > > +be_perms[1].id = domid; > > +be_perms[1].perms = XS_PERM_READ; > > + > > +xs_rm(ctx->xsh, t, be_path); > > +xs_mkdir(ctx->xsh, t, be_path); > > +xs_set_permissions(ctx->xsh, t, be_path, be_perms, > > + ARRAY_SIZE(be_perms)); > > +libxl__xs_writev(gc, t, be_path, libxl__xs_kvs_of_flexarray(gc, back)); > > > > -flexarray_append_pair(back, "num_devs", GCSPRINTF("%d", num)); > > +flexarray_append_pair(front, "backen
Re: [PATCH v4 03/23] libxl: Make sure devices added by pci-attach are reflected in the config
On 12/3/20 3:17 PM, Paul Durrant wrote: >> -Original Message- >> From: Oleksandr Andrushchenko >> Sent: 01 December 2020 13:12 >> To: Paul Durrant ; xen-devel@lists.xenproject.org >> Cc: Paul Durrant ; Ian Jackson ; >> Wei Liu ; >> Anthony PERARD >> Subject: Re: [PATCH v4 03/23] libxl: Make sure devices added by pci-attach >> are reflected in the config >> >> Hi, Paul! >> >> On 11/24/20 10:01 AM, Paul Durrant wrote: >>> From: Paul Durrant >>> >>> Currently libxl__device_pci_add_xenstore() is broken in that does not >>> update the domain's configuration for the first device added (which causes >>> creation of the overall backend area in xenstore). This can be easily >>> observed >>> by running 'xl list -l' after adding a single device: the device will be >>> missing. >>> >>> This patch fixes the problem and adds a DEBUG log line to allow easy >>> verification that the domain configuration is being modified. Also, the use >>> of libxl__device_generic_add() is dropped as it leads to a confusing >>> situation >>> where only partial backend information is written under the xenstore >>> '/libxl' path. For LIBXL__DEVICE_KIND_PCI devices the only definitive >>> information in xenstore is under '/local/domain/0/backend' (the '0' being >>> hard-coded). >>> >>> NOTE: This patch includes a whitespace in add_pcis_done(). >>> >>> Signed-off-by: Paul Durrant >>> --- >>> Cc: Ian Jackson >>> Cc: Wei Liu >>> Cc: Anthony PERARD >>> >>> v2: >>>- Avoid having two completely different ways of adding devices into >>> xenstore >>> >>> v3: >>>- Revert some changes form v2 as there is confusion over use of the libxl >>> and backend xenstore paths which needs to be fixed >>> --- >>>tools/libs/light/libxl_pci.c | 87 >>> +++- >>>1 file changed, 45 insertions(+), 42 deletions(-) >>> >>> diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c >>> index 9d44b28f0a..da01c77ba2 100644 >>> --- a/tools/libs/light/libxl_pci.c >>> +++ b/tools/libs/light/libxl_pci.c >>> @@ -79,39 +79,55 @@ static void libxl__device_from_pci(libxl__gc *gc, >>> uint32_t domid, >>>device->kind = LIBXL__DEVICE_KIND_PCI; >>>} >>> >>> -static int libxl__create_pci_backend(libxl__gc *gc, uint32_t domid, >>> - const libxl_device_pci *pci, >>> - int num) >>> +static void libxl__create_pci_backend(libxl__gc *gc, xs_transaction_t t, >>> + uint32_t domid, const >>> libxl_device_pci *pci) >>>{ >>> -flexarray_t *front = NULL; >>> -flexarray_t *back = NULL; >>> -libxl__device device; >>> -int i; >>> +libxl_ctx *ctx = libxl__gc_owner(gc); >>> +flexarray_t *front, *back; >>> +char *fe_path, *be_path; >>> +struct xs_permissions fe_perms[2], be_perms[2]; >>> + >>> +LOGD(DEBUG, domid, "Creating pci backend"); >>> >>>front = flexarray_make(gc, 16, 1); >>>back = flexarray_make(gc, 16, 1); >>> >>> -LOGD(DEBUG, domid, "Creating pci backend"); >>> - >>> -/* add pci device */ >>> -libxl__device_from_pci(gc, domid, pci, &device); >>> +fe_path = libxl__domain_device_frontend_path(gc, domid, 0, >>> + LIBXL__DEVICE_KIND_PCI); >>> +be_path = libxl__domain_device_backend_path(gc, 0, domid, 0, >>> +LIBXL__DEVICE_KIND_PCI); >>> >>> +flexarray_append_pair(back, "frontend", fe_path); >>>flexarray_append_pair(back, "frontend-id", GCSPRINTF("%d", domid)); >>> -flexarray_append_pair(back, "online", "1"); >>> +flexarray_append_pair(back, "online", GCSPRINTF("%d", 1)); >>>flexarray_append_pair(back, "state", GCSPRINTF("%d", >>> XenbusStateInitialising)); >>>flexarray_append_pair(back, "domain", libxl__domid_to_name(gc, >>> domid)); >>> >>> -for (i = 0; i < num; i++, pci++) >>> -libxl_create_pci_backend_device(gc, back, i, pci); >>> +be_perms[0].id = 0; >> There was a discussion [1] on PCI on ARM and one of the question was that it >> is possible >> >> that we have the pci backend running in a late hardware domain/driver >> domain, which may >> >> not be Domain-0. Do you think we can avoid using 0 here and get some clue of >> the domain >> >> from "*backend=domain-id"? If not set it will return Domain-0's ID and won't >> break anything* > Not sure what you're asking for since... My bad, please ignore Reviewed-by: Oleksandr Andrushchenko Thank you, Oleksandr > >> *Thank you,* >> >> *Oleksandr >> * >> >>> +be_perms[0].perms = XS_PERM_NONE; >>> +be_perms[1].id = domid; >>> +be_perms[1].perms = XS_PERM_READ; >>> + >>> +xs_rm(ctx->xsh, t, be_path); >>> +xs_mkdir(ctx->xsh, t, be_path); >>> +xs_set_permissions(ctx->xsh, t, be_path, be_perms, >>> + ARRAY_SIZE(be_perms)); >>> +libxl__xs_writev(gc, t, be_path, li
Re: [PATCH v5 0/4] Xen ABI feature control
Acked-by: Christian Lindig From: Jürgen Groß Sent: Thursday, December 03, 2020 13:15 To: Paul Durrant; xen-devel@lists.xenproject.org Cc: Paul Durrant; Andrew Cooper; Anthony Perard; Christian Lindig; David Scott; George Dunlap; Ian Jackson; Jan Beulich; Julien Grall; Roger Pau Monne; Stefano Stabellini; Volodymyr Babchuk; Wei Liu Subject: Re: [PATCH v5 0/4] Xen ABI feature control On 03.12.20 13:41, Paul Durrant wrote: > From: Paul Durrant > > This series was previously called "evtchn: Introduce a per-guest knob to > control FIFO ABI". It is been extensively re-worked and extended to cover > another ABI feature. > > Paul Durrant (4): >domctl: introduce a new domain create flag, > XEN_DOMCTL_CDF_evtchn_fifo, ... >domctl: introduce a new domain create flag, > XEN_DOMCTL_CDF_evtchn_upcall, ... >libxl: introduce a 'libxl_xen_abi_features' enumeration... >xl: introduce a 'xen-abi-features' option... > > docs/man/xl.cfg.5.pod.in | 50 > tools/include/libxl.h| 10 +++ > tools/libs/light/libxl_arm.c | 22 +- > tools/libs/light/libxl_create.c | 31 > tools/libs/light/libxl_types.idl | 7 + > tools/libs/light/libxl_x86.c | 17 ++- > tools/ocaml/libs/xc/xenctrl.ml | 2 ++ > tools/ocaml/libs/xc/xenctrl.mli | 2 ++ > tools/xl/xl_parse.c | 50 ++-- > xen/arch/arm/domain.c| 3 +- > xen/arch/arm/domain_build.c | 3 +- > xen/arch/arm/setup.c | 3 +- > xen/arch/x86/domain.c| 8 + > xen/arch/x86/hvm/hvm.c | 3 ++ > xen/arch/x86/setup.c | 4 ++- > xen/common/domain.c | 3 +- > xen/common/event_channel.c | 24 +-- > xen/include/public/domctl.h | 6 +++- > 18 files changed, 229 insertions(+), 19 deletions(-) > --- > Cc: Andrew Cooper > Cc: Anthony PERARD > Cc: Christian Lindig > Cc: David Scott > Cc: George Dunlap > Cc: Ian Jackson > Cc: Jan Beulich > Cc: Julien Grall > Cc: "Roger Pau Monné" > Cc: Stefano Stabellini > Cc: Volodymyr Babchuk > Cc: Wei Liu > Do we want to add a create flag for each such feature, or would it be better to set options like those via hypfs? It would be fairly easy to ad dynamic hypfs paths, e.g.: /domain//abi-features/evtchn-fifo /domain//abi-features/evtchn-upcall which would have boolean type and could be set as long as the domain hasn't been started. xl support could even be rather generic, without the need to add coding to xl for each new feature. This is no objection to this series, but just an idea how to avoid extending the use of unstable interfaces. Thoughts? Juergen
Re: [PATCH] vpci/msix: exit early if MSI-X is disabled
On 02.12.2020 09:38, Jan Beulich wrote: > On 01.12.2020 18:40, Roger Pau Monne wrote: >> --- a/xen/drivers/vpci/msix.c >> +++ b/xen/drivers/vpci/msix.c >> @@ -357,7 +357,11 @@ static int msix_write(struct vcpu *v, unsigned long >> addr, unsigned int len, >> * so that it picks the new state. >> */ >> entry->masked = new_masked; >> -if ( !new_masked && msix->enabled && !msix->masked && >> entry->updated ) >> + >> +if ( !msix->enabled ) >> +break; >> + >> +if ( !new_masked && !msix->masked && entry->updated ) >> { >> /* >> * If MSI-X is enabled, the function mask is not active, the >> entry > > What about a "disabled" -> "enabled-but-masked" transition? This, > afaict, similarly won't trigger setting up of entries from > control_write(), and hence I'd expect the ASSERT() to similarly > trigger when subsequently an entry's mask bit gets altered. > > I'd also be fine making this further adjustment, if you agree, > but the one thing I haven't been able to fully convince myself of > is that there's then still no need to set ->updated to true. I've taken another look. I think setting ->updated (or something equivalent) is needed in that case, in order to not lose the setting of the entry mask bit. However, this would only defer the problem to control_write(): This would now need to call vpci_msix_arch_mask_entry() under suitable conditions, but avoid calling it when the entry is disabled or was never set up. No matter whether making the setting of ->updated conditional, or adding a conditional call in update_entry(), we'd need to evaluate whether the entry is currently disabled. Imo, instead of introducing a new arch hook for this, it's easier to make vpci_msix_arch_mask_entry() tolerate getting called on a disabled entry. Below my proposed alternative change. While writing the description I started wondering why we require address or data fields to have got written before the first unmask. I don't think the hardware imposes such a requirement; zeros would be used instead, whatever this means. Let's not forget that it's only the primary purpose of MSI/MSI-X to trigger interrupts. Forcing the writes to go elsewhere in memory is not forbidden from all I know, and could be used by a driver. IOW I think ->updated should start out as set to true. But of course vpci_msi_update() then would need to check the upper address bits and avoid setting up an interrupt if they're not 0xfee. And further arrangements would be needed to have the guest requested write actually get carried out correctly. Jan x86/vPCI: tolerate (un)masking a disabled MSI-X entry None of the four reasons causing vpci_msix_arch_mask_entry() to get called (there's just a single call site) are impossible or illegal prior to an entry actually having got set up: - the entry may remain masked (in this case, however, a prior masked -> unmasked transition would already not have worked), - MSI-X may not be enabled, - the global mask bit may be set, - the entry may not otherwise have been updated. Hence the function asserting that the entry was previously set up was simply wrong. Since the caller tracks the masked state (and setting up of an entry would only be effected when that software bit is clear), it's okay to skip both masking and unmasking requests in this case. Fixes: d6281be9d0145 ('vpci/msix: add MSI-X handlers') Reported-by: Manuel Bouyer Signed-off-by: Jan Beulich --- a/xen/arch/x86/hvm/vmsi.c +++ b/xen/arch/x86/hvm/vmsi.c @@ -840,8 +840,8 @@ void vpci_msi_arch_print(const struct vp void vpci_msix_arch_mask_entry(struct vpci_msix_entry *entry, const struct pci_dev *pdev, bool mask) { -ASSERT(entry->arch.pirq != INVALID_PIRQ); -vpci_mask_pirq(pdev->domain, entry->arch.pirq, mask); +if ( entry->arch.pirq != INVALID_PIRQ ) +vpci_mask_pirq(pdev->domain, entry->arch.pirq, mask); } int vpci_msix_arch_enable_entry(struct vpci_msix_entry *entry,
RE: [PATCH v5 0/4] Xen ABI feature control
> -Original Message- > From: Jürgen Groß > Sent: 03 December 2020 13:15 > To: Paul Durrant ; xen-devel@lists.xenproject.org > Cc: Paul Durrant ; Andrew Cooper > ; Anthony PERARD > ; Christian Lindig ; > David Scott > ; George Dunlap ; Ian Jackson > ; Jan > Beulich ; Julien Grall ; Roger Pau Monné > ; > Stefano Stabellini ; Volodymyr Babchuk > ; Wei Liu > > Subject: Re: [PATCH v5 0/4] Xen ABI feature control > > On 03.12.20 13:41, Paul Durrant wrote: > > From: Paul Durrant > > > > This series was previously called "evtchn: Introduce a per-guest knob to > > control FIFO ABI". It is been extensively re-worked and extended to cover > > another ABI feature. > > > > Paul Durrant (4): > >domctl: introduce a new domain create flag, > > XEN_DOMCTL_CDF_evtchn_fifo, ... > >domctl: introduce a new domain create flag, > > XEN_DOMCTL_CDF_evtchn_upcall, ... > >libxl: introduce a 'libxl_xen_abi_features' enumeration... > >xl: introduce a 'xen-abi-features' option... > > > > docs/man/xl.cfg.5.pod.in | 50 > > tools/include/libxl.h| 10 +++ > > tools/libs/light/libxl_arm.c | 22 +- > > tools/libs/light/libxl_create.c | 31 > > tools/libs/light/libxl_types.idl | 7 + > > tools/libs/light/libxl_x86.c | 17 ++- > > tools/ocaml/libs/xc/xenctrl.ml | 2 ++ > > tools/ocaml/libs/xc/xenctrl.mli | 2 ++ > > tools/xl/xl_parse.c | 50 ++-- > > xen/arch/arm/domain.c| 3 +- > > xen/arch/arm/domain_build.c | 3 +- > > xen/arch/arm/setup.c | 3 +- > > xen/arch/x86/domain.c| 8 + > > xen/arch/x86/hvm/hvm.c | 3 ++ > > xen/arch/x86/setup.c | 4 ++- > > xen/common/domain.c | 3 +- > > xen/common/event_channel.c | 24 +-- > > xen/include/public/domctl.h | 6 +++- > > 18 files changed, 229 insertions(+), 19 deletions(-) > > --- > > Cc: Andrew Cooper > > Cc: Anthony PERARD > > Cc: Christian Lindig > > Cc: David Scott > > Cc: George Dunlap > > Cc: Ian Jackson > > Cc: Jan Beulich > > Cc: Julien Grall > > Cc: "Roger Pau Monné" > > Cc: Stefano Stabellini > > Cc: Volodymyr Babchuk > > Cc: Wei Liu > > > > Do we want to add a create flag for each such feature, or would it be > better to set options like those via hypfs? > > It would be fairly easy to ad dynamic hypfs paths, e.g.: > > /domain//abi-features/evtchn-fifo > /domain//abi-features/evtchn-upcall > > which would have boolean type and could be set as long as the domain > hasn't been started. > > xl support could even be rather generic, without the need to add coding > to xl for each new feature. > > This is no objection to this series, but just an idea how to avoid > extending the use of unstable interfaces. > > Thoughts? > I was not aware we could have something that was dynamic only before I domain is started. We'd still want libxl to write the features rather than xl doing it directly I think as we still want it to be the owner of the default settings. Personally it still feels like this kind of setting does want to be an explicit part of domain creation, though using hypfs does sound like a neat idea. Paul > > Juergen
Re: [PATCH v5 0/4] Xen ABI feature control
On 03.12.20 14:51, Paul Durrant wrote: -Original Message- From: Jürgen Groß Sent: 03 December 2020 13:15 To: Paul Durrant ; xen-devel@lists.xenproject.org Cc: Paul Durrant ; Andrew Cooper ; Anthony PERARD ; Christian Lindig ; David Scott ; George Dunlap ; Ian Jackson ; Jan Beulich ; Julien Grall ; Roger Pau Monné ; Stefano Stabellini ; Volodymyr Babchuk ; Wei Liu Subject: Re: [PATCH v5 0/4] Xen ABI feature control On 03.12.20 13:41, Paul Durrant wrote: From: Paul Durrant This series was previously called "evtchn: Introduce a per-guest knob to control FIFO ABI". It is been extensively re-worked and extended to cover another ABI feature. Paul Durrant (4): domctl: introduce a new domain create flag, XEN_DOMCTL_CDF_evtchn_fifo, ... domctl: introduce a new domain create flag, XEN_DOMCTL_CDF_evtchn_upcall, ... libxl: introduce a 'libxl_xen_abi_features' enumeration... xl: introduce a 'xen-abi-features' option... docs/man/xl.cfg.5.pod.in | 50 tools/include/libxl.h| 10 +++ tools/libs/light/libxl_arm.c | 22 +- tools/libs/light/libxl_create.c | 31 tools/libs/light/libxl_types.idl | 7 + tools/libs/light/libxl_x86.c | 17 ++- tools/ocaml/libs/xc/xenctrl.ml | 2 ++ tools/ocaml/libs/xc/xenctrl.mli | 2 ++ tools/xl/xl_parse.c | 50 ++-- xen/arch/arm/domain.c| 3 +- xen/arch/arm/domain_build.c | 3 +- xen/arch/arm/setup.c | 3 +- xen/arch/x86/domain.c| 8 + xen/arch/x86/hvm/hvm.c | 3 ++ xen/arch/x86/setup.c | 4 ++- xen/common/domain.c | 3 +- xen/common/event_channel.c | 24 +-- xen/include/public/domctl.h | 6 +++- 18 files changed, 229 insertions(+), 19 deletions(-) --- Cc: Andrew Cooper Cc: Anthony PERARD Cc: Christian Lindig Cc: David Scott Cc: George Dunlap Cc: Ian Jackson Cc: Jan Beulich Cc: Julien Grall Cc: "Roger Pau Monné" Cc: Stefano Stabellini Cc: Volodymyr Babchuk Cc: Wei Liu Do we want to add a create flag for each such feature, or would it be better to set options like those via hypfs? It would be fairly easy to ad dynamic hypfs paths, e.g.: /domain//abi-features/evtchn-fifo /domain//abi-features/evtchn-upcall which would have boolean type and could be set as long as the domain hasn't been started. xl support could even be rather generic, without the need to add coding to xl for each new feature. This is no objection to this series, but just an idea how to avoid extending the use of unstable interfaces. Thoughts? I was not aware we could have something that was dynamic only before I domain is started. Look at my current cpupool/hypfs series: the per-cpupool scheduling granularity can be modified only if no cpu is assigned to the cpupool. We'd still want libxl to write the features rather than xl doing it directly I think as we still want it to be the owner of the default settings. Personally it still feels like this kind of setting does want to be an explicit part of domain creation, though using hypfs does sound like a neat idea. No problem of doing it in libxl. libxl is already using libxenhypfs. Juergen OpenPGP_0xB0DE9DD628BF132F.asc Description: application/pgp-keys OpenPGP_signature Description: OpenPGP digital signature
[PATCH v5 01/23] xl / libxl: s/pcidev/pci and remove DEFINE_DEVICE_TYPE_STRUCT_X
From: Paul Durrant The seemingly arbitrary use of 'pci' and 'pcidev' in the code in libxl_pci.c is confusing and also compromises use of some macros used for other device types. Indeed it seems that DEFINE_DEVICE_TYPE_STRUCT_X exists solely because of this duality. This patch purges use of 'pcidev' from the libxl code, allowing evaluation of DEFINE_DEVICE_TYPE_STRUCT_X to be replaced with DEFINE_DEVICE_TYPE_STRUCT, hence allowing removal of the former. For consistency the xl and libs/util code is also modified, but in this case it is purely cosmetic. NOTE: Some of the more gross formatting errors (such as lack of spaces after keywords) that came into context have been fixed in libxl_pci.c. Signed-off-by: Paul Durrant Reviewed-by: Oleksandr Andrushchenko --- Cc: Ian Jackson Cc: Wei Liu Cc: Anthony PERARD v5: - Minor cosmetic fix --- tools/include/libxl.h | 17 +- tools/libs/light/libxl_create.c | 6 +- tools/libs/light/libxl_dm.c | 18 +- tools/libs/light/libxl_internal.h | 45 ++- tools/libs/light/libxl_pci.c | 582 +++--- tools/libs/light/libxl_types.idl | 2 +- tools/libs/util/libxlu_pci.c | 36 +- tools/xl/xl_parse.c | 26 +- tools/xl/xl_pci.c | 68 ++-- tools/xl/xl_sxp.c | 12 +- 10 files changed, 408 insertions(+), 404 deletions(-) diff --git a/tools/include/libxl.h b/tools/include/libxl.h index 1ea5b4f446e8..fbe4c81ba511 100644 --- a/tools/include/libxl.h +++ b/tools/include/libxl.h @@ -444,6 +444,13 @@ */ #define LIBXL_HAVE_DISK_SAFE_REMOVE 1 +/* + * LIBXL_HAVE_CONFIG_PCIS indicates that the 'pcidevs' and 'num_pcidevs' + * fields in libxl_domain_config have been renamed to 'pcis' and 'num_pcis' + * respectively. + */ +#define LIBXL_HAVE_CONFIG_PCIS 1 + /* * libxl ABI compatibility * @@ -2300,15 +2307,15 @@ int libxl_device_pvcallsif_destroy(libxl_ctx *ctx, uint32_t domid, /* PCI Passthrough */ int libxl_device_pci_add(libxl_ctx *ctx, uint32_t domid, - libxl_device_pci *pcidev, + libxl_device_pci *pci, const libxl_asyncop_how *ao_how) LIBXL_EXTERNAL_CALLERS_ONLY; int libxl_device_pci_remove(libxl_ctx *ctx, uint32_t domid, -libxl_device_pci *pcidev, +libxl_device_pci *pci, const libxl_asyncop_how *ao_how) LIBXL_EXTERNAL_CALLERS_ONLY; int libxl_device_pci_destroy(libxl_ctx *ctx, uint32_t domid, - libxl_device_pci *pcidev, + libxl_device_pci *pci, const libxl_asyncop_how *ao_how) LIBXL_EXTERNAL_CALLERS_ONLY; @@ -2352,8 +2359,8 @@ int libxl_device_events_handler(libxl_ctx *ctx, * added or is not bound, the functions will emit a warning but return * SUCCESS. */ -int libxl_device_pci_assignable_add(libxl_ctx *ctx, libxl_device_pci *pcidev, int rebind); -int libxl_device_pci_assignable_remove(libxl_ctx *ctx, libxl_device_pci *pcidev, int rebind); +int libxl_device_pci_assignable_add(libxl_ctx *ctx, libxl_device_pci *pci, int rebind); +int libxl_device_pci_assignable_remove(libxl_ctx *ctx, libxl_device_pci *pci, int rebind); libxl_device_pci *libxl_device_pci_assignable_list(libxl_ctx *ctx, int *num); /* CPUID handling */ diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c index 321a13e519b5..1f5052c52033 100644 --- a/tools/libs/light/libxl_create.c +++ b/tools/libs/light/libxl_create.c @@ -1100,7 +1100,7 @@ int libxl__domain_config_setdefault(libxl__gc *gc, goto error_out; } -bool need_pt = d_config->num_pcidevs || d_config->num_dtdevs; +bool need_pt = d_config->num_pcis || d_config->num_dtdevs; if (c_info->passthrough == LIBXL_PASSTHROUGH_DEFAULT) { c_info->passthrough = need_pt ? LIBXL_PASSTHROUGH_ENABLED : LIBXL_PASSTHROUGH_DISABLED; @@ -1141,7 +1141,7 @@ int libxl__domain_config_setdefault(libxl__gc *gc, * assignment when PoD is enabled. */ if (d_config->c_info.type != LIBXL_DOMAIN_TYPE_PV && -d_config->num_pcidevs && pod_enabled) { +d_config->num_pcis && pod_enabled) { ret = ERROR_INVAL; LOGD(ERROR, domid, "PCI device assignment for HVM guest failed due to PoD enabled"); @@ -1817,7 +1817,7 @@ const libxl__device_type *device_type_tbl[] = { &libxl__vtpm_devtype, &libxl__usbctrl_devtype, &libxl__usbdev_devtype, -&libxl__pcidev_devtype, +&libxl__pci_devtype, &libxl__dtdev_devtype, &libxl__vdispl_devtype, &libxl__vsnd_devtype, diff --git a/tools/libs/light/libxl_dm.c b/tools/libs/light/libxl_dm.c index 3da83259c08e..8ebe1b60c9d7 100644 --- a/tools/libs/light/libxl_dm.c +++ b/tools/libs/light/libxl_dm.c @@ -442,7 +442,7 @@ int libxl__domain_de
[PATCH v5 02/23] libxl: make libxl__device_list() work correctly for LIBXL__DEVICE_KIND_PCI...
From: Paul Durrant ... devices. Currently there is an assumption built into libxl__device_list() that device backends are fully enumarated under the '/libxl' path in xenstore. This is not the case for PCI backend devices, which are only properly enumerated under '/local/domain/0/backend'. This patch adds a new get_path() method to libxl__device_type to allow a backend implementation (such as PCI) to specify the xenstore path where devices are enumerated and modifies libxl__device_list() to use this method if it is available. Also, if the get_num() method is defined then the from_xenstore() method expects to be passed the backend path without the device number concatenated, so this issue is also rectified. Having made libxl__device_list() work correctly, this patch removes the open-coded libxl_pci_device_pci_list() in favour of an evaluation of the LIBXL_DEFINE_DEVICE_LIST() macro. This has the side-effect of also defining libxl_pci_device_pci_list_free() which will be used in subsequent patches. Signed-off-by: Paul Durrant Reviewed-by: Oleksandr Andrushchenko --- Cc: Ian Jackson Cc: Wei Liu Cc: Anthony PERARD v3: - New in v3 (replacing "libxl: use LIBXL_DEFINE_DEVICE_LIST for pci devices") --- tools/include/libxl.h | 7 tools/libs/light/libxl_device.c | 70 --- tools/libs/light/libxl_internal.h | 2 + tools/libs/light/libxl_pci.c | 29 - 4 files changed, 54 insertions(+), 54 deletions(-) diff --git a/tools/include/libxl.h b/tools/include/libxl.h index fbe4c81ba511..ee52d3cf7e7e 100644 --- a/tools/include/libxl.h +++ b/tools/include/libxl.h @@ -451,6 +451,12 @@ */ #define LIBXL_HAVE_CONFIG_PCIS 1 +/* + * LIBXL_HAVE_DEVICE_PCI_LIST_FREE indicates that the + * libxl_device_pci_list_free() function is defined. + */ +#define LIBXL_HAVE_DEVICE_PCI_LIST_FREE 1 + /* * libxl ABI compatibility * @@ -2321,6 +2327,7 @@ int libxl_device_pci_destroy(libxl_ctx *ctx, uint32_t domid, libxl_device_pci *libxl_device_pci_list(libxl_ctx *ctx, uint32_t domid, int *num); +void libxl_device_pci_list_free(libxl_device_pci* list, int num); /* * Turns the current process into a backend device service daemon diff --git a/tools/libs/light/libxl_device.c b/tools/libs/light/libxl_device.c index e081faf9a94e..ac173a043d31 100644 --- a/tools/libs/light/libxl_device.c +++ b/tools/libs/light/libxl_device.c @@ -2011,7 +2011,7 @@ void *libxl__device_list(libxl__gc *gc, const libxl__device_type *dt, void *r = NULL; void *list = NULL; void *item = NULL; -char *libxl_path; +char *path; char **dir = NULL; unsigned int ndirs = 0; unsigned int ndevs = 0; @@ -2019,42 +2019,46 @@ void *libxl__device_list(libxl__gc *gc, const libxl__device_type *dt, *num = 0; -libxl_path = GCSPRINTF("%s/device/%s", - libxl__xs_libxl_path(gc, domid), - libxl__device_kind_to_string(dt->type)); +if (dt->get_path) { +rc = dt->get_path(gc, domid, &path); +if (rc) goto out; +} else { +path = GCSPRINTF("%s/device/%s", + libxl__xs_libxl_path(gc, domid), + libxl__device_kind_to_string(dt->type)); +} -dir = libxl__xs_directory(gc, XBT_NULL, libxl_path, &ndirs); - -if (dir && ndirs) { -if (dt->get_num) { -if (ndirs != 1) { -LOGD(ERROR, domid, "multiple entries in %s\n", libxl_path); -rc = ERROR_FAIL; -goto out; -} -rc = dt->get_num(gc, GCSPRINTF("%s/%s", libxl_path, *dir), &ndevs); -if (rc) goto out; -} else { +if (dt->get_num) { +rc = dt->get_num(gc, path, &ndevs); +if (rc) goto out; +} else { +dir = libxl__xs_directory(gc, XBT_NULL, path, &ndirs); +if (dir && ndirs) ndevs = ndirs; +} + +if (!ndevs) +return NULL; + +list = libxl__malloc(NOGC, dt->dev_elem_size * ndevs); +item = list; + +while (*num < ndevs) { +dt->init(item); + +if (dt->from_xenstore) { +int nr = dt->get_num ? *num : atoi(*dir); +char *device_path = dt->get_num ? path : +GCSPRINTF("%s/%d", path, nr); + +rc = dt->from_xenstore(gc, device_path, nr, item); +if (rc) goto out; } -list = libxl__malloc(NOGC, dt->dev_elem_size * ndevs); -item = list; -while (*num < ndevs) { -dt->init(item); - -if (dt->from_xenstore) { -int nr = dt->get_num ? *num : atoi(*dir); -char *device_libxl_path = GCSPRINTF("%s/%s", libxl_path, *dir); -rc = dt->from_xenstore(gc, device_libxl_path, nr, item); -if (rc) goto out; -} - -item = (uint8_t *)item + dt->dev_elem_size; -++(*num); -
[PATCH v5 07/23] libxl: stop using aodev->device_config in libxl__device_pci_add()...
From: Paul Durrant ... to hold a pointer to the device. There is already a 'pci' field in 'pci_add_state' so simply use that from the start. This also allows the 'pci' (#3) argument to be dropped from do_pci_add(). NOTE: This patch also changes the type of the 'pci_domid' field in 'pci_add_state' from 'int' to 'libxl_domid' which is more appropriate given what the field is used for. Signed-off-by: Paul Durrant Reviewed-by: Oleksandr Andrushchenko --- Cc: Ian Jackson Cc: Wei Liu --- tools/libs/light/libxl_pci.c | 19 +++ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c index 23f3f78992fd..31eaa95923c4 100644 --- a/tools/libs/light/libxl_pci.c +++ b/tools/libs/light/libxl_pci.c @@ -1074,7 +1074,7 @@ typedef struct pci_add_state { libxl__ev_qmp qmp; libxl__ev_time timeout; libxl_device_pci *pci; -int pci_domid; +libxl_domid pci_domid; } pci_add_state; static void pci_add_qemu_trad_watch_state_cb(libxl__egc *egc, @@ -1091,7 +1091,6 @@ static void pci_add_dm_done(libxl__egc *, static void do_pci_add(libxl__egc *egc, libxl_domid domid, - libxl_device_pci *pci, pci_add_state *pas) { STATE_AO_GC(pas->aodev->ao); @@ -1101,7 +1100,6 @@ static void do_pci_add(libxl__egc *egc, /* init pci_add_state */ libxl__xswait_init(&pas->xswait); libxl__ev_qmp_init(&pas->qmp); -pas->pci = pci; pas->pci_domid = domid; libxl__ev_time_init(&pas->timeout); @@ -1564,13 +1562,10 @@ void libxl__device_pci_add(libxl__egc *egc, uint32_t domid, int stubdomid = 0; pci_add_state *pas; -/* Store *pci to be used by callbacks */ -aodev->device_config = pci; -aodev->device_type = &libxl__pci_devtype; - GCNEW(pas); pas->aodev = aodev; pas->domid = domid; +pas->pci = pci; pas->starting = starting; pas->callback = device_pci_add_stubdom_done; @@ -1624,9 +1619,10 @@ void libxl__device_pci_add(libxl__egc *egc, uint32_t domid, GCNEW(pci_s); libxl_device_pci_init(pci_s); libxl_device_pci_copy(CTX, pci_s, pci); +pas->pci = pci_s; pas->callback = device_pci_add_stubdom_wait; -do_pci_add(egc, stubdomid, pci_s, pas); /* must be last */ +do_pci_add(egc, stubdomid, pas); /* must be last */ return; } @@ -1681,9 +1677,8 @@ static void device_pci_add_stubdom_done(libxl__egc *egc, int i; /* Convenience aliases */ -libxl__ao_device *aodev = pas->aodev; libxl_domid domid = pas->domid; -libxl_device_pci *pci = aodev->device_config; +libxl_device_pci *pci = pas->pci; if (rc) goto out; @@ -1718,7 +1713,7 @@ static void device_pci_add_stubdom_done(libxl__egc *egc, pci->vdevfn = orig_vdev; } pas->callback = device_pci_add_done; -do_pci_add(egc, domid, pci, pas); /* must be last */ +do_pci_add(egc, domid, pas); /* must be last */ return; } } @@ -1734,7 +1729,7 @@ static void device_pci_add_done(libxl__egc *egc, EGC_GC; libxl__ao_device *aodev = pas->aodev; libxl_domid domid = pas->domid; -libxl_device_pci *pci = aodev->device_config; +libxl_device_pci *pci = pas->pci; if (rc) { LOGD(ERROR, domid, -- 2.20.1
[PATCH v5 05/23] libxl: s/detatched/detached in libxl_pci.c
From: Paul Durrant Simply spelling correction. Purely cosmetic fix. Signed-off-by: Paul Durrant Reviewed-by: Oleksandr Andrushchenko --- Cc: Ian Jackson Cc: Wei Liu --- tools/libs/light/libxl_pci.c | 22 +++--- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c index a06c88076519..35ee810d5df3 100644 --- a/tools/libs/light/libxl_pci.c +++ b/tools/libs/light/libxl_pci.c @@ -1864,7 +1864,7 @@ static void pci_remove_qmp_query_cb(libxl__egc *egc, libxl__ev_qmp *qmp, const libxl__json_object *response, int rc); static void pci_remove_timeout(libxl__egc *egc, libxl__ev_time *ev, const struct timeval *requested_abs, int rc); -static void pci_remove_detatched(libxl__egc *egc, +static void pci_remove_detached(libxl__egc *egc, pci_remove_state *prs, int rc); static void pci_remove_stubdom_done(libxl__egc *egc, libxl__ao_device *aodev); @@ -1978,7 +1978,7 @@ skip1: skip_irq: rc = 0; out_fail: -pci_remove_detatched(egc, prs, rc); /* must be last */ +pci_remove_detached(egc, prs, rc); /* must be last */ } static void pci_remove_qemu_trad_watch_state_cb(libxl__egc *egc, @@ -2002,7 +2002,7 @@ static void pci_remove_qemu_trad_watch_state_cb(libxl__egc *egc, rc = qemu_pci_remove_xenstore(gc, domid, pci, prs->force); out: -pci_remove_detatched(egc, prs, rc); +pci_remove_detached(egc, prs, rc); } static void pci_remove_qmp_device_del(libxl__egc *egc, @@ -2028,7 +2028,7 @@ static void pci_remove_qmp_device_del(libxl__egc *egc, return; out: -pci_remove_detatched(egc, prs, rc); +pci_remove_detached(egc, prs, rc); } static void pci_remove_qmp_device_del_cb(libxl__egc *egc, @@ -2051,7 +2051,7 @@ static void pci_remove_qmp_device_del_cb(libxl__egc *egc, return; out: -pci_remove_detatched(egc, prs, rc); +pci_remove_detached(egc, prs, rc); } static void pci_remove_qmp_retry_timer_cb(libxl__egc *egc, libxl__ev_time *ev, @@ -2067,7 +2067,7 @@ static void pci_remove_qmp_retry_timer_cb(libxl__egc *egc, libxl__ev_time *ev, return; out: -pci_remove_detatched(egc, prs, rc); +pci_remove_detached(egc, prs, rc); } static void pci_remove_qmp_query_cb(libxl__egc *egc, @@ -2127,7 +2127,7 @@ static void pci_remove_qmp_query_cb(libxl__egc *egc, } out: -pci_remove_detatched(egc, prs, rc); /* must be last */ +pci_remove_detached(egc, prs, rc); /* must be last */ } static void pci_remove_timeout(libxl__egc *egc, libxl__ev_time *ev, @@ -2146,12 +2146,12 @@ static void pci_remove_timeout(libxl__egc *egc, libxl__ev_time *ev, /* If we timed out, we might still want to keep destroying the device * (when force==true), so let the next function decide what to do on * error */ -pci_remove_detatched(egc, prs, rc); +pci_remove_detached(egc, prs, rc); } -static void pci_remove_detatched(libxl__egc *egc, - pci_remove_state *prs, - int rc) +static void pci_remove_detached(libxl__egc *egc, +pci_remove_state *prs, +int rc) { STATE_AO_GC(prs->aodev->ao); int stubdomid = 0; -- 2.20.1
[PATCH v5 09/23] libxl: remove unnecessary check from libxl__device_pci_add()
From: Paul Durrant The code currently checks explicitly whether the device is already assigned, but this is actually unnecessary as assigned devices do not form part of the list returned by libxl_device_pci_assignable_list() and hence the libxl_pci_assignable() test would have already failed. Signed-off-by: Paul Durrant Reviewed-by: Oleksandr Andrushchenko --- Cc: Ian Jackson Cc: Wei Liu --- tools/libs/light/libxl_pci.c | 16 +--- 1 file changed, 1 insertion(+), 15 deletions(-) diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c index 57cf6ffc85de..24e79afcaa36 100644 --- a/tools/libs/light/libxl_pci.c +++ b/tools/libs/light/libxl_pci.c @@ -1555,8 +1555,7 @@ void libxl__device_pci_add(libxl__egc *egc, uint32_t domid, { STATE_AO_GC(aodev->ao); libxl_ctx *ctx = libxl__gc_owner(gc); -libxl_device_pci *assigned; -int num_assigned, rc; +int rc; int stubdomid = 0; pci_add_state *pas; @@ -1595,19 +1594,6 @@ void libxl__device_pci_add(libxl__egc *egc, uint32_t domid, goto out; } -rc = get_all_assigned_devices(gc, &assigned, &num_assigned); -if ( rc ) { -LOGD(ERROR, domid, - "cannot determine if device is assigned, refusing to continue"); -goto out; -} -if ( is_pci_in_array(assigned, num_assigned, pci->domain, - pci->bus, pci->dev, pci->func) ) { -LOGD(ERROR, domid, "PCI device already attached to a domain"); -rc = ERROR_FAIL; -goto out; -} - libxl__device_pci_reset(gc, pci->domain, pci->bus, pci->dev, pci->func); stubdomid = libxl_get_stubdom_id(ctx, domid); -- 2.20.1
[PATCH v5 08/23] libxl: generalise 'driver_path' xenstore access functions in libxl_pci.c
From: Paul Durrant For the purposes of re-binding a device to its previous driver libxl__device_pci_assignable_add() writes the driver path into xenstore. This path is then read back in libxl__device_pci_assignable_remove(). The functions that support this writing to and reading from xenstore are currently dedicated for this purpose and hence the node name 'driver_path' is hard-coded. This patch generalizes these utility functions and passes 'driver_path' as an argument. Subsequent patches will invoke them to access other nodes. NOTE: Because functions will have a broader use (other than storing a driver path in lieu of pciback) the base xenstore path is also changed from '/libxl/pciback' to '/libxl/pci'. Signed-off-by: Paul Durrant Reviewed-by: Oleksandr Andrushchenko --- Cc: Ian Jackson Cc: Wei Liu --- tools/libs/light/libxl_pci.c | 66 +--- 1 file changed, 32 insertions(+), 34 deletions(-) diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c index 31eaa95923c4..57cf6ffc85de 100644 --- a/tools/libs/light/libxl_pci.c +++ b/tools/libs/light/libxl_pci.c @@ -737,48 +737,46 @@ static int pciback_dev_unassign(libxl__gc *gc, libxl_device_pci *pci) return 0; } -#define PCIBACK_INFO_PATH "/libxl/pciback" +#define PCI_INFO_PATH "/libxl/pci" -static void pci_assignable_driver_path_write(libxl__gc *gc, -libxl_device_pci *pci, -char *driver_path) +static char *pci_info_xs_path(libxl__gc *gc, libxl_device_pci *pci, + const char *node) { -char *path; +return node ? +GCSPRINTF(PCI_INFO_PATH"/"PCI_BDF_XSPATH"/%s", + pci->domain, pci->bus, pci->dev, pci->func, + node) : +GCSPRINTF(PCI_INFO_PATH"/"PCI_BDF_XSPATH, + pci->domain, pci->bus, pci->dev, pci->func); +} -path = GCSPRINTF(PCIBACK_INFO_PATH"/"PCI_BDF_XSPATH"/driver_path", - pci->domain, - pci->bus, - pci->dev, - pci->func); -if ( libxl__xs_printf(gc, XBT_NULL, path, "%s", driver_path) < 0 ) { -LOGE(WARN, "Write of %s to node %s failed.", driver_path, path); + +static void pci_info_xs_write(libxl__gc *gc, libxl_device_pci *pci, + const char *node, const char *val) +{ +char *path = pci_info_xs_path(gc, pci, node); + +if ( libxl__xs_printf(gc, XBT_NULL, path, "%s", val) < 0 ) { +LOGE(WARN, "Write of %s to node %s failed.", val, path); } } -static char * pci_assignable_driver_path_read(libxl__gc *gc, - libxl_device_pci *pci) +static char *pci_info_xs_read(libxl__gc *gc, libxl_device_pci *pci, + const char *node) { -return libxl__xs_read(gc, XBT_NULL, - GCSPRINTF( - PCIBACK_INFO_PATH "/" PCI_BDF_XSPATH "/driver_path", - pci->domain, - pci->bus, - pci->dev, - pci->func)); +char *path = pci_info_xs_path(gc, pci, node); + +return libxl__xs_read(gc, XBT_NULL, path); } -static void pci_assignable_driver_path_remove(libxl__gc *gc, - libxl_device_pci *pci) +static void pci_info_xs_remove(libxl__gc *gc, libxl_device_pci *pci, + const char *node) { +char *path = pci_info_xs_path(gc, pci, node); libxl_ctx *ctx = libxl__gc_owner(gc); /* Remove the xenstore entry */ -xs_rm(ctx->xsh, XBT_NULL, - GCSPRINTF(PCIBACK_INFO_PATH "/" PCI_BDF_XSPATH, -pci->domain, -pci->bus, -pci->dev, -pci->func) ); +xs_rm(ctx->xsh, XBT_NULL, path); } static int libxl__device_pci_assignable_add(libxl__gc *gc, @@ -824,9 +822,9 @@ static int libxl__device_pci_assignable_add(libxl__gc *gc, /* Store driver_path for rebinding to dom0 */ if ( rebind ) { if ( driver_path ) { -pci_assignable_driver_path_write(gc, pci, driver_path); +pci_info_xs_write(gc, pci, "driver_path", driver_path); } else if ( (driver_path = - pci_assignable_driver_path_read(gc, pci)) != NULL ) { + pci_info_xs_read(gc, pci, "driver_path")) != NULL ) { LOG(INFO, PCI_BDF" not bound to a driver, will be rebound to %s", dom, bus, dev, func, driver_path); } else { @@ -834,7 +832,7 @@ static int libxl__device_pci_assignable_add(libxl__gc *gc, dom, bus, dev, func); } } else { -pci_assignable_driver_path_remove(gc, pci); +pci_info_xs_remove(gc, pci, "driver_path"); } if ( pciback_dev_assign(gc, pci) ) { @@ -884,7
[PATCH v5 03/23] libxl: Make sure devices added by pci-attach are reflected in the config
From: Paul Durrant Currently libxl__device_pci_add_xenstore() is broken in that does not update the domain's configuration for the first device added (which causes creation of the overall backend area in xenstore). This can be easily observed by running 'xl list -l' after adding a single device: the device will be missing. This patch fixes the problem and adds a DEBUG log line to allow easy verification that the domain configuration is being modified. Also, the use of libxl__device_generic_add() is dropped as it leads to a confusing situation where only partial backend information is written under the xenstore '/libxl' path. For LIBXL__DEVICE_KIND_PCI devices the only definitive information in xenstore is under '/local/domain/0/backend' (the '0' being hard-coded). NOTE: This patch includes a whitespace in add_pcis_done(). Signed-off-by: Paul Durrant Reviewed-by: Oleksandr Andrushchenko --- Cc: Ian Jackson Cc: Wei Liu Cc: Anthony PERARD v3: - Revert some changes form v2 as there is confusion over use of the libxl and backend xenstore paths which needs to be fixed v2: - Avoid having two completely different ways of adding devices into xenstore --- tools/libs/light/libxl_pci.c | 87 +++- 1 file changed, 45 insertions(+), 42 deletions(-) diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c index 757746a8dec1..aa7633dfef16 100644 --- a/tools/libs/light/libxl_pci.c +++ b/tools/libs/light/libxl_pci.c @@ -79,39 +79,55 @@ static void libxl__device_from_pci(libxl__gc *gc, uint32_t domid, device->kind = LIBXL__DEVICE_KIND_PCI; } -static int libxl__create_pci_backend(libxl__gc *gc, uint32_t domid, - const libxl_device_pci *pci, - int num) +static void libxl__create_pci_backend(libxl__gc *gc, xs_transaction_t t, + uint32_t domid, const libxl_device_pci *pci) { -flexarray_t *front = NULL; -flexarray_t *back = NULL; -libxl__device device; -int i; +libxl_ctx *ctx = libxl__gc_owner(gc); +flexarray_t *front, *back; +char *fe_path, *be_path; +struct xs_permissions fe_perms[2], be_perms[2]; + +LOGD(DEBUG, domid, "Creating pci backend"); front = flexarray_make(gc, 16, 1); back = flexarray_make(gc, 16, 1); -LOGD(DEBUG, domid, "Creating pci backend"); - -/* add pci device */ -libxl__device_from_pci(gc, domid, pci, &device); +fe_path = libxl__domain_device_frontend_path(gc, domid, 0, + LIBXL__DEVICE_KIND_PCI); +be_path = libxl__domain_device_backend_path(gc, 0, domid, 0, +LIBXL__DEVICE_KIND_PCI); +flexarray_append_pair(back, "frontend", fe_path); flexarray_append_pair(back, "frontend-id", GCSPRINTF("%d", domid)); -flexarray_append_pair(back, "online", "1"); +flexarray_append_pair(back, "online", GCSPRINTF("%d", 1)); flexarray_append_pair(back, "state", GCSPRINTF("%d", XenbusStateInitialising)); flexarray_append_pair(back, "domain", libxl__domid_to_name(gc, domid)); -for (i = 0; i < num; i++, pci++) -libxl_create_pci_backend_device(gc, back, i, pci); +be_perms[0].id = 0; +be_perms[0].perms = XS_PERM_NONE; +be_perms[1].id = domid; +be_perms[1].perms = XS_PERM_READ; -flexarray_append_pair(back, "num_devs", GCSPRINTF("%d", num)); +xs_rm(ctx->xsh, t, be_path); +xs_mkdir(ctx->xsh, t, be_path); +xs_set_permissions(ctx->xsh, t, be_path, be_perms, + ARRAY_SIZE(be_perms)); +libxl__xs_writev(gc, t, be_path, libxl__xs_kvs_of_flexarray(gc, back)); + +flexarray_append_pair(front, "backend", be_path); flexarray_append_pair(front, "backend-id", GCSPRINTF("%d", 0)); flexarray_append_pair(front, "state", GCSPRINTF("%d", XenbusStateInitialising)); -return libxl__device_generic_add(gc, XBT_NULL, &device, - libxl__xs_kvs_of_flexarray(gc, back), - libxl__xs_kvs_of_flexarray(gc, front), - NULL); +fe_perms[0].id = domid; +fe_perms[0].perms = XS_PERM_NONE; +fe_perms[1].id = 0; +fe_perms[1].perms = XS_PERM_READ; + +xs_rm(ctx->xsh, t, fe_path); +xs_mkdir(ctx->xsh, t, fe_path); +xs_set_permissions(ctx->xsh, t, fe_path, + fe_perms, ARRAY_SIZE(fe_perms)); +libxl__xs_writev(gc, t, fe_path, libxl__xs_kvs_of_flexarray(gc, front)); } static int libxl__device_pci_add_xenstore(libxl__gc *gc, @@ -135,8 +151,6 @@ static int libxl__device_pci_add_xenstore(libxl__gc *gc, be_path = libxl__domain_device_backend_path(gc, 0, domid, 0, LIBXL__DEVICE_KIND_PCI); num_devs = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/num_devs", be_path)); -if (!num_devs) -return libxl__cre
[PATCH v5 00/23] xl / libxl: named PCI pass-through devices
From: Paul Durrant Paul Durrant (23): xl / libxl: s/pcidev/pci and remove DEFINE_DEVICE_TYPE_STRUCT_X libxl: make libxl__device_list() work correctly for LIBXL__DEVICE_KIND_PCI... libxl: Make sure devices added by pci-attach are reflected in the config libxl: add/recover 'rdm_policy' to/from PCI backend in xenstore libxl: s/detatched/detached in libxl_pci.c libxl: remove extraneous arguments to do_pci_remove() in libxl_pci.c libxl: stop using aodev->device_config in libxl__device_pci_add()... libxl: generalise 'driver_path' xenstore access functions in libxl_pci.c libxl: remove unnecessary check from libxl__device_pci_add() libxl: remove get_all_assigned_devices() from libxl_pci.c libxl: make sure callers of libxl_device_pci_list() free the list after use libxl: add libxl_device_pci_assignable_list_free()... libxl: use COMPARE_PCI() macro is_pci_in_array()... docs/man: extract documentation of PCI_SPEC_STRING from the xl.cfg manpage... docs/man: improve documentation of PCI_SPEC_STRING... docs/man: fix xl(1) documentation for 'pci' operations libxl: introduce 'libxl_pci_bdf' in the idl... libxlu: introduce xlu_pci_parse_spec_string() libxl: modify libxl_device_pci_assignable_add/remove/list/list_free()... docs/man: modify xl(1) in preparation for naming of assignable devices xl / libxl: support naming of assignable devices docs/man: modify xl-pci-configuration(5) to add 'name' field to PCI_SPEC_STRING xl / libxl: support 'xl pci-attach/detach' by name docs/man/xl-pci-configuration.5.pod | 218 ++ docs/man/xl.1.pod.in | 39 +- docs/man/xl.cfg.5.pod.in | 68 +- tools/golang/xenlight/helpers.gen.go | 77 +- tools/golang/xenlight/types.gen.go |8 +- tools/include/libxl.h| 67 +- tools/include/libxlutil.h|8 +- tools/libs/light/libxl_create.c |6 +- tools/libs/light/libxl_device.c | 70 +- tools/libs/light/libxl_dm.c | 18 +- tools/libs/light/libxl_internal.h| 55 +- tools/libs/light/libxl_pci.c | 1048 ++ tools/libs/light/libxl_types.idl | 19 +- tools/libs/util/libxlu_pci.c | 379 +- tools/ocaml/libs/xl/xenlight_stubs.c | 19 +- tools/xl/xl_cmdtable.c | 16 +- tools/xl/xl_parse.c | 28 +- tools/xl/xl_pci.c| 159 ++-- tools/xl/xl_sxp.c| 12 +- 19 files changed, 1376 insertions(+), 938 deletions(-) create mode 100644 docs/man/xl-pci-configuration.5.pod -- 2.20.1
[PATCH v5 04/23] libxl: add/recover 'rdm_policy' to/from PCI backend in xenstore
From: Paul Durrant Other parameters, such as 'msitranslate' and 'permissive' are dealt with but 'rdm_policy' appears to be have been completely missed. Signed-off-by: Paul Durrant Reviewed-by: Oleksandr Andrushchenko --- Cc: Ian Jackson Cc: Wei Liu --- tools/libs/light/libxl_pci.c | 9 ++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c index aa7633dfef16..a06c88076519 100644 --- a/tools/libs/light/libxl_pci.c +++ b/tools/libs/light/libxl_pci.c @@ -61,9 +61,9 @@ static void libxl_create_pci_backend_device(libxl__gc *gc, flexarray_append_pair(back, GCSPRINTF("vdevfn-%d", num), GCSPRINTF("%x", pci->vdevfn)); flexarray_append(back, GCSPRINTF("opts-%d", num)); flexarray_append(back, - GCSPRINTF("msitranslate=%d,power_mgmt=%d,permissive=%d", - pci->msitranslate, pci->power_mgmt, - pci->permissive)); + GCSPRINTF("msitranslate=%d,power_mgmt=%d,permissive=%d,rdm_policy=%s", +pci->msitranslate, pci->power_mgmt, +pci->permissive, libxl_rdm_reserve_policy_to_string(pci->rdm_policy))); flexarray_append_pair(back, GCSPRINTF("state-%d", num), GCSPRINTF("%d", XenbusStateInitialising)); } @@ -2374,6 +2374,9 @@ static int libxl__device_pci_from_xs_be(libxl__gc *gc, } else if (!strcmp(p, "permissive")) { p = strtok_r(NULL, ",=", &saveptr); pci->permissive = atoi(p); +} else if (!strcmp(p, "rdm_policy")) { +p = strtok_r(NULL, ",=", &saveptr); +libxl_rdm_reserve_policy_from_string(p, &pci->rdm_policy); } } while ((p = strtok_r(NULL, ",=", &saveptr)) != NULL); } -- 2.20.1
[PATCH v5 06/23] libxl: remove extraneous arguments to do_pci_remove() in libxl_pci.c
From: Paul Durrant Both 'domid' and 'pci' are available in 'pci_remove_state' so there is no need to also pass them as separate arguments. Signed-off-by: Paul Durrant Reviewed-by: Oleksandr Andrushchenko --- Cc: Ian Jackson Cc: Wei Liu --- tools/libs/light/libxl_pci.c | 9 - 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c index 35ee810d5df3..23f3f78992fd 100644 --- a/tools/libs/light/libxl_pci.c +++ b/tools/libs/light/libxl_pci.c @@ -1871,14 +1871,14 @@ static void pci_remove_stubdom_done(libxl__egc *egc, static void pci_remove_done(libxl__egc *egc, pci_remove_state *prs, int rc); -static void do_pci_remove(libxl__egc *egc, uint32_t domid, - libxl_device_pci *pci, int force, - pci_remove_state *prs) +static void do_pci_remove(libxl__egc *egc, pci_remove_state *prs) { STATE_AO_GC(prs->aodev->ao); libxl_ctx *ctx = libxl__gc_owner(gc); libxl_device_pci *assigned; +uint32_t domid = prs->domid; libxl_domain_type type = libxl__domain_type(gc, domid); +libxl_device_pci *pci = prs->pci; int rc, num; uint32_t domainid = domid; @@ -2275,7 +2275,6 @@ static void device_pci_remove_common_next(libxl__egc *egc, EGC_GC; /* Convenience aliases */ -libxl_domid domid = prs->domid; libxl_device_pci *const pci = prs->pci; libxl__ao_device *const aodev = prs->aodev; const unsigned int pfunc_mask = prs->pfunc_mask; @@ -2293,7 +2292,7 @@ static void device_pci_remove_common_next(libxl__egc *egc, } else { pci->vdevfn = orig_vdev; } -do_pci_remove(egc, domid, pci, prs->force, prs); +do_pci_remove(egc, prs); return; } } -- 2.20.1
[PATCH v5 23/23] xl / libxl: support 'xl pci-attach/detach' by name
From: Paul Durrant This patch adds a 'name' field into the idl for 'libxl_device_pci' and libxlu_pci_parse_spec_string() is modified to parse the new 'name' parameter of PCI_SPEC_STRING detailed in the updated documention in xl-pci-configuration(5). If the 'name' field is non-NULL then both libxl_device_pci_add() and libxl_device_pci_remove() will use it to look up the device BDF in the list of assignable devices. Signed-off-by: Paul Durrant --- Cc: Ian Jackson Cc: Wei Liu Cc: Anthony PERARD --- tools/include/libxl.h| 6 +++ tools/libs/light/libxl_pci.c | 67 +--- tools/libs/light/libxl_types.idl | 1 + tools/libs/util/libxlu_pci.c | 7 +++- 4 files changed, 75 insertions(+), 6 deletions(-) diff --git a/tools/include/libxl.h b/tools/include/libxl.h index 4025d3a3d437..5b55a2015533 100644 --- a/tools/include/libxl.h +++ b/tools/include/libxl.h @@ -484,6 +484,12 @@ */ #define LIBXL_HAVE_PCI_ASSIGNABLE_NAME 1 +/* + * LIBXL_HAVE_DEVICE_PCI_NAME indicates that the 'name' field of + * libxl_device_pci is defined. + */ +#define LIBXL_HAVE_DEVICE_PCI_NAME 1 + /* * libxl ABI compatibility * diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c index e62383dd7b8f..74c3db26df05 100644 --- a/tools/libs/light/libxl_pci.c +++ b/tools/libs/light/libxl_pci.c @@ -60,6 +60,10 @@ static void libxl_create_pci_backend_device(libxl__gc *gc, int num, const libxl_device_pci *pci) { +if (pci->name) { +flexarray_append(back, GCSPRINTF("name-%d", num)); +flexarray_append(back, GCSPRINTF("%s", pci->name)); +} flexarray_append(back, GCSPRINTF("key-%d", num)); flexarray_append(back, GCSPRINTF(PCI_BDF, pci->bdf.domain, pci->bdf.bus, pci->bdf.dev, pci->bdf.func)); flexarray_append(back, GCSPRINTF("dev-%d", num)); @@ -284,6 +288,7 @@ retry_transaction: retry_transaction2: t = xs_transaction_start(ctx->xsh); +xs_rm(ctx->xsh, t, GCSPRINTF("%s/name-%d", be_path, i)); xs_rm(ctx->xsh, t, GCSPRINTF("%s/state-%d", be_path, i)); xs_rm(ctx->xsh, t, GCSPRINTF("%s/key-%d", be_path, i)); xs_rm(ctx->xsh, t, GCSPRINTF("%s/dev-%d", be_path, i)); @@ -322,6 +327,12 @@ retry_transaction2: xs_write(ctx->xsh, t, GCSPRINTF("%s/vdevfn-%d", be_path, j - 1), tmp, strlen(tmp)); xs_rm(ctx->xsh, t, tmppath); } +tmppath = GCSPRINTF("%s/name-%d", be_path, j); +tmp = libxl__xs_read(gc, t, tmppath); +if (tmp) { +xs_write(ctx->xsh, t, GCSPRINTF("%s/name-%d", be_path, j - 1), tmp, strlen(tmp)); +xs_rm(ctx->xsh, t, tmppath); +} } if (!xs_transaction_end(ctx->xsh, t, 0)) if (errno == EAGAIN) @@ -1619,6 +1630,23 @@ void libxl__device_pci_add(libxl__egc *egc, uint32_t domid, pas->starting = starting; pas->callback = device_pci_add_stubdom_done; +if (pci->name) { +libxl_pci_bdf *pcibdf = +libxl_device_pci_assignable_name2bdf(CTX, pci->name); + +if (!pcibdf) { +rc = ERROR_FAIL; +goto out; +} + +LOGD(DETAIL, domid, "'%s' -> %04x:%02x:%02x.%u", pci->name, + pcibdf->domain, pcibdf->bus, pcibdf->dev, pcibdf->func); + +libxl_pci_bdf_copy(CTX, &pci->bdf, pcibdf); +libxl_pci_bdf_dispose(pcibdf); +free(pcibdf); +} + if (libxl__domain_type(gc, domid) == LIBXL_DOMAIN_TYPE_HVM) { rc = xc_test_assign_device(ctx->xch, domid, pci_encode_bdf(&pci->bdf)); @@ -1767,11 +1795,19 @@ static void device_pci_add_done(libxl__egc *egc, libxl_device_pci *pci = &pas->pci; if (rc) { -LOGD(ERROR, domid, - "libxl__device_pci_add failed for " - "PCI device %x:%x:%x.%x (rc %d)", - pci->bdf.domain, pci->bdf.bus, pci->bdf.dev, pci->bdf.func, - rc); +if (pci->name) { +LOGD(ERROR, domid, + "libxl__device_pci_add failed for " + "PCI device '%s' (rc %d)", + pci->name, + rc); +} else { +LOGD(ERROR, domid, + "libxl__device_pci_add failed for " + "PCI device %x:%x:%x.%x (rc %d)", + pci->bdf.domain, pci->bdf.bus, pci->bdf.dev, pci->bdf.func, + rc); +} pci_info_xs_remove(gc, &pci->bdf, "domid"); } libxl_device_pci_dispose(pci); @@ -2288,6 +2324,23 @@ static void libxl__device_pci_remove_common(libxl__egc *egc, libxl__ev_time_init(&prs->timeout); libxl__ev_time_init(&prs->retry_timer); +if (pci->name) { +libxl_pci_bdf *pcibdf = +libxl_device_pci_assignable_name2bdf(CTX, pci->name); + +if (!pcibdf) { +rc = ERROR_FAIL; +goto out; +} + +LOGD(DETAIL, domid,
[PATCH v5 12/23] libxl: add libxl_device_pci_assignable_list_free()...
From: Paul Durrant ... to be used by callers of libxl_device_pci_assignable_list(). Currently there is no API for callers of libxl_device_pci_assignable_list() to free the list. The xl function pciassignable_list() calls libxl_device_pci_dispose() on each element of the returned list, but libxl_pci_assignable() in libxl_pci.c does not. Neither does the implementation of libxl_device_pci_assignable_list() call libxl_device_pci_init(). This patch adds the new API function, makes sure it is used everywhere and also modifies libxl_device_pci_assignable_list() to initialize list entries rather than just zeroing them. Signed-off-by: Paul Durrant Acked-by: Christian Lindig Reviewed-by: Oleksandr Andrushchenko --- Cc: Ian Jackson Cc: Wei Liu Cc: David Scott Cc: Anthony PERARD --- tools/include/libxl.h| 7 +++ tools/libs/light/libxl_pci.c | 14 -- tools/ocaml/libs/xl/xenlight_stubs.c | 3 +-- tools/xl/xl_pci.c| 3 +-- 4 files changed, 21 insertions(+), 6 deletions(-) diff --git a/tools/include/libxl.h b/tools/include/libxl.h index ee52d3cf7e7e..8225809d94a8 100644 --- a/tools/include/libxl.h +++ b/tools/include/libxl.h @@ -457,6 +457,12 @@ */ #define LIBXL_HAVE_DEVICE_PCI_LIST_FREE 1 +/* + * LIBXL_HAVE_DEVICE_PCI_ASSIGNABLE_LIST_FREE indicates that the + * libxl_device_pci_assignable_list_free() function is defined. + */ +#define LIBXL_HAVE_DEVICE_PCI_ASSIGNABLE_LIST_FREE 1 + /* * libxl ABI compatibility * @@ -2369,6 +2375,7 @@ int libxl_device_events_handler(libxl_ctx *ctx, int libxl_device_pci_assignable_add(libxl_ctx *ctx, libxl_device_pci *pci, int rebind); int libxl_device_pci_assignable_remove(libxl_ctx *ctx, libxl_device_pci *pci, int rebind); libxl_device_pci *libxl_device_pci_assignable_list(libxl_ctx *ctx, int *num); +void libxl_device_pci_assignable_list_free(libxl_device_pci *list, int num); /* CPUID handling */ int libxl_cpuid_parse_config(libxl_cpuid_policy_list *cpuid, const char* str); diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c index 5ef37fe8dfef..e42c2ba56b73 100644 --- a/tools/libs/light/libxl_pci.c +++ b/tools/libs/light/libxl_pci.c @@ -457,7 +457,7 @@ libxl_device_pci *libxl_device_pci_assignable_list(libxl_ctx *ctx, int *num) pcis = new; new = pcis + *num; -memset(new, 0, sizeof(*new)); +libxl_device_pci_init(new); pci_struct_fill(new, dom, bus, dev, func, 0); if (pci_info_xs_read(gc, new, "domid")) /* already assigned */ @@ -472,6 +472,16 @@ out: return pcis; } +void libxl_device_pci_assignable_list_free(libxl_device_pci *list, int num) +{ +int i; + +for (i = 0; i < num; i++) +libxl_device_pci_dispose(&list[i]); + +free(list); +} + /* Unbind device from its current driver, if any. If driver_path is non-NULL, * store the path to the original driver in it. */ static int sysfs_dev_unbind(libxl__gc *gc, libxl_device_pci *pci, @@ -1490,7 +1500,7 @@ static int libxl_pci_assignable(libxl_ctx *ctx, libxl_device_pci *pci) pcis[i].func == pci->func) break; } -free(pcis); +libxl_device_pci_assignable_list_free(pcis, num); return i != num; } diff --git a/tools/ocaml/libs/xl/xenlight_stubs.c b/tools/ocaml/libs/xl/xenlight_stubs.c index 1181971da4e7..352a00134d70 100644 --- a/tools/ocaml/libs/xl/xenlight_stubs.c +++ b/tools/ocaml/libs/xl/xenlight_stubs.c @@ -894,9 +894,8 @@ value stub_xl_device_pci_assignable_list(value ctx) Field(list, 1) = temp; temp = list; Store_field(list, 0, Val_device_pci(&c_list[i])); - libxl_device_pci_dispose(&c_list[i]); } - free(c_list); + libxl_device_pci_assignable_list_free(c_list, nb); CAMLreturn(list); } diff --git a/tools/xl/xl_pci.c b/tools/xl/xl_pci.c index 7c0f102ac7b7..f71498cbb570 100644 --- a/tools/xl/xl_pci.c +++ b/tools/xl/xl_pci.c @@ -164,9 +164,8 @@ static void pciassignable_list(void) for (i = 0; i < num; i++) { printf("%04x:%02x:%02x.%01x\n", pcis[i].domain, pcis[i].bus, pcis[i].dev, pcis[i].func); -libxl_device_pci_dispose(&pcis[i]); } -free(pcis); +libxl_device_pci_assignable_list_free(pcis, num); } int main_pciassignable_list(int argc, char **argv) -- 2.20.1
[PATCH v5 20/23] docs/man: modify xl(1) in preparation for naming of assignable devices
From: Paul Durrant A subsequent patch will introduce code to allow a name to be specified to 'xl pci-assignable-add' such that the assignable device may be referred to by than name in subsequent operations. Signed-off-by: Paul Durrant --- Cc: Ian Jackson Cc: Wei Liu --- docs/man/xl.1.pod.in | 19 --- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/docs/man/xl.1.pod.in b/docs/man/xl.1.pod.in index c5fbce3b5c4b..0822a5842835 100644 --- a/docs/man/xl.1.pod.in +++ b/docs/man/xl.1.pod.in @@ -1595,19 +1595,23 @@ List virtual network interfaces for a domain. =over 4 -=item B +=item B [I<-n>] List all the B of assignable PCI devices. See -L for more information. +L for more information. If the -n option is +specified then any name supplied when the device was made assignable +will also be displayed. These are devices in the system which are configured to be available for passthrough and are bound to a suitable PCI backend driver in domain 0 rather than a real driver. -=item B I +=item B [I<-n NAME>] I Make the device at B assignable to guests. See -L for more information. +L for more information. If the -n option is +supplied then the assignable device entry will the named with the +given B. This will bind the device to the pciback driver and assign it to the "quarantine domain". If it is already bound to a driver, it will @@ -1622,10 +1626,11 @@ not to do this on a device critical to domain 0's operation, such as storage controllers, network interfaces, or GPUs that are currently being used. -=item B [I<-r>] I +=item B [I<-r>] I|I -Make the device at B not assignable to guests. See -L for more information. +Make a device non-assignable to guests. The device may be identified +either by its B or the B supplied when the device was made +assignable. See L for more information. This will at least unbind the device from pciback, and re-assign it from the "quarantine domain" back to domain 0. If the -r -- 2.20.1
[PATCH v5 10/23] libxl: remove get_all_assigned_devices() from libxl_pci.c
From: Paul Durrant Use of this function is a very inefficient way to check whether a device has already been assigned. This patch adds code that saves the domain id in xenstore at the point of assignment, and removes it again when the device id de-assigned (or the domain is destroyed). It is then straightforward to check whether a device has been assigned by checking whether a device has a saved domain id. NOTE: To facilitate the xenstore check it is necessary to move the pci_info_xs_read() earlier in libxl_pci.c. To keep related functions together, the rest of the pci_info_xs_XXX() functions are moved too. Signed-off-by: Paul Durrant Reviewed-by: Oleksandr Andrushchenko --- Cc: Ian Jackson Cc: Wei Liu --- tools/libs/light/libxl_pci.c | 149 +-- 1 file changed, 55 insertions(+), 94 deletions(-) diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c index 24e79afcaa36..0aecf3a4a8e2 100644 --- a/tools/libs/light/libxl_pci.c +++ b/tools/libs/light/libxl_pci.c @@ -336,50 +336,6 @@ retry_transaction2: return 0; } -static int get_all_assigned_devices(libxl__gc *gc, libxl_device_pci **list, int *num) -{ -char **domlist; -unsigned int nd = 0, i; - -*list = NULL; -*num = 0; - -domlist = libxl__xs_directory(gc, XBT_NULL, "/local/domain", &nd); -for(i = 0; i < nd; i++) { -char *path, *num_devs; - -path = GCSPRINTF("/local/domain/0/backend/%s/%s/0/num_devs", - libxl__device_kind_to_string(LIBXL__DEVICE_KIND_PCI), - domlist[i]); -num_devs = libxl__xs_read(gc, XBT_NULL, path); -if ( num_devs ) { -int ndev = atoi(num_devs), j; -char *devpath, *bdf; - -for(j = 0; j < ndev; j++) { -devpath = GCSPRINTF("/local/domain/0/backend/%s/%s/0/dev-%u", - libxl__device_kind_to_string(LIBXL__DEVICE_KIND_PCI), -domlist[i], j); -bdf = libxl__xs_read(gc, XBT_NULL, devpath); -if ( bdf ) { -unsigned dom, bus, dev, func; -if ( sscanf(bdf, PCI_BDF, &dom, &bus, &dev, &func) != 4 ) -continue; - -*list = realloc(*list, sizeof(libxl_device_pci) * ((*num) + 1)); -if (*list == NULL) -return ERROR_NOMEM; -pci_struct_fill(*list + *num, dom, bus, dev, func, 0); -(*num)++; -} -} -} -} -libxl__ptr_add(gc, *list); - -return 0; -} - static int is_pci_in_array(libxl_device_pci *assigned, int num_assigned, int dom, int bus, int dev, int func) { @@ -427,19 +383,58 @@ static int sysfs_write_bdf(libxl__gc *gc, const char * sysfs_path, return 0; } +#define PCI_INFO_PATH "/libxl/pci" + +static char *pci_info_xs_path(libxl__gc *gc, libxl_device_pci *pci, + const char *node) +{ +return node ? +GCSPRINTF(PCI_INFO_PATH"/"PCI_BDF_XSPATH"/%s", + pci->domain, pci->bus, pci->dev, pci->func, + node) : +GCSPRINTF(PCI_INFO_PATH"/"PCI_BDF_XSPATH, + pci->domain, pci->bus, pci->dev, pci->func); +} + + +static int pci_info_xs_write(libxl__gc *gc, libxl_device_pci *pci, + const char *node, const char *val) +{ +char *path = pci_info_xs_path(gc, pci, node); +int rc = libxl__xs_printf(gc, XBT_NULL, path, "%s", val); + +if (rc) LOGE(WARN, "Write of %s to node %s failed.", val, path); + +return rc; +} + +static char *pci_info_xs_read(libxl__gc *gc, libxl_device_pci *pci, + const char *node) +{ +char *path = pci_info_xs_path(gc, pci, node); + +return libxl__xs_read(gc, XBT_NULL, path); +} + +static void pci_info_xs_remove(libxl__gc *gc, libxl_device_pci *pci, + const char *node) +{ +char *path = pci_info_xs_path(gc, pci, node); +libxl_ctx *ctx = libxl__gc_owner(gc); + +/* Remove the xenstore entry */ +xs_rm(ctx->xsh, XBT_NULL, path); +} + libxl_device_pci *libxl_device_pci_assignable_list(libxl_ctx *ctx, int *num) { GC_INIT(ctx); -libxl_device_pci *pcis = NULL, *new, *assigned; +libxl_device_pci *pcis = NULL, *new; struct dirent *de; DIR *dir; -int r, num_assigned; *num = 0; -r = get_all_assigned_devices(gc, &assigned, &num_assigned); -if (r) goto out; - dir = opendir(SYSFS_PCIBACK_DRIVER); if (NULL == dir) { if (errno == ENOENT) { @@ -455,9 +450,6 @@ libxl_device_pci *libxl_device_pci_assignable_list(libxl_ctx *ctx, int *num) if (sscanf(de->d_name, PCI_BDF, &dom, &bus, &dev, &func) != 4) continue; -if (is_pci_in_array(assigned, num_assigned, dom, bus, dev, func)) -
[PATCH v5 18/23] libxlu: introduce xlu_pci_parse_spec_string()
From: Paul Durrant This patch largely re-writes the code to parse a PCI_SPEC_STRING and enters it via the newly introduced function. The new parser also deals with 'bdf' and 'vslot' as non-positional paramaters, as per the documentation in xl-pci-configuration(5). The existing xlu_pci_parse_bdf() function remains, but now strictly parses BDF values. Some existing callers of xlu_pci_parse_bdf() are modified to call xlu_pci_parse_spec_string() as per the documentation in xl(1). NOTE: Usage text in xl_cmdtable.c and error messages are also modified appropriately. Fixes: d25cc3ec93eb ("libxl: workaround gcc 10.2 maybe-uninitialized warning") Signed-off-by: Paul Durrant --- Cc: Ian Jackson Cc: Wei Liu Cc: Anthony PERARD --- tools/include/libxlutil.h| 8 +- tools/libs/util/libxlu_pci.c | 374 +++ tools/xl/xl_cmdtable.c | 4 +- tools/xl/xl_parse.c | 4 +- tools/xl/xl_pci.c| 37 ++-- 5 files changed, 230 insertions(+), 197 deletions(-) diff --git a/tools/include/libxlutil.h b/tools/include/libxlutil.h index 92e35c546278..cdd6aab4f816 100644 --- a/tools/include/libxlutil.h +++ b/tools/include/libxlutil.h @@ -108,10 +108,16 @@ int xlu_disk_parse(XLU_Config *cfg, int nspecs, const char *const *specs, * resulting disk struct is used with libxl. */ +/* + * PCI BDF + */ +int xlu_pci_parse_bdf(XLU_Config *cfg, libxl_pci_bdf *bdf, const char *str); + /* * PCI specification parsing */ -int xlu_pci_parse_bdf(XLU_Config *cfg, libxl_device_pci *pcidev, const char *str); +int xlu_pci_parse_spec_string(XLU_Config *cfg, libxl_device_pci *pci, + const char *str); /* * RDM parsing diff --git a/tools/libs/util/libxlu_pci.c b/tools/libs/util/libxlu_pci.c index 5c107f264260..a8b6ce542736 100644 --- a/tools/libs/util/libxlu_pci.c +++ b/tools/libs/util/libxlu_pci.c @@ -1,5 +1,7 @@ #define _GNU_SOURCE +#include + #include "libxlu_internal.h" #include "libxlu_disk_l.h" #include "libxlu_disk_i.h" @@ -9,185 +11,213 @@ #define XLU__PCI_ERR(_c, _x, _a...) \ if((_c) && (_c)->report) fprintf((_c)->report, _x, ##_a) -static int hex_convert(const char *str, unsigned int *val, unsigned int mask) +static int parse_bdf(libxl_pci_bdf *bdfp, uint32_t *vfunc_maskp, + const char *str, const char **endp) { -unsigned long ret; -char *end; +const char *ptr = str; +unsigned int colons = 0; +unsigned int domain, bus, dev, func; +int n; -ret = strtoul(str, &end, 16); -if ( end == str || *end != '\0' ) -return -1; -if ( ret & ~mask ) -return -1; -*val = (unsigned int)ret & mask; -return 0; -} - -static int pci_struct_fill(libxl_device_pci *pci, unsigned int domain, - unsigned int bus, unsigned int dev, - unsigned int func, unsigned int vdevfn) -{ -pci->bdf.domain = domain; -pci->bdf.bus = bus; -pci->bdf.dev = dev; -pci->bdf.func = func; -pci->vdevfn = vdevfn; -return 0; -} - -#define STATE_DOMAIN0 -#define STATE_BUS 1 -#define STATE_DEV 2 -#define STATE_FUNC 3 -#define STATE_VSLOT 4 -#define STATE_OPTIONS_K 6 -#define STATE_OPTIONS_V 7 -#define STATE_TERMINAL 8 -#define STATE_TYPE 9 -#define STATE_RDM_STRATEGY 10 -#define STATE_RESERVE_POLICY11 -#define INVALID 0x -int xlu_pci_parse_bdf(XLU_Config *cfg, libxl_device_pci *pci, const char *str) -{ -unsigned state = STATE_DOMAIN; -unsigned dom = INVALID, bus = INVALID, dev = INVALID, func = INVALID, vslot = 0; -char *buf2, *tok, *ptr, *end, *optkey = NULL; - -if ( NULL == (buf2 = ptr = strdup(str)) ) -return ERROR_NOMEM; - -for(tok = ptr, end = ptr + strlen(ptr) + 1; ptr < end; ptr++) { -switch(state) { -case STATE_DOMAIN: -if ( *ptr == ':' ) { -state = STATE_BUS; -*ptr = '\0'; -if ( hex_convert(tok, &dom, 0x) ) -goto parse_error; -tok = ptr + 1; -} -break; -case STATE_BUS: -if ( *ptr == ':' ) { -state = STATE_DEV; -*ptr = '\0'; -if ( hex_convert(tok, &bus, 0xff) ) -goto parse_error; -tok = ptr + 1; -}else if ( *ptr == '.' ) { -state = STATE_FUNC; -*ptr = '\0'; -if ( dom & ~0xff ) -goto parse_error; -bus = dom; -dom = 0; -if ( hex_convert(tok, &dev, 0xff) ) -goto parse_error; -tok = ptr + 1; -} -break; -case STATE_DEV: -if ( *ptr == '.' ) { -state = STATE_FUNC; -*ptr = '\0'; -if ( hex_convert(tok, &dev, 0xff) ) -
[PATCH v5 13/23] libxl: use COMPARE_PCI() macro is_pci_in_array()...
From: Paul Durrant ... rather than an open-coded equivalent. This patch tidies up the is_pci_in_array() function, making it take a single 'libxl_device_pci' argument rather than separate domain, bus, device and function arguments. The already-available COMPARE_PCI() macro can then be used and it is also modified to return 'bool' rather than 'int'. The patch also modifies libxl_pci_assignable() to use is_pci_in_array() rather than a separate open-coded equivalent, and also modifies it to return a 'bool' rather than an 'int'. NOTE: The COMPARE_PCI() macro is also fixed to include the 'domain' in its comparison, which should always have been the case. Signed-off-by: Paul Durrant Reviewed-by: Oleksandr Andrushchenko --- Cc: Ian Jackson Cc: Wei Liu --- tools/libs/light/libxl_internal.h | 7 +++--- tools/libs/light/libxl_pci.c | 38 +++ 2 files changed, 17 insertions(+), 28 deletions(-) diff --git a/tools/libs/light/libxl_internal.h b/tools/libs/light/libxl_internal.h index ecee61b5419c..02f8a3179c44 100644 --- a/tools/libs/light/libxl_internal.h +++ b/tools/libs/light/libxl_internal.h @@ -4746,9 +4746,10 @@ void libxl__xcinfo2xlinfo(libxl_ctx *ctx, * devices have same identifier. */ #define COMPARE_DEVID(a, b) ((a)->devid == (b)->devid) #define COMPARE_DISK(a, b) (!strcmp((a)->vdev, (b)->vdev)) -#define COMPARE_PCI(a, b) ((a)->func == (b)->func &&\ - (a)->bus == (b)->bus && \ - (a)->dev == (b)->dev) +#define COMPARE_PCI(a, b) ((a)->domain == (b)->domain && \ + (a)->bus == (b)->bus && \ + (a)->dev == (b)->dev && \ + (a)->func == (b)->func) #define COMPARE_USB(a, b) ((a)->ctrl == (b)->ctrl && \ (a)->port == (b)->port) #define COMPARE_USBCTRL(a, b) ((a)->devid == (b)->devid) diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c index e42c2ba56b73..4bf4a168e032 100644 --- a/tools/libs/light/libxl_pci.c +++ b/tools/libs/light/libxl_pci.c @@ -336,24 +336,17 @@ retry_transaction2: return 0; } -static int is_pci_in_array(libxl_device_pci *assigned, int num_assigned, - int dom, int bus, int dev, int func) +static bool is_pci_in_array(libxl_device_pci *pcis, int num, +libxl_device_pci *pci) { int i; -for(i = 0; i < num_assigned; i++) { -if ( assigned[i].domain != dom ) -continue; -if ( assigned[i].bus != bus ) -continue; -if ( assigned[i].dev != dev ) -continue; -if ( assigned[i].func != func ) -continue; -return 1; +for (i = 0; i < num; i++) { +if (COMPARE_PCI(pci, &pcis[i])) +break; } -return 0; +return i < num; } /* Write the standard BDF into the sysfs path given by sysfs_path. */ @@ -1487,21 +1480,17 @@ int libxl_device_pci_add(libxl_ctx *ctx, uint32_t domid, return AO_INPROGRESS; } -static int libxl_pci_assignable(libxl_ctx *ctx, libxl_device_pci *pci) +static bool libxl_pci_assignable(libxl_ctx *ctx, libxl_device_pci *pci) { libxl_device_pci *pcis; -int num, i; +int num; +bool assignable; pcis = libxl_device_pci_assignable_list(ctx, &num); -for (i = 0; i < num; i++) { -if (pcis[i].domain == pci->domain && -pcis[i].bus == pci->bus && -pcis[i].dev == pci->dev && -pcis[i].func == pci->func) -break; -} +assignable = is_pci_in_array(pcis, num, pci); libxl_device_pci_assignable_list_free(pcis, num); -return i != num; + +return assignable; } static void device_pci_add_stubdom_wait(libxl__egc *egc, @@ -1834,8 +1823,7 @@ static void do_pci_remove(libxl__egc *egc, pci_remove_state *prs) goto out_fail; } -attached = is_pci_in_array(pcis, num, pci->domain, - pci->bus, pci->dev, pci->func); +attached = is_pci_in_array(pcis, num, pci); libxl_device_pci_list_free(pcis, num); rc = ERROR_INVAL; -- 2.20.1
[PATCH v5 11/23] libxl: make sure callers of libxl_device_pci_list() free the list after use
From: Paul Durrant A previous patch introduced libxl_device_pci_list_free() which should be used by callers of libxl_device_pci_list() to properly dispose of the exported 'libxl_device_pci' types and the free the memory holding them. Whilst all current callers do ensure the memory is freed, only the code in xl's pcilist() function actually calls libxl_device_pci_dispose(). As it stands this laxity does not lead to any memory leaks, but the simple addition of .e.g. a 'string' into the idl definition of 'libxl_device_pci' would lead to leaks. This patch makes sure all callers of libxl_device_pci_list() can call libxl_device_pci_list_free() by keeping copies of 'libxl_device_pci' structures inline in 'pci_add_state' and 'pci_remove_state' (and also making sure these are properly disposed at the end of the operations) rather than keeping pointers to the structures returned by libxl_device_pci_list(). Signed-off-by: Paul Durrant Reviewed-by: Oleksandr Andrushchenko --- Cc: Ian Jackson Cc: Wei Liu Cc: Anthony PERARD --- tools/libs/light/libxl_pci.c | 68 tools/xl/xl_pci.c| 3 +- 2 files changed, 38 insertions(+), 33 deletions(-) diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c index 0aecf3a4a8e2..5ef37fe8dfef 100644 --- a/tools/libs/light/libxl_pci.c +++ b/tools/libs/light/libxl_pci.c @@ -1025,7 +1025,7 @@ typedef struct pci_add_state { libxl__xswait_state xswait; libxl__ev_qmp qmp; libxl__ev_time timeout; -libxl_device_pci *pci; +libxl_device_pci pci; libxl_domid pci_domid; } pci_add_state; @@ -1097,7 +1097,7 @@ static void pci_add_qemu_trad_watch_state_cb(libxl__egc *egc, /* Convenience aliases */ libxl_domid domid = pas->domid; -libxl_device_pci *pci = pas->pci; +libxl_device_pci *pci = &pas->pci; rc = check_qemu_running(gc, domid, xswa, rc, state); if (rc == ERROR_NOT_READY) @@ -1118,7 +1118,7 @@ static void pci_add_qmp_device_add(libxl__egc *egc, pci_add_state *pas) /* Convenience aliases */ libxl_domid domid = pas->domid; -libxl_device_pci *pci = pas->pci; +libxl_device_pci *pci = &pas->pci; libxl__ev_qmp *const qmp = &pas->qmp; rc = libxl__ev_time_register_rel(ao, &pas->timeout, @@ -1199,7 +1199,7 @@ static void pci_add_qmp_query_pci_cb(libxl__egc *egc, int dev_slot, dev_func; /* Convenience aliases */ -libxl_device_pci *pci = pas->pci; +libxl_device_pci *pci = &pas->pci; if (rc) goto out; @@ -1300,7 +1300,7 @@ static void pci_add_dm_done(libxl__egc *egc, /* Convenience aliases */ bool starting = pas->starting; -libxl_device_pci *pci = pas->pci; +libxl_device_pci *pci = &pas->pci; bool hvm = libxl__domain_type(gc, domid) == LIBXL_DOMAIN_TYPE_HVM; libxl__ev_qmp_dispose(gc, &pas->qmp); @@ -1516,7 +1516,10 @@ void libxl__device_pci_add(libxl__egc *egc, uint32_t domid, GCNEW(pas); pas->aodev = aodev; pas->domid = domid; -pas->pci = pci; + +libxl_device_pci_copy(CTX, &pas->pci, pci); +pci = &pas->pci; + pas->starting = starting; pas->callback = device_pci_add_stubdom_done; @@ -1555,12 +1558,6 @@ void libxl__device_pci_add(libxl__egc *egc, uint32_t domid, stubdomid = libxl_get_stubdom_id(ctx, domid); if (stubdomid != 0) { -libxl_device_pci *pci_s; - -GCNEW(pci_s); -libxl_device_pci_init(pci_s); -libxl_device_pci_copy(CTX, pci_s, pci); -pas->pci = pci_s; pas->callback = device_pci_add_stubdom_wait; do_pci_add(egc, stubdomid, pas); /* must be last */ @@ -1619,7 +1616,7 @@ static void device_pci_add_stubdom_done(libxl__egc *egc, /* Convenience aliases */ libxl_domid domid = pas->domid; -libxl_device_pci *pci = pas->pci; +libxl_device_pci *pci = &pas->pci; if (rc) goto out; @@ -1670,7 +1667,7 @@ static void device_pci_add_done(libxl__egc *egc, EGC_GC; libxl__ao_device *aodev = pas->aodev; libxl_domid domid = pas->domid; -libxl_device_pci *pci = pas->pci; +libxl_device_pci *pci = &pas->pci; if (rc) { LOGD(ERROR, domid, @@ -1680,6 +1677,7 @@ static void device_pci_add_done(libxl__egc *egc, rc); pci_info_xs_remove(gc, pci, "domid"); } +libxl_device_pci_dispose(pci); aodev->rc = rc; aodev->callback(egc, aodev); } @@ -1770,7 +1768,7 @@ static int qemu_pci_remove_xenstore(libxl__gc *gc, uint32_t domid, typedef struct pci_remove_state { libxl__ao_device *aodev; libxl_domid domid; -libxl_device_pci *pci; +libxl_device_pci pci; bool force; bool hvm; unsigned int orig_vdev; @@ -1812,23 +1810,26 @@ static void do_pci_remove(libxl__egc *egc, pci_remove_state *prs) { STATE_AO_GC(prs->aodev->ao); libxl_ctx *ctx = libxl__gc_owner(gc); -libxl_device_pci *assigned; +libxl_device_pci *pcis; +bool attached; uint3
[PATCH v5 15/23] docs/man: improve documentation of PCI_SPEC_STRING...
From: Paul Durrant ... and prepare for adding support for non-positional parsing of 'bdf' and 'vslot' in a subsequent patch. Also document 'BDF' as a first-class parameter type and fix the documentation to state that the default value of 'rdm_policy' is actually 'strict', not 'relaxed', as can be seen in libxl__device_pci_setdefault(). Signed-off-by: Paul Durrant --- Cc: Ian Jackson Cc: Wei Liu --- docs/man/xl-pci-configuration.5.pod | 187 +++- 1 file changed, 153 insertions(+), 34 deletions(-) diff --git a/docs/man/xl-pci-configuration.5.pod b/docs/man/xl-pci-configuration.5.pod index 72a27bd95dec..4dd73bc498d6 100644 --- a/docs/man/xl-pci-configuration.5.pod +++ b/docs/man/xl-pci-configuration.5.pod @@ -6,32 +6,105 @@ xl-pci-configuration - XL PCI Configuration Syntax =head1 SYNTAX -This document specifies the format for B which is used by -the L pci configuration option, and related L commands. +This document specifies the format for B and B which are +used by the L pci configuration option, and related L +commands. -Each B has the form of -B<[:]BB:DD.F[@VSLOT],KEY=VALUE,KEY=VALUE,...> where: +A B has the following form: + +[:]BB:SS.F + +B is the domain number, B is the bus number, B is the device (or +slot) number, and B is the function number. This is the same scheme as +used in the output of L for the device in question. By default +L will omit the domain (B) if it is zero and hence a zero +value for domain may also be omitted when specifying a B. + +Each B has the one of the forms: =over 4 -=item B<[:]BB:DD.F> +[[@,][=,]* +[=,]* -Identifies the PCI device from the host perspective in the domain -(B), Bus (B), Device (B) and Function (B) syntax. This is -the same scheme as used in the output of B for the device in -question. +=back -Note: by default B will omit the domain (B) if it -is zero and it is optional here also. You may specify the function -(B) as B<*> to indicate all functions. +For example, these strings are equivalent: -=item B<@VSLOT> +=over 4 -Specifies the virtual slot where the guest will see this -device. This is equivalent to the B which the guest sees. In a -guest B and B are C<:00>. +36:00.0@20,seize=1 +36:00.0,vslot=20,seize=1 +bdf=36:00.0,vslot=20,seize=1 -=item B +=back + +More formally, the string is a series of comma-separated keyword/value +pairs, flags and positional parameters. Parameters which are not bare +keywords and which do not contain "=" symbols are assigned to the +positional parameters, in the order specified below. The positional +parameters may also be specified by name. + +Each parameter may be specified at most once, either as a positional +parameter or a named parameter. Default values apply if the parameter +is not specified, or if it is specified with an empty value (whether +positionally or explicitly). + +B: In context of B (see L), parameters other than +B will be ignored. + +=head1 Positional Parameters + +=over 4 + +=item B=I + +=over 4 + +=item Description + +This identifies the PCI device from the host perspective. + +In the context of a B you may specify the function (B) as +B<*> to indicate all functions of a multi-function device. + +=item Default Value + +None. This parameter is mandatory as it identifies the device. + +=back + +=item B=I + +=over 4 + +=item Description + +Specifies the virtual slot (device) number where the guest will see this +device. For example, running L in a Linux guest where B +was specified as C<8> would identify the device as C<00:08.0>. Virtual domain +and bus numbers are always 0. + +B This parameter is always parsed as a hexidecimal value. + +=item Default Value + +None. This parameter is not mandatory. An available B will be selected +if this parameter is not specified. + +=back + +=back + +=head1 Other Parameters and Flags + +=over 4 + +=item B=I + +=over 4 + +=item Description By default pciback only allows PV guests to write "known safe" values into PCI configuration space, likewise QEMU (both qemu-xen and @@ -46,33 +119,79 @@ more control over the device, which may have security or stability implications. It is recommended to only enable this option for trusted VMs under administrator's control. -=item B +=item Default Value + +0 + +=back + +=item B=I + +=over 4 + +=item Description Specifies that MSI-INTx translation should be turned on for the PCI device. When enabled, MSI-INTx translation will always enable MSI on -the PCI device regardless of whether the guest uses INTx or MSI. Some -device drivers, such as NVIDIA's, detect an inconsistency and do not +the PCI device regardless of whether the guest uses INTx or MSI. + +=item Default Value + +Some device drivers, such as NVIDIA's, detect an inconsistency and do not function when this option is enabled. Therefore the default is false (0). -=item B +=back -Tells B to automatically attempt to re-assign a device to -pciback if it is not already a
[PATCH v5 16/23] docs/man: fix xl(1) documentation for 'pci' operations
From: Paul Durrant Currently the documentation completely fails to mention the existence of PCI_SPEC_STRING. This patch tidies things up, specifically clarifying that 'pci-assignable-add/remove' take arguments where as 'pci-attach/detach' take arguments (which will be enforced in a subsequent patch). Signed-off-by: Paul Durrant --- Cc: Ian Jackson Cc: Wei Liu --- docs/man/xl.1.pod.in | 28 +--- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/docs/man/xl.1.pod.in b/docs/man/xl.1.pod.in index f92bacfa7277..c5fbce3b5c4b 100644 --- a/docs/man/xl.1.pod.in +++ b/docs/man/xl.1.pod.in @@ -1597,14 +1597,18 @@ List virtual network interfaces for a domain. =item B -List all the assignable PCI devices. +List all the B of assignable PCI devices. See +L for more information. + These are devices in the system which are configured to be available for passthrough and are bound to a suitable PCI backend driver in domain 0 rather than a real driver. =item B I -Make the device at PCI Bus/Device/Function BDF assignable to guests. +Make the device at B assignable to guests. See +L for more information. + This will bind the device to the pciback driver and assign it to the "quarantine domain". If it is already bound to a driver, it will first be unbound, and the original driver stored so that it can be @@ -1620,8 +1624,10 @@ being used. =item B [I<-r>] I -Make the device at PCI Bus/Device/Function BDF not assignable to -guests. This will at least unbind the device from pciback, and +Make the device at B not assignable to guests. See +L for more information. + +This will at least unbind the device from pciback, and re-assign it from the "quarantine domain" back to domain 0. If the -r option is specified, it will also attempt to re-bind the device to its original driver, making it usable by Domain 0 again. If the device is @@ -1637,15 +1643,15 @@ As always, this should only be done if you trust the guest, or are confident that the particular device you're re-assigning to dom0 will cancel all in-flight DMA on FLR. -=item B I I +=item B I I -Hot-plug a new pass-through pci device to the specified domain. -B is the PCI Bus/Device/Function of the physical device to pass-through. +Hot-plug a new pass-through pci device to the specified domain. See +L for more information. -=item B [I] I I +=item B [I] I I -Hot-unplug a previously assigned pci device from a domain. B is the PCI -Bus/Device/Function of the physical device to be removed from the guest domain. +Hot-unplug a pci device that was previously passed through to a domain. See +L for more information. B @@ -1660,7 +1666,7 @@ even without guest domain's collaboration. =item B I -List pass-through pci devices for a domain. +List the B of pci devices passed through to a domain. =back -- 2.20.1
[PATCH v5 14/23] docs/man: extract documentation of PCI_SPEC_STRING from the xl.cfg manpage...
From: Paul Durrant ... and put it into a new xl-pci-configuration(5) manpage, akin to the xl-network-configration(5) and xl-disk-configuration(5) manpages. This patch moves the content of the section verbatim. A subsequent patch will improve the documentation, once it is in its new location. Signed-off-by: Paul Durrant Reviewed-by: Oleksandr Andrushchenko --- Cc: Ian Jackson Cc: Wei Liu --- docs/man/xl-pci-configuration.5.pod | 78 + docs/man/xl.cfg.5.pod.in| 68 + 2 files changed, 79 insertions(+), 67 deletions(-) create mode 100644 docs/man/xl-pci-configuration.5.pod diff --git a/docs/man/xl-pci-configuration.5.pod b/docs/man/xl-pci-configuration.5.pod new file mode 100644 index ..72a27bd95dec --- /dev/null +++ b/docs/man/xl-pci-configuration.5.pod @@ -0,0 +1,78 @@ +=encoding utf8 + +=head1 NAME + +xl-pci-configuration - XL PCI Configuration Syntax + +=head1 SYNTAX + +This document specifies the format for B which is used by +the L pci configuration option, and related L commands. + +Each B has the form of +B<[:]BB:DD.F[@VSLOT],KEY=VALUE,KEY=VALUE,...> where: + +=over 4 + +=item B<[:]BB:DD.F> + +Identifies the PCI device from the host perspective in the domain +(B), Bus (B), Device (B) and Function (B) syntax. This is +the same scheme as used in the output of B for the device in +question. + +Note: by default B will omit the domain (B) if it +is zero and it is optional here also. You may specify the function +(B) as B<*> to indicate all functions. + +=item B<@VSLOT> + +Specifies the virtual slot where the guest will see this +device. This is equivalent to the B which the guest sees. In a +guest B and B are C<:00>. + +=item B + +By default pciback only allows PV guests to write "known safe" values +into PCI configuration space, likewise QEMU (both qemu-xen and +qemu-xen-traditional) imposes the same constraint on HVM guests. +However, many devices require writes to other areas of the configuration space +in order to operate properly. This option tells the backend (pciback or QEMU) +to allow all writes to the PCI configuration space of this device by this +domain. + +B it gives the guest much +more control over the device, which may have security or stability +implications. It is recommended to only enable this option for +trusted VMs under administrator's control. + +=item B + +Specifies that MSI-INTx translation should be turned on for the PCI +device. When enabled, MSI-INTx translation will always enable MSI on +the PCI device regardless of whether the guest uses INTx or MSI. Some +device drivers, such as NVIDIA's, detect an inconsistency and do not +function when this option is enabled. Therefore the default is false (0). + +=item B + +Tells B to automatically attempt to re-assign a device to +pciback if it is not already assigned. + +B If you set this option, B will gladly re-assign a critical +system device, such as a network or a disk controller being used by +dom0 without confirmation. Please use with care. + +=item B + +B<(HVM only)> Specifies that the VM should be able to program the +D0-D3hot power management states for the PCI device. The default is false (0). + +=item B + +B<(HVM/x86 only)> This is the same as the policy setting inside the B +option but just specific to a given device. The default is "relaxed". + +Note: this would override global B option. + +=back diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in index 0532739c1fff..b00644e852f9 100644 --- a/docs/man/xl.cfg.5.pod.in +++ b/docs/man/xl.cfg.5.pod.in @@ -1101,73 +1101,7 @@ option is valid only when the B option is specified. =item B Specifies the host PCI devices to passthrough to this guest. -Each B has the form of -B<[:]BB:DD.F[@VSLOT],KEY=VALUE,KEY=VALUE,...> where: - -=over 4 - -=item B<[:]BB:DD.F> - -Identifies the PCI device from the host perspective in the domain -(B), Bus (B), Device (B) and Function (B) syntax. This is -the same scheme as used in the output of B for the device in -question. - -Note: by default B will omit the domain (B) if it -is zero and it is optional here also. You may specify the function -(B) as B<*> to indicate all functions. - -=item B<@VSLOT> - -Specifies the virtual slot where the guest will see this -device. This is equivalent to the B which the guest sees. In a -guest B and B are C<:00>. - -=item B - -By default pciback only allows PV guests to write "known safe" values -into PCI configuration space, likewise QEMU (both qemu-xen and -qemu-xen-traditional) imposes the same constraint on HVM guests. -However, many devices require writes to other areas of the configuration space -in order to operate properly. This option tells the backend (pciback or QEMU) -to allow all writes to the PCI configuration space of this device by this -domain. - -B it gives the guest much -more control over the device, which may have security or stability -implications. It is recomme
[PATCH v5 17/23] libxl: introduce 'libxl_pci_bdf' in the idl...
From: Paul Durrant ... and use in 'libxl_device_pci' This patch is preparatory work for restricting the type passed to functions that only require BDF information, rather than passing a 'libxl_device_pci' structure which is only partially filled. In this patch only the minimal mechanical changes necessary to deal with the structural changes are made. Subsequent patches will adjust the code to make better use of the new type. Signed-off-by: Paul Durrant --- Cc: George Dunlap Cc: Nick Rosbrook Cc: Ian Jackson Cc: Wei Liu Cc: Anthony PERARD --- tools/golang/xenlight/helpers.gen.go | 77 ++ tools/golang/xenlight/types.gen.go | 8 +- tools/include/libxl.h| 6 ++ tools/libs/light/libxl_dm.c | 8 +- tools/libs/light/libxl_internal.h| 3 +- tools/libs/light/libxl_pci.c | 148 +-- tools/libs/light/libxl_types.idl | 16 +-- tools/libs/util/libxlu_pci.c | 8 +- tools/xl/xl_pci.c| 6 +- tools/xl/xl_sxp.c| 4 +- 10 files changed, 167 insertions(+), 117 deletions(-) diff --git a/tools/golang/xenlight/helpers.gen.go b/tools/golang/xenlight/helpers.gen.go index c8605994e7fe..b7230f693c69 100644 --- a/tools/golang/xenlight/helpers.gen.go +++ b/tools/golang/xenlight/helpers.gen.go @@ -1999,6 +1999,41 @@ xc.colo_checkpoint_port = C.CString(x.ColoCheckpointPort)} return nil } +// NewPciBdf returns an instance of PciBdf initialized with defaults. +func NewPciBdf() (*PciBdf, error) { +var ( +x PciBdf +xc C.libxl_pci_bdf) + +C.libxl_pci_bdf_init(&xc) +defer C.libxl_pci_bdf_dispose(&xc) + +if err := x.fromC(&xc); err != nil { +return nil, err } + +return &x, nil} + +func (x *PciBdf) fromC(xc *C.libxl_pci_bdf) error { + x.Func = byte(xc._func) +x.Dev = byte(xc.dev) +x.Bus = byte(xc.bus) +x.Domain = int(xc.domain) + + return nil} + +func (x *PciBdf) toC(xc *C.libxl_pci_bdf) (err error){defer func(){ +if err != nil{ +C.libxl_pci_bdf_dispose(xc)} +}() + +xc._func = C.uint8_t(x.Func) +xc.dev = C.uint8_t(x.Dev) +xc.bus = C.uint8_t(x.Bus) +xc.domain = C.int(x.Domain) + + return nil + } + // NewDevicePci returns an instance of DevicePci initialized with defaults. func NewDevicePci() (*DevicePci, error) { var ( @@ -2014,10 +2049,9 @@ return nil, err } return &x, nil} func (x *DevicePci) fromC(xc *C.libxl_device_pci) error { - x.Func = byte(xc._func) -x.Dev = byte(xc.dev) -x.Bus = byte(xc.bus) -x.Domain = int(xc.domain) + if err := x.Bdf.fromC(&xc.bdf);err != nil { +return fmt.Errorf("converting field Bdf: %v", err) +} x.Vdevfn = uint32(xc.vdevfn) x.VfuncMask = uint32(xc.vfunc_mask) x.Msitranslate = bool(xc.msitranslate) @@ -2033,10 +2067,9 @@ if err != nil{ C.libxl_device_pci_dispose(xc)} }() -xc._func = C.uint8_t(x.Func) -xc.dev = C.uint8_t(x.Dev) -xc.bus = C.uint8_t(x.Bus) -xc.domain = C.int(x.Domain) +if err := x.Bdf.toC(&xc.bdf); err != nil { +return fmt.Errorf("converting field Bdf: %v", err) +} xc.vdevfn = C.uint32_t(x.Vdevfn) xc.vfunc_mask = C.uint32_t(x.VfuncMask) xc.msitranslate = C.bool(x.Msitranslate) @@ -2766,13 +2799,13 @@ if err := x.Nics[i].fromC(&v); err != nil { return fmt.Errorf("converting field Nics: %v", err) } } } -x.Pcidevs = nil -if n := int(xc.num_pcidevs); n > 0 { -cPcidevs := (*[1<<28]C.libxl_device_pci)(unsafe.Pointer(xc.pcidevs))[:n:n] -x.Pcidevs = make([]DevicePci, n) -for i, v := range cPcidevs { -if err := x.Pcidevs[i].fromC(&v); err != nil { -return fmt.Errorf("converting field Pcidevs: %v", err) } +x.Pcis = nil +if n := int(xc.num_pcis); n > 0 { +cPcis := (*[1<<28]C.libxl_device_pci)(unsafe.Pointer(xc.pcis))[:n:n] +x.Pcis = make([]DevicePci, n) +for i, v := range cPcis { +if err := x.Pcis[i].fromC(&v); err != nil { +return fmt.Errorf("converting field Pcis: %v", err) } } } x.Rdms = nil @@ -2922,13 +2955,13 @@ return fmt.Errorf("converting field Nics: %v", err) } } } -if numPcidevs := len(x.Pcidevs); numPcidevs > 0 { -xc.pcidevs = (*C.libxl_device_pci)(C.malloc(C.ulong(numPcidevs)*C.sizeof_libxl_device_pci)) -xc.num_pcidevs = C.int(numPcidevs) -cPcidevs := (*[1<<28]C.libxl_device_pci)(unsafe.Pointer(xc.pcidevs))[:numPcidevs:numPcidevs] -for i,v := range x.Pcidevs { -if err := v.toC(&cPcidevs[i]); err != nil { -return fmt.Errorf("converting field Pcidevs: %v", err) +if numPcis := len(x.Pcis); numPcis > 0 { +xc.pcis = (*C.libxl_device_pci)(C.malloc(C.ulong(numPcis)*C.sizeof_libxl_device_pci)) +xc.num_pcis = C.int(numPcis) +cPcis := (*[1<<28]C.libxl_device_pci)(unsafe.Pointer(xc.pcis))[:numPcis:numPcis] +for i,v := range x.Pcis { +if err := v.toC(&cPcis[i]); err != nil { +return fmt.Errorf("converting field Pcis: %v", err) } } } diff --git a/tools/golang/xenlight/types.gen.go b/tools/golang/xenlight/types.gen.go index b4c5df0f2c5c..bc62ae8ce9d1 100644 --- a/tools/golang/xenlight/types.gen.go +++ b/tools/golang/xenlight/types.gen.go @@ -707,11 +707,15 @@ ColoCheckpointHost string ColoCheckpointPort string } -type
[PATCH v5 22/23] docs/man: modify xl-pci-configuration(5) to add 'name' field to PCI_SPEC_STRING
From: Paul Durrant Since assignable devices can be named, a subsequent patch will support use of a PCI_SPEC_STRING containing a 'name' parameter instead of a 'bdf'. In this case the name will be used to look up the 'bdf' in the list of assignable (or assigned) devices. Signed-off-by: Paul Durrant --- Cc: Ian Jackson Cc: Wei Liu --- docs/man/xl-pci-configuration.5.pod | 25 +++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/docs/man/xl-pci-configuration.5.pod b/docs/man/xl-pci-configuration.5.pod index 4dd73bc498d6..db3360307cbd 100644 --- a/docs/man/xl-pci-configuration.5.pod +++ b/docs/man/xl-pci-configuration.5.pod @@ -51,7 +51,7 @@ is not specified, or if it is specified with an empty value (whether positionally or explicitly). B: In context of B (see L), parameters other than -B will be ignored. +B or B will be ignored. =head1 Positional Parameters @@ -70,7 +70,11 @@ B<*> to indicate all functions of a multi-function device. =item Default Value -None. This parameter is mandatory as it identifies the device. +None. This parameter is mandatory in its positional form. As a non-positional +parameter it is also mandatory unless a B parameter is present, in +which case B must not be present since the B will be used to find +the B in the list of assignable devices. See L for more information +on naming assignable devices. =back @@ -194,4 +198,21 @@ B: This overrides the global B option. =back +=item B=I + +=over 4 + +=item Description + +This is the name given when the B was made assignable. See L for +more information on naming assignable devices. + +=item Default Value + +None. This parameter must not be present if a B parameter is present. +If a B parameter is not present then B is mandatory as it is +required to look up the B in the list of assignable devices. + +=back + =back -- 2.20.1
[PATCH v5 21/23] xl / libxl: support naming of assignable devices
From: Paul Durrant This patch modifies libxl_device_pci_assignable_add() to take an optional 'name' argument, which (if supplied) is saved into xenstore and can hence be used to refer to the now-assignable BDF in subsequent operations. To facilitate this, a new libxl_device_pci_assignable_name2bdf() function is added. The xl code is modified to allow a name to be specified in the 'pci-assignable-add' operation and also allow an option to be specified to 'pci-assignable-list' requesting that names be displayed. The latter is facilitated by a new libxl_device_pci_assignable_bdf2name() function. Finally xl 'pci-assignable-remove' is modified to that either a name or BDF can be supplied. The supplied 'identifier' is first assumed to be a name, but if libxl_device_pci_assignable_name2bdf() fails to find a matching BDF the identifier itself will be parsed as a BDF. Names my only include printable characters and may not include whitespace. Signed-off-by: Paul Durrant Acked-by: Christian Lindig --- Cc: Ian Jackson Cc: Wei Liu Cc: David Scott Cc: Anthony PERARD v4: - Fix unitialized return value in libxl_device_pci_assignable_name2bdf() that was discovered in CI --- tools/include/libxl.h| 19 +- tools/libs/light/libxl_pci.c | 86 ++-- tools/ocaml/libs/xl/xenlight_stubs.c | 3 +- tools/xl/xl_cmdtable.c | 12 ++-- tools/xl/xl_pci.c| 80 ++ 5 files changed, 164 insertions(+), 36 deletions(-) diff --git a/tools/include/libxl.h b/tools/include/libxl.h index 5703fdf367c5..4025d3a3d437 100644 --- a/tools/include/libxl.h +++ b/tools/include/libxl.h @@ -476,6 +476,14 @@ */ #define LIBXL_HAVE_PCI_ASSIGNABLE_BDF 1 +/* + * LIBXL_HAVE_PCI_ASSIGNABLE_NAME indicates that the + * libxl_device_pci_assignable_add() function takes a 'name' argument + * and that the libxl_device_pci_assignable_name2bdf() and + * libxl_device_pci_assignable_bdf2name() functions are defined. + */ +#define LIBXL_HAVE_PCI_ASSIGNABLE_NAME 1 + /* * libxl ABI compatibility * @@ -2385,11 +2393,18 @@ int libxl_device_events_handler(libxl_ctx *ctx, * added or is not bound, the functions will emit a warning but return * SUCCESS. */ -int libxl_device_pci_assignable_add(libxl_ctx *ctx, libxl_pci_bdf *pcibdf, int rebind); -int libxl_device_pci_assignable_remove(libxl_ctx *ctx, libxl_pci_bdf *pcibdf, int rebind); +int libxl_device_pci_assignable_add(libxl_ctx *ctx, libxl_pci_bdf *pcibdf, +const char *name, int rebind); +int libxl_device_pci_assignable_remove(libxl_ctx *ctx, libxl_pci_bdf *pcibdf, + int rebind); libxl_pci_bdf *libxl_device_pci_assignable_list(libxl_ctx *ctx, int *num); void libxl_device_pci_assignable_list_free(libxl_pci_bdf *list, int num); +libxl_pci_bdf *libxl_device_pci_assignable_name2bdf(libxl_ctx *ctx, +const char *name); +char *libxl_device_pci_assignable_bdf2name(libxl_ctx *ctx, + libxl_pci_bdf *pcibdf); + /* CPUID handling */ int libxl_cpuid_parse_config(libxl_cpuid_policy_list *cpuid, const char* str); int libxl_cpuid_parse_config_xend(libxl_cpuid_policy_list *cpuid, diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c index eecbd6efb694..e62383dd7b8f 100644 --- a/tools/libs/light/libxl_pci.c +++ b/tools/libs/light/libxl_pci.c @@ -745,6 +745,7 @@ static int pciback_dev_unassign(libxl__gc *gc, libxl_pci_bdf *pcibdf) static int libxl__device_pci_assignable_add(libxl__gc *gc, libxl_pci_bdf *pcibdf, +const char *name, int rebind) { libxl_ctx *ctx = libxl__gc_owner(gc); @@ -753,6 +754,23 @@ static int libxl__device_pci_assignable_add(libxl__gc *gc, int rc; struct stat st; +/* Sanitise any name that was passed */ +if (name) { +unsigned int i, n = strlen(name); + +if (n > 64) { /* Reasonable upper bound on name length */ +LOG(ERROR, "Name too long"); +return ERROR_FAIL; +} + +for (i = 0; i < n; i++) { +if (!isgraph(name[i])) { +LOG(ERROR, "Names may only include printable characters"); +return ERROR_FAIL; +} +} +} + /* Local copy for convenience */ dom = pcibdf->domain; bus = pcibdf->bus; @@ -773,7 +791,7 @@ static int libxl__device_pci_assignable_add(libxl__gc *gc, } if ( rc ) { LOG(WARN, PCI_BDF" already assigned to pciback", dom, bus, dev, func); -goto quarantine; +goto name; } /* Check to see if there's already a driver that we need to unbind from */ @@ -804,7 +822,12 @@ static int libxl__device_pci_assignable_add(libxl__gc *gc, return ERROR_FAIL; }
[PATCH v5 19/23] libxl: modify libxl_device_pci_assignable_add/remove/list/list_free()...
From: Paul Durrant ... to use 'libxl_pci_bdf' rather than 'libxl_device_pci'. This patch modifies the API and callers accordingly. It also modifies several internal functions in libxl_pci.c that support the API to also use 'libxl_pci_bdf'. NOTE: The OCaml bindings are adjusted to contain the interface change. It should therefore not affect compatibility with OCaml-based utilities. Signed-off-by: Paul Durrant Acked-by: Christian Lindig --- Cc: Ian Jackson Cc: Wei Liu Cc: David Scott Cc: Anthony PERARD --- tools/include/libxl.h| 15 +- tools/libs/light/libxl_pci.c | 213 +++ tools/ocaml/libs/xl/xenlight_stubs.c | 15 +- tools/xl/xl_pci.c| 32 ++-- 4 files changed, 156 insertions(+), 119 deletions(-) diff --git a/tools/include/libxl.h b/tools/include/libxl.h index 5edacccbd1da..5703fdf367c5 100644 --- a/tools/include/libxl.h +++ b/tools/include/libxl.h @@ -469,6 +469,13 @@ */ #define LIBXL_HAVE_PCI_BDF 1 +/* + * LIBXL_HAVE_PCI_ASSIGNABLE_BDF indicates that the + * libxl_device_pci_assignable_add/remove/list/list_free() functions all + * use the 'libxl_pci_bdf' type rather than 'libxl_device_pci' type. + */ +#define LIBXL_HAVE_PCI_ASSIGNABLE_BDF 1 + /* * libxl ABI compatibility * @@ -2378,10 +2385,10 @@ int libxl_device_events_handler(libxl_ctx *ctx, * added or is not bound, the functions will emit a warning but return * SUCCESS. */ -int libxl_device_pci_assignable_add(libxl_ctx *ctx, libxl_device_pci *pci, int rebind); -int libxl_device_pci_assignable_remove(libxl_ctx *ctx, libxl_device_pci *pci, int rebind); -libxl_device_pci *libxl_device_pci_assignable_list(libxl_ctx *ctx, int *num); -void libxl_device_pci_assignable_list_free(libxl_device_pci *list, int num); +int libxl_device_pci_assignable_add(libxl_ctx *ctx, libxl_pci_bdf *pcibdf, int rebind); +int libxl_device_pci_assignable_remove(libxl_ctx *ctx, libxl_pci_bdf *pcibdf, int rebind); +libxl_pci_bdf *libxl_device_pci_assignable_list(libxl_ctx *ctx, int *num); +void libxl_device_pci_assignable_list_free(libxl_pci_bdf *list, int num); /* CPUID handling */ int libxl_cpuid_parse_config(libxl_cpuid_policy_list *cpuid, const char* str); diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c index e0bcab4ee840..eecbd6efb694 100644 --- a/tools/libs/light/libxl_pci.c +++ b/tools/libs/light/libxl_pci.c @@ -25,26 +25,33 @@ #define PCI_BDF_XSPATH "%04x-%02x-%02x-%01x" #define PCI_PT_QDEV_ID "pci-pt-%02x_%02x.%01x" -static unsigned int pci_encode_bdf(libxl_device_pci *pci) +static unsigned int pci_encode_bdf(libxl_pci_bdf *pcibdf) { unsigned int value; -value = pci->bdf.domain << 16; -value |= (pci->bdf.bus & 0xff) << 8; -value |= (pci->bdf.dev & 0x1f) << 3; -value |= (pci->bdf.func & 0x7); +value = pcibdf->domain << 16; +value |= (pcibdf->bus & 0xff) << 8; +value |= (pcibdf->dev & 0x1f) << 3; +value |= (pcibdf->func & 0x7); return value; } +static void pcibdf_struct_fill(libxl_pci_bdf *pcibdf, unsigned int domain, + unsigned int bus, unsigned int dev, + unsigned int func) +{ +pcibdf->domain = domain; +pcibdf->bus = bus; +pcibdf->dev = dev; +pcibdf->func = func; +} + static void pci_struct_fill(libxl_device_pci *pci, unsigned int domain, unsigned int bus, unsigned int dev, unsigned int func, unsigned int vdevfn) { -pci->bdf.domain = domain; -pci->bdf.bus = bus; -pci->bdf.dev = dev; -pci->bdf.func = func; +pcibdf_struct_fill(&pci->bdf, domain, bus, dev, func); pci->vdevfn = vdevfn; } @@ -350,8 +357,8 @@ static bool is_pci_in_array(libxl_device_pci *pcis, int num, } /* Write the standard BDF into the sysfs path given by sysfs_path. */ -static int sysfs_write_bdf(libxl__gc *gc, const char * sysfs_path, - libxl_device_pci *pci) +static int sysfs_write_bdf(libxl__gc *gc, const char *sysfs_path, + libxl_pci_bdf *pcibdf) { int rc, fd; char *buf; @@ -362,8 +369,8 @@ static int sysfs_write_bdf(libxl__gc *gc, const char * sysfs_path, return ERROR_FAIL; } -buf = GCSPRINTF(PCI_BDF, pci->bdf.domain, pci->bdf.bus, -pci->bdf.dev, pci->bdf.func); +buf = GCSPRINTF(PCI_BDF, pcibdf->domain, pcibdf->bus, +pcibdf->dev, pcibdf->func); rc = write(fd, buf, strlen(buf)); /* Annoying to have two if's, but we need the errno */ if (rc < 0) @@ -378,22 +385,22 @@ static int sysfs_write_bdf(libxl__gc *gc, const char * sysfs_path, #define PCI_INFO_PATH "/libxl/pci" -static char *pci_info_xs_path(libxl__gc *gc, libxl_device_pci *pci, +static char *pci_info_xs_path(libxl__gc *gc, libxl_pci_bdf *pcibdf, const char *node) { return node ? GCSPRINTF
Re: [ANNOUNCE] Call for agenda items for December 2020 Community Call @ 16:00 UTC
On 30.11.2020 13:16, George Dunlap wrote: >> On Nov 30, 2020, at 10:25 AM, Jan Beulich wrote: >> >> On 27.11.2020 12:52, George Dunlap wrote: >>> The proposed agenda is in >>> https://cryptpad.fr/pad/#/2/pad/edit/OPN55rXaOncupuWuHxtddzWJ/ and you can >>> edit to add items. Alternatively, you can reply to this mail directly. >> >> The "New series / series requiring attention" section is gone. Was >> this intentional? If not, I would have wanted to propose that items >> from that list which we didn't get to on the previous call be >> automatically propagated. According to my observation it is more >> likely than not that nothing would have changed in their status. >> Hence it may be easier to take one off the list if indeed it has >> got unstalled. > > Oops — I meant to delete the content, but not the header. > > Hopefully “not getting to that part of the call” should be rare; but yes, > copying it over (perhaps with a color to indicate that it’s been carried over > from last time) sounds reasonable. I’ll do that. Seeing that it didn't happen for today's meeting, I've blindly copied over the previous meeting's section, before adding new items there. Jan
Re: [PATCH v2 7/8] xen/arm: Remove Linux specific code that is not usable in XEN
Hello Julien, > On 2 Dec 2020, at 2:45 pm, Julien Grall wrote: > > > > On 26/11/2020 17:02, Rahul Singh wrote: >> struct io_pgtable_ops, struct io_pgtable_cfg, struct iommu_flush_ops, >> and struct iommu_ops related code are linux specific. > > So the assumption is we are going to support only sharing with the P2M and > the IOMMU. That's probably fine short term, but long-term we are going to > need unsharing the page-tables (there are issues on some platforms if the ITS > doorbell is accessed by the processors). > > I am ok with removing anything related to the unsharing code. But I think it > should be clarified here. Ok I will add in commit message what is removed from the code. > >> Remove code related to above struct as code is dead code in XEN. >> Signed-off-by: Rahul Singh >> --- >> xen/drivers/passthrough/arm/smmu-v3.c | 457 -- >> 1 file changed, 457 deletions(-) >> diff --git a/xen/drivers/passthrough/arm/smmu-v3.c >> b/xen/drivers/passthrough/arm/smmu-v3.c >> index 40e3890a58..55d1cba194 100644 >> --- a/xen/drivers/passthrough/arm/smmu-v3.c >> +++ b/xen/drivers/passthrough/arm/smmu-v3.c >> @@ -402,13 +402,7 @@ >> #define ARM_SMMU_CMDQ_SYNC_TIMEOUT_US 100 /* 1s! */ >> #define ARM_SMMU_CMDQ_SYNC_SPIN_COUNT 10 >> -#define MSI_IOVA_BASE 0x800 >> -#define MSI_IOVA_LENGTH 0x10 >> - >> static bool disable_bypass = 1; >> -module_param_named(disable_bypass, disable_bypass, bool, S_IRUGO); >> -MODULE_PARM_DESC(disable_bypass, >> -"Disable bypass streams such that incoming transactions from devices >> that are not attached to an iommu domain will report an abort back to the >> device and will not be allowed to pass through the SMMU."); > > Per your commit message, this doesn't look related to what you intend to > remove. Maybe your commit message should be updated? Ok. > >>enum pri_resp { >> PRI_RESP_DENY = 0, >> @@ -599,7 +593,6 @@ struct arm_smmu_domain { >> struct arm_smmu_device *smmu; >> struct mutexinit_mutex; /* Protects smmu pointer */ >> - struct io_pgtable_ops *pgtbl_ops; >> boolnon_strict; >> enum arm_smmu_domain_stage stage; >> @@ -1297,74 +1290,6 @@ static void arm_smmu_tlb_inv_context(void *cookie) >> arm_smmu_cmdq_issue_sync(smmu); >> } >> -static void arm_smmu_tlb_inv_range_nosync(unsigned long iova, size_t size, >> - size_t granule, bool leaf, void >> *cookie) >> -{ >> -struct arm_smmu_domain *smmu_domain = cookie; >> -struct arm_smmu_device *smmu = smmu_domain->smmu; >> -struct arm_smmu_cmdq_ent cmd = { >> -.tlbi = { >> -.leaf = leaf, >> -.addr = iova, >> -}, >> -}; >> - >> -cmd.opcode = CMDQ_OP_TLBI_S2_IPA; >> -cmd.tlbi.vmid = smmu_domain->s2_cfg.vmid; >> - >> -do { >> -arm_smmu_cmdq_issue_cmd(smmu, &cmd); >> -cmd.tlbi.addr += granule; >> -} while (size -= granule); >> -} >> - >> -static void arm_smmu_tlb_inv_page_nosync(struct iommu_iotlb_gather *gather, >> - unsigned long iova, size_t granule, >> - void *cookie) >> -{ >> -arm_smmu_tlb_inv_range_nosync(iova, granule, granule, true, cookie); >> -} >> - >> -static void arm_smmu_tlb_inv_walk(unsigned long iova, size_t size, >> - size_t granule, void *cookie) >> -{ >> -struct arm_smmu_domain *smmu_domain = cookie; >> -struct arm_smmu_device *smmu = smmu_domain->smmu; >> - >> -arm_smmu_tlb_inv_range_nosync(iova, size, granule, false, cookie); >> -arm_smmu_cmdq_issue_sync(smmu); >> -} >> - >> -static void arm_smmu_tlb_inv_leaf(unsigned long iova, size_t size, >> - size_t granule, void *cookie) >> -{ >> -struct arm_smmu_domain *smmu_domain = cookie; >> -struct arm_smmu_device *smmu = smmu_domain->smmu; >> - >> -arm_smmu_tlb_inv_range_nosync(iova, size, granule, true, cookie); >> -arm_smmu_cmdq_issue_sync(smmu); >> -} >> - >> -static const struct iommu_flush_ops arm_smmu_flush_ops = { >> -.tlb_flush_all = arm_smmu_tlb_inv_context, >> -.tlb_flush_walk = arm_smmu_tlb_inv_walk, >> -.tlb_flush_leaf = arm_smmu_tlb_inv_leaf, >> -.tlb_add_page = arm_smmu_tlb_inv_page_nosync, >> -}; >> - >> -/* IOMMU API */ >> -static bool arm_smmu_capable(enum iommu_cap cap) >> -{ >> -switch (cap) { >> -case IOMMU_CAP_CACHE_COHERENCY: >> -return true; >> -case IOMMU_CAP_NOEXEC: >> -return true; >> -default: >> -return false; >> -} >> -} >> - >> static struct iommu_domain *arm_smmu_domain_alloc(unsigned type) >> { >> struct arm_smmu_domain *smmu_domain; >> @@ -1421,7 +1346,6 @@ static void arm_smmu_domain_free(struct iommu_domain >>
Re: [PATCH v2 8/8] xen/arm: Add support for SMMUv3 driver
Hello Stefano, > On 3 Dec 2020, at 4:13 am, Stefano Stabellini wrote: > > On Wed, 2 Dec 2020, Julien Grall wrote: >> On 02/12/2020 02:51, Stefano Stabellini wrote: >>> On Thu, 26 Nov 2020, Rahul Singh wrote: +/* Alias to Xen device tree helpers */ +#define device_node dt_device_node +#define of_phandle_args dt_phandle_args +#define of_device_id dt_device_match +#define of_match_node dt_match_node +#define of_property_read_u32(np, pname, out) (!dt_property_read_u32(np, pname, out)) +#define of_property_read_bool dt_property_read_bool +#define of_parse_phandle_with_args dt_parse_phandle_with_args >>> >>> Given all the changes to the file by the previous patches we are >>> basically fully (or almost fully) adapting this code to Xen. >>> >>> So at that point I wonder if we should just as well make these changes >>> (e.g. s/of_phandle_args/dt_phandle_args/g) to the code too. >> >> I have already accepted the fact that keeping Linux code as-is is nearly >> impossible without much workaround :). The benefits tends to also limited as >> we noticed for the SMMU driver. >> >> I would like to point out that this may make quite difficult (if not >> impossible) to revert the previous patches which remove support for some >> features (e.g. atomic, MSI, ATS). >> >> If we are going to adapt the code to Xen (I'd like to keep Linux code style >> though), then I think we should consider to keep code that may be useful in >> the near future (at least MSI, ATS). > > (I am fine with keeping the Linux code style.) > > We could try to keep the code as similar to Linux as possible. This > didn't work out in the past. > > Otherwise, we could fully adapt the driver to Xen. If we fully adapt the > driver to Xen (code style aside) it is better to be consistent and also > do substitutions like s/of_phandle_args/dt_phandle_args/g. Then the > policy becomes clear: the code comes from Linux but it is 100% adapted > to Xen. > > > Now the question about what to do about the MSI and ATS code is a good > one. We know that we are going to want that code at some point in the > next 2 years. Like you wrote, if we fully adapt the code to Xen and > remove MSI and ATS code, then it is going to be harder to add it back. > > So maybe keeping the MSI and ATS code for now, even if it cannot work, > would be better. I think this strategy works well if the MSI and ATS > code can be disabled easily, i.e. with a couple of lines of code in the > init function rather than #ifdef everywhere. It doesn't work well if we > have to add #ifdef everywhere. > > It looks like MSI could be disabled adding a couple of lines to > arm_smmu_setup_msis. > > Similarly ATS seems to be easy to disable by forcing ats_enabled to > false. > > So yes, this looks like a good way forward. Rahul, what do you think? I am ok to have the PCI ATS and MSI functionality in the code. As per the discussion next version of the patch will include below modification:Please let me know if there are any suggestion overall that should be added in next version. 1. Keep the PCI ATS and MSI functionality code. 2. Make the code with XEN compatible ( remove linux compatibility functions) 3. Keep the Linux coding style for code imported from Linux. 4. Fix all comments. I have one query what will be coding style for new code to make driver work for XEN ? > > > +#define FIELD_GET(_mask, _reg) \ +(typeof(_mask))(((_reg) & (_mask)) >> (__builtin_ffsll(_mask) - 1)) + +#define WRITE_ONCE(x, val) \ +do {\ +*(volatile typeof(x) *)&(x) = (val);\ +} while (0) >>> >>> maybe we should define this in xen/include/xen/lib.h >> >> I have attempted such discussion in the past and this resulted to more >> bikeshed than it is worth it. So I would suggest to re-implement WRITE_ONCE() >> as write_atomic() for now. > > Good suggestion, less discussions more code :-) I will use the write_atomic. Regards, Rahul
Re: [PATCH v3 5/5] evtchn: don't call Xen consumer callback with per-channel lock held
On Thu, Dec 3, 2020 at 5:09 AM Jan Beulich wrote: > > On 02.12.2020 22:10, Julien Grall wrote: > > On 23/11/2020 13:30, Jan Beulich wrote: > >> While there don't look to be any problems with this right now, the lock > >> order implications from holding the lock can be very difficult to follow > >> (and may be easy to violate unknowingly). The present callbacks don't > >> (and no such callback should) have any need for the lock to be held. > >> > >> However, vm_event_disable() frees the structures used by respective > >> callbacks and isn't otherwise synchronized with invocations of these > >> callbacks, so maintain a count of in-progress calls, for evtchn_close() > >> to wait to drop to zero before freeing the port (and dropping the lock). > > > > AFAICT, this callback is not the only place where the synchronization is > > missing in the VM event code. > > > > For instance, vm_event_put_request() can also race against > > vm_event_disable(). > > > > So shouldn't we handle this issue properly in VM event? > > I suppose that's a question to the VM event folks rather than me? IMHO it would obviously be better if the Xen side could handle situations like these gracefully. OTOH it is also reasonable to expect the privileged toolstack to perform its own sanity checks before disabling. Like right now for disabling vm_event we first pause the VM, process all remaining events that were on the ring, and only then disable the interface. This avoids the race condition mentioned above (among other issues). It's not perfect - we ran into problems where event replies don't have the desired effect because the VM is paused - but for the most part doing this is sufficient. So I don't consider this to be a priority at the moment. That said, if anyone is so inclined to fix this up, I would be happy to review & ack. Tamas
Re: [PATCH v2 15/17] xen/cpupool: add cpupool directories
On 02.12.2020 16:46, Jürgen Groß wrote: > On 01.12.20 09:21, Juergen Gross wrote: >> @@ -1003,12 +1006,131 @@ static struct notifier_block cpu_nfb = { >> .notifier_call = cpu_callback >> }; >> >> +#ifdef CONFIG_HYPFS >> +static const struct hypfs_entry *cpupool_pooldir_enter( >> +const struct hypfs_entry *entry); >> + >> +static struct hypfs_funcs cpupool_pooldir_funcs = { >> +.enter = cpupool_pooldir_enter, >> +.exit = hypfs_node_exit, >> +.read = hypfs_read_dir, >> +.write = hypfs_write_deny, >> +.getsize = hypfs_getsize, >> +.findentry = hypfs_dir_findentry, >> +}; >> + >> +static HYPFS_VARDIR_INIT(cpupool_pooldir, "%u", &cpupool_pooldir_funcs); >> + >> +static const struct hypfs_entry *cpupool_pooldir_enter( >> +const struct hypfs_entry *entry) >> +{ >> +return &cpupool_pooldir.e; >> +} > I have found a more generic way to handle entering a dyndir node, > resulting in no need to have cpupool_pooldir_enter() and > cpupool_pooldir_funcs. > > This will add some more lines to the previous patch, but less than > saved here. Which may then mean it's not a good use of time to look at v2 patch 14, considering there's a lot of other stuff in need of looking at? Jan
Re: [PATCH v2 12/17] xen/hypfs: add new enter() and exit() per node callbacks
On 01.12.2020 09:21, Juergen Gross wrote: > In order to better support resource allocation and locking for dynamic > hypfs nodes add enter() and exit() callbacks to struct hypfs_funcs. > > The enter() callback is called when entering a node during hypfs user > actions (traversing, reading or writing it), while the exit() callback > is called when leaving a node (accessing another node at the same or a > higher directory level, or when returning to the user). > > For avoiding recursion this requires a parent pointer in each node. > Let the enter() callback return the entry address which is stored as > the last accessed node in order to be able to use a template entry for > that purpose in case of dynamic entries. I guess I'll learn in subsequent patches why this is necessary / useful. Right now it looks odd for the function to simple return the incoming argument, as this way it's clear the caller knows the correct value already. > @@ -100,11 +112,58 @@ static void hypfs_unlock(void) > } > } > > +const struct hypfs_entry *hypfs_node_enter(const struct hypfs_entry *entry) > +{ > +return entry; > +} > + > +void hypfs_node_exit(const struct hypfs_entry *entry) > +{ > +} > + > +static int node_enter(const struct hypfs_entry *entry) > +{ > +const struct hypfs_entry **last = &this_cpu(hypfs_last_node_entered); > + > +entry = entry->funcs->enter(entry); > +if ( IS_ERR(entry) ) > +return PTR_ERR(entry); > + > +ASSERT(!*last || *last == entry->parent); > + > +*last = entry; > + > +return 0; > +} > + > +static void node_exit(const struct hypfs_entry *entry) > +{ > +const struct hypfs_entry **last = &this_cpu(hypfs_last_node_entered); > + > +if ( !*last ) > +return; Under what conditions is this legitimate to happen? IOW shouldn't there be an ASSERT_UNREACHABLE() here? Jan
Re: [PATCH v2 13/17] xen/hypfs: support dynamic hypfs nodes
On 01.12.2020 09:21, Juergen Gross wrote: > Add a HYPFS_VARDIR_INIT() macro for initializing such a directory > statically, taking a struct hypfs_funcs pointer as parameter additional > to those of HYPFS_DIR_INIT(). > > Modify HYPFS_VARSIZE_INIT() to take the function vector pointer as an > additional parameter as this will be needed for dynamical entries. > > For being able to let the generic hypfs coding continue to work on > normal struct hypfs_entry entities even for dynamical nodes add some > infrastructure for allocating a working area for the current hypfs > request in order to store needed information for traversing the tree. > This area is anchored in a percpu pointer and can be retrieved by any > level of the dynamic entries. The normal way to handle allocation and > freeing is to allocate the data in the enter() callback of a node and > to free it in the related exit() callback. > > Add a hypfs_add_dyndir() function for adding a dynamic directory > template to the tree, which is needed for having the correct reference > to its position in hypfs. > > Signed-off-by: Juergen Gross > --- > V2: > - switch to xzalloc_bytes() in hypfs_alloc_dyndata() (Jan Beulich) > - carved out from previous patch > - use enter() and exit() callbacks for allocating and freeing > dyndata memory I can't seem to be able to spot what this describes, and the respective part of the description therefore also remains unclear to me. Not the least again when considering multi-level templates, where potentially each of the handlers may want to allocate dyndata, yet only one party can at a time. > - add hypfs_add_dyndir() Overall this patch adds a lot of (for now) dead code, which makes it hard to judge whether this is what's needed. I guess I'll again learn more by reding further patches. Jan
Re: [PATCH v2 15/17] xen/cpupool: add cpupool directories
On 03.12.20 15:46, Jan Beulich wrote: On 02.12.2020 16:46, Jürgen Groß wrote: On 01.12.20 09:21, Juergen Gross wrote: @@ -1003,12 +1006,131 @@ static struct notifier_block cpu_nfb = { .notifier_call = cpu_callback }; +#ifdef CONFIG_HYPFS +static const struct hypfs_entry *cpupool_pooldir_enter( +const struct hypfs_entry *entry); + +static struct hypfs_funcs cpupool_pooldir_funcs = { +.enter = cpupool_pooldir_enter, +.exit = hypfs_node_exit, +.read = hypfs_read_dir, +.write = hypfs_write_deny, +.getsize = hypfs_getsize, +.findentry = hypfs_dir_findentry, +}; + +static HYPFS_VARDIR_INIT(cpupool_pooldir, "%u", &cpupool_pooldir_funcs); + +static const struct hypfs_entry *cpupool_pooldir_enter( +const struct hypfs_entry *entry) +{ +return &cpupool_pooldir.e; +} I have found a more generic way to handle entering a dyndir node, resulting in no need to have cpupool_pooldir_enter() and cpupool_pooldir_funcs. This will add some more lines to the previous patch, but less than saved here. Which may then mean it's not a good use of time to look at v2 patch 14, considering there's a lot of other stuff in need of looking at? All of V2 patch 14 remains valid, there is just a generic enter function added in V3. Juergen OpenPGP_0xB0DE9DD628BF132F.asc Description: application/pgp-keys OpenPGP_signature Description: OpenPGP digital signature
Re: [PATCH v2 12/17] xen/hypfs: add new enter() and exit() per node callbacks
On 03.12.20 15:59, Jan Beulich wrote: On 01.12.2020 09:21, Juergen Gross wrote: In order to better support resource allocation and locking for dynamic hypfs nodes add enter() and exit() callbacks to struct hypfs_funcs. The enter() callback is called when entering a node during hypfs user actions (traversing, reading or writing it), while the exit() callback is called when leaving a node (accessing another node at the same or a higher directory level, or when returning to the user). For avoiding recursion this requires a parent pointer in each node. Let the enter() callback return the entry address which is stored as the last accessed node in order to be able to use a template entry for that purpose in case of dynamic entries. I guess I'll learn in subsequent patches why this is necessary / useful. Right now it looks odd for the function to simple return the incoming argument, as this way it's clear the caller knows the correct value already. Basically for dynamic entries based on a template the entry function will return the address of template->e instead of the dynamic entry itself. This will enable to have the standard entry functions for any nodes being linked to the template. @@ -100,11 +112,58 @@ static void hypfs_unlock(void) } } +const struct hypfs_entry *hypfs_node_enter(const struct hypfs_entry *entry) +{ +return entry; +} + +void hypfs_node_exit(const struct hypfs_entry *entry) +{ +} + +static int node_enter(const struct hypfs_entry *entry) +{ +const struct hypfs_entry **last = &this_cpu(hypfs_last_node_entered); + +entry = entry->funcs->enter(entry); +if ( IS_ERR(entry) ) +return PTR_ERR(entry); + +ASSERT(!*last || *last == entry->parent); + +*last = entry; + +return 0; +} + +static void node_exit(const struct hypfs_entry *entry) +{ +const struct hypfs_entry **last = &this_cpu(hypfs_last_node_entered); + +if ( !*last ) +return; Under what conditions is this legitimate to happen? IOW shouldn't there be an ASSERT_UNREACHABLE() here? This is for the "/" node. Juergen OpenPGP_0xB0DE9DD628BF132F.asc Description: application/pgp-keys OpenPGP_signature Description: OpenPGP digital signature
Re: [PATCH v2 13/17] xen/hypfs: support dynamic hypfs nodes
On 03.12.20 16:08, Jan Beulich wrote: On 01.12.2020 09:21, Juergen Gross wrote: Add a HYPFS_VARDIR_INIT() macro for initializing such a directory statically, taking a struct hypfs_funcs pointer as parameter additional to those of HYPFS_DIR_INIT(). Modify HYPFS_VARSIZE_INIT() to take the function vector pointer as an additional parameter as this will be needed for dynamical entries. For being able to let the generic hypfs coding continue to work on normal struct hypfs_entry entities even for dynamical nodes add some infrastructure for allocating a working area for the current hypfs request in order to store needed information for traversing the tree. This area is anchored in a percpu pointer and can be retrieved by any level of the dynamic entries. The normal way to handle allocation and freeing is to allocate the data in the enter() callback of a node and to free it in the related exit() callback. Add a hypfs_add_dyndir() function for adding a dynamic directory template to the tree, which is needed for having the correct reference to its position in hypfs. Signed-off-by: Juergen Gross --- V2: - switch to xzalloc_bytes() in hypfs_alloc_dyndata() (Jan Beulich) - carved out from previous patch - use enter() and exit() callbacks for allocating and freeing dyndata memory I can't seem to be able to spot what this describes, and the respective part of the description therefore also remains unclear I think all pieces are coming together with patch 15. to me. Not the least again when considering multi-level templates, where potentially each of the handlers may want to allocate dyndata, yet only one party can at a time. Right now: yes. In case needed it will be rather easy to have a linked list of dyndata entities, with the percpu dyndata variable pointing to the most recent one (the one of the currently deepest nesting level). - add hypfs_add_dyndir() Overall this patch adds a lot of (for now) dead code, which makes it hard to judge whether this is what's needed. I guess I'll again learn more by reding further patches. I hope so. OpenPGP_0xB0DE9DD628BF132F.asc Description: application/pgp-keys OpenPGP_signature Description: OpenPGP digital signature
Re: [PATCH v5 1/4] domctl: introduce a new domain create flag, XEN_DOMCTL_CDF_evtchn_fifo, ...
On 03.12.2020 13:41, Paul Durrant wrote: > From: Paul Durrant > > ...to control the visibility of the FIFO event channel operations > (EVTCHNOP_init_control, EVTCHNOP_expand_array, and EVTCHNOP_set_priority) to > the guest. > > These operations were added to the public header in commit d2d50c2f308f > ("evtchn: add FIFO-based event channel ABI") and the first implementation > appeared in the two subsequent commits: edc8872aeb4a ("evtchn: implement > EVTCHNOP_set_priority and add the set_priority hook") and 88910061ec61 > ("evtchn: add FIFO-based event channel hypercalls and port ops"). Prior to > that, a guest issuing those operations would receive a return value of > -ENOSYS (not implemented) from Xen. Guests aware of the FIFO operations but > running on an older (pre-4.4) Xen would fall back to using the 2-level event > channel interface upon seeing this return value. > > Unfortunately the uncontrolable appearance of these new operations in Xen 4.4 > onwards has implications for hibernation of some Linux guests. During resume > from hibernation, there are two kernels involved: the "boot" kernel and the > "resume" kernel. The guest boot kernel may default to use FIFO operations and > instruct Xen via EVTCHNOP_init_control to switch from 2-level to FIFO. On the > other hand, the resume kernel keeps assuming 2-level, because it was > hibernated > on a version of Xen that did not support the FIFO operations. > > To maintain compatibility it is necessary to make Xen behave as it did > before the new operations were added and hence the code in this patch ensures > that, if XEN_DOMCTL_CDF_evtchn_fifo is not set, the FIFO event channel > operations will again result in -ENOSYS being returned to the guest. I have to admit I'm now even more concerned of the control for such going into Xen, the more with the now 2nd use in the subsequent patch. The implication of this would seem to be that whenever we add new hypercalls or sub-ops, a domain creation control would also need adding determining whether that new sub-op is actually okay to use by a guest. Or else I'd be keen to up front see criteria at least roughly outlined by which it could be established whether such an override control is needed. I'm also not convinced such controls really want to be opt-in rather than opt-out. While perhaps sensible as long as a feature is experimental, not exposing stuff by default may mean slower adoption of new (and hopefully better) functionality. I realize there's still the option of having the tool stack default to enable, and just the hypervisor defaulting to disable, but anyway. > --- a/xen/arch/arm/domain.c > +++ b/xen/arch/arm/domain.c > @@ -622,7 +622,8 @@ int arch_sanitise_domain_config(struct > xen_domctl_createdomain *config) > unsigned int max_vcpus; > > /* HVM and HAP must be set. IOMMU may or may not be */ > -if ( (config->flags & ~XEN_DOMCTL_CDF_iommu) != > +if ( (config->flags & > + ~(XEN_DOMCTL_CDF_iommu | XEN_DOMCTL_CDF_evtchn_fifo) != > (XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap) ) > { > dprintk(XENLOG_INFO, "Unsupported configuration %#x\n", > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -2478,7 +2478,8 @@ void __init create_domUs(void) > struct domain *d; > struct xen_domctl_createdomain d_cfg = { > .arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE, > -.flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap, > +.flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap | > + XEN_DOMCTL_CDF_evtchn_fifo, > .max_evtchn_port = -1, > .max_grant_frames = 64, > .max_maptrack_frames = 1024, > --- a/xen/arch/arm/setup.c > +++ b/xen/arch/arm/setup.c > @@ -805,7 +805,8 @@ void __init start_xen(unsigned long boot_phys_offset, > struct bootmodule *xen_bootmodule; > struct domain *dom0; > struct xen_domctl_createdomain dom0_cfg = { > -.flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap, > +.flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap | > + XEN_DOMCTL_CDF_evtchn_fifo, > .max_evtchn_port = -1, > .max_grant_frames = gnttab_dom0_frames(), > .max_maptrack_frames = -1, > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -738,7 +738,8 @@ static struct domain *__init create_dom0(const module_t > *image, > const char *loader) > { > struct xen_domctl_createdomain dom0_cfg = { > -.flags = IS_ENABLED(CONFIG_TBOOT) ? XEN_DOMCTL_CDF_s3_integrity : 0, > +.flags = XEN_DOMCTL_CDF_evtchn_fifo | > + (IS_ENABLED(CONFIG_TBOOT) ? XEN_DOMCTL_CDF_s3_integrity : > 0), > .max_evtchn_port = -1, > .max_grant_frames = -1, > .max_maptrack_frames = -1, > --- a/xen/common/domain.c > +++ b/xen/common/domain.c > @@ -307,7 +307,7 @@ static int sanitise_domain_config(struct > xen_do
Re: [PATCH v2 12/17] xen/hypfs: add new enter() and exit() per node callbacks
On 03.12.2020 16:14, Jürgen Groß wrote: > On 03.12.20 15:59, Jan Beulich wrote: >> On 01.12.2020 09:21, Juergen Gross wrote: >>> @@ -100,11 +112,58 @@ static void hypfs_unlock(void) >>> } >>> } >>> >>> +const struct hypfs_entry *hypfs_node_enter(const struct hypfs_entry *entry) >>> +{ >>> +return entry; >>> +} >>> + >>> +void hypfs_node_exit(const struct hypfs_entry *entry) >>> +{ >>> +} >>> + >>> +static int node_enter(const struct hypfs_entry *entry) >>> +{ >>> +const struct hypfs_entry **last = &this_cpu(hypfs_last_node_entered); >>> + >>> +entry = entry->funcs->enter(entry); >>> +if ( IS_ERR(entry) ) >>> +return PTR_ERR(entry); >>> + >>> +ASSERT(!*last || *last == entry->parent); >>> + >>> +*last = entry; >>> + >>> +return 0; >>> +} >>> + >>> +static void node_exit(const struct hypfs_entry *entry) >>> +{ >>> +const struct hypfs_entry **last = &this_cpu(hypfs_last_node_entered); >>> + >>> +if ( !*last ) >>> +return; >> >> Under what conditions is this legitimate to happen? IOW shouldn't >> there be an ASSERT_UNREACHABLE() here? > > This is for the "/" node. I.e. would ASSERT(!entry->parent) be appropriate to add here, at the same time serving as documentation of what you've just said? Jan
Re: [PATCH v2 14/17] xen/hypfs: add support for id-based dynamic directories
On 01.12.2020 09:21, Juergen Gross wrote: > --- a/xen/common/hypfs.c > +++ b/xen/common/hypfs.c > @@ -355,6 +355,81 @@ unsigned int hypfs_getsize(const struct hypfs_entry > *entry) > return entry->size; > } > > +int hypfs_read_dyndir_id_entry(const struct hypfs_entry_dir *template, > + unsigned int id, bool is_last, > + XEN_GUEST_HANDLE_PARAM(void) *uaddr) > +{ > +struct xen_hypfs_dirlistentry direntry; > +char name[HYPFS_DYNDIR_ID_NAMELEN]; > +unsigned int e_namelen, e_len; > + > +e_namelen = snprintf(name, sizeof(name), template->e.name, id); > +e_len = DIRENTRY_SIZE(e_namelen); > +direntry.e.pad = 0; > +direntry.e.type = template->e.type; > +direntry.e.encoding = template->e.encoding; > +direntry.e.content_len = template->e.funcs->getsize(&template->e); > +direntry.e.max_write_len = template->e.max_size; > +direntry.off_next = is_last ? 0 : e_len; > +if ( copy_to_guest(*uaddr, &direntry, 1) ) > +return -EFAULT; > +if ( copy_to_guest_offset(*uaddr, DIRENTRY_NAME_OFF, name, > + e_namelen + 1) ) > +return -EFAULT; > + > +guest_handle_add_offset(*uaddr, e_len); > + > +return 0; > +} > + > +static struct hypfs_entry *hypfs_dyndir_findentry( > +const struct hypfs_entry_dir *dir, const char *name, unsigned int > name_len) > +{ > +const struct hypfs_dyndir_id *data; > + > +data = hypfs_get_dyndata(); > + > +/* Use template with original findentry function. */ > +return data->template->e.funcs->findentry(data->template, name, > name_len); > +} > + > +static int hypfs_read_dyndir(const struct hypfs_entry *entry, > + XEN_GUEST_HANDLE_PARAM(void) uaddr) > +{ > +const struct hypfs_dyndir_id *data; > + > +data = hypfs_get_dyndata(); > + > +/* Use template with original read function. */ > +return data->template->e.funcs->read(&data->template->e, uaddr); > +} > + > +struct hypfs_entry *hypfs_gen_dyndir_entry_id( > +const struct hypfs_entry_dir *template, unsigned int id) > +{ > +struct hypfs_dyndir_id *data; > + > +data = hypfs_get_dyndata(); > + > +data->template = template; > +data->id = id; > +snprintf(data->name, sizeof(data->name), template->e.name, id); > +data->dir = *template; > +data->dir.e.name = data->name; I'm somewhat puzzled, if not confused, by the apparent redundancy of this name generation with that in hypfs_read_dyndir_id_entry(). Wasn't the idea to be able to use generic functions on these generated entries? Also, seeing that other function's name, wouldn't the one here want to be named hypfs_gen_dyndir_id_entry()? Jan
RE: [PATCH v5 1/4] domctl: introduce a new domain create flag, XEN_DOMCTL_CDF_evtchn_fifo, ...
> -Original Message- > From: Jan Beulich > Sent: 03 December 2020 15:23 > To: Paul Durrant > Cc: Paul Durrant ; Eslam Elnikety ; > Ian Jackson > ; Wei Liu ; Anthony PERARD > ; Andrew Cooper > ; George Dunlap ; Julien > Grall ; > Stefano Stabellini ; Christian Lindig > ; David > Scott ; Volodymyr Babchuk ; > Roger Pau Monné > ; xen-devel@lists.xenproject.org > Subject: Re: [PATCH v5 1/4] domctl: introduce a new domain create flag, > XEN_DOMCTL_CDF_evtchn_fifo, > ... > > On 03.12.2020 13:41, Paul Durrant wrote: > > From: Paul Durrant > > > > ...to control the visibility of the FIFO event channel operations > > (EVTCHNOP_init_control, EVTCHNOP_expand_array, and EVTCHNOP_set_priority) to > > the guest. > > > > These operations were added to the public header in commit d2d50c2f308f > > ("evtchn: add FIFO-based event channel ABI") and the first implementation > > appeared in the two subsequent commits: edc8872aeb4a ("evtchn: implement > > EVTCHNOP_set_priority and add the set_priority hook") and 88910061ec61 > > ("evtchn: add FIFO-based event channel hypercalls and port ops"). Prior to > > that, a guest issuing those operations would receive a return value of > > -ENOSYS (not implemented) from Xen. Guests aware of the FIFO operations but > > running on an older (pre-4.4) Xen would fall back to using the 2-level event > > channel interface upon seeing this return value. > > > > Unfortunately the uncontrolable appearance of these new operations in Xen > > 4.4 > > onwards has implications for hibernation of some Linux guests. During resume > > from hibernation, there are two kernels involved: the "boot" kernel and the > > "resume" kernel. The guest boot kernel may default to use FIFO operations > > and > > instruct Xen via EVTCHNOP_init_control to switch from 2-level to FIFO. On > > the > > other hand, the resume kernel keeps assuming 2-level, because it was > > hibernated > > on a version of Xen that did not support the FIFO operations. > > > > To maintain compatibility it is necessary to make Xen behave as it did > > before the new operations were added and hence the code in this patch > > ensures > > that, if XEN_DOMCTL_CDF_evtchn_fifo is not set, the FIFO event channel > > operations will again result in -ENOSYS being returned to the guest. > > I have to admit I'm now even more concerned of the control for such > going into Xen, the more with the now 2nd use in the subsequent patch. > The implication of this would seem to be that whenever we add new > hypercalls or sub-ops, a domain creation control would also need > adding determining whether that new sub-op is actually okay to use by > a guest. Or else I'd be keen to up front see criteria at least roughly > outlined by which it could be established whether such an override > control is needed. > Ultimately I think any new hypercall (or related set of hypercalls) added to the ABI needs to be opt-in on a per-domain basis, so that we know that from when a domain is first created it will not see a change in its environment unless the VM administrator wants that to happen. > I'm also not convinced such controls really want to be opt-in rather > than opt-out. They really need to be opt-in I think. From a cloud provider PoV it is important that nothing in a customer's environment changes unless we want it to. Otherwise we have no way to deploy an updated hypervisor version without risking crashing their VMs. > While perhaps sensible as long as a feature is > experimental, not exposing stuff by default may mean slower adoption > of new (and hopefully better) functionality. I realize there's still > the option of having the tool stack default to enable, and just the > hypervisor defaulting to disable, but anyway. > Ok. I don't see a problem in default-to-enable behaviour... but I guess we will need to add ABI features to migration stream to fix things properly. > > --- a/xen/arch/arm/domain.c > > +++ b/xen/arch/arm/domain.c > > @@ -622,7 +622,8 @@ int arch_sanitise_domain_config(struct > > xen_domctl_createdomain *config) > > unsigned int max_vcpus; > > > > /* HVM and HAP must be set. IOMMU may or may not be */ > > -if ( (config->flags & ~XEN_DOMCTL_CDF_iommu) != > > +if ( (config->flags & > > + ~(XEN_DOMCTL_CDF_iommu | XEN_DOMCTL_CDF_evtchn_fifo) != > > (XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap) ) > > { > > dprintk(XENLOG_INFO, "Unsupported configuration %#x\n", > > --- a/xen/arch/arm/domain_build.c > > +++ b/xen/arch/arm/domain_build.c > > @@ -2478,7 +2478,8 @@ void __init create_domUs(void) > > struct domain *d; > > struct xen_domctl_createdomain d_cfg = { > > .arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE, > > -.flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap, > > +.flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap | > > + XEN_DOMCTL_CDF_evtchn_fifo, > > .max_evtchn_port = -1, > >
Re: [PATCH v5 1/4] domctl: introduce a new domain create flag, XEN_DOMCTL_CDF_evtchn_fifo, ...
On 03.12.2020 16:45, Paul Durrant wrote: >> From: Jan Beulich >> Sent: 03 December 2020 15:23 >> >> On 03.12.2020 13:41, Paul Durrant wrote: >>> From: Paul Durrant >>> >>> ...to control the visibility of the FIFO event channel operations >>> (EVTCHNOP_init_control, EVTCHNOP_expand_array, and EVTCHNOP_set_priority) to >>> the guest. >>> >>> These operations were added to the public header in commit d2d50c2f308f >>> ("evtchn: add FIFO-based event channel ABI") and the first implementation >>> appeared in the two subsequent commits: edc8872aeb4a ("evtchn: implement >>> EVTCHNOP_set_priority and add the set_priority hook") and 88910061ec61 >>> ("evtchn: add FIFO-based event channel hypercalls and port ops"). Prior to >>> that, a guest issuing those operations would receive a return value of >>> -ENOSYS (not implemented) from Xen. Guests aware of the FIFO operations but >>> running on an older (pre-4.4) Xen would fall back to using the 2-level event >>> channel interface upon seeing this return value. >>> >>> Unfortunately the uncontrolable appearance of these new operations in Xen >>> 4.4 >>> onwards has implications for hibernation of some Linux guests. During resume >>> from hibernation, there are two kernels involved: the "boot" kernel and the >>> "resume" kernel. The guest boot kernel may default to use FIFO operations >>> and >>> instruct Xen via EVTCHNOP_init_control to switch from 2-level to FIFO. On >>> the >>> other hand, the resume kernel keeps assuming 2-level, because it was >>> hibernated >>> on a version of Xen that did not support the FIFO operations. >>> >>> To maintain compatibility it is necessary to make Xen behave as it did >>> before the new operations were added and hence the code in this patch >>> ensures >>> that, if XEN_DOMCTL_CDF_evtchn_fifo is not set, the FIFO event channel >>> operations will again result in -ENOSYS being returned to the guest. >> >> I have to admit I'm now even more concerned of the control for such >> going into Xen, the more with the now 2nd use in the subsequent patch. >> The implication of this would seem to be that whenever we add new >> hypercalls or sub-ops, a domain creation control would also need >> adding determining whether that new sub-op is actually okay to use by >> a guest. Or else I'd be keen to up front see criteria at least roughly >> outlined by which it could be established whether such an override >> control is needed. >> > > Ultimately I think any new hypercall (or related set of hypercalls) added to > the ABI needs to be opt-in on a per-domain basis, so that we know that from > when a domain is first created it will not see a change in its environment > unless the VM administrator wants that to happen. A new hypercall appearing is a change to the guest's environment, yes, but a backwards compatible one. I don't see how this would harm a guest. This and ... >> I'm also not convinced such controls really want to be opt-in rather >> than opt-out. > > They really need to be opt-in I think. From a cloud provider PoV it is > important that nothing in a customer's environment changes unless we want it > to. Otherwise we have no way to deploy an updated hypervisor version without > risking crashing their VMs. ... this sound to me more like workarounds for buggy guests than functionality the hypervisor _needs_ to have. (I can appreciate the specific case here for the specific scenario you provide as an exception.) >>> --- a/xen/arch/arm/domain.c >>> +++ b/xen/arch/arm/domain.c >>> @@ -622,7 +622,8 @@ int arch_sanitise_domain_config(struct >>> xen_domctl_createdomain *config) >>> unsigned int max_vcpus; >>> >>> /* HVM and HAP must be set. IOMMU may or may not be */ >>> -if ( (config->flags & ~XEN_DOMCTL_CDF_iommu) != >>> +if ( (config->flags & >>> + ~(XEN_DOMCTL_CDF_iommu | XEN_DOMCTL_CDF_evtchn_fifo) != >>> (XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap) ) >>> { >>> dprintk(XENLOG_INFO, "Unsupported configuration %#x\n", >>> --- a/xen/arch/arm/domain_build.c >>> +++ b/xen/arch/arm/domain_build.c >>> @@ -2478,7 +2478,8 @@ void __init create_domUs(void) >>> struct domain *d; >>> struct xen_domctl_createdomain d_cfg = { >>> .arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE, >>> -.flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap, >>> +.flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap | >>> + XEN_DOMCTL_CDF_evtchn_fifo, >>> .max_evtchn_port = -1, >>> .max_grant_frames = 64, >>> .max_maptrack_frames = 1024, >>> --- a/xen/arch/arm/setup.c >>> +++ b/xen/arch/arm/setup.c >>> @@ -805,7 +805,8 @@ void __init start_xen(unsigned long boot_phys_offset, >>> struct bootmodule *xen_bootmodule; >>> struct domain *dom0; >>> struct xen_domctl_createdomain dom0_cfg = { >>> -.flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap, >>> +.flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCT
[xen-unstable test] 157166: tolerable FAIL - PUSHED
flight 157166 xen-unstable real [real] flight 157180 xen-unstable real-retest [real] http://logs.test-lab.xenproject.org/osstest/logs/157166/ http://logs.test-lab.xenproject.org/osstest/logs/157180/ Failures :-/ but no regressions. Tests which are failing intermittently (not blocking): test-amd64-i386-xl-qemut-debianhvm-i386-xsm 12 debian-hvm-install fail pass in 157180-retest Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-rtds 20 guest-localmigrate/x10 fail like 157147 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 157147 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 157147 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 157147 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 157147 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 157147 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 157147 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 157147 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 157147 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 157147 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 157147 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 157147 test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-i386-xl-pvshim14 guest-start fail never pass test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-xl 16 saverestore-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-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 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-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-i386-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-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 14 migrate-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-armhf-armhf-xl-cubietruck 15 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 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-arm64-arm64-xl-seattle 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 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-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-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 version targeted for testing: xen cabf60fc32d4cfa1d74a2bdfcdb294a31da5d68e baseline version: xen 3ae469af8e680df31eecd0a2ac6a83b58ad7ce53 Last test of basis 157147 2020-12-02 01:52:26 Z1 days Testing same since 157166 2020-12-02 23:08:38 Z0 days1 attempts Pe
Re: [SPECIFICATION RFC] The firmware and bootloader log specification
On Sat, Nov 14, 2020 at 2:01 AM Daniel Kiper wrote: ... > The log specification should be as much as possible platform agnostic > and self contained. The final version of this spec should be merged into > existing specifications, e.g. UEFI, ACPI, Multiboot2, or be a standalone > spec, e.g. as a part of OASIS Standards. The former seems better but is > not perfect too... With all respect... https://xkcd.com/927/ -- With Best Regards, Andy Shevchenko
[ovmf test] 157178: all pass - PUSHED
flight 157178 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/157178/ Perfect :-) All tests in this flight passed as required version targeted for testing: ovmf 484e869dfd3b7e4b6c47cb65ae5d5f499fcc056e baseline version: ovmf 7c4ab1c2ef60a4690177d2361f8dd44d7d7df7f8 Last test of basis 157167 2020-12-02 23:40:53 Z0 days Testing same since 157178 2020-12-03 12:29:37 Z0 days1 attempts People who touched revisions under test: Abner Chang Fan Wang Jiaxin Wu Ray Ni Siyuan Fu Ting Ye 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 7c4ab1c2ef..484e869dfd 484e869dfd3b7e4b6c47cb65ae5d5f499fcc056e -> xen-tested-master
RE: [PATCH v5 1/4] domctl: introduce a new domain create flag, XEN_DOMCTL_CDF_evtchn_fifo, ...
> -Original Message- > From: Jan Beulich > Sent: 03 December 2020 15:57 > To: p...@xen.org > Cc: 'Paul Durrant' ; 'Eslam Elnikety' > ; 'Ian Jackson' > ; 'Wei Liu' ; 'Anthony PERARD' > ; 'Andrew > Cooper' ; 'George Dunlap' > ; 'Julien Grall' > ; 'Stefano Stabellini' ; 'Christian > Lindig' > ; 'David Scott' ; 'Volodymyr > Babchuk' > ; 'Roger Pau Monné' ; > xen-devel@lists.xenproject.org > Subject: Re: [PATCH v5 1/4] domctl: introduce a new domain create flag, > XEN_DOMCTL_CDF_evtchn_fifo, > ... > > On 03.12.2020 16:45, Paul Durrant wrote: > >> From: Jan Beulich > >> Sent: 03 December 2020 15:23 > >> > >> On 03.12.2020 13:41, Paul Durrant wrote: > >>> From: Paul Durrant > >>> > >>> ...to control the visibility of the FIFO event channel operations > >>> (EVTCHNOP_init_control, EVTCHNOP_expand_array, and EVTCHNOP_set_priority) > >>> to > >>> the guest. > >>> > >>> These operations were added to the public header in commit d2d50c2f308f > >>> ("evtchn: add FIFO-based event channel ABI") and the first implementation > >>> appeared in the two subsequent commits: edc8872aeb4a ("evtchn: implement > >>> EVTCHNOP_set_priority and add the set_priority hook") and 88910061ec61 > >>> ("evtchn: add FIFO-based event channel hypercalls and port ops"). Prior to > >>> that, a guest issuing those operations would receive a return value of > >>> -ENOSYS (not implemented) from Xen. Guests aware of the FIFO operations > >>> but > >>> running on an older (pre-4.4) Xen would fall back to using the 2-level > >>> event > >>> channel interface upon seeing this return value. > >>> > >>> Unfortunately the uncontrolable appearance of these new operations in Xen > >>> 4.4 > >>> onwards has implications for hibernation of some Linux guests. During > >>> resume > >>> from hibernation, there are two kernels involved: the "boot" kernel and > >>> the > >>> "resume" kernel. The guest boot kernel may default to use FIFO operations > >>> and > >>> instruct Xen via EVTCHNOP_init_control to switch from 2-level to FIFO. On > >>> the > >>> other hand, the resume kernel keeps assuming 2-level, because it was > >>> hibernated > >>> on a version of Xen that did not support the FIFO operations. > >>> > >>> To maintain compatibility it is necessary to make Xen behave as it did > >>> before the new operations were added and hence the code in this patch > >>> ensures > >>> that, if XEN_DOMCTL_CDF_evtchn_fifo is not set, the FIFO event channel > >>> operations will again result in -ENOSYS being returned to the guest. > >> > >> I have to admit I'm now even more concerned of the control for such > >> going into Xen, the more with the now 2nd use in the subsequent patch. > >> The implication of this would seem to be that whenever we add new > >> hypercalls or sub-ops, a domain creation control would also need > >> adding determining whether that new sub-op is actually okay to use by > >> a guest. Or else I'd be keen to up front see criteria at least roughly > >> outlined by which it could be established whether such an override > >> control is needed. > >> > > > > Ultimately I think any new hypercall (or related set of hypercalls) added > > to the ABI needs to be > opt-in on a per-domain basis, so that we know that from when a domain is > first created it will not see > a change in its environment unless the VM administrator wants that to happen. > > A new hypercall appearing is a change to the guest's environment, yes, > but a backwards compatible one. I don't see how this would harm a guest. Say we have a guest which is aware of the newer Xen and is coded to use the new hypercall *but* we start it on an older Xen where the new hypercall is not implemented. *Then* we migrate it to the newer Xen... the new hypercall suddenly becomes available and changes the guest behaviour. In the worst case, the guest is sufficiently confused that it crashes. > This and ... > > >> I'm also not convinced such controls really want to be opt-in rather > >> than opt-out. > > > > They really need to be opt-in I think. From a cloud provider PoV it is > > important that nothing in a > customer's environment changes unless we want it to. Otherwise we have no way > to deploy an updated > hypervisor version without risking crashing their VMs. > > ... this sound to me more like workarounds for buggy guests than > functionality the hypervisor _needs_ to have. (I can appreciate > the specific case here for the specific scenario you provide as > an exception.) If we want to have a hypervisor that can be used in a cloud environment then Xen absolutely needs this capability. > > >>> --- a/xen/arch/arm/domain.c > >>> +++ b/xen/arch/arm/domain.c > >>> @@ -622,7 +622,8 @@ int arch_sanitise_domain_config(struct > >>> xen_domctl_createdomain *config) > >>> unsigned int max_vcpus; > >>> > >>> /* HVM and HAP must be set. IOMMU may or may not be */ > >>> -if ( (config->flags & ~XEN_DOMCTL_CDF_iommu) != > >>> +if ( (config->flags & > >>>
Re: [PATCH v5 1/4] domctl: introduce a new domain create flag, XEN_DOMCTL_CDF_evtchn_fifo, ...
On 03.12.20 18:07, Paul Durrant wrote: -Original Message- From: Jan Beulich Sent: 03 December 2020 15:57 To: p...@xen.org Cc: 'Paul Durrant' ; 'Eslam Elnikety' ; 'Ian Jackson' ; 'Wei Liu' ; 'Anthony PERARD' ; 'Andrew Cooper' ; 'George Dunlap' ; 'Julien Grall' ; 'Stefano Stabellini' ; 'Christian Lindig' ; 'David Scott' ; 'Volodymyr Babchuk' ; 'Roger Pau Monné' ; xen-devel@lists.xenproject.org Subject: Re: [PATCH v5 1/4] domctl: introduce a new domain create flag, XEN_DOMCTL_CDF_evtchn_fifo, ... On 03.12.2020 16:45, Paul Durrant wrote: From: Jan Beulich Sent: 03 December 2020 15:23 On 03.12.2020 13:41, Paul Durrant wrote: From: Paul Durrant ...to control the visibility of the FIFO event channel operations (EVTCHNOP_init_control, EVTCHNOP_expand_array, and EVTCHNOP_set_priority) to the guest. These operations were added to the public header in commit d2d50c2f308f ("evtchn: add FIFO-based event channel ABI") and the first implementation appeared in the two subsequent commits: edc8872aeb4a ("evtchn: implement EVTCHNOP_set_priority and add the set_priority hook") and 88910061ec61 ("evtchn: add FIFO-based event channel hypercalls and port ops"). Prior to that, a guest issuing those operations would receive a return value of -ENOSYS (not implemented) from Xen. Guests aware of the FIFO operations but running on an older (pre-4.4) Xen would fall back to using the 2-level event channel interface upon seeing this return value. Unfortunately the uncontrolable appearance of these new operations in Xen 4.4 onwards has implications for hibernation of some Linux guests. During resume from hibernation, there are two kernels involved: the "boot" kernel and the "resume" kernel. The guest boot kernel may default to use FIFO operations and instruct Xen via EVTCHNOP_init_control to switch from 2-level to FIFO. On the other hand, the resume kernel keeps assuming 2-level, because it was hibernated on a version of Xen that did not support the FIFO operations. To maintain compatibility it is necessary to make Xen behave as it did before the new operations were added and hence the code in this patch ensures that, if XEN_DOMCTL_CDF_evtchn_fifo is not set, the FIFO event channel operations will again result in -ENOSYS being returned to the guest. I have to admit I'm now even more concerned of the control for such going into Xen, the more with the now 2nd use in the subsequent patch. The implication of this would seem to be that whenever we add new hypercalls or sub-ops, a domain creation control would also need adding determining whether that new sub-op is actually okay to use by a guest. Or else I'd be keen to up front see criteria at least roughly outlined by which it could be established whether such an override control is needed. Ultimately I think any new hypercall (or related set of hypercalls) added to the ABI needs to be opt-in on a per-domain basis, so that we know that from when a domain is first created it will not see a change in its environment unless the VM administrator wants that to happen. A new hypercall appearing is a change to the guest's environment, yes, but a backwards compatible one. I don't see how this would harm a guest. Say we have a guest which is aware of the newer Xen and is coded to use the new hypercall *but* we start it on an older Xen where the new hypercall is not implemented. *Then* we migrate it to the newer Xen... the new hypercall suddenly becomes available and changes the guest behaviour. In the worst case, the guest is sufficiently confused that it crashes. This and ... I'm also not convinced such controls really want to be opt-in rather than opt-out. They really need to be opt-in I think. From a cloud provider PoV it is important that nothing in a customer's environment changes unless we want it to. Otherwise we have no way to deploy an updated hypervisor version without risking crashing their VMs. ... this sound to me more like workarounds for buggy guests than functionality the hypervisor _needs_ to have. (I can appreciate the specific case here for the specific scenario you provide as an exception.) If we want to have a hypervisor that can be used in a cloud environment then Xen absolutely needs this capability. --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -622,7 +622,8 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config) unsigned int max_vcpus; /* HVM and HAP must be set. IOMMU may or may not be */ -if ( (config->flags & ~XEN_DOMCTL_CDF_iommu) != +if ( (config->flags & + ~(XEN_DOMCTL_CDF_iommu | XEN_DOMCTL_CDF_evtchn_fifo) != (XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap) ) { dprintk(XENLOG_INFO, "Unsupported configuration %#x\n", --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -2478,7 +2478,8 @@ void __init create_domUs(void) struct domain *d; struct xen_domctl_createdomain d_cfg = { .a
RE: [PATCH v5 1/4] domctl: introduce a new domain create flag, XEN_DOMCTL_CDF_evtchn_fifo, ...
> -Original Message- [snip] > > All of the hunks above point out a scalability issue if we were to > follow this route for even just a fair part of new sub-ops, and I > suppose you've noticed this with the next patch presumably touching > all the same places again. > >>> > >>> Indeed. This solution works for now but is probably not what we want in > >>> the long run. Same goes > for > >> the current way we control viridian features via an HVM param. It is good > >> enough for now IMO since > >> domctl is not a stable interface. Any ideas about how we might implement a > >> better interface in the > >> longer term? > >> > >> While it has other downsides, Jürgen's proposal doesn't have any > >> similar scalability issue afaics. Another possible model would > >> seem to be to key new hypercalls to hypervisor CPUID leaf bits, > >> and derive their availability from a guest's CPUID policy. Of > >> course this won't work when needing to retrofit guarding like > >> you want to do here. > >> > > > > Ok, I'll take a look hypfs as an immediate solution, if that's preferred. > > Paul, if you'd like me to add a few patches to my series doing the basic > framework (per-domain nodes, interfaces for setting struct domain fields > via hypfs), I can do that. You could then concentrate on the tools side. > That would be very helpful. Thanks Juergen. Paul
Re: [PATCH v2 8/8] xen/arm: Add support for SMMUv3 driver
On Thu, 3 Dec 2020, Rahul Singh wrote: > > On 3 Dec 2020, at 4:13 am, Stefano Stabellini > > wrote: > > On Wed, 2 Dec 2020, Julien Grall wrote: > >> On 02/12/2020 02:51, Stefano Stabellini wrote: > >>> On Thu, 26 Nov 2020, Rahul Singh wrote: > +/* Alias to Xen device tree helpers */ > +#define device_node dt_device_node > +#define of_phandle_args dt_phandle_args > +#define of_device_id dt_device_match > +#define of_match_node dt_match_node > +#define of_property_read_u32(np, pname, out) (!dt_property_read_u32(np, > pname, out)) > +#define of_property_read_bool dt_property_read_bool > +#define of_parse_phandle_with_args dt_parse_phandle_with_args > >>> > >>> Given all the changes to the file by the previous patches we are > >>> basically fully (or almost fully) adapting this code to Xen. > >>> > >>> So at that point I wonder if we should just as well make these changes > >>> (e.g. s/of_phandle_args/dt_phandle_args/g) to the code too. > >> > >> I have already accepted the fact that keeping Linux code as-is is nearly > >> impossible without much workaround :). The benefits tends to also limited > >> as > >> we noticed for the SMMU driver. > >> > >> I would like to point out that this may make quite difficult (if not > >> impossible) to revert the previous patches which remove support for some > >> features (e.g. atomic, MSI, ATS). > >> > >> If we are going to adapt the code to Xen (I'd like to keep Linux code style > >> though), then I think we should consider to keep code that may be useful in > >> the near future (at least MSI, ATS). > > > > (I am fine with keeping the Linux code style.) > > > > We could try to keep the code as similar to Linux as possible. This > > didn't work out in the past. > > > > Otherwise, we could fully adapt the driver to Xen. If we fully adapt the > > driver to Xen (code style aside) it is better to be consistent and also > > do substitutions like s/of_phandle_args/dt_phandle_args/g. Then the > > policy becomes clear: the code comes from Linux but it is 100% adapted > > to Xen. > > > > > > Now the question about what to do about the MSI and ATS code is a good > > one. We know that we are going to want that code at some point in the > > next 2 years. Like you wrote, if we fully adapt the code to Xen and > > remove MSI and ATS code, then it is going to be harder to add it back. > > > > So maybe keeping the MSI and ATS code for now, even if it cannot work, > > would be better. I think this strategy works well if the MSI and ATS > > code can be disabled easily, i.e. with a couple of lines of code in the > > init function rather than #ifdef everywhere. It doesn't work well if we > > have to add #ifdef everywhere. > > > > It looks like MSI could be disabled adding a couple of lines to > > arm_smmu_setup_msis. > > > > Similarly ATS seems to be easy to disable by forcing ats_enabled to > > false. > > > > So yes, this looks like a good way forward. Rahul, what do you think? > > > I am ok to have the PCI ATS and MSI functionality in the code. > As per the discussion next version of the patch will include below > modification:Please let me know if there are any suggestion overall that > should be added in next version. > > 1. Keep the PCI ATS and MSI functionality code. I'll repeat one point I wrote above that I think it is important: please try to disable ATS and MSI without invasive changes, ideally just a couple of lines to force-disable each feature. > 2. Make the code with XEN compatible ( remove linux compatibility functions) > 3. Keep the Linux coding style for code imported from Linux. > 4. Fix all comments. Sounds good. > I have one query what will be coding style for new code to make driver work > for XEN ? We try to keep the same code style for the entirety of a source file. In this case, the whole driver should use Linux code style (both imported code and new code).
Re: [PATCH v2] xen/irq: Propagate the error from init_one_desc_irq() in init_*_irq_data()
On Wed, 2 Dec 2020, Bertrand Marquis wrote: > > On 28 Nov 2020, at 11:36, Julien Grall wrote: > > > > From: Julien Grall > > > > init_one_desc_irq() can return an error if it is unable to allocate > > memory. While this is unlikely to happen during boot (called from > > init_{,local_}irq_data()), it is better to harden the code by > > propagting the return value. > > > > Spotted by coverity. > > > > CID: 106529 > > > > Signed-off-by: Julien Grall > > Reviewed-by: Roger Paul Monné > > Reviewed-by: Bertrand Marquis Acked-by: Stefano Stabellini > > --- > >Changes in v2: > >- Add Roger's reviewed-by for x86 > >- Handle > > --- > > xen/arch/arm/irq.c | 12 ++-- > > xen/arch/x86/irq.c | 7 ++- > > 2 files changed, 16 insertions(+), 3 deletions(-) > > > > diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c > > index 3877657a5277..b71b099e6fa2 100644 > > --- a/xen/arch/arm/irq.c > > +++ b/xen/arch/arm/irq.c > > @@ -88,7 +88,11 @@ static int __init init_irq_data(void) > > for ( irq = NR_LOCAL_IRQS; irq < NR_IRQS; irq++ ) > > { > > struct irq_desc *desc = irq_to_desc(irq); > > -init_one_irq_desc(desc); > > +int rc = init_one_irq_desc(desc); > > + > > +if ( rc ) > > +return rc; > > + > > desc->irq = irq; > > desc->action = NULL; > > } > > @@ -105,7 +109,11 @@ static int init_local_irq_data(void) > > for ( irq = 0; irq < NR_LOCAL_IRQS; irq++ ) > > { > > struct irq_desc *desc = irq_to_desc(irq); > > -init_one_irq_desc(desc); > > +int rc = init_one_irq_desc(desc); > > + > > +if ( rc ) > > +return rc; > > + > > desc->irq = irq; > > desc->action = NULL; > > > > diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c > > index 45966947919e..3ebd684415ac 100644 > > --- a/xen/arch/x86/irq.c > > +++ b/xen/arch/x86/irq.c > > @@ -428,9 +428,14 @@ int __init init_irq_data(void) > > > > for ( irq = 0; irq < nr_irqs_gsi; irq++ ) > > { > > +int rc; > > + > > desc = irq_to_desc(irq); > > desc->irq = irq; > > -init_one_irq_desc(desc); > > + > > +rc = init_one_irq_desc(desc); > > +if ( rc ) > > +return rc; > > } > > for ( ; irq < nr_irqs; irq++ ) > > irq_to_desc(irq)->irq = irq; > > -- > > 2.17.1 > > > > > >
[qemu-mainline test] 157174: regressions - FAIL
flight 157174 qemu-mainline real [real] flight 157185 qemu-mainline real-retest [real] http://logs.test-lab.xenproject.org/osstest/logs/157174/ http://logs.test-lab.xenproject.org/osstest/logs/157185/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-amd64-amd64-libvirt-vhd 19 guest-start/debian.repeat fail REGR. vs. 152631 test-amd64-amd64-xl-qcow2 21 guest-start/debian.repeat fail REGR. vs. 152631 test-armhf-armhf-xl-vhd 17 guest-start/debian.repeat fail REGR. vs. 152631 Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 152631 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 152631 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 152631 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 152631 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 152631 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 152631 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 152631 test-amd64-i386-xl-pvshim14 guest-start fail never pass test-arm64-arm64-xl-seattle 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-xsm 15 migrate-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-amd64-libvirt-vhd 14 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-arm64-arm64-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl 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-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-armhf-armhf-xl-cubietruck 15 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 16 saverestore-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 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-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-credit2 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 15 migrate-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-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-raw 14 migrate-support-checkfail never pass version targeted for testing: qemuud73c46e4a84e47ffc61b8bf7c378b1383e7316b5 baseline version: qemuu1d806cef0e38b5db8347a8e12f214d543204a314 Last test of basis 152631 2020-08-20 09:07:46 Z 105 days Failing since152659 2020-08-21 14:07:39 Z 104 days 217 attempts Testing same since 157142 2020-12-01 20:39:57 Z1 days3 attempts People who touched revisions under test: Aaron Lindsay Alberto Garcia Aleksandar Markovic Alex Bennée Alex Chen Alex Williamson Alexander Bulek
[linux-linus test] 157176: regressions - FAIL
flight 157176 linux-linus real [real] http://logs.test-lab.xenproject.org/osstest/logs/157176/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-amd64-i386-qemut-rhel6hvm-intel 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-qemuu-ws16-amd64 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-xsm7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-qemut-debianhvm-amd64 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-qemuu-debianhvm-amd64-shadow 7 xen-install fail REGR. vs. 152332 test-amd64-i386-qemuu-rhel6hvm-intel 7 xen-install fail REGR. vs. 152332 test-amd64-i386-libvirt 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-qemuu-debianhvm-i386-xsm 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-qemut-ws16-amd64 7 xen-install fail REGR. vs. 152332 test-amd64-coresched-i386-xl 7 xen-install fail REGR. vs. 152332 test-amd64-i386-qemut-rhel6hvm-amd 7 xen-installfail REGR. vs. 152332 test-amd64-i386-qemuu-rhel6hvm-amd 7 xen-installfail REGR. vs. 152332 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 7 xen-install fail REGR. vs. 152332 test-amd64-i386-libvirt-xsm 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-qemuu-debianhvm-amd64 7 xen-install fail REGR. vs. 152332 test-amd64-i386-pair 10 xen-install/src_host fail REGR. vs. 152332 test-amd64-i386-pair 11 xen-install/dst_host fail REGR. vs. 152332 test-amd64-i386-examine 6 xen-install fail REGR. vs. 152332 test-amd64-i386-freebsd10-amd64 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-raw7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-pvshim 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-qemut-debianhvm-i386-xsm 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-shadow 7 xen-install fail REGR. vs. 152332 test-amd64-i386-freebsd10-i386 7 xen-installfail REGR. vs. 152332 test-amd64-i386-xl-qemuu-ovmf-amd64 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-qemut-win7-amd64 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-qemuu-win7-amd64 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-qemut-stubdom-debianhvm-amd64-xsm 7 xen-install fail REGR. vs. 152332 test-amd64-i386-libvirt-pair 10 xen-install/src_host fail REGR. vs. 152332 test-amd64-i386-libvirt-pair 11 xen-install/dst_host fail REGR. vs. 152332 test-arm64-arm64-libvirt-xsm 10 host-ping-check-xen fail REGR. vs. 152332 test-arm64-arm64-xl-xsm 10 host-ping-check-xen fail REGR. vs. 152332 test-arm64-arm64-examine 8 reboot fail REGR. vs. 152332 test-arm64-arm64-xl 8 xen-boot fail REGR. vs. 152332 test-amd64-amd64-amd64-pvgrub 20 guest-stop fail REGR. vs. 152332 test-amd64-amd64-i386-pvgrub 20 guest-stop fail REGR. vs. 152332 test-amd64-i386-xl-qemuu-dmrestrict-amd64-dmrestrict 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl7 xen-install fail REGR. vs. 152332 test-arm64-arm64-xl-seattle 8 xen-boot fail REGR. vs. 152332 Tests which did not succeed, but are not blocking: test-arm64-arm64-xl-credit2 11 leak-check/basis(11)fail blocked in 152332 test-arm64-arm64-xl-credit1 11 leak-check/basis(11)fail blocked in 152332 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 152332 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 152332 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 152332 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 152332 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 152332 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 152332 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 152332 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-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail 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-armhf-armhf-xl-cubietruck 15 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-rtds 15 migrate-support-check
[ovmf test] 157184: all pass - PUSHED
flight 157184 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/157184/ Perfect :-) All tests in this flight passed as required version targeted for testing: ovmf 126115a9fb3f89f8609336c87aa82fe7da19a9aa baseline version: ovmf 484e869dfd3b7e4b6c47cb65ae5d5f499fcc056e Last test of basis 157178 2020-12-03 12:29:37 Z0 days Testing same since 157184 2020-12-03 17:09:49 Z0 days1 attempts People who touched revisions under test: Abner Chang Guo Dong Patrick Rudolph 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 484e869dfd..126115a9fb 126115a9fb3f89f8609336c87aa82fe7da19a9aa -> xen-tested-master
[xen-unstable test] 157182: tolerable FAIL - PUSHED
flight 157182 xen-unstable real [real] http://logs.test-lab.xenproject.org/osstest/logs/157182/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-rtds 20 guest-localmigrate/x10 fail like 157166 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 157166 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 157166 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 157166 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 157166 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 157166 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 157166 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 157166 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 157166 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 157166 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 157166 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 157166 test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-i386-xl-pvshim14 guest-start fail never pass test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-xl 16 saverestore-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-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 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-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail 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-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 14 migrate-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-cubietruck 15 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 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-arm64-arm64-xl-seattle 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 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-armhf-armhf-xl-rtds 15 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail never pass test-armhf-armhf-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-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 version targeted for testing: xen aec46884784c2494a30221da775d4ac2c43a4d42 baseline version: xen cabf60fc32d4cfa1d74a2bdfcdb294a31da5d68e Last test of basis 157166 2020-12-02 23:08:38 Z1 days Testing same since 157182 2020-12-03 16:02:08 Z0 days1 attempts People who touched revisions under test: Andrew Cooper Julien Grall jobs: build-amd64-xsm pass build-arm64-xsm pass build-i386-xsm
[ovmf test] 157191: all pass - PUSHED
flight 157191 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/157191/ Perfect :-) All tests in this flight passed as required version targeted for testing: ovmf 6af76adbbfccd31f4f8753fb0ddbbd9f4372f572 baseline version: ovmf 126115a9fb3f89f8609336c87aa82fe7da19a9aa Last test of basis 157184 2020-12-03 17:09:49 Z0 days Testing same since 157191 2020-12-04 01:39:45 Z0 days1 attempts People who touched revisions under test: Laszlo Ersek Ray Ni 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 126115a9fb..6af76adbbf 6af76adbbfccd31f4f8753fb0ddbbd9f4372f572 -> xen-tested-master
[qemu-mainline test] 157186: regressions - FAIL
flight 157186 qemu-mainline real [real] flight 157196 qemu-mainline real-retest [real] http://logs.test-lab.xenproject.org/osstest/logs/157186/ http://logs.test-lab.xenproject.org/osstest/logs/157196/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-amd64-amd64-libvirt-vhd 19 guest-start/debian.repeat fail REGR. vs. 152631 test-amd64-amd64-xl-qcow2 21 guest-start/debian.repeat fail REGR. vs. 152631 test-armhf-armhf-xl-vhd 17 guest-start/debian.repeat fail REGR. vs. 152631 Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 152631 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 152631 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 152631 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 152631 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 152631 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 152631 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 152631 test-amd64-i386-xl-pvshim14 guest-start fail never pass test-arm64-arm64-xl-seattle 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-xsm 15 migrate-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-amd64-libvirt-vhd 14 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-arm64-arm64-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl 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-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-armhf-armhf-xl-cubietruck 15 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 16 saverestore-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 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-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-credit2 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 15 migrate-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-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-raw 14 migrate-support-checkfail never pass version targeted for testing: qemuud73c46e4a84e47ffc61b8bf7c378b1383e7316b5 baseline version: qemuu1d806cef0e38b5db8347a8e12f214d543204a314 Last test of basis 152631 2020-08-20 09:07:46 Z 105 days Failing since152659 2020-08-21 14:07:39 Z 104 days 218 attempts Testing same since 157142 2020-12-01 20:39:57 Z2 days4 attempts People who touched revisions under test: Aaron Lindsay Alberto Garcia Aleksandar Markovic Alex Bennée Alex Chen Alex Williamson Alexander Bulek
[linux-linus test] 157188: regressions - FAIL
flight 157188 linux-linus real [real] http://logs.test-lab.xenproject.org/osstest/logs/157188/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-amd64-i386-qemut-rhel6hvm-intel 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-qemuu-ws16-amd64 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-xsm7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-qemut-debianhvm-amd64 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-qemuu-debianhvm-amd64-shadow 7 xen-install fail REGR. vs. 152332 test-amd64-i386-qemuu-rhel6hvm-intel 7 xen-install fail REGR. vs. 152332 test-amd64-i386-libvirt 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-qemuu-debianhvm-i386-xsm 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-qemut-ws16-amd64 7 xen-install fail REGR. vs. 152332 test-amd64-coresched-i386-xl 7 xen-install fail REGR. vs. 152332 test-amd64-i386-qemut-rhel6hvm-amd 7 xen-installfail REGR. vs. 152332 test-amd64-i386-qemuu-rhel6hvm-amd 7 xen-installfail REGR. vs. 152332 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 7 xen-install fail REGR. vs. 152332 test-amd64-i386-libvirt-xsm 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-qemuu-debianhvm-amd64 7 xen-install fail REGR. vs. 152332 test-amd64-i386-pair 10 xen-install/src_host fail REGR. vs. 152332 test-amd64-i386-pair 11 xen-install/dst_host fail REGR. vs. 152332 test-amd64-i386-examine 6 xen-install fail REGR. vs. 152332 test-amd64-i386-freebsd10-amd64 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-raw7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-pvshim 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-qemut-debianhvm-i386-xsm 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-shadow 7 xen-install fail REGR. vs. 152332 test-amd64-i386-freebsd10-i386 7 xen-installfail REGR. vs. 152332 test-amd64-i386-xl-qemuu-ovmf-amd64 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-qemut-win7-amd64 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-qemuu-win7-amd64 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-qemut-stubdom-debianhvm-amd64-xsm 7 xen-install fail REGR. vs. 152332 test-amd64-i386-libvirt-pair 10 xen-install/src_host fail REGR. vs. 152332 test-amd64-i386-libvirt-pair 11 xen-install/dst_host fail REGR. vs. 152332 test-arm64-arm64-xl 10 host-ping-check-xen fail REGR. vs. 152332 test-arm64-arm64-examine 8 reboot fail REGR. vs. 152332 test-amd64-amd64-amd64-pvgrub 20 guest-stop fail REGR. vs. 152332 test-amd64-amd64-i386-pvgrub 20 guest-stop fail REGR. vs. 152332 test-amd64-i386-xl-qemuu-dmrestrict-amd64-dmrestrict 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl7 xen-install fail REGR. vs. 152332 test-arm64-arm64-xl-seattle 10 host-ping-check-xen fail REGR. vs. 152332 test-arm64-arm64-xl-xsm 10 host-ping-check-xen fail in 157176 REGR. vs. 152332 test-arm64-arm64-libvirt-xsm 10 host-ping-check-xen fail in 157176 REGR. vs. 152332 Tests which are failing intermittently (not blocking): test-arm64-arm64-xl 8 xen-boot fail in 157176 pass in 157188 test-arm64-arm64-xl-seattle 8 xen-boot fail in 157176 pass in 157188 test-arm64-arm64-xl-credit1 8 xen-boot fail pass in 157176 test-arm64-arm64-xl-xsm 8 xen-boot fail pass in 157176 test-arm64-arm64-xl-credit2 8 xen-boot fail pass in 157176 test-amd64-amd64-examine 4 memdisk-try-append fail pass in 157176 test-arm64-arm64-libvirt-xsm 8 xen-boot fail pass in 157176 Tests which did not succeed, but are not blocking: test-arm64-arm64-xl-credit2 11 leak-check/basis(11) fail in 157176 blocked in 152332 test-arm64-arm64-xl-credit1 11 leak-check/basis(11) fail in 157176 blocked in 152332 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 152332 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 152332 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 152332 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 152332 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 152332 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 152332 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 152332 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-thunderx 15 migrate-support-checkfail never
Re: [PATCH v5 1/4] domctl: introduce a new domain create flag, XEN_DOMCTL_CDF_evtchn_fifo, ...
On 03.12.2020 18:07, Paul Durrant wrote: >> From: Jan Beulich >> Sent: 03 December 2020 15:57 >> >> On 03.12.2020 16:45, Paul Durrant wrote: From: Jan Beulich Sent: 03 December 2020 15:23 On 03.12.2020 13:41, Paul Durrant wrote: > From: Paul Durrant > > ...to control the visibility of the FIFO event channel operations > (EVTCHNOP_init_control, EVTCHNOP_expand_array, and EVTCHNOP_set_priority) > to > the guest. > > These operations were added to the public header in commit d2d50c2f308f > ("evtchn: add FIFO-based event channel ABI") and the first implementation > appeared in the two subsequent commits: edc8872aeb4a ("evtchn: implement > EVTCHNOP_set_priority and add the set_priority hook") and 88910061ec61 > ("evtchn: add FIFO-based event channel hypercalls and port ops"). Prior to > that, a guest issuing those operations would receive a return value of > -ENOSYS (not implemented) from Xen. Guests aware of the FIFO operations > but > running on an older (pre-4.4) Xen would fall back to using the 2-level > event > channel interface upon seeing this return value. > > Unfortunately the uncontrolable appearance of these new operations in Xen > 4.4 > onwards has implications for hibernation of some Linux guests. During > resume > from hibernation, there are two kernels involved: the "boot" kernel and > the > "resume" kernel. The guest boot kernel may default to use FIFO operations > and > instruct Xen via EVTCHNOP_init_control to switch from 2-level to FIFO. On > the > other hand, the resume kernel keeps assuming 2-level, because it was > hibernated > on a version of Xen that did not support the FIFO operations. > > To maintain compatibility it is necessary to make Xen behave as it did > before the new operations were added and hence the code in this patch > ensures > that, if XEN_DOMCTL_CDF_evtchn_fifo is not set, the FIFO event channel > operations will again result in -ENOSYS being returned to the guest. I have to admit I'm now even more concerned of the control for such going into Xen, the more with the now 2nd use in the subsequent patch. The implication of this would seem to be that whenever we add new hypercalls or sub-ops, a domain creation control would also need adding determining whether that new sub-op is actually okay to use by a guest. Or else I'd be keen to up front see criteria at least roughly outlined by which it could be established whether such an override control is needed. >>> >>> Ultimately I think any new hypercall (or related set of hypercalls) added >>> to the ABI needs to be >> opt-in on a per-domain basis, so that we know that from when a domain is >> first created it will not see >> a change in its environment unless the VM administrator wants that to happen. >> >> A new hypercall appearing is a change to the guest's environment, yes, >> but a backwards compatible one. I don't see how this would harm a guest. > > Say we have a guest which is aware of the newer Xen and is coded to use the > new hypercall *but* we start it on an older Xen where the new hypercall is > not implemented. *Then* we migrate it to the newer Xen... the new hypercall > suddenly becomes available and changes the guest behaviour. In the worst > case, the guest is sufficiently confused that it crashes. If a guest recognizes a new hypercall is unavailable, why would it ever try to make use of it again, unless it knows it may appear after having been migrated (which implies it's prepared for the sudden appearance)? This to me still looks like an attempt to work around guest bugs. And a halfhearted one, as you cherry pick just two out of many additions to the original 3.0 ABI. >> This and ... >> I'm also not convinced such controls really want to be opt-in rather than opt-out. >>> >>> They really need to be opt-in I think. From a cloud provider PoV it is >>> important that nothing in a >> customer's environment changes unless we want it to. Otherwise we have no >> way to deploy an updated >> hypervisor version without risking crashing their VMs. >> >> ... this sound to me more like workarounds for buggy guests than >> functionality the hypervisor _needs_ to have. (I can appreciate >> the specific case here for the specific scenario you provide as >> an exception.) > > If we want to have a hypervisor that can be used in a cloud environment > then Xen absolutely needs this capability. As per above you can conclude that I'm still struggling to see the "why" part here. > --- a/xen/arch/arm/domain.c > +++ b/xen/arch/arm/domain.c > @@ -622,7 +622,8 @@ int arch_sanitise_domain_config(struct > xen_domctl_createdomain *config) > unsigned int max_vcpus; > > /* HVM and HAP must be set. IOMMU may or may not be */ > -if ( (config-