Xen Coding style and clang-format
Hi all, I would like to bring up a discussion about a Xen-checker tool, the development of which has been temporarily suspended. The idea of this tool is to use the clang-format approach as a base for Xen ‘checkpatch’ process. The new tool consists of modified clang-format binary and modified clang-format-diff.py python script to automate Xen patches format checking and reformatting. The tool can be used as a pre-commit hook to check and format every patch automatically. Xen checker is currently in a state of testing, but there are controversial points regarding the coding style that I would like to discuss with the community and make a unanimous decision on the correctness of the formatting. I would like to know your opinion on the following coding style cases. Which option do you think is correct? 1) Function prototype when the string length is longer than the allowed one -static int __init -acpi_parse_gic_cpu_interface(struct acpi_subtable_header *header, - const unsigned long end) +static int __init acpi_parse_gic_cpu_interface( +struct acpi_subtable_header *header, const unsigned long end) 2) Wrapping an operator to a new line when the length of the line is longer than the allowed one -if ( table->revision > 6 - || (table->revision == 6 && fadt->minor_revision >= 0) ) +if ( table->revision > 6 || + (table->revision == 6 && fadt->minor_revision >= 0) ) 3) define code style -#define ALLREGS \ -C(r0,r0_usr); C(r1,r1_usr); C(r2,r2_usr); C(r3,r3_usr); \ -C(cpsr,cpsr) +#define ALLREGS\ +C(r0, r0_usr); \ +C(r1, r1_usr); \ +C(r2, r2_usr); \ 4) Comment style -/* PC should be always a multiple of 4, as Xen is using ARM instruction set */ +/* PC should be always a multiple of 4, as Xen is using ARM instruction set + */ Please find below the repo created by Viktor Mitin with xen clang- format under the next link (branch xen-clang-format): https://github.com/xen-troops/llvm-project/tree/xen-clang-format The next script can be used as an example of how to build clang-format: https://github.com/viktor-mitin/xen-clang-format-example/blob/master/build_clang_format.sh Regards, Anastasiia Lukianenko
Re: Xen Coding style and clang-format
Hi, On Wed, 2020-09-30 at 10:24 +, George Dunlap wrote: > > On Sep 30, 2020, at 10:57 AM, Jan Beulich > > wrote: > > > > On 30.09.2020 11:18, Anastasiia Lukianenko wrote: > > > I would like to know your opinion on the following coding style > > > cases. > > > Which option do you think is correct? > > > 1) Function prototype when the string length is longer than the > > > allowed > > > one > > > -static int __init > > > -acpi_parse_gic_cpu_interface(struct acpi_subtable_header > > > *header, > > > - const unsigned long end) > > > +static int __init acpi_parse_gic_cpu_interface( > > > +struct acpi_subtable_header *header, const unsigned long > > > end) > > > > Both variants are deemed valid style, I think (same also goes for > > function calls with this same problem). In fact you mix two > > different style aspects together (placement of parameter > > declarations and placement of return type etc) - for each > > individually both forms are deemed acceptable, I think. > > If we’re going to have a tool go through and report (correct?) all > these coding style things, it’s an opportunity to think if we want to > add new coding style requirements (or change existing requirements). > I am ready to discuss new requirements and implement them in rules of the Xen Coding style checker. > -George Regards, Anastasiia
Re: Xen Coding style and clang-format
Hi all, On Thu, 2020-10-01 at 10:06 +, George Dunlap wrote: > > On Oct 1, 2020, at 10:06 AM, Anastasiia Lukianenko < > > anastasiia_lukiane...@epam.com> wrote: > > > > Hi, > > > > On Wed, 2020-09-30 at 10:24 +, George Dunlap wrote: > > > > On Sep 30, 2020, at 10:57 AM, Jan Beulich > > > > wrote: > > > > > > > > On 30.09.2020 11:18, Anastasiia Lukianenko wrote: > > > > > I would like to know your opinion on the following coding > > > > > style > > > > > cases. > > > > > Which option do you think is correct? > > > > > 1) Function prototype when the string length is longer than > > > > > the > > > > > allowed > > > > > one > > > > > -static int __init > > > > > -acpi_parse_gic_cpu_interface(struct acpi_subtable_header > > > > > *header, > > > > > - const unsigned long end) > > > > > +static int __init acpi_parse_gic_cpu_interface( > > > > > +struct acpi_subtable_header *header, const unsigned long > > > > > end) > > > > > > > > Both variants are deemed valid style, I think (same also goes > > > > for > > > > function calls with this same problem). In fact you mix two > > > > different style aspects together (placement of parameter > > > > declarations and placement of return type etc) - for each > > > > individually both forms are deemed acceptable, I think. > > > > > > If we’re going to have a tool go through and report (correct?) > > > all > > > these coding style things, it’s an opportunity to think if we > > > want to > > > add new coding style requirements (or change existing > > > requirements). > > > > > > > I am ready to discuss new requirements and implement them in rules > > of > > the Xen Coding style checker. > > Thank you. :-) But what I meant was: Right now we don’t require one > approach or the other for this specific instance. Do we want to > choose one? > > I think in this case it makes sense to do the easiest thing. If it’s > easy to make the current tool accept both styles, let’s just do that > for now. If the tool currently forces you to choose one of the two > styles, let’s choose one. > > -George During the detailed study of the Xen checker and the Clang-Format Style Options, it was found that this tool, unfortunately, is not so flexible to allow the author to independently choose the formatting style in situations that I described in the last letter. For example define code style: -#define ALLREGS \ -C(r0, r0_usr); C(r1, r1_usr); C(r2, r2_usr); C(r3, r3_usr); \ -C(cpsr, cpsr) +#define ALLREGS\ +C(r0, r0_usr); \ +C(r1, r1_usr); \ +C(r2, r2_usr); \ There are also some inconsistencies in the formatting of the tool and what is written in the hyung coding style rules. For example, the comment format: -/* PC should be always a multiple of 4, as Xen is using ARM instruction set */ +/* PC should be always a multiple of 4, as Xen is using ARM instruction set + */ I would like to draw your attention to the fact that the comment behaves in this way, since the line length exceeds the allowable one. The ReflowComments option is responsible for this format. It can be turned off, but then the result will be: ReflowComments=false: /* second veryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLongComment with plenty of information */ ReflowComments=true: /* second veryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLongComment with plenty of * information */ So I want to know if the community is ready to add new formatting options and edit old ones. Below I will give examples of what corrections the checker is currently making (the first variant in each case is existing code and the second variant is formatted by checker). If they fit the standards, then I can document them in the coding style. If not, then I try to configure the checker. But the idea is that we need to choose one option that will be considered correct. 1) Function prototype when the string length is longer than the allowed -static int __init -acpi_parse_gic_cpu_interface(struct acpi_subtable_header *header, - const unsigned long end) +static int __init acpi_parse_gic_cpu_interface( +struct acpi_subtable_header *header, const unsigned long end) 2) Wrapping an operation to a new line when the string length is longer than the allowed -status = acpi_get_table(ACPI_SIG_SPCR, 0, -(struct acpi_table_header **)&spcr); +
Re: Xen Coding style and clang-format
Hi all, On Wed, 2020-10-07 at 18:07 -0700, Stefano Stabellini wrote: > On Wed, 7 Oct 2020, Anastasiia Lukianenko wrote: > > On Thu, 2020-10-01 at 10:06 +, George Dunlap wrote: > > > > On Oct 1, 2020, at 10:06 AM, Anastasiia Lukianenko < > > > > anastasiia_lukiane...@epam.com> wrote: > > > > > > > > Hi, > > > > > > > > On Wed, 2020-09-30 at 10:24 +, George Dunlap wrote: > > > > > > On Sep 30, 2020, at 10:57 AM, Jan Beulich < > > > > > > jbeul...@suse.com> > > > > > > wrote: > > > > > > > > > > > > On 30.09.2020 11:18, Anastasiia Lukianenko wrote: > > > > > > > I would like to know your opinion on the following coding > > > > > > > style > > > > > > > cases. > > > > > > > Which option do you think is correct? > > > > > > > 1) Function prototype when the string length is longer > > > > > > > than > > > > > > > the > > > > > > > allowed > > > > > > > one > > > > > > > -static int __init > > > > > > > -acpi_parse_gic_cpu_interface(struct acpi_subtable_header > > > > > > > *header, > > > > > > > - const unsigned long end) > > > > > > > +static int __init acpi_parse_gic_cpu_interface( > > > > > > > +struct acpi_subtable_header *header, const unsigned > > > > > > > long > > > > > > > end) > > > > > > > > > > > > Both variants are deemed valid style, I think (same also > > > > > > goes > > > > > > for > > > > > > function calls with this same problem). In fact you mix two > > > > > > different style aspects together (placement of parameter > > > > > > declarations and placement of return type etc) - for each > > > > > > individually both forms are deemed acceptable, I think. > > > > > > > > > > If we’re going to have a tool go through and report > > > > > (correct?) > > > > > all > > > > > these coding style things, it’s an opportunity to think if we > > > > > want to > > > > > add new coding style requirements (or change existing > > > > > requirements). > > > > > > > > > > > > > I am ready to discuss new requirements and implement them in > > > > rules > > > > of > > > > the Xen Coding style checker. > > > > > > Thank you. :-) But what I meant was: Right now we don’t require > > > one > > > approach or the other for this specific instance. Do we want to > > > choose one? > > > > > > I think in this case it makes sense to do the easiest thing. If > > > it’s > > > easy to make the current tool accept both styles, let’s just do > > > that > > > for now. If the tool currently forces you to choose one of the > > > two > > > styles, let’s choose one. > > > > > > -George > > > > During the detailed study of the Xen checker and the Clang-Format > > Style > > Options, it was found that this tool, unfortunately, is not so > > flexible > > to allow the author to independently choose the formatting style in > > situations that I described in the last letter. For example define > > code > > style: > > -#define ALLREGS \ > > -C(r0, r0_usr); C(r1, r1_usr); C(r2, r2_usr); C(r3, > > r3_usr); \ > > -C(cpsr, cpsr) > > +#define ALLREGS\ > > +C(r0, r0_usr); \ > > +C(r1, r1_usr); \ > > +C(r2, r2_usr); \ > > There are also some inconsistencies in the formatting of the tool > > and > > what is written in the hyung coding style rules. For example, the > > comment format: > > -/* PC should be always a multiple of 4, as Xen is using ARM > > instruction set */ > > +/* PC should be always a multiple of 4, as Xen is using ARM > > instruction set > > + */ > > I would like to draw your attention to the fact that the comment > > behaves in this way, since the line length exceeds the allowable > > one. > > The ReflowComments option is responsible for this format. It can be > > turned off, but then the result will be: > > ReflowCom
Re: Xen Coding style and clang-format
Hi all, On Tue, 2020-10-13 at 14:30 +0200, Jan Beulich wrote: > On 12.10.2020 20:09, George Dunlap wrote: > > > On Oct 7, 2020, at 11:19 AM, Anastasiia Lukianenko < > > > anastasiia_lukiane...@epam.com> wrote: > > > So I want to know if the community is ready to add new formatting > > > options and edit old ones. Below I will give examples of what > > > corrections the checker is currently making (the first variant in > > > each > > > case is existing code and the second variant is formatted by > > > checker). > > > If they fit the standards, then I can document them in the coding > > > style. If not, then I try to configure the checker. But the idea > > > is > > > that we need to choose one option that will be considered > > > correct. > > > 1) Function prototype when the string length is longer than the > > > allowed > > > -static int __init > > > -acpi_parse_gic_cpu_interface(struct acpi_subtable_header > > > *header, > > > - const unsigned long end) > > > +static int __init acpi_parse_gic_cpu_interface( > > > +struct acpi_subtable_header *header, const unsigned long > > > end) > > > > Jan already commented on this one; is there any way to tell the > > checker to ignore this discrepancy? > > > > If not, I think we should just choose one; I’d go with the latter. If it turns out to make the checker more flexible, then I will try to add both options as correct. > > > > > 2) Wrapping an operation to a new line when the string length is > > > longer > > > than the allowed > > > -status = acpi_get_table(ACPI_SIG_SPCR, 0, > > > -(struct acpi_table_header **)&spcr); > > > +status = > > > +acpi_get_table(ACPI_SIG_SPCR, 0, (struct > > > acpi_table_header > > > **)&spcr); > > > > Personally I prefer the first version. > > Same here. Until I found a way to save the first option, I think this case may remain in the opinion of the author. > > > > 3) Space after brackets > > > -return ((char *) base + offset); > > > +return ((char *)base + offset); > > > > This seems like a good change to me. > > > > > 4) Spaces in brackets in switch condition > > > -switch ( domctl->cmd ) > > > +switch (domctl->cmd) > > > > This is explicitly against the current coding style. Fixed this in the new version of checker. > > > > > 5) Spaces in brackets in operation > > > -imm = ( insn >> BRANCH_INSN_IMM_SHIFT ) & > > > BRANCH_INSN_IMM_MASK; > > > +imm = (insn >> BRANCH_INSN_IMM_SHIFT) & > > > BRANCH_INSN_IMM_MASK; > > > > I *think* this is already the official style. > > > > > 6) Spaces in brackets in return > > > -return ( !sym->name[2] || sym->name[2] == '.' ); > > > +return (!sym->name[2] || sym->name[2] == '.'); > > > > Similarly, I think this is already the official style. > > > > > 7) Space after sizeof > > > -clean_and_invalidate_dcache_va_range(new_ptr, sizeof > > > (*new_ptr) * > > > len); > > > +clean_and_invalidate_dcache_va_range(new_ptr, > > > sizeof(*new_ptr) * > > > len); > > > > I think this is correct. > > I agree with George on all of the above. > > > > 8) Spaces before comment if it’s on the same line > > > -case R_ARM_MOVT_ABS: /* S + A */ > > > +case R_ARM_MOVT_ABS:/* S + A */ > > > > > > -if ( tmp == 0UL ) /* Are any bits set? */ > > > -return result + size; /* Nope. */ > > > +if ( tmp == 0UL ) /* Are any bits set? */ > > > +return result + size; /* Nope. */ > > > > Seem OK to me. > > I don't think we have any rules how far apart a comment needs > to be; I don't think there should be any complaints or > "corrections" here. > > > > 9) Space after for_each_vcpu > > > -for_each_vcpu(d, v) > > > +for_each_vcpu (d, v) > > > > Er, not sure about this one. This is actually a macro; but > > obviously it looks like for ( ). > > > > I think Jan will probably have an opinion, and I think he’ll be > > back tomorrow; so maybe wait just a day or two before starting to > > prep your series. > > This makes it look like Linux style. In
Re: Xen Coding style and clang-format
Hi all, On Tue, 2020-10-20 at 18:13 +0100, Julien Grall wrote: > Hi, > > On 19/10/2020 19:07, Stefano Stabellini wrote: > > On Fri, 16 Oct 2020, Artem Mygaiev wrote: > > > -Original Message- > > > From: Julien Grall > > > Sent: пятница, 16 октября 2020 г. 13:24 > > > To: Anastasiia Lukianenko ; > > > jbeul...@suse.com; george.dun...@citrix.com > > > Cc: Artem Mygaiev ; vicooo...@gmail.com; > > > xen-devel@lists.xenproject.org; committ...@xenproject.org; > > > viktor.mitin...@gmail.com; Volodymyr Babchuk < > > > volodymyr_babc...@epam.com> > > > Subject: Re: Xen Coding style and clang-format > > > > > > > Hi, > > > > > > > > On 16/10/2020 10:42, Anastasiia Lukianenko wrote: > > > > > Thanks for your advices, which helped me improve the checker. > > > > > I > > > > > understand that there are still some disagreements about the > > > > > formatting, but as I said before, the checker cannot be very > > > > > flexible > > > > > and take into account all the author's ideas. > > > > > > > > I am not sure what you refer by "author's ideas" here. The > > > > checker > > > > should follow a coding style (Xen or a modified version): > > > > - Anything not following the coding style should be > > > > considered as > > > > invalid. > > > > - Anything not written in the coding style should be left > > > > untouched/uncommented by the checker. > > > > > > > > > > Agree > > > > > > > > I suggest using the > > > > > checker not as a mandatory check, but as an indication to the > > > > > author of > > > > > possible formatting errors that he can correct or ignore. > > > > > > > > I can understand that short term we would want to make it > > > > optional so > > > > either the coding style or the checker can be tuned. But I > > > > don't think > > > > this is an ideal situation to be in long term. > > > > > > > > The goal of the checker is to automatically verify the coding > > > > style and > > > > get it consistent across Xen. If we make it optional or it is > > > > "unreliable", then we lose the two benefits and possibly > > > > increase the > > > > contributor frustration as the checker would say A but we need > > > > B. > > > > > > > > Therefore, we need to make sure the checker and the coding > > > > style match. > > > > I don't have any opinions on the approach to achieve that. > > > > > > Of the list of remaining issues from Anastasiia, looks like only > > > items 5 > > > and 6 are conform to official Xen coding style. As for remainning > > > ones, > > > I would like to suggest disabling those that are controversial > > > (items 1, > > > 2, 4, 8, 9, 10). Maybe we want to have further discussion on > > > refining > > > coding style, we can use these as starting point. If we are open > > > to > > > extending style now, I would suggest to add rules that seem to be > > > meaningful (items 3, 7) and keep them in checker. > > > > Good approach. Yes, I would like to keep 3, 7 in the checker. > > > > I would also keep 8 and add a small note to the coding style to say > > that > > comments should be aligned where possible. > > +1 for this. Although, I don't mind the coding style used as long as > we > have a checker and the code is consistent :). > > Cheers, > Thank you for advices :) Now I'm trying to figure out the option that needs to be corrected for the checker to work correctly: Wrapping an operation to a new line when the string length is longer than the allowed -status = acpi_get_table(ACPI_SIG_SPCR, 0, -(struct acpi_table_header **)&spcr); +status = +acpi_get_table(ACPI_SIG_SPCR, 0, (struct acpi_table_header **)&spcr); As it turned out, this case is quite rare and the rule for transferring parameters works correctly in other cases: -status = acpi_get_table(ACPI_SIG_SPCR, 0, &spcr, ACPI_SIG_SPC, 0, ACPI_SIG_SP, 0); +status = acpi_get_table(ACPI_SIG_SPCR, 0, &spcr, ACPI_SIG_SPC, 0, +ACPI_SIG_SP, 0); Thus the checker does not work correctly in the case when the prototype parameter starts with a parenthesis. I'm going to ask clang community is this behavior is expected or maybe it's a bug. Regards, Anastasiia
[PATCH 0/9] Add xl PCI daemon (server for libxl PCI)
From: Anastasiia Lukianenko This work introduces xl PCI daemon that acts as a server for the client in the libxl PCI. The toolstack in Dom0 accesses sysfs to get information about PCI devices and in case of Hardware Domain that sysfs lives in Hardware Domain, so Dom0 can't read that information directly anymore. For that reason we introduce a "proxy" which directs requests from Dom0 to the domain which owns PCI devices, being it Dom0 itself or any other domain, e.g. Hardware Domain. xl PCI daemon is based on vchan-node2 tool and uses the libxenvchan library for messaging transport. It was verified that this model works both remotely (in different domains) and locally (within one domain) as requested by Xen community [1]. The server processes the requests that come from the client. For better interoperability between guest OSes, e.g. Linux vs FreeBSD, the path in the request is not a direct sysfs entry as seen in Dom0, but instead it is a key-word defined in the protocol. For now libxl PCI refers to sysfs in the following functions, therefore such a subset was implemented in xl PCI daemon commands handling: * ‘ls’ - the server receives "dir_id" as a parameter. Xl pcid server transmits information about devices assigned to the PCI driver by sending a reply with the remote directory listing. Client cannot pass the sysfs path, as it is not the same on different operating systems. Therefore, for example, we pass parameter "pciback_driver" and on the server side we refer to "/sys/bus/pci/drivers/pciback" if Linux is used); * ‘write’- the server receives the file name and value as parameters where and what to write; * ‘read_hex’ - xl PCI daemon returns hex value read from the given path; * ‘exists’ - check if path exists; * ‘read_resources’ - returns an array of PCI device resources (start, end, flags) read from given path; * ‘unbind’ - unbinds PCI device; * ‘reset’ - resets PCI device. Requests and responses are formed in the form of a json which is already used by libxenlight (xl). The commands are aimed at preventing the libxl PCI from reading / writing from / to the local sysfs directly. To do this, the libxl PCI itself was changed, vchan and pcid support was added to libxl - instead of working with the read and write functions from / to the sysfs, functionality was added to send requests to the server, then receive and process the response. Libxl vchan is used as transport between libxl and the server side. Libxl vchan adds support for JSON messages processing. Libxl pcid functions allow to process the received message from PCI server and to generate a reply in JSON format for the client. The pcid functions in the libxl allow the use of internal libxl structures and functions, so there is no code duplications on server side. The issue of simultaneous access of several processes to the channel is not resolved as well. For example, the commands “xl pci-assignable-list” and “xl pci-assignable-add” will be sent at the same time. Due to the fact that there is only one channel, the processing of two requests will cause undefined behavior on the server side. In the future, it is necessary to take into account cases when from different domains there can be several requests from a client at the same time or from the same but xl commands are executed concurrently. [1] - https://www.mail-archive.com/xen-devel@lists.xenproject.org/msg84452.html Regards, Anastasiia Anastasiia Lukianenko (9): tools/libs/light: Add vchan support to libxl tools/libs/light: Add functions for handling PCI messages in JSON format tools/xl: Add pcid daemon to xl tools/libs/light: Add "ls" command processing to xl pcid daemon and libxl PCI tools/libs/light: Add "write" command to xl pcid and libxl PCI tools/libs/light: Add "read" command to xl pcid and libxl PCI tools/libs/light: Make Libxl PCI get values from xl pcid instead of libxl side tools/libs/light: Add "unbind" and "read resources" commands to libxl PCI and xl pcid tools/libs/light: Add "reset" and "remove" PCI dev commands to xl pcid daemon and libxl PCI tools/configure | 5 +- tools/helpers/Makefile| 4 +- tools/hotplug/FreeBSD/rc.d/xlpcid.in | 75 +++ tools/hotplug/Linux/init.d/xlpcid.in | 76 +++ tools/hotplug/NetBSD/rc.d/xlpcid.in | 75 +++ tools/include/pcid.h | 73 +++ tools/libs/light/Makefile | 8 +- tools/libs/light/libxl_pci.c | 566 ++ tools/libs/light/libxl_pcid.c | 527 tools/libs/light/libxl_types.idl | 1 + tools/libs/light/libxl_types_internal.idl | 6 + tools/libs/light/libxl_vchan.c| 359 ++ tools/libs/light/libxl_vchan.h| 81 tools/xl/Makefile
[PATCH 1/9] tools/libs/light: Add vchan support to libxl
From: Anastasiia Lukianenko Add possibility to send commands from libxl and execute them on server side. Libxl vchan adds support for JSON messages processing. The using of libxl vchan is preventing the client from libxl from reading and writing from / to the local sysfs directly.To do this, the libxl vchan was added - instead of working with the read and write functions from / to the sysfs, functionality allows to send requests to the server, then receive and process the response. Signed-off-by: Oleksandr Andrushchenko Signed-off-by: Anastasiia Lukianenko --- tools/helpers/Makefile | 4 +- tools/libs/light/Makefile | 7 +- tools/libs/light/libxl_vchan.c | 359 + tools/libs/light/libxl_vchan.h | 81 tools/xl/Makefile | 3 +- 5 files changed, 448 insertions(+), 6 deletions(-) create mode 100644 tools/libs/light/libxl_vchan.c create mode 100644 tools/libs/light/libxl_vchan.h diff --git a/tools/helpers/Makefile b/tools/helpers/Makefile index 1bcc97ea8a..ad2cdf32ad 100644 --- a/tools/helpers/Makefile +++ b/tools/helpers/Makefile @@ -29,12 +29,12 @@ $(INIT_XENSTORE_DOMAIN_OBJS): CFLAGS += $(CFLAGS_libxenlight) all: $(PROGS) xen-init-dom0: $(XEN_INIT_DOM0_OBJS) - $(CC) $(LDFLAGS) -o $@ $(XEN_INIT_DOM0_OBJS) $(LDLIBS_libxenctrl) $(LDLIBS_libxentoollog) $(LDLIBS_libxenstore) $(LDLIBS_libxenlight) $(APPEND_LDFLAGS) + $(CC) $(LDFLAGS) -o $@ $(XEN_INIT_DOM0_OBJS) $(LDLIBS_libxenctrl) $(LDLIBS_libxentoollog) $(LDLIBS_libxenstore) $(LDLIBS_libxenlight) $(LDLIBS_libxenvchan) $(APPEND_LDFLAGS) $(INIT_XENSTORE_DOMAIN_OBJS): _paths.h init-xenstore-domain: $(INIT_XENSTORE_DOMAIN_OBJS) - $(CC) $(LDFLAGS) -o $@ $(INIT_XENSTORE_DOMAIN_OBJS) $(LDLIBS_libxentoollog) $(LDLIBS_libxenstore) $(LDLIBS_libxenctrl) $(LDLIBS_libxenguest) $(LDLIBS_libxenlight) $(APPEND_LDFLAGS) + $(CC) $(LDFLAGS) -o $@ $(INIT_XENSTORE_DOMAIN_OBJS) $(LDLIBS_libxentoollog) $(LDLIBS_libxenstore) $(LDLIBS_libxenctrl) $(LDLIBS_libxenguest) $(LDLIBS_libxenlight) $(LDLIBS_libxenvchan) $(APPEND_LDFLAGS) .PHONY: install install: all diff --git a/tools/libs/light/Makefile b/tools/libs/light/Makefile index 4a4de12610..f5d34b3371 100644 --- a/tools/libs/light/Makefile +++ b/tools/libs/light/Makefile @@ -76,6 +76,7 @@ SRCS-y += libxl.c SRCS-y += libxl_create.c SRCS-y += libxl_dm.c SRCS-y += libxl_pci.c +SRCS-y += libxl_vchan.c SRCS-y += libxl_dom.c SRCS-y += libxl_exec.c SRCS-y += libxl_xshelp.c @@ -176,7 +177,7 @@ LDUSELIBS-y += $(PTHREAD_LIBS) LDUSELIBS-y += -lyajl LDUSELIBS += $(LDUSELIBS-y) -$(LIB_OBJS) $(PIC_OBJS) $(LIBXL_TEST_OBJS): CFLAGS += $(CFLAGS_LIBXL) -include $(XEN_ROOT)/tools/config.h +$(LIB_OBJS) $(PIC_OBJS) $(LIBXL_TEST_OBJS): CFLAGS += $(CFLAGS_LIBXL) $(CFLAGS_libxenevtchn) -include $(XEN_ROOT)/tools/config.h $(ACPI_OBJS) $(ACPI_PIC_OBJS): CFLAGS += -I. -DLIBACPI_STDUTILS=\"$(CURDIR)/libxl_x86_acpi.h\" $(TEST_PROG_OBJS) _libxl.api-for-check: CFLAGS += $(CFLAGS_libxentoollog) $(CFLAGS_libxentoolcore) libxl_dom.o libxl_dom.opic: CFLAGS += -I$(XEN_ROOT)/tools # include libacpi/x86.h @@ -240,13 +241,13 @@ libxenlight_test.so: $(PIC_OBJS) $(LIBXL_TEST_OBJS) $(CC) $(LDFLAGS) -Wl,$(SONAME_LDFLAG) -Wl,libxenlight.so.$(MAJOR) $(SHLIB_LDFLAGS) -o $@ $^ $(LDUSELIBS) $(APPEND_LDFLAGS) test_%: test_%.o test_common.o libxenlight_test.so - $(CC) $(LDFLAGS) -o $@ $^ $(filter-out %libxenlight.so, $(LDLIBS_libxenlight)) $(LDLIBS_libxentoollog) $(LDLIBS_libxentoolcore) -lyajl $(APPEND_LDFLAGS) + $(CC) $(LDFLAGS) -o $@ $^ $(filter-out %libxenlight.so, $(LDLIBS_libxenlight)) $(LDLIBS_libxentoollog) $(LDLIBS_libxentoolcore) $(LDLIBS_libxenvchan) -lyajl $(APPEND_LDFLAGS) libxl-save-helper: $(SAVE_HELPER_OBJS) libxenlight.so $(CC) $(LDFLAGS) -o $@ $(SAVE_HELPER_OBJS) $(LDLIBS_libxentoollog) $(LDLIBS_libxenctrl) $(LDLIBS_libxenguest) $(LDLIBS_libxentoolcore) $(APPEND_LDFLAGS) testidl: testidl.o libxenlight.so - $(CC) $(LDFLAGS) -o $@ testidl.o $(LDLIBS_libxenlight) $(LDLIBS_libxentoollog) $(LDLIBS_libxentoolcore) $(APPEND_LDFLAGS) + $(CC) $(LDFLAGS) -o $@ testidl.o $(LDLIBS_libxenlight) $(LDLIBS_libxentoollog) $(LDLIBS_libxentoolcore) $(LDLIBS_libxenvchan) $(APPEND_LDFLAGS) install: installlocal $(LIBHEADERS) diff --git a/tools/libs/light/libxl_vchan.c b/tools/libs/light/libxl_vchan.c new file mode 100644 index 00..b533243fd3 --- /dev/null +++ b/tools/libs/light/libxl_vchan.c @@ -0,0 +1,359 @@ +/* + * Vchan support for JSON messages processing + * + * Copyright (C) 2021 EPAM Systems Inc. + * Author: Oleksandr Andrushchenko + * Author: Anastasiia Lukianenko + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU Lesser General Public License as published + * by the Free Software Foundation; version 2.1 only. with the special + * exception on linking described in file LICENSE. + * + * This
[PATCH 2/9] tools/libs/light: Add functions for handling PCI messages in JSON format
From: Anastasiia Lukianenko
These functions allow to process the received message from PCI server and
to generate a reply in JSON format for the client.
Signed-off-by: Anastasiia Lukianenko
---
tools/include/pcid.h | 49
tools/libs/light/Makefile | 1 +
tools/libs/light/libxl_pcid.c | 134 ++
tools/libs/light/libxl_types.idl | 1 +
tools/libs/light/libxl_types_internal.idl | 6 +
5 files changed, 191 insertions(+)
create mode 100644 tools/include/pcid.h
create mode 100644 tools/libs/light/libxl_pcid.c
diff --git a/tools/include/pcid.h b/tools/include/pcid.h
new file mode 100644
index 00..59a7aad64a
--- /dev/null
+++ b/tools/include/pcid.h
@@ -0,0 +1,49 @@
+/*
+Common definitions for Xen PCI client-server protocol.
+Copyright (C) 2021 EPAM Systems Inc.
+
+This library is free software; you can redistribute it and/or
+modify it under the terms of the GNU Lesser General Public
+License as published by the Free Software Foundation; either
+version 2.1 of the License, or (at your option) any later version.
+
+This library is distributed in the hope that it will be useful,
+but WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+Lesser General Public License for more details.
+
+You should have received a copy of the GNU Lesser General Public
+License along with this library; If not, see
<http://www.gnu.org/licenses/>.
+*/
+
+#ifndef PCID_H
+#define PCID_H
+
+#define PCID_XS_DIR "/local/domain/"
+#define PCID_XS_PATH"/data/pcid-vchan"
+
+#define PCI_RECEIVE_BUFFER_SIZE 4096
+#define PCI_MAX_SIZE_RX_BUF MB(1)
+
+#define PCID_MSG_FIELD_ID"id"
+#define PCID_MSG_FIELD_ARGS "arguments"
+
+#define PCID_PCIBACK_DRIVER "pciback_driver"
+
+#if defined(__linux__)
+#define SYSFS_PCIBACK_DRIVER "/sys/bus/pci/drivers/pciback"
+#endif
+
+int libxl_pcid_process(libxl_ctx *ctx);
+
+#endif /* PCID_H */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "linux"
+ * indent-tabs-mode: t
+ * c-basic-offset: 8
+ * tab-width: 8
+ * End:
+ */
diff --git a/tools/libs/light/Makefile b/tools/libs/light/Makefile
index f5d34b3371..40e3a7d7ce 100644
--- a/tools/libs/light/Makefile
+++ b/tools/libs/light/Makefile
@@ -76,6 +76,7 @@ SRCS-y += libxl.c
SRCS-y += libxl_create.c
SRCS-y += libxl_dm.c
SRCS-y += libxl_pci.c
+SRCS-y += libxl_pcid.c
SRCS-y += libxl_vchan.c
SRCS-y += libxl_dom.c
SRCS-y += libxl_exec.c
diff --git a/tools/libs/light/libxl_pcid.c b/tools/libs/light/libxl_pcid.c
new file mode 100644
index 00..6317c77a3c
--- /dev/null
+++ b/tools/libs/light/libxl_pcid.c
@@ -0,0 +1,134 @@
+/*
+Utils for xl pcid daemon
+
+Copyright (C) 2021 EPAM Systems Inc.
+
+This library is free software; you can redistribute it and/or
+modify it under the terms of the GNU Lesser General Public
+License as published by the Free Software Foundation; either
+version 2.1 of the License, or (at your option) any later version.
+
+This library is distributed in the hope that it will be useful,
+but WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+Lesser General Public License for more details.
+
+You should have received a copy of the GNU Lesser General Public
+License along with this library; If not, see
<http://www.gnu.org/licenses/>.
+ */
+
+#define _GNU_SOURCE // required for strchrnul()
+
+#include "libxl_osdeps.h" /* must come before any other headers */
+
+#include "libxl_internal.h"
+#include "libxl_vchan.h"
+
+#include
+#include
+
+#include
+#include
+
+#include
+#include
+#include
+
+#define DOM0_ID 0
+
+static int pcid_handle_message(libxl__gc *gc, const libxl__json_object
*request,
+ libxl__json_object **result)
+{
+const libxl__json_object *command_obj;
+char *command_name;
+
+command_obj = libxl__json_map_get(VCHAN_MSG_EXECUTE, request, JSON_ANY);
+if (!command_obj) {
+LOGE(ERROR, "Execution command not found\n");
+return ERROR_FAIL;
+}
+command_name = command_obj->u.string;
+
+return 0;
+}
+
+static char *pcid_prepare_reply(libxl__gc *gc, const char *cmd,
+libxl__json_object *result, int id)
+{
+yajl_gen hand = NULL;
+/* memory for 'buf' is owned by 'hand' */
+const unsigned char *buf;
+libxl_yajl_length len;
+yajl_gen_status s;
+char *ret = NULL;
+int rc;
+
+hand = libxl_yajl_gen_alloc(NULL);
+if (!hand) {
+LOGE(ERROR, "Error with hand allocation\n");
+goto out;
+}
+
+#if HA
[PATCH 4/9] tools/libs/light: Add "ls" command processing to xl pcid daemon and libxl PCI
From: Anastasiia Lukianenko
Messaging format is based on json style. Xl pcid receives "ls" command from
client and sends a reply:
1. success case - array of directory's names
2. error case - message with "error" in return
Xl pcid server transmits information about devices assigned to the PCI
driver by sending reply with directory's names. Libxl PCI processes reply
message instead of opening directory in libxl side.
Signed-off-by: Anastasiia Lukianenko
---
tools/include/pcid.h | 7 ++
tools/libs/light/libxl_pci.c | 136 +-
tools/libs/light/libxl_pcid.c | 59 +++
3 files changed, 184 insertions(+), 18 deletions(-)
diff --git a/tools/include/pcid.h b/tools/include/pcid.h
index 59a7aad64a..e9c3c497c6 100644
--- a/tools/include/pcid.h
+++ b/tools/include/pcid.h
@@ -28,12 +28,19 @@
#define PCID_MSG_FIELD_ID"id"
#define PCID_MSG_FIELD_ARGS "arguments"
+#define PCID_CMD_LIST"ls"
+#define PCID_CMD_DIR_ID "dir_id"
+
#define PCID_PCIBACK_DRIVER "pciback_driver"
#if defined(__linux__)
#define SYSFS_PCIBACK_DRIVER "/sys/bus/pci/drivers/pciback"
#endif
+#define PCI_INFO_PATH "/libxl/pci"
+#define PCI_BDF_XSPATH "%04x-%02x-%02x-%01x"
+#define PCI_BDF"%04x:%02x:%02x.%01x"
+
int libxl_pcid_process(libxl_ctx *ctx);
#endif /* PCID_H */
diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
index 30e203c264..6534b70777 100644
--- a/tools/libs/light/libxl_pci.c
+++ b/tools/libs/light/libxl_pci.c
@@ -17,6 +17,9 @@
#include "libxl_osdeps.h" /* must come before any other headers */
#include "libxl_internal.h"
+#include "libxl_vchan.h"
+
+#include
#define PCI_BDF"%04x:%02x:%02x.%01x"
#define PCI_BDF_SHORT "%02x:%02x.%01x"
@@ -47,6 +50,100 @@ static void pci_struct_fill(libxl_device_pci *pci, unsigned
int domain,
pci->func = func;
}
+static libxl__pcid_message_type pci_response_type(const libxl__json_object *o)
+{
+libxl__pcid_message_type type;
+libxl__json_map_node *node = NULL;
+int i;
+
+for (i = 0; (node = libxl__json_map_node_get(o, i)); i++) {
+if (libxl__pcid_message_type_from_string(node->map_key, &type) == 0)
+return type;
+}
+return LIBXL__PCID_MESSAGE_TYPE_INVALID;
+}
+
+static int pci_handle_msg(libxl__gc *gc, const libxl__json_object *request,
+ libxl__json_object **result)
+{
+libxl__pcid_message_type type = pci_response_type(request);
+
+if (type == LIBXL__PCID_MESSAGE_TYPE_RETURN)
+*result = (libxl__json_object *)libxl__json_map_get(VCHAN_MSG_RETURN,
+request, JSON_ANY);
+
+return 0;
+}
+
+static char *pci_prepare_cmd(libxl__gc *gc, const char *cmd,
+ libxl__json_object *args, int id)
+{
+yajl_gen hand = NULL;
+/* memory for 'buf' is owned by 'hand' */
+const unsigned char *buf;
+libxl_yajl_length len;
+yajl_gen_status s;
+char *ret = NULL;
+
+hand = libxl_yajl_gen_alloc(NULL);
+
+if (!hand)
+return NULL;
+
+#if HAVE_YAJL_V2
+/* Disable beautify for data */
+yajl_gen_config(hand, yajl_gen_beautify, 0);
+#endif
+
+yajl_gen_map_open(hand);
+libxl__yajl_gen_asciiz(hand, VCHAN_MSG_EXECUTE);
+libxl__yajl_gen_asciiz(hand, cmd);
+libxl__yajl_gen_asciiz(hand, PCID_MSG_FIELD_ID);
+yajl_gen_integer(hand, id);
+if (args) {
+libxl__yajl_gen_asciiz(hand, PCID_MSG_FIELD_ARGS);
+libxl__json_object_to_yajl_gen(gc, hand, args);
+}
+yajl_gen_map_close(hand);
+
+s = yajl_gen_get_buf(hand, &buf, &len);
+
+if (s != yajl_gen_status_ok)
+goto out;
+
+ret = libxl__sprintf(gc, "%*.*s" END_OF_MESSAGE,
+ (int)len, (int)len, buf);
+
+out:
+yajl_gen_free(hand);
+return ret;
+}
+
+static struct vchan_info *pci_prepare_vchan(libxl__gc *gc)
+{
+struct vchan_info *vchan;
+libxl_domid domid;
+char *xs_path;
+
+domid = vchan_find_server(gc, PCID_XS_DIR, PCID_XS_PATH);
+if (domid == DOMID_INVALID) {
+LOGE(ERROR, "Can't find vchan server");
+return NULL;
+}
+vchan = libxl__zalloc(gc, sizeof(*vchan));
+xs_path = GCSPRINTF(PCID_XS_DIR"%d"PCID_XS_PATH, domid);
+vchan->state = vchan_get_instance(gc, domid, xs_path, VCHAN_CLIENT);
+if (!(vchan->state))
+return NULL;
+
+vchan->handle_msg = pci_handle_msg;
+vchan->prepare_cmd = pci_prepare_cmd;
+vchan->receive_buf_size = PCI_RECEIVE_BUFFER_SIZE;
+vchan->max_buf_size = PCI_MAX_SIZE_RX_BUF;
+
+return vchan;
+}
+
static void libxl_create_pc
[PATCH 3/9] tools/xl: Add pcid daemon to xl
From: Anastasiia Lukianenko Add draft version of pcid server (based on vchan-node2), which can receive messages from the client. Signed-off-by: Anastasiia Lukianenko --- tools/configure | 5 +- tools/hotplug/FreeBSD/rc.d/xlpcid.in | 75 ++ tools/hotplug/Linux/init.d/xlpcid.in | 76 ++ tools/hotplug/NetBSD/rc.d/xlpcid.in | 75 ++ tools/xl/Makefile| 4 +- tools/xl/xl.h| 1 + tools/xl/xl_cmdtable.c | 7 +++ tools/xl/xl_pcid.c | 79 8 files changed, 319 insertions(+), 3 deletions(-) create mode 100644 tools/hotplug/FreeBSD/rc.d/xlpcid.in create mode 100644 tools/hotplug/Linux/init.d/xlpcid.in create mode 100644 tools/hotplug/NetBSD/rc.d/xlpcid.in create mode 100644 tools/xl/xl_pcid.c diff --git a/tools/configure b/tools/configure index b21ade08c0..be9fc9b1d4 100755 --- a/tools/configure +++ b/tools/configure @@ -2454,7 +2454,7 @@ ac_compiler_gnu=$ac_cv_c_compiler_gnu -ac_config_files="$ac_config_files ../config/Tools.mk hotplug/FreeBSD/rc.d/xencommons hotplug/FreeBSD/rc.d/xendriverdomain hotplug/Linux/init.d/sysconfig.xencommons hotplug/Linux/init.d/sysconfig.xendomains hotplug/Linux/init.d/xen-watchdog hotplug/Linux/init.d/xencommons hotplug/Linux/init.d/xendomains hotplug/Linux/init.d/xendriverdomain hotplug/Linux/launch-xenstore hotplug/Linux/vif-setup hotplug/Linux/xen-hotplug-common.sh hotplug/Linux/xendomains hotplug/NetBSD/rc.d/xencommons hotplug/NetBSD/rc.d/xendriverdomain ocaml/xenstored/oxenstored.conf" +ac_config_files="$ac_config_files ../config/Tools.mk hotplug/FreeBSD/rc.d/xencommons hotplug/FreeBSD/rc.d/xendriverdomain hotplug/FreeBSD/rc.d/xlpcid hotplug/Linux/init.d/sysconfig.xencommons hotplug/Linux/init.d/sysconfig.xendomains hotplug/Linux/init.d/xlpcid hotplug/Linux/init.d/xen-watchdog hotplug/Linux/init.d/xencommons hotplug/Linux/init.d/xendomains hotplug/Linux/init.d/xendriverdomain hotplug/Linux/launch-xenstore hotplug/Linux/vif-setup hotplug/Linux/xen-hotplug-common.sh hotplug/Linux/xendomains hotplug/NetBSD/rc.d/xencommons hotplug/NetBSD/rc.d/xendriverdomain hotplug/NetBSD/rc.d/xlpcid ocaml/xenstored/oxenstored.conf" ac_config_headers="$ac_config_headers config.h" @@ -10892,8 +10892,10 @@ do "../config/Tools.mk") CONFIG_FILES="$CONFIG_FILES ../config/Tools.mk" ;; "hotplug/FreeBSD/rc.d/xencommons") CONFIG_FILES="$CONFIG_FILES hotplug/FreeBSD/rc.d/xencommons" ;; "hotplug/FreeBSD/rc.d/xendriverdomain") CONFIG_FILES="$CONFIG_FILES hotplug/FreeBSD/rc.d/xendriverdomain" ;; +"hotplug/FreeBSD/rc.d/xlpcid") CONFIG_FILES="$CONFIG_FILES hotplug/FreeBSD/rc.d/xlpcid" ;; "hotplug/Linux/init.d/sysconfig.xencommons") CONFIG_FILES="$CONFIG_FILES hotplug/Linux/init.d/sysconfig.xencommons" ;; "hotplug/Linux/init.d/sysconfig.xendomains") CONFIG_FILES="$CONFIG_FILES hotplug/Linux/init.d/sysconfig.xendomains" ;; +"hotplug/Linux/init.d/xlpcid") CONFIG_FILES="$CONFIG_FILES hotplug/Linux/init.d/xlpcid" ;; "hotplug/Linux/init.d/xen-watchdog") CONFIG_FILES="$CONFIG_FILES hotplug/Linux/init.d/xen-watchdog" ;; "hotplug/Linux/init.d/xencommons") CONFIG_FILES="$CONFIG_FILES hotplug/Linux/init.d/xencommons" ;; "hotplug/Linux/init.d/xendomains") CONFIG_FILES="$CONFIG_FILES hotplug/Linux/init.d/xendomains" ;; @@ -10904,6 +10906,7 @@ do "hotplug/Linux/xendomains") CONFIG_FILES="$CONFIG_FILES hotplug/Linux/xendomains" ;; "hotplug/NetBSD/rc.d/xencommons") CONFIG_FILES="$CONFIG_FILES hotplug/NetBSD/rc.d/xencommons" ;; "hotplug/NetBSD/rc.d/xendriverdomain") CONFIG_FILES="$CONFIG_FILES hotplug/NetBSD/rc.d/xendriverdomain" ;; +"hotplug/NetBSD/rc.d/xlpcid") CONFIG_FILES="$CONFIG_FILES hotplug/NetBSD/rc.d/xlpcid" ;; "ocaml/xenstored/oxenstored.conf") CONFIG_FILES="$CONFIG_FILES ocaml/xenstored/oxenstored.conf" ;; "config.h") CONFIG_HEADERS="$CONFIG_HEADERS config.h" ;; "hotplug/Linux/systemd/proc-xen.mount") CONFIG_FILES="$CONFIG_FILES hotplug/Linux/systemd/proc-xen.mount" ;; diff --git a/tools/hotplug/FreeBSD/rc.d/xlpcid.in b/tools/hotplug/FreeBSD/rc.d/xlpcid.in new file mode 100644 index 00..2817bfaeed --- /dev/null +++ b/tools/hotplug/FreeBSD/rc.d/xlpcid.in @@ -0,0 +1,75 @@ +#! /bin/bash +# +# xlpcid +# +# description: Run xlpcid daemon +### BEGIN INIT INFO +# Provides: xlpcid +# Short-Description: Start/stop xlpcid +# Description: Run xlpcid daemon +### END INIT INFO +# + +. @XEN_SCRIPT_DIR@/hotpl
[PATCH 5/9] tools/libs/light: Add "write" command to xl pcid and libxl PCI
From: Anastasiia Lukianenko
Add functions for handling "write" command on xl pcid side. Libxl PCI side
receives reply:
1. success - string in "return" message
2. fail - error type of reply message
Signed-off-by: Anastasiia Lukianenko
---
tools/include/pcid.h | 7
tools/libs/light/libxl_pci.c | 69 +++
tools/libs/light/libxl_pcid.c | 69 +++
3 files changed, 113 insertions(+), 32 deletions(-)
diff --git a/tools/include/pcid.h b/tools/include/pcid.h
index e9c3c497c6..935d99b186 100644
--- a/tools/include/pcid.h
+++ b/tools/include/pcid.h
@@ -31,7 +31,14 @@
#define PCID_CMD_LIST"ls"
#define PCID_CMD_DIR_ID "dir_id"
+#define PCID_CMD_WRITE "write"
+#define PCID_CMD_PCI_PATH"pci_path"
+#define PCID_CMD_PCI_INFO"pci_info"
+
#define PCID_PCIBACK_DRIVER "pciback_driver"
+#define PCID_PCI_DEV "pci_dev"
+
+#define SYSFS_DRIVER_PATH"driver_path"
#if defined(__linux__)
#define SYSFS_PCIBACK_DRIVER "/sys/bus/pci/drivers/pciback"
diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
index 6534b70777..03ce42dec3 100644
--- a/tools/libs/light/libxl_pci.c
+++ b/tools/libs/light/libxl_pci.c
@@ -455,28 +455,34 @@ 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,
+static int sysfs_write_bdf(libxl__gc *gc, const char *sysfs_path,
+ const char *pci_path,
libxl_device_pci *pci)
{
-int rc, fd;
char *buf;
-fd = open(sysfs_path, O_WRONLY);
-if (fd < 0) {
-LOGE(ERROR, "Couldn't open %s", sysfs_path);
-return ERROR_FAIL;
-}
+struct vchan_info *vchan;
+libxl__json_object *args = NULL, *result = NULL;
-buf = GCSPRINTF(PCI_BDF, pci->domain, pci->bus,
-pci->dev, pci->func);
-rc = write(fd, buf, strlen(buf));
-/* Annoying to have two if's, but we need the errno */
-if (rc < 0)
-LOGE(ERROR, "write to %s returned %d", sysfs_path, rc);
-close(fd);
+vchan = pci_prepare_vchan(gc);
+if (!vchan)
+return ERROR_FAIL;
-if (rc < 0)
+buf = GCSPRINTF(PCI_BDF, pci->domain, pci->bus, pci->dev, pci->func);
+if (strcmp(SYSFS_PCI_DEV, sysfs_path) == 0)
+libxl__vchan_param_add_string(gc, &args, PCID_CMD_DIR_ID,
PCID_PCI_DEV);
+else if (strcmp(SYSFS_PCIBACK_DRIVER, sysfs_path) == 0)
+libxl__vchan_param_add_string(gc, &args, PCID_CMD_DIR_ID,
PCID_PCIBACK_DRIVER);
+else if (strcmp(SYSFS_DRIVER_PATH, sysfs_path) == 0)
+libxl__vchan_param_add_string(gc, &args, PCID_CMD_DIR_ID,
SYSFS_DRIVER_PATH);
+
+libxl__vchan_param_add_string(gc, &args, PCID_CMD_PCI_PATH, pci_path);
+libxl__vchan_param_add_string(gc, &args, PCID_CMD_PCI_INFO, buf);
+result = vchan_send_command(gc, vchan, PCID_CMD_WRITE, args);
+if (!result) {
+LOGE(WARN, "Write %s to %s failed\n", buf, sysfs_path);
return ERROR_FAIL;
+}
return 0;
}
@@ -593,14 +599,13 @@ void
libxl_device_pci_assignable_list_free(libxl_device_pci *list, int num)
static int sysfs_dev_unbind(libxl__gc *gc, libxl_device_pci *pci,
char **driver_path)
{
-char * spath, *dp = NULL;
+char *spath, *pci_path, *dp = NULL;
struct stat st;
-spath = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/driver",
- pci->domain,
- pci->bus,
- pci->dev,
- pci->func);
+pci_path = GCSPRINTF("/"PCI_BDF"/driver", pci->domain, pci->bus,
+ pci->dev, pci->func);
+
+spath = GCSPRINTF(SYSFS_PCI_DEV"%s", pci_path);
if ( !lstat(spath, &st) ) {
/* Find the canonical path to the driver. */
dp = libxl__zalloc(gc, PATH_MAX);
@@ -614,7 +619,7 @@ static int sysfs_dev_unbind(libxl__gc *gc, libxl_device_pci
*pci,
/* Unbind from the old driver */
spath = GCSPRINTF("%s/unbind", dp);
-if ( sysfs_write_bdf(gc, spath, pci) < 0 ) {
+if (sysfs_write_bdf(gc, SYSFS_PCI_DEV, pci_path, pci) < 0) {
LOGE(ERROR, "Couldn't unbind device");
return -1;
}
@@ -816,14 +821,14 @@ static int pciback_dev_assign(libxl__gc *gc,
libxl_device_pci *pci)
LOGE(ERROR, "Error checking for pciback slot");
return ERROR_FAIL;
} else if (rc == 0) {
-if ( sysfs_write_bdf(gc, SYSFS
[PATCH 6/9] tools/libs/light: Add "read" command to xl pcid and libxl PCI
From: Anastasiia Lukianenko
Libxl PCI can use xl pcid server to read integer value from the given path.
Signed-off-by: Anastasiia Lukianenko
---
tools/include/pcid.h | 1 +
tools/libs/light/libxl_pci.c | 125 ++
tools/libs/light/libxl_pcid.c | 50 ++
3 files changed, 119 insertions(+), 57 deletions(-)
diff --git a/tools/include/pcid.h b/tools/include/pcid.h
index 935d99b186..f39011ecb8 100644
--- a/tools/include/pcid.h
+++ b/tools/include/pcid.h
@@ -32,6 +32,7 @@
#define PCID_CMD_DIR_ID "dir_id"
#define PCID_CMD_WRITE "write"
+#define PCID_CMD_READ_HEX"read_hex"
#define PCID_CMD_PCI_PATH"pci_path"
#define PCID_CMD_PCI_INFO"pci_info"
diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
index 03ce42dec3..d5ddca4964 100644
--- a/tools/libs/light/libxl_pci.c
+++ b/tools/libs/light/libxl_pci.c
@@ -634,83 +634,94 @@ static int sysfs_dev_unbind(libxl__gc *gc,
libxl_device_pci *pci,
static uint16_t sysfs_dev_get_vendor(libxl__gc *gc, libxl_device_pci *pci)
{
char *pci_device_vendor_path =
-GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/vendor",
- pci->domain, pci->bus, pci->dev, pci->func);
-uint16_t read_items;
+GCSPRINTF("/"PCI_BDF"/vendor", pci->domain, pci->bus,
+ pci->dev, pci->func);
uint16_t pci_device_vendor;
+struct vchan_info *vchan;
+libxl__json_object *args = NULL, *result = NULL;
-FILE *f = fopen(pci_device_vendor_path, "r");
-if (!f) {
-LOGE(ERROR,
- "pci device "PCI_BDF" does not have vendor attribute",
- pci->domain, pci->bus, pci->dev, pci->func);
-return 0x;
-}
-read_items = fscanf(f, "0x%hx\n", &pci_device_vendor);
-fclose(f);
-if (read_items != 1) {
-LOGE(ERROR,
- "cannot read vendor of pci device "PCI_BDF,
- pci->domain, pci->bus, pci->dev, pci->func);
-return 0x;
-}
+vchan = pci_prepare_vchan(gc);
+if (!vchan)
+goto fail;
+libxl__vchan_param_add_string(gc, &args, PCID_CMD_PCI_INFO,
+ pci_device_vendor_path);
+libxl__vchan_param_add_string(gc, &args, PCID_CMD_DIR_ID,
+ PCID_PCI_DEV);
+result = vchan_send_command(gc, vchan, PCID_CMD_READ_HEX, args);
+if (!result)
+goto fail;
+
+pci_device_vendor = libxl__json_object_get_integer(result);
return pci_device_vendor;
+
+fail:
+LOGE(ERROR,
+ "cannot read vendor of pci device "PCI_BDF,
+ pci->domain, pci->bus, pci->dev, pci->func);
+return 0x;
}
static uint16_t sysfs_dev_get_device(libxl__gc *gc, libxl_device_pci *pci)
{
char *pci_device_device_path =
-GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/device",
- pci->domain, pci->bus, pci->dev, pci->func);
-uint16_t read_items;
+GCSPRINTF("/"PCI_BDF"/device", pci->domain, pci->bus,
+ pci->dev, pci->func);
uint16_t pci_device_device;
+struct vchan_info *vchan;
+libxl__json_object *args = NULL, *result = NULL;
-FILE *f = fopen(pci_device_device_path, "r");
-if (!f) {
-LOGE(ERROR,
- "pci device "PCI_BDF" does not have device attribute",
- pci->domain, pci->bus, pci->dev, pci->func);
-return 0x;
-}
-read_items = fscanf(f, "0x%hx\n", &pci_device_device);
-fclose(f);
-if (read_items != 1) {
-LOGE(ERROR,
- "cannot read device of pci device "PCI_BDF,
- pci->domain, pci->bus, pci->dev, pci->func);
-return 0x;
-}
+vchan = pci_prepare_vchan(gc);
+if (!vchan)
+goto fail;
+libxl__vchan_param_add_string(gc, &args, PCID_CMD_PCI_INFO,
+ pci_device_device_path);
+libxl__vchan_param_add_string(gc, &args, PCID_CMD_DIR_ID,
+ PCID_PCI_DEV);
+result = vchan_send_command(gc, vchan, PCID_CMD_READ_HEX, args);
+if (!result)
+goto fail;
+
+pci_device_device = libxl__json_object_get_integer(result);
return pci_device_device;
+
+fail:
+LOGE(ERROR,
+ "cannot read device of pci device "PCI_BDF,
+ pci->domain, pci->bus, pci->dev, pci->func);
+return 0x;
}
static int sysfs_dev_get_class(libxl__gc *gc, libxl_device_pci *pci,
unsigned long *class)
{
-char *pci_device_class_path = GCSPRINTF(SYSFS_PCI_DE
[PATCH 8/9] tools/libs/light: Add "unbind" and "read resources" commands to libxl PCI and xl pcid
From: Anastasiia Lukianenko
Send requests to xl pcid from libxl PCI to prevent the libxl PCI from
reading / writing from / to the local sysfs directly.
"Unbind" command returns driver's path or "nolstat" result in success and
NULL if writing to PCI path failed.
"Read resources" command returns an array of PCI resources - start, end and
flags in success, NULL if getting resources from PCI path failed.
Signed-off-by: Anastasiia Lukianenko
---
tools/include/pcid.h | 7 +++
tools/libs/light/libxl_pci.c | 99 ++--
tools/libs/light/libxl_pcid.c | 105 ++
3 files changed, 169 insertions(+), 42 deletions(-)
diff --git a/tools/include/pcid.h b/tools/include/pcid.h
index 5c8efbb435..3153bafb19 100644
--- a/tools/include/pcid.h
+++ b/tools/include/pcid.h
@@ -29,11 +29,13 @@
#define PCID_MSG_FIELD_ARGS "arguments"
#define PCID_CMD_LIST"ls"
+#define PCID_CMD_UNBIND "unbind"
#define PCID_CMD_DIR_ID "dir_id"
#define PCID_CMD_WRITE "write"
#define PCID_CMD_READ_HEX"read_hex"
#define PCID_CMD_EXISTS "exists"
+#define PCID_CMD_READ_RESOURCES "read_resources"
#define PCID_CMD_PCI_PATH"pci_path"
#define PCID_CMD_PCI_INFO"pci_info"
@@ -44,12 +46,17 @@
#if defined(__linux__)
#define SYSFS_PCIBACK_DRIVER "/sys/bus/pci/drivers/pciback"
+#define SYSFS_PCI_DEV "/sys/bus/pci/devices"
#endif
#define PCI_INFO_PATH "/libxl/pci"
#define PCI_BDF_XSPATH "%04x-%02x-%02x-%01x"
#define PCI_BDF"%04x:%02x:%02x.%01x"
+#define RESOURCE_START "start"
+#define RESOURCE_END "end"
+#define RESOURCE_FLAGS "flags"
+
int libxl_pcid_process(libxl_ctx *ctx);
#endif /* PCID_H */
diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
index ab6709890e..3d9bf4830b 100644
--- a/tools/libs/light/libxl_pci.c
+++ b/tools/libs/light/libxl_pci.c
@@ -599,34 +599,32 @@ void
libxl_device_pci_assignable_list_free(libxl_device_pci *list, int num)
static int sysfs_dev_unbind(libxl__gc *gc, libxl_device_pci *pci,
char **driver_path)
{
-char *spath, *pci_path, *dp = NULL;
-struct stat st;
+char *pci_path, *dp = NULL;
pci_path = GCSPRINTF("/"PCI_BDF"/driver", pci->domain, pci->bus,
pci->dev, pci->func);
+char *pci_info;
+struct vchan_info *vchan;
+libxl__json_object *args = NULL, *result = NULL;
-spath = GCSPRINTF(SYSFS_PCI_DEV"%s", pci_path);
-if ( !lstat(spath, &st) ) {
-/* Find the canonical path to the driver. */
-dp = libxl__zalloc(gc, PATH_MAX);
-dp = realpath(spath, dp);
-if ( !dp ) {
-LOGE(ERROR, "realpath() failed");
-return -1;
-}
-
-LOG(DEBUG, "Driver re-plug path: %s", dp);
+vchan = pci_prepare_vchan(gc);
+if (!vchan)
+return ERROR_FAIL;
-/* Unbind from the old driver */
-spath = GCSPRINTF("%s/unbind", dp);
-if (sysfs_write_bdf(gc, SYSFS_PCI_DEV, pci_path, pci) < 0) {
-LOGE(ERROR, "Couldn't unbind device");
-return -1;
-}
+pci_info = GCSPRINTF(PCI_BDF, pci->domain, pci->bus, pci->dev, pci->func);
+libxl__vchan_param_add_string(gc, &args, PCID_CMD_PCI_PATH, pci_path);
+libxl__vchan_param_add_string(gc, &args, PCID_CMD_PCI_INFO, pci_info);
+result = vchan_send_command(gc, vchan, PCID_CMD_UNBIND, args);
+if (!result) {
+LOGE(WARN, "Write to %s%s failed\n", SYSFS_PCI_DEV, pci_path);
+return -1;
}
-if ( driver_path )
+if (driver_path) {
+if (strcmp(result->u.string, "nolstat") != 0)
+dp = (char *)libxl__json_object_get_string(result);
*driver_path = dp;
+}
return 0;
}
@@ -1488,8 +1486,6 @@ static void pci_add_dm_done(libxl__egc *egc,
STATE_AO_GC(pas->aodev->ao);
libxl_ctx *ctx = libxl__gc_owner(gc);
libxl_domid domid = pas->pci_domid;
-char *sysfs_path;
-FILE *f;
unsigned long long start, end, flags, size;
int irq, i;
int r;
@@ -1510,20 +1506,38 @@ static void pci_add_dm_done(libxl__egc *egc,
if (isstubdom)
starting = false;
-sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/resource", pci->domain,
- pci->bus, pci->dev, pci->func);
-f = fopen(sysfs_path, "r");
-start = end = flags = size = 0;
-irq = 0;
+struct vchan_info *vchan;
+libxl__json_object *result = NULL, *args = NULL;
+const libx
[PATCH 7/9] tools/libs/light: Make Libxl PCI get values from xl pcid instead of libxl side
From: Anastasiia Lukianenko
pci_multifunction_check needs to use "ls" and "lstat" commands to get
information from sysfs.
Add "is_exists" command processing to xl pcid daemon to make possible
pci_multifunction_check read directories and check lstat by using xl pcid.
Signed-off-by: Anastasiia Lukianenko
---
tools/include/pcid.h | 1 +
tools/libs/light/libxl_pci.c | 47 ++-
tools/libs/light/libxl_pcid.c | 47 +++
3 files changed, 72 insertions(+), 23 deletions(-)
diff --git a/tools/include/pcid.h b/tools/include/pcid.h
index f39011ecb8..5c8efbb435 100644
--- a/tools/include/pcid.h
+++ b/tools/include/pcid.h
@@ -33,6 +33,7 @@
#define PCID_CMD_WRITE "write"
#define PCID_CMD_READ_HEX"read_hex"
+#define PCID_CMD_EXISTS "exists"
#define PCID_CMD_PCI_PATH"pci_path"
#define PCID_CMD_PCI_INFO"pci_info"
diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
index d5ddca4964..ab6709890e 100644
--- a/tools/libs/light/libxl_pci.c
+++ b/tools/libs/light/libxl_pci.c
@@ -1091,45 +1091,46 @@ int libxl_device_pci_assignable_remove(libxl_ctx *ctx,
libxl_device_pci *pci,
*/
static int pci_multifunction_check(libxl__gc *gc, libxl_device_pci *pci,
unsigned int *func_mask)
{
-struct dirent *de;
-DIR *dir;
-
*func_mask = 0;
+struct vchan_info *vchan;
+libxl__json_object *result = NULL, *args = NULL;
+const libxl__json_object *lstat_obj, *dir;
+const char *dir_name;
+int i;
-dir = opendir(SYSFS_PCI_DEV);
-if ( NULL == dir ) {
-LOGE(ERROR, "Couldn't open %s", SYSFS_PCI_DEV);
+vchan = pci_prepare_vchan(gc);
+if (!vchan)
return -1;
-}
-while( (de = readdir(dir)) ) {
+libxl__vchan_param_add_string(gc, &args, PCID_CMD_DIR_ID, PCID_PCI_DEV);
+result = vchan_send_command(gc, vchan, PCID_CMD_LIST, args);
+if (!result)
+return -1;
+
+for (i = 0; (dir = libxl__json_array_get(result, i)); i++) {
+dir_name = libxl__json_object_get_string(dir);
unsigned dom, bus, dev, func;
-struct stat st;
char *path;
-if ( sscanf(de->d_name, PCI_BDF, &dom, &bus, &dev, &func) != 4 )
+if (sscanf(dir_name, PCI_BDF, &dom, &bus, &dev, &func) != 4)
continue;
-if ( pci->domain != dom )
+if (pci->domain != dom)
continue;
-if ( pci->bus != bus )
+if (pci->bus != bus)
continue;
-if ( pci->dev != dev )
+if (pci->dev != dev)
continue;
-path = GCSPRINTF("%s/" PCI_BDF, SYSFS_PCIBACK_DRIVER, dom, bus, dev,
func);
-if ( lstat(path, &st) ) {
-if ( errno == ENOENT )
-LOG(ERROR, PCI_BDF " is not assigned to pciback driver",
-dom, bus, dev, func);
-else
-LOGE(ERROR, "Couldn't lstat %s", path);
-closedir(dir);
+path = GCSPRINTF("/" PCI_BDF, dom, bus, dev, func);
+libxl__vchan_param_add_string(gc, &args, PCID_CMD_DIR_ID,
PCID_PCIBACK_DRIVER);
+libxl__vchan_param_add_string(gc, &args, PCID_CMD_PCI_INFO, path);
+lstat_obj = vchan_send_command(gc, vchan, PCID_CMD_EXISTS, args);
+if (!lstat_obj)
return -1;
-}
+
(*func_mask) |= (1 << func);
}
-closedir(dir);
return 0;
}
diff --git a/tools/libs/light/libxl_pcid.c b/tools/libs/light/libxl_pcid.c
index 0f736c68af..28d773f48d 100644
--- a/tools/libs/light/libxl_pcid.c
+++ b/tools/libs/light/libxl_pcid.c
@@ -34,6 +34,10 @@
#include
#include
+#include
+#include
+#include
+
#define DOM0_ID 0
static struct libxl__json_object *process_ls_cmd(libxl__gc *gc,
@@ -202,6 +206,47 @@ out:
return result;
}
+static libxl__json_object *process_exists_cmd(libxl__gc *gc,
+ const struct libxl__json_object
*resp)
+{
+libxl__json_object *result = NULL;
+const struct libxl__json_object *args, *pci_path, *pci_info;
+char *full_path;
+struct stat st;
+
+args = libxl__json_map_get(PCID_MSG_FIELD_ARGS, resp, JSON_MAP);
+if (!args)
+goto out;
+pci_path = libxl__json_map_get(PCID_CMD_DIR_ID, args, JSON_ANY);
+if (!pci_path)
+goto out;
+pci_info = libxl__json_map_get(PCID_CMD_PCI_INFO, args, JSON_ANY);
+if (!pci_info)
+goto out;
+if (strcmp(pci_path->u.string, PCID_PCIBACK_DRIVER) == 0)
+full_path = libxl__sprintf(gc, SYSFS_PCIBACK_DRIVER"%s",
pci_info->u.string);
+else
+full_path = pci_info->u.string;
+
+if (lstat(full_path, &st)) {
+if (errno == ENOENT)
+
[PATCH 9/9] tools/libs/light: Add "reset" and "remove" PCI dev commands to xl pcid daemon and libxl PCI
From: Anastasiia Lukianenko
Add "reset" command processing to xl pcid daemon.
Make possible sending "reset"/"remove" PCI device requests from libxl PCI
to xl pcid daemon instead of using sysfs on libxl side.
Signed-off-by: Anastasiia Lukianenko
---
tools/include/pcid.h | 1 +
tools/libs/light/libxl_pci.c | 94 +++
tools/libs/light/libxl_pcid.c | 63 +++
3 files changed, 115 insertions(+), 43 deletions(-)
diff --git a/tools/include/pcid.h b/tools/include/pcid.h
index 3153bafb19..38ea06c602 100644
--- a/tools/include/pcid.h
+++ b/tools/include/pcid.h
@@ -30,6 +30,7 @@
#define PCID_CMD_LIST"ls"
#define PCID_CMD_UNBIND "unbind"
+#define PCID_CMD_RESET "reset"
#define PCID_CMD_DIR_ID "dir_id"
#define PCID_CMD_WRITE "write"
diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
index 3d9bf4830b..0498baa47e 100644
--- a/tools/libs/light/libxl_pci.c
+++ b/tools/libs/light/libxl_pci.c
@@ -1635,37 +1635,26 @@ static int libxl__device_pci_reset(libxl__gc *gc,
unsigned int domain, unsigned
unsigned int dev, unsigned int func)
{
char *reset;
-int fd, rc;
-
-reset = GCSPRINTF("%s/do_flr", SYSFS_PCIBACK_DRIVER);
-fd = open(reset, O_WRONLY);
-if (fd >= 0) {
-char *buf = GCSPRINTF(PCI_BDF, domain, bus, dev, func);
-rc = write(fd, buf, strlen(buf));
-if (rc < 0)
-LOGD(ERROR, domain, "write to %s returned %d", reset, rc);
-close(fd);
-return rc < 0 ? rc : 0;
-}
-if (errno != ENOENT)
-LOGED(ERROR, domain, "Failed to access pciback path %s", reset);
-reset = GCSPRINTF("%s/"PCI_BDF"/reset", SYSFS_PCI_DEV, domain, bus, dev,
func);
-fd = open(reset, O_WRONLY);
-if (fd >= 0) {
-rc = write(fd, "1", 1);
-if (rc < 0)
-LOGED(ERROR, domain, "write to %s returned %d", reset, rc);
-close(fd);
-return rc < 0 ? rc : 0;
-}
-if (errno == ENOENT) {
-LOGD(ERROR, domain,
- "The kernel doesn't support reset from sysfs for PCI device
"PCI_BDF,
- domain, bus, dev, func);
-} else {
-LOGED(ERROR, domain, "Failed to access reset path %s", reset);
+char *buf;
+struct vchan_info *vchan;
+libxl__json_object *args = NULL, *result = NULL;
+
+vchan = pci_prepare_vchan(gc);
+if (!vchan)
+return -1;
+
+reset = GCSPRINTF("%s", "/do_flr");
+buf = GCSPRINTF(PCI_BDF, domain, bus, dev, func);
+
+libxl__vchan_param_add_string(gc, &args, PCID_CMD_PCI_PATH, reset);
+libxl__vchan_param_add_string(gc, &args, PCID_CMD_PCI_INFO, buf);
+result = vchan_send_command(gc, vchan, PCID_CMD_RESET, args);
+if (!result) {
+LOGD(ERROR, domain, "write to %s returned error", reset);
+return -1;
}
-return -1;
+
+return 0;
}
int libxl__device_pci_setdefault(libxl__gc *gc, uint32_t domid,
@@ -2085,20 +2074,35 @@ static void do_pci_remove(libxl__egc *egc,
pci_remove_state *prs)
goto out_fail;
}
} else {
-char *sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/resource",
pci->domain,
+char *sysfs_path = GCSPRINTF("/"PCI_BDF"/resource", pci->domain,
pci->bus, pci->dev, pci->func);
-FILE *f = fopen(sysfs_path, "r");
unsigned int start = 0, end = 0, flags = 0, size = 0;
int irq = 0;
int i;
+struct vchan_info *vchan;
+libxl__json_object *args = NULL, *result = NULL;
+const libxl__json_object *addr;
+int j = 0;
-if (f == NULL) {
-LOGED(ERROR, domainid, "Couldn't open %s", sysfs_path);
+vchan = pci_prepare_vchan(gc);
+if (!vchan)
+goto out_fail;
+libxl__vchan_param_add_string(gc, &args, PCID_CMD_PCI_INFO,
sysfs_path);
+result = vchan_send_command(gc, vchan, PCID_CMD_READ_RESOURCES, args);
+if (!result) {
+LOGED(ERROR, domainid, "Couldn't get resources from %s",
sysfs_path);
+rc = ERROR_FAIL;
goto skip1;
}
+
for (i = 0; i < PROC_PCI_NUM_RESOURCES; i++) {
-if (fscanf(f, "0x%x 0x%x 0x%x\n", &start, &end, &flags) != 3)
-continue;
+addr = libxl__json_array_get(result, j++);
+start = libxl__json_object_get_integer(addr);
+addr = libxl__json_array_get(result, j++);
+end = libxl__json_object_get_integer(addr);
+addr = libxl__json_
Hand over of the Xen shared info page
Hi all, The problem described below concerns cases when a shared info page needs to be handed over from one entity in the system to another, for example, there is a bootloader or any other code that may run before the guest OS' kernel. Normally, to map the shared info page guests allocate a memory page from their RAM and map the shared info on top of it. Specifically we use XENMAPSPACE_shared_info memory space in XENMEM_add_to_physmap hypercall. As the info page exists throughout the guest existence this doesn't hurt the guest, but when the page gets out of accounting, e.g. after bootloader jumps to Linux and the page is not handed over to it, the mapped page becomes a problem. Consider the case with U-boot bootloader which already has Xen support. The U-boot’s Xen guest implementation allocates a shared info page between Xen and the guest domain and U-boot uses domain's RAM address space to create and map the shared info page by using XENMEM_add_to_physmap hypercall [1]. After U-boot transfers control to the operating system (Linux, Android, etc), the shared info page is still mapped in domain’s address space, e.g. its RAM. So, after we leave U-boot, this page becomes just an ordinary memory page from Linux POV while it is still a shared info page from Xen POV. This can lead to undefined behavior, errors etc as Xen can write something to the shared info page and when Linux tries to use it - data corruption may happen. This happens because there is no unmap function in Xen API to remove an existing shared info page mapping. We could use only hypercall XENMEM_remove_from_physmap which eventually will create a hole in the domain's RAM address space which may also lead to guest’s crash while accessing that memory. We noticed this problem and the workaround was implemented using the special GUEST_MAGIC memory region [2]. Now we want to make a proper solution based on GUEST_MAGIC_BASE, which does not belong to the guest’s RAM address space [3]. Using the example of how offsets for the console and xenstore are implemented, we can add a new shared_info offset and increase the number of magic pages [4] and implement related functionality, so there is a similar API to query that magic page location as it is done for console PFN and others. This approach would allow the use of the XENMEM_remove_from_physmap hypercall without creating gaps in the RAM address space for Xen guest OS [5]. [1] - https://github.com/u-boot/u-boot/blob/master/drivers/xen/hypervisor.c#L141 [2] - https://github.com/xen-troops/u-boot/commit/f759b151116af204a5ab02ace82c09300cd6233a [3] - https://github.com/xen-project/xen/blob/master/xen/include/public/arch-arm.h#L432 [4] - https://github.com/xen-project/xen/blob/25849c8b16f2a5b7fcd0a823e80a5f1b590291f9/tools/libs/guest/xg_dom_arm.c#L29 [5] - https://github.com/xen-troops/u-boot/blob/android-master/drivers/xen/hypervisor.c#L162 Regards, Anastasiia Lukianenko
Re: Hand over of the Xen shared info page
Hi Julien! On Thu, 2021-05-13 at 09:37 +0100, Julien Grall wrote: > > On 13/05/2021 09:03, Anastasiia Lukianenko wrote: > > Hi all, > > Hi, > > > The problem described below concerns cases when a shared info page > > needs to be handed over from one entity in the system to another, > > for > > example, there is a bootloader or any other code that may run > > before > > the guest OS' kernel. > > Normally, to map the shared info page guests allocate a memory page > > from their RAM and map the shared info on top of it. Specifically > > we > > use XENMAPSPACE_shared_info memory space in XENMEM_add_to_physmap > > hypercall. As the info page exists throughout the guest existence > > this > > doesn't hurt the guest, but when the page gets out of accounting, > > e.g. > > after bootloader jumps to Linux and the page is not handed over to > > it, > > the mapped page becomes a problem. > > Consider the case with U-boot bootloader which already has Xen > > support. > > The U-boot’s Xen guest implementation allocates a shared info page > > between Xen and the guest domain and U-boot uses domain's RAM > > address > > space to create and map the shared info page by using > > XENMEM_add_to_physmap hypercall [1]. > > > > After U-boot transfers control to the operating system (Linux, > > Android, > > etc), the shared info page is still mapped in domain’s address > > space, > > e.g. its RAM. So, after we leave U-boot, this page becomes just an > > ordinary memory page from Linux POV while it is still a shared info > > page from Xen POV. This can lead to undefined behavior, errors etc > > as > > Xen can write something to the shared info page and when Linux > > tries to > > use it - data corruption may happen. > > This happens because there is no unmap function in Xen API to > > remove an > > existing shared info page mapping. We could use only hypercall > > XENMEM_remove_from_physmap which eventually will create a hole in > > the > > domain's RAM address space which may also lead to guest’s crash > > while > > accessing that memory. > > The hypercall XENMEM_remove_from_physmap is the correct hypercall > here > and work as intented. It is not Xen business to keep track what was > the > original page (it may have been RAM, device...). > > The problem here is the hypercall XENMEM_add_to_physmap is misused > in > U-boot. When you give an address for the mapping you are telling Xen > "Here a free region to map the share paged". IOW, Xen will throw > away > whatever was before because that was you asked. > > If you want to map in place of the RAM page, then the correct > approach > is to: >1) Request Xen to remove the RAM page from the P2M >2) Map the shared page >/* Use it */ >3) Unmap the shared page >4) Allocate the memory > > You can avoid 1) and 4) by finding free region in the address space. > > > > > We noticed this problem and the workaround was implemented using > > the > > special GUEST_MAGIC memory region [2]. > > > > Now we want to make a proper solution based on GUEST_MAGIC_BASE, > > which > > does not belong to the guest’s RAM address space [3]. Using the > > example > > of how offsets for the console and xenstore are implemented, we can > > add > > a new shared_info offset and increase the number of magic pages [4] > > and > > implement related functionality, so there is a similar API to query > > that magic page location as it is done for console PFN and others. > > They are not the same type. The console PFN points memory already > populated in the guest address space. > > For the domain shared page, this is memory belonging to Xen that you > will map in your address space. A domain can map it anywhere it > wants. > > > This approach would allow the use of the XENMEM_remove_from_physmap > > hypercall without creating gaps in the RAM address space for Xen > > guest > > OS [5]. > > See above to prevent the gap. I appreciate this means a superpage > may > get shattered. > > The alternative is for U-boot to go through the DT and infer which > regions are free (IOW any region not described). Thank you for interest in the problem and advice on how to solve it. Could you please clarify how we could find free regions using DT in U- boot? Regards, Anastasiia > > Cheers, >
