[Xen-devel] [ovmf test] 104113: all pass - PUSHED
flight 104113 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/104113/ Perfect :-) All tests in this flight passed as required version targeted for testing: ovmf 7c14bc8769fbe1670e3b3d09d6cd531713eb74a4 baseline version: ovmf 1b3d5b0699cf6222698ed8373026813b61e87637 Last test of basis 104105 2017-01-11 00:14:50 Z0 days Failing since104107 2017-01-11 03:29:54 Z0 days3 attempts Testing same since 104113 2017-01-11 05:45:23 Z0 days1 attempts People who touched revisions under test: Chen A Chen Ruiyu 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 : + branch=ovmf + revision=7c14bc8769fbe1670e3b3d09d6cd531713eb74a4 + . ./cri-lock-repos ++ . ./cri-common +++ . ./cri-getconfig +++ umask 002 +++ getrepos getconfig Repos perl -e ' use Osstest; readglobalconfig(); print $c{"Repos"} or die $!; ' +++ local repos=/home/osstest/repos +++ '[' -z /home/osstest/repos ']' +++ '[' '!' -d /home/osstest/repos ']' +++ echo /home/osstest/repos ++ repos=/home/osstest/repos ++ repos_lock=/home/osstest/repos/lock ++ '[' x '!=' x/home/osstest/repos/lock ']' ++ OSSTEST_REPOS_LOCK_LOCKED=/home/osstest/repos/lock ++ exec with-lock-ex -w /home/osstest/repos/lock ./ap-push ovmf 7c14bc8769fbe1670e3b3d09d6cd531713eb74a4 + branch=ovmf + revision=7c14bc8769fbe1670e3b3d09d6cd531713eb74a4 + . ./cri-lock-repos ++ . ./cri-common +++ . ./cri-getconfig +++ umask 002 +++ getrepos getconfig Repos perl -e ' use Osstest; readglobalconfig(); print $c{"Repos"} or die $!; ' +++ local repos=/home/osstest/repos +++ '[' -z /home/osstest/repos ']' +++ '[' '!' -d /home/osstest/repos ']' +++ echo /home/osstest/repos ++ repos=/home/osstest/repos ++ repos_lock=/home/osstest/repos/lock ++ '[' x/home/osstest/repos/lock '!=' x/home/osstest/repos/lock ']' + . ./cri-common ++ . ./cri-getconfig ++ umask 002 + select_xenbranch + case "$branch" in + tree=ovmf + xenbranch=xen-unstable + '[' xovmf = xlinux ']' + linuxbranch= + '[' x = x ']' + qemuubranch=qemu-upstream-unstable + select_prevxenbranch ++ ./cri-getprevxenbranch xen-unstable + prevxenbranch=xen-4.8-testing + '[' x7c14bc8769fbe1670e3b3d09d6cd531713eb74a4 = x ']' + : tested/2.6.39.x + . ./ap-common ++ : osst...@xenbits.xen.org +++ getconfig OsstestUpstream +++ perl -e ' use Osstest; readglobalconfig(); print $c{"OsstestUpstream"} or die $!; ' ++ : ++ : git://xenbits.xen.org/xen.git ++ : osst...@xenbits.xen.org:/home/xen/git/xen.git ++ : git://xenbits.xen.org/qemu-xen-traditional.git ++ : git://git.kernel.org ++ : git://git.kernel.org/pub/scm/linux/kernel/git ++ : git ++ : git://xenbits.xen.org/xtf.git ++ : osst...@xenbits.xen.org:/home/xen/git/xtf.git ++ : git://xenbits.xen.org/xtf.git ++ : git://xenbits.xen.org/libvirt.git ++ : osst...@xenbits.xen.org:/home/xen/git/libvirt.git ++ : git://xenbits.xen.org/libvirt.git ++ : git://xenbits.xen.org/osstest/rumprun.git ++ : git ++ : git://xenbits.xen.org/osstest/rumprun.git ++ : osst...@xenbits.xen.org:/home/xen/git/osstest/rumprun.git ++ : git://git.seabios.org/seabios.git ++ : osst...@xenbits.xen.org:/home/xen/git/osstest/seabios.git ++ : git://xenbits.xen.org/osstest/seabios.git ++ : https://github.com/tianocore/edk2.git ++ : osst...@xenbits.xen.org:/home/xen/git/osstest/ovmf.git ++ : git://xenbits.xen.org/osstest/ovmf.git ++ : git://xenbits.xen.org/osstest/linux-firmware.git ++ : osst...@xenbits.xen.org:/home/osstest/ext/linux-firmware.git ++ : git://git.kernel.org/pub/scm/linux/kernel/
[Xen-devel] [xen-unstable test] 104104: tolerable FAIL - PUSHED
flight 104104 xen-unstable real [real] http://logs.test-lab.xenproject.org/osstest/logs/104104/ Failures :-/ but no regressions. Regressions which are regarded as allowable (not blocking): test-armhf-armhf-libvirt 13 saverestore-support-checkfail like 104099 test-armhf-armhf-libvirt-xsm 13 saverestore-support-checkfail like 104099 test-amd64-i386-xl-qemuu-win7-amd64 16 guest-stop fail like 104099 test-amd64-i386-xl-qemut-win7-amd64 16 guest-stop fail like 104099 test-amd64-amd64-xl-qemut-win7-amd64 16 guest-stopfail like 104099 test-amd64-amd64-xl-qemuu-win7-amd64 16 guest-stopfail like 104099 test-armhf-armhf-libvirt-qcow2 12 saverestore-support-check fail like 104099 test-armhf-armhf-libvirt-raw 12 saverestore-support-checkfail like 104099 test-amd64-amd64-xl-rtds 9 debian-install fail like 104099 Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-pvh-amd 11 guest-start fail never pass test-amd64-amd64-xl-pvh-intel 11 guest-start fail never pass test-amd64-i386-libvirt 12 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 12 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 12 migrate-support-checkfail never pass test-amd64-amd64-libvirt 12 migrate-support-checkfail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check fail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check fail never pass test-armhf-armhf-xl-arndale 12 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 13 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 11 migrate-support-checkfail never pass test-amd64-amd64-qemuu-nested-amd 16 debian-hvm-install/l1/l2 fail never pass test-armhf-armhf-xl 12 migrate-support-checkfail never pass test-armhf-armhf-xl 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-xsm 12 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 12 migrate-support-checkfail never pass test-armhf-armhf-xl-xsm 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-cubietruck 12 migrate-support-checkfail never pass test-armhf-armhf-libvirt 12 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-cubietruck 13 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-xsm 12 migrate-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 11 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 11 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 12 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-vhd 11 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 12 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 12 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 13 saverestore-support-checkfail never pass version targeted for testing: xen ffc103c223a6d12e5221f66b7e96396a61ba1b20 baseline version: xen 9d81162fd2c69b5c9279670c8739d891cf7571da Last test of basis 104099 2017-01-10 13:14:57 Z0 days Testing same since 104104 2017-01-10 23:45:04 Z0 days1 attempts People who touched revisions under test: Andrew Cooper Suravee Suthikulpanit jobs: build-amd64-xsm pass build-armhf-xsm pass build-i386-xsm pass build-amd64-xtf pass build-amd64 pass build-armhf pass build-i386 pass build-amd64-libvirt pass build-armhf-libvirt pass build-i386-libvirt pass build-amd64-oldkern pass build-i386-oldkern pass build-amd64-prev pass build-i386-prev pass build-amd64-pvopspass build-armhf-pvopspass build-i3
Re: [Xen-devel] [PATCH v4 12/24] x86: refactor psr: set value: implement write msr flow.
On 17-01-10 08:15:15, Jan Beulich wrote: > >>> On 14.12.16 at 05:07, wrote: > > --- a/xen/arch/x86/psr.c > > +++ b/xen/arch/x86/psr.c > > @@ -186,6 +186,9 @@ struct feat_ops { > > unsigned int (*exceeds_cos_max)(const uint64_t val[], > > const struct feat_node *feat, > > unsigned int cos); > > +/* write_msr is used to write out feature MSR register. */ > > +int (*write_msr)(unsigned int cos, const uint64_t val[], > > + struct feat_node *feat); > > Looks like this function again returns number-of-values, yet this time > without a comment saying so. While you don't need to replicate > that description multiple time, please at least has a brief reference. > That said, with the type checks moved out I think this return value > model won't be needed anymore - the caller, having checked the > type, could then simply call the get-num-val (or however it was > named) hook to know how many array entries to skip. > For write msr, we may need iterate the whole feature list to write values for every feature if the input value is not same as old on the COS ID. So, I prefer to keep current return value, the number-of-values handled. That would be clear and easy to implement. Of course, we can call get_cos_num to get the returen value or define a macro to replace the digit. How do you think? > > @@ -889,9 +909,67 @@ static int alloc_new_cos(const struct psr_socket_info > > *info, > > return -ENOENT; > > } > > > > +static unsigned int get_socket_cpu(unsigned int socket) > > +{ > > +if ( likely(socket < nr_sockets) ) > > +return cpumask_any(socket_cpumask[socket]); > > + > > +return nr_cpu_ids; > > +} > > + > > +struct cos_write_info > > +{ > > +unsigned int cos; > > +struct list_head *feat_list; > > +const uint64_t *val; > > +}; > > + > > +static void do_write_psr_msr(void *data) > > +{ > > +struct cos_write_info *info = (struct cos_write_info *)data; > > +unsigned int cos = info->cos; > > +struct list_head *feat_list= info->feat_list; > > +const uint64_t *val= info->val; > > +struct feat_node *feat_tmp; > > +int ret; > > + > > +if ( !feat_list ) > > +return; > > + > > +/* We need set all features values into MSRs. */ > > +list_for_each_entry(feat_tmp, feat_list, list) > > +{ > > +ret = feat_tmp->ops.write_msr(cos, val, feat_tmp); > > +if ( ret <= 0) > > Missing blank. > Sorry. > > +return; > > + > > +val += ret; > > +} > > +} > > + > > static int write_psr_msr(unsigned int socket, unsigned int cos, > > const uint64_t *val) > > { > > +struct psr_socket_info *info = get_socket_info(socket); > > + > > +struct cos_write_info data = > > No blank lines between declarations please (unless there are > extraordinarily many). > Ok, thanks! > Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 11/24] x86: refactor psr: set value: implement cos id allocation flow.
On 17-01-10 08:08:19, Jan Beulich wrote: > >>> On 14.12.16 at 05:07, wrote: > > --- a/xen/arch/x86/psr.c > > +++ b/xen/arch/x86/psr.c > > @@ -169,6 +169,23 @@ struct feat_ops { > > */ > > int (*compare_val)(const uint64_t val[], const struct feat_node *feat, > > unsigned int cos, bool *found); > > +/* > > + * exceeds_cos_max is used to check if the input cos id exceeds the > > + * feature's cos_max and if the input value is not the default one. > > + * Even if the associated cos exceeds the cos_max, HW can work with > > default > > + * value. That is the reason we need check if input value is default > > one. > > + * If both criteria are fulfilled, that means the input exceeds the > > + * range. > > Isn't this last sentence the wrong way round? > Sorry. > > + * The return value of the function means the number of the value array > > + * entries to skip or error. > > + * 1 - one entry in value array. > > + * 2 - two entries in value array, e.g. CDP uses two entries. > > + * 0 - error, exceed cos_max and the input value is not default. > > + */ > > +unsigned int (*exceeds_cos_max)(const uint64_t val[], > > +const struct feat_node *feat, > > +unsigned int cos); > > IIRC I did recommend "exceeds" in the name during earlier review, > but also iirc the semantics of the call were different back then. > Please try to name functions such that they describe themselves > in at least a minimalistic ways. My main issue with this naming is > that the function returning non-zero (i.e. true in C meaning within > conditionals) means "no" here rather than "yes". fits_cos_max() > would therefore be a possibly better fit. > Thanks for the suggestion! > > +static bool exceeds_cos_max(const uint64_t *val, > > +uint32_t array_len, > > +const struct psr_socket_info *info, > > +unsigned int cos) > > +{ > > +unsigned int ret; > > +const uint64_t *val_tmp = val; > > +const struct feat_node *feat_tmp; > > + > > +list_for_each_entry(feat_tmp, &info->feat_list, list) > > +{ > > +ret = feat_tmp->ops.exceeds_cos_max(val_tmp, feat_tmp, cos); > > +if ( !ret ) > > +return false; > > + > > +val_tmp += ret; > > +if ( val_tmp - val > array_len ) > > +return false; > > +} > > + > > +return true; > > +} > > Similarly here - name and return value don't fit together; I think > you want to invert the return values or (along the lines above) > rename the function. > Will rename the callback function to make it accurate. Thanks! > > static int alloc_new_cos(const struct psr_socket_info *info, > > const uint64_t *val, uint32_t array_len, > > unsigned int old_cos, > > enum cbm_type type) > > { > > -return 0; > > +unsigned int cos; > > +unsigned int cos_max = 0; > > +const struct feat_node *feat_tmp; > > +const unsigned int *ref = info->cos_ref; > > + > > +/* > > + * cos_max is the one of the feature which is being set. > > + */ > > +list_for_each_entry(feat_tmp, &info->feat_list, list) > > +{ > > +cos_max = feat_tmp->ops.get_cos_max_from_type(feat_tmp, type); > > +if ( cos_max > 0 ) > > +break; > > +} > > + > > +if ( !cos_max ) > > +return -ENOENT; > > + > > +/* > > + * If old cos is referred only by the domain, then use it. And, we > > cannot > > + * use id 0 because it stores the default values. > > + */ > > +if ( ref[old_cos] == 1 && old_cos ) > > Please swap both sides of && - cheaper checks should come first if > possible. > Sure, thanks! > > +if ( exceeds_cos_max(val, array_len, info, old_cos) ) > > Also please fold the two if()-s. > Ok, thanks! > > +return old_cos; > > And then, as indicated before, this part is not really an allocation, > but a re-use, so would likely better move into the caller (or the > function's name should be adjusted). > Prefer to change function name to 'pick_avail_cos'. > > +/* Find an unused one other than cos0. */ > > +for ( cos = 1; cos <= cos_max; cos++ ) > > +/* > > + * ref is 0 means this COS is not used by other domain and > > + * can be used for current setting. > > + */ > > +if ( !ref[cos] ) > > +{ > > +if ( !exceeds_cos_max(val, array_len, info, cos) ) > > +return -ENOENT; > > + > > +return cos; > > +} > > While a comment is just white space, this looks suspicious without > another pair of braces around the for() body. > Sure. > Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 10/24] x86: refactor psr: set value: implement cos finding flow.
On 17-01-10 07:53:07, Jan Beulich wrote: > >>> On 14.12.16 at 05:07, wrote: > > Continue with previous patch, we can try to find if there is a > > Please take into consideration that a series may be applied in small > steps. References such as "previous patch" are thus possibly > meaningless. Please instead refer to the patch by title. Also I think > you mean "continue from ...". > Thank you! > > @@ -666,7 +724,58 @@ static int find_cos(const uint64_t *val, uint32_t > > array_len, > > enum cbm_type type, > > const struct psr_socket_info *info) > > { > > -return 0; > > +unsigned int cos; > > +const unsigned int *ref = info->cos_ref; > > +const struct feat_node *feat_tmp; > > +const uint64_t *val_tmp = val; > > +int ret; > > +bool found = false; > > +unsigned int cos_max = 0; > > + > > +/* cos_max is the one of the feature which is being set. */ > > +list_for_each_entry(feat_tmp, &info->feat_list, list) > > +{ > > +cos_max = feat_tmp->ops.get_cos_max_from_type(feat_tmp, type); > > +if ( cos_max > 0 ) > > +break; > > +} > > + > > +for ( cos = 0; cos <= cos_max; cos++ ) > > +{ > > +if ( cos && !ref[cos] ) > > +continue; > > + > > +/* Not found, need find again from beginning. */ > > +val_tmp = val; > > +list_for_each_entry(feat_tmp, &info->feat_list, list) > > +{ > > +/* > > + * Compare value according to feature list order. > > + * We must follow this order because value array is assembled > > + * as this order in get_old_set_new(). > > + */ > > +ret = feat_tmp->ops.compare_val(val_tmp, feat_tmp, cos, > > &found); > > +if ( ret < 0 ) > > +return ret; > > + > > +/* If fail to match, go to next cos to compare. */ > > +if ( !found ) > > +break; > > + > > +val_tmp += ret; > > +if ( val_tmp - val > array_len ) > > +return -EINVAL; > > +} > > + > > +/* > > + * With this cos id, every entry of value array can match. This cos > > + * is what we find. > > + */ > > "can match" seems rather misleading to me. I think you mean > something like "For this COS ID all entries in the values array did > match. Use it." > Yes, sorry for the wording. > Other than that various of the comments given for earlier patches > apply here, in particular the fact that the type matching should > move out of the hook functions. > Will try this. Thanks! > Jan > > > ___ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 09/24] x86: refactor psr: set value: assemble features value array.
On 17-01-10 07:34:00, Jan Beulich wrote: > >>> On 14.12.16 at 05:07, wrote: > > --- a/xen/arch/x86/psr.c > > +++ b/xen/arch/x86/psr.c > > @@ -121,6 +121,35 @@ struct feat_ops { > > /* get_val is used to get feature COS register value. */ > > bool (*get_val)(const struct feat_node *feat, unsigned int cos, > > enum cbm_type type, uint64_t *val); > > +/* > > + * get_cos_num is used to get the COS registers amount used by the > > + * feature for one setting, e.g. CDP uses 2 COSs but CAT uses 1. > > + */ > > +unsigned int (*get_cos_num)(const struct feat_node *feat); > > +/* > > + * get_old_val and set_new_val are a pair of functions called together. > > + * The caller will traverse all features in the list and call both > > + * functions for every feature to do below two things: > > + * 1. get old_cos register value of all supported features and > > + * 2. set the new value for the feature. > > + * > > + * All the values are set into value array according the traversal > > order, > > + * meaning the same order of feature list members. > > + * > > + * The return value is the amount of entries to skip in the value array > > + * or error. > > + * 1 - one entry in value array. > > + * 2 - two entries in value array, e.g. CDP uses two entries. > > Doesn't this match the get_cos_num() return value? Would be nice to > be spelled out (and, where possible, ASSERT()ed). > Yes, thanks! > > @@ -186,6 +215,29 @@ static void free_feature(struct psr_socket_info *info) > > } > > } > > > > +static bool_t psr_check_cbm(unsigned int cbm_len, uint64_t cbm) > > bool (and then true/false in the function body) > Ok, thanks! > > +static int l3_cat_set_new_val(uint64_t val[], > > + const struct feat_node *feat, > > + unsigned int old_cos, > > + enum cbm_type type, > > + uint64_t m) > > +{ > > +if ( type != PSR_CBM_TYPE_L3 ) > > +/* L3 CAT uses one COS. Skip it. */ > > +return 1; > > If this is the wrong type, how can you return 1 (or any positive > value) here? This basically suggests that, as recommended for an > earlier operation already, that you want the type check done in > the caller. Or wait - isn't type not matching an error here, as you > iterate the same list twice in the same order? Which then gets us > back to me mentioning that the set-new-value should really be > done in common code, not in the callbacks; it would really only be > the check (psr_check_cbm() in the L3 case above) which would > require a callback. > Your understanding is right. The value array will be iterated twice. First, assemble all features old value into it. Second, set the target value into array according to the target feature's position in the array. Because different features may have different behaviors, e.g. CDP uses two entries of the array, I think it would be better to make 'set_new_val' be a callback function. > Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 08/24] x86: refactor psr: set value: implement framework.
On 17-01-10 07:17:38, Jan Beulich wrote: > > To make the set value flow be general and can support multiple features > > at same time, it includes below steps: > > 1. Get COS ID of current domain using. > > 2. Assemble a value array to store all features current value > >in it and replace the current value of the feature which is > >being set to the new input value. > > 3. Find if there is already a COS ID on which all features' > >values are same as the array. Then, we can reuse this COS > >ID. > > 4. If fail to find, we need allocate a new COS ID. Only COS ID which ref > >is 0 or 1 can be allocated. > > Using "allocate" here in conjunction with ref count being 1 is a little > misleading here - allocation would mean a fresh ID, whereas in the > case of ref == 1 you mean to re-use the current one. > Maybe 'pick an available COS ID' is more appropriate. > > --- a/xen/arch/x86/psr.c > > +++ b/xen/arch/x86/psr.c > > @@ -513,18 +513,197 @@ int psr_get_val(struct domain *d, unsigned int > > socket, > > return -ENOENT; > > } > > > > -int psr_set_l3_cbm(struct domain *d, unsigned int socket, > > - uint64_t cbm, enum cbm_type type) > > +/* Set value functions */ > > +static unsigned int get_cos_num(const struct psr_socket_info *info) > > { > > return 0; > > } > > > > +static int get_old_set_new(uint64_t *val, > > + uint32_t array_len, > > + const struct psr_socket_info *info, > > + unsigned int old_cos, > > + enum cbm_type type, > > + uint64_t m) > > +{ > > +return 0; > > +} > > + > > +static int find_cos(const uint64_t *val, uint32_t array_len, > > +enum cbm_type type, > > +const struct psr_socket_info *info) > > +{ > > +return 0; > > +} > > + > > +static int alloc_new_cos(const struct psr_socket_info *info, > > + const uint64_t *val, uint32_t array_len, > > + unsigned int old_cos, > > + enum cbm_type type) > > +{ > > +return 0; > > +} > > + > > +static int write_psr_msr(unsigned int socket, unsigned int cos, > > + const uint64_t *val) > > +{ > > +return 0; > > +} > > I think all of the above functions should return an error as long as > they're stubbed out, yet I don't think 0 means error in all cases (in > particular a return value of plain int suggests 0 to mean success). > Thanks, will consider the return values carefully. > > +int psr_set_val(struct domain *d, unsigned int socket, > > +uint64_t val, enum cbm_type type) > > +{ > > +unsigned int old_cos; > > +int cos, ret; > > +unsigned int *ref; > > +uint64_t *val_array; > > +struct psr_socket_info *info = get_socket_info(socket); > > +uint32_t array_len; > > + > > +if ( IS_ERR(info) ) > > +return PTR_ERR(info); > > + > > +/* > > + * Step 0: > > + * old_cos means the COS ID current domain is using. By default, it is > > 0. > > + * > > + * For every COS ID, there is a reference count to record how many > > domains > > + * are using the COS register corresponding to this COS ID. > > + * - If ref[old_cos] is 0, that means this COS is not used by any > > domain. > > + * - If ref[old_cos] is 1, that means this COS is only used by current > > + * domain. > > + * - If ref[old_cos] is more than 1, that mean multiple domains are > > using > > + * this COS. > > + */ > > +old_cos = d->arch.psr_cos_ids[socket]; > > +if ( old_cos > MAX_COS_REG_CNT ) > > +return -EOVERFLOW; > > + > > +ref = info->cos_ref; > > + > > +/* > > + * Step 1: > > + * Assemle a value array to store all featues cos_reg_val[old_cos]. > > + * And, set the input val into array according to the feature's > > + * position in array. > > + */ > > +array_len = get_cos_num((const struct psr_socket_info *)info); > > What is this cast doing here? (There are more of this kind below.) > Hmm, I remember there may be warning without cast. Let me confirm it. If no warning, will remove them. > > +val_array = xzalloc_array(uint64_t, array_len); > > +if ( !val_array ) > > +return -ENOMEM; > > + > > +if ( (ret = get_old_set_new(val_array, array_len, > > +(const struct psr_socket_info *)info, > > +old_cos, type, val)) != 0 ) > > Just like for earlier versions I continue to be unconvinced that > the get-current-settings and the replace-target-value should be > a single operation. In particular I'd expect the function to be able > to store the target value, as long as the array entries are > ordered in a suitable way. > Ok, will split to two functions. One for getting old values of all features. The other stores the target value into array acco
Re: [Xen-devel] [PATCH v13 0/3] iommu: add rmrr Xen command line option
> From: Venu Busireddy > Sent: Wednesday, January 11, 2017 6:58 AM > > From: Elena Ufimtseva > > Add Xen command line option rmrr to specify RMRR regions that are not > defined in ACPI thus causing IO Page Faults and prevent dom0 from booting > if "iommu=dom0-strict" option is specified on the Xen command line. > These additional regions will be added to the list of RMRR regions parsed > from ACPI. > > Changes in v13: > - Implement feedback from Kevin Tian. >https://lists.xenproject.org/archives/html/xen-devel/2015-10/msg03169.html >https://lists.xenproject.org/archives/html/xen-devel/2015-10/msg03170.html >https://lists.xenproject.org/archives/html/xen-devel/2015-10/msg03171.html Looks I gave my ack/review to all three patches. But you didn't put my acked-by in patch [3/3]. Is there substantial change against v12 which requires my further review? > - Limit all source lines and comments to 80 characters per line. > - Implement coding style suggestions from Konrad Wilk. > - Changed the Author to Elena Ufimtseva > > Changes in v12: > - Mostly cosmetic fixes from Jan's review on v11. > > Changes in v11: > - changed macro to print extra RMRR ranges and added argument macro; > - fixed the overlapping check if condition error; > - fixed the loop exit condition when checking pfn in RMRR region; > > Elena Ufimtseva (3): > iommu VT-d: separate rmrr addition function. > pci: add wrapper for parse_pci. > iommu: add rmrr Xen command line option for extra rmrrs > > docs/misc/xen-command-line.markdown | 13 ++ > xen/drivers/passthrough/vtd/dmar.c | 324 > +--- > xen/drivers/pci/pci.c | 11 ++ > xen/include/xen/pci.h | 3 + > 4 files changed, 292 insertions(+), 59 deletions(-) > > > ___ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3] x86/HVM: Fix teardown ordering in hvm_vcpu_destroy()
> From: Jan Beulich [mailto:jbeul...@suse.com] > Sent: Tuesday, January 10, 2017 10:42 PM > > >>> On 10.01.17 at 15:26, wrote: > > On 10/01/17 14:15, Andrew Cooper wrote: > >> On 10/01/17 14:03, Suravee Suthikulpanit wrote: > >>> The order of destroy function calls in hvm_vcpu_destroy() should be > >>> the reverse of init calls in hvm_vcpu_initialise(). > >>> > >>> Signed-off-by: Suravee Suthikulpanit > >>> Reviewed-by: Konrad Rzeszutek Wilk > >>> Reviewed-by: Kevin Tian > >>> Cc: Jan Beulich > >>> Cc: Boris Ostrovsky > >> Reviewed-by: Andrew Cooper and queued. > > > > Wait no. > > Which clearly suggests that the earlier R-b-s should have been > dropped too. > > > The order in vcpu_initialise is > > > > hvm_vcpu_cacheattr_init() > > vlapic_init() > > hvm_funcs.vcpu_initialise() > > softirq_tasklet_init() > > setup_compat_arg_xlat() > > > > Therefore, moving the tasklet_kill() is wrong. > > > > The overall delta should be: > > > > --- a/xen/arch/x86/hvm/hvm.c > > +++ b/xen/arch/x86/hvm/hvm.c > > @@ -1626,12 +1626,12 @@ void hvm_vcpu_destroy(struct vcpu *v) > > free_compat_arg_xlat(v); > > > > tasklet_kill(&v->arch.hvm_vcpu.assert_evtchn_irq_tasklet); > > -hvm_vcpu_cacheattr_destroy(v); > > +hvm_funcs.vcpu_destroy(v); > > > > if ( is_hvm_vcpu(v) ) > > vlapic_destroy(v); > > > > -hvm_funcs.vcpu_destroy(v); > > +hvm_vcpu_cacheattr_destroy(v); > > } > > > > IIRC. > > > > If you agree, I will fold this correction in while committing. > > This variant is > Reviewed-by: Jan Beulich > > Jan here is my updated Reviewed-by: Kevin Tian with Andrew's change. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 01/25] x86/cpuid: Introduce guest_cpuid() and struct cpuid_leaf
> From: Andrew Cooper [mailto:andrew.coop...@citrix.com] > Sent: Monday, January 09, 2017 7:03 PM > > Longterm, pv_cpuid() and hvm_cpuid() will be merged into a single > guest_cpuid(), which is also capable of working outside of current context. > > To aid this transtion, introduce guest_cpuid() with the intended API, which > simply defers back to pv_cpuid() or hvm_cpuid() as appropriate. > > Introduce struct cpuid_leaf which is used to represent the results of a CPUID > query in a more efficient mannor than passing four pointers through the > calltree. > > Update all codepaths which should use the new guest_cpuid() API. These are > the codepaths which have variable inputs, and (other than some specific > x86_emulate() cases) all pertain to servicing a CPUID instruction from a > guest. > > The other codepaths using {pv,hvm}_cpuid() with fixed inputs will later be > adjusted to read their data straight from the policy block. > > No intended functional change. > > Signed-off-by: Andrew Cooper Reviewed-by: Kevin Tian ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2] x86/HVM: restrict permitted instructions during special purpose emulation
> From: Jan Beulich [mailto:jbeul...@suse.com] > Sent: Thursday, January 05, 2017 6:55 PM > > Most invocations of the instruction emulator are for VM exits where the > set of legitimate instructions (i.e. ones capable of causing the > respective exit) is rather small. Restrict the permitted sets via a new > callback, at once eliminating the abuse of handle_mmio() for non-MMIO > operations. > > A seemingly unrelated comment adjustment is being done here to keep > x86_emulate() in sync with x86_insn_is_mem_write() (in the context of > which this was found to be wrong). > > Signed-off-by: Jan Beulich Reviewed-by: Kevin Tian ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] x86/VMX: Implement vmptrst
> From: Anshul Makkar [mailto:anshul.mak...@citrix.com] > Sent: Friday, January 06, 2017 2:42 AM > > Current codebase doesn't implement and use vmptrst. Implementing it as it may > be required in future. > > Signed-off-by: Anshul Makkar Then let's do it when it's really required. :-) Thanks Kevin ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [ovmf test] 104109: regressions - FAIL
flight 104109 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/104109/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-i386-xsm5 xen-buildfail REGR. vs. 104105 build-amd64-xsm 5 xen-buildfail REGR. vs. 104105 build-i3865 xen-buildfail REGR. vs. 104105 build-amd64 5 xen-buildfail REGR. vs. 104105 Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-qemuu-ovmf-amd64 1 build-check(1) blocked n/a build-amd64-libvirt 1 build-check(1) blocked n/a test-amd64-i386-xl-qemuu-ovmf-amd64 1 build-check(1) blocked n/a build-i386-libvirt1 build-check(1) blocked n/a version targeted for testing: ovmf de8cea6f3c2d83966e131a2edc4b26daf970ccd4 baseline version: ovmf 1b3d5b0699cf6222698ed8373026813b61e87637 Last test of basis 104105 2017-01-11 00:14:50 Z0 days Testing same since 104107 2017-01-11 03:29:54 Z0 days2 attempts People who touched revisions under test: Chen A Chen Ruiyu Ni jobs: build-amd64-xsm fail build-i386-xsm fail build-amd64 fail build-i386 fail build-amd64-libvirt blocked build-i386-libvirt blocked build-amd64-pvopspass build-i386-pvops pass test-amd64-amd64-xl-qemuu-ovmf-amd64 blocked test-amd64-i386-xl-qemuu-ovmf-amd64 blocked 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 Not pushing. commit de8cea6f3c2d83966e131a2edc4b26daf970ccd4 Author: Ruiyu Ni Date: Mon Jan 9 10:51:58 2017 +0800 ShellPkg/dh: Support dump from GUID and "decode" parameter To follow Shell spec 2.2, change "dh" to support dump from protocol GUID and support "decode" parameter to dump the GUID/name mapping. Contributed-under: TianoCore Contribution Agreement 1.0 Cc: Jaben Carsey Cc: Ruiyu Ni Signed-off-by: Chen A Chen Signed-off-by: Ruiyu Ni Reviewed-by: Jaben Carsey commit 28f898f856c8b35e8f578ad2855df23b2cc6fc91 Author: Chen A Chen Date: Thu Jan 5 13:29:50 2017 +0800 ShellPkg/Dh: Fix coding style issues The change doesn't impact the functionality. Contributed-under: TianoCore Contribution Agreement 1.0 Cc: Jaben Carsey Cc: Ruiyu Ni Signed-off-by: Ruiyu Ni Signed-off-by: Chen A Chen Reviewed-by: Jaben Carsey commit 0976f90821e2677a85d72e93de9dc140c2c99742 Author: Chen A Chen Date: Thu Dec 29 14:52:45 2016 +0800 ShellPkg/HandleParsingLib: Add new API GetAllMappingGuids Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Ruiyu Ni Signed-off-by: Chen A Chen Reviewed-by: Jaben Carsey commit 0e88348e4b252cd68b88e1d87085208be2293651 Author: Ruiyu Ni Date: Mon Jan 9 16:45:40 2017 +0800 ShellPkg/HandleParsingLib: Return NULL name for unknown GUID GetStringNameFromGuid() returns NULL for unknown GUID, instead of returning "UnknownDevice". The behavior change matches ShellProtocol.GetGuidName(). Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Ruiyu Ni Reviewed-by: Jaben Carsey commit d4ec9a5725d67bb1770008513f4c6f1dce2c9b43 Author: Ruiyu Ni Date: Mon Jan 9 13:21:28 2017 +0800 ShellPkg/HandleParsingLib: Rename global variables Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Ruiyu Ni Reviewed-by: Jaben Carsey ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 3/3] xen: optimize xenbus driver for multiple concurrent xenstore accesses
On 10/01/17 23:56, Boris Ostrovsky wrote: > > > >> diff --git a/drivers/xen/xenbus/xenbus_xs.c b/drivers/xen/xenbus/xenbus_xs.c >> index ebc768f..ebdfbee 100644 >> --- a/drivers/xen/xenbus/xenbus_xs.c >> +++ b/drivers/xen/xenbus/xenbus_xs.c > > >> - >> -static struct xs_handle xs_state; >> +/* >> + * Framework to protect suspend/resume handling against normal Xenstore >> + * message handling: >> + * During suspend/resume there must be no open transaction and no pending >> + * Xenstore request. >> + * New watch events happening in this time can be ignored by firing all >> watches >> + * after resume. >> + */ >> +/* Lock protecting enter/exit critical region. */ >> +static DEFINE_SPINLOCK(xs_state_lock); >> +/* Wait queue for all callers waiting for critical region to become usable. >> */ >> +static DECLARE_WAIT_QUEUE_HEAD(xs_state_enter_wq); >> +/* Wait queue for suspend handling waiting for critical region being empty. >> */ >> +static DECLARE_WAIT_QUEUE_HEAD(xs_state_exit_wq); >> +/* Number of users in critical region. */ >> +static unsigned int xs_state_users; >> +/* Suspend handler waiting or already active? */ >> +static int xs_suspend_active; > > I think these two should be declared next to xs_state _lock since they > are protected by it. Or maybe even put them into some sort of a state > struct. I think placing them near the lock and adding a comment is enough. >> + >> + >> +static bool test_reply(struct xb_req_data *req) >> +{ >> +if (req->state == xb_req_state_got_reply || !xenbus_ok()) >> +return true; >> + >> +/* Make sure to reread req->state each time. */ >> +cpu_relax(); > > I don't think I understand why this is needed. I need a compiler barrier. Otherwise the compiler read req->state only once outside the while loop. >> + >> +return false; >> +} >> + > > >> +static void xs_send(struct xb_req_data *req, struct xsd_sockmsg *msg) >> { >> -mutex_lock(&xs_state.transaction_mutex); >> -atomic_inc(&xs_state.transaction_count); >> -mutex_unlock(&xs_state.transaction_mutex); >> -} >> +bool notify; >> >> -static void transaction_end(void) >> -{ >> -if (atomic_dec_and_test(&xs_state.transaction_count)) >> -wake_up(&xs_state.transaction_wq); >> -} >> +req->msg = *msg; >> +req->err = 0; >> +req->state = xb_req_state_queued; >> +init_waitqueue_head(&req->wq); >> >> -static void transaction_suspend(void) >> -{ >> -mutex_lock(&xs_state.transaction_mutex); >> -wait_event(xs_state.transaction_wq, >> - atomic_read(&xs_state.transaction_count) == 0); >> -} >> +xs_request_enter(req); >> >> -static void transaction_resume(void) >> -{ >> -mutex_unlock(&xs_state.transaction_mutex); >> +req->msg.req_id = xs_request_id++; > > Is it safe to do this without a lock? You are right: I should move this to xs_request_enter() inside the lock. I think I'll let return xs_request_enter() the request id. >> + >> +int xenbus_dev_request_and_reply(struct xsd_sockmsg *msg, void *par) >> +{ >> +struct xb_req_data *req; >> +struct kvec *vec; >> + >> +req = kmalloc(sizeof(*req) + sizeof(*vec), GFP_KERNEL); > > Is there a reason why you are using different flags here? Yes. This function is always called in user context. No need to be more restrictive. >> @@ -263,11 +295,20 @@ static void *xs_talkv(struct xenbus_transaction t, >>unsigned int num_vecs, >>unsigned int *len) >> { >> +struct xb_req_data *req; >> struct xsd_sockmsg msg; >> void *ret = NULL; >> unsigned int i; >> int err; >> >> +req = kmalloc(sizeof(*req), GFP_NOIO | __GFP_HIGH); >> +if (!req) >> +return ERR_PTR(-ENOMEM); >> + >> +req->vec = iovec; >> +req->num_vecs = num_vecs; >> +req->cb = xs_wake_up; >> + >> msg.tx_id = t.id; >> msg.req_id = 0; > > Is this still needed? You are assigning it in xs_send(). Right. Can be removed. >> +static int xs_reboot_notify(struct notifier_block *nb, >> +unsigned long code, void *unused) >> { >> -struct xs_stored_msg *msg; > > > >> +struct xb_req_data *req; >> + >> +mutex_lock(&xb_write_mutex); >> +list_for_each_entry(req, &xs_reply_list, list) >> +wake_up(&req->wq); >> +list_for_each_entry(req, &xb_write_list, list) >> +wake_up(&req->wq); > > We are waking up waiters here but there is not guarantee that waiting > threads will have a chance to run, is there? You are right. But this isn't the point. We want to avoid blocking a reboot due to some needed thread waiting for xenstore. And this task is being accomplished here. Juergen ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 07/24] x86: refactor psr: implement get value flow.
On 17-01-10 06:50:36, Jan Beulich wrote: > >>> On 14.12.16 at 05:07, wrote: > > This patch implements get value flow including L3 CAT callback > > function. > > > > It also changes domctl interface to make it more general. > > > > With this patch, 'psr-cat-show' can work for L3 CAT. > > How about CDP? You don't implement anything for it here, and looking > over the subjects of the remaining patches there also doesn't seem to > be anything to come. > I split CDP codes out to a few patches from 13 to 16. CDP is a stand alone feature now but not combined with L3 CAT. [PATCH v4 13/24] x86: refactor psr: implement CPU init and free flow for CDP. > Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 06/24] x86: refactor psr: implement get hw info flow.
On 17-01-10 06:46:03, Jan Beulich wrote: > >>> On 14.12.16 at 05:07, wrote: > > --- a/xen/arch/x86/psr.c > > +++ b/xen/arch/x86/psr.c > > @@ -115,6 +115,9 @@ struct feat_ops { > > struct psr_socket_info *info); > > /* get_max_cos_max is used to get feature's cos_max. */ > > unsigned int (*get_max_cos_max)(const struct feat_node *feat); > > +/* get_feat_info is used to get feature HW info. */ > > +bool (*get_feat_info)(const struct feat_node *feat, enum cbm_type type, > > + uint32_t dat[], uint32_t array_len); > > data, value, or val would all seem okay, but dat suggests an acronym > of other than data (which I think it is meant to be). > Ok, will change it to data. > > @@ -220,9 +223,24 @@ static unsigned int l3_cat_get_max_cos_max(const > > struct > > feat_node *feat) > > return feat->info.l3_cat_info.cos_max; > > } > > > > +static bool l3_cat_get_feat_info(const struct feat_node *feat, > > + enum cbm_type type, > > + uint32_t dat[], uint32_t array_len) > > array_len wants to be size_t or unsigned int. And more generally, > please avoid fixed width types when you don't really mean such. > Got it, thank you! > > +int psr_get_info(unsigned int socket, enum cbm_type type, > > + uint32_t dat[], uint32_t array_len) > > +{ > > +struct psr_socket_info *info = get_socket_info(socket); > > +struct feat_node *feat_tmp; > > With the hook function taking a pointer to const I don#t see why > this one can't be const, too. > Do you mean feat_tmp? Thanks! > > +if ( IS_ERR(info) ) > > +return PTR_ERR(info); > > + > > +list_for_each_entry(feat_tmp, &info->feat_list, list) > > +if ( feat_tmp->ops.get_feat_info(feat_tmp, type, dat, array_len) ) > > Wouldn't the type check better be done here than inside each > function? That would then also allow for terminating the loop > earlier (when the type was found, instead of when a function > returns success). > Ok, thanks for the suggestion! > > --- a/xen/include/asm-x86/psr.h > > +++ b/xen/include/asm-x86/psr.h > > @@ -33,6 +33,11 @@ > > /* L3 CDP Enable bit*/ > > #define PSR_L3_QOS_CDP_ENABLE_BIT 0x0 > > > > +/* Used by psr_get_info() */ > > +#define CBM_LEN 0 > > +#define COS_MAX 1 > > +#define CDP_FLAG 2 > > These needing putting in a header means that you want to prefix > them, e.g. by PSR_. Also with the next value used e.g. as array > dimension, I think you also want to name that value (currently 3) > and use it instead of a plain number which would need to be > adjusted everywhere once a value gets added here. > > Also - is CDP_FLAG really a suitable name for flags? Wouldn't > PSR_FLAGS be better (as being more general)? > Thanks for the suggestions! > Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH 1/9] treewide: Constify most dma_map_ops structures
Most dma_map_ops structures are never modified. Constify these structures such that these can be write-protected. This patch has been generated as follows: git grep -l 'struct dma_map_ops' | xargs -d\\n sed -i \ -e 's/struct dma_map_ops/const struct dma_map_ops/g' \ -e 's/const struct dma_map_ops {/struct dma_map_ops {/g' \ -e 's/^const struct dma_map_ops;$/struct dma_map_ops;/' \ -e 's/const const struct dma_map_ops /const struct dma_map_ops /g'; sed -i -e 's/const \(struct dma_map_ops intel_dma_ops\)/\1/' \ $(git grep -l 'struct dma_map_ops intel_dma_ops'); sed -i -e 's/const \(struct dma_map_ops dma_iommu_ops\)/\1/' \ $(git grep -l 'struct dma_map_ops' | grep ^arch/powerpc); sed -i -e '/^struct vmd_dev {$/,/^};$/ s/const \(struct dma_map_ops[[:blank:]]dma_ops;\)/\1/' \ -e '/^static void vmd_setup_dma_ops/,/^}$/ s/const \(struct dma_map_ops \*dest\)/\1/' \ -e 's/const \(struct dma_map_ops \*dest = \&vmd->dma_ops\)/\1/' \ drivers/pci/host/*.c sed -i -e '/^void __init pci_iommu_alloc(void)$/,/^}$/ s/dma_ops->/intel_dma_ops./' arch/ia64/kernel/pci-dma.c sed -i -e 's/static const struct dma_map_ops sn_dma_ops/static struct dma_map_ops sn_dma_ops/' arch/ia64/sn/pci/pci_dma.c Signed-off-by: Bart Van Assche Reviewed-by: Christoph Hellwig Cc: Aurelien Jacquiot Cc: Catalin Marinas Cc: Chris Zankel Cc: David Howells Cc: David S. Miller Cc: Fenghua Yu Cc: Geert Uytterhoeven Cc: Geoff Levand Cc: Guan Xuetao Cc: H. Peter Anvin Cc: Haavard Skinnemoen Cc: Hans-Christian Egtvedt Cc: Helge Deller Cc: Ingo Molnar Cc: James E.J. Bottomley Cc: Jesper Nilsson Cc: Joerg Roedel Cc: Jon Mason Cc: Jonas Bonn Cc: Ley Foon Tan Cc: Mark Salter Cc: Max Filippov Cc: Mikael Starvik Cc: Muli Ben-Yehuda Cc: Rich Felker Cc: Russell King Cc: Stafford Horne Cc: Stefan Kristiansson Cc: Stefano Stabellini Cc: Thomas Gleixner Cc: Tony Luck Cc: Will Deacon Cc: x...@kernel.org Cc: Yoshinori Sato Cc: adi-buildroot-de...@lists.sourceforge.net Cc: io...@lists.linux-foundation.org Cc: linux-al...@vger.kernel.org Cc: linux-am33-l...@redhat.com Cc: linux-arm-ker...@lists.infradead.org Cc: linux-c6x-...@linux-c6x.org Cc: linux-cris-ker...@axis.com Cc: linux-hexa...@vger.kernel.org Cc: linux-i...@vger.kernel.org Cc: linux-m...@lists.linux-m68k.org Cc: linux-me...@vger.kernel.org Cc: linux-m...@linux-mips.org Cc: linux-par...@vger.kernel.org Cc: linux-...@vger.kernel.org Cc: linux-s...@vger.kernel.org Cc: linux...@vger.kernel.org Cc: linux-snps-...@lists.infradead.org Cc: linux-xte...@linux-xtensa.org Cc: linuxppc-...@lists.ozlabs.org Cc: nios2-...@lists.rocketboards.org Cc: openr...@lists.librecores.org Cc: sparcli...@vger.kernel.org Cc: uclinux-h8-de...@lists.sourceforge.jp Cc: xen-de...@lists.xenproject.org --- arch/alpha/include/asm/dma-mapping.h | 4 +-- arch/alpha/kernel/pci-noop.c | 4 +-- arch/alpha/kernel/pci_iommu.c | 4 +-- arch/arc/include/asm/dma-mapping.h | 4 +-- arch/arc/mm/dma.c | 2 +- arch/arm/common/dmabounce.c| 2 +- arch/arm/include/asm/device.h | 2 +- arch/arm/include/asm/dma-mapping.h | 10 +++--- arch/arm/mm/dma-mapping.c | 22 ++-- arch/arm/xen/mm.c | 4 +-- arch/arm64/include/asm/device.h| 2 +- arch/arm64/include/asm/dma-mapping.h | 6 ++-- arch/arm64/mm/dma-mapping.c| 6 ++-- arch/avr32/include/asm/dma-mapping.h | 4 +-- arch/avr32/mm/dma-coherent.c | 2 +- arch/blackfin/include/asm/dma-mapping.h| 4 +-- arch/blackfin/kernel/dma-mapping.c | 2 +- arch/c6x/include/asm/dma-mapping.h | 4 +-- arch/c6x/kernel/dma.c | 2 +- arch/cris/arch-v32/drivers/pci/dma.c | 2 +- arch/cris/include/asm/dma-mapping.h| 6 ++-- arch/frv/include/asm/dma-mapping.h | 4 +-- arch/frv/mb93090-mb00/pci-dma-nommu.c | 2 +- arch/frv/mb93090-mb00/pci-dma.c| 2 +- arch/h8300/include/asm/dma-mapping.h | 4 +-- arch/h8300/kernel/dma.c| 2 +- arch/hexagon/include/asm/dma-mapping.h | 4 +-- arch/hexagon/kernel/dma.c | 4 +-- arch/ia64/hp/common/hwsw_iommu.c | 4 +-- arch/ia64/hp/common/sba_iommu.c| 4 +-- arch/ia64/include/asm/dma-mapping.h| 2 +- arch/ia64/include/asm/machvec.h| 4 +-- arch/ia64/kernel/dma-mapping.c | 4 +-- arch/ia64/kernel/pci-dma.c | 10 +++--- arch/ia64/kernel/pci-swiotlb.c | 2 +- arch/m32r/include/asm/device.h | 2 +- a
[Xen-devel] [PATCH v13 0/3] iommu: add rmrr Xen command line option
From: Elena Ufimtseva Add Xen command line option rmrr to specify RMRR regions that are not defined in ACPI thus causing IO Page Faults and prevent dom0 from booting if "iommu=dom0-strict" option is specified on the Xen command line. These additional regions will be added to the list of RMRR regions parsed from ACPI. Changes in v13: - Implement feedback from Kevin Tian. https://lists.xenproject.org/archives/html/xen-devel/2015-10/msg03169.html https://lists.xenproject.org/archives/html/xen-devel/2015-10/msg03170.html https://lists.xenproject.org/archives/html/xen-devel/2015-10/msg03171.html - Limit all source lines and comments to 80 characters per line. - Implement coding style suggestions from Konrad Wilk. - Changed the Author to Elena Ufimtseva Changes in v12: - Mostly cosmetic fixes from Jan's review on v11. Changes in v11: - changed macro to print extra RMRR ranges and added argument macro; - fixed the overlapping check if condition error; - fixed the loop exit condition when checking pfn in RMRR region; Elena Ufimtseva (3): iommu VT-d: separate rmrr addition function. pci: add wrapper for parse_pci. iommu: add rmrr Xen command line option for extra rmrrs docs/misc/xen-command-line.markdown | 13 ++ xen/drivers/passthrough/vtd/dmar.c | 324 +--- xen/drivers/pci/pci.c | 11 ++ xen/include/xen/pci.h | 3 + 4 files changed, 292 insertions(+), 59 deletions(-) ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v13 3/3] iommu: add rmrr Xen command line option for extra rmrrs
From: Elena Ufimtseva On some platforms firmware fails to specify RMRR regions in ACPI tables and thus those regions will not be mapped in dom0 or guests and may cause IO Page Faults and prevent dom0 from booting if "iommu=dom0-strict" option is specified on the Xen command line. New Xen command line option rmrr allows to specify such devices and memory regions. These regions are added to the list of RMRR defined in ACPI if the device is present in system. As a result, additional RMRRs will be mapped 1:1 in dom0 with correct permissions. The above mentioned problems were discovered during the PVH work with ThinkCentre M and Dell 5600T. No official documentation was found so far in regards to what devices and why cause this. Experiments show that ThinkCentre M USB devices with enabled debug port generate DMA read transactions to the regions of memory marked reserved in host e820 map. For Dell 5600T the device and faulting addresses are not found yet. For detailed history of the discussion please check following threads: http://lists.Xen.org/archives/html/xen-devel/2015-02/msg01724.html http://lists.Xen.org/archives/html/xen-devel/2015-01/msg02513.html Format for rmrr Xen command line option: rmrr=start<-end>=[s1]bdf1[,[s1]bdf2[,...]];start<-end>=[s2]bdf1[,[s2]bdf2[,...]] For example, for Lenovo ThinkCentre M, use: rmrr=0xd5d45=0:0:1d.0;0xd5d46=0:0:1a.0 If grub2 used and multiple ranges are specified, ';' should be quoted/escaped, refer to grub2 manual for more information. Signed-off-by: Elena Ufimtseva Signed-off-by: Venu Busireddy --- docs/misc/xen-command-line.markdown | 13 +++ xen/drivers/passthrough/vtd/dmar.c | 201 +++- 2 files changed, 213 insertions(+), 1 deletion(-) Changes in v13: - Implement feedback from Kevin Tian. https://lists.xenproject.org/archives/html/xen-devel/2015-10/msg03169.html https://lists.xenproject.org/archives/html/xen-devel/2015-10/msg03170.html https://lists.xenproject.org/archives/html/xen-devel/2015-10/msg03171.html - Limit all source lines and comments to 80 characters per line. - Implement coding style suggestions from Konrad Wilk. diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown index 0138978..a11fdf9 100644 --- a/docs/misc/xen-command-line.markdown +++ b/docs/misc/xen-command-line.markdown @@ -1377,6 +1377,19 @@ Specify the host reboot method. 'efi' instructs Xen to reboot using the EFI reboot call (in EFI mode by default it will use that method first). +### rmrr +> '= start<-end>=[s1]bdf1[,[s1]bdf2[,...]];start<-end>=[s2]bdf1[,[s2]bdf2[,...]] + +Define RMRR units that are missing from ACPI table along with device they +belong to and use them for 1:1 mapping. End addresses can be omitted and one +page will be mapped. The ranges are inclusive when start and end are specified. +If segment of the first device is not specified, segment zero will be used. +If other segments are not specified, first device segment will be used. +If a segment is specified for other than the first device and it does not match +the one specified for the first one, an error will be reported. +Note: grub2 requires to escape or use quotations if special characters are used, +namely ';', refer to the grub2 documentation if multiple ranges are specified. + ### ro-hpet > `= ` diff --git a/xen/drivers/passthrough/vtd/dmar.c b/xen/drivers/passthrough/vtd/dmar.c index 84bf63d..23ba7a6 100644 --- a/xen/drivers/passthrough/vtd/dmar.c +++ b/xen/drivers/passthrough/vtd/dmar.c @@ -859,6 +859,132 @@ out: return ret; } +#define MAX_EXTRA_RMRR_PAGES 16 +#define MAX_EXTRA_RMRR 10 + +/* RMRR units derived from command line rmrr option. */ +#define MAX_EXTRA_RMRR_DEV 20 +struct user_rmrr { +struct list_head list; +unsigned long base_pfn, end_pfn; +unsigned int dev_count; +u32 sbdf[MAX_EXTRA_RMRR_DEV]; +}; + +static __initdata unsigned int nr_rmrr; +static struct __initdata user_rmrr user_rmrrs[MAX_EXTRA_RMRR]; + +/* Macro for RMRR inclusive range formatting. */ +#define ERMRRU_FMT "[%lx-%lx]" +#define ERMRRU_ARG(eru) eru.base_pfn, eru.end_pfn + +static int __init add_user_rmrr(void) +{ +struct acpi_rmrr_unit *rmrr, *rmrru; +unsigned int idx, seg, i; +unsigned long base, end; +bool overlap; + +for ( i = 0; i < nr_rmrr; i++ ) +{ +base = user_rmrrs[i].base_pfn; +end = user_rmrrs[i].end_pfn; + +if ( base > end ) +{ +printk(XENLOG_ERR VTDPREFIX + "Invalid RMRR Range "ERMRRU_FMT"\n", + ERMRRU_ARG(user_rmrrs[i])); +continue; +} + +if ( (end - base) >= MAX_EXTRA_RMRR_PAGES ) +{ +printk(XENLOG_ERR VTDPREFIX + "RMRR range "ERMRRU_FMT" exceeds "\ + __stringify(MAX_EXTRA_RMRR_PAGES)" pages\n", + ERMRRU_ARG(user_rmrrs[i])); +continue; +} + +
[Xen-devel] [PATCH v13 2/3] pci: add wrapper for parse_pci.
From: Elena Ufimtseva For sbdf's parsing in RMRR command line, add parse_pci_seg with additional parameter def_seg. parse_pci_seg will help to identify if segment was found in string being parsed or default segment was used. Make a wrapper parse_pci so the rest of the callers are not affected. Signed-off-by: Elena Ufimtseva Signed-off-by: Venu Busireddy --- xen/drivers/pci/pci.c | 11 +++ xen/include/xen/pci.h | 3 +++ 2 files changed, 14 insertions(+) v13: - Renamed __parse_pci() to parse_pci_seg() as suggested by Konrad Wilk. diff --git a/xen/drivers/pci/pci.c b/xen/drivers/pci/pci.c index ca07ed0..13d3309 100644 --- a/xen/drivers/pci/pci.c +++ b/xen/drivers/pci/pci.c @@ -119,11 +119,21 @@ const char *__init parse_pci(const char *s, unsigned int *seg_p, unsigned int *bus_p, unsigned int *dev_p, unsigned int *func_p) { +bool def_seg; + +return parse_pci_seg(s, seg_p, bus_p, dev_p, func_p, &def_seg); +} + +const char *__init parse_pci_seg(const char *s, unsigned int *seg_p, + unsigned int *bus_p, unsigned int *dev_p, + unsigned int *func_p, bool *def_seg) +{ unsigned long seg = simple_strtoul(s, &s, 16), bus, dev, func; if ( *s != ':' ) return NULL; bus = simple_strtoul(s + 1, &s, 16); +*def_seg = false; if ( *s == ':' ) dev = simple_strtoul(s + 1, &s, 16); else @@ -131,6 +141,7 @@ const char *__init parse_pci(const char *s, unsigned int *seg_p, dev = bus; bus = seg; seg = 0; +*def_seg = true; } if ( func_p ) { diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h index 0872401..59b6e8a 100644 --- a/xen/include/xen/pci.h +++ b/xen/include/xen/pci.h @@ -159,6 +159,9 @@ int pci_find_ext_capability(int seg, int bus, int devfn, int cap); int pci_find_next_ext_capability(int seg, int bus, int devfn, int pos, int cap); const char *parse_pci(const char *, unsigned int *seg, unsigned int *bus, unsigned int *dev, unsigned int *func); +const char *parse_pci_seg(const char *, unsigned int *seg, unsigned int *bus, + unsigned int *dev, unsigned int *func, bool *def_seg); + bool_t pcie_aer_get_firmware_first(const struct pci_dev *); ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v13 1/3] iommu VT-d: separate rmrr addition function.
From: Elena Ufimtseva In preparation for auxiliary RMRR data provided on Xen command line, make RMRR adding a separate function. Also free memery for rmrr device scope in error path. Signed-off-by: Elena Ufimtseva Signed-off-by: Venu Busireddy Acked-by: Kevin Tian --- xen/drivers/passthrough/vtd/dmar.c | 123 - 1 file changed, 65 insertions(+), 58 deletions(-) diff --git a/xen/drivers/passthrough/vtd/dmar.c b/xen/drivers/passthrough/vtd/dmar.c index 08c1d2d..84bf63d 100644 --- a/xen/drivers/passthrough/vtd/dmar.c +++ b/xen/drivers/passthrough/vtd/dmar.c @@ -579,6 +579,68 @@ out: return ret; } +static int register_one_rmrr(struct acpi_rmrr_unit *rmrru) +{ +bool ignore = false; +unsigned int i = 0; +int ret = 0; + +/* Skip checking if segment is not accessible yet. */ +if ( !pci_known_segment(rmrru->segment) ) +i = UINT_MAX; + +for ( ; i < rmrru->scope.devices_cnt; i++ ) +{ +u8 b = PCI_BUS(rmrru->scope.devices[i]); +u8 d = PCI_SLOT(rmrru->scope.devices[i]); +u8 f = PCI_FUNC(rmrru->scope.devices[i]); + +if ( pci_device_detect(rmrru->segment, b, d, f) == 0 ) +{ +dprintk(XENLOG_WARNING VTDPREFIX, +" Non-existent device (%04x:%02x:%02x.%u) is reported" +" in RMRR (%"PRIx64", %"PRIx64")'s scope!\n", +rmrru->segment, b, d, f, +rmrru->base_address, rmrru->end_address); +ignore = true; +} +else +{ +ignore = false; +break; +} +} + +if ( ignore ) +{ +dprintk(XENLOG_WARNING VTDPREFIX, +" Ignore the RMRR (%"PRIx64", %"PRIx64") due to " +"devices under its scope are not PCI discoverable!\n", +rmrru->base_address, rmrru->end_address); +scope_devices_free(&rmrru->scope); +xfree(rmrru); +} +else if ( rmrru->base_address > rmrru->end_address ) +{ +dprintk(XENLOG_WARNING VTDPREFIX, +" The RMRR (%"PRIx64", %"PRIx64") is incorrect!\n", +rmrru->base_address, rmrru->end_address); +scope_devices_free(&rmrru->scope); +xfree(rmrru); +ret = -EFAULT; +} +else +{ +if ( iommu_verbose ) +dprintk(VTDPREFIX, +" RMRR region: base_addr %"PRIx64" end_addr %"PRIx64"\n", +rmrru->base_address, rmrru->end_address); +acpi_register_rmrr_unit(rmrru); +} + +return ret; +} + static int __init acpi_parse_one_rmrr(struct acpi_dmar_header *header) { @@ -628,65 +690,10 @@ acpi_parse_one_rmrr(struct acpi_dmar_header *header) ret = acpi_parse_dev_scope(dev_scope_start, dev_scope_end, &rmrru->scope, RMRR_TYPE, rmrr->segment); -if ( ret || (rmrru->scope.devices_cnt == 0) ) -xfree(rmrru); +if ( !ret && (rmrru->scope.devices_cnt != 0) ) +register_one_rmrr(rmrru); else -{ -u8 b, d, f; -bool_t ignore = 0; -unsigned int i = 0; - -/* Skip checking if segment is not accessible yet. */ -if ( !pci_known_segment(rmrr->segment) ) -i = UINT_MAX; - -for ( ; i < rmrru->scope.devices_cnt; i++ ) -{ -b = PCI_BUS(rmrru->scope.devices[i]); -d = PCI_SLOT(rmrru->scope.devices[i]); -f = PCI_FUNC(rmrru->scope.devices[i]); - -if ( !pci_device_detect(rmrr->segment, b, d, f) ) -{ -printk(XENLOG_WARNING VTDPREFIX - " Non-existent device (%04x:%02x:%02x.%u) reported in RMRR (%"PRIx64", %"PRIx64")'s scope!\n", - rmrr->segment, b, d, f, - rmrru->base_address, rmrru->end_address); -ignore = 1; -} -else -{ -ignore = 0; -break; -} -} - -if ( ignore ) -{ -printk(XENLOG_WARNING VTDPREFIX - " Ignore RMRR (%"PRIx64", %"PRIx64") (some devices in its scope are not PCI discoverable)\n", - rmrru->base_address, rmrru->end_address); -scope_devices_free(&rmrru->scope); -xfree(rmrru); -} -else if ( base_addr > end_addr ) -{ -printk(XENLOG_WARNING VTDPREFIX - " RMRR (%"PRIx64", %"PRIx64") is incorrect\n", - rmrru->base_address, rmrru->end_address); -scope_devices_free(&rmrru->scope); -xfree(rmrru); -ret = -EFAULT; -} -else -{ -if ( iommu_verbose ) -printk(VTDPREFIX - " RMRR region: base_addr %"PRIx64" end_address %"PRIx64"\n", - rmrru->base_address, rmrru->end_address); -acpi_register_rmrr
[Xen-devel] [ovmf test] 104107: regressions - FAIL
flight 104107 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/104107/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-i386-xsm5 xen-buildfail REGR. vs. 104105 build-amd64-xsm 5 xen-buildfail REGR. vs. 104105 build-i3865 xen-buildfail REGR. vs. 104105 build-amd64 5 xen-buildfail REGR. vs. 104105 Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-qemuu-ovmf-amd64 1 build-check(1) blocked n/a build-amd64-libvirt 1 build-check(1) blocked n/a test-amd64-i386-xl-qemuu-ovmf-amd64 1 build-check(1) blocked n/a build-i386-libvirt1 build-check(1) blocked n/a version targeted for testing: ovmf de8cea6f3c2d83966e131a2edc4b26daf970ccd4 baseline version: ovmf 1b3d5b0699cf6222698ed8373026813b61e87637 Last test of basis 104105 2017-01-11 00:14:50 Z0 days Testing same since 104107 2017-01-11 03:29:54 Z0 days1 attempts People who touched revisions under test: Chen A Chen Ruiyu Ni jobs: build-amd64-xsm fail build-i386-xsm fail build-amd64 fail build-i386 fail build-amd64-libvirt blocked build-i386-libvirt blocked build-amd64-pvopspass build-i386-pvops pass test-amd64-amd64-xl-qemuu-ovmf-amd64 blocked test-amd64-i386-xl-qemuu-ovmf-amd64 blocked 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 Not pushing. commit de8cea6f3c2d83966e131a2edc4b26daf970ccd4 Author: Ruiyu Ni Date: Mon Jan 9 10:51:58 2017 +0800 ShellPkg/dh: Support dump from GUID and "decode" parameter To follow Shell spec 2.2, change "dh" to support dump from protocol GUID and support "decode" parameter to dump the GUID/name mapping. Contributed-under: TianoCore Contribution Agreement 1.0 Cc: Jaben Carsey Cc: Ruiyu Ni Signed-off-by: Chen A Chen Signed-off-by: Ruiyu Ni Reviewed-by: Jaben Carsey commit 28f898f856c8b35e8f578ad2855df23b2cc6fc91 Author: Chen A Chen Date: Thu Jan 5 13:29:50 2017 +0800 ShellPkg/Dh: Fix coding style issues The change doesn't impact the functionality. Contributed-under: TianoCore Contribution Agreement 1.0 Cc: Jaben Carsey Cc: Ruiyu Ni Signed-off-by: Ruiyu Ni Signed-off-by: Chen A Chen Reviewed-by: Jaben Carsey commit 0976f90821e2677a85d72e93de9dc140c2c99742 Author: Chen A Chen Date: Thu Dec 29 14:52:45 2016 +0800 ShellPkg/HandleParsingLib: Add new API GetAllMappingGuids Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Ruiyu Ni Signed-off-by: Chen A Chen Reviewed-by: Jaben Carsey commit 0e88348e4b252cd68b88e1d87085208be2293651 Author: Ruiyu Ni Date: Mon Jan 9 16:45:40 2017 +0800 ShellPkg/HandleParsingLib: Return NULL name for unknown GUID GetStringNameFromGuid() returns NULL for unknown GUID, instead of returning "UnknownDevice". The behavior change matches ShellProtocol.GetGuidName(). Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Ruiyu Ni Reviewed-by: Jaben Carsey commit d4ec9a5725d67bb1770008513f4c6f1dce2c9b43 Author: Ruiyu Ni Date: Mon Jan 9 13:21:28 2017 +0800 ShellPkg/HandleParsingLib: Rename global variables Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Ruiyu Ni Reviewed-by: Jaben Carsey ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [ovmf test] 104105: all pass - PUSHED
flight 104105 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/104105/ Perfect :-) All tests in this flight passed as required version targeted for testing: ovmf 1b3d5b0699cf6222698ed8373026813b61e87637 baseline version: ovmf f4d575b51bbdb61e9f634dcabb84149ca836cffa Last test of basis 104103 2017-01-10 22:14:23 Z0 days Testing same since 104105 2017-01-11 00:14:50 Z0 days1 attempts People who touched revisions under test: Daniil Egranov Maurice Ma 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 : + branch=ovmf + revision=1b3d5b0699cf6222698ed8373026813b61e87637 + . ./cri-lock-repos ++ . ./cri-common +++ . ./cri-getconfig +++ umask 002 +++ getrepos getconfig Repos perl -e ' use Osstest; readglobalconfig(); print $c{"Repos"} or die $!; ' +++ local repos=/home/osstest/repos +++ '[' -z /home/osstest/repos ']' +++ '[' '!' -d /home/osstest/repos ']' +++ echo /home/osstest/repos ++ repos=/home/osstest/repos ++ repos_lock=/home/osstest/repos/lock ++ '[' x '!=' x/home/osstest/repos/lock ']' ++ OSSTEST_REPOS_LOCK_LOCKED=/home/osstest/repos/lock ++ exec with-lock-ex -w /home/osstest/repos/lock ./ap-push ovmf 1b3d5b0699cf6222698ed8373026813b61e87637 + branch=ovmf + revision=1b3d5b0699cf6222698ed8373026813b61e87637 + . ./cri-lock-repos ++ . ./cri-common +++ . ./cri-getconfig +++ umask 002 +++ getrepos getconfig Repos perl -e ' use Osstest; readglobalconfig(); print $c{"Repos"} or die $!; ' +++ local repos=/home/osstest/repos +++ '[' -z /home/osstest/repos ']' +++ '[' '!' -d /home/osstest/repos ']' +++ echo /home/osstest/repos ++ repos=/home/osstest/repos ++ repos_lock=/home/osstest/repos/lock ++ '[' x/home/osstest/repos/lock '!=' x/home/osstest/repos/lock ']' + . ./cri-common ++ . ./cri-getconfig ++ umask 002 + select_xenbranch + case "$branch" in + tree=ovmf + xenbranch=xen-unstable + '[' xovmf = xlinux ']' + linuxbranch= + '[' x = x ']' + qemuubranch=qemu-upstream-unstable + select_prevxenbranch ++ ./cri-getprevxenbranch xen-unstable + prevxenbranch=xen-4.8-testing + '[' x1b3d5b0699cf6222698ed8373026813b61e87637 = x ']' + : tested/2.6.39.x + . ./ap-common ++ : osst...@xenbits.xen.org +++ getconfig OsstestUpstream +++ perl -e ' use Osstest; readglobalconfig(); print $c{"OsstestUpstream"} or die $!; ' ++ : ++ : git://xenbits.xen.org/xen.git ++ : osst...@xenbits.xen.org:/home/xen/git/xen.git ++ : git://xenbits.xen.org/qemu-xen-traditional.git ++ : git://git.kernel.org ++ : git://git.kernel.org/pub/scm/linux/kernel/git ++ : git ++ : git://xenbits.xen.org/xtf.git ++ : osst...@xenbits.xen.org:/home/xen/git/xtf.git ++ : git://xenbits.xen.org/xtf.git ++ : git://xenbits.xen.org/libvirt.git ++ : osst...@xenbits.xen.org:/home/xen/git/libvirt.git ++ : git://xenbits.xen.org/libvirt.git ++ : git://xenbits.xen.org/osstest/rumprun.git ++ : git ++ : git://xenbits.xen.org/osstest/rumprun.git ++ : osst...@xenbits.xen.org:/home/xen/git/osstest/rumprun.git ++ : git://git.seabios.org/seabios.git ++ : osst...@xenbits.xen.org:/home/xen/git/osstest/seabios.git ++ : git://xenbits.xen.org/osstest/seabios.git ++ : https://github.com/tianocore/edk2.git ++ : osst...@xenbits.xen.org:/home/xen/git/osstest/ovmf.git ++ : git://xenbits.xen.org/osstest/ovmf.git ++ : git://xenbits.xen.org/osstest/linux-firmware.git ++ : osst...@xenbits.xen.org:/home/osstest/ext/linux-firmware.git ++ : git://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git ++ : osst...@xenbits.xen.org:/home/xen
Re: [Xen-devel] [PATCH v4 05/24] x86: refactor psr: implement Domain init/free and schedule flows.
On 17-01-10 06:34:50, Jan Beulich wrote: > >>> On 14.12.16 at 05:07, wrote: > > @@ -358,11 +366,32 @@ void psr_free_rmid(struct domain *d) > > d->arch.psr_rmid = 0; > > } > > > > +static inline unsigned int get_max_cos_max(const struct psr_socket_info > > *info) > > +{ > > +const struct feat_node *feat_tmp; > > Same remark as on the earlier patch regarding the _tmp suffix. > Should this reoccur in later patches, I won't give the same > comment again. > Thank you! I will take care all of them. > > +unsigned int cos_max = 0; > > + > > +list_for_each_entry(feat_tmp, &info->feat_list, list) > > +cos_max = max(feat_tmp->ops.get_max_cos_max(feat_tmp), cos_max); > > + > > +return cos_max; > > +} > > + > > static inline void psr_assoc_init(void) > > { > > struct psr_assoc *psra = &this_cpu(psr_assoc); > > > > -if ( psr_cmt_enabled() ) > > +if ( socket_info ) > > +{ > > +unsigned int socket = cpu_to_socket(smp_processor_id()); > > +const struct psr_socket_info *info = socket_info + socket; > > +unsigned int cos_max = get_max_cos_max(info); > > + > > +if ( info->feat_mask ) > > +psra->cos_mask = ((1ull << get_count_order(cos_max)) - 1) << > > 32; > > +} > > + > > +if ( psr_cmt_enabled() || psra->cos_mask ) > > rdmsrl(MSR_IA32_PSR_ASSOC, psra->val); > > } > > > > @@ -371,6 +400,12 @@ static inline void psr_assoc_rmid(uint64_t *reg, > > unsigned int rmid) > > *reg = (*reg & ~rmid_mask) | (rmid & rmid_mask); > > } > > > > +static inline void psr_assoc_cos(uint64_t *reg, unsigned int cos, > > + uint64_t cos_mask) > > +{ > > +*reg = (*reg & ~cos_mask) | (((uint64_t)cos << 32) & cos_mask); > > This recurring 32 would perhaps better become a #define, so they > can be identified as the same entity (at least I think they are). > Good suggestion, thanks! > Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 04/24] x86: refactor psr: implement CPU init and free flow.
On 17-01-10 04:45:05, Jan Beulich wrote: > >>> On 14.12.16 at 05:07, wrote: > > @@ -141,11 +144,79 @@ struct psr_assoc { > > > > struct psr_cmt *__read_mostly psr_cmt; > > > > +static struct psr_socket_info *__read_mostly socket_info; > > + > > static unsigned int opt_psr; > > static unsigned int __initdata opt_rmid_max = 255; > > +static unsigned int __read_mostly opt_cos_max = MAX_COS_REG_CNT; > > static uint64_t rmid_mask; > > static DEFINE_PER_CPU(struct psr_assoc, psr_assoc); > > > > +/* Declare feature list entry. */ > > +static struct feat_node *feat_l3_cat; > > Hmm, if you indeed (again) need such a helper object, then please > make the comment actually say so. As it is, the comment is mostly > meaningless. > Thanks! Will add more comments to explain it. > > +/* Common functions. */ > > +static void free_feature(struct psr_socket_info *info) > > +{ > > +struct feat_node *feat_tmp; > > + > > +if ( !info ) > > +return; > > + > > +list_for_each_entry(feat_tmp, &info->feat_list, list) > > +{ > > +clear_bit(feat_tmp->feature, &info->feat_mask); > > +list_del(&feat_tmp->list); > > +xfree(feat_tmp); > > +} > > This requires list_for_each_entry_safe() to be used, to avoid a > use-after-free issue (or alternatively a while(!list_empty()) loop). > Thanks for the suggestion! > > +/* Free feature which are not added into feat_list. */ > > +if ( feat_l3_cat ) > > +{ > > +xfree(feat_l3_cat); > > +feat_l3_cat = NULL; > > +} > > Why don't you leave this around, avoiding the need for an > allocation the next time a CPU comes online? Also note that xfree() > deals fine with a NULL input, so conditionals like this are pointless. > Thanks! Will keep them. > > +/* L3 CAT callback functions implementation. */ > > +static void l3_cat_init_feature(unsigned int eax, unsigned int ebx, > > +unsigned int ecx, unsigned int edx, > > This is rather unfortunate naming: How does the reader of this code > know what these values represent, without having to first go look in > the caller? > Do you mean the 'eax'-'edx'? How about 'eax_register'? > > +struct feat_node *feat, > > +struct psr_socket_info *info) > > +{ > > +struct psr_cat_hw_info l3_cat; > > +unsigned int socket; > > + > > +/* No valid value so do not enable feature. */ > > +if ( !eax || !edx ) > > +return; > > + > > +l3_cat.cbm_len = (eax & CAT_CBM_LEN_MASK) + 1; > > +l3_cat.cos_max = min(opt_cos_max, edx & CAT_COS_MAX_MASK); > > + > > +/* cos=0 is reserved as default cbm(all ones). */ > > +feat->cos_reg_val[0] = (1ull << l3_cat.cbm_len) - 1; > > Considering how cbm_len gets calculated a few lines up, I can't see > how this can end up being all ones (as the comment says). At most > this can be 0x (as a 64-bit value) afaics. > Sorry for the confusion. All one means all bits within cbm_len are 1. E.g. the cbm_len is 11. Then, value of cos_reg_val[0] is '(1 << 11) - 1', equals 0x7ff. Will correct the comment. > > +feat->feature = PSR_SOCKET_L3_CAT; > > +__set_bit(PSR_SOCKET_L3_CAT, &info->feat_mask); > > + > > +feat->info.l3_cat_info = l3_cat; > > + > > +info->nr_feat++; > > + > > +/* Add this feature into list. */ > > +list_add_tail(&feat->list, &info->feat_list); > > + > > +socket = cpu_to_socket(smp_processor_id()); > > +printk(XENLOG_INFO "L3 CAT: enabled on socket %u, cos_max:%u, > > cbm_len:%u\n", > > + socket, feat->info.l3_cat_info.cos_max, > > + feat->info.l3_cat_info.cbm_len); > > I don't think we want such printed for every socket, at least not by > default. Please, if you want to keep it, make it dependent upon e.g. > opt_cpu_info. > Thanks! Will limit the print by opt_cpu_info. > > +} > > + > > +struct feat_ops l3_cat_ops = { > > static const > Ok, thanks! > > @@ -340,18 +414,113 @@ void psr_domain_free(struct domain *d) > > psr_free_rmid(d); > > } > > > > -static int psr_cpu_prepare(unsigned int cpu) > > +static int cpu_prepare_work(unsigned int cpu) > > { > > +if ( !socket_info ) > > +return 0; > > + > > +/* Malloc memory for the global feature head here. */ > > +if ( feat_l3_cat == NULL && > > + (feat_l3_cat = xzalloc(struct feat_node)) == NULL ) > > +return -ENOMEM; > > + > > return 0; > > } > > > > +static void cpu_init_work(void) > > +{ > > +unsigned int eax, ebx, ecx, edx; > > +struct psr_socket_info *info; > > +unsigned int socket; > > +unsigned int cpu = smp_processor_id(); > > +const struct cpuinfo_x86 *c = cpu_data + cpu; > > Please use current_cpu_data instead of open coding it. > Thanks for the suggestion! > > +struct feat_node *feat_tmp; > > Looking at the uses, I don't think this is temporary in any way - why > not just "feat"? > No problem, thanks! > > +
Re: [Xen-devel] [PATCH] xen: do not re-use pirq number cached in pci device msi msg data
On Tue, 10 Jan 2017, Dan Streetman wrote: > On Tue, Jan 10, 2017 at 2:03 PM, Stefano Stabellini > wrote: > > On Tue, 10 Jan 2017, Dan Streetman wrote: > >> On Tue, Jan 10, 2017 at 10:57 AM, Dan Streetman wrote: > >> > On Mon, Jan 9, 2017 at 2:30 PM, Stefano Stabellini > >> > wrote: > >> >> On Mon, 9 Jan 2017, Konrad Rzeszutek Wilk wrote: > >> >>> On Mon, Jan 09, 2017 at 10:42:41AM -0500, Dan Streetman wrote: > >> >>> > On Mon, Jan 9, 2017 at 9:59 AM, Boris Ostrovsky > >> >>> > wrote: > >> >>> > > On 01/06/2017 08:06 PM, Konrad Rzeszutek Wilk wrote: > >> >>> > >> On Thu, Jan 05, 2017 at 02:28:56PM -0500, Dan Streetman wrote: > >> >>> > >>> Do not read a pci device's msi message data to see if a pirq was > >> >>> > >>> previously configured for the device's msi/msix, as the old pirq > >> >>> > >>> was > >> >>> > >>> unmapped and may now be in use by another pci device. The > >> >>> > >>> previous > >> >>> > >>> pirq should never be re-used; instead a new pirq should always be > >> >>> > >>> allocated from the hypervisor. > >> >>> > >> Won't this cause a starvation problem? That is we will run out of > >> >>> > >> PIRQs > >> >>> > >> as we are not reusing them? > >> >>> > > > >> >>> > > Don't we free the pirq when we unmap it? > >> >>> > > >> >>> > I think this is actually a bit worse than I initially thought. After > >> >>> > looking a bit closer, and I think there's an asymmetry with pirq > >> >>> > allocation: > >> >>> > >> >>> Lets include Stefano, > >> >>> > >> >>> Thank you for digging in this! This has quite the deja-vu > >> >>> feeling as I believe I hit this at some point in the past and > >> >>> posted some possible ways of fixing this. But sadly I can't > >> >>> find the thread. > >> >> > >> >> This issue seems to be caused by: > >> >> > >> >> commit af42b8d12f8adec6711cb824549a0edac6a4ae8f > >> >> Author: Stefano Stabellini > >> >> Date: Wed Dec 1 14:51:44 2010 + > >> >> > >> >> xen: fix MSI setup and teardown for PV on HVM guests > >> >> > >> >> which was a fix to a bug: > >> >> > >> >> This fixes a bug in xen_hvm_setup_msi_irqs that manifests itself > >> >> when > >> >> trying to enable the same MSI for the second time: the old MSI to > >> >> pirq > >> >> mapping is still valid at this point but xen_hvm_setup_msi_irqs > >> >> would > >> >> try to assign a new pirq anyway. > >> >> A simple way to reproduce this bug is to assign an MSI capable > >> >> network > >> >> card to a PV on HVM guest, if the user brings down the corresponding > >> >> ethernet interface and up again, Linux would fail to enable MSIs on > >> >> the > >> >> device. > >> >> > >> >> I don't remember any of the details. From the description of this bug, > >> >> it seems that Xen changed behavior in the past few years: before it used > >> >> to keep the pirq-MSI mapping, while today it doesn't. If I wrote "the > >> >> old MSI to pirq mapping is still valid at this point", the pirq couldn't > >> >> have been completely freed, then reassigned to somebody else the way it > >> >> is described in this email. > >> >> > >> >> I think we should indentify the changeset or Xen version that introduced > >> >> the new behavior. If it is old enough, we might be able to just revert > >> >> af42b8d12f8adec6711cb824549a0edac6a4ae8f. Otherwise we could make the > >> >> behavior conditional to the Xen version. > >> > > >> > Are PT devices the only MSI-capable devices available in a Xen guest? > >> > That's where I'm seeing this problem, with PT NVMe devices. > > > > They are the main ones. It is possible to have emulated virtio devices > > with emulated MSIs, for example virtio-net. Althought they are not in > > the Xen Project CI-loop, so I wouldn't be surprised if they are broken > > too. > > > > > >> > I can say that on the Xen guest with NVMe PT devices I'm testing on, > >> > with the patch from this thread (which essentially reverts your commit > >> > above) as well as some added debug to see the pirq numbers, cycles of > >> > 'modprobe nvme ; rmmod nvme' don't cause pirq starvation, as the > >> > hypervisor provides essentially the same pirqs for each modprobe, > >> > since they were freed by the rmmod. > > > > I am fine with reverting the old patch, but we need to understand what > > caused the change in behavior first. Maybe somebody else with a Xen PCI > > passthrough setup at hand can help with that. > > > > In the Xen code I can still see: > > > > case ECS_PIRQ: { > > struct pirq *pirq = pirq_info(d1, chn1->u.pirq.irq); > > > > if ( !pirq ) > > break; > > if ( !is_hvm_domain(d1) ) > > pirq_guest_unbind(d1, pirq); > > > > which means that pirq_guest_unbind should only be called on evtchn_close > > if the guest is not an HVM guest. > > I tried an experiment to call get_free_pirq on both sides of a > evtchn_close hcall, using two SRIOV nics. When I rmmod/modprobe, I > see something interesting; each nic uses 3 MSIs, and it looks l
[Xen-devel] [qemu-mainline test] 104100: tolerable FAIL - PUSHED
flight 104100 qemu-mainline real [real] http://logs.test-lab.xenproject.org/osstest/logs/104100/ Failures :-/ but no regressions. Regressions which are regarded as allowable (not blocking): test-armhf-armhf-libvirt-xsm 13 saverestore-support-checkfail like 104098 test-armhf-armhf-libvirt 13 saverestore-support-checkfail like 104098 test-amd64-amd64-xl-qemuu-win7-amd64 16 guest-stopfail like 104098 test-amd64-i386-xl-qemuu-win7-amd64 16 guest-stop fail like 104098 test-armhf-armhf-libvirt-qcow2 12 saverestore-support-check fail like 104098 test-armhf-armhf-libvirt-raw 12 saverestore-support-checkfail like 104098 test-armhf-armhf-xl-rtds 15 guest-start/debian.repeatfail like 104098 test-amd64-amd64-xl-rtds 9 debian-install fail like 104098 Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-pvh-amd 11 guest-start fail never pass test-amd64-amd64-xl-pvh-intel 11 guest-start fail never pass test-amd64-amd64-libvirt 12 migrate-support-checkfail never pass test-amd64-i386-libvirt 12 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 12 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 12 migrate-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check fail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check fail never pass test-amd64-amd64-libvirt-vhd 11 migrate-support-checkfail never pass test-amd64-amd64-qemuu-nested-amd 16 debian-hvm-install/l1/l2 fail never pass test-armhf-armhf-xl-credit2 12 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-xsm 12 migrate-support-checkfail never pass test-armhf-armhf-xl-xsm 13 saverestore-support-checkfail never pass test-armhf-armhf-xl 12 migrate-support-checkfail never pass test-armhf-armhf-xl 13 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-xsm 12 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 12 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 13 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 12 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 12 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 13 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 11 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 11 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 12 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 12 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-vhd 11 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 12 saverestore-support-checkfail never pass version targeted for testing: qemuu41a0e54756a9ae6b60be34bb33302a7e085fdb07 baseline version: qemuuf634151b02ce5c80605383894f1f63f2c12e0033 Last test of basis 104098 2017-01-10 11:11:00 Z0 days Testing same since 104100 2017-01-10 16:45:31 Z0 days1 attempts People who touched revisions under test: Cao jin Dmitry Fleytman Dou Liyang Dr. David Alan Gilbert Gonglei Halil Pasic Igor Mammedov Jason Wang Li Qiang Maxime Coquelin Michael S. Tsirkin Paolo Bonzini Peter Maydell Peter Xu Yuri Benditovich jobs: build-amd64-xsm pass build-armhf-xsm pass build-i386-xsm pass build-amd64 pass build-armhf pass build-i386 pass build-amd64-libvirt pass build-armhf-libvirt pass build-i386-libvirt pass build-amd64-pvopspass build-armhf-pvopspass build-i386-pvops pass test-amd64-amd64-xl pass test-armhf-armhf-xl pass test-amd64-i386-xl
[Xen-devel] [xen-unstable baseline-only test] 68352: regressions - trouble: blocked/broken/fail/pass
This run is configured for baseline tests only. flight 68352 xen-unstable real [real] http://osstest.xs.citrite.net/~osstest/testlogs/logs/68352/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-amd64-i386-xl-qemuu-ovmf-amd64 3 host-install(3) broken REGR. vs. 68338 test-amd64-amd64-qemuu-nested-intel 3 host-install(3) broken REGR. vs. 68338 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 3 host-install(3) broken REGR. vs. 68338 test-amd64-amd64-xl-xsm 6 xen-boot fail REGR. vs. 68338 test-amd64-amd64-xl-qemuu-debianhvm-amd64 15 guest-localmigrate/x10 fail REGR. vs. 68338 Regressions which are regarded as allowable (not blocking): test-amd64-i386-xl-qemuu-win7-amd64 16 guest-stop fail like 68338 test-amd64-i386-xl-qemuu-winxpsp3-vcpus1 9 windows-installfail like 68338 test-amd64-i386-xl-qemut-winxpsp3-vcpus1 9 windows-installfail like 68338 Tests which did not succeed, but are not blocking: test-amd64-amd64-rumprun-amd64 1 build-check(1) blocked n/a test-amd64-i386-rumprun-i386 1 build-check(1) blocked n/a build-i386-rumprun6 xen-buildfail never pass build-amd64-rumprun 6 xen-buildfail never pass test-armhf-armhf-xl-xsm 12 migrate-support-checkfail never pass test-armhf-armhf-xl-xsm 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-midway 12 migrate-support-checkfail never pass test-armhf-armhf-xl-midway 13 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-xsm 12 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 12 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 13 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 12 migrate-support-checkfail never pass test-armhf-armhf-libvirt-xsm 14 guest-saverestorefail never pass test-armhf-armhf-libvirt 14 guest-saverestorefail never pass test-armhf-armhf-xl-multivcpu 12 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 13 saverestore-support-checkfail never pass test-amd64-amd64-xl-pvh-amd 11 guest-start fail never pass test-amd64-amd64-xl-pvh-intel 11 guest-start fail never pass test-armhf-armhf-xl-rtds 12 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 13 saverestore-support-checkfail never pass test-amd64-amd64-libvirt 12 migrate-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 11 migrate-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 13 guest-saverestorefail never pass test-armhf-armhf-xl 12 migrate-support-checkfail never pass test-armhf-armhf-xl 13 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-raw 11 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 13 guest-saverestorefail never pass test-amd64-amd64-libvirt-xsm 12 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 12 migrate-support-checkfail never pass test-amd64-i386-libvirt 12 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 11 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 12 saverestore-support-checkfail never pass test-amd64-amd64-qemuu-nested-amd 16 debian-hvm-install/l1/l2 fail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check fail never pass test-amd64-amd64-xl-qemut-win7-amd64 16 guest-stop fail never pass test-amd64-amd64-libvirt-vhd 11 migrate-support-checkfail never pass test-amd64-i386-xl-qemut-win7-amd64 16 guest-stop fail never pass test-amd64-amd64-xl-qemuu-win7-amd64 16 guest-stop fail never pass version targeted for testing: xen dc562334db2b1fc232dda884f84bb0172e1d1480 baseline version: xen 9009ea4706b10798878ab8dfe34e40dbf315c339 Last test of basis68338 2017-01-07 11:19:04 Z3 days Testing same since68352 2017-01-10 12:48:35 Z0 days1 attempts People who touched revisions under test: Cédric Bosdonnat Wei Liu jobs: build-amd64-xsm pass build-armhf-xsm pass build-i386-xsm pass build-amd64-xtf pass build-amd64 pass build-armhf pass build-i386
Re: [Xen-devel] [PATCH 2/2] xen/kbdif: add multi-touch support
On Tue, 10 Jan 2017, Oleksandr Andrushchenko wrote: > On 01/07/2017 12:37 AM, Stefano Stabellini wrote: > > On Fri, 6 Jan 2017, Oleksandr Andrushchenko wrote: > > > From: Oleksandr Andrushchenko > > > > > > Signed-off-by: Oleksandr Andrushchenko > > > --- > > > xen/include/public/io/kbdif.h | 228 > > > ++ > > > 1 file changed, 228 insertions(+) > > > > > > diff --git a/xen/include/public/io/kbdif.h b/xen/include/public/io/kbdif.h > > > index 0e19a40..1b446f9 100644 > > > --- a/xen/include/public/io/kbdif.h > > > +++ b/xen/include/public/io/kbdif.h > > > @@ -57,6 +57,12 @@ > > >* Backends, which support reporting of absolute coordinates for > > > pointer > > >* device should set this to 1. > > >* > > > + * feature-multi-touch > > > + * Values: > > > + * > > > + * Backends, which support reporting of multi-touch events > > > + * should set this to 1. > > > + * > > >*- Pointer Device Parameters > > > > > >* > > >* width > > > @@ -87,6 +93,11 @@ > > >* Request from backend to report absolute pointer coordinates > > >* (XENKBD_TYPE_POS) instead of relative ones (XENKBD_TYPE_MOTION). > > >* > > > + * request-multi-touch > > > + * Values: > > > + * > > > + * Request from backend to report multi-touch events. > > I think in English should be "request backend to report multi-touch > > events". Same above. > Done > > > > > + * > > >*--- Request Transport Parameters > > > --- > > >* > > >* event-channel > > > @@ -107,6 +118,43 @@ > > >* OBSOLETE, not recommended for use. > > >* A unique (within the guest domain given) value identifying event > > >* channel and page reference pair. > > > + * > > > + *--- Multi-touch Device Parameters > > > --- > > > + * > > > + * Every multi-touch input device uses a dedicated event ring and is > > For clarity I would say "Every multi-touch input device uses a dedicated > > frontend/backend connection". That includes a ring, an event channel, > > and xenstore entries. > > > Done > > > + * configured via XenStore properties under XENKBD_PATH_MTOUCH folder. > > I don't understand why we need a new xenstore folder. Why do we need > > XENKBD_PATH_MTOUCH? > This is just another (IMO, cleaner) approach to define > properties in XenStore for multiple instances. > Let's see what vif does on my PC: > /local/domain/11/device/vif/0/queue-0/tx-ring-ref = "2304" > ... > /local/domain/11/device/vif/0/queue-1/tx-ring-ref = "2306" > ... > /local/domain/11/device/vif/0/queue-2/tx-ring-ref = "2308" > ... > /local/domain/11/device/vif/0/queue-3/tx-ring-ref = "2310" > ... > /local/domain/11/device/vif/0/request-rx-copy = "1" > > We have folders "queue-%d" per queue which in my case are > > under XENKBD_PATH_MTOUCH. > > /local/domain/11/device/vkbd/0/mtouch/0/width = "3200" > /local/domain/11/device/vkbd/0/mtouch/0/height = "3200" > /local/domain/11/device/vkbd/0/mtouch/0/num-contacts = "5" > /local/domain/11/device/vkbd/0/mtouch/1/width = "6400" > /local/domain/11/device/vkbd/0/mtouch/1/height = "6400" > /local/domain/11/device/vkbd/0/mtouch/1/num-contacts = "10" > > Namely, instead of "mtouch-%d" I use "mtouch/%d/. > This is just like using "/local/domain/%d" but not > "/local/domain-%d", so "mtouch/%d" seem to be more > consistent. I agree that mtouch/%d is better than mtouch-%d, but it's only necessary if we have more than one mtouch per vkbd. I would configure it so that one can only have one mtouch per vkbd: /local/domain/11/device/vkbd/0/mtouch/width = "3200" /local/domain/11/device/vkbd/0/mtouch/height = "3200" /local/domain/11/device/vkbd/1/mtouch/width = "6400" /local/domain/11/device/vkbd/1/mtouch/height = "6400" This is also what I was referring to when I wrote: > It is useful to distinguish mouse events from keyboard events, from > multitouch events more easily, but I don't think we should support > multiple keyboards or multiple mice on the same xenkbd ring. If we need > two mice, we can setup two xenkdb rings. The same applies to multitouch devices, I think. But maybe you have good reasons for supporting multiple multitouch devices on a single vkbd connection? How many do you expect to have on a single VM? ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] Xen ARM - Exposing a PL011 to the guest
On Tue, 10 Jan 2017, Bhupinder Thakur wrote: > Hi Stefano, > > > > On 7 January 2017 at 03:24, Stefano Stabellini wrote: > > Hello Bhupinder, > > > > it is good to hear that you are making progress. > > > > > > On Fri, 6 Jan 2017, Bhupinder Thakur wrote: > >> Hi, > >> > >> I am trying to allocate a new SPI VIRQ for sending pl011 interrupt to > >> the guest OS. Currently Xen does not allow to allocate a SPI VIRQ for > >> a guest domain. I tried allocating a new SPI VIRQ by calling > >> vgic_allocate_spi() but it failed as the SPI VIRQ range for the guests > >> is limited to 0 (vgic_num_irqs() return 32 and therefore in effect > >> says that no SPI VIRQ can be allocated because below 32 are for > >> PPI/SGI VIRQs). > > > > Yes, you should use vgic_allocate_virq. For that to work, you need to > > make sure that nr_spis for the domain is > 0. Given that nr_spis is > > passed from libxl by calling xc_domain_create, I would set nr_spis to 1 > > in libxl__arch_domain_prepare_config. > > > I increment the nr_spis by 1 in domain_vgic_init() to ensure the > vgic_allocate_virq() is able to allocate a SPI for pl011. With that > change, I am able to inject a SPI irq into guest and handle it. Great! ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [DOC v7] PV Calls protocol design
Upon request from Konrad, I am attaching the output of pahole on the C structs defined by PVCalls. As you can see, alignments and sizes of all fields are the same, except for the padding at the end of many request structs (they need to be multiple of 8 bytes on 64bit). However, struct xen_pvcalls_dummy dummy ensures that the overall size of struct xen_pvcalls_request is the same on both 32 and 64. On Thu, 13 Oct 2016, Stefano Stabellini wrote: > Hi all, > > This is the design document of the PV Calls protocol. You can find > prototypes of the Linux frontend and backend drivers here: > > git://git.kernel.org/pub/scm/linux/kernel/git/sstabellini/xen.git pvcalls-7 > > To use them, make sure to enable CONFIG_XEN_PVCALLS in your kernel > config and add "pvcalls=1" to the command line of your DomU Linux > kernel. You also need the toolstack to create the initial xenstore nodes > for the protocol. To do that, please apply the attached patch to libxl > (the patch is based on Xen 4.7.0-rc3) and add "pvcalls=1" to your DomU > config file. > > Please review! > > Cheers, > > Stefano > > > Changes in v7: > - add a glossary of Xen terms > - add a paragraph on why Xen was chosen > - wording improvements > - add links to xenstore documents and headers > - specify that the current version is "1" > - rename max-dataring-page-order to max-page-order > - rename networking-calls to function-calls > - add links to [Data ring] throughout the document > - explain the difference between req_id and id > - mention that future commands larger than 56 bytes will require a > version increase > - mention that the list of commands is in calling order > - clarify that reuse data rings are found by ref > - rename ENOTSUPP to ENOTSUP > - add padding in struct pvcalls_data_intf for cachelining > - rename pvcalls_ring_queued to pvcalls_ring_unconsumed > > > Changes in v6: > - add reuse field in release command > - add "networking-calls" backend node on xenstore > - fixed tab/whitespace indentation > > Changes in v5: > - clarify text > - rename id to req_id > - rename sockid to id > - move id to request and response specific fields > - add version node to xenstore > > Changes in v4: > - rename xensock to pvcalls > > Changes in v3: > - add a dummy element to struct xen_xensock_request to make sure the > size of the struct is the same on both x86_32 and x86_64 > > Changes in v2: > - add max-dataring-page-order > - add "Publish backend features and transport parameters" to backend > xenbus workflow > - update new cmd values > - update xen_xensock_request > - add backlog parameter to listen and binary layout > - add description of new data ring format (interface+data) > - modify connect and accept to reflect new data ring format > - add link to POSIX docs > - add error numbers > - add address format section and relevant numeric definitions > - add explicit mention of unimplemented commands > - add protocol node name > - add xenbus shutdown diagram > - add socket operation > > --- > > > # PV Calls Protocol version 1 > > ## Glossary > > The following is a list of terms and definitions used in the Xen > community. If you are a Xen contributor you can skip this section. > > * PV > > Short for paravirtualized. > > * Dom0 > > First virtual machine that boots. In most configurations Dom0 is > privileged and has control over hardware devices, such as network > cards, graphic cards, etc. > > * DomU > > Regular unprivileged Xen virtual machine. > > * Domain > > A Xen virtual machine. Dom0 and all DomUs are all separate Xen > domains. > > * Guest > > Same as domain: a Xen virtual machine. > > * Frontend > > Each DomU has one or more paravirtualized frontend drivers to access > disks, network, console, graphics, etc. The presence of PV devices is > advertized on XenStore, a cross domain key-value database. Frontends > are similar in intent to the virtio drivers in Linux. > > * Backend > > A Xen paravirtualized backend typically runs in Dom0 and it is used to > export disks, network, console, graphics, etcs, to DomUs. Backends can > live both in kernel space and in userspace. For example xen-blkback > lives under drivers/block in the Linux kernel and xen_disk lives under > hw/block in QEMU. Paravirtualized backends are similar in intent to > virtio device emulators. > > * VMX and SVM > > On Intel processors, VMX is the CPU flag for VT-x, hardware > virtualization support. It corresponds to SVM on AMD processors. > > > > ## Rationale > > PV Calls is a paravirtualized protocol that allows the implementation of > a set of POSIX functions in a different domain. The PV Calls frontend > sends POSIX function calls to the backend, which implements them and > returns a value to the frontend. > > This version of the document covers networking function calls, such as > connect, accept, bind, release, listen, poll, recvmsg and sendmsg; but > the protocol is meant to be easily extend
[Xen-devel] [ovmf test] 104103: all pass - PUSHED
flight 104103 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/104103/ Perfect :-) All tests in this flight passed as required version targeted for testing: ovmf f4d575b51bbdb61e9f634dcabb84149ca836cffa baseline version: ovmf 462a3eba8feaf3716d294111725742ff5aa08488 Last test of basis 104102 2017-01-10 20:44:52 Z0 days Testing same since 104103 2017-01-10 22:14:23 Z0 days1 attempts People who touched revisions under test: Michael Kinney 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 : + branch=ovmf + revision=f4d575b51bbdb61e9f634dcabb84149ca836cffa + . ./cri-lock-repos ++ . ./cri-common +++ . ./cri-getconfig +++ umask 002 +++ getrepos getconfig Repos perl -e ' use Osstest; readglobalconfig(); print $c{"Repos"} or die $!; ' +++ local repos=/home/osstest/repos +++ '[' -z /home/osstest/repos ']' +++ '[' '!' -d /home/osstest/repos ']' +++ echo /home/osstest/repos ++ repos=/home/osstest/repos ++ repos_lock=/home/osstest/repos/lock ++ '[' x '!=' x/home/osstest/repos/lock ']' ++ OSSTEST_REPOS_LOCK_LOCKED=/home/osstest/repos/lock ++ exec with-lock-ex -w /home/osstest/repos/lock ./ap-push ovmf f4d575b51bbdb61e9f634dcabb84149ca836cffa + branch=ovmf + revision=f4d575b51bbdb61e9f634dcabb84149ca836cffa + . ./cri-lock-repos ++ . ./cri-common +++ . ./cri-getconfig +++ umask 002 +++ getrepos getconfig Repos perl -e ' use Osstest; readglobalconfig(); print $c{"Repos"} or die $!; ' +++ local repos=/home/osstest/repos +++ '[' -z /home/osstest/repos ']' +++ '[' '!' -d /home/osstest/repos ']' +++ echo /home/osstest/repos ++ repos=/home/osstest/repos ++ repos_lock=/home/osstest/repos/lock ++ '[' x/home/osstest/repos/lock '!=' x/home/osstest/repos/lock ']' + . ./cri-common ++ . ./cri-getconfig ++ umask 002 + select_xenbranch + case "$branch" in + tree=ovmf + xenbranch=xen-unstable + '[' xovmf = xlinux ']' + linuxbranch= + '[' x = x ']' + qemuubranch=qemu-upstream-unstable + select_prevxenbranch ++ ./cri-getprevxenbranch xen-unstable + prevxenbranch=xen-4.8-testing + '[' xf4d575b51bbdb61e9f634dcabb84149ca836cffa = x ']' + : tested/2.6.39.x + . ./ap-common ++ : osst...@xenbits.xen.org +++ getconfig OsstestUpstream +++ perl -e ' use Osstest; readglobalconfig(); print $c{"OsstestUpstream"} or die $!; ' ++ : ++ : git://xenbits.xen.org/xen.git ++ : osst...@xenbits.xen.org:/home/xen/git/xen.git ++ : git://xenbits.xen.org/qemu-xen-traditional.git ++ : git://git.kernel.org ++ : git://git.kernel.org/pub/scm/linux/kernel/git ++ : git ++ : git://xenbits.xen.org/xtf.git ++ : osst...@xenbits.xen.org:/home/xen/git/xtf.git ++ : git://xenbits.xen.org/xtf.git ++ : git://xenbits.xen.org/libvirt.git ++ : osst...@xenbits.xen.org:/home/xen/git/libvirt.git ++ : git://xenbits.xen.org/libvirt.git ++ : git://xenbits.xen.org/osstest/rumprun.git ++ : git ++ : git://xenbits.xen.org/osstest/rumprun.git ++ : osst...@xenbits.xen.org:/home/xen/git/osstest/rumprun.git ++ : git://git.seabios.org/seabios.git ++ : osst...@xenbits.xen.org:/home/xen/git/osstest/seabios.git ++ : git://xenbits.xen.org/osstest/seabios.git ++ : https://github.com/tianocore/edk2.git ++ : osst...@xenbits.xen.org:/home/xen/git/osstest/ovmf.git ++ : git://xenbits.xen.org/osstest/ovmf.git ++ : git://xenbits.xen.org/osstest/linux-firmware.git ++ : osst...@xenbits.xen.org:/home/osstest/ext/linux-firmware.git ++ : git://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git ++ : osst...@xenbits.xen.org:/home/xen/git/linux-pvo
Re: [Xen-devel] [PATCH] xen: do not re-use pirq number cached in pci device msi msg data
>> In the Xen code I can still see: >> >> case ECS_PIRQ: { >> struct pirq *pirq = pirq_info(d1, chn1->u.pirq.irq); >> >> if ( !pirq ) >> break; >> if ( !is_hvm_domain(d1) ) >> pirq_guest_unbind(d1, pirq); >> >> which means that pirq_guest_unbind should only be called on evtchn_close >> if the guest is not an HVM guest. A few lines further down we have: pirq->evtchn = 0; pirq_cleanup_check(pirq, d1); unlink_pirq_port(chn1, d1->vcpu[chn1->notify_vcpu_id]); #ifdef CONFIG_X86 if ( is_hvm_domain(d1) && domain_pirq_to_irq(d1, pirq->pirq) > 0 ) unmap_domain_pirq_emuirq(d1, pirq->pirq); #endif which is where we free up the pirq. -boris ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [xen-unstable test] 104099: tolerable FAIL - PUSHED
flight 104099 xen-unstable real [real] http://logs.test-lab.xenproject.org/osstest/logs/104099/ Failures :-/ but no regressions. Regressions which are regarded as allowable (not blocking): test-armhf-armhf-libvirt 13 saverestore-support-checkfail like 104094 test-armhf-armhf-libvirt-xsm 13 saverestore-support-checkfail like 104094 test-amd64-i386-xl-qemuu-win7-amd64 16 guest-stop fail like 104094 test-amd64-i386-xl-qemut-win7-amd64 16 guest-stop fail like 104094 test-amd64-amd64-xl-qemut-win7-amd64 16 guest-stopfail like 104094 test-amd64-amd64-xl-qemuu-win7-amd64 16 guest-stopfail like 104094 test-armhf-armhf-libvirt-qcow2 12 saverestore-support-check fail like 104094 test-armhf-armhf-libvirt-raw 12 saverestore-support-checkfail like 104094 test-amd64-amd64-xl-rtds 9 debian-install fail like 104094 Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-pvh-amd 11 guest-start fail never pass test-amd64-amd64-xl-pvh-intel 11 guest-start fail never pass test-amd64-amd64-libvirt-xsm 12 migrate-support-checkfail never pass test-amd64-i386-libvirt 12 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 12 migrate-support-checkfail never pass test-amd64-amd64-libvirt 12 migrate-support-checkfail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check fail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check fail never pass test-amd64-amd64-libvirt-vhd 11 migrate-support-checkfail never pass test-amd64-amd64-qemuu-nested-amd 16 debian-hvm-install/l1/l2 fail never pass test-armhf-armhf-xl 12 migrate-support-checkfail never pass test-armhf-armhf-xl 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-xsm 12 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 12 migrate-support-checkfail never pass test-armhf-armhf-xl-xsm 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-cubietruck 12 migrate-support-checkfail never pass test-armhf-armhf-libvirt 12 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-cubietruck 13 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-xsm 12 migrate-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 11 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 11 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 12 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-vhd 11 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 12 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 12 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-arndale 12 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 13 saverestore-support-checkfail never pass version targeted for testing: xen 9d81162fd2c69b5c9279670c8739d891cf7571da baseline version: xen dc562334db2b1fc232dda884f84bb0172e1d1480 Last test of basis 104094 2017-01-10 05:16:04 Z0 days Testing same since 104099 2017-01-10 13:14:57 Z0 days1 attempts People who touched revisions under test: Anthony PERARD Doug Goldstein Eric DeVolder He Chen Ian Jackson Jan Beulich Juergen Gross Wei Liu jobs: build-amd64-xsm pass build-armhf-xsm pass build-i386-xsm pass build-amd64-xtf pass build-amd64 pass build-armhf pass build-i386 pass build-amd64-libvirt pass build-armhf-libvirt pass build-i386-libvirt pass build-amd64-oldkern pass build-i386-oldkern pass build-amd64-prev pass build-i386-prev pass build-amd64-pvopspass
[Xen-devel] [PATCH 2/??] memory allocation fix
Signed-off-by: Doug Goldstein --- Daniel, Feel free to fold this in where it needs to go or break this up into a patch (or patches) part of your series. This is what it took for me to get it to boot on one of my Intel NUCs and to get into the dom0 kernel booting on my other machines. There's still an issue with the page tables somewhere I believe because that's where its dying on the other machines. Writing to cr3 or cr0. QEMU is likely an issue with OVMF or what they're doing. 0x7fb27408 is smack in the middle of ACPI NVS. (XEN) [5.506811][<7fb27408>] 7fb27408 (XEN) [5.509252][] vsnprintf+0x7eb/0xb70 (XEN) [5.511779][] i387.c#_vcpu_save_fpu+0x8a/0x180 (XEN) [5.514548][] efi_runtime_call+0xa24/0xac0 (XEN) [5.517233][] efi_runtime_call+0x8be/0xac0 (XEN) [5.519853][] avc_has_perm+0x51/0x70 (XEN) [5.521985][] do_platform_op+0x73f/0x16e0 (XEN) [5.524604][] do_platform_op+0x73f/0x16e0 (XEN) [5.527172][] do_mmu_update+0x221/0x12b0 (XEN) [5.531455][] do_platform_op+0/0x16e0 (XEN) [5.533421][] pv_hypercall+0xf6/0x1c0 (XEN) [5.537808][] timer.c#timer_softirq_action+0xc0/0x250 (XEN) [5.543847][] timer.c#timer_softirq_action+0x132/0x250 (XEN) [5.546743][] entry.o#test_all_events+0/0x2a This happens right after dom0 spits out: [0.368037] PTP clock support registered As an aside given all the Intel guys CC'd, what's a communication channel I can use to report issues with the NUC's EFI implementation that's not canned responses from level 1 tech support? --- xen/arch/x86/efi/efi-boot.h | 10 +- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h index dc857d8..878f683 100644 --- a/xen/arch/x86/efi/efi-boot.h +++ b/xen/arch/x86/efi/efi-boot.h @@ -172,7 +172,7 @@ static void __init efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable, if ( !trampoline_phys && desc->PhysicalStart + len <= 0x10 && len >= cfg.size + extra_mem && desc->PhysicalStart + len > cfg.addr ) -cfg.addr = (desc->PhysicalStart + len - cfg.size) & PAGE_MASK; +cfg.addr = (desc->PhysicalStart + len - (cfg.size + extra_mem)) & PAGE_MASK; /* fall through */ case EfiLoaderCode: case EfiLoaderData: @@ -686,6 +686,14 @@ paddr_t __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTa setup_efi_pci(); efi_variables(); +cfg.addr = 0x10; +cfg.size = (trampoline_end - trampoline_start) + (64 << 10); +if ( efi_bs->AllocatePages(AllocateMaxAddress, EfiLoaderData, +PFN_UP(cfg.size), &cfg.addr) != EFI_SUCCESS ) { +cfg.addr = 0; +PrintStr(L"Trampoline space cannot be allocated; will try fallback.\r\n"); +} + if ( gop ) efi_set_gop_mode(gop, gop_mode); -- 2.10.2 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 3/3] xen: optimize xenbus driver for multiple concurrent xenstore accesses
> diff --git a/drivers/xen/xenbus/xenbus_xs.c b/drivers/xen/xenbus/xenbus_xs.c > index ebc768f..ebdfbee 100644 > --- a/drivers/xen/xenbus/xenbus_xs.c > +++ b/drivers/xen/xenbus/xenbus_xs.c > - > -static struct xs_handle xs_state; > +/* > + * Framework to protect suspend/resume handling against normal Xenstore > + * message handling: > + * During suspend/resume there must be no open transaction and no pending > + * Xenstore request. > + * New watch events happening in this time can be ignored by firing all > watches > + * after resume. > + */ > +/* Lock protecting enter/exit critical region. */ > +static DEFINE_SPINLOCK(xs_state_lock); > +/* Wait queue for all callers waiting for critical region to become usable. > */ > +static DECLARE_WAIT_QUEUE_HEAD(xs_state_enter_wq); > +/* Wait queue for suspend handling waiting for critical region being empty. > */ > +static DECLARE_WAIT_QUEUE_HEAD(xs_state_exit_wq); > +/* Number of users in critical region. */ > +static unsigned int xs_state_users; > +/* Suspend handler waiting or already active? */ > +static int xs_suspend_active; I think these two should be declared next to xs_state _lock since they are protected by it. Or maybe even put them into some sort of a state struct. > + > + > +static bool test_reply(struct xb_req_data *req) > +{ > + if (req->state == xb_req_state_got_reply || !xenbus_ok()) > + return true; > + > + /* Make sure to reread req->state each time. */ > + cpu_relax(); I don't think I understand why this is needed. > + > + return false; > +} > + > +static void xs_send(struct xb_req_data *req, struct xsd_sockmsg *msg) > { > - mutex_lock(&xs_state.transaction_mutex); > - atomic_inc(&xs_state.transaction_count); > - mutex_unlock(&xs_state.transaction_mutex); > -} > + bool notify; > > -static void transaction_end(void) > -{ > - if (atomic_dec_and_test(&xs_state.transaction_count)) > - wake_up(&xs_state.transaction_wq); > -} > + req->msg = *msg; > + req->err = 0; > + req->state = xb_req_state_queued; > + init_waitqueue_head(&req->wq); > > -static void transaction_suspend(void) > -{ > - mutex_lock(&xs_state.transaction_mutex); > - wait_event(xs_state.transaction_wq, > -atomic_read(&xs_state.transaction_count) == 0); > -} > + xs_request_enter(req); > > -static void transaction_resume(void) > -{ > - mutex_unlock(&xs_state.transaction_mutex); > + req->msg.req_id = xs_request_id++; Is it safe to do this without a lock? > + > +int xenbus_dev_request_and_reply(struct xsd_sockmsg *msg, void *par) > +{ > + struct xb_req_data *req; > + struct kvec *vec; > + > + req = kmalloc(sizeof(*req) + sizeof(*vec), GFP_KERNEL); Is there a reason why you are using different flags here? > @@ -263,11 +295,20 @@ static void *xs_talkv(struct xenbus_transaction t, > unsigned int num_vecs, > unsigned int *len) > { > + struct xb_req_data *req; > struct xsd_sockmsg msg; > void *ret = NULL; > unsigned int i; > int err; > > + req = kmalloc(sizeof(*req), GFP_NOIO | __GFP_HIGH); > + if (!req) > + return ERR_PTR(-ENOMEM); > + > + req->vec = iovec; > + req->num_vecs = num_vecs; > + req->cb = xs_wake_up; > + > msg.tx_id = t.id; > msg.req_id = 0; Is this still needed? You are assigning it in xs_send(). > +static int xs_reboot_notify(struct notifier_block *nb, > + unsigned long code, void *unused) > { > - struct xs_stored_msg *msg; > + struct xb_req_data *req; > + > + mutex_lock(&xb_write_mutex); > + list_for_each_entry(req, &xs_reply_list, list) > + wake_up(&req->wq); > + list_for_each_entry(req, &xb_write_list, list) > + wake_up(&req->wq); We are waking up waiters here but there is not guarantee that waiting threads will have a chance to run, is there? -boris ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [ovmf test] 104102: all pass - PUSHED
flight 104102 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/104102/ Perfect :-) All tests in this flight passed as required version targeted for testing: ovmf 462a3eba8feaf3716d294111725742ff5aa08488 baseline version: ovmf 363dc42226a1d8ae02c73f9dd81da65af91b5fdd Last test of basis 104096 2017-01-10 09:45:43 Z0 days Testing same since 104102 2017-01-10 20:44:52 Z0 days1 attempts People who touched revisions under test: Michael D Kinney Michael Kinney 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 : + branch=ovmf + revision=462a3eba8feaf3716d294111725742ff5aa08488 + . ./cri-lock-repos ++ . ./cri-common +++ . ./cri-getconfig +++ umask 002 +++ getrepos getconfig Repos perl -e ' use Osstest; readglobalconfig(); print $c{"Repos"} or die $!; ' +++ local repos=/home/osstest/repos +++ '[' -z /home/osstest/repos ']' +++ '[' '!' -d /home/osstest/repos ']' +++ echo /home/osstest/repos ++ repos=/home/osstest/repos ++ repos_lock=/home/osstest/repos/lock ++ '[' x '!=' x/home/osstest/repos/lock ']' ++ OSSTEST_REPOS_LOCK_LOCKED=/home/osstest/repos/lock ++ exec with-lock-ex -w /home/osstest/repos/lock ./ap-push ovmf 462a3eba8feaf3716d294111725742ff5aa08488 + branch=ovmf + revision=462a3eba8feaf3716d294111725742ff5aa08488 + . ./cri-lock-repos ++ . ./cri-common +++ . ./cri-getconfig +++ umask 002 +++ getrepos getconfig Repos perl -e ' use Osstest; readglobalconfig(); print $c{"Repos"} or die $!; ' +++ local repos=/home/osstest/repos +++ '[' -z /home/osstest/repos ']' +++ '[' '!' -d /home/osstest/repos ']' +++ echo /home/osstest/repos ++ repos=/home/osstest/repos ++ repos_lock=/home/osstest/repos/lock ++ '[' x/home/osstest/repos/lock '!=' x/home/osstest/repos/lock ']' + . ./cri-common ++ . ./cri-getconfig ++ umask 002 + select_xenbranch + case "$branch" in + tree=ovmf + xenbranch=xen-unstable + '[' xovmf = xlinux ']' + linuxbranch= + '[' x = x ']' + qemuubranch=qemu-upstream-unstable + select_prevxenbranch ++ ./cri-getprevxenbranch xen-unstable + prevxenbranch=xen-4.8-testing + '[' x462a3eba8feaf3716d294111725742ff5aa08488 = x ']' + : tested/2.6.39.x + . ./ap-common ++ : osst...@xenbits.xen.org +++ getconfig OsstestUpstream +++ perl -e ' use Osstest; readglobalconfig(); print $c{"OsstestUpstream"} or die $!; ' ++ : ++ : git://xenbits.xen.org/xen.git ++ : osst...@xenbits.xen.org:/home/xen/git/xen.git ++ : git://xenbits.xen.org/qemu-xen-traditional.git ++ : git://git.kernel.org ++ : git://git.kernel.org/pub/scm/linux/kernel/git ++ : git ++ : git://xenbits.xen.org/xtf.git ++ : osst...@xenbits.xen.org:/home/xen/git/xtf.git ++ : git://xenbits.xen.org/xtf.git ++ : git://xenbits.xen.org/libvirt.git ++ : osst...@xenbits.xen.org:/home/xen/git/libvirt.git ++ : git://xenbits.xen.org/libvirt.git ++ : git://xenbits.xen.org/osstest/rumprun.git ++ : git ++ : git://xenbits.xen.org/osstest/rumprun.git ++ : osst...@xenbits.xen.org:/home/xen/git/osstest/rumprun.git ++ : git://git.seabios.org/seabios.git ++ : osst...@xenbits.xen.org:/home/xen/git/osstest/seabios.git ++ : git://xenbits.xen.org/osstest/seabios.git ++ : https://github.com/tianocore/edk2.git ++ : osst...@xenbits.xen.org:/home/xen/git/osstest/ovmf.git ++ : git://xenbits.xen.org/osstest/ovmf.git ++ : git://xenbits.xen.org/osstest/linux-firmware.git ++ : osst...@xenbits.xen.org:/home/osstest/ext/linux-firmware.git ++ : git://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git ++ : osst...@xenbits.xen.org:/ho
Re: [Xen-devel] Granularity of Credit and RTDS Scheduler
On Tue, Jan 10, 2017 at 4:32 PM, wy11 wrote: > Thank you very much for your explanation. > > To me, it seems to be an accounting problem because even if the time is > accounted immediately a VCPU wake up or sleeps, the problem remains if the > time is recorded in microseconds or million seconds because a VCPU can run > for less than a unit so that the time accounted is 0. > > If the time granularity of RTDS is nanosecond, then it is no longer a > problem. Yep. > Can you please help me to know where I can find it in the source > code? It's in burn_budget function in sched_rt.c . > > Again, thanks a lot for your help. :-) Meng > > > Quoting Meng Xu : > >> [cc. Dario and George] >> >> On Fri, Jan 6, 2017 at 1:34 PM, wy11 wrote: >>> >>> Dear Xen developers, >> >> >> Hi, >> >>> >>> Recently I read a paper about possible theft of service attacks in Xen >>> hypervisor. >>> >>> https://arxiv.org/pdf/1103.0759.pdf >> >> >> I quickly read it. It is interesting to see that EC2 suffers from such >> issue. >> According to 4.1, it seems to me that this is more like a scheduler >> "bug" in budget accounting logic. >> When the attack VCPU wake up, the scheduler should starts to counting >> all time consumed from now on for the attack VM, instead of the victim >> VM. When the attack VCPU sleeps, the scheduler should accounts the >> budget consumed for the attack VM. >> >> In the event-driven RTDS scheduler, this issue should not happen. The >> scheduler did account the budget for the correct VMs, IIRC. >> Is there any experiment showing that RTDS scheduler suffers this issue? >> >>> >>> Due to the 10 ms intervals between sampling points, a malicious VM is >>> able >>> to run less than a interval and sleep to avoid being accounted. >> >> >> I don't think the scheduling interval is the issue. It is more like a >> budget accounting issue for me. >> Dario and George may have better answer for this. >> >>> >>> According to the info page of RTDS, it seems that after V4.7, a RTDS >>> based >>> scheduler achieves a granularity of microsecond. >> >> >> This is just the time granularity for users to specify the VCPU >> parameters. >> In the scheduler, it is in nanoseconds. >> >>> However, is it able that a >>> VM runs for less than a microsecond and relinquish the host actively so >>> as >>> to keep its budget? >> >> >> Nope. I don't think the attack model in the paper will succeed for the >> RTDS scheduler. >> If I understand the attack model correctly, it is the budget >> accounting issue instead of timing granularity issue. (Please correct >> me if I'm wrong). >> >> If you have a script to show the attack on RTDS scheduler, I would be >> happy to reproduce it on my machine and help fix it. >> >>> >>> A similar problem occurs in earlier Linux kernel, and it is fixed in >>> today's >>> Linux on x86 machines by utilizing a clock source TSC with a granularity >>> of >>> nanoseconds. I'd like to know if there is any reason that the Xen >>> hypervisor >>> does not choose a nanosecond scheduler? >> >> >> What do you mean by a nanosecond scheduler? In the kernel, scheduler >> accounts the budget in nanoseconds. >> >> Meng >> >> >> -- >> --- >> Meng Xu >> PhD Student in Computer and Information Science >> University of Pennsylvania >> http://www.cis.upenn.edu/~mengxu/ >> >> !DSPAM:8895,5871ea96257295422997164! > > > > -- --- Meng Xu PhD Student in Computer and Information Science University of Pennsylvania http://www.cis.upenn.edu/~mengxu/ ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCHv7 00/11] CONFIG_DEBUG_VIRTUAL for arm64
Hi, This is v7 of the patches to add CONFIG_DEBUG_VIRTUAL for arm64. This is a simple reordering of patches from v6 per request of Will Deacon for ease of merging support for arm which depends on this series. Laura Abbott (11): lib/Kconfig.debug: Add ARCH_HAS_DEBUG_VIRTUAL mm/cma: Cleanup highmem check mm: Introduce lm_alias kexec: Switch to __pa_symbol mm/kasan: Switch to using __pa_symbol and lm_alias mm/usercopy: Switch to using lm_alias drivers: firmware: psci: Use __pa_symbol for kernel symbol arm64: Move some macros under #ifndef __ASSEMBLY__ arm64: Add cast for virt_to_pfn arm64: Use __pa_symbol for kernel symbols arm64: Add support for CONFIG_DEBUG_VIRTUAL arch/arm64/Kconfig| 1 + arch/arm64/include/asm/kvm_mmu.h | 4 +- arch/arm64/include/asm/memory.h | 66 +-- arch/arm64/include/asm/mmu_context.h | 6 +-- arch/arm64/include/asm/pgtable.h | 2 +- arch/arm64/kernel/acpi_parking_protocol.c | 3 +- arch/arm64/kernel/cpu-reset.h | 2 +- arch/arm64/kernel/cpufeature.c| 3 +- arch/arm64/kernel/hibernate.c | 20 +++--- arch/arm64/kernel/insn.c | 2 +- arch/arm64/kernel/psci.c | 3 +- arch/arm64/kernel/setup.c | 9 +++-- arch/arm64/kernel/smp_spin_table.c| 3 +- arch/arm64/kernel/vdso.c | 8 +++- arch/arm64/mm/Makefile| 2 + arch/arm64/mm/init.c | 12 +++--- arch/arm64/mm/kasan_init.c| 22 +++ arch/arm64/mm/mmu.c | 33 ++-- arch/arm64/mm/physaddr.c | 30 ++ arch/x86/Kconfig | 1 + drivers/firmware/psci.c | 2 +- include/linux/mm.h| 4 ++ kernel/kexec_core.c | 2 +- lib/Kconfig.debug | 5 ++- mm/cma.c | 15 +++ mm/kasan/kasan_init.c | 15 +++ mm/usercopy.c | 4 +- 27 files changed, 180 insertions(+), 99 deletions(-) create mode 100644 arch/arm64/mm/physaddr.c -- 2.7.4 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] xen: do not re-use pirq number cached in pci device msi msg data
On Tue, Jan 10, 2017 at 2:03 PM, Stefano Stabellini wrote: > On Tue, 10 Jan 2017, Dan Streetman wrote: >> On Tue, Jan 10, 2017 at 10:57 AM, Dan Streetman wrote: >> > On Mon, Jan 9, 2017 at 2:30 PM, Stefano Stabellini >> > wrote: >> >> On Mon, 9 Jan 2017, Konrad Rzeszutek Wilk wrote: >> >>> On Mon, Jan 09, 2017 at 10:42:41AM -0500, Dan Streetman wrote: >> >>> > On Mon, Jan 9, 2017 at 9:59 AM, Boris Ostrovsky >> >>> > wrote: >> >>> > > On 01/06/2017 08:06 PM, Konrad Rzeszutek Wilk wrote: >> >>> > >> On Thu, Jan 05, 2017 at 02:28:56PM -0500, Dan Streetman wrote: >> >>> > >>> Do not read a pci device's msi message data to see if a pirq was >> >>> > >>> previously configured for the device's msi/msix, as the old pirq >> >>> > >>> was >> >>> > >>> unmapped and may now be in use by another pci device. The previous >> >>> > >>> pirq should never be re-used; instead a new pirq should always be >> >>> > >>> allocated from the hypervisor. >> >>> > >> Won't this cause a starvation problem? That is we will run out of >> >>> > >> PIRQs >> >>> > >> as we are not reusing them? >> >>> > > >> >>> > > Don't we free the pirq when we unmap it? >> >>> > >> >>> > I think this is actually a bit worse than I initially thought. After >> >>> > looking a bit closer, and I think there's an asymmetry with pirq >> >>> > allocation: >> >>> >> >>> Lets include Stefano, >> >>> >> >>> Thank you for digging in this! This has quite the deja-vu >> >>> feeling as I believe I hit this at some point in the past and >> >>> posted some possible ways of fixing this. But sadly I can't >> >>> find the thread. >> >> >> >> This issue seems to be caused by: >> >> >> >> commit af42b8d12f8adec6711cb824549a0edac6a4ae8f >> >> Author: Stefano Stabellini >> >> Date: Wed Dec 1 14:51:44 2010 + >> >> >> >> xen: fix MSI setup and teardown for PV on HVM guests >> >> >> >> which was a fix to a bug: >> >> >> >> This fixes a bug in xen_hvm_setup_msi_irqs that manifests itself when >> >> trying to enable the same MSI for the second time: the old MSI to pirq >> >> mapping is still valid at this point but xen_hvm_setup_msi_irqs would >> >> try to assign a new pirq anyway. >> >> A simple way to reproduce this bug is to assign an MSI capable network >> >> card to a PV on HVM guest, if the user brings down the corresponding >> >> ethernet interface and up again, Linux would fail to enable MSIs on >> >> the >> >> device. >> >> >> >> I don't remember any of the details. From the description of this bug, >> >> it seems that Xen changed behavior in the past few years: before it used >> >> to keep the pirq-MSI mapping, while today it doesn't. If I wrote "the >> >> old MSI to pirq mapping is still valid at this point", the pirq couldn't >> >> have been completely freed, then reassigned to somebody else the way it >> >> is described in this email. >> >> >> >> I think we should indentify the changeset or Xen version that introduced >> >> the new behavior. If it is old enough, we might be able to just revert >> >> af42b8d12f8adec6711cb824549a0edac6a4ae8f. Otherwise we could make the >> >> behavior conditional to the Xen version. >> > >> > Are PT devices the only MSI-capable devices available in a Xen guest? >> > That's where I'm seeing this problem, with PT NVMe devices. > > They are the main ones. It is possible to have emulated virtio devices > with emulated MSIs, for example virtio-net. Althought they are not in > the Xen Project CI-loop, so I wouldn't be surprised if they are broken > too. > > >> > I can say that on the Xen guest with NVMe PT devices I'm testing on, >> > with the patch from this thread (which essentially reverts your commit >> > above) as well as some added debug to see the pirq numbers, cycles of >> > 'modprobe nvme ; rmmod nvme' don't cause pirq starvation, as the >> > hypervisor provides essentially the same pirqs for each modprobe, >> > since they were freed by the rmmod. > > I am fine with reverting the old patch, but we need to understand what > caused the change in behavior first. Maybe somebody else with a Xen PCI > passthrough setup at hand can help with that. > > In the Xen code I can still see: > > case ECS_PIRQ: { > struct pirq *pirq = pirq_info(d1, chn1->u.pirq.irq); > > if ( !pirq ) > break; > if ( !is_hvm_domain(d1) ) > pirq_guest_unbind(d1, pirq); > > which means that pirq_guest_unbind should only be called on evtchn_close > if the guest is not an HVM guest. I tried an experiment to call get_free_pirq on both sides of a evtchn_close hcall, using two SRIOV nics. When I rmmod/modprobe, I see something interesting; each nic uses 3 MSIs, and it looks like when they shut down, each nic's 3 pirqs are not available until after the nic is done shutting down, so it seems like closing the evtchn isn't what makes the pirq free. [3697700.390188] xen:events: creating evtchn using pirq 101 irq 290 [3697700.390214] xen:events: creating evtchn using pirq 10
Re: [Xen-devel] Granularity of Credit and RTDS Scheduler
Thank you very much for your explanation. To me, it seems to be an accounting problem because even if the time is accounted immediately a VCPU wake up or sleeps, the problem remains if the time is recorded in microseconds or million seconds because a VCPU can run for less than a unit so that the time accounted is 0. If the time granularity of RTDS is nanosecond, then it is no longer a problem. Can you please help me to know where I can find it in the source code? Again, thanks a lot for your help. Best Regards, Wenqiu Quoting Meng Xu : [cc. Dario and George] On Fri, Jan 6, 2017 at 1:34 PM, wy11 wrote: Dear Xen developers, Hi, Recently I read a paper about possible theft of service attacks in Xen hypervisor. https://arxiv.org/pdf/1103.0759.pdf I quickly read it. It is interesting to see that EC2 suffers from such issue. According to 4.1, it seems to me that this is more like a scheduler "bug" in budget accounting logic. When the attack VCPU wake up, the scheduler should starts to counting all time consumed from now on for the attack VM, instead of the victim VM. When the attack VCPU sleeps, the scheduler should accounts the budget consumed for the attack VM. In the event-driven RTDS scheduler, this issue should not happen. The scheduler did account the budget for the correct VMs, IIRC. Is there any experiment showing that RTDS scheduler suffers this issue? Due to the 10 ms intervals between sampling points, a malicious VM is able to run less than a interval and sleep to avoid being accounted. I don't think the scheduling interval is the issue. It is more like a budget accounting issue for me. Dario and George may have better answer for this. According to the info page of RTDS, it seems that after V4.7, a RTDS based scheduler achieves a granularity of microsecond. This is just the time granularity for users to specify the VCPU parameters. In the scheduler, it is in nanoseconds. However, is it able that a VM runs for less than a microsecond and relinquish the host actively so as to keep its budget? Nope. I don't think the attack model in the paper will succeed for the RTDS scheduler. If I understand the attack model correctly, it is the budget accounting issue instead of timing granularity issue. (Please correct me if I'm wrong). If you have a script to show the attack on RTDS scheduler, I would be happy to reproduce it on my machine and help fix it. A similar problem occurs in earlier Linux kernel, and it is fixed in today's Linux on x86 machines by utilizing a clock source TSC with a granularity of nanoseconds. I'd like to know if there is any reason that the Xen hypervisor does not choose a nanosecond scheduler? What do you mean by a nanosecond scheduler? In the kernel, scheduler accounts the budget in nanoseconds. Meng -- --- Meng Xu PhD Student in Computer and Information Science University of Pennsylvania http://www.cis.upenn.edu/~mengxu/ !DSPAM:8895,5871ea96257295422997164! ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [ovmf baseline-only test] 68353: all pass
This run is configured for baseline tests only. flight 68353 ovmf real [real] http://osstest.xs.citrite.net/~osstest/testlogs/logs/68353/ Perfect :-) All tests in this flight passed as required version targeted for testing: ovmf 363dc42226a1d8ae02c73f9dd81da65af91b5fdd baseline version: ovmf fca422890777a02c027061fbceee454c9f117870 Last test of basis68351 2017-01-10 09:18:30 Z0 days Testing same since68353 2017-01-10 13:47:36 Z0 days1 attempts People who touched revisions under test: Chao Zhang Zhang, Chao B 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.xs.citrite.net logs: /home/osstest/logs images: /home/osstest/images Logs, config files, etc. are available at http://osstest.xs.citrite.net/~osstest/testlogs/logs Test harness code can be found at http://xenbits.xensource.com/gitweb?p=osstest.git;a=summary Push not applicable. commit 363dc42226a1d8ae02c73f9dd81da65af91b5fdd Author: Zhang, Chao B Date: Tue Jan 10 14:53:08 2017 +0800 SecurityPkg: Tcg2ConfigDxe/Tcg2Smm: Fix TPM2 HID issue Fix wrong TPM2 HID generation logic. Cc: Star Zeng Cc: Yao Jiewen Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Chao Zhang Reviewed-by: Star Zeng Reviewed-by: Yao Jiewen ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v11 07/13] x86: add multiboot2 protocol support for EFI platforms
On 1/9/17 7:37 PM, Doug Goldstein wrote: > On 12/5/16 4:25 PM, Daniel Kiper wrote: >> diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h >> index 62c010e..dc857d8 100644 >> --- a/xen/arch/x86/efi/efi-boot.h >> +++ b/xen/arch/x86/efi/efi-boot.h >> @@ -146,6 +146,8 @@ static void __init >> efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable, >> { >> struct e820entry *e; >> unsigned int i; >> +/* Check for extra mem for mbi data if Xen is loaded via multiboot2 >> protocol. */ >> +UINTN extra_mem = efi_enabled(EFI_LOADER) ? 0 : (64 << 10); > > Just wondering where the constant came from? And if there should be a > little bit of information about it. To me its just weird to shift 64. Its the size of the stack used in the assembly code. > >> >> /* Populate E820 table and check trampoline area availability. */ >> e = e820map - 1; >> @@ -168,7 +170,8 @@ static void __init >> efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable, >> /* fall through */ >> case EfiConventionalMemory: >> if ( !trampoline_phys && desc->PhysicalStart + len <= 0x10 >> && >> - len >= cfg.size && desc->PhysicalStart + len > cfg.addr ) >> + len >= cfg.size + extra_mem && >> + desc->PhysicalStart + len > cfg.addr ) >> cfg.addr = (desc->PhysicalStart + len - cfg.size) & >> PAGE_MASK; > > So this is where the current series blows up and fails on real hardware. Honestly this was my misunderstanding and this shouldn't ever be used to get memory for the trampoline. This also has the bug in it that it needs to be: ASSERT(cfg.size > 0); cfg.addr = (desc->PhysicalStart + len - (cfg.size + extra_mem) & PAGE_MASK; > No where in the EFI + MB2 code path is cfg.size ever initialized. Its > only initialized in the straight EFI case. The result is that cfg.addr > is set to the section immediately following this. Took a bit to > trackdown because I checked for memory overlaps with where Xen was > loaded and where it was relocated to but forgot to check for overlaps > with the trampoline code. This is the address where the trampoline jumps > are copied. > > Personally I'd like to see an ASSERT added or the code swizzled around > in such a way that its not possible to get into a bad state. But this is > probably another patch series. > >> /* fall through */ >> case EfiLoaderCode: >> @@ -210,12 +213,14 @@ static void *__init >> efi_arch_allocate_mmap_buffer(UINTN map_size) >> >> static void __init efi_arch_pre_exit_boot(void) >> { >> -if ( !trampoline_phys ) >> -{ >> -if ( !cfg.addr ) >> -blexit(L"No memory for trampoline"); >> +if ( trampoline_phys ) >> +return; >> + >> +if ( !cfg.addr ) >> +blexit(L"No memory for trampoline"); >> + >> +if ( efi_enabled(EFI_LOADER) ) >> relocate_trampoline(cfg.addr); Why is this call even here anymore? Its called in efi_arch_memory_setup() already. If it was unable to allocate memory under the 1mb region its just going to trample over ANY conventional memory region that might be in use. >> -} >> } >> >> static void __init noreturn efi_arch_post_exit_boot(void) >> @@ -653,6 +658,43 @@ static bool_t __init >> efi_arch_use_config_file(EFI_SYSTEM_TABLE *SystemTable) >> >> static void efi_arch_flush_dcache_area(const void *vaddr, UINTN size) { } >> >> +paddr_t __init efi_multiboot2(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE >> *SystemTable) >> +{ >> +EFI_GRAPHICS_OUTPUT_PROTOCOL *gop; >> +UINTN cols, gop_mode = ~0, rows; >> + >> +__set_bit(EFI_BOOT, &efi_flags); >> +__set_bit(EFI_RS, &efi_flags); >> + >> +efi_init(ImageHandle, SystemTable); >> + >> +efi_console_set_mode(); >> + >> +if ( StdOut->QueryMode(StdOut, StdOut->Mode->Mode, >> + &cols, &rows) == EFI_SUCCESS ) >> +efi_arch_console_init(cols, rows); >> + >> +gop = efi_get_gop(); >> + >> +if ( gop ) >> +gop_mode = efi_find_gop_mode(gop, 0, 0, 0); >> + >> +efi_arch_edd(); >> +efi_arch_cpu(); >> + >> +efi_tables(); >> +setup_efi_pci(); >> +efi_variables(); > > This is probably where you missed the call to "efi_arch_memory_setup();" > that gave me hell above. Well it turns out that calling "efi_arch_memory_setup()" isn't correct because it also messes with the page tables AND also does the trampoline relocation. Which after this call finishes then it does the trampoline relocations in assembly. The code currently makes the assumption it can use any conventional memory range below 1mb (and unfortunately does the math incorrectly and instead uses the region following the conventional memory region). You need to use AllocatePages() otherwise you are trampling memory that might have been allocated by the bootloader or any multiboot modules (e.g. tboot) prior to Xen. I'm just going to write a patch to
[Xen-devel] [xen-unstable-smoke test] 104101: tolerable all pass - PUSHED
flight 104101 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/104101/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 12 migrate-support-checkfail never pass test-armhf-armhf-xl 12 migrate-support-checkfail never pass test-armhf-armhf-xl 13 saverestore-support-checkfail never pass version targeted for testing: xen ffc103c223a6d12e5221f66b7e96396a61ba1b20 baseline version: xen 9d81162fd2c69b5c9279670c8739d891cf7571da Last test of basis 104097 2017-01-10 11:01:32 Z0 days Testing same since 104101 2017-01-10 18:01:05 Z0 days1 attempts People who touched revisions under test: Andrew Cooper Suravee Suthikulpanit jobs: build-amd64 pass build-armhf pass build-amd64-libvirt pass test-armhf-armhf-xl pass test-amd64-amd64-xl-qemuu-debianhvm-i386 pass test-amd64-amd64-libvirt pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Pushing revision : + branch=xen-unstable-smoke + revision=ffc103c223a6d12e5221f66b7e96396a61ba1b20 + . ./cri-lock-repos ++ . ./cri-common +++ . ./cri-getconfig +++ umask 002 +++ getrepos getconfig Repos perl -e ' use Osstest; readglobalconfig(); print $c{"Repos"} or die $!; ' +++ local repos=/home/osstest/repos +++ '[' -z /home/osstest/repos ']' +++ '[' '!' -d /home/osstest/repos ']' +++ echo /home/osstest/repos ++ repos=/home/osstest/repos ++ repos_lock=/home/osstest/repos/lock ++ '[' x '!=' x/home/osstest/repos/lock ']' ++ OSSTEST_REPOS_LOCK_LOCKED=/home/osstest/repos/lock ++ exec with-lock-ex -w /home/osstest/repos/lock ./ap-push xen-unstable-smoke ffc103c223a6d12e5221f66b7e96396a61ba1b20 + branch=xen-unstable-smoke + revision=ffc103c223a6d12e5221f66b7e96396a61ba1b20 + . ./cri-lock-repos ++ . ./cri-common +++ . ./cri-getconfig +++ umask 002 +++ getrepos getconfig Repos perl -e ' use Osstest; readglobalconfig(); print $c{"Repos"} or die $!; ' +++ local repos=/home/osstest/repos +++ '[' -z /home/osstest/repos ']' +++ '[' '!' -d /home/osstest/repos ']' +++ echo /home/osstest/repos ++ repos=/home/osstest/repos ++ repos_lock=/home/osstest/repos/lock ++ '[' x/home/osstest/repos/lock '!=' x/home/osstest/repos/lock ']' + . ./cri-common ++ . ./cri-getconfig ++ umask 002 + select_xenbranch + case "$branch" in + tree=xen + xenbranch=xen-unstable-smoke + qemuubranch=qemu-upstream-unstable + '[' xxen = xlinux ']' + linuxbranch= + '[' xqemu-upstream-unstable = x ']' + select_prevxenbranch ++ ./cri-getprevxenbranch xen-unstable-smoke + prevxenbranch=xen-4.8-testing + '[' xffc103c223a6d12e5221f66b7e96396a61ba1b20 = x ']' + : tested/2.6.39.x + . ./ap-common ++ : osst...@xenbits.xen.org +++ getconfig OsstestUpstream +++ perl -e ' use Osstest; readglobalconfig(); print $c{"OsstestUpstream"} or die $!; ' ++ : ++ : git://xenbits.xen.org/xen.git ++ : osst...@xenbits.xen.org:/home/xen/git/xen.git ++ : git://xenbits.xen.org/qemu-xen-traditional.git ++ : git://git.kernel.org ++ : git://git.kernel.org/pub/scm/linux/kernel/git ++ : git ++ : git://xenbits.xen.org/xtf.git ++ : osst...@xenbits.xen.org:/home/xen/git/xtf.git ++ : git://xenbits.xen.org/xtf.git ++ : git://xenbits.xen.org/libvirt.git ++ : osst...@xenbits.xen.org:/home/xen/git/libvirt.git ++ : git://xenbits.xen.org/libvirt.git ++ : git://xenbits.xen.org/osstest/rumprun.git ++ : git ++ : git://xenbits.xen.org/osstest/rumprun.git ++ : osst...@xenbits.xen.org:/home/xen/git/osstest/rumprun.git ++ : git://git.seabios.org/seabios.git ++ : osst...@xenbits.xen.org:/home/xen/git/osstest/seabios.git ++ : git://xenbits.xen.org/osstest/seabios.git ++ : https://github.com/tianocore/edk2.git ++ : osst...@xenbits.xen.org:/home/xen/git/osstest/ovmf.git ++ : git://xenbits.xen.org/osstest/ovmf.git ++ : git://xenbits.xen.org/osstest/linux-firmware.git ++ : osst...@xenbits.xen.org:/home/osstest/ext/linux-firmware.git ++ : git://git.kernel.org/pub/scm
Re: [Xen-devel] Xen Community Call on new PV protocols, Tuesday 10th Jan 9AM PST
These are the minutes I took during the call: Xen PV Drivers Lifecycle document ready to be committed. Common Pitfalls for new PV protocols: - 32 vs 64 fields - not Linux centric - missing version fields and feature flags Full list of outstanding PV protocols: pvcalls, xen-9pfs, multitouch events, sound, display, netfront/netback extension PVCalls: should be OK to move forward, Konrad will review next Sound: should be OK to move forward, Konrad will review next Display: concerns about whether it will replace xenfb. Konrad will take a look after Multitouch: new, but simple, Stefano will review Netfront/netback extension: new, still many comments outstanding, probably not for the next release Things to do - use pahole 32 bit and 64 bit to check the structs, publish the output on xendevel - use version field and feature flags on xenstore - write new PV protocols guideline doc to explain common pitfalls and versioning to newcomers - write a README in docs/misc with the state of new PV protocols when they are checked in - Oleksandr will publish a library for userspace backends in C++ to xenbits ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH RFC 13/14] OvmfPkg: Introduce XenIoPvhDxe to initialize Grant Tables
On Thu, Dec 08, 2016 at 03:33:39PM +, Anthony PERARD wrote: > This "device" use XenIoMmioLib to reserve some space to be use by grant > tables. It's use 0xfc00, which might not be a good choice... Perhaps parse the E820 and find an E820_RSV region and use that? And obviously don't use the same that is for APIC? ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 3/3] xen: optimize xenbus driver for multiple concurrent xenstore accesses
> diff --git a/drivers/xen/xenbus/xenbus_dev_frontend.c > b/drivers/xen/xenbus/xenbus_dev_frontend.c > index e4b9847..4d343ee 100644 > --- a/drivers/xen/xenbus/xenbus_dev_frontend.c > +++ b/drivers/xen/xenbus/xenbus_dev_frontend.c > LGTM. -boris ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] xen: do not re-use pirq number cached in pci device msi msg data
On Tue, 10 Jan 2017, Dan Streetman wrote: > On Tue, Jan 10, 2017 at 10:57 AM, Dan Streetman wrote: > > On Mon, Jan 9, 2017 at 2:30 PM, Stefano Stabellini > > wrote: > >> On Mon, 9 Jan 2017, Konrad Rzeszutek Wilk wrote: > >>> On Mon, Jan 09, 2017 at 10:42:41AM -0500, Dan Streetman wrote: > >>> > On Mon, Jan 9, 2017 at 9:59 AM, Boris Ostrovsky > >>> > wrote: > >>> > > On 01/06/2017 08:06 PM, Konrad Rzeszutek Wilk wrote: > >>> > >> On Thu, Jan 05, 2017 at 02:28:56PM -0500, Dan Streetman wrote: > >>> > >>> Do not read a pci device's msi message data to see if a pirq was > >>> > >>> previously configured for the device's msi/msix, as the old pirq was > >>> > >>> unmapped and may now be in use by another pci device. The previous > >>> > >>> pirq should never be re-used; instead a new pirq should always be > >>> > >>> allocated from the hypervisor. > >>> > >> Won't this cause a starvation problem? That is we will run out of > >>> > >> PIRQs > >>> > >> as we are not reusing them? > >>> > > > >>> > > Don't we free the pirq when we unmap it? > >>> > > >>> > I think this is actually a bit worse than I initially thought. After > >>> > looking a bit closer, and I think there's an asymmetry with pirq > >>> > allocation: > >>> > >>> Lets include Stefano, > >>> > >>> Thank you for digging in this! This has quite the deja-vu > >>> feeling as I believe I hit this at some point in the past and > >>> posted some possible ways of fixing this. But sadly I can't > >>> find the thread. > >> > >> This issue seems to be caused by: > >> > >> commit af42b8d12f8adec6711cb824549a0edac6a4ae8f > >> Author: Stefano Stabellini > >> Date: Wed Dec 1 14:51:44 2010 + > >> > >> xen: fix MSI setup and teardown for PV on HVM guests > >> > >> which was a fix to a bug: > >> > >> This fixes a bug in xen_hvm_setup_msi_irqs that manifests itself when > >> trying to enable the same MSI for the second time: the old MSI to pirq > >> mapping is still valid at this point but xen_hvm_setup_msi_irqs would > >> try to assign a new pirq anyway. > >> A simple way to reproduce this bug is to assign an MSI capable network > >> card to a PV on HVM guest, if the user brings down the corresponding > >> ethernet interface and up again, Linux would fail to enable MSIs on the > >> device. > >> > >> I don't remember any of the details. From the description of this bug, > >> it seems that Xen changed behavior in the past few years: before it used > >> to keep the pirq-MSI mapping, while today it doesn't. If I wrote "the > >> old MSI to pirq mapping is still valid at this point", the pirq couldn't > >> have been completely freed, then reassigned to somebody else the way it > >> is described in this email. > >> > >> I think we should indentify the changeset or Xen version that introduced > >> the new behavior. If it is old enough, we might be able to just revert > >> af42b8d12f8adec6711cb824549a0edac6a4ae8f. Otherwise we could make the > >> behavior conditional to the Xen version. > > > > Are PT devices the only MSI-capable devices available in a Xen guest? > > That's where I'm seeing this problem, with PT NVMe devices. They are the main ones. It is possible to have emulated virtio devices with emulated MSIs, for example virtio-net. Althought they are not in the Xen Project CI-loop, so I wouldn't be surprised if they are broken too. > > I can say that on the Xen guest with NVMe PT devices I'm testing on, > > with the patch from this thread (which essentially reverts your commit > > above) as well as some added debug to see the pirq numbers, cycles of > > 'modprobe nvme ; rmmod nvme' don't cause pirq starvation, as the > > hypervisor provides essentially the same pirqs for each modprobe, > > since they were freed by the rmmod. I am fine with reverting the old patch, but we need to understand what caused the change in behavior first. Maybe somebody else with a Xen PCI passthrough setup at hand can help with that. In the Xen code I can still see: case ECS_PIRQ: { struct pirq *pirq = pirq_info(d1, chn1->u.pirq.irq); if ( !pirq ) break; if ( !is_hvm_domain(d1) ) pirq_guest_unbind(d1, pirq); which means that pirq_guest_unbind should only be called on evtchn_close if the guest is not an HVM guest. > >>> > tl;dr: > >>> > > >>> > pci_enable_msix_range() -> each MSIX (or MSI) now has a pirq > >>> > allocated, and reserved in the hypervisor > >>> > > >>> > request_irq() -> an event channel is opened for the specific pirq, and > >>> > maps the pirq with the hypervisor > >>> > > >>> > free_irq() -> the event channel is closed, and the pirq is unmapped, > >>> > but that unmap function also frees the pirq! The hypervisor can/will > >>> > give it away to the next call to get_free_pirq. However, the pci > >>> > msi/msix data area still contains the pirq number, and the next call > >>> > to request_irq() will re-use the pirq. > >>> > > >>> > pci_disable_msix() -> this has no effe
Re: [Xen-devel] [edk2] [PATCH RFC 06/14] OvmfPkg/XenPlatformPei: Add xen PVH specific code
On Tue, Jan 10, 2017 at 05:54:49PM +0100, Laszlo Ersek wrote: > On 01/10/17 17:18, Anthony PERARD wrote: > > On Thu, Jan 05, 2017 at 11:30:32AM +0100, Laszlo Ersek wrote: > >> On 12/08/16 16:33, Anthony PERARD wrote: > >>> diff --git a/OvmfPkg/XenPlatformPei/Platform.c > >>> b/OvmfPkg/XenPlatformPei/Platform.c > >>> index bf78878..9fc713c 100644 > >>> --- a/OvmfPkg/XenPlatformPei/Platform.c > >>> +++ b/OvmfPkg/XenPlatformPei/Platform.c > >>> @@ -362,6 +362,10 @@ MiscInitialization ( > >>>AcpiCtlReg = POWER_MGMT_REGISTER_Q35 (ICH9_ACPI_CNTL); > >>>AcpiEnBit = ICH9_ACPI_CNTL_ACPI_EN; > >>>break; > >>> +case 0x: > >>> + // xen PVH, ignore > >>> + PcdStatus = PcdSet16S (PcdOvmfHostBridgePciDevId, > >>> mHostBridgeDevId); > >>> + return; > >> > >> Can we create a new macro for 0x in > >> "OvmfPkg/Include/OvmfPlatforms.h", and use that here? > >> > >> Normally I would suggest a separate header file under "OvmfPkg/Include", > >> similar to "Q35MchIch9.h" and "I440FxPiix4.h", and to include that new > >> file in "OvmfPlatforms.h", but here we only need a single "chipset > >> identifier", so I guess that can go directly into "OvmfPlatforms.h". > >> > >> I mainly suggest this because in patch 08/14, the same 0x case label > >> is being added to code shared with QEMU, and using a verbose macro there > >> is much better than a magic number. In turn, we should use the same > >> macro here, I think. > > > > This value is just the result of a read to an incorrect location, and > > the return value is -1. There is no PCI when running in Xen PVH, at > > least for now. I think I should try harder to find out if OVMF is > > running in PVH, and so I would not need this case 0x at all. > > > > Right, that would be best. > > In fact, this affects two modules, (Xen)PlatformPei and > PlatformBootManagerLib. In both, we already have the XenDetected() > function. Maybe you can add a XenPvhDetected() function as well (which > would return TRUE only for PVH, while the original XenDetected() would > continue returning TRUE for both HVM and PVH). Then the PCI host bridge > ID checking could be entirely short-circuited if XenPvhDetected() > returned TRUE first. It's very likely that when doing PCI-passthrough to a PVH guest an emulated PCI host bridge is going to be available, so short-circuiting the bridge check just based on PVH detection would then be wrong. Roger. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC 0/2] XEN SWIOTLB for ARM dma operations extension
On Tue, Jan 10, 2017 at 06:44:25PM +0200, Andrii Anisov wrote: > From: Andrii Anisov > > Some drivers need additional dma ops to be provided by xen swiotlb. Because > common operations implementation came from x86 world and does not suite ARM > needs. > > dma_mmap patch is a port of an antique and lost patch discussed here: > http://marc.info/?l=xen-devel&m=139695901430667&w=4 It would be good to have that as part of this patchset, otherwise this is kind of non-compiling type patch. But more importantly, I think you need to also to take into account Stefano's comments: http://marc.info/?l=xen-devel&m=139991966525561&w=4 > > Andrii Anisov (1): > swiotlb-xen: implement xen_swiotlb_get_sgtable callback > > Stefano Stabellini (1): > swiotlb-xen: implement xen_swiotlb_dma_mmap callback > > arch/arm/xen/mm.c | 25 + > 1 file changed, 25 insertions(+) > > -- > 2.7.4 > > > ___ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] xen: do not re-use pirq number cached in pci device msi msg data
On Tue, Jan 10, 2017 at 10:57 AM, Dan Streetman wrote: > On Mon, Jan 9, 2017 at 2:30 PM, Stefano Stabellini > wrote: >> On Mon, 9 Jan 2017, Konrad Rzeszutek Wilk wrote: >>> On Mon, Jan 09, 2017 at 10:42:41AM -0500, Dan Streetman wrote: >>> > On Mon, Jan 9, 2017 at 9:59 AM, Boris Ostrovsky >>> > wrote: >>> > > On 01/06/2017 08:06 PM, Konrad Rzeszutek Wilk wrote: >>> > >> On Thu, Jan 05, 2017 at 02:28:56PM -0500, Dan Streetman wrote: >>> > >>> Do not read a pci device's msi message data to see if a pirq was >>> > >>> previously configured for the device's msi/msix, as the old pirq was >>> > >>> unmapped and may now be in use by another pci device. The previous >>> > >>> pirq should never be re-used; instead a new pirq should always be >>> > >>> allocated from the hypervisor. >>> > >> Won't this cause a starvation problem? That is we will run out of PIRQs >>> > >> as we are not reusing them? >>> > > >>> > > Don't we free the pirq when we unmap it? >>> > >>> > I think this is actually a bit worse than I initially thought. After >>> > looking a bit closer, and I think there's an asymmetry with pirq >>> > allocation: >>> >>> Lets include Stefano, >>> >>> Thank you for digging in this! This has quite the deja-vu >>> feeling as I believe I hit this at some point in the past and >>> posted some possible ways of fixing this. But sadly I can't >>> find the thread. >> >> This issue seems to be caused by: >> >> commit af42b8d12f8adec6711cb824549a0edac6a4ae8f >> Author: Stefano Stabellini >> Date: Wed Dec 1 14:51:44 2010 + >> >> xen: fix MSI setup and teardown for PV on HVM guests >> >> which was a fix to a bug: >> >> This fixes a bug in xen_hvm_setup_msi_irqs that manifests itself when >> trying to enable the same MSI for the second time: the old MSI to pirq >> mapping is still valid at this point but xen_hvm_setup_msi_irqs would >> try to assign a new pirq anyway. >> A simple way to reproduce this bug is to assign an MSI capable network >> card to a PV on HVM guest, if the user brings down the corresponding >> ethernet interface and up again, Linux would fail to enable MSIs on the >> device. >> >> I don't remember any of the details. From the description of this bug, >> it seems that Xen changed behavior in the past few years: before it used >> to keep the pirq-MSI mapping, while today it doesn't. If I wrote "the >> old MSI to pirq mapping is still valid at this point", the pirq couldn't >> have been completely freed, then reassigned to somebody else the way it >> is described in this email. >> >> I think we should indentify the changeset or Xen version that introduced >> the new behavior. If it is old enough, we might be able to just revert >> af42b8d12f8adec6711cb824549a0edac6a4ae8f. Otherwise we could make the >> behavior conditional to the Xen version. > > Are PT devices the only MSI-capable devices available in a Xen guest? > That's where I'm seeing this problem, with PT NVMe devices. > > I can say that on the Xen guest with NVMe PT devices I'm testing on, > with the patch from this thread (which essentially reverts your commit > above) as well as some added debug to see the pirq numbers, cycles of > 'modprobe nvme ; rmmod nvme' don't cause pirq starvation, as the > hypervisor provides essentially the same pirqs for each modprobe, > since they were freed by the rmmod. > >> >> >>> > tl;dr: >>> > >>> > pci_enable_msix_range() -> each MSIX (or MSI) now has a pirq >>> > allocated, and reserved in the hypervisor >>> > >>> > request_irq() -> an event channel is opened for the specific pirq, and >>> > maps the pirq with the hypervisor >>> > >>> > free_irq() -> the event channel is closed, and the pirq is unmapped, >>> > but that unmap function also frees the pirq! The hypervisor can/will >>> > give it away to the next call to get_free_pirq. However, the pci >>> > msi/msix data area still contains the pirq number, and the next call >>> > to request_irq() will re-use the pirq. >>> > >>> > pci_disable_msix() -> this has no effect on the pirq in the hypervisor >>> > (it's already been freed), and it also doesn't clear anything from the >>> > msi/msix data area, so the pirq is still cached there. >>> > >>> > >>> > It seems like the hypervisor needs to be fixed to *not* unmap the pirq >>> > when the event channel is closed - or at least, not to change it to >>> > IRQ_UNBOUND state? And, the pci_disable_msix call should eventually >>> > call into something in the Xen guest kernel that actually does the >>> > pirq unmapping, and clear it from the msi data area (i.e. >>> > pci_write_msi_msg) >>> >>> The problem is that Xen changes have sailed a long long time ago. >>> > >>> > Alternately, if the hypervisor doesn't change, then the call into the >>> > hypervisor to actually allocate a pirq needs to move from the >>> > pci_enable_msix_range() call to the request_irq() call? So that when >>> > the msi/msix range is enabled, it doesn't actually reserve any pirq's >>> > for any of t
Re: [Xen-devel] Ubuntu 16.04.1 LTS kernel 4.4.0-57 over-allocation and xen-access fail
On 01/10/2017 06:40 PM, Tamas K Lengyel wrote: > > (replying to all): I'm not in favor of this patch mainly because it is > > not stealthy. A malicious kernel could easily track what events are > > being sent on the ring. With DRAKVUF I could work around this using > > altp2m pfn-remapping, but for other tools this is can be a serious > > information leak. > > I understand your point, however the alternative is potential lack of > availability to monitor which is arguably a more severe problem. _Any_ > guest could choose to do what this Ubuntu 16.04 guest does, and then > connecting to the guest via vm_event can only be done once. > > > IMHO in that case you should implement your own internal version of this > function to fall back to instead of forcing all tools to go down this > path. The requirements to do that in your own tool are accessible so > there is no need to push that into libxc. Obviously I won't send a patch you're against as co-maintainer of vm_event, however looking at my version of xc_vm_event_enable() (from Xen 4.6), it's calling xc_vm_event_control() which is not publicly accesible - so going my own way would in this case still require libxc modifications. We could also add another parameter to xc_vm_event_enable(), for example "bool remove_page_from_guest_physmap" to control this behaviour, which would change the API - or leave the current API alone and add xc_vm_event_control_remap() or something to that effect. But in any case I don't see how any of this can be achieved with zero libxc modifications. Thanks, Razvan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [edk2] [PATCH RFC 06/14] OvmfPkg/XenPlatformPei: Add xen PVH specific code
On 01/10/17 17:18, Anthony PERARD wrote: > On Thu, Jan 05, 2017 at 11:30:32AM +0100, Laszlo Ersek wrote: >> On 12/08/16 16:33, Anthony PERARD wrote: >>> - learn about memory size from the E820 >>> - ignore error if host bridge devid is 0x, PVH does not have PCI and >>> reading from unexisting device return -1. >>> >>> Contributed-under: TianoCore Contribution Agreement 1.0 >>> Signed-off-by: Anthony PERARD >>> --- >>> OvmfPkg/XenPlatformPei/MemDetect.c | 71 >>> ++ >>> OvmfPkg/XenPlatformPei/Platform.c | 5 +++ >>> OvmfPkg/XenPlatformPei/Platform.h | 10 ++ >>> 3 files changed, 86 insertions(+) >>> >>> diff --git a/OvmfPkg/XenPlatformPei/MemDetect.c >>> b/OvmfPkg/XenPlatformPei/MemDetect.c >>> index 4ecdf5e..0d80775 100644 >>> --- a/OvmfPkg/XenPlatformPei/MemDetect.c >>> +++ b/OvmfPkg/XenPlatformPei/MemDetect.c >>> @@ -42,6 +42,70 @@ UINT8 mPhysMemAddressWidth; >>> STATIC UINT32 mS3AcpiReservedMemoryBase; >>> STATIC UINT32 mS3AcpiReservedMemorySize; >>> >>> +STATIC UINT32 mXenLowerMemorySize = 0; >>> +STATIC UINT64 mXenHighMemorySize = 0; >>> + >>> +VOID >>> +XenReadMemorySizes ( >>> + VOID >>> + ) >>> +{ >>> + EFI_E820_ENTRY64 *E820Map; >>> + UINT32E820EntriesCount; >>> + EFI_STATUS Status; >>> + >>> + Status = XenGetE820Map (&E820Map, &E820EntriesCount); >>> + ASSERT_EFI_ERROR (Status); >>> + >>> + mXenLowerMemorySize = 0; >>> + mXenHighMemorySize = 0; >>> + >>> + if (E820EntriesCount > 0) { >>> +EFI_E820_ENTRY64 *Entry; >>> +UINT32 Loop; >>> + >>> +for (Loop = 0; Loop < E820EntriesCount; Loop++) { >>> + Entry = E820Map + Loop; >>> + >>> + // >>> + // Only care about RAM >>> + // >>> + if (Entry->Type != EfiAcpiAddressRangeMemory) { >>> +continue; >>> + } >>> + >>> + if ((Entry->BaseAddr + Entry->Length) <= SIZE_16MB) { >>> +continue; >>> + } >>> + >>> + if (Entry->BaseAddr < SIZE_16MB) { >>> +UINT64 bottom = Entry->BaseAddr; >>> +UINT64 size = Entry->Length; >>> +size -= SIZE_16MB - bottom; >>> +bottom = SIZE_16MB; >>> +mXenLowerMemorySize += size; >>> +continue; >>> + } >>> + if (Entry->BaseAddr <= SIZE_4GB) { >>> +UINT64 size = Entry->Length; >>> +mXenLowerMemorySize += size; >>> +continue; >>> + } >>> + >>> + if (Entry->BaseAddr == SIZE_4GB) { >>> +mXenHighMemorySize = Entry->Length; >>> +continue; >>> + } >>> + >>> + DEBUG ((EFI_D_INFO, "%a: ignored XenE820 entry (0x%llx, 0x%llx)\n", >>> + __FUNCTION__, >>> + Entry->BaseAddr, >>> + Entry->Length)); >>> +} >>> +mXenLowerMemorySize += SIZE_16MB; >>> + } >>> +} >>> + >>> UINT32 >>> GetSystemMemorySizeBelow4gb ( >>>VOID >>> @@ -50,6 +114,9 @@ GetSystemMemorySizeBelow4gb ( >>>UINT8 Cmos0x34; >>>UINT8 Cmos0x35; >>> >>> + if (mXen && mXenLowerMemorySize) >>> +return mXenLowerMemorySize; >>> + >>>// >>>// CMOS 0x34/0x35 specifies the system memory above 16 MB. >>>// * CMOS(0x35) is the high byte >>> @@ -74,6 +141,10 @@ GetSystemMemorySizeAbove4gb ( >>>UINT32 Size; >>>UINTN CmosIndex; >>> >>> + // if lower memory is specified that way, return also high memory >>> + if (mXen && mXenLowerMemorySize) >>> +return mXenHighMemorySize; >>> + >>>// >>>// CMOS 0x5b-0x5d specifies the system memory above 4GB MB. >>>// * CMOS(0x5d) is the most significant size byte >>> diff --git a/OvmfPkg/XenPlatformPei/Platform.c >>> b/OvmfPkg/XenPlatformPei/Platform.c >>> index bf78878..9fc713c 100644 >>> --- a/OvmfPkg/XenPlatformPei/Platform.c >>> +++ b/OvmfPkg/XenPlatformPei/Platform.c >>> @@ -362,6 +362,10 @@ MiscInitialization ( >>>AcpiCtlReg = POWER_MGMT_REGISTER_Q35 (ICH9_ACPI_CNTL); >>>AcpiEnBit = ICH9_ACPI_CNTL_ACPI_EN; >>>break; >>> +case 0x: >>> + // xen PVH, ignore >>> + PcdStatus = PcdSet16S (PcdOvmfHostBridgePciDevId, mHostBridgeDevId); >>> + return; >> >> Can we create a new macro for 0x in >> "OvmfPkg/Include/OvmfPlatforms.h", and use that here? >> >> Normally I would suggest a separate header file under "OvmfPkg/Include", >> similar to "Q35MchIch9.h" and "I440FxPiix4.h", and to include that new >> file in "OvmfPlatforms.h", but here we only need a single "chipset >> identifier", so I guess that can go directly into "OvmfPlatforms.h". >> >> I mainly suggest this because in patch 08/14, the same 0x case label >> is being added to code shared with QEMU, and using a verbose macro there >> is much better than a magic number. In turn, we should use the same >> macro here, I think. > > This value is just the result of a read to an incorrect location, and > the return value is -1. There is no PCI when running in Xen PVH, at > least for now. I think I should try harder to find out if OVMF is > running in PVH, and so I would not need this case 0x
[Xen-devel] [RFC 0/2] XEN SWIOTLB for ARM dma operations extension
From: Andrii Anisov Some drivers need additional dma ops to be provided by xen swiotlb. Because common operations implementation came from x86 world and does not suite ARM needs. dma_mmap patch is a port of an antique and lost patch discussed here: http://marc.info/?l=xen-devel&m=139695901430667&w=4 Andrii Anisov (1): swiotlb-xen: implement xen_swiotlb_get_sgtable callback Stefano Stabellini (1): swiotlb-xen: implement xen_swiotlb_dma_mmap callback arch/arm/xen/mm.c | 25 + 1 file changed, 25 insertions(+) -- 2.7.4 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [RFC 1/2] swiotlb-xen: implement xen_swiotlb_dma_mmap callback
From: Stefano Stabellini This function creates userspace mapping for the DMA-coherent memory. Signed-off-by: Stefano Stabellini Signed-off-by: Oleksandr Dmytryshyn Signed-off-by: Andrii Anisov --- arch/arm/xen/mm.c | 14 ++ 1 file changed, 14 insertions(+) diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c index c5f9a9e..050cf34 100644 --- a/arch/arm/xen/mm.c +++ b/arch/arm/xen/mm.c @@ -163,6 +163,19 @@ bool xen_arch_need_swiotlb(struct device *dev, !is_device_dma_coherent(dev)); } +/* + * Create userspace mapping for the DMA-coherent memory. + */ +static int xen_swiotlb_dma_mmap(struct device *dev, struct vm_area_struct *vma, +void *cpu_addr, dma_addr_t dma_addr, size_t size, +struct dma_attrs *attrs) +{ + if (__generic_dma_ops(dev)->mmap) + return __generic_dma_ops(dev)->mmap(dev, vma, cpu_addr, dma_addr, size, attrs); + + return dma_common_mmap(dev, vma, cpu_addr, dma_addr, size); +} + int xen_create_contiguous_region(phys_addr_t pstart, unsigned int order, unsigned int address_bits, dma_addr_t *dma_handle) @@ -199,6 +212,7 @@ static struct dma_map_ops xen_swiotlb_dma_ops = { .unmap_page = xen_swiotlb_unmap_page, .dma_supported = xen_swiotlb_dma_supported, .set_dma_mask = xen_swiotlb_set_dma_mask, + .mmap = xen_swiotlb_dma_mmap, }; int __init xen_mm_init(void) -- 2.7.4 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [RFC 2/2] swiotlb-xen: implement xen_swiotlb_get_sgtable callback
From: Andrii Anisov Signed-off-by: Andrii Anisov --- arch/arm/xen/mm.c | 11 +++ 1 file changed, 11 insertions(+) diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c index 050cf34..4061dd0 100644 --- a/arch/arm/xen/mm.c +++ b/arch/arm/xen/mm.c @@ -176,6 +176,16 @@ static int xen_swiotlb_dma_mmap(struct device *dev, struct vm_area_struct *vma, return dma_common_mmap(dev, vma, cpu_addr, dma_addr, size); } +static int xen_swiotlb_get_sgtable(struct device *dev, struct sg_table *sgt, +void *cpu_addr, dma_addr_t handle, size_t size, +struct dma_attrs *attrs) +{ + if (__generic_dma_ops(dev)->get_sgtable) + return __generic_dma_ops(dev)->get_sgtable(dev, sgt, cpu_addr, handle, +size, attrs); + return dma_common_get_sgtable(dev, sgt, cpu_addr, handle, size); +} + int xen_create_contiguous_region(phys_addr_t pstart, unsigned int order, unsigned int address_bits, dma_addr_t *dma_handle) @@ -213,6 +223,7 @@ static struct dma_map_ops xen_swiotlb_dma_ops = { .dma_supported = xen_swiotlb_dma_supported, .set_dma_mask = xen_swiotlb_set_dma_mask, .mmap = xen_swiotlb_dma_mmap, + .get_sgtable = xen_swiotlb_get_sgtable, }; int __init xen_mm_init(void) -- 2.7.4 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 3/3] xen: optimize xenbus driver for multiple concurrent xenstore accesses
On 10/01/17 17:36, Boris Ostrovsky wrote: > +static int process_msg(void) +{ + static struct xs_thread_state_read state; + struct xb_req_data *req; + int err; + unsigned int len; + + if (!state.in_msg) { + state.in_msg = true; + state.in_hdr = true; + state.used = 0; + + /* + * We must disallow save/restore while reading a message. + * A partial read across s/r leaves us out of sync with + * xenstored. + */ + mutex_lock(&xs_response_mutex); + + if (!xb_data_to_read()) { + /* We raced with save/restore: pending data 'gone'. */ + mutex_unlock(&xs_response_mutex); + state.in_msg = false; > > Just noticed: should in_hdr be set to false here as well? Doesn't matter: It is valid only if in_msg is true. > + return 0; + } > > Or set it to true here. > + } + + if (state.in_hdr) { + if (state.used != sizeof(state.msg)) { + err = xb_read((void *)&state.msg + state.used, +sizeof(state.msg) - state.used); + if (err < 0) + goto out; + state.used += err; + if (state.used != sizeof(state.msg)) + return 0; >>> Would it be possible to do locking at the caller? I understand that you >>> are trying to hold the lock across multiple invocations of this function >>> but it feels somewhat counter-intuitive and bug-prone. >> I think that would be difficult. >> >>> If it's not possible then at least please add a comment explaining >>> locking algorithm. >> Okay. Something like: >> >> /* >> * xs_response_mutex is locked as long as we are processing one >> * message. state.in_msg will be true as long as we are holding the >> * lock in process_msg(). > > > Then in_msg is the same as mutex_is_locked(&xs_response_mutex). And if > so, do we really need it? Yes. xs_response_mutex is used in suspend path, too. Juergen ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] Ubuntu 16.04.1 LTS kernel 4.4.0-57 over-allocation and xen-access fail
On Tue, Jan 10, 2017 at 9:34 AM, Razvan Cojocaru wrote: > On 01/10/2017 06:29 PM, Tamas K Lengyel wrote: > > > > > > On Tue, Jan 10, 2017 at 8:35 AM, Razvan Cojocaru > > mailto:rcojoc...@bitdefender.com>> wrote: > > > > >>> What I meant was taking out this call: > > >>> > > >>> /* Remove the ring_pfn from the guest's physmap */ > > >>> rc1 = xc_domain_decrease_reservation_exact(xch, domain_id, > 1, 0, > > >>> &ring_pfn); > > >>> if ( rc1 != 0 ) > > >>> PERROR("Failed to remove ring page from guest physmap"); > > >>> > > >>> To leave the frame in the guest physmap. The issue is > fundamentally > > >>> that after this frame has been taken out, something kicks the VM > to > > >>> realise it has an extra frame of balloonable space, which it > clearly > > >>> compensates for. > > >>> > > >>> You can work around the added attack surface by marking it RO in > > EPT; > > >>> neither Xen's nor dom0's mappings are translated via EPT, so > > they can > > >>> still make updates, but the guest won't be able to write to it. > > >>> > > >>> I should say that this is all a gross hack, and is in desperate > > need of > > >>> a proper API to make rings entirely outside of the gfn space, > > but this > > >>> hack should work for now. > > >> Thanks! So far, it seems to work like a charm like this: > > > > > > Great. > > > > > >> > > >> diff --git a/tools/libxc/xc_vm_event.c b/tools/libxc/xc_vm_event.c > > >> index 2fef96a..5dd00a6 100644 > > >> --- a/tools/libxc/xc_vm_event.c > > >> +++ b/tools/libxc/xc_vm_event.c > > >> @@ -130,9 +130,17 @@ void *xc_vm_event_enable(xc_interface *xch, > > domid_t > > >> domain_id, int param, > > >> } > > >> > > >> /* Remove the ring_pfn from the guest's physmap */ > > >> +/* > > >> rc1 = xc_domain_decrease_reservation_exact(xch, domain_id, > 1, 0, > > >> &ring_pfn); > > >> if ( rc1 != 0 ) > > >> PERROR("Failed to remove ring page from guest physmap"); > > >> +*/ > > >> + > > >> +if ( xc_set_mem_access(xch, domain_id, XENMEM_access_r, > > mmap_pfn, 1) ) > > >> +{ > > >> +PERROR("Could not set ring page read-only\n"); > > >> +goto out; > > >> +} > > >> > > >> out: > > >> saved_errno = errno; > > >> > > >> Should I send this as a patch for mainline as well? > > > > > > Probably a good idea, although I would include a code comment > > explaining > > > what is going on, because this is subtle if you don't know the > > context. > > > > Will do, I'll send a patch out as soon as we've done a few more > rounds > > of testing. > > > > > > (replying to all): I'm not in favor of this patch mainly because it is > > not stealthy. A malicious kernel could easily track what events are > > being sent on the ring. With DRAKVUF I could work around this using > > altp2m pfn-remapping, but for other tools this is can be a serious > > information leak. > > I understand your point, however the alternative is potential lack of > availability to monitor which is arguably a more severe problem. _Any_ > guest could choose to do what this Ubuntu 16.04 guest does, and then > connecting to the guest via vm_event can only be done once. > IMHO in that case you should implement your own internal version of this function to fall back to instead of forcing all tools to go down this path. The requirements to do that in your own tool are accessible so there is no need to push that into libxc. Tamas ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 3/3] xen: optimize xenbus driver for multiple concurrent xenstore accesses
>> /* >> * xs_response_mutex is locked as long as we are processing one >> * message. state.in_msg will be true as long as we are holding the >> * lock in process_msg(). > > Then in_msg is the same as mutex_is_locked(&xs_response_mutex). And if > so, do we really need it? Nevermind this. The lock can be held by others, obviously. -boris ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 3/3] xen: optimize xenbus driver for multiple concurrent xenstore accesses
>>> +static int process_msg(void) >>> +{ >>> + static struct xs_thread_state_read state; >>> + struct xb_req_data *req; >>> + int err; >>> + unsigned int len; >>> + >>> + if (!state.in_msg) { >>> + state.in_msg = true; >>> + state.in_hdr = true; >>> + state.used = 0; >>> + >>> + /* >>> +* We must disallow save/restore while reading a message. >>> +* A partial read across s/r leaves us out of sync with >>> +* xenstored. >>> +*/ >>> + mutex_lock(&xs_response_mutex); >>> + >>> + if (!xb_data_to_read()) { >>> + /* We raced with save/restore: pending data 'gone'. */ >>> + mutex_unlock(&xs_response_mutex); >>> + state.in_msg = false; Just noticed: should in_hdr be set to false here as well? >>> + return 0; >>> + } Or set it to true here. >>> + } >>> + >>> + if (state.in_hdr) { >>> + if (state.used != sizeof(state.msg)) { >>> + err = xb_read((void *)&state.msg + state.used, >>> + sizeof(state.msg) - state.used); >>> + if (err < 0) >>> + goto out; >>> + state.used += err; >>> + if (state.used != sizeof(state.msg)) >>> + return 0; >> Would it be possible to do locking at the caller? I understand that you >> are trying to hold the lock across multiple invocations of this function >> but it feels somewhat counter-intuitive and bug-prone. > I think that would be difficult. > >> If it's not possible then at least please add a comment explaining >> locking algorithm. > Okay. Something like: > > /* > * xs_response_mutex is locked as long as we are processing one > * message. state.in_msg will be true as long as we are holding the > * lock in process_msg(). Then in_msg is the same as mutex_is_locked(&xs_response_mutex). And if so, do we really need it? -boris ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] Ubuntu 16.04.1 LTS kernel 4.4.0-57 over-allocation and xen-access fail
On 01/10/2017 06:29 PM, Tamas K Lengyel wrote: > > > On Tue, Jan 10, 2017 at 8:35 AM, Razvan Cojocaru > mailto:rcojoc...@bitdefender.com>> wrote: > > >>> What I meant was taking out this call: > >>> > >>> /* Remove the ring_pfn from the guest's physmap */ > >>> rc1 = xc_domain_decrease_reservation_exact(xch, domain_id, 1, 0, > >>> &ring_pfn); > >>> if ( rc1 != 0 ) > >>> PERROR("Failed to remove ring page from guest physmap"); > >>> > >>> To leave the frame in the guest physmap. The issue is fundamentally > >>> that after this frame has been taken out, something kicks the VM to > >>> realise it has an extra frame of balloonable space, which it clearly > >>> compensates for. > >>> > >>> You can work around the added attack surface by marking it RO in > EPT; > >>> neither Xen's nor dom0's mappings are translated via EPT, so > they can > >>> still make updates, but the guest won't be able to write to it. > >>> > >>> I should say that this is all a gross hack, and is in desperate > need of > >>> a proper API to make rings entirely outside of the gfn space, > but this > >>> hack should work for now. > >> Thanks! So far, it seems to work like a charm like this: > > > > Great. > > > >> > >> diff --git a/tools/libxc/xc_vm_event.c b/tools/libxc/xc_vm_event.c > >> index 2fef96a..5dd00a6 100644 > >> --- a/tools/libxc/xc_vm_event.c > >> +++ b/tools/libxc/xc_vm_event.c > >> @@ -130,9 +130,17 @@ void *xc_vm_event_enable(xc_interface *xch, > domid_t > >> domain_id, int param, > >> } > >> > >> /* Remove the ring_pfn from the guest's physmap */ > >> +/* > >> rc1 = xc_domain_decrease_reservation_exact(xch, domain_id, 1, 0, > >> &ring_pfn); > >> if ( rc1 != 0 ) > >> PERROR("Failed to remove ring page from guest physmap"); > >> +*/ > >> + > >> +if ( xc_set_mem_access(xch, domain_id, XENMEM_access_r, > mmap_pfn, 1) ) > >> +{ > >> +PERROR("Could not set ring page read-only\n"); > >> +goto out; > >> +} > >> > >> out: > >> saved_errno = errno; > >> > >> Should I send this as a patch for mainline as well? > > > > Probably a good idea, although I would include a code comment > explaining > > what is going on, because this is subtle if you don't know the > context. > > Will do, I'll send a patch out as soon as we've done a few more rounds > of testing. > > > (replying to all): I'm not in favor of this patch mainly because it is > not stealthy. A malicious kernel could easily track what events are > being sent on the ring. With DRAKVUF I could work around this using > altp2m pfn-remapping, but for other tools this is can be a serious > information leak. I understand your point, however the alternative is potential lack of availability to monitor which is arguably a more severe problem. _Any_ guest could choose to do what this Ubuntu 16.04 guest does, and then connecting to the guest via vm_event can only be done once. Thanks, Razvan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [qemu-mainline test] 104098: tolerable FAIL - PUSHED
flight 104098 qemu-mainline real [real] http://logs.test-lab.xenproject.org/osstest/logs/104098/ Failures :-/ but no regressions. Regressions which are regarded as allowable (not blocking): test-armhf-armhf-libvirt-xsm 13 saverestore-support-checkfail like 104086 test-armhf-armhf-libvirt 13 saverestore-support-checkfail like 104086 test-amd64-amd64-xl-qemuu-win7-amd64 16 guest-stopfail like 104086 test-amd64-i386-xl-qemuu-win7-amd64 16 guest-stop fail like 104086 test-armhf-armhf-libvirt-qcow2 12 saverestore-support-check fail like 104086 test-armhf-armhf-libvirt-raw 12 saverestore-support-checkfail like 104086 test-armhf-armhf-xl-rtds 15 guest-start/debian.repeatfail like 104086 test-amd64-amd64-xl-rtds 9 debian-install fail like 104086 Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-pvh-amd 11 guest-start fail never pass test-amd64-amd64-xl-pvh-intel 11 guest-start fail never pass test-amd64-amd64-libvirt 12 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 12 migrate-support-checkfail never pass test-amd64-i386-libvirt 12 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 12 migrate-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check fail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check fail never pass test-amd64-amd64-libvirt-vhd 11 migrate-support-checkfail never pass test-amd64-amd64-qemuu-nested-amd 16 debian-hvm-install/l1/l2 fail never pass test-armhf-armhf-xl-credit2 12 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-xsm 12 migrate-support-checkfail never pass test-armhf-armhf-xl-xsm 13 saverestore-support-checkfail never pass test-armhf-armhf-xl 12 migrate-support-checkfail never pass test-armhf-armhf-xl 13 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-xsm 12 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 12 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 13 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 12 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 12 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 13 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 11 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 11 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 12 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 12 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-vhd 11 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 12 saverestore-support-checkfail never pass version targeted for testing: qemuuf634151b02ce5c80605383894f1f63f2c12e0033 baseline version: qemuu77424a452abe5f941d8cd81f1e85f42bca31c9ef Last test of basis 104086 2017-01-09 18:41:57 Z0 days Testing same since 104098 2017-01-10 11:11:00 Z0 days1 attempts People who touched revisions under test: Aurelien Jarno James Hogan Jin Guojie Peter Maydell Richard Henderson YunQiang Su jobs: build-amd64-xsm pass build-armhf-xsm pass build-i386-xsm pass build-amd64 pass build-armhf pass build-i386 pass build-amd64-libvirt pass build-armhf-libvirt pass build-i386-libvirt pass build-amd64-pvopspass build-armhf-pvopspass build-i386-pvops pass test-amd64-amd64-xl pass test-armhf-armhf-xl pass test-amd64-i386-xl pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm pass test-amd64-i386-libvirt-qemuu
Re: [Xen-devel] [edk2] [PATCH RFC 06/14] OvmfPkg/XenPlatformPei: Add xen PVH specific code
On Thu, Jan 05, 2017 at 11:30:32AM +0100, Laszlo Ersek wrote: > On 12/08/16 16:33, Anthony PERARD wrote: > > - learn about memory size from the E820 > > - ignore error if host bridge devid is 0x, PVH does not have PCI and > > reading from unexisting device return -1. > > > > Contributed-under: TianoCore Contribution Agreement 1.0 > > Signed-off-by: Anthony PERARD > > --- > > OvmfPkg/XenPlatformPei/MemDetect.c | 71 > > ++ > > OvmfPkg/XenPlatformPei/Platform.c | 5 +++ > > OvmfPkg/XenPlatformPei/Platform.h | 10 ++ > > 3 files changed, 86 insertions(+) > > > > diff --git a/OvmfPkg/XenPlatformPei/MemDetect.c > > b/OvmfPkg/XenPlatformPei/MemDetect.c > > index 4ecdf5e..0d80775 100644 > > --- a/OvmfPkg/XenPlatformPei/MemDetect.c > > +++ b/OvmfPkg/XenPlatformPei/MemDetect.c > > @@ -42,6 +42,70 @@ UINT8 mPhysMemAddressWidth; > > STATIC UINT32 mS3AcpiReservedMemoryBase; > > STATIC UINT32 mS3AcpiReservedMemorySize; > > > > +STATIC UINT32 mXenLowerMemorySize = 0; > > +STATIC UINT64 mXenHighMemorySize = 0; > > + > > +VOID > > +XenReadMemorySizes ( > > + VOID > > + ) > > +{ > > + EFI_E820_ENTRY64 *E820Map; > > + UINT32E820EntriesCount; > > + EFI_STATUS Status; > > + > > + Status = XenGetE820Map (&E820Map, &E820EntriesCount); > > + ASSERT_EFI_ERROR (Status); > > + > > + mXenLowerMemorySize = 0; > > + mXenHighMemorySize = 0; > > + > > + if (E820EntriesCount > 0) { > > +EFI_E820_ENTRY64 *Entry; > > +UINT32 Loop; > > + > > +for (Loop = 0; Loop < E820EntriesCount; Loop++) { > > + Entry = E820Map + Loop; > > + > > + // > > + // Only care about RAM > > + // > > + if (Entry->Type != EfiAcpiAddressRangeMemory) { > > +continue; > > + } > > + > > + if ((Entry->BaseAddr + Entry->Length) <= SIZE_16MB) { > > +continue; > > + } > > + > > + if (Entry->BaseAddr < SIZE_16MB) { > > +UINT64 bottom = Entry->BaseAddr; > > +UINT64 size = Entry->Length; > > +size -= SIZE_16MB - bottom; > > +bottom = SIZE_16MB; > > +mXenLowerMemorySize += size; > > +continue; > > + } > > + if (Entry->BaseAddr <= SIZE_4GB) { > > +UINT64 size = Entry->Length; > > +mXenLowerMemorySize += size; > > +continue; > > + } > > + > > + if (Entry->BaseAddr == SIZE_4GB) { > > +mXenHighMemorySize = Entry->Length; > > +continue; > > + } > > + > > + DEBUG ((EFI_D_INFO, "%a: ignored XenE820 entry (0x%llx, 0x%llx)\n", > > + __FUNCTION__, > > + Entry->BaseAddr, > > + Entry->Length)); > > +} > > +mXenLowerMemorySize += SIZE_16MB; > > + } > > +} > > + > > UINT32 > > GetSystemMemorySizeBelow4gb ( > >VOID > > @@ -50,6 +114,9 @@ GetSystemMemorySizeBelow4gb ( > >UINT8 Cmos0x34; > >UINT8 Cmos0x35; > > > > + if (mXen && mXenLowerMemorySize) > > +return mXenLowerMemorySize; > > + > >// > >// CMOS 0x34/0x35 specifies the system memory above 16 MB. > >// * CMOS(0x35) is the high byte > > @@ -74,6 +141,10 @@ GetSystemMemorySizeAbove4gb ( > >UINT32 Size; > >UINTN CmosIndex; > > > > + // if lower memory is specified that way, return also high memory > > + if (mXen && mXenLowerMemorySize) > > +return mXenHighMemorySize; > > + > >// > >// CMOS 0x5b-0x5d specifies the system memory above 4GB MB. > >// * CMOS(0x5d) is the most significant size byte > > diff --git a/OvmfPkg/XenPlatformPei/Platform.c > > b/OvmfPkg/XenPlatformPei/Platform.c > > index bf78878..9fc713c 100644 > > --- a/OvmfPkg/XenPlatformPei/Platform.c > > +++ b/OvmfPkg/XenPlatformPei/Platform.c > > @@ -362,6 +362,10 @@ MiscInitialization ( > >AcpiCtlReg = POWER_MGMT_REGISTER_Q35 (ICH9_ACPI_CNTL); > >AcpiEnBit = ICH9_ACPI_CNTL_ACPI_EN; > >break; > > +case 0x: > > + // xen PVH, ignore > > + PcdStatus = PcdSet16S (PcdOvmfHostBridgePciDevId, mHostBridgeDevId); > > + return; > > Can we create a new macro for 0x in > "OvmfPkg/Include/OvmfPlatforms.h", and use that here? > > Normally I would suggest a separate header file under "OvmfPkg/Include", > similar to "Q35MchIch9.h" and "I440FxPiix4.h", and to include that new > file in "OvmfPlatforms.h", but here we only need a single "chipset > identifier", so I guess that can go directly into "OvmfPlatforms.h". > > I mainly suggest this because in patch 08/14, the same 0x case label > is being added to code shared with QEMU, and using a verbose macro there > is much better than a magic number. In turn, we should use the same > macro here, I think. This value is just the result of a read to an incorrect location, and the return value is -1. There is no PCI when running in Xen PVH, at least for now. I think I should try harder to find out if OVMF is running in PVH, and so I would not need this case 0x at all. -- Anthony PERARD
Re: [Xen-devel] Ubuntu 16.04.1 LTS kernel 4.4.0-57 over-allocation and xen-access fail
On Tue, Jan 10, 2017 at 8:35 AM, Razvan Cojocaru wrote: > >>> What I meant was taking out this call: > >>> > >>> /* Remove the ring_pfn from the guest's physmap */ > >>> rc1 = xc_domain_decrease_reservation_exact(xch, domain_id, 1, 0, > >>> &ring_pfn); > >>> if ( rc1 != 0 ) > >>> PERROR("Failed to remove ring page from guest physmap"); > >>> > >>> To leave the frame in the guest physmap. The issue is fundamentally > >>> that after this frame has been taken out, something kicks the VM to > >>> realise it has an extra frame of balloonable space, which it clearly > >>> compensates for. > >>> > >>> You can work around the added attack surface by marking it RO in EPT; > >>> neither Xen's nor dom0's mappings are translated via EPT, so they can > >>> still make updates, but the guest won't be able to write to it. > >>> > >>> I should say that this is all a gross hack, and is in desperate need of > >>> a proper API to make rings entirely outside of the gfn space, but this > >>> hack should work for now. > >> Thanks! So far, it seems to work like a charm like this: > > > > Great. > > > >> > >> diff --git a/tools/libxc/xc_vm_event.c b/tools/libxc/xc_vm_event.c > >> index 2fef96a..5dd00a6 100644 > >> --- a/tools/libxc/xc_vm_event.c > >> +++ b/tools/libxc/xc_vm_event.c > >> @@ -130,9 +130,17 @@ void *xc_vm_event_enable(xc_interface *xch, > domid_t > >> domain_id, int param, > >> } > >> > >> /* Remove the ring_pfn from the guest's physmap */ > >> +/* > >> rc1 = xc_domain_decrease_reservation_exact(xch, domain_id, 1, 0, > >> &ring_pfn); > >> if ( rc1 != 0 ) > >> PERROR("Failed to remove ring page from guest physmap"); > >> +*/ > >> + > >> +if ( xc_set_mem_access(xch, domain_id, XENMEM_access_r, mmap_pfn, > 1) ) > >> +{ > >> +PERROR("Could not set ring page read-only\n"); > >> +goto out; > >> +} > >> > >> out: > >> saved_errno = errno; > >> > >> Should I send this as a patch for mainline as well? > > > > Probably a good idea, although I would include a code comment explaining > > what is going on, because this is subtle if you don't know the context. > > Will do, I'll send a patch out as soon as we've done a few more rounds > of testing. > (replying to all): I'm not in favor of this patch mainly because it is not stealthy. A malicious kernel could easily track what events are being sent on the ring. With DRAKVUF I could work around this using altp2m pfn-remapping, but for other tools this is can be a serious information leak. Tamas ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3] x86/HVM: Fix teardown ordering in hvm_vcpu_destroy()
On 1/10/17 21:26, Andrew Cooper wrote: On 10/01/17 14:15, Andrew Cooper wrote: On 10/01/17 14:03, Suravee Suthikulpanit wrote: The order of destroy function calls in hvm_vcpu_destroy() should be the reverse of init calls in hvm_vcpu_initialise(). Signed-off-by: Suravee Suthikulpanit Reviewed-by: Konrad Rzeszutek Wilk Reviewed-by: Kevin Tian Cc: Jan Beulich Cc: Boris Ostrovsky Reviewed-by: Andrew Cooper and queued. Wait no. The order in vcpu_initialise is hvm_vcpu_cacheattr_init() vlapic_init() hvm_funcs.vcpu_initialise() softirq_tasklet_init() setup_compat_arg_xlat() Therefore, moving the tasklet_kill() is wrong. The overall delta should be: andrewcoop@andrewcoop:/local/xen.git/xen$ git diff HEAD^ diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 4c0f561..9f74334 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -1626,12 +1626,12 @@ void hvm_vcpu_destroy(struct vcpu *v) free_compat_arg_xlat(v); tasklet_kill(&v->arch.hvm_vcpu.assert_evtchn_irq_tasklet); -hvm_vcpu_cacheattr_destroy(v); +hvm_funcs.vcpu_destroy(v); if ( is_hvm_vcpu(v) ) vlapic_destroy(v); -hvm_funcs.vcpu_destroy(v); +hvm_vcpu_cacheattr_destroy(v); } IIRC. If you agree, I will fold this correction in while committing. ~Andrew Ah, right... sorry I missed the tasklet stuff. Thanks, Suravee ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [edk2] [PATCH RFC 04/14] OvmfPkg: Introduce XenPlatformPei
On Thu, Jan 05, 2017 at 10:59:24AM +0100, Laszlo Ersek wrote: > On 12/08/16 16:33, Anthony PERARD wrote: > > A copy of OvmfPkg/PlatformPei without some of QEMU specific > > initialization, Xen does not support QemuFwCfg. > > > > Contributed-under: TianoCore Contribution Agreement 1.0 > > Signed-off-by: Anthony PERARD > > --- > > OvmfPkg/XenOvmf.dsc | 2 +- > > OvmfPkg/XenOvmf.fdf | 2 +- > > OvmfPkg/XenPlatformPei/Cmos.c | 64 > > OvmfPkg/XenPlatformPei/Cmos.h | 56 > > OvmfPkg/XenPlatformPei/Fv.c | 100 ++ > > OvmfPkg/XenPlatformPei/MemDetect.c| 449 + > > OvmfPkg/XenPlatformPei/Platform.c | 536 > > ++ > > OvmfPkg/XenPlatformPei/Platform.h | 104 ++ > > OvmfPkg/XenPlatformPei/Xen.c | 231 + > > OvmfPkg/XenPlatformPei/Xen.h | 45 +++ > > OvmfPkg/XenPlatformPei/XenPlatformPei.inf | 110 ++ > > 11 files changed, 1697 insertions(+), 2 deletions(-) > > create mode 100644 OvmfPkg/XenPlatformPei/Cmos.c > > create mode 100644 OvmfPkg/XenPlatformPei/Cmos.h > > create mode 100644 OvmfPkg/XenPlatformPei/Fv.c > > create mode 100644 OvmfPkg/XenPlatformPei/MemDetect.c > > create mode 100644 OvmfPkg/XenPlatformPei/Platform.c > > create mode 100644 OvmfPkg/XenPlatformPei/Platform.h > > create mode 100644 OvmfPkg/XenPlatformPei/Xen.c > > create mode 100644 OvmfPkg/XenPlatformPei/Xen.h > > create mode 100644 OvmfPkg/XenPlatformPei/XenPlatformPei.inf > > (1) You might want to add Citrix copyright notices to new files. > > (2) This module is a good example where the moved Xen functionality > (even for HVM guests) should be possible to remove from the original > PlatformPei module. (In a later, final patch.) Is that right? Yes, that should be possible. > (3) In the commit message, please list the removed fw_cfg-dependent > functionality in more detail (all of which is dynamically skipped when > the current PlatformPei module runs on Xen): Will do. > - GetFirstNonAddress(): controlling the 64-bit PCI MMIO aperture via the > (experimental) "opt/ovmf/X-PciMmio64Mb" file > > - GetFirstNonAddress(): honoring the hotplug DIMM area > ("etc/reserved-memory-end") in the placement of the 64-bit PCI MMIO aperture > > - NoexecDxeInitialization() is removed, so PcdPropertiesTableEnable and > PcdSetNxForStack are left constant FALSE (not set dynamically from > "opt/ovmf/PcdXxxx") > > - MaxCpuCountInitialization(), PublishPeiMemory(): the max CPU count is > not taken from the QemuFwCfgItemSmpCpuCount fw_cfg key; > PcdCpuMaxLogicalProcessorNumber is used intact and > PcdCpuApInitTimeOutInMicroSeconds is never changed or used. > > - InitializeXenPlatform(), S3Verification(): S3 is assumed disabled (not > consulting "etc/system-states" via QemuFwCfgS3Enabled()). > > - InstallFeatureControlCallback(): the feature control MSR is not set > from "etc/msr_feature_control" > > Also removed: > - SMRAM/TSEG-related low mem size adjusting (PcdSmmSmramRequire is > assumed FALSE) in PublishPeiMemory(), > - QemuInitializeRam() entirely, > > (4) I think the removal of PcdSmmSmramRequire is incomplete. IMO this > PCD should either be removed completely (all uses, including the INF > reference), assuming a FALSE value in its place, or the PCD should not > be touched at all. The FeaturePcdGet() call in PublishPeiMemory() -- > gating the "TSEG is chipped from the end of low RAM" stuff -- seems to > be singled out for removal for no good reason. Yes, I think I will remove all of if. There is not much reason to keep something that is not tested, and I don't know if it can work on Xen. I guess SMM can be readded later. > (5) It would be nice to hint at the reason (which is implemented in a > later patch) why a separate module is a good idea here. That is, why the > expected PVH2 functionality is best not added to the current PlatformPei > module. Will do. -- Anthony PERARD ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [distros-debian-snapshot test] 68350: tolerable FAIL
flight 68350 distros-debian-snapshot real [real] http://osstest.xs.citrite.net/~osstest/testlogs/logs/68350/ Failures :-/ but no regressions. Regressions which are regarded as allowable (not blocking): test-amd64-amd64-amd64-weekly-netinst-pygrub 9 debian-di-install fail like 68308 test-armhf-armhf-armhf-daily-netboot-pygrub 9 debian-di-install fail like 68308 test-amd64-i386-i386-daily-netboot-pvgrub 9 debian-di-install fail like 68308 test-amd64-i386-amd64-daily-netboot-pygrub 9 debian-di-install fail like 68308 test-amd64-amd64-i386-daily-netboot-pygrub 9 debian-di-install fail like 68308 test-amd64-amd64-amd64-daily-netboot-pvgrub 9 debian-di-install fail like 68308 test-amd64-amd64-i386-weekly-netinst-pygrub 9 debian-di-install fail like 68308 test-amd64-i386-i386-weekly-netinst-pygrub 9 debian-di-install fail like 68308 test-amd64-i386-amd64-weekly-netinst-pygrub 9 debian-di-install fail like 68308 baseline version: flight 68308 jobs: build-amd64 pass build-armhf pass build-i386 pass build-amd64-pvopspass build-armhf-pvopspass build-i386-pvops pass test-amd64-amd64-amd64-daily-netboot-pvgrub fail test-amd64-i386-i386-daily-netboot-pvgrubfail test-amd64-i386-amd64-daily-netboot-pygrub fail test-armhf-armhf-armhf-daily-netboot-pygrub fail test-amd64-amd64-i386-daily-netboot-pygrub fail test-amd64-amd64-amd64-current-netinst-pygrubpass test-amd64-i386-amd64-current-netinst-pygrub pass test-amd64-amd64-i386-current-netinst-pygrub pass test-amd64-i386-i386-current-netinst-pygrub pass test-amd64-amd64-amd64-weekly-netinst-pygrub fail test-amd64-i386-amd64-weekly-netinst-pygrub fail test-amd64-amd64-i386-weekly-netinst-pygrub fail test-amd64-i386-i386-weekly-netinst-pygrub fail sg-report-flight on osstest.xs.citrite.net logs: /home/osstest/logs images: /home/osstest/images Logs, config files, etc. are available at http://osstest.xs.citrite.net/~osstest/testlogs/logs Test harness code can be found at http://xenbits.xensource.com/gitweb?p=osstest.git;a=summary Push not applicable. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH 1/2] tools/xenstore: start with empty data base
Today xenstored tries to open a tdb data base file on disk when it is started. As this is problematic in most cases the scripts used to start xenstored ensure xenstored won't find such a file in order to start with an empty xenstore. A tdb data base file can't be used to restore all Xenstore state as e.g. Xenstore watches are not kept in the tdb data base. The file is meant to be used for debugging purposes after a xenstored crash only. Instead of opening a Xenstore data base file found on disk always start with an empty data base. This will avoid problems in case someone is testing multiple xenstored versions without rebooting (which is not supported but helps debugging in some cases). Signed-off-by: Juergen Gross --- tools/xenstore/xenstored_core.c | 60 +++-- 1 file changed, 9 insertions(+), 51 deletions(-) diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c index 3770056..3dc06d4 100644 --- a/tools/xenstore/xenstored_core.c +++ b/tools/xenstore/xenstored_core.c @@ -1157,17 +1157,6 @@ static int _rm(struct connection *conn, const void *ctx, struct node *node, } -static void internal_rm(const char *name) -{ - char *tname = talloc_strdup(NULL, name); - struct node *node = read_node(NULL, tname, tname); - if (node) - _rm(NULL, tname, node, tname); - talloc_free(node); - talloc_free(tname); -} - - static int do_rm(struct connection *conn, struct buffered_data *in) { struct node *node; @@ -1582,49 +1571,18 @@ static void setup_structure(void) barf_perror("Could not create tdbname"); if (!(tdb_flags & TDB_INTERNAL)) - tdb_ctx = tdb_open_ex(tdbname, 0, tdb_flags, O_RDWR, 0, - &tdb_logger, NULL); + unlink(tdbname); - if (tdb_ctx) { - /* XXX When we make xenstored able to restart, this will have - to become cleverer, checking for existing domains and not - removing the corresponding entries, but for now xenstored - cannot be restarted without losing all the registered - watches, which breaks all the backend drivers anyway. We - can therefore get away with just clearing /local and - expecting Xend to put the appropriate entries back in. + tdb_ctx = tdb_open_ex(tdbname, 7919, tdb_flags, O_RDWR|O_CREAT|O_EXCL, + 0640, &tdb_logger, NULL); + if (!tdb_ctx) + barf_perror("Could not create tdb file %s", tdbname); - When this change is made it is important to note that - dom0's entries must be cleaned up on reboot _before_ this - daemon starts, otherwise the backend drivers and dom0's - balloon driver will pick up stale entries. In the case of - the balloon driver, this can be fatal. - */ - char *tlocal = talloc_strdup(NULL, "/local"); + manual_node("/", "tool"); + manual_node("/tool", "xenstored"); + manual_node("/tool/xenstored", NULL); - check_store(); - - if (remove_local) { - internal_rm("/local"); - create_node(NULL, NULL, tlocal, NULL, 0); - - check_store(); - } - - talloc_free(tlocal); - } - else { - tdb_ctx = tdb_open_ex(tdbname, 7919, tdb_flags, O_RDWR|O_CREAT, - 0640, &tdb_logger, NULL); - if (!tdb_ctx) - barf_perror("Could not create tdb file %s", tdbname); - - manual_node("/", "tool"); - manual_node("/tool", "xenstored"); - manual_node("/tool/xenstored", NULL); - - check_store(); - } + check_store(); } -- 2.10.2 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH 2/2] tools: don't remove tdb data base file before starting xenstored
As xenstored now is always starting with an empty tdb data base there is no need any longer to remove the file before starting xenstored. A rerun of autogen.sh is required. Signed-off-by: Juergen Gross --- tools/hotplug/Linux/launch-xenstore.in | 1 - 1 file changed, 1 deletion(-) diff --git a/tools/hotplug/Linux/launch-xenstore.in b/tools/hotplug/Linux/launch-xenstore.in index 01193be..991dec8 100644 --- a/tools/hotplug/Linux/launch-xenstore.in +++ b/tools/hotplug/Linux/launch-xenstore.in @@ -54,7 +54,6 @@ test -f @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons && . @CONFIG_DIR@/@CONFIG_LEAF [ "$XENSTORETYPE" = "daemon" ] && { [ -z "$XENSTORED_ROOTDIR" ] && XENSTORED_ROOTDIR="@XEN_LIB_STORED@" - /bin/rm -f $XENSTORED_ROOTDIR/tdb* &>/dev/null [ -z "$XENSTORED_TRACE" ] || XENSTORED_ARGS="$XENSTORED_ARGS -T @XEN_LOG_DIR@/xenstored-trace.log" [ -z "$XENSTORED" ] && XENSTORED=@XENSTORED@ [ -x "$XENSTORED" ] || { -- 2.10.2 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH 0/2] tools/xenstore: don't use old tdb file in xenstored
Today xenstored tries to open an existing tdb file when started. As this is known to be problematic the start script of xenstored is deleting any old tdb file. This can be made easier and less error prone by letting xenstored use a new file. Juergen Gross (2): tools/xenstore: start with empty data base tools: don't remove tdb data base file before starting xenstored tools/hotplug/Linux/launch-xenstore.in | 1 - tools/xenstore/xenstored_core.c| 60 +- 2 files changed, 9 insertions(+), 52 deletions(-) -- 2.10.2 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [edk2] [PATCH RFC 03/14] OvmfPkg/XenOvmf.dsc: Introduce XenResetVector
On Wed, Jan 04, 2017 at 08:49:15PM +0100, Laszlo Ersek wrote: > > diff --git a/OvmfPkg/XenResetVector/Ia32/PageTables64.asm > > b/OvmfPkg/XenResetVector/Ia32/PageTables64.asm > > new file mode 100644 > > index 000..b5a4cf8 > > --- /dev/null > > +++ b/OvmfPkg/XenResetVector/Ia32/PageTables64.asm > > @@ -0,0 +1,93 @@ > > +;-- > > +; @file > > +; Sets the CR3 register for 64-bit paging > > +; > > +; Copyright (c) 2008 - 2013, Intel Corporation. All rights reserved. > > +; This program and the accompanying materials > > +; are licensed and made available under the terms and conditions of the > > BSD License > > +; which accompanies this distribution. The full text of the license may > > be found at > > +; http://opensource.org/licenses/bsd-license.php > > +; > > +; THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > > +; WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR > > IMPLIED. > > +; > > +;-- > > + > > +BITS32 > > + > > +%define PAGE_PRESENT0x01 > > +%define PAGE_READ_WRITE 0x02 > > +%define PAGE_USER_SUPERVISOR0x04 > > +%define PAGE_WRITE_THROUGH 0x08 > > +%define PAGE_CACHE_DISABLE 0x010 > > +%define PAGE_ACCESSED 0x020 > > +%define PAGE_DIRTY 0x040 > > +%define PAGE_PAT 0x080 > > +%define PAGE_GLOBAL 0x0100 > > +%define PAGE_2M_MBO0x080 > > +%define PAGE_2M_PAT 0x01000 > > + > > +%define PAGE_2M_PDE_ATTR (PAGE_2M_MBO + \ > > + PAGE_ACCESSED + \ > > + PAGE_DIRTY + \ > > + PAGE_READ_WRITE + \ > > + PAGE_PRESENT) > > + > > +%define PAGE_PDP_ATTR (PAGE_ACCESSED + \ > > + PAGE_READ_WRITE + \ > > + PAGE_PRESENT) > > + > > + > > +; > > +; Modified: EAX, ECX > > +; > > +SetCr3ForPageTables64: > > + > > +; > > +; For OVMF, build some initial page tables at 0x80-0x806000. > > (6) Are you intentionally undoing, in the copy, commit 73d66c5871cc? If > so, why? No, I just need to use --find-copies-harder to find out about recent changes. > For now I suspect that you originally created this patch before commit > 73d66c5871cc came into existence. If that's the case, then please rebase > (re-copy and customize) this patch to the most recent ResetVector state, > including commit 73d66c5871cc. I don't think I've customize this file, this could be a link, or I could try to have the build system use the original file from OvmfPkg/ResetVector. Thanks, -- Anthony PERARD ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] xen: do not re-use pirq number cached in pci device msi msg data
On Mon, Jan 9, 2017 at 2:30 PM, Stefano Stabellini wrote: > On Mon, 9 Jan 2017, Konrad Rzeszutek Wilk wrote: >> On Mon, Jan 09, 2017 at 10:42:41AM -0500, Dan Streetman wrote: >> > On Mon, Jan 9, 2017 at 9:59 AM, Boris Ostrovsky >> > wrote: >> > > On 01/06/2017 08:06 PM, Konrad Rzeszutek Wilk wrote: >> > >> On Thu, Jan 05, 2017 at 02:28:56PM -0500, Dan Streetman wrote: >> > >>> Do not read a pci device's msi message data to see if a pirq was >> > >>> previously configured for the device's msi/msix, as the old pirq was >> > >>> unmapped and may now be in use by another pci device. The previous >> > >>> pirq should never be re-used; instead a new pirq should always be >> > >>> allocated from the hypervisor. >> > >> Won't this cause a starvation problem? That is we will run out of PIRQs >> > >> as we are not reusing them? >> > > >> > > Don't we free the pirq when we unmap it? >> > >> > I think this is actually a bit worse than I initially thought. After >> > looking a bit closer, and I think there's an asymmetry with pirq >> > allocation: >> >> Lets include Stefano, >> >> Thank you for digging in this! This has quite the deja-vu >> feeling as I believe I hit this at some point in the past and >> posted some possible ways of fixing this. But sadly I can't >> find the thread. > > This issue seems to be caused by: > > commit af42b8d12f8adec6711cb824549a0edac6a4ae8f > Author: Stefano Stabellini > Date: Wed Dec 1 14:51:44 2010 + > > xen: fix MSI setup and teardown for PV on HVM guests > > which was a fix to a bug: > > This fixes a bug in xen_hvm_setup_msi_irqs that manifests itself when > trying to enable the same MSI for the second time: the old MSI to pirq > mapping is still valid at this point but xen_hvm_setup_msi_irqs would > try to assign a new pirq anyway. > A simple way to reproduce this bug is to assign an MSI capable network > card to a PV on HVM guest, if the user brings down the corresponding > ethernet interface and up again, Linux would fail to enable MSIs on the > device. > > I don't remember any of the details. From the description of this bug, > it seems that Xen changed behavior in the past few years: before it used > to keep the pirq-MSI mapping, while today it doesn't. If I wrote "the > old MSI to pirq mapping is still valid at this point", the pirq couldn't > have been completely freed, then reassigned to somebody else the way it > is described in this email. > > I think we should indentify the changeset or Xen version that introduced > the new behavior. If it is old enough, we might be able to just revert > af42b8d12f8adec6711cb824549a0edac6a4ae8f. Otherwise we could make the > behavior conditional to the Xen version. Are PT devices the only MSI-capable devices available in a Xen guest? That's where I'm seeing this problem, with PT NVMe devices. I can say that on the Xen guest with NVMe PT devices I'm testing on, with the patch from this thread (which essentially reverts your commit above) as well as some added debug to see the pirq numbers, cycles of 'modprobe nvme ; rmmod nvme' don't cause pirq starvation, as the hypervisor provides essentially the same pirqs for each modprobe, since they were freed by the rmmod. > > >> > tl;dr: >> > >> > pci_enable_msix_range() -> each MSIX (or MSI) now has a pirq >> > allocated, and reserved in the hypervisor >> > >> > request_irq() -> an event channel is opened for the specific pirq, and >> > maps the pirq with the hypervisor >> > >> > free_irq() -> the event channel is closed, and the pirq is unmapped, >> > but that unmap function also frees the pirq! The hypervisor can/will >> > give it away to the next call to get_free_pirq. However, the pci >> > msi/msix data area still contains the pirq number, and the next call >> > to request_irq() will re-use the pirq. >> > >> > pci_disable_msix() -> this has no effect on the pirq in the hypervisor >> > (it's already been freed), and it also doesn't clear anything from the >> > msi/msix data area, so the pirq is still cached there. >> > >> > >> > It seems like the hypervisor needs to be fixed to *not* unmap the pirq >> > when the event channel is closed - or at least, not to change it to >> > IRQ_UNBOUND state? And, the pci_disable_msix call should eventually >> > call into something in the Xen guest kernel that actually does the >> > pirq unmapping, and clear it from the msi data area (i.e. >> > pci_write_msi_msg) >> >> The problem is that Xen changes have sailed a long long time ago. >> > >> > Alternately, if the hypervisor doesn't change, then the call into the >> > hypervisor to actually allocate a pirq needs to move from the >> > pci_enable_msix_range() call to the request_irq() call? So that when >> > the msi/msix range is enabled, it doesn't actually reserve any pirq's >> > for any of the vectors; each request_irq/free_irq pair do the pirq >> > allocate-and-map/unmap... >> >> >> Or a third one: We keep an pirq->device lookup and inhibit free_irq() >> fro
Re: [Xen-devel] [PATCH] tpm: Restore functionality to xen vtpm driver.
On 12/22/2016 03:41 PM, Dr. Greg Wettstein wrote: > Functionality of the xen-tpmfront driver was lost secondary to > the introduction of xenbus multi-page support in the following > commit: > > ccc9d90a9a8b5c4ad7e9708ec41f75ff9e98d61d > > xenbus_client: Extend interface to support multi-page ring > > In this commit a pointer to the shared page address was being > passed to the xenbus_grant_ring() function rather then the > address of the shared page itself. This resulted in a situation > where the driver would attach to the vtpm-stubdom but any attempt > to send a command to the stub domain would timeout. > > A diagnostic finding for this regression is the following error > message being generated when the xen-tpmfront driver probes for a > device: > > <3>vtpm vtpm-0: tpm_transmit: tpm_send: error -62 > > <3>vtpm vtpm-0: A TPM error (-62) occurred attempting to determine the > timeouts > > This fix is relevant to all kernels from 4.1 forward which is the > release in which multi-page xenbus support was introduced. > > Daniel De Graaf formulated the fix by code inspection after the > regression point was located. > > Signed-off-by: Dr. Greg Wettstein Greg, Are you planning to re-submit this? Thanks. -boris > --- > drivers/char/tpm/xen-tpmfront.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/char/tpm/xen-tpmfront.c b/drivers/char/tpm/xen-tpmfront.c > index 5aaa268..dd83a07 100644 > --- a/drivers/char/tpm/xen-tpmfront.c > +++ b/drivers/char/tpm/xen-tpmfront.c > @@ -203,7 +203,7 @@ static int setup_ring(struct xenbus_device *dev, struct > tpm_private *priv) > return -ENOMEM; > } > > - rv = xenbus_grant_ring(dev, &priv->shr, 1, &gref); > + rv = xenbus_grant_ring(dev, priv->shr, 1, &gref); > if (rv < 0) > return rv; > ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] Ubuntu 16.04.1 LTS kernel 4.4.0-57 over-allocation and xen-access fail
>>> What I meant was taking out this call: >>> >>> /* Remove the ring_pfn from the guest's physmap */ >>> rc1 = xc_domain_decrease_reservation_exact(xch, domain_id, 1, 0, >>> &ring_pfn); >>> if ( rc1 != 0 ) >>> PERROR("Failed to remove ring page from guest physmap"); >>> >>> To leave the frame in the guest physmap. The issue is fundamentally >>> that after this frame has been taken out, something kicks the VM to >>> realise it has an extra frame of balloonable space, which it clearly >>> compensates for. >>> >>> You can work around the added attack surface by marking it RO in EPT; >>> neither Xen's nor dom0's mappings are translated via EPT, so they can >>> still make updates, but the guest won't be able to write to it. >>> >>> I should say that this is all a gross hack, and is in desperate need of >>> a proper API to make rings entirely outside of the gfn space, but this >>> hack should work for now. >> Thanks! So far, it seems to work like a charm like this: > > Great. > >> >> diff --git a/tools/libxc/xc_vm_event.c b/tools/libxc/xc_vm_event.c >> index 2fef96a..5dd00a6 100644 >> --- a/tools/libxc/xc_vm_event.c >> +++ b/tools/libxc/xc_vm_event.c >> @@ -130,9 +130,17 @@ void *xc_vm_event_enable(xc_interface *xch, domid_t >> domain_id, int param, >> } >> >> /* Remove the ring_pfn from the guest's physmap */ >> +/* >> rc1 = xc_domain_decrease_reservation_exact(xch, domain_id, 1, 0, >> &ring_pfn); >> if ( rc1 != 0 ) >> PERROR("Failed to remove ring page from guest physmap"); >> +*/ >> + >> +if ( xc_set_mem_access(xch, domain_id, XENMEM_access_r, mmap_pfn, 1) ) >> +{ >> +PERROR("Could not set ring page read-only\n"); >> +goto out; >> +} >> >> out: >> saved_errno = errno; >> >> Should I send this as a patch for mainline as well? > > Probably a good idea, although I would include a code comment explaining > what is going on, because this is subtle if you don't know the context. Will do, I'll send a patch out as soon as we've done a few more rounds of testing. Thanks again, Razvan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] Ubuntu 16.04.1 LTS kernel 4.4.0-57 over-allocation and xen-access fail
On 10/01/17 15:02, Razvan Cojocaru wrote: > On 01/10/2017 04:13 PM, Andrew Cooper wrote: >> On 10/01/17 09:06, Razvan Cojocaru wrote: >>> On 01/09/2017 02:54 PM, Andrew Cooper wrote: On 09/01/17 11:36, Razvan Cojocaru wrote: > Hello, > > We've come across a weird phenomenon: an Ubuntu 16.04.1 LTS HVM guest > running kernel 4.4.0 installed via XenCenter in XenServer Dundee seems > to eat up all the RAM it can: > > (XEN) [ 394.379760] d1v1 Over-allocation for domain 1: 524545 > 524544 > > This leads to a problem with xen-access, specifically libxc which does > this in xc_vm_event_enable() (this is Xen 4.6): > > ring_page = xc_map_foreign_batch(xch, domain_id, PROT_READ | PROT_WRITE, > &mmap_pfn, 1); > > if ( mmap_pfn & XEN_DOMCTL_PFINFO_XTAB ) > { > /* Map failed, populate ring page */ > rc1 = xc_domain_populate_physmap_exact(xch, domain_id, 1, 0, 0, >&ring_pfn); > if ( rc1 != 0 ) > { > PERROR("Failed to populate ring pfn\n"); > goto out; > } > > The first time everything works fine, xen-access can map the ring page. > But most of the time the second time fails in the > xc_domain_populate_physmap_exact() call, and again this is dumped in the > Xen log (once for each failed attempt): > > (XEN) [ 395.952188] d0v3 Over-allocation for domain 1: 524545 > 524544 Thinking further about this, what happens if you avoid removing the page on exit? The first populate succeeds, and if you leave the page populated, the second time you come around the loop, it should not be of type XTAB, and the map should succeed. >>> Sorry for the late reply, had to put out another fire yesterday. >>> >>> I've taken your recommendation to roughly mean this: >>> >>> diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c >>> index ba9690a..805564b 100644 >>> --- a/xen/common/vm_event.c >>> +++ b/xen/common/vm_event.c >>> @@ -100,8 +100,11 @@ static int vm_event_enable( >>> return 0; >>> >>> err: >>> +/* >>> destroy_ring_for_helper(&ved->ring_page, >>> ved->ring_pg_struct); >>> +*/ >>> +ved->ring_page = NULL; >>> vm_event_ring_unlock(ved); >>> >>> return rc; >>> @@ -229,9 +232,12 @@ static int vm_event_disable(struct domain *d, >>> struct vm_event_domain *ved) >>> } >>> } >>> >>> +/* >>> destroy_ring_for_helper(&ved->ring_page, >>> ved->ring_pg_struct); >>> + */ >>> >>> +ved->ring_page = NULL; >>> vm_event_cleanup_domain(d); >>> >>> vm_event_ring_unlock(ved); >>> >>> but this unfortunately still fails to map the page the second time. Do >>> you mean to simply no longer munmap() the ring page from libxc / the >>> client application? >> Neither. >> >> First of all, I notice that this is probably buggy: >> >> ring_pfn = pfn; >> mmap_pfn = pfn; >> rc1 = xc_get_pfn_type_batch(xch, domain_id, 1, &mmap_pfn); >> if ( rc1 || mmap_pfn & XEN_DOMCTL_PFINFO_XTAB ) >> { >> /* Page not in the physmap, try to populate it */ >> rc1 = xc_domain_populate_physmap_exact(xch, domain_id, 1, 0, 0, >> &ring_pfn); >> if ( rc1 != 0 ) >> { >> PERROR("Failed to populate ring pfn\n"); >> goto out; >> } >> } >> >> A failure of xc_get_pfn_type_batch() is not a suggestion that population >> might work. >> >> >> What I meant was taking out this call: >> >> /* Remove the ring_pfn from the guest's physmap */ >> rc1 = xc_domain_decrease_reservation_exact(xch, domain_id, 1, 0, >> &ring_pfn); >> if ( rc1 != 0 ) >> PERROR("Failed to remove ring page from guest physmap"); >> >> To leave the frame in the guest physmap. The issue is fundamentally >> that after this frame has been taken out, something kicks the VM to >> realise it has an extra frame of balloonable space, which it clearly >> compensates for. >> >> You can work around the added attack surface by marking it RO in EPT; >> neither Xen's nor dom0's mappings are translated via EPT, so they can >> still make updates, but the guest won't be able to write to it. >> >> I should say that this is all a gross hack, and is in desperate need of >> a proper API to make rings entirely outside of the gfn space, but this >> hack should work for now. > Thanks! So far, it seems to work like a charm like this: Great. > > diff --git a/tools/libxc/xc_vm_event.c b/tools/libxc/xc_vm_event.c > index 2fef96a..5dd00a6 100644 > --- a/tools/libxc/xc_vm_event.c > +++ b/tools/libxc/xc_vm_event.c > @@ -130,9 +130,17 @@ void *xc_vm_event_enable(xc_interface *xch, domid_t > domain_id, int param, > } > > /* Remove the ring_pfn from the guest's physma
Re: [Xen-devel] [PATCH v11 01/13] x86: add multiboot2 protocol support
On 1/10/17 2:38 AM, Jan Beulich wrote: On 10.01.17 at 02:21, wrote: >> On 12/5/16 4:25 PM, Daniel Kiper wrote: >>> +/* Flags set in the 'flags' member of the multiboot header. */ >>> +#define MULTIBOOT2_TAG_TYPE_END0 >>> +#define MULTIBOOT2_TAG_TYPE_CMDLINE1 >>> +#define MULTIBOOT2_TAG_TYPE_BOOT_LOADER_NAME 2 >>> +#define MULTIBOOT2_TAG_TYPE_MODULE 3 >>> +#define MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO 4 >>> +#define MULTIBOOT2_TAG_TYPE_BOOTDEV5 >>> +#define MULTIBOOT2_TAG_TYPE_MMAP 6 >>> +#define MULTIBOOT2_TAG_TYPE_VBE7 >>> +#define MULTIBOOT2_TAG_TYPE_FRAMEBUFFER8 >>> +#define MULTIBOOT2_TAG_TYPE_ELF_SECTIONS 9 >>> +#define MULTIBOOT2_TAG_TYPE_APM10 >> >> Nitpicky but it would be nice if these lined up. > > Well, the question is what tab width you expect them to line up > with - with tab == 8 spaces they do line up for me. > > Also - please trim your quotes in replies. > > Jan > Well given that this is a new file and CODING_STYLE says spaces over tabs and to treat indent level as 4 spaces. That was what my assumption was. -- Doug Goldstein signature.asc Description: OpenPGP digital signature ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 12/24] x86: refactor psr: set value: implement write msr flow.
>>> On 14.12.16 at 05:07, wrote: > --- a/xen/arch/x86/psr.c > +++ b/xen/arch/x86/psr.c > @@ -186,6 +186,9 @@ struct feat_ops { > unsigned int (*exceeds_cos_max)(const uint64_t val[], > const struct feat_node *feat, > unsigned int cos); > +/* write_msr is used to write out feature MSR register. */ > +int (*write_msr)(unsigned int cos, const uint64_t val[], > + struct feat_node *feat); Looks like this function again returns number-of-values, yet this time without a comment saying so. While you don't need to replicate that description multiple time, please at least has a brief reference. That said, with the type checks moved out I think this return value model won't be needed anymore - the caller, having checked the type, could then simply call the get-num-val (or however it was named) hook to know how many array entries to skip. > @@ -889,9 +909,67 @@ static int alloc_new_cos(const struct psr_socket_info > *info, > return -ENOENT; > } > > +static unsigned int get_socket_cpu(unsigned int socket) > +{ > +if ( likely(socket < nr_sockets) ) > +return cpumask_any(socket_cpumask[socket]); > + > +return nr_cpu_ids; > +} > + > +struct cos_write_info > +{ > +unsigned int cos; > +struct list_head *feat_list; > +const uint64_t *val; > +}; > + > +static void do_write_psr_msr(void *data) > +{ > +struct cos_write_info *info = (struct cos_write_info *)data; > +unsigned int cos = info->cos; > +struct list_head *feat_list= info->feat_list; > +const uint64_t *val= info->val; > +struct feat_node *feat_tmp; > +int ret; > + > +if ( !feat_list ) > +return; > + > +/* We need set all features values into MSRs. */ > +list_for_each_entry(feat_tmp, feat_list, list) > +{ > +ret = feat_tmp->ops.write_msr(cos, val, feat_tmp); > +if ( ret <= 0) Missing blank. > +return; > + > +val += ret; > +} > +} > + > static int write_psr_msr(unsigned int socket, unsigned int cos, > const uint64_t *val) > { > +struct psr_socket_info *info = get_socket_info(socket); > + > +struct cos_write_info data = No blank lines between declarations please (unless there are extraordinarily many). Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 11/24] x86: refactor psr: set value: implement cos id allocation flow.
>>> On 14.12.16 at 05:07, wrote: > --- a/xen/arch/x86/psr.c > +++ b/xen/arch/x86/psr.c > @@ -169,6 +169,23 @@ struct feat_ops { > */ > int (*compare_val)(const uint64_t val[], const struct feat_node *feat, > unsigned int cos, bool *found); > +/* > + * exceeds_cos_max is used to check if the input cos id exceeds the > + * feature's cos_max and if the input value is not the default one. > + * Even if the associated cos exceeds the cos_max, HW can work with > default > + * value. That is the reason we need check if input value is default one. > + * If both criteria are fulfilled, that means the input exceeds the > + * range. Isn't this last sentence the wrong way round? > + * The return value of the function means the number of the value array > + * entries to skip or error. > + * 1 - one entry in value array. > + * 2 - two entries in value array, e.g. CDP uses two entries. > + * 0 - error, exceed cos_max and the input value is not default. > + */ > +unsigned int (*exceeds_cos_max)(const uint64_t val[], > +const struct feat_node *feat, > +unsigned int cos); IIRC I did recommend "exceeds" in the name during earlier review, but also iirc the semantics of the call were different back then. Please try to name functions such that they describe themselves in at least a minimalistic ways. My main issue with this naming is that the function returning non-zero (i.e. true in C meaning within conditionals) means "no" here rather than "yes". fits_cos_max() would therefore be a possibly better fit. > +static bool exceeds_cos_max(const uint64_t *val, > +uint32_t array_len, > +const struct psr_socket_info *info, > +unsigned int cos) > +{ > +unsigned int ret; > +const uint64_t *val_tmp = val; > +const struct feat_node *feat_tmp; > + > +list_for_each_entry(feat_tmp, &info->feat_list, list) > +{ > +ret = feat_tmp->ops.exceeds_cos_max(val_tmp, feat_tmp, cos); > +if ( !ret ) > +return false; > + > +val_tmp += ret; > +if ( val_tmp - val > array_len ) > +return false; > +} > + > +return true; > +} Similarly here - name and return value don't fit together; I think you want to invert the return values or (along the lines above) rename the function. > static int alloc_new_cos(const struct psr_socket_info *info, > const uint64_t *val, uint32_t array_len, > unsigned int old_cos, > enum cbm_type type) > { > -return 0; > +unsigned int cos; > +unsigned int cos_max = 0; > +const struct feat_node *feat_tmp; > +const unsigned int *ref = info->cos_ref; > + > +/* > + * cos_max is the one of the feature which is being set. > + */ > +list_for_each_entry(feat_tmp, &info->feat_list, list) > +{ > +cos_max = feat_tmp->ops.get_cos_max_from_type(feat_tmp, type); > +if ( cos_max > 0 ) > +break; > +} > + > +if ( !cos_max ) > +return -ENOENT; > + > +/* > + * If old cos is referred only by the domain, then use it. And, we cannot > + * use id 0 because it stores the default values. > + */ > +if ( ref[old_cos] == 1 && old_cos ) Please swap both sides of && - cheaper checks should come first if possible. > +if ( exceeds_cos_max(val, array_len, info, old_cos) ) Also please fold the two if()-s. > +return old_cos; And then, as indicated before, this part is not really an allocation, but a re-use, so would likely better move into the caller (or the function's name should be adjusted). > +/* Find an unused one other than cos0. */ > +for ( cos = 1; cos <= cos_max; cos++ ) > +/* > + * ref is 0 means this COS is not used by other domain and > + * can be used for current setting. > + */ > +if ( !ref[cos] ) > +{ > +if ( !exceeds_cos_max(val, array_len, info, cos) ) > +return -ENOENT; > + > +return cos; > +} While a comment is just white space, this looks suspicious without another pair of braces around the for() body. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 1/2] xen/netfront: set default upper limit of tx/rx queues to 8
On 10/01/17 15:53, Boris Ostrovsky wrote: > On 01/10/2017 08:32 AM, Juergen Gross wrote: >> The default for the number of tx/rx queues of one interface is the >> number of vcpus of the system today. As each queue pair reserves 512 >> grant pages this default consumes a ridiculous number of grants for >> large guests. >> >> Limit the queue number to 8 as default. This value can be modified >> via a module parameter if required. >> >> Signed-off-by: Juergen Gross >> --- >> drivers/net/xen-netfront.c | 6 -- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c >> index a479cd9..490c865 100644 >> --- a/drivers/net/xen-netfront.c >> +++ b/drivers/net/xen-netfront.c >> @@ -57,6 +57,7 @@ >> #include >> >> /* Module parameters */ >> +#define MAX_QUEUES_DEFAULT 8 >> static unsigned int xennet_max_queues; >> module_param_named(max_queues, xennet_max_queues, uint, 0644); >> MODULE_PARM_DESC(max_queues, >> @@ -2164,11 +2165,12 @@ static int __init netif_init(void) >> >> pr_info("Initialising Xen virtual ethernet driver\n"); >> >> -/* Allow as many queues as there are CPUs if user has not >> +/* Allow as many queues as there are CPUs inut max. 8 if user has not > > Based on comment change in the second patch: s/inut/but/ ? Also, comment > style in both patches. Typo: yes Style: no. checkpatch tells me that the net directory doesn't follow common kernel style. > > Other than that, for both: > > Reviewed-by: Boris Ostrovsky Thanks, Juergen ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] Ubuntu 16.04.1 LTS kernel 4.4.0-57 over-allocation and xen-access fail
On 01/10/2017 04:13 PM, Andrew Cooper wrote: > On 10/01/17 09:06, Razvan Cojocaru wrote: >> On 01/09/2017 02:54 PM, Andrew Cooper wrote: >>> On 09/01/17 11:36, Razvan Cojocaru wrote: Hello, We've come across a weird phenomenon: an Ubuntu 16.04.1 LTS HVM guest running kernel 4.4.0 installed via XenCenter in XenServer Dundee seems to eat up all the RAM it can: (XEN) [ 394.379760] d1v1 Over-allocation for domain 1: 524545 > 524544 This leads to a problem with xen-access, specifically libxc which does this in xc_vm_event_enable() (this is Xen 4.6): ring_page = xc_map_foreign_batch(xch, domain_id, PROT_READ | PROT_WRITE, &mmap_pfn, 1); if ( mmap_pfn & XEN_DOMCTL_PFINFO_XTAB ) { /* Map failed, populate ring page */ rc1 = xc_domain_populate_physmap_exact(xch, domain_id, 1, 0, 0, &ring_pfn); if ( rc1 != 0 ) { PERROR("Failed to populate ring pfn\n"); goto out; } The first time everything works fine, xen-access can map the ring page. But most of the time the second time fails in the xc_domain_populate_physmap_exact() call, and again this is dumped in the Xen log (once for each failed attempt): (XEN) [ 395.952188] d0v3 Over-allocation for domain 1: 524545 > 524544 >>> Thinking further about this, what happens if you avoid removing the page >>> on exit? >>> >>> The first populate succeeds, and if you leave the page populated, the >>> second time you come around the loop, it should not be of type XTAB, and >>> the map should succeed. >> Sorry for the late reply, had to put out another fire yesterday. >> >> I've taken your recommendation to roughly mean this: >> >> diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c >> index ba9690a..805564b 100644 >> --- a/xen/common/vm_event.c >> +++ b/xen/common/vm_event.c >> @@ -100,8 +100,11 @@ static int vm_event_enable( >> return 0; >> >> err: >> +/* >> destroy_ring_for_helper(&ved->ring_page, >> ved->ring_pg_struct); >> +*/ >> +ved->ring_page = NULL; >> vm_event_ring_unlock(ved); >> >> return rc; >> @@ -229,9 +232,12 @@ static int vm_event_disable(struct domain *d, >> struct vm_event_domain *ved) >> } >> } >> >> +/* >> destroy_ring_for_helper(&ved->ring_page, >> ved->ring_pg_struct); >> + */ >> >> +ved->ring_page = NULL; >> vm_event_cleanup_domain(d); >> >> vm_event_ring_unlock(ved); >> >> but this unfortunately still fails to map the page the second time. Do >> you mean to simply no longer munmap() the ring page from libxc / the >> client application? > > Neither. > > First of all, I notice that this is probably buggy: > > ring_pfn = pfn; > mmap_pfn = pfn; > rc1 = xc_get_pfn_type_batch(xch, domain_id, 1, &mmap_pfn); > if ( rc1 || mmap_pfn & XEN_DOMCTL_PFINFO_XTAB ) > { > /* Page not in the physmap, try to populate it */ > rc1 = xc_domain_populate_physmap_exact(xch, domain_id, 1, 0, 0, > &ring_pfn); > if ( rc1 != 0 ) > { > PERROR("Failed to populate ring pfn\n"); > goto out; > } > } > > A failure of xc_get_pfn_type_batch() is not a suggestion that population > might work. > > > What I meant was taking out this call: > > /* Remove the ring_pfn from the guest's physmap */ > rc1 = xc_domain_decrease_reservation_exact(xch, domain_id, 1, 0, > &ring_pfn); > if ( rc1 != 0 ) > PERROR("Failed to remove ring page from guest physmap"); > > To leave the frame in the guest physmap. The issue is fundamentally > that after this frame has been taken out, something kicks the VM to > realise it has an extra frame of balloonable space, which it clearly > compensates for. > > You can work around the added attack surface by marking it RO in EPT; > neither Xen's nor dom0's mappings are translated via EPT, so they can > still make updates, but the guest won't be able to write to it. > > I should say that this is all a gross hack, and is in desperate need of > a proper API to make rings entirely outside of the gfn space, but this > hack should work for now. Thanks! So far, it seems to work like a charm like this: diff --git a/tools/libxc/xc_vm_event.c b/tools/libxc/xc_vm_event.c index 2fef96a..5dd00a6 100644 --- a/tools/libxc/xc_vm_event.c +++ b/tools/libxc/xc_vm_event.c @@ -130,9 +130,17 @@ void *xc_vm_event_enable(xc_interface *xch, domid_t domain_id, int param, } /* Remove the ring_pfn from the guest's physmap */ +/* rc1 = xc_domain_decrease_reservation_exact(xch, domain_id, 1, 0, &ring_pfn); if ( rc1 != 0 ) PERROR("Failed to remove ring page from guest physmap");
Re: [Xen-devel] [edk2] [PATCH RFC 00/14] Specific platform to run OVMF in Xen PVH and HVM guests
On Wed, Jan 04, 2017 at 08:52:00PM +0100, Laszlo Ersek wrote: > On 12/08/16 16:33, Anthony PERARD wrote: > > Hi, > > > > I've started to create a Xen specifig plaform, in OvmfPkg/XenOvmf.dsc > > with the goal to make it work on both Xen HVM and Xen PVHv2 > > Does this mean we can ultimately move all Xen roles from the current > platform DSC files to the new Xen DSC file entirely? Yes, I had this in mind will working on the series. I would just need to teach our build system (in xen.git) to look for this new platform file. > If so (which I think I would like), then for each module M that exhibits > all of the following properties: > - M is dynamically customized to Xen vs. QEMU, > - M is replaced by a dedicated module M' in the Xen DSC, > I think we should also remove the Xen-specific code from the original M > (as last step, likely in separate patches). > > In addition, Xen platform specific device drivers should be removed as > well from the original DSC files. > > What do you think? Yes, I think all of it sound good. > > The first few patches only create the platform and duplicate some code from > > OvmfPkg and the later patches (from OvmfPkg/XenPlatformPei: Add xen PVH > > specific code) makes OVMF boot in a Xen PVH guest, and can boot a Linux. > > > > == Part 1: XenOvmf.dsc > > > > - OvmfPkg: Create platform XenOvmf > > which for now remove virtio drivers and some SMM > > > > - OvmfPkg/XenOvmf: Update debug IO port for Xen > > > > - OvmfPkg/XenOvmf.dsc: Introduce XenResetVector > > Just for one change, enable cache in CR0 as on Xen, OVMF run from RAM, that > > disabling cache can make OVMF very slow. > > > > ... I might reply to this email again (the remaining stuff), as I > progress with the review. > > Thanks > Laszlo Thanks, -- Anthony PERARD ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 1/2] xen/netfront: set default upper limit of tx/rx queues to 8
On 01/10/2017 08:32 AM, Juergen Gross wrote: > The default for the number of tx/rx queues of one interface is the > number of vcpus of the system today. As each queue pair reserves 512 > grant pages this default consumes a ridiculous number of grants for > large guests. > > Limit the queue number to 8 as default. This value can be modified > via a module parameter if required. > > Signed-off-by: Juergen Gross > --- > drivers/net/xen-netfront.c | 6 -- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c > index a479cd9..490c865 100644 > --- a/drivers/net/xen-netfront.c > +++ b/drivers/net/xen-netfront.c > @@ -57,6 +57,7 @@ > #include > > /* Module parameters */ > +#define MAX_QUEUES_DEFAULT 8 > static unsigned int xennet_max_queues; > module_param_named(max_queues, xennet_max_queues, uint, 0644); > MODULE_PARM_DESC(max_queues, > @@ -2164,11 +2165,12 @@ static int __init netif_init(void) > > pr_info("Initialising Xen virtual ethernet driver\n"); > > - /* Allow as many queues as there are CPUs if user has not > + /* Allow as many queues as there are CPUs inut max. 8 if user has not Based on comment change in the second patch: s/inut/but/ ? Also, comment style in both patches. Other than that, for both: Reviewed-by: Boris Ostrovsky >* specified a value. >*/ > if (xennet_max_queues == 0) > - xennet_max_queues = num_online_cpus(); > + xennet_max_queues = min_t(unsigned int, MAX_QUEUES_DEFAULT, > + num_online_cpus()); > > return xenbus_register_frontend(&netfront_driver); > } ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 10/24] x86: refactor psr: set value: implement cos finding flow.
>>> On 14.12.16 at 05:07, wrote: > Continue with previous patch, we can try to find if there is a Please take into consideration that a series may be applied in small steps. References such as "previous patch" are thus possibly meaningless. Please instead refer to the patch by title. Also I think you mean "continue from ...". > @@ -666,7 +724,58 @@ static int find_cos(const uint64_t *val, uint32_t > array_len, > enum cbm_type type, > const struct psr_socket_info *info) > { > -return 0; > +unsigned int cos; > +const unsigned int *ref = info->cos_ref; > +const struct feat_node *feat_tmp; > +const uint64_t *val_tmp = val; > +int ret; > +bool found = false; > +unsigned int cos_max = 0; > + > +/* cos_max is the one of the feature which is being set. */ > +list_for_each_entry(feat_tmp, &info->feat_list, list) > +{ > +cos_max = feat_tmp->ops.get_cos_max_from_type(feat_tmp, type); > +if ( cos_max > 0 ) > +break; > +} > + > +for ( cos = 0; cos <= cos_max; cos++ ) > +{ > +if ( cos && !ref[cos] ) > +continue; > + > +/* Not found, need find again from beginning. */ > +val_tmp = val; > +list_for_each_entry(feat_tmp, &info->feat_list, list) > +{ > +/* > + * Compare value according to feature list order. > + * We must follow this order because value array is assembled > + * as this order in get_old_set_new(). > + */ > +ret = feat_tmp->ops.compare_val(val_tmp, feat_tmp, cos, &found); > +if ( ret < 0 ) > +return ret; > + > +/* If fail to match, go to next cos to compare. */ > +if ( !found ) > +break; > + > +val_tmp += ret; > +if ( val_tmp - val > array_len ) > +return -EINVAL; > +} > + > +/* > + * With this cos id, every entry of value array can match. This cos > + * is what we find. > + */ "can match" seems rather misleading to me. I think you mean something like "For this COS ID all entries in the values array did match. Use it." Other than that various of the comments given for earlier patches apply here, in particular the fact that the type matching should move out of the hook functions. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3] x86/HVM: Fix teardown ordering in hvm_vcpu_destroy()
>>> On 10.01.17 at 15:26, wrote: > On 10/01/17 14:15, Andrew Cooper wrote: >> On 10/01/17 14:03, Suravee Suthikulpanit wrote: >>> The order of destroy function calls in hvm_vcpu_destroy() should be >>> the reverse of init calls in hvm_vcpu_initialise(). >>> >>> Signed-off-by: Suravee Suthikulpanit >>> Reviewed-by: Konrad Rzeszutek Wilk >>> Reviewed-by: Kevin Tian >>> Cc: Jan Beulich >>> Cc: Boris Ostrovsky >> Reviewed-by: Andrew Cooper and queued. > > Wait no. Which clearly suggests that the earlier R-b-s should have been dropped too. > The order in vcpu_initialise is > > hvm_vcpu_cacheattr_init() > vlapic_init() > hvm_funcs.vcpu_initialise() > softirq_tasklet_init() > setup_compat_arg_xlat() > > Therefore, moving the tasklet_kill() is wrong. > > The overall delta should be: > > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -1626,12 +1626,12 @@ void hvm_vcpu_destroy(struct vcpu *v) > free_compat_arg_xlat(v); > > tasklet_kill(&v->arch.hvm_vcpu.assert_evtchn_irq_tasklet); > -hvm_vcpu_cacheattr_destroy(v); > +hvm_funcs.vcpu_destroy(v); > > if ( is_hvm_vcpu(v) ) > vlapic_destroy(v); > > -hvm_funcs.vcpu_destroy(v); > +hvm_vcpu_cacheattr_destroy(v); > } > > IIRC. > > If you agree, I will fold this correction in while committing. This variant is Reviewed-by: Jan Beulich Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 09/24] x86: refactor psr: set value: assemble features value array.
>>> On 14.12.16 at 05:07, wrote: > --- a/xen/arch/x86/psr.c > +++ b/xen/arch/x86/psr.c > @@ -121,6 +121,35 @@ struct feat_ops { > /* get_val is used to get feature COS register value. */ > bool (*get_val)(const struct feat_node *feat, unsigned int cos, > enum cbm_type type, uint64_t *val); > +/* > + * get_cos_num is used to get the COS registers amount used by the > + * feature for one setting, e.g. CDP uses 2 COSs but CAT uses 1. > + */ > +unsigned int (*get_cos_num)(const struct feat_node *feat); > +/* > + * get_old_val and set_new_val are a pair of functions called together. > + * The caller will traverse all features in the list and call both > + * functions for every feature to do below two things: > + * 1. get old_cos register value of all supported features and > + * 2. set the new value for the feature. > + * > + * All the values are set into value array according the traversal order, > + * meaning the same order of feature list members. > + * > + * The return value is the amount of entries to skip in the value array > + * or error. > + * 1 - one entry in value array. > + * 2 - two entries in value array, e.g. CDP uses two entries. Doesn't this match the get_cos_num() return value? Would be nice to be spelled out (and, where possible, ASSERT()ed). > @@ -186,6 +215,29 @@ static void free_feature(struct psr_socket_info *info) > } > } > > +static bool_t psr_check_cbm(unsigned int cbm_len, uint64_t cbm) bool (and then true/false in the function body) > +static int l3_cat_set_new_val(uint64_t val[], > + const struct feat_node *feat, > + unsigned int old_cos, > + enum cbm_type type, > + uint64_t m) > +{ > +if ( type != PSR_CBM_TYPE_L3 ) > +/* L3 CAT uses one COS. Skip it. */ > +return 1; If this is the wrong type, how can you return 1 (or any positive value) here? This basically suggests that, as recommended for an earlier operation already, that you want the type check done in the caller. Or wait - isn't type not matching an error here, as you iterate the same list twice in the same order? Which then gets us back to me mentioning that the set-new-value should really be done in common code, not in the callbacks; it would really only be the check (psr_check_cbm() in the L3 case above) which would require a callback. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3] x86/HVM: Fix teardown ordering in hvm_vcpu_destroy()
On 10/01/17 14:15, Andrew Cooper wrote: > On 10/01/17 14:03, Suravee Suthikulpanit wrote: >> The order of destroy function calls in hvm_vcpu_destroy() should be >> the reverse of init calls in hvm_vcpu_initialise(). >> >> Signed-off-by: Suravee Suthikulpanit >> Reviewed-by: Konrad Rzeszutek Wilk >> Reviewed-by: Kevin Tian >> Cc: Jan Beulich >> Cc: Boris Ostrovsky > Reviewed-by: Andrew Cooper and queued. Wait no. The order in vcpu_initialise is hvm_vcpu_cacheattr_init() vlapic_init() hvm_funcs.vcpu_initialise() softirq_tasklet_init() setup_compat_arg_xlat() Therefore, moving the tasklet_kill() is wrong. The overall delta should be: andrewcoop@andrewcoop:/local/xen.git/xen$ git diff HEAD^ diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 4c0f561..9f74334 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -1626,12 +1626,12 @@ void hvm_vcpu_destroy(struct vcpu *v) free_compat_arg_xlat(v); tasklet_kill(&v->arch.hvm_vcpu.assert_evtchn_irq_tasklet); -hvm_vcpu_cacheattr_destroy(v); +hvm_funcs.vcpu_destroy(v); if ( is_hvm_vcpu(v) ) vlapic_destroy(v); -hvm_funcs.vcpu_destroy(v); +hvm_vcpu_cacheattr_destroy(v); } IIRC. If you agree, I will fold this correction in while committing. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3] x86/HVM: Fix teardown ordering in hvm_vcpu_destroy()
On 10/01/17 14:03, Suravee Suthikulpanit wrote: > The order of destroy function calls in hvm_vcpu_destroy() should be > the reverse of init calls in hvm_vcpu_initialise(). > > Signed-off-by: Suravee Suthikulpanit > Reviewed-by: Konrad Rzeszutek Wilk > Reviewed-by: Kevin Tian > Cc: Jan Beulich > Cc: Boris Ostrovsky Reviewed-by: Andrew Cooper and queued. > --- > Note: I separate this out from the previously sent AMD SVM AVIC patch > series since this is a standalone fix. > > xen/arch/x86/hvm/hvm.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c > index 25dc759..d465596 100644 > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -1610,13 +1610,13 @@ void hvm_vcpu_destroy(struct vcpu *v) > > free_compat_arg_xlat(v); > > -tasklet_kill(&v->arch.hvm_vcpu.assert_evtchn_irq_tasklet); > -hvm_vcpu_cacheattr_destroy(v); > +hvm_funcs.vcpu_destroy(v); > > if ( is_hvm_vcpu(v) ) > vlapic_destroy(v); > > -hvm_funcs.vcpu_destroy(v); > +tasklet_kill(&v->arch.hvm_vcpu.assert_evtchn_irq_tasklet); > +hvm_vcpu_cacheattr_destroy(v); > } > > void hvm_vcpu_down(struct vcpu *v) ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] Ubuntu 16.04.1 LTS kernel 4.4.0-57 over-allocation and xen-access fail
On 10/01/17 09:06, Razvan Cojocaru wrote: > On 01/09/2017 02:54 PM, Andrew Cooper wrote: >> On 09/01/17 11:36, Razvan Cojocaru wrote: >>> Hello, >>> >>> We've come across a weird phenomenon: an Ubuntu 16.04.1 LTS HVM guest >>> running kernel 4.4.0 installed via XenCenter in XenServer Dundee seems >>> to eat up all the RAM it can: >>> >>> (XEN) [ 394.379760] d1v1 Over-allocation for domain 1: 524545 > 524544 >>> >>> This leads to a problem with xen-access, specifically libxc which does >>> this in xc_vm_event_enable() (this is Xen 4.6): >>> >>> ring_page = xc_map_foreign_batch(xch, domain_id, PROT_READ | PROT_WRITE, >>> &mmap_pfn, 1); >>> >>> if ( mmap_pfn & XEN_DOMCTL_PFINFO_XTAB ) >>> { >>> /* Map failed, populate ring page */ >>> rc1 = xc_domain_populate_physmap_exact(xch, domain_id, 1, 0, 0, >>>&ring_pfn); >>> if ( rc1 != 0 ) >>> { >>> PERROR("Failed to populate ring pfn\n"); >>> goto out; >>> } >>> >>> The first time everything works fine, xen-access can map the ring page. >>> But most of the time the second time fails in the >>> xc_domain_populate_physmap_exact() call, and again this is dumped in the >>> Xen log (once for each failed attempt): >>> >>> (XEN) [ 395.952188] d0v3 Over-allocation for domain 1: 524545 > 524544 >> Thinking further about this, what happens if you avoid removing the page >> on exit? >> >> The first populate succeeds, and if you leave the page populated, the >> second time you come around the loop, it should not be of type XTAB, and >> the map should succeed. > Sorry for the late reply, had to put out another fire yesterday. > > I've taken your recommendation to roughly mean this: > > diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c > index ba9690a..805564b 100644 > --- a/xen/common/vm_event.c > +++ b/xen/common/vm_event.c > @@ -100,8 +100,11 @@ static int vm_event_enable( > return 0; > > err: > +/* > destroy_ring_for_helper(&ved->ring_page, > ved->ring_pg_struct); > +*/ > +ved->ring_page = NULL; > vm_event_ring_unlock(ved); > > return rc; > @@ -229,9 +232,12 @@ static int vm_event_disable(struct domain *d, > struct vm_event_domain *ved) > } > } > > +/* > destroy_ring_for_helper(&ved->ring_page, > ved->ring_pg_struct); > + */ > > +ved->ring_page = NULL; > vm_event_cleanup_domain(d); > > vm_event_ring_unlock(ved); > > but this unfortunately still fails to map the page the second time. Do > you mean to simply no longer munmap() the ring page from libxc / the > client application? Neither. First of all, I notice that this is probably buggy: ring_pfn = pfn; mmap_pfn = pfn; rc1 = xc_get_pfn_type_batch(xch, domain_id, 1, &mmap_pfn); if ( rc1 || mmap_pfn & XEN_DOMCTL_PFINFO_XTAB ) { /* Page not in the physmap, try to populate it */ rc1 = xc_domain_populate_physmap_exact(xch, domain_id, 1, 0, 0, &ring_pfn); if ( rc1 != 0 ) { PERROR("Failed to populate ring pfn\n"); goto out; } } A failure of xc_get_pfn_type_batch() is not a suggestion that population might work. What I meant was taking out this call: /* Remove the ring_pfn from the guest's physmap */ rc1 = xc_domain_decrease_reservation_exact(xch, domain_id, 1, 0, &ring_pfn); if ( rc1 != 0 ) PERROR("Failed to remove ring page from guest physmap"); To leave the frame in the guest physmap. The issue is fundamentally that after this frame has been taken out, something kicks the VM to realise it has an extra frame of balloonable space, which it clearly compensates for. You can work around the added attack surface by marking it RO in EPT; neither Xen's nor dom0's mappings are translated via EPT, so they can still make updates, but the guest won't be able to write to it. I should say that this is all a gross hack, and is in desperate need of a proper API to make rings entirely outside of the gfn space, but this hack should work for now. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 08/24] x86: refactor psr: set value: implement framework.
> To make the set value flow be general and can support multiple features > at same time, it includes below steps: > 1. Get COS ID of current domain using. > 2. Assemble a value array to store all features current value >in it and replace the current value of the feature which is >being set to the new input value. > 3. Find if there is already a COS ID on which all features' >values are same as the array. Then, we can reuse this COS >ID. > 4. If fail to find, we need allocate a new COS ID. Only COS ID which ref >is 0 or 1 can be allocated. Using "allocate" here in conjunction with ref count being 1 is a little misleading here - allocation would mean a fresh ID, whereas in the case of ref == 1 you mean to re-use the current one. > --- a/xen/arch/x86/psr.c > +++ b/xen/arch/x86/psr.c > @@ -513,18 +513,197 @@ int psr_get_val(struct domain *d, unsigned int socket, > return -ENOENT; > } > > -int psr_set_l3_cbm(struct domain *d, unsigned int socket, > - uint64_t cbm, enum cbm_type type) > +/* Set value functions */ > +static unsigned int get_cos_num(const struct psr_socket_info *info) > { > return 0; > } > > +static int get_old_set_new(uint64_t *val, > + uint32_t array_len, > + const struct psr_socket_info *info, > + unsigned int old_cos, > + enum cbm_type type, > + uint64_t m) > +{ > +return 0; > +} > + > +static int find_cos(const uint64_t *val, uint32_t array_len, > +enum cbm_type type, > +const struct psr_socket_info *info) > +{ > +return 0; > +} > + > +static int alloc_new_cos(const struct psr_socket_info *info, > + const uint64_t *val, uint32_t array_len, > + unsigned int old_cos, > + enum cbm_type type) > +{ > +return 0; > +} > + > +static int write_psr_msr(unsigned int socket, unsigned int cos, > + const uint64_t *val) > +{ > +return 0; > +} I think all of the above functions should return an error as long as they're stubbed out, yet I don't think 0 means error in all cases (in particular a return value of plain int suggests 0 to mean success). > +int psr_set_val(struct domain *d, unsigned int socket, > +uint64_t val, enum cbm_type type) > +{ > +unsigned int old_cos; > +int cos, ret; > +unsigned int *ref; > +uint64_t *val_array; > +struct psr_socket_info *info = get_socket_info(socket); > +uint32_t array_len; > + > +if ( IS_ERR(info) ) > +return PTR_ERR(info); > + > +/* > + * Step 0: > + * old_cos means the COS ID current domain is using. By default, it is 0. > + * > + * For every COS ID, there is a reference count to record how many > domains > + * are using the COS register corresponding to this COS ID. > + * - If ref[old_cos] is 0, that means this COS is not used by any domain. > + * - If ref[old_cos] is 1, that means this COS is only used by current > + * domain. > + * - If ref[old_cos] is more than 1, that mean multiple domains are using > + * this COS. > + */ > +old_cos = d->arch.psr_cos_ids[socket]; > +if ( old_cos > MAX_COS_REG_CNT ) > +return -EOVERFLOW; > + > +ref = info->cos_ref; > + > +/* > + * Step 1: > + * Assemle a value array to store all featues cos_reg_val[old_cos]. > + * And, set the input val into array according to the feature's > + * position in array. > + */ > +array_len = get_cos_num((const struct psr_socket_info *)info); What is this cast doing here? (There are more of this kind below.) > +val_array = xzalloc_array(uint64_t, array_len); > +if ( !val_array ) > +return -ENOMEM; > + > +if ( (ret = get_old_set_new(val_array, array_len, > +(const struct psr_socket_info *)info, > +old_cos, type, val)) != 0 ) Just like for earlier versions I continue to be unconvinced that the get-current-settings and the replace-target-value should be a single operation. In particular I'd expect the function to be able to store the target value, as long as the array entries are ordered in a suitable way. > +{ > +xfree(val_array); > +return ret; > +} > + > +/* > + * Lock here to make sure the ref is not changed during find and > + * write process. > + */ > +spin_lock(&info->ref_lock); > + > +/* > + * Step 2: > + * Try to find if there is already a COS ID on which all features' values > + * are same as the array. Then, we can reuse this COS ID. > + */ > +cos = find_cos((const uint64_t *)val_array, array_len, type, > + (const struct psr_socket_info *)info); > +if ( cos >= 0 ) > +{ > +if ( cos == old_cos ) > +{ > +sp
[Xen-devel] [PATCH v3] x86/HVM: Fix teardown ordering in hvm_vcpu_destroy()
The order of destroy function calls in hvm_vcpu_destroy() should be the reverse of init calls in hvm_vcpu_initialise(). Signed-off-by: Suravee Suthikulpanit Reviewed-by: Konrad Rzeszutek Wilk Reviewed-by: Kevin Tian Cc: Jan Beulich Cc: Boris Ostrovsky --- Note: I separate this out from the previously sent AMD SVM AVIC patch series since this is a standalone fix. xen/arch/x86/hvm/hvm.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 25dc759..d465596 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -1610,13 +1610,13 @@ void hvm_vcpu_destroy(struct vcpu *v) free_compat_arg_xlat(v); -tasklet_kill(&v->arch.hvm_vcpu.assert_evtchn_irq_tasklet); -hvm_vcpu_cacheattr_destroy(v); +hvm_funcs.vcpu_destroy(v); if ( is_hvm_vcpu(v) ) vlapic_destroy(v); -hvm_funcs.vcpu_destroy(v); +tasklet_kill(&v->arch.hvm_vcpu.assert_evtchn_irq_tasklet); +hvm_vcpu_cacheattr_destroy(v); } void hvm_vcpu_down(struct vcpu *v) -- 1.9.1 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 0/2] xen/net: limit number of tx/rx queues
On Tue, Jan 10, 2017 at 02:32:50PM +0100, Juergen Gross wrote: > The Xen network frontend/backend supports multiple tx/rx queues for one > virtual interface. The number of queues supported by the backend is > set to the number of cpus of the backend driver domain (usually dom0) > and the number of queues requested by the frontend is limited by the > number of vcpus of the related guest. > > On large systems this can lead to ridiculous large number of queues > exhausting the required number of grant pages rather quick. > > To avoid this limit the default maximum on both sides to 8. Both > frontend and backend maximum can be individually tuned via module > parameters. > > Juergen Gross (2): > xen/netfront: set default upper limit of tx/rx queues to 8 > xen/netback: set default upper limit of tx/rx queues to 8 > Acked-by: Wei Liu ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 07/24] x86: refactor psr: implement get value flow.
>>> On 14.12.16 at 05:07, wrote: > This patch implements get value flow including L3 CAT callback > function. > > It also changes domctl interface to make it more general. > > With this patch, 'psr-cat-show' can work for L3 CAT. How about CDP? You don't implement anything for it here, and looking over the subjects of the remaining patches there also doesn't seem to be anything to come. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 06/24] x86: refactor psr: implement get hw info flow.
>>> On 14.12.16 at 05:07, wrote: > --- a/xen/arch/x86/psr.c > +++ b/xen/arch/x86/psr.c > @@ -115,6 +115,9 @@ struct feat_ops { > struct psr_socket_info *info); > /* get_max_cos_max is used to get feature's cos_max. */ > unsigned int (*get_max_cos_max)(const struct feat_node *feat); > +/* get_feat_info is used to get feature HW info. */ > +bool (*get_feat_info)(const struct feat_node *feat, enum cbm_type type, > + uint32_t dat[], uint32_t array_len); data, value, or val would all seem okay, but dat suggests an acronym of other than data (which I think it is meant to be). > @@ -220,9 +223,24 @@ static unsigned int l3_cat_get_max_cos_max(const struct > feat_node *feat) > return feat->info.l3_cat_info.cos_max; > } > > +static bool l3_cat_get_feat_info(const struct feat_node *feat, > + enum cbm_type type, > + uint32_t dat[], uint32_t array_len) array_len wants to be size_t or unsigned int. And more generally, please avoid fixed width types when you don't really mean such. > +int psr_get_info(unsigned int socket, enum cbm_type type, > + uint32_t dat[], uint32_t array_len) > +{ > +struct psr_socket_info *info = get_socket_info(socket); > +struct feat_node *feat_tmp; With the hook function taking a pointer to const I don#t see why this one can't be const, too. > +if ( IS_ERR(info) ) > +return PTR_ERR(info); > + > +list_for_each_entry(feat_tmp, &info->feat_list, list) > +if ( feat_tmp->ops.get_feat_info(feat_tmp, type, dat, array_len) ) Wouldn't the type check better be done here than inside each function? That would then also allow for terminating the loop earlier (when the type was found, instead of when a function returns success). > --- a/xen/include/asm-x86/psr.h > +++ b/xen/include/asm-x86/psr.h > @@ -33,6 +33,11 @@ > /* L3 CDP Enable bit*/ > #define PSR_L3_QOS_CDP_ENABLE_BIT 0x0 > > +/* Used by psr_get_info() */ > +#define CBM_LEN 0 > +#define COS_MAX 1 > +#define CDP_FLAG 2 These needing putting in a header means that you want to prefix them, e.g. by PSR_. Also with the next value used e.g. as array dimension, I think you also want to name that value (currently 3) and use it instead of a plain number which would need to be adjusted everywhere once a value gets added here. Also - is CDP_FLAG really a suitable name for flags? Wouldn't PSR_FLAGS be better (as being more general)? Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 05/24] x86: refactor psr: implement Domain init/free and schedule flows.
>>> On 14.12.16 at 05:07, wrote: > @@ -358,11 +366,32 @@ void psr_free_rmid(struct domain *d) > d->arch.psr_rmid = 0; > } > > +static inline unsigned int get_max_cos_max(const struct psr_socket_info > *info) > +{ > +const struct feat_node *feat_tmp; Same remark as on the earlier patch regarding the _tmp suffix. Should this reoccur in later patches, I won't give the same comment again. > +unsigned int cos_max = 0; > + > +list_for_each_entry(feat_tmp, &info->feat_list, list) > +cos_max = max(feat_tmp->ops.get_max_cos_max(feat_tmp), cos_max); > + > +return cos_max; > +} > + > static inline void psr_assoc_init(void) > { > struct psr_assoc *psra = &this_cpu(psr_assoc); > > -if ( psr_cmt_enabled() ) > +if ( socket_info ) > +{ > +unsigned int socket = cpu_to_socket(smp_processor_id()); > +const struct psr_socket_info *info = socket_info + socket; > +unsigned int cos_max = get_max_cos_max(info); > + > +if ( info->feat_mask ) > +psra->cos_mask = ((1ull << get_count_order(cos_max)) - 1) << 32; > +} > + > +if ( psr_cmt_enabled() || psra->cos_mask ) > rdmsrl(MSR_IA32_PSR_ASSOC, psra->val); > } > > @@ -371,6 +400,12 @@ static inline void psr_assoc_rmid(uint64_t *reg, > unsigned int rmid) > *reg = (*reg & ~rmid_mask) | (rmid & rmid_mask); > } > > +static inline void psr_assoc_cos(uint64_t *reg, unsigned int cos, > + uint64_t cos_mask) > +{ > +*reg = (*reg & ~cos_mask) | (((uint64_t)cos << 32) & cos_mask); This recurring 32 would perhaps better become a #define, so they can be identified as the same entity (at least I think they are). Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH 0/2] xen/net: limit number of tx/rx queues
The Xen network frontend/backend supports multiple tx/rx queues for one virtual interface. The number of queues supported by the backend is set to the number of cpus of the backend driver domain (usually dom0) and the number of queues requested by the frontend is limited by the number of vcpus of the related guest. On large systems this can lead to ridiculous large number of queues exhausting the required number of grant pages rather quick. To avoid this limit the default maximum on both sides to 8. Both frontend and backend maximum can be individually tuned via module parameters. Juergen Gross (2): xen/netfront: set default upper limit of tx/rx queues to 8 xen/netback: set default upper limit of tx/rx queues to 8 drivers/net/xen-netback/netback.c | 6 -- drivers/net/xen-netfront.c| 6 -- 2 files changed, 8 insertions(+), 4 deletions(-) -- 2.10.2 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH 2/2] xen/netback: set default upper limit of tx/rx queues to 8
The default for the maximum number of tx/rx queues of one interface is the number of cpus of the system today. As each queue pair reserves 512 grant pages this default consumes a ridiculous number of grants for large guests. Limit the queue number to 8 as default. This value can be modified via a module parameter if required. Signed-off-by: Juergen Gross --- drivers/net/xen-netback/netback.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c index 47b4810..f9bcf4a 100644 --- a/drivers/net/xen-netback/netback.c +++ b/drivers/net/xen-netback/netback.c @@ -67,6 +67,7 @@ module_param(rx_drain_timeout_msecs, uint, 0444); unsigned int rx_stall_timeout_msecs = 6; module_param(rx_stall_timeout_msecs, uint, 0444); +#define MAX_QUEUES_DEFAULT 8 unsigned int xenvif_max_queues; module_param_named(max_queues, xenvif_max_queues, uint, 0644); MODULE_PARM_DESC(max_queues, @@ -1622,11 +1623,12 @@ static int __init netback_init(void) if (!xen_domain()) return -ENODEV; - /* Allow as many queues as there are CPUs if user has not + /* Allow as many queues as there are CPUs but max. 8 if user has not * specified a value. */ if (xenvif_max_queues == 0) - xenvif_max_queues = num_online_cpus(); + xenvif_max_queues = min_t(unsigned int, MAX_QUEUES_DEFAULT, + num_online_cpus()); if (fatal_skb_slots < XEN_NETBK_LEGACY_SLOTS_MAX) { pr_info("fatal_skb_slots too small (%d), bump it to XEN_NETBK_LEGACY_SLOTS_MAX (%d)\n", -- 2.10.2 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH 1/2] xen/netfront: set default upper limit of tx/rx queues to 8
The default for the number of tx/rx queues of one interface is the number of vcpus of the system today. As each queue pair reserves 512 grant pages this default consumes a ridiculous number of grants for large guests. Limit the queue number to 8 as default. This value can be modified via a module parameter if required. Signed-off-by: Juergen Gross --- drivers/net/xen-netfront.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c index a479cd9..490c865 100644 --- a/drivers/net/xen-netfront.c +++ b/drivers/net/xen-netfront.c @@ -57,6 +57,7 @@ #include /* Module parameters */ +#define MAX_QUEUES_DEFAULT 8 static unsigned int xennet_max_queues; module_param_named(max_queues, xennet_max_queues, uint, 0644); MODULE_PARM_DESC(max_queues, @@ -2164,11 +2165,12 @@ static int __init netif_init(void) pr_info("Initialising Xen virtual ethernet driver\n"); - /* Allow as many queues as there are CPUs if user has not + /* Allow as many queues as there are CPUs inut max. 8 if user has not * specified a value. */ if (xennet_max_queues == 0) - xennet_max_queues = num_online_cpus(); + xennet_max_queues = min_t(unsigned int, MAX_QUEUES_DEFAULT, + num_online_cpus()); return xenbus_register_frontend(&netfront_driver); } -- 2.10.2 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [ovmf baseline-only test] 68351: all pass
This run is configured for baseline tests only. flight 68351 ovmf real [real] http://osstest.xs.citrite.net/~osstest/testlogs/logs/68351/ Perfect :-) All tests in this flight passed as required version targeted for testing: ovmf fca422890777a02c027061fbceee454c9f117870 baseline version: ovmf 7ecfa0aa38a3601c958a81dc36f69b5e04e40584 Last test of basis68347 2017-01-09 22:17:25 Z0 days Testing same since68351 2017-01-10 09:18:30 Z0 days1 attempts People who touched revisions under test: Chao Zhang Zhang, Chao B 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.xs.citrite.net logs: /home/osstest/logs images: /home/osstest/images Logs, config files, etc. are available at http://osstest.xs.citrite.net/~osstest/testlogs/logs Test harness code can be found at http://xenbits.xensource.com/gitweb?p=osstest.git;a=summary Push not applicable. commit fca422890777a02c027061fbceee454c9f117870 Author: Zhang, Chao B Date: Fri Dec 23 16:55:32 2016 +0800 SecurityPkg: Tcg2Config: TPM2 ACPI Table Rev Option Add TPM2 ACPI Table Rev Option in Tcg2Config UI. Rev 4 is defined in TCG ACPI Specification 00.37 Cc: Star Zeng Cc: Yao Jiewen Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Chao Zhang Reviewed-by: Star Zeng Reviewed-by: Yao Jiewen commit 8f07a374b1d0497b6676491de8cbe2f08f4f7e9f Author: Zhang, Chao B Date: Mon Dec 26 10:50:36 2016 +0800 MdePkg: Tpm2Acpi.h: Update TPM2 ACPI table version Update TPM2 ACPI Table revision to 4. New version & data structure is defined in TCG ACPI Spec 00.37 Cc: Star Zeng Cc: Yao Jiewen Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Chao Zhang Reviewed-by: Star Zeng Reviewed-by: Yao Jiewen ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 6/6] x86emul: improve CR/DR access handling
>>> On 10.01.17 at 12:59, wrote: > On 10/01/17 09:06, Jan Beulich wrote: >> - don't accept LOCK for DR accesses (it's undefined in the manuals) >> - only accept LOCK for CR accesses when the respective feature flag is >> set (which would not normally be the case for Intel) >> - add (rather than or) 8 when LOCK is present; real hardware #UDs >> when both REX.W and LOCK are present, implying that these would >> rather access hypothetical CR16...23 >> - eliminate explicit decode_register() calls >> - streamline remaining read/write code >> >> No further functional change, i.e. not addressing the missing exception >> generation (#UD for invalid CR/DR encodings, #GP(0) for invalid write >> values, #DB for DR accesses with DR7.GD set). >> >> Signed-off-by: Jan Beulich >> >> --- a/xen/arch/x86/x86_emulate/x86_emulate.c >> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c >> @@ -194,7 +194,8 @@ static const opcode_desc_t twobyte_table >> ImplicitOps|ModRM, ImplicitOps|ModRM, ImplicitOps|ModRM, >> ImplicitOps|ModRM, >> ImplicitOps|ModRM, ImplicitOps|ModRM, ImplicitOps|ModRM, >> ImplicitOps|ModRM, >> /* 0x20 - 0x27 */ >> -ImplicitOps|ModRM, ImplicitOps|ModRM, ImplicitOps|ModRM, >> ImplicitOps|ModRM, >> +DstMem|SrcImplicit|ModRM, DstMem|SrcImplicit|ModRM, >> +DstImplicit|SrcMem|ModRM, DstImplicit|SrcMem|ModRM, > > These are Src/Dst Reg rather than Mem, although its not immediately > obvious how this would alter the outcome. No - SrcReg / DstReg describe operands encoded in bit 3..5 of a ModRM byte, whereas SrcMem / DstMem describe such encoded in bits 0..2 together with 6..7. The fact that mod is required to be 3 for these insns isn't being expressed in the static table at all. (And I guess you then understand how getting this wrong here would affect the outcome.) Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [xen-unstable-smoke test] 104097: tolerable all pass - PUSHED
flight 104097 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/104097/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 12 migrate-support-checkfail never pass test-armhf-armhf-xl 12 migrate-support-checkfail never pass test-armhf-armhf-xl 13 saverestore-support-checkfail never pass version targeted for testing: xen 9d81162fd2c69b5c9279670c8739d891cf7571da baseline version: xen dc562334db2b1fc232dda884f84bb0172e1d1480 Last test of basis 104082 2017-01-09 13:01:27 Z0 days Testing same since 104097 2017-01-10 11:01:32 Z0 days1 attempts People who touched revisions under test: Anthony PERARD Doug Goldstein Eric DeVolder He Chen Ian Jackson Jan Beulich Juergen Gross Wei Liu jobs: build-amd64 pass build-armhf pass build-amd64-libvirt pass test-armhf-armhf-xl pass test-amd64-amd64-xl-qemuu-debianhvm-i386 pass test-amd64-amd64-libvirt pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Pushing revision : + branch=xen-unstable-smoke + revision=9d81162fd2c69b5c9279670c8739d891cf7571da + . ./cri-lock-repos ++ . ./cri-common +++ . ./cri-getconfig +++ umask 002 +++ getrepos getconfig Repos perl -e ' use Osstest; readglobalconfig(); print $c{"Repos"} or die $!; ' +++ local repos=/home/osstest/repos +++ '[' -z /home/osstest/repos ']' +++ '[' '!' -d /home/osstest/repos ']' +++ echo /home/osstest/repos ++ repos=/home/osstest/repos ++ repos_lock=/home/osstest/repos/lock ++ '[' x '!=' x/home/osstest/repos/lock ']' ++ OSSTEST_REPOS_LOCK_LOCKED=/home/osstest/repos/lock ++ exec with-lock-ex -w /home/osstest/repos/lock ./ap-push xen-unstable-smoke 9d81162fd2c69b5c9279670c8739d891cf7571da + branch=xen-unstable-smoke + revision=9d81162fd2c69b5c9279670c8739d891cf7571da + . ./cri-lock-repos ++ . ./cri-common +++ . ./cri-getconfig +++ umask 002 +++ getrepos getconfig Repos perl -e ' use Osstest; readglobalconfig(); print $c{"Repos"} or die $!; ' +++ local repos=/home/osstest/repos +++ '[' -z /home/osstest/repos ']' +++ '[' '!' -d /home/osstest/repos ']' +++ echo /home/osstest/repos ++ repos=/home/osstest/repos ++ repos_lock=/home/osstest/repos/lock ++ '[' x/home/osstest/repos/lock '!=' x/home/osstest/repos/lock ']' + . ./cri-common ++ . ./cri-getconfig ++ umask 002 + select_xenbranch + case "$branch" in + tree=xen + xenbranch=xen-unstable-smoke + qemuubranch=qemu-upstream-unstable + '[' xxen = xlinux ']' + linuxbranch= + '[' xqemu-upstream-unstable = x ']' + select_prevxenbranch ++ ./cri-getprevxenbranch xen-unstable-smoke + prevxenbranch=xen-4.8-testing + '[' x9d81162fd2c69b5c9279670c8739d891cf7571da = x ']' + : tested/2.6.39.x + . ./ap-common ++ : osst...@xenbits.xen.org +++ getconfig OsstestUpstream +++ perl -e ' use Osstest; readglobalconfig(); print $c{"OsstestUpstream"} or die $!; ' ++ : ++ : git://xenbits.xen.org/xen.git ++ : osst...@xenbits.xen.org:/home/xen/git/xen.git ++ : git://xenbits.xen.org/qemu-xen-traditional.git ++ : git://git.kernel.org ++ : git://git.kernel.org/pub/scm/linux/kernel/git ++ : git ++ : git://xenbits.xen.org/xtf.git ++ : osst...@xenbits.xen.org:/home/xen/git/xtf.git ++ : git://xenbits.xen.org/xtf.git ++ : git://xenbits.xen.org/libvirt.git ++ : osst...@xenbits.xen.org:/home/xen/git/libvirt.git ++ : git://xenbits.xen.org/libvirt.git ++ : git://xenbits.xen.org/osstest/rumprun.git ++ : git ++ : git://xenbits.xen.org/osstest/rumprun.git ++ : osst...@xenbits.xen.org:/home/xen/git/osstest/rumprun.git ++ : git://git.seabios.org/seabios.git ++ : osst...@xenbits.xen.org:/home/xen/git/osstest/seabios.git ++ : git://xenbits.xen.org/osstest/seabios.git ++ : https://github.com/tianocore/edk2.git ++ : osst...@xenbits.xen.org:/home/xen/git/osstest/ovmf.git ++ : git://xenbits.xen.org/osstest/ovmf.git ++ : git://xenbits.xen.org/osstest/linux-firmware.git ++ : osst...@xenbit