[Xen-devel] subscribe
___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCHv2 1/3] x86/fpu: improve check for XSAVE* not writing FIP/FDP fields
Jan:
We have an answer from the architects:
"We can confirm that our CPUs do treat FIP as a 48-bit pointer. When loaded by
one of the restore instructions, bit 47 is sign-extended into bits 63:48,
making the result canonical. As a result, the save instructions will always
save a canonical address for FIP."
Thanks for your patience,
-Paul
-Original Message-
From: xen-devel-boun...@lists.xen.org [mailto:xen-devel-boun...@lists.xen.org]
On Behalf Of Jan Beulich
Sent: Wednesday, February 24, 2016 2:50 AM
Subject: Re: [Xen-devel] [PATCHv2 1/3] x86/fpu: improve check for XSAVE* not
writing FIP/FDP fields
>>> On 24.02.16 at 11:37, wrote:
> Sorry I didn't quite get the question here. Could anyone of you write
> down a standalone description of the problem then I can forward
> internally to confirm since my translation might be inaccurate here?
What we'd like to get formally stated is whether FIP is guaranteed to be
treated as 48-bit pointer, which upon loading/storing by 64-bit
{F,}X{XSAVE,RSTOR} will get truncated/canonicalized. With FDP being a full
64-bit pointer on Intel CPUs (but only a 48 bit one on AMD ones), and both your
and their manuals implicitly describing both as full 64-bit fields, FIP
potentially also being a full 64-bit field on past, present, or future CPUs
would render David's intended code improvement unsafe.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[Xen-devel] Altp2m misbehavior
Hello list: Been playing with the altp2m code and noticed that it's not behaving in Xen 4.7 tree. Two changesets have been identified as where the be behavior changes. The change is commit bd2239d9fa975a1ee5bcd27c218ae042cd0a57bc, x86/HVM: always intercept #AC and #DB where an executable goes from working to crashing after a 10+ hypercalls. The second change is in commit 484c14b7254e8d8936c05e3c28e332ea825c0155: x86/vmx: enable PML by default This hangs the VM guest on the first few hypercalls. -Paul ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] Patch to honor --enable-githttp
The following patch fixes the toplevel Xen makefile gereration to honor
-enable-githttp if the option is used during configuration.
commit 040510913564ec7e9f46ce833f8bdf1a40950ff7
Author: Paul Lai
Date: Mon Apr 11 10:43:57 2016 -0700
Honor '--enable-githttp' in toplevel Makefile generation
During the make world, git mini-os.git didn't honor the 'configure
--enable-githttp' option. The 'enable-githttp' was only honored in
the tools subdirectory.
diff --git a/config/Toplevel.mk.in b/config/Toplevel.mk.in
index 4db7eaf..1d99189 100644
--- a/config/Toplevel.mk.in
+++ b/config/Toplevel.mk.in
@@ -1 +1,2 @@
SUBSYSTEMS := @SUBSYSTEMS@
+GIT_HTTP := @githttp@
diff --git a/configure b/configure
index 3c269fa..78e8025 100755
--- a/configure
+++ b/configure
@@ -594,6 +594,7 @@ stubdom
tools
xen
subdirs
+githttp
XEN_DUMP_DIR
XEN_PAGING_DIR
XEN_LOCK_DIR
@@ -660,6 +661,7 @@ enable_option_checking
with_initddir
with_sysconfig_leaf_dir
with_xen_dumpdir
+enable_githttp
enable_xen
enable_tools
enable_stubdom
@@ -1284,6 +1286,8 @@ Optional Features:
--disable-option-checking ignore unrecognized --enable/--with options
--disable-FEATURE do not include FEATURE (same as --enable-FEATURE=no)
--enable-FEATURE[=ARG] include FEATURE [ARG=yes]
+ --enable-githttpDownload GIT repositories via HTTP (default is
+ DISABLED)
--disable-xen Disable build and install of xen
--disable-tools Disable build and install of tools
--enable-stubdomEnable build and install of stubdom
@@ -1985,6 +1989,29 @@ XEN_DUMP_DIR=$xen_dumpdir_path
+# Check whether --enable-githttp was given.
+if test "${enable_githttp+set}" = set; then :
+ enableval=$enable_githttp;
+fi
+
+
+if test "x$enable_githttp" = "xno"; then :
+
+ax_cv_githttp="n"
+
+elif test "x$enable_githttp" = "xyes"; then :
+
+ax_cv_githttp="y"
+
+elif test -z $ax_cv_githttp; then :
+
+ax_cv_githttp="n"
+
+fi
+githttp=$ax_cv_githttp
+
+
+
case "$host_cpu" in
i[3456]86|x86_64)
arch_enable_stubdom=y
diff --git a/configure.ac b/configure.ac
index 1843b52..7388b28 100644
--- a/configure.ac
+++ b/configure.ac
@@ -17,6 +17,7 @@ m4_include([m4/subsystem.m4])
m4_include([m4/paths.m4])
AX_XEN_EXPAND_CONFIG()
+AX_ARG_DEFAULT_DISABLE([githttp], [Download GIT repositories via HTTP])
dnl mini-os is only ported to certain platforms
case "$host_cpu" in
enable_githttp.patch
Description: enable_githttp.patch
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] Patch to honor --enable-githttp
Wei: I'm not sure what "refreshing this patch" this patch means. I just 'git pull origin master' from origin ( http://xenbits.xen.org/git-http/xen.git ) and rebased my work branch with this patch on it cleanly. No conflicts to resolve. Looking to help out, -Paul -Original Message- From: Wei Liu [mailto:wei.l...@citrix.com] Sent: Wednesday, May 4, 2016 2:42 AM To: Lai, Paul C Cc: xen-de...@lists.xenproject.org; Wei Liu Subject: Re: [Xen-devel] Patch to honor --enable-githttp Hi Paul We are about to release a new version of Xen soon. I think this patch is nice to have for the new release. Would you be up for refreshing this patch? This is also a good chance to get yourself started with Xen development. Feel free to ask me any question. Thanks Wei. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] Patch to honor --enable-githttp
Just re-submitted via `git send-email` ... hopefully the formatting is to your liking. Please let me know otherwise. -Paul -Original Message- From: Wei Liu [mailto:wei.l...@citrix.com] Sent: Wednesday, May 4, 2016 9:18 AM To: Lai, Paul C Cc: Wei Liu ; xen-de...@lists.xenproject.org Subject: Re: [Xen-devel] Patch to honor --enable-githttp On Wed, May 04, 2016 at 04:05:21PM +, Lai, Paul C wrote: > Wei: > I'm not sure what "refreshing this patch" this patch means. > That means to add your SoB (and go through other stuff mentioned in our contribution guideline wiki page) and resend it to xen-devel. > I just 'git pull origin master' from origin ( > http://xenbits.xen.org/git-http/xen.git ) and rebased my work branch with > this patch on it cleanly. No conflicts to resolve. > There are two reasons that your patch is not yet ready: 1. there is no Signed-off-by tag; 2. it is not sent out correctly so it can't be applied cleanly. So you do need to fix outstanding issues before we can accept it. These topics are both covered in the wiki page (Signing off patch and sending patch). If you're interested in how Xen community do development I suggest you go through that page. Or do you want me to just take over the patch? I can do that, too. In that case you still need to give me your SoB. If you have more questions, feel free to ask. Wei. > Looking to help out, > -Paul > > -Original Message- > From: Wei Liu [mailto:wei.l...@citrix.com] > Sent: Wednesday, May 4, 2016 2:42 AM > To: Lai, Paul C > Cc: xen-de...@lists.xenproject.org; Wei Liu > Subject: Re: [Xen-devel] Patch to honor --enable-githttp > > Hi Paul > > We are about to release a new version of Xen soon. I think this patch is nice > to have for the new release. Would you be up for refreshing this patch? > > This is also a good chance to get yourself started with Xen development. > Feel free to ask me any question. > > Thanks > Wei. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH Altp2m cleanup v4 1/4] x86/HVM: adjust feature checking in MSR intercept handling
[Paul] in-line -Original Message- From: Jan Beulich [mailto:jbeul...@suse.com] Sent: Thursday, September 8, 2016 3:33 AM To: Lai, Paul C Cc: george.dun...@citrix.com; Sahita, Ravi ; xen-devel Subject: Re: [PATCH Altp2m cleanup v4 1/4] x86/HVM: adjust feature checking in MSR intercept handling >>> On 08.09.16 at 00:04, wrote: > From: Jan Beulich > > Consistently consult hvm_cpuid(). With that, BNDCFGS gets better > handled outside of VMX specific code, just like XSS. Don't needlessly > check for MTRR support when the MSR being accessed clearly is not an > MTRR one. > > Signed-off-by: Jan Beulich > Reviewed-by: Andrew Cooper Why did you (re)send this? It went in yesterday together with its VMX prereq. Without that prereq it's useless (as it won't apply), and if you worked on a tree where the prereq was already present, then this one would have been present too (as they got pushed at the same time). Jan [Paul] Argh -- looks like I used the wrong commit# during the git send-email command grrr... Apologies ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH Altp2m cleanup v4 2/4] altp2m cleanup work
[Paul] in-line -Original Message- From: Jan Beulich [mailto:jbeul...@suse.com] Sent: Thursday, September 8, 2016 7:47 AM To: Lai, Paul C Cc: george.dun...@citrix.com; Sahita, Ravi ; xen-devel Subject: Re: [PATCH Altp2m cleanup v4 2/4] altp2m cleanup work >>> On 08.09.16 at 00:04, wrote: > Indent goto labels by one space > Inline (header) altp2m functions > Define default behavior in switch > Define max and min for range of altp2m macroed values > > Signed-off-by: Paul Lai > --- Missing a brief summary of changes from previous version here. > @@ -5413,6 +5426,8 @@ static int do_altp2m_op( > rc = p2m_change_altp2m_gfn(d, a.u.change_gfn.view, > _gfn(a.u.change_gfn.old_gfn), > _gfn(a.u.change_gfn.new_gfn)); > +default: > +ASSERT_UNREACHABLE(); > } Did you test anything using HVMOP_altp2m_change_gfn with this change, on a debug build? There's obviously an unintended fall- through right now. [Paul] - Yes, this patch series was tested with Tamas's HVMOP_altp2m_change_gfn in a debug build. > /* emulates #VE */ > -bool_t altp2m_vcpu_emulate_ve(struct vcpu *v); > +static inline bool_t altp2m_vcpu_emulate_ve(struct vcpu *v) Already on v3 I had asked to switch to plain bool (and true/false as appropriate). [Paul] - Maybe I misunderstood something here. I fixed the return value to false in the patch. ... Ah, it's the non-false case that's returning void. Will fix. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH Altp2m cleanup v4 2/4] altp2m cleanup work
[Paul2] in-line -Original Message- From: Tamas K Lengyel [mailto:tamas.k.leng...@gmail.com] Sent: Thursday, September 8, 2016 9:07 AM To: Lai, Paul C Cc: Jan Beulich ; xen-devel ; Sahita, Ravi ; george.dun...@citrix.com Subject: Re: [Xen-devel] [PATCH Altp2m cleanup v4 2/4] altp2m cleanup work On Thu, Sep 8, 2016 at 9:50 AM, Lai, Paul C wrote: > [Paul] in-line > > -Original Message- > From: Jan Beulich [mailto:jbeul...@suse.com] > Sent: Thursday, September 8, 2016 7:47 AM > To: Lai, Paul C > Cc: george.dun...@citrix.com; Sahita, Ravi ; > xen-devel > Subject: Re: [PATCH Altp2m cleanup v4 2/4] altp2m cleanup work > >>>> On 08.09.16 at 00:04, wrote: >> Indent goto labels by one space >> Inline (header) altp2m functions >> Define default behavior in switch >> Define max and min for range of altp2m macroed values >> >> Signed-off-by: Paul Lai >> --- > > Missing a brief summary of changes from previous version here. > >> @@ -5413,6 +5426,8 @@ static int do_altp2m_op( >> rc = p2m_change_altp2m_gfn(d, a.u.change_gfn.view, >> _gfn(a.u.change_gfn.old_gfn), >> _gfn(a.u.change_gfn.new_gfn)); >> +default: >> +ASSERT_UNREACHABLE(); >> } > > Did you test anything using HVMOP_altp2m_change_gfn with this change, on a > debug build? There's obviously an unintended fall- through right now. > > [Paul] - Yes, this patch series was tested with Tamas's > HVMOP_altp2m_change_gfn in a debug build. Not sure what you used here, the xen-access tool right now in Xen doesn't (yet) exercise the gfn remapping, only the mem-access parts. Sergej has a patch for this in the arm-altp2m series though. Tamas [Paul2] All I was testing was if xen-access behaved as before the gfn remapping patch was introduced. After the gfn remapping patch was introduced, 'xen-acess -m 1 altp2m_write' hanged the system. Now, with your fix, it doesn't and the output of xen-access appears as before. Thanks, -Paul ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH Altp2m cleanup v4 3/4] Move altp2m specific functions to altp2m files.
[Paul2] in-line
-Original Message-
From: Jan Beulich [mailto:jbeul...@suse.com]
Sent: Thursday, September 8, 2016 8:02 AM
To: Lai, Paul C
Cc: george.dun...@citrix.com; Sahita, Ravi ; xen-devel
Subject: Re: [PATCH Altp2m cleanup v4 3/4] Move altp2m specific functions to
altp2m files.
>>> On 08.09.16 at 00:04, wrote:
> @@ -65,6 +66,56 @@ altp2m_vcpu_destroy(struct vcpu *v)
> vcpu_unpause(v);
> }
>
> +int
> +altp2m_domain_init(struct domain *d)
> +{
> +int rc;
> +unsigned int i;
> +
> +if ( !hvm_altp2m_supported() )
> +{
> +rc = 0;
> +goto out;
> +}
> +/* Init alternate p2m data. */
> +if ( (d->arch.altp2m_eptp = alloc_xenheap_page()) == NULL )
> +{
> +rc = -ENOMEM;
> +goto out;
> +}
> +
> +for ( i = 0; i < MAX_EPTP; i++ )
> +d->arch.altp2m_eptp[i] = mfn_x(INVALID_MFN);
> +
> +for ( i = 0; i < MAX_ALTP2M; i++ )
> +{
> +rc = p2m_alloc_table(d->arch.altp2m_p2m[i]);
> +if ( rc != 0 )
> + goto out;
> +}
> +
> +d->arch.altp2m_active = 0;
> + out:
> +return rc;
> +}
I don't follow: I did specifically ask to avoid goto when where the label is
there's just a single statement (return in this case).
[Paul2] In the v3 2/3 patch series you said:
[Jan] When the epilogue (after the target label) is just a return statement,
please avoid using goto.
[PAUL] I don't see this code in an epilogue (after the target label).
[Paul2] I didn't get an answer to the question. After scratching my head, just
realized you want the 'goto' to become a 'return'. I will make this change in
order to pass your review (and the coding style of Xen). That said, I'm a
believer in one exit per function -- to me, this is a much cleaner coding
style. I'm not trying to start a debate, I'm just explaining how I was
confused.
> +void
> +altp2m_domain_teardown(struct domain *d) {
> +unsigned int i;
> +
> +if ( !hvm_altp2m_supported() )
> + return;
Bad tab indentation.
> @@ -499,27 +500,7 @@ int hap_enable(struct domain *d, u32 mode)
> goto out;
> }
>
> -if ( hvm_altp2m_supported() )
> -{
> -/* Init alternate p2m data */
> -if ( (d->arch.altp2m_eptp = alloc_xenheap_page()) == NULL )
> -{
> -rv = -ENOMEM;
> -goto out;
> -}
> -
> -for ( i = 0; i < MAX_EPTP; i++ )
> -d->arch.altp2m_eptp[i] = mfn_x(INVALID_MFN);
> -
> -for ( i = 0; i < MAX_ALTP2M; i++ )
> -{
> -rv = p2m_alloc_table(d->arch.altp2m_p2m[i]);
> -if ( rv != 0 )
> - goto out;
> -}
> -
> -d->arch.altp2m_active = 0;
> -}
> +rv = altp2m_domain_init(d);
>
> /* Now let other users see the new mode */
> d->arch.paging.mode = mode | PG_HAP_enable;
I don't think you should reach the following statement(s) when your function
returned an error. Even if this might be benign now, this is easy to overlook
if later more code gets added near the end of the function.
Also I wonder whether in an error case there's not a possibility for memory to
be leaked. That wouldn't be a problem your patch introduces, but I think you
could have noticed and fixed this as you touch the code anyway.
[Paul2] Good catch
> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -1329,6 +1329,45 @@ void setup_ept_dump(void)
> register_keyhandler('D', ept_dump_p2m_table, "dump VT-x EPT
> tables", 0); }
>
> +void p2m_init_altp2m_ept( struct domain *d, unsigned int i)
Another instance of you not having corrected what was previously pointed out:
There's a stray blank here. And even if I hadn't said there are multiple
instances of this, you should still apply such comments to all of your series.
And I only now notice that this e.g.
also applies to the suggested re-ordering of operations in
altp2m_domain_teardown(). I think I'm going to stop reviewing this series here:
Please make sure you address all review comments (either verbally or by code
adjustment) before submitting a new version (or in extreme cases, like due to
lack of feedback, point out open issues right after the first --- separator).
[Paul2] I did do a sweep for the stray blanks (and other like typos/style
issues), but your eyes are much better trained. Will do again.
[Paul 2] Thanks for the quick feedback.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
[Xen-devel] trying to get started w/ osstest
I'm looking to get started with osstest and running to some roadblocks.
The page
https://blog.xenproject.org/2013/02/02/xen-automatic-test-system-osstest/ looks
like the place to begin, but
1. The README link returns 404 (file not found)
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=refs/heads/standalone
2. When I try to clone the project via the https: URL, another file not
found error returns. The URL tried was
https://xenbits.xen.org/git-http/osstest.git.
And it was taken from http://xenbits.xen.org/gitweb/?p=osstest.git;a=summary,
[ No, I can't use the git: address due to a company firewall. Yes, I can use
an http{s}:
URL for the regular Xen sources. ]
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v1 Altp2m cleanup 0/3] Cleaning up altp2m code
Jan: Ok... adding back xen-devel. Thanks for the pointer to the thread. I'm looking at the existing Xen code and the changes mentioned by George (and supported by Tim) are already in. The patch suggested by George is http://lists.xenproject.org/archives/html/xen-devel/2015-07/txtEYYYL6ATei.txt from George's post of http://lists.xenproject.org/archives/html/xen-devel/2015-07/msg01088.html The current code has the following snippets, which match/mirror the intent of the above referenced patch. xen/arch/x86/mm/mem_sharing.c: ... +bool_t sve; if ( atomic_read(&d->shr_pages) == 0 ) break; -mfn = p2m->get_entry(p2m, gfn, &t, &a, 0, NULL); +mfn = p2m->get_entry(p2m, gfn, &t, &a, 0, NULL, NULL); xen/arch/x86/mm/p2m-ept.c: if ( sve != -1 ) new_entry.suppress_ve = !!sve; else new_entry.suppress_ve = is_epte_valid(&old_entry) ? old_entry.suppress_ve : 1; ... [WRT side note: it's easier to see what your referring to if you present what you see. If I search, I'm guessing at what you're referring to. Often (at least for me), I end up have two different conversations.] Regards, -Paul -Original Message- From: Jan Beulich [mailto:jbeul...@suse.com] Sent: Wednesday, June 22, 2016 8:54 AM To: Lai, Paul C Cc: Sahita, Ravi Subject: RE: [PATCH v1 Altp2m cleanup 0/3] Cleaning up altp2m code >>> On 22.06.16 at 17:26, wrote: > Dropping Xen-devel. Not sure why, but anyway. > Jan/Ravi: > > Please send me reminder for the " main work item was to get rid of the > multitude of function parameters the set_entry()". This wasn't in the > instructions I (if I recall correctly) was given. See for example http://lists.xenproject.org/archives/html/xen-devel/2015-07/msg01331.html and the surrounding thread, as well as replies to later versions of this same patch. (As a side note, it's not clear to me why you made me search xen-devel for this discussion, when you could have just as easily done this yourself.) Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v1 Altp2m cleanup 1/3] altp2m cleanup work
I'm opposed to moving HVMOP_cmd_min and HVMOP_cmd_max somewhere else. That
would make reading/understanding of the macros more difficult. This practice
is common. Also, If min & max are defined elsewhere, it will be more likely to
lead to mistakes/bugs.
The use of "_min" and "_max" should be quite clear and is common use in linux
code; Yes, I know this is xen code and I see it here too. If there's a better
way, please propose the better. Maybe you're suggesting the macro names should
be all caps:
HVMOP_CMD_MIN, HVMOP_CMD_MAX
? I was following the coding style within the file itself.
I'm open to suggestions,
-Paul
-Original Message-
From: Jan Beulich [mailto:jbeul...@suse.com]
Sent: Thursday, June 23, 2016 8:45 AM
To: Lai, Paul C
Cc: Sahita, Ravi ; xen-devel
Subject: Re: [PATCH v1 Altp2m cleanup 1/3] altp2m cleanup work
>>> On 21.06.16 at 18:04, wrote:
> --- a/xen/include/public/hvm/hvm_op.h
> +++ b/xen/include/public/hvm/hvm_op.h
> @@ -479,6 +479,8 @@ struct xen_hvm_altp2m_op {
> #define HVMOP_altp2m_set_mem_access 7
> /* Change a p2m entry to have a different gfn->mfn mapping */
> #define HVMOP_altp2m_change_gfn 8
> +#define HVMOP_cmd_min HVMOP_altp2m_get_domain_state
> +#define HVMOP_cmd_max HVMOP_altp2m_change_gfn
I don't see why these would need to be in the public interface. Nor were their
names chosen to properly describe their purpose.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v1 Altp2m cleanup 1/3] altp2m cleanup work
[Paul] inlined -Original Message- From: Jan Beulich [mailto:jbeul...@suse.com] Sent: Thursday, June 23, 2016 11:19 PM To: Lai, Paul C Cc: Sahita, Ravi ; xen-devel Subject: RE: [PATCH v1 Altp2m cleanup 1/3] altp2m cleanup work >>> On 23.06.16 at 20:23, wrote: First of all - please don't top post. [Paul] Understood > I'm opposed to moving HVMOP_cmd_min and HVMOP_cmd_max somewhere else. > That would make reading/understanding of the macros more difficult. > This practice is common. Also, If min & max are defined elsewhere, it > will be more likely to lead to mistakes/bugs. I understand that, but I'm going to nak any patch that introduces clutter (which then will need to be retained forever) to the public interface. A possible compromise might be to frame these in __XEN__ conditionals, but this would need to be outweighed by the resulting benefits, which I'm not convinced of. [Paul] Still waiting for a better idea. > The use of "_min" and "_max" should be quite clear and is common use > in linux code; Yes, I know this is xen code and I see it here too. If > there's a better way, please propose the better. Maybe you're > suggesting the macro names should be all caps: > HVMOP_CMD_MIN, HVMOP_CMD_MAX > ? I was following the coding style within the file itself. The problem isn't the use of min and max, but the selected names suggesting these are the minimum/maximum HVMOPs, whereas - afaict - you really mean to frame the altp2m subset. Which btw, together with the above, points at another issue here: What if later another altp2m subop gets added, and especially when its new value ends up not being adjacent to the existing range? [Paul] Again, waiting for a better idea. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 Altp2m cleanup 0/3] Cleaning up altp2m code
Please disregard this submission for review. I forgot to rebase to current master and this patch is against a code base that is out of date. Apologies for the noise. -Original Message- From: Xen-devel [mailto:xen-devel-boun...@lists.xen.org] On Behalf Of Paul Lai Sent: Wednesday, August 3, 2016 12:17 PM To: xen-de...@lists.xensource.com Cc: dunl...@gmail.com; Sahita, Ravi ; jbeul...@suse.com Subject: [Xen-devel] [PATCH v2 Altp2m cleanup 0/3] Cleaning up altp2m code Incorporating comments from v1 altp2m clean code URLs: https://lists.xenproject.org/archives/html/xen-devel/2016-06/msg03223.html https://lists.xenproject.org/archives/html/xen-devel/2016-06/msg03465.html https://lists.xenproject.org/archives/html/xen-devel/2016-06/msg03467.html Older comments, reason for the code clean effort, are the following URLs: https://lists.xenproject.org/archives/html/xen-devel/2015-07/msg04323.html https://lists.xenproject.org/archives/html/xen-devel/2015-07/msg04454.html https://lists.xenproject.org/archives/html/xen-devel/2015-07/msg04530.html Paul Lai (3): altp2m cleanup work Move altp2m specific functions to altp2m files. Making altp2m struct dynamically allocated. xen/arch/x86/hvm/hvm.c| 54 +--- xen/arch/x86/hvm/vmx/vmx.c| 2 +- xen/arch/x86/mm/altp2m.c | 45 + xen/arch/x86/mm/hap/hap.c | 35 ++--- xen/arch/x86/mm/mm-locks.h| 4 +- xen/arch/x86/mm/p2m-ept.c | 39 ++ xen/arch/x86/mm/p2m.c | 104 +- xen/include/asm-x86/altp2m.h | 11 +++- xen/include/asm-x86/domain.h | 5 +- xen/include/asm-x86/hvm/hvm.h | 19 +-- xen/include/asm-x86/hvm/vmx/vmx.h | 3 ++ xen/include/asm-x86/p2m.h | 18 --- 12 files changed, 193 insertions(+), 146 deletions(-) -- 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 v2 Altp2m cleanup v3 2/3] Move altp2m specific functions to altp2m files.
[PAUL] in-line
Ravi -- please comment on swap comment below.
-Original Message-
From: Jan Beulich [mailto:jbeul...@suse.com]
Sent: Friday, September 2, 2016 6:31 AM
To: Lai, Paul C
Cc: george.dun...@citrix.com; Sahita, Ravi ; xen-devel
Subject: Re: [PATCH v2 Altp2m cleanup v3 2/3] Move altp2m specific functions to
altp2m files.
>>> On 19.08.16 at 19:22, wrote:
> @@ -65,6 +66,50 @@ altp2m_vcpu_destroy(struct vcpu *v)
> vcpu_unpause(v);
> }
>
> +int
> +hvm_altp2m_init( struct domain *d)
Stray blank (more elsewhere). Also I don't think hvm_ is a proper prefix for a
function placed in this file, i.e. if altp2m_init() is used elsewhere already,
then altp2m_hvm_init() or perhaps better
altp2m_domain_init() please.
> +{
> +int rc = 0;
> +unsigned int i = 0;
Pointless initializers.
> +/* Init alternate p2m data. */
> +if ( (d->arch.altp2m_eptp = alloc_xenheap_page()) == NULL )
> +{
> +rc = -ENOMEM;
> +goto out;
> +}
When the epilogue (after the target label) is just a return statement, please
avoid using goto.
[PAUL] I don't see this code in an epilogue (after the target label).
> +void
> +hvm_altp2m_teardown( struct domain *d) {
> +unsigned int i = 0;
> +d->arch.altp2m_active = 0;
> +
> +if ( d->arch.altp2m_eptp )
> +{
> +free_xenheap_page(d->arch.altp2m_eptp);
> +d->arch.altp2m_eptp = NULL;
> +}
Please avoid the if() - free_xenheap_page() happily deals with a NULL pointer
passed to it.
> +for ( i = 0; i < MAX_ALTP2M; i++ )
> +p2m_teardown(d->arch.altp2m_p2m[i]);
I realize it's been that way in the original code, but wouldn't swapping the
two things be both more natural and more robust?
[PAUL] Ravi ?
> @@ -501,24 +502,9 @@ int hap_enable(struct domain *d, u32 mode)
>
> if ( hvm_altp2m_supported() )
> {
> -/* Init alternate p2m data */
> -if ( (d->arch.altp2m_eptp = alloc_xenheap_page()) == NULL )
> -{
> -rv = -ENOMEM;
> -goto out;
> -}
> -
> -for ( i = 0; i < MAX_EPTP; i++ )
> -d->arch.altp2m_eptp[i] = mfn_x(INVALID_MFN);
> -
> -for ( i = 0; i < MAX_ALTP2M; i++ )
> -{
> -rv = p2m_alloc_table(d->arch.altp2m_p2m[i]);
> -if ( rv != 0 )
> - goto out;
> -}
> -
> -d->arch.altp2m_active = 0;
> +rv = hvm_altp2m_init(d);
> +if ( rv != 0 )
> + goto out;
> }
>
> /* Now let other users see the new mode */ @@ -534,18 +520,7 @@
> void hap_final_teardown(struct domain *d)
> unsigned int i;
>
> if ( hvm_altp2m_supported() )
> -{
> -d->arch.altp2m_active = 0;
> -
> -if ( d->arch.altp2m_eptp )
> -{
> -free_xenheap_page(d->arch.altp2m_eptp);
> -d->arch.altp2m_eptp = NULL;
> -}
> -
> -for ( i = 0; i < MAX_ALTP2M; i++ )
> -p2m_teardown(d->arch.altp2m_p2m[i]);
> -}
> +hvm_altp2m_teardown(d);
I wonder whether in both cases the hvm_altp2m_supported() would better also be
moved into the functions.
It looks like the parts above and below this point, except for header file
adjustments, are completely independent. Please consider splitting into two
patches.
> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -1329,6 +1329,45 @@ void setup_ept_dump(void)
> register_keyhandler('D', ept_dump_p2m_table, "dump VT-x EPT
> tables", 0); }
>
> +void p2m_init_altp2m_ept_helper( struct domain *d, unsigned int i)
Please drop the _helper suffix now that there is _ept.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 Altp2m cleanup v3 3/3] Making altp2m struct dynamically allocated.
[PAUL] in line
-Original Message-
From: Jan Beulich [mailto:jbeul...@suse.com]
Sent: Friday, September 2, 2016 6:47 AM
To: Lai, Paul C
Cc: george.dun...@citrix.com; Sahita, Ravi ; xen-devel
Subject: Re: [PATCH v2 Altp2m cleanup v3 3/3] Making altp2m struct dynamically
allocated.
>>> On 19.08.16 at 19:22, wrote:
> Ravi Sahita's dynamically allocated altp2m structs
I think I've asked before: With this and ...
> Signed-off-by: Paul Lai
> Reviewed-by: Ravi Sahita
... this - who's the actual author?
[PAUL] Ravi is the actual author. I thought the commit message would have been
clear :(
> @@ -5279,11 +5279,11 @@ static int do_altp2m_op(
> break;
> }
>
> -ostate = d->arch.altp2m_active;
> -d->arch.altp2m_active = !!a.u.domain_state.state;
> +ostate = altp2m_active(d);
> +set_altp2m_active(d, !!a.u.domain_state.state);
The !! shouldn't be needed anymore.
> --- a/xen/arch/x86/mm/altp2m.c
> +++ b/xen/arch/x86/mm/altp2m.c
> @@ -73,23 +73,23 @@ hvm_altp2m_init( struct domain *d)
> unsigned int i = 0;
>
> /* Init alternate p2m data. */
> -if ( (d->arch.altp2m_eptp = alloc_xenheap_page()) == NULL )
> +if ( (d->arch.altp2m->altp2m_eptp = alloc_xenheap_page()) == NULL
> + )
> {
> rc = -ENOMEM;
> goto out;
> }
>
> for ( i = 0; i < MAX_EPTP; i++ )
> -d->arch.altp2m_eptp[i] = mfn_x(INVALID_MFN);
> +d->arch.altp2m->altp2m_eptp[i] = mfn_x(INVALID_MFN);
>
> for ( i = 0; i < MAX_ALTP2M; i++ )
> {
> -rc = p2m_alloc_table(d->arch.altp2m_p2m[i]);
> +rc = p2m_alloc_table(d->arch.altp2m->altp2m_p2m[i]);
> if ( rc != 0 )
> goto out;
> }
>
> -d->arch.altp2m_active = 0;
> +set_altp2m_active(d, 0);
"false" please (also elsewhere).
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -193,12 +193,15 @@ static void p2m_teardown_altp2m(struct domain
> *d)
>
> for ( i = 0; i < MAX_ALTP2M; i++ )
> {
> -if ( !d->arch.altp2m_p2m[i] )
> +if ( !d->arch.altp2m->altp2m_p2m[i] )
> continue;
> -p2m = d->arch.altp2m_p2m[i];
> +p2m = d->arch.altp2m->altp2m_p2m[i];
> p2m_free_one(p2m);
> -d->arch.altp2m_p2m[i] = NULL;
> +d->arch.altp2m->altp2m_p2m[i] = NULL;
> }
> +
> +if ( d->arch.altp2m )
> +xfree(d->arch.altp2m);
Two problems here: First, xfree() happily deals with NULL being passed. But
then, such a NULL check clearly should not come _after_ the pointer did already
get dereferenced. I.e. first of all you need to clarify for yourself whether
the function can be called with the pointer being NULL.
[PAUL] great catch
> @@ -206,10 +209,14 @@ static int p2m_init_altp2m(struct domain *d)
And then, considering this is the last patch in the series - how come these two
functions are still in this source file?
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -338,6 +338,13 @@ struct p2m_domain {
> };
> };
>
> +struct altp2m_domain {
> +bool_t altp2m_active;
> +struct p2m_domain *altp2m_p2m[MAX_ALTP2M];
> +mm_lock_t altp2m_list_lock;
> +uint64_t *altp2m_eptp;
> +};
None of the altp2m_ prefixes here are really useful for anything afaics.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
