[PATCH 0/6] xen/pvshim: fix for staging

[PATCH 0/6] xen/pvshim: fix for staging

[Roger Pau Monne]

Hello,

The following series contain some code and style fixes for the pvshim
(comet) code that has been merged into staging. IMHO patches 1-3 should
be backported to the stable comet branch.

A branch with the series is also available at:

git://xenbits.xen.org/people/royger/xen.git pvshim_fixes_v1

Thanks, Roger.

Roger Pau Monne (6):
  xen/pvshim: map vcpu_info earlier for APs
  xen/pvh: place the trampoline at page 0x1
  xen/pvshim: identity pin shim vCPUs to pCPUs
  xen/pvshim: simplify replace_va_mapping code
  xen/pvshim: fix coding style issues
  firmware/shim: fix build process to use POSIX find options

 tools/firmware/xen-dir/Makefile |  3 ++-
 xen/arch/x86/boot/head.S        |  3 +++
 xen/arch/x86/dom0_build.c       |  5 ++---
 xen/arch/x86/mm.c               |  8 ++++++--
 xen/arch/x86/pv/shim.c          | 39 +++++++++++++++------------------------
 xen/arch/x86/smpboot.c          |  6 +++---
 6 files changed, 31 insertions(+), 33 deletions(-)

-- 
2.15.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 1/6] xen/pvshim: map vcpu_info earlier for APs

[Roger Pau Monne]

Or else init_percpu_time is going to dereference a NULL pointer when
trying to access vcpu_info.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
Should be backported to the 4.10.0-shim-comet branch.
---
 xen/arch/x86/smpboot.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index 63ca053b35..2cdd431b5f 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -349,6 +349,9 @@ void start_secondary(void *unused)
     else
         microcode_resume_cpu(cpu);
 
+    if ( xen_guest )
+        hypervisor_ap_setup();
+
     smp_callin();
 
     init_percpu_time();
@@ -376,9 +379,6 @@ void start_secondary(void *unused)
     cpumask_set_cpu(cpu, &cpu_online_map);
     unlock_vector_lock();
 
-    if ( xen_guest )
-        hypervisor_ap_setup();
-
     /* We can take interrupts now: we're officially "up". */
     local_irq_enable();
     mtrr_ap_init();
-- 
2.15.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/6] xen/pvshim: map vcpu_info earlier for APs

[Wei Liu]

On Wed, Jan 17, 2018 at 09:48:09AM +0000, Roger Pau Monne wrote:
> Or else init_percpu_time is going to dereference a NULL pointer when
> trying to access vcpu_info.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/6] xen/pvshim: map vcpu_info earlier for APs

[Jan Beulich]

>>> On 17.01.18 at 11:50, <wei.liu2@citrix.com> wrote:
> On Wed, Jan 17, 2018 at 09:48:09AM +0000, Roger Pau Monne wrote:
>> Or else init_percpu_time is going to dereference a NULL pointer when
>> trying to access vcpu_info.
>> 
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Reviewed-by: Wei Liu <wei.liu2@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 2/6] xen/pvh: place the trampoline at page 0x1

[Roger Pau Monne]

Since PVH guest jump straight into trampoline_setup trampoline_phys is
not initialized, thus the trampoline is relocated to address 0.

This works, but has the undesirable effect of having VA 0 mapped to
MFN 0, which means NULL pointed dereferences no longer trigger a page
fault.

In order to solve this, place the trampoline at page 0x1 and reserve
the memory used by it.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
---
Should be backported to the 4.10.0-shim-comet branch.
---
 xen/arch/x86/boot/head.S | 3 +++
 xen/arch/x86/mm.c        | 8 ++++++--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index 4fe5a776b1..7829e3f07c 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -411,6 +411,9 @@ __pvh_start:
         /* Skip bootloader setup and bios setup, go straight to trampoline */
         movb    $1, sym_esi(pvh_boot)
         movb    $1, sym_esi(skip_realmode)
+
+        /* Set trampoline_phys to use page 1. */
+        movw    $0x1000, sym_esi(trampoline_phys)
         jmp     trampoline_setup
 
 #endif /* CONFIG_PVH_GUEST */
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 1147a1afb1..586af2ba9a 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -292,9 +292,13 @@ void __init arch_init_memory(void)
     /*
      * First 1MB of RAM is historically marked as I/O.  If we booted PVH,
      * reclaim the space.  Irrespective, leave MFN 0 as special for the sake
-     * of 0 being a very common default value.
+     * of 0 being a very common default value. Also reserve page 0x1 which is
+     * used by the trampoline code.
      */
-    for ( i = 0; i < (pvh_boot ? 1 : 0x100); i++ )
+    for ( i = 0;
+          i < (pvh_boot ? (1 + PFN_UP(trampoline_end - trampoline_start))
+                        : 0x100);
+          i++ )
         share_xen_page_with_guest(mfn_to_page(_mfn(i)),
                                   dom_io, XENSHARE_writable);
 
-- 
2.15.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/6] xen/pvh: place the trampoline at page 0x1

[Wei Liu]

On Wed, Jan 17, 2018 at 09:48:10AM +0000, Roger Pau Monne wrote:
> Since PVH guest jump straight into trampoline_setup trampoline_phys is
> not initialized, thus the trampoline is relocated to address 0.
> 
> This works, but has the undesirable effect of having VA 0 mapped to
> MFN 0, which means NULL pointed dereferences no longer trigger a page
> fault.
> 
> In order to solve this, place the trampoline at page 0x1 and reserve
> the memory used by it.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> ---
> Should be backported to the 4.10.0-shim-comet branch.
> ---
>  xen/arch/x86/boot/head.S | 3 +++
>  xen/arch/x86/mm.c        | 8 ++++++--
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
> index 4fe5a776b1..7829e3f07c 100644
> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -411,6 +411,9 @@ __pvh_start:
>          /* Skip bootloader setup and bios setup, go straight to trampoline */
>          movb    $1, sym_esi(pvh_boot)
>          movb    $1, sym_esi(skip_realmode)
> +
> +        /* Set trampoline_phys to use page 1. */

Could you please add the rationale here -- to avoid having VA 0 mapped.

> +        movw    $0x1000, sym_esi(trampoline_phys)
>          jmp     trampoline_setup
>  
>  #endif /* CONFIG_PVH_GUEST */
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index 1147a1afb1..586af2ba9a 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -292,9 +292,13 @@ void __init arch_init_memory(void)
>      /*
>       * First 1MB of RAM is historically marked as I/O.  If we booted PVH,
>       * reclaim the space.  Irrespective, leave MFN 0 as special for the sake
> -     * of 0 being a very common default value.
> +     * of 0 being a very common default value. Also reserve page 0x1 which is
> +     * used by the trampoline code.
>       */
> -    for ( i = 0; i < (pvh_boot ? 1 : 0x100); i++ )
> +    for ( i = 0;
> +          i < (pvh_boot ? (1 + PFN_UP(trampoline_end - trampoline_start))
> +                        : 0x100);
> +          i++ )

There is now a hardcoded assumption that the trampoline is mapped at
page 0x1. Maybe there should be a dedicated loop to share the trampoline
with dom_io?

Wei.

>          share_xen_page_with_guest(mfn_to_page(_mfn(i)),
>                                    dom_io, XENSHARE_writable);
>  
> -- 
> 2.15.1
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/6] xen/pvh: place the trampoline at page 0x1

[Roger Pau Monné]

On Wed, Jan 17, 2018 at 10:55:22AM +0000, Wei Liu wrote:
> On Wed, Jan 17, 2018 at 09:48:10AM +0000, Roger Pau Monne wrote:
> > diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
> > index 4fe5a776b1..7829e3f07c 100644
> > --- a/xen/arch/x86/boot/head.S
> > +++ b/xen/arch/x86/boot/head.S
> > @@ -411,6 +411,9 @@ __pvh_start:
> >          /* Skip bootloader setup and bios setup, go straight to trampoline */
> >          movb    $1, sym_esi(pvh_boot)
> >          movb    $1, sym_esi(skip_realmode)
> > +
> > +        /* Set trampoline_phys to use page 1. */
> 
> Could you please add the rationale here -- to avoid having VA 0 mapped.

Sure.

> > +        movw    $0x1000, sym_esi(trampoline_phys)
> >          jmp     trampoline_setup
> >  
> >  #endif /* CONFIG_PVH_GUEST */
> > diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> > index 1147a1afb1..586af2ba9a 100644
> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -292,9 +292,13 @@ void __init arch_init_memory(void)
> >      /*
> >       * First 1MB of RAM is historically marked as I/O.  If we booted PVH,
> >       * reclaim the space.  Irrespective, leave MFN 0 as special for the sake
> > -     * of 0 being a very common default value.
> > +     * of 0 being a very common default value. Also reserve page 0x1 which is
> > +     * used by the trampoline code.
> >       */
> > -    for ( i = 0; i < (pvh_boot ? 1 : 0x100); i++ )
> > +    for ( i = 0;
> > +          i < (pvh_boot ? (1 + PFN_UP(trampoline_end - trampoline_start))
> > +                        : 0x100);
> > +          i++ )
> 
> There is now a hardcoded assumption that the trampoline is mapped at
> page 0x1. Maybe there should be a dedicated loop to share the trampoline
> with dom_io?

I could add a loop like:

for ( i = PFN_DOWN(trampoline_phys);
      i < (pvh_boot ? (PFN_UP(trampoline_phys + trampoline_end - trampoline_start))
                    : 0);
      i++ )

But it seems quite redundant.

The comment above the loop already explains the position of the
trampoline for PVH, do you think it would be fine to add something
like:

BUG_ON(pvh_boot && trampoline_phys != 0x1000);

Just before the loop?

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/6] xen/pvh: place the trampoline at page 0x1

[Wei Liu]

On Wed, Jan 17, 2018 at 11:35:15AM +0000, Roger Pau Monné wrote:
> 
> > > +        movw    $0x1000, sym_esi(trampoline_phys)
> > >          jmp     trampoline_setup
> > >  
> > >  #endif /* CONFIG_PVH_GUEST */
> > > diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> > > index 1147a1afb1..586af2ba9a 100644
> > > --- a/xen/arch/x86/mm.c
> > > +++ b/xen/arch/x86/mm.c
> > > @@ -292,9 +292,13 @@ void __init arch_init_memory(void)
> > >      /*
> > >       * First 1MB of RAM is historically marked as I/O.  If we booted PVH,
> > >       * reclaim the space.  Irrespective, leave MFN 0 as special for the sake
> > > -     * of 0 being a very common default value.
> > > +     * of 0 being a very common default value. Also reserve page 0x1 which is
> > > +     * used by the trampoline code.
> > >       */
> > > -    for ( i = 0; i < (pvh_boot ? 1 : 0x100); i++ )
> > > +    for ( i = 0;
> > > +          i < (pvh_boot ? (1 + PFN_UP(trampoline_end - trampoline_start))
> > > +                        : 0x100);
> > > +          i++ )
> > 
> > There is now a hardcoded assumption that the trampoline is mapped at
> > page 0x1. Maybe there should be a dedicated loop to share the trampoline
> > with dom_io?
> 
> I could add a loop like:
> 
> for ( i = PFN_DOWN(trampoline_phys);
>       i < (pvh_boot ? (PFN_UP(trampoline_phys + trampoline_end - trampoline_start))
>                     : 0);
>       i++ )
> 
> But it seems quite redundant.
> 
> The comment above the loop already explains the position of the
> trampoline for PVH, do you think it would be fine to add something

What I'm afraid is that once we move the trampoline somewhere else the
comment and code will be stale and we can't easily know about that.

> like:
> 
> BUG_ON(pvh_boot && trampoline_phys != 0x1000);
> 
> Just before the loop?

This will do, too.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 2/6] xen/pvh: place the trampoline at page 0x1

[Roger Pau Monne]

Since PVH guest jump straight into trampoline_setup trampoline_phys is
not initialized, thus the trampoline is relocated to address 0.

This works, but has the undesirable effect of having VA 0 mapped to
MFN 0, which means NULL pointed dereferences no longer trigger a page
fault.

In order to solve this, place the trampoline at page 0x1 and reserve
the memory used by it.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
---
Changes since v1:
 - Expand comment in head.S.
 - Add a BUG_ON to check trampoline_phys value in the PVH case.
---
Should be backported to the 4.10.0-shim-comet branch.
---
 xen/arch/x86/boot/head.S | 3 +++
 xen/arch/x86/mm.c        | 9 +++++++--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index 4fe5a776b1..0f652cea11 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -411,6 +411,9 @@ __pvh_start:
         /* Skip bootloader setup and bios setup, go straight to trampoline */
         movb    $1, sym_esi(pvh_boot)
         movb    $1, sym_esi(skip_realmode)
+
+        /* Set trampoline_phys to use mfn 1 to avoid having a mapping at VA 0 */
+        movw    $0x1000, sym_esi(trampoline_phys)
         jmp     trampoline_setup
 
 #endif /* CONFIG_PVH_GUEST */
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 1147a1afb1..356f939f64 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -292,9 +292,14 @@ void __init arch_init_memory(void)
     /*
      * First 1MB of RAM is historically marked as I/O.  If we booted PVH,
      * reclaim the space.  Irrespective, leave MFN 0 as special for the sake
-     * of 0 being a very common default value.
+     * of 0 being a very common default value. Also reserve page 0x1 which is
+     * used by the trampoline code on PVH.
      */
-    for ( i = 0; i < (pvh_boot ? 1 : 0x100); i++ )
+    BUG_ON(pvh_boot && trampoline_phys != 0x1000);
+    for ( i = 0;
+          i < (pvh_boot ? (1 + PFN_UP(trampoline_end - trampoline_start))
+                        : 0x100);
+          i++ )
         share_xen_page_with_guest(mfn_to_page(_mfn(i)),
                                   dom_io, XENSHARE_writable);
 
-- 
2.15.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 2/6] xen/pvh: place the trampoline at page 0x1

[Wei Liu]

On Wed, Jan 17, 2018 at 12:00:41PM +0000, Roger Pau Monne wrote:
> Since PVH guest jump straight into trampoline_setup trampoline_phys is
> not initialized, thus the trampoline is relocated to address 0.
> 
> This works, but has the undesirable effect of having VA 0 mapped to
> MFN 0, which means NULL pointed dereferences no longer trigger a page
> fault.
> 
> In order to solve this, place the trampoline at page 0x1 and reserve
> the memory used by it.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 2/6] xen/pvh: place the trampoline at page 0x1

[Jan Beulich]

>>> On 17.01.18 at 13:00, <roger.pau@citrix.com> wrote:
> Since PVH guest jump straight into trampoline_setup trampoline_phys is
> not initialized, thus the trampoline is relocated to address 0.
> 
> This works, but has the undesirable effect of having VA 0 mapped to
> MFN 0, which means NULL pointed dereferences no longer trigger a page
> fault.
> 
> In order to solve this, place the trampoline at page 0x1 and reserve
> the memory used by it.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> ---
> Changes since v1:
>  - Expand comment in head.S.
>  - Add a BUG_ON to check trampoline_phys value in the PVH case.
> ---
> Should be backported to the 4.10.0-shim-comet branch.
> ---
>  xen/arch/x86/boot/head.S | 3 +++
>  xen/arch/x86/mm.c        | 9 +++++++--
>  2 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
> index 4fe5a776b1..0f652cea11 100644
> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -411,6 +411,9 @@ __pvh_start:
>          /* Skip bootloader setup and bios setup, go straight to trampoline 
> */
>          movb    $1, sym_esi(pvh_boot)
>          movb    $1, sym_esi(skip_realmode)
> +
> +        /* Set trampoline_phys to use mfn 1 to avoid having a mapping at VA 
> 0 */
> +        movw    $0x1000, sym_esi(trampoline_phys)
>          jmp     trampoline_setup
>  
>  #endif /* CONFIG_PVH_GUEST */
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index 1147a1afb1..356f939f64 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -292,9 +292,14 @@ void __init arch_init_memory(void)
>      /*
>       * First 1MB of RAM is historically marked as I/O.  If we booted PVH,
>       * reclaim the space.  Irrespective, leave MFN 0 as special for the sake
> -     * of 0 being a very common default value.
> +     * of 0 being a very common default value. Also reserve page 0x1 which is

This isn't in line with ...

> +     * used by the trampoline code on PVH.
>       */
> -    for ( i = 0; i < (pvh_boot ? 1 : 0x100); i++ )
> +    BUG_ON(pvh_boot && trampoline_phys != 0x1000);
> +    for ( i = 0;
> +          i < (pvh_boot ? (1 + PFN_UP(trampoline_end - trampoline_start))

... this. How about having the comment say "Also reserve pages
which are used ..."? Could certainly be addressed while committing;
with this or some similar wording
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 3/6] xen/pvshim: identity pin shim vCPUs to pCPUs

[Roger Pau Monne]

Since VCPUOP_{up/down} already identity pins vCPU hotplug to pCPU
hotplug also pin the vCPUs to the pCPUs in the scheduler. This prevent
vCPU migration and should improve performance.

While there also use __cpumask_set_cpu instead of cpumask_set_cpu,
there's no need to use the locked variant.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
---
Should be backported to the 4.10.0-shim-comet branch.
---
 xen/arch/x86/dom0_build.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
index 17cb1272c1..555660b853 100644
--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -140,9 +140,8 @@ struct vcpu *__init dom0_setup_vcpu(struct domain *d,
     {
         if ( pv_shim )
         {
-
-            cpumask_setall(v->cpu_hard_affinity);
-            cpumask_setall(v->cpu_soft_affinity);
+            __cpumask_set_cpu(vcpu_id, v->cpu_hard_affinity);
+            __cpumask_set_cpu(vcpu_id, v->cpu_soft_affinity);
         }
         else
         {
-- 
2.15.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/6] xen/pvshim: identity pin shim vCPUs to pCPUs

[Wei Liu]

On Wed, Jan 17, 2018 at 09:48:11AM +0000, Roger Pau Monne wrote:
> Since VCPUOP_{up/down} already identity pins vCPU hotplug to pCPU
> hotplug also pin the vCPUs to the pCPUs in the scheduler. This prevent

The description is a bit ambiguous. I read it as "the shim hotplug code
pins vcpu to pcpu while doing VCPUOP_{up/down}" but in fact it is "the
shim hotplug code assumes identity mapping between vcpu and pcpu".

With this clarified.

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

> vCPU migration and should improve performance.
> 
> While there also use __cpumask_set_cpu instead of cpumask_set_cpu,
> there's no need to use the locked variant.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> ---
> Should be backported to the 4.10.0-shim-comet branch.
> ---
>  xen/arch/x86/dom0_build.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
> index 17cb1272c1..555660b853 100644
> --- a/xen/arch/x86/dom0_build.c
> +++ b/xen/arch/x86/dom0_build.c
> @@ -140,9 +140,8 @@ struct vcpu *__init dom0_setup_vcpu(struct domain *d,
>      {
>          if ( pv_shim )
>          {
> -
> -            cpumask_setall(v->cpu_hard_affinity);
> -            cpumask_setall(v->cpu_soft_affinity);
> +            __cpumask_set_cpu(vcpu_id, v->cpu_hard_affinity);
> +            __cpumask_set_cpu(vcpu_id, v->cpu_soft_affinity);
>          }
>          else
>          {
> -- 
> 2.15.1
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 3/6] xen/pvshim: identity pin shim vCPUs to pCPUs

[Roger Pau Monne]

Since VCPUOP_{up/down} already identity maps vCPU hotplug to pCPU
hotplug also identity pin the vCPUs to the pCPUs in the scheduler.
This prevents vCPU migration and should improve performance.

While there also use __cpumask_set_cpu instead of cpumask_set_cpu,
there's no need to use the locked variant.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
---
Changes since v1:
 - Clarify commit message.
---
Should be backported to the 4.10.0-shim-comet branch.
---
 xen/arch/x86/dom0_build.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
index 17cb1272c1..555660b853 100644
--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -140,9 +140,8 @@ struct vcpu *__init dom0_setup_vcpu(struct domain *d,
     {
         if ( pv_shim )
         {
-
-            cpumask_setall(v->cpu_hard_affinity);
-            cpumask_setall(v->cpu_soft_affinity);
+            __cpumask_set_cpu(vcpu_id, v->cpu_hard_affinity);
+            __cpumask_set_cpu(vcpu_id, v->cpu_soft_affinity);
         }
         else
         {
-- 
2.15.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 3/6] xen/pvshim: identity pin shim vCPUs to pCPUs

[Jan Beulich]

>>> On 17.01.18 at 17:29, <roger.pau@citrix.com> wrote:
> Since VCPUOP_{up/down} already identity maps vCPU hotplug to pCPU
> hotplug also identity pin the vCPUs to the pCPUs in the scheduler.
> This prevents vCPU migration and should improve performance.
> 
> While there also use __cpumask_set_cpu instead of cpumask_set_cpu,
> there's no need to use the locked variant.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Reviewed-by: Wei Liu <wei.liu2@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 3/6] xen/pvshim: identity pin shim vCPUs to pCPUs

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH v2 3/6] xen/pvshim: identity pin shim vCPUs to pCPUs"):
> On 17.01.18 at 17:29, <roger.pau@citrix.com> wrote:
> > Since VCPUOP_{up/down} already identity maps vCPU hotplug to pCPU
> > hotplug also identity pin the vCPUs to the pCPUs in the scheduler.
> > This prevents vCPU migration and should improve performance.
> > 
> > While there also use __cpumask_set_cpu instead of cpumask_set_cpu,
> > there's no need to use the locked variant.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > Reviewed-by: Wei Liu <wei.liu2@citrix.com>
> 
> Acked-by: Jan Beulich <jbeulich@suse.com>

Should this be in the security response comet branch ?

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 3/6] xen/pvshim: identity pin shim vCPUs to pCPUs

[Jan Beulich]

>>> On 18.01.18 at 12:11, <ian.jackson@eu.citrix.com> wrote:
> Jan Beulich writes ("Re: [PATCH v2 3/6] xen/pvshim: identity pin shim vCPUs 
> to pCPUs"):
>> On 17.01.18 at 17:29, <roger.pau@citrix.com> wrote:
>> > Since VCPUOP_{up/down} already identity maps vCPU hotplug to pCPU
>> > hotplug also identity pin the vCPUs to the pCPUs in the scheduler.
>> > This prevents vCPU migration and should improve performance.
>> > 
>> > While there also use __cpumask_set_cpu instead of cpumask_set_cpu,
>> > there's no need to use the locked variant.
>> > 
>> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> > Reviewed-by: Wei Liu <wei.liu2@citrix.com>
>> 
>> Acked-by: Jan Beulich <jbeulich@suse.com>
> 
> Should this be in the security response comet branch ?

I believe so, but better ask Roger and Wei.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 3/6] xen/pvshim: identity pin shim vCPUs to pCPUs

[Roger Pau Monné]

On Thu, Jan 18, 2018 at 04:22:11AM -0700, Jan Beulich wrote:
> >>> On 18.01.18 at 12:11, <ian.jackson@eu.citrix.com> wrote:
> > Jan Beulich writes ("Re: [PATCH v2 3/6] xen/pvshim: identity pin shim vCPUs 
> > to pCPUs"):
> >> On 17.01.18 at 17:29, <roger.pau@citrix.com> wrote:
> >> > Since VCPUOP_{up/down} already identity maps vCPU hotplug to pCPU
> >> > hotplug also identity pin the vCPUs to the pCPUs in the scheduler.
> >> > This prevents vCPU migration and should improve performance.
> >> > 
> >> > While there also use __cpumask_set_cpu instead of cpumask_set_cpu,
> >> > there's no need to use the locked variant.
> >> > 
> >> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >> > Reviewed-by: Wei Liu <wei.liu2@citrix.com>
> >> 
> >> Acked-by: Jan Beulich <jbeulich@suse.com>
> > 
> > Should this be in the security response comet branch ?
> 
> I believe so, but better ask Roger and Wei.

It should be in the 4.10 comment branch only (not the 4.8), since it's
a shim patch.

In any case, it's not a bug fix, just a performance improvement but
still nice to have IMHO.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/6] xen/pvshim: identity pin shim vCPUs to pCPUs

[George Dunlap]

[-- Attachment #1: Type: text/plain, Size: 711 bytes --]

On Wed, Jan 17, 2018 at 9:48 AM, Roger Pau Monne <roger.pau@citrix.com> wrote:
> Since VCPUOP_{up/down} already identity pins vCPU hotplug to pCPU
> hotplug also pin the vCPUs to the pCPUs in the scheduler. This prevent
> vCPU migration and should improve performance.
>
> While there also use __cpumask_set_cpu instead of cpumask_set_cpu,
> there's no need to use the locked variant.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Sorry, I just realized this -- we already have a way to pin a VM 1:1
-- d->is_pinned should do what you want here without having to
special-case the pvshim.

It seems like something like the attached might be better (compile-tested only).

 -George

[-- Attachment #2: 0001-x86-shim-Use-d-is_pinned-to-pin-the-shim.patch --]
[-- Type: text/x-patch, Size: 1992 bytes --]

From e6094b9d61b99a3fdbd648d9cefb719436972f88 Mon Sep 17 00:00:00 2001
From: George Dunlap <george.dunlap@citrix.com>
Date: Wed, 24 Jan 2018 17:58:39 +0000
Subject: [PATCH] x86/shim: Use d->is_pinned to pin the shim

Rather than special-casing it inside dom0_setup_vcpu()

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
 xen/arch/x86/dom0_build.c | 14 +++-----------
 xen/common/domain.c       |  6 +++++-
 2 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
index 555660b853..483cb6cee9 100644
--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -138,17 +138,9 @@ struct vcpu *__init dom0_setup_vcpu(struct domain *d,
 
     if ( v )
     {
-        if ( pv_shim )
-        {
-            __cpumask_set_cpu(vcpu_id, v->cpu_hard_affinity);
-            __cpumask_set_cpu(vcpu_id, v->cpu_soft_affinity);
-        }
-        else
-        {
-            if ( !d->is_pinned && !dom0_affinity_relaxed )
-                cpumask_copy(v->cpu_hard_affinity, &dom0_cpus);
-            cpumask_copy(v->cpu_soft_affinity, &dom0_cpus);
-        }
+        if ( !d->is_pinned && !dom0_affinity_relaxed )
+            cpumask_copy(v->cpu_hard_affinity, &dom0_cpus);
+        cpumask_copy(v->cpu_soft_affinity, &dom0_cpus);
     }
 
     return v;
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 558318e852..4ce7980550 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -318,7 +318,11 @@ struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
     {
         if ( hardware_domid < 0 || hardware_domid >= DOMID_FIRST_RESERVED )
             panic("The value of hardware_dom must be a valid domain ID");
-        d->is_pinned = opt_dom0_vcpus_pin;
+        d->is_pinned =
+#ifdef CONFIG_X86
+            pv_shim ? true :
+#endif
+            opt_dom0_vcpus_pin;
         d->disable_migrate = 1;
         old_hwdom = hardware_domain;
         hardware_domain = d;
-- 
2.15.1


[-- Attachment #3: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/6] xen/pvshim: identity pin shim vCPUs to pCPUs

[Roger Pau Monné]

On Wed, Jan 24, 2018 at 06:03:28PM +0000, George Dunlap wrote:
> On Wed, Jan 17, 2018 at 9:48 AM, Roger Pau Monne <roger.pau@citrix.com> wrote:
> > Since VCPUOP_{up/down} already identity pins vCPU hotplug to pCPU
> > hotplug also pin the vCPUs to the pCPUs in the scheduler. This prevent
> > vCPU migration and should improve performance.
> >
> > While there also use __cpumask_set_cpu instead of cpumask_set_cpu,
> > there's no need to use the locked variant.
> >
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Sorry, I just realized this -- we already have a way to pin a VM 1:1
> -- d->is_pinned should do what you want here without having to
> special-case the pvshim.
> 
> It seems like something like the attached might be better (compile-tested only).

I haven't tested the proposed patch, but there's a peculiarity of the
shim that I think makes this approach invalid.

When Xen is booted in shim mode the APs are never started, which means
that dom0_cpus only contains the BSP, and that alloc_vcpu is always
going to be called with cpu == 0. This in turn means that
sched_init_vcpu is also always called with cpu_id == 0, and if
is_pinned is set it would force all vCPUs to be pinned to the BSP
AFAICT.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/6] xen/pvshim: identity pin shim vCPUs to pCPUs

[George Dunlap]

On Thu, Jan 25, 2018 at 9:14 AM, Roger Pau Monné <roger.pau@citrix.com> wrote:
> On Wed, Jan 24, 2018 at 06:03:28PM +0000, George Dunlap wrote:
>> On Wed, Jan 17, 2018 at 9:48 AM, Roger Pau Monne <roger.pau@citrix.com> wrote:
>> > Since VCPUOP_{up/down} already identity pins vCPU hotplug to pCPU
>> > hotplug also pin the vCPUs to the pCPUs in the scheduler. This prevent
>> > vCPU migration and should improve performance.
>> >
>> > While there also use __cpumask_set_cpu instead of cpumask_set_cpu,
>> > there's no need to use the locked variant.
>> >
>> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>
>> Sorry, I just realized this -- we already have a way to pin a VM 1:1
>> -- d->is_pinned should do what you want here without having to
>> special-case the pvshim.
>>
>> It seems like something like the attached might be better (compile-tested only).
>
> I haven't tested the proposed patch, but there's a peculiarity of the
> shim that I think makes this approach invalid.
>
> When Xen is booted in shim mode the APs are never started, which means
> that dom0_cpus only contains the BSP, and that alloc_vcpu is always
> going to be called with cpu == 0. This in turn means that
> sched_init_vcpu is also always called with cpu_id == 0, and if
> is_pinned is set it would force all vCPUs to be pinned to the BSP
> AFAICT.

Right, I see -- dom0 may not actually be running on pcpus 0-N (and in
theory there may be more vcpus than pcpus), so the code goes through
and pins them one-by-one based on what cpus it actually has, rather
than using the vcpu_id directly.

So it looks like you want to bring up cpus in Xen only when the guest
brings them up.  So in shim mode, you only bring up the BSP.  You want
all possible "dom0" (i.e. L2) vcpus *created* at boot time, and you
also want them pinned to their respective "pcpus" (L1 vcpus).

But you can't call alloc_vcpu() with the appropriate "pcpuid", because
then sched_init_vcpu() will set v->processor equal to a pcpu which
isn't up yet.  So you set dom0_cpus to contain only cpu0, so that
alloc_vcpu() will always be called with cpu 0; and then special-case
the affinity so that when it *does* come up, the affinity is already
set.

Is that about right?

What about setting the hard affinity in pv_shim_cpu_up() instead?
(You don't need to set the soft affinity, as the hard affinity will
override it.)

-George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/6] xen/pvshim: identity pin shim vCPUs to pCPUs

[Roger Pau Monné]

On Thu, Jan 25, 2018 at 02:03:35PM +0000, George Dunlap wrote:
> On Thu, Jan 25, 2018 at 9:14 AM, Roger Pau Monné <roger.pau@citrix.com> wrote:
> > On Wed, Jan 24, 2018 at 06:03:28PM +0000, George Dunlap wrote:
> >> On Wed, Jan 17, 2018 at 9:48 AM, Roger Pau Monne <roger.pau@citrix.com> wrote:
> >> > Since VCPUOP_{up/down} already identity pins vCPU hotplug to pCPU
> >> > hotplug also pin the vCPUs to the pCPUs in the scheduler. This prevent
> >> > vCPU migration and should improve performance.
> >> >
> >> > While there also use __cpumask_set_cpu instead of cpumask_set_cpu,
> >> > there's no need to use the locked variant.
> >> >
> >> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >>
> >> Sorry, I just realized this -- we already have a way to pin a VM 1:1
> >> -- d->is_pinned should do what you want here without having to
> >> special-case the pvshim.
> >>
> >> It seems like something like the attached might be better (compile-tested only).
> >
> > I haven't tested the proposed patch, but there's a peculiarity of the
> > shim that I think makes this approach invalid.
> >
> > When Xen is booted in shim mode the APs are never started, which means
> > that dom0_cpus only contains the BSP, and that alloc_vcpu is always
> > going to be called with cpu == 0. This in turn means that
> > sched_init_vcpu is also always called with cpu_id == 0, and if
> > is_pinned is set it would force all vCPUs to be pinned to the BSP
> > AFAICT.
> 
> Right, I see -- dom0 may not actually be running on pcpus 0-N (and in
> theory there may be more vcpus than pcpus), so the code goes through
> and pins them one-by-one based on what cpus it actually has, rather
> than using the vcpu_id directly.
> 
> So it looks like you want to bring up cpus in Xen only when the guest
> brings them up.  So in shim mode, you only bring up the BSP.  You want
> all possible "dom0" (i.e. L2) vcpus *created* at boot time, and you
> also want them pinned to their respective "pcpus" (L1 vcpus).
> 
> But you can't call alloc_vcpu() with the appropriate "pcpuid", because
> then sched_init_vcpu() will set v->processor equal to a pcpu which
> isn't up yet.  So you set dom0_cpus to contain only cpu0, so that
> alloc_vcpu() will always be called with cpu 0; and then special-case
> the affinity so that when it *does* come up, the affinity is already
> set.
> 
> Is that about right?

Yes, I think so.

> What about setting the hard affinity in pv_shim_cpu_up() instead?

That would mean setting it up each time the CPU is brought up. Seems
easier to set only once during domain build and forget about it.

> (You don't need to set the soft affinity, as the hard affinity will
> override it.)

That seems fine to me. I more or less guessed that, but didn't look
that carefully.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 4/6] xen/pvshim: simplify replace_va_mapping code

[Roger Pau Monne]

No functional change.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/pv/shim.c | 21 ++++++---------------
 1 file changed, 6 insertions(+), 15 deletions(-)

diff --git a/xen/arch/x86/pv/shim.c b/xen/arch/x86/pv/shim.c
index 702249719e..4f94047695 100644
--- a/xen/arch/x86/pv/shim.c
+++ b/xen/arch/x86/pv/shim.c
@@ -117,21 +117,12 @@ uint64_t pv_shim_mem(uint64_t avail)
 static void __init replace_va_mapping(struct domain *d, l4_pgentry_t *l4start,
                                       unsigned long va, unsigned long mfn)
 {
-    struct page_info *page;
-    l4_pgentry_t *pl4e;
-    l3_pgentry_t *pl3e;
-    l2_pgentry_t *pl2e;
-    l1_pgentry_t *pl1e;
-
-    pl4e = l4start + l4_table_offset(va);
-    pl3e = l4e_to_l3e(*pl4e);
-    pl3e += l3_table_offset(va);
-    pl2e = l3e_to_l2e(*pl3e);
-    pl2e += l2_table_offset(va);
-    pl1e = l2e_to_l1e(*pl2e);
-    pl1e += l1_table_offset(va);
-
-    page = mfn_to_page(l1e_get_pfn(*pl1e));
+    l4_pgentry_t *pl4e = l4start + l4_table_offset(va);
+    l3_pgentry_t *pl3e = l4e_to_l3e(*pl4e) + l3_table_offset(va);
+    l2_pgentry_t *pl2e = l3e_to_l2e(*pl3e) + l2_table_offset(va);
+    l1_pgentry_t *pl1e = l2e_to_l1e(*pl2e) + l1_table_offset(va);
+    struct page_info *page =  mfn_to_page(l1e_get_pfn(*pl1e));;
+
     put_page_and_type(page);
 
     *pl1e = l1e_from_pfn(mfn, (!is_pv_32bit_domain(d) ? L1_PROT
-- 
2.15.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 4/6] xen/pvshim: simplify replace_va_mapping code

[Wei Liu]

On Wed, Jan 17, 2018 at 09:48:12AM +0000, Roger Pau Monne wrote:
> No functional change.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 4/6] xen/pvshim: simplify replace_va_mapping code

[Jan Beulich]

>>> On 17.01.18 at 10:48, <roger.pau@citrix.com> wrote:
> No functional change.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

I'm not sure I'd call it a simplification, though - you only re-order
code.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 4/6] xen/pvshim: simplify replace_va_mapping code

[Andrew Cooper]

On 17/01/2018 09:48, Roger Pau Monne wrote:
> No functional change.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
>  xen/arch/x86/pv/shim.c | 21 ++++++---------------
>  1 file changed, 6 insertions(+), 15 deletions(-)
>
> diff --git a/xen/arch/x86/pv/shim.c b/xen/arch/x86/pv/shim.c
> index 702249719e..4f94047695 100644
> --- a/xen/arch/x86/pv/shim.c
> +++ b/xen/arch/x86/pv/shim.c
> @@ -117,21 +117,12 @@ uint64_t pv_shim_mem(uint64_t avail)
>  static void __init replace_va_mapping(struct domain *d, l4_pgentry_t *l4start,
>                                        unsigned long va, unsigned long mfn)
>  {
> -    struct page_info *page;
> -    l4_pgentry_t *pl4e;
> -    l3_pgentry_t *pl3e;
> -    l2_pgentry_t *pl2e;
> -    l1_pgentry_t *pl1e;
> -
> -    pl4e = l4start + l4_table_offset(va);
> -    pl3e = l4e_to_l3e(*pl4e);
> -    pl3e += l3_table_offset(va);
> -    pl2e = l3e_to_l2e(*pl3e);
> -    pl2e += l2_table_offset(va);
> -    pl1e = l2e_to_l1e(*pl2e);
> -    pl1e += l1_table_offset(va);
> -
> -    page = mfn_to_page(l1e_get_pfn(*pl1e));
> +    l4_pgentry_t *pl4e = l4start + l4_table_offset(va);
> +    l3_pgentry_t *pl3e = l4e_to_l3e(*pl4e) + l3_table_offset(va);
> +    l2_pgentry_t *pl2e = l3e_to_l2e(*pl3e) + l2_table_offset(va);
> +    l1_pgentry_t *pl1e = l2e_to_l1e(*pl2e) + l1_table_offset(va);
> +    struct page_info *page =  mfn_to_page(l1e_get_pfn(*pl1e));;

Double ;;  (Can be fixed on commit)

How about switching to using the typesafe mfn_to_page() before we gain
more code that we expect to change in the future?  If its only here then
it is probably worth folding into this commit.  If not, it should be a
separate patch.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 5/6] xen/pvshim: fix coding style issues

[Roger Pau Monne]

Fix a couple of coding style issues.

No code or functional change.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/pv/shim.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/pv/shim.c b/xen/arch/x86/pv/shim.c
index 4f94047695..2c6bd62ba5 100644
--- a/xen/arch/x86/pv/shim.c
+++ b/xen/arch/x86/pv/shim.c
@@ -264,14 +264,14 @@ int pv_shim_shutdown(uint8_t reason)
                                            &old_console_pfn));
 
     /* Pause the other vcpus before starting the migration. */
-    for_each_vcpu(d, v)
+    for_each_vcpu ( d, v )
         if ( v != current )
             vcpu_pause_by_systemcontroller(v);
 
     rc = xen_hypercall_shutdown(SHUTDOWN_suspend);
     if ( rc )
     {
-        for_each_vcpu(d, v)
+        for_each_vcpu ( d, v )
             if ( v != current )
                 vcpu_unpause_by_systemcontroller(v);
 
@@ -347,7 +347,7 @@ int pv_shim_shutdown(uint8_t reason)
      */
     write_start_info(d);
 
-    for_each_vcpu(d, v)
+    for_each_vcpu ( d, v )
     {
         /* Unmap guest vcpu_info pages. */
         unmap_vcpu_info(v);
@@ -428,7 +428,7 @@ static long pv_shim_event_channel_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
          */
         rc = xen_hypercall_event_channel_op(EVTCHNOP_alloc_unbound, &alloc);
         if ( rc )
-           break;
+            break;
 
         /* Force L1 to use the event channel port allocated on L0. */
         rc = evtchn_bind_virq(&virq, alloc.port);
@@ -477,7 +477,7 @@ static long pv_shim_event_channel_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         {
             rc = xen_hypercall_event_channel_op(EVTCHNOP_bind_vcpu, &vcpu);
             if ( !rc )
-                 evtchn_assign_vcpu(d, vcpu.port, vcpu.vcpu);
+                evtchn_assign_vcpu(d, vcpu.port, vcpu.vcpu);
         }
 
         break;
@@ -596,9 +596,9 @@ void pv_shim_inject_evtchn(unsigned int port)
 {
     if ( port_is_valid(guest, port) )
     {
-         struct evtchn *chn = evtchn_from_port(guest, port);
+        struct evtchn *chn = evtchn_from_port(guest, port);
 
-         evtchn_port_set_pending(guest, chn->notify_vcpu_id, chn);
+        evtchn_port_set_pending(guest, chn->notify_vcpu_id, chn);
     }
 }
 
@@ -633,7 +633,7 @@ static long pv_shim_grant_table_op(unsigned int cmd,
         }
         if ( compat )
 #define XLAT_gnttab_setup_table_HNDL_frame_list(d, s)
-                XLAT_gnttab_setup_table(&nat, &cmp);
+            XLAT_gnttab_setup_table(&nat, &cmp);
 #undef XLAT_gnttab_setup_table_HNDL_frame_list
 
         nat.status = GNTST_okay;
@@ -728,7 +728,7 @@ static long pv_shim_grant_table_op(unsigned int cmd,
 
         if ( compat )
 #define XLAT_gnttab_setup_table_HNDL_frame_list(d, s)
-                XLAT_gnttab_setup_table(&cmp, &nat);
+            XLAT_gnttab_setup_table(&cmp, &nat);
 #undef XLAT_gnttab_setup_table_HNDL_frame_list
 
         if ( unlikely(compat ? __copy_to_guest(uop, &cmp, 1)
-- 
2.15.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 5/6] xen/pvshim: fix coding style issues

[Wei Liu]

On Wed, Jan 17, 2018 at 09:48:13AM +0000, Roger Pau Monne wrote:
> Fix a couple of coding style issues.
> 
> No code or functional change.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 5/6] xen/pvshim: fix coding style issues

[Jan Beulich]

>>> On 17.01.18 at 10:48, <roger.pau@citrix.com> wrote:
> --- a/xen/arch/x86/pv/shim.c
> +++ b/xen/arch/x86/pv/shim.c
> @@ -264,14 +264,14 @@ int pv_shim_shutdown(uint8_t reason)
>                                             &old_console_pfn));
>  
>      /* Pause the other vcpus before starting the migration. */
> -    for_each_vcpu(d, v)
> +    for_each_vcpu ( d, v )

These aren't style violations - either style is fine, as it is ambiguous
whether for_each_vcpu is to be treated like a keyword.

All the other changes appear to be indentation ones, which is a
little hard to see in a mail viewer not using mono-spaced fonts.
Anyway
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 6/6] firmware/shim: fix build process to use POSIX find options

[Roger Pau Monne]

The -printf find option is not POSIX compatible, so replace it with
another rune.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/firmware/xen-dir/Makefile | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/firmware/xen-dir/Makefile b/tools/firmware/xen-dir/Makefile
index adf6c31e8d..de754c752e 100644
--- a/tools/firmware/xen-dir/Makefile
+++ b/tools/firmware/xen-dir/Makefile
@@ -21,7 +21,8 @@ linkfarm.stamp: $(DEP_DIRS) $(DEP_FILES) FORCE
 	$(foreach d, $(LINK_DIRS), \
 		 (mkdir -p $(D)/$(d); \
 		  cd $(D)/$(d); \
-		  find $(XEN_ROOT)/$(d)/ -type d -printf "./%P\n" |  xargs mkdir -p);)
+		  find $(XEN_ROOT)/$(d)/ -type d -exec sh -c \
+		      "echo {} | sed 's,^$(XEN_ROOT)/$(d)/,,g' | xargs mkdir -p" \;);)
 	$(foreach d, $(LINK_DIRS), \
 		(cd $(XEN_ROOT); \
 		 find $(d) ! -type l -type f \
-- 
2.15.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 6/6] firmware/shim: fix build process to use POSIX find options

[Wei Liu]

On Wed, Jan 17, 2018 at 09:48:14AM +0000, Roger Pau Monne wrote:
> The -printf find option is not POSIX compatible, so replace it with
> another rune.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 6/6] firmware/shim: fix build process to use POSIX find options

[Ian Jackson]

Roger Pau Monne writes ("[PATCH 6/6] firmware/shim: fix build process to use POSIX find options"):
> The -printf find option is not POSIX compatible, so replace it with
> another rune.
...
>  		  cd $(D)/$(d); \
> -		  find $(XEN_ROOT)/$(d)/ -type d -printf "./%P\n" |  xargs mkdir -p);)
> +		  find $(XEN_ROOT)/$(d)/ -type d -exec sh -c \
> +		      "echo {} | sed 's,^$(XEN_ROOT)/$(d)/,,g' | xargs mkdir -p" \;);)

This is now a pretty nasty shell construct.

If you're going to use sed, you could just
   find ... -print | sed ... | xargs mkdir -p

Substituting {} into the middle of the shell rune is bad practice in
general because it might contain metacharacters or spaces.  In our
build system this doesn't matter because that breaks anyway, but it's
very poor style (and also it buries the actual data flow path into the
middle of a complicated manglement).

If you wanted to use -exec and sh, you could do something like this
   -exec sh -c 'exec mkdir "${1#$(XEN_ROOT}/$(d)/}"' x {}
maybe.

And finally, I don't really understand why it is necessary to strip
$(XEN_ROOT)/$(d)/ out at all.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 6/6] firmware/shim: fix build process to use POSIX find options

[Wei Liu]

On Wed, Jan 17, 2018 at 04:24:27PM +0000, Ian Jackson wrote:
> Roger Pau Monne writes ("[PATCH 6/6] firmware/shim: fix build process to use POSIX find options"):
> > The -printf find option is not POSIX compatible, so replace it with
> > another rune.
> ...
> >  		  cd $(D)/$(d); \
> > -		  find $(XEN_ROOT)/$(d)/ -type d -printf "./%P\n" |  xargs mkdir -p);)
> > +		  find $(XEN_ROOT)/$(d)/ -type d -exec sh -c \
> > +		      "echo {} | sed 's,^$(XEN_ROOT)/$(d)/,,g' | xargs mkdir -p" \;);)
> 
> This is now a pretty nasty shell construct.
> 
> If you're going to use sed, you could just
>    find ... -print | sed ... | xargs mkdir -p
> 
> Substituting {} into the middle of the shell rune is bad practice in
> general because it might contain metacharacters or spaces.  In our
> build system this doesn't matter because that breaks anyway, but it's
> very poor style (and also it buries the actual data flow path into the
> middle of a complicated manglement).
> 
> If you wanted to use -exec and sh, you could do something like this
>    -exec sh -c 'exec mkdir "${1#$(XEN_ROOT}/$(d)/}"' x {}
> maybe.
> 
> And finally, I don't really understand why it is necessary to strip
> $(XEN_ROOT)/$(d)/ out at all.
> 

We want the path without $(XEN_ROOT)/$(d) so we can construct the same
tree under $(d). It is better to just look at the tree structure after
running the linkfarm rune.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 6/6] firmware/shim: fix build process to use POSIX find options

[Ian Jackson]

Wei Liu writes ("Re: [PATCH 6/6] firmware/shim: fix build process to use POSIX find options"):
> On Wed, Jan 17, 2018 at 04:24:27PM +0000, Ian Jackson wrote:
> > And finally, I don't really understand why it is necessary to strip
> > $(XEN_ROOT)/$(d)/ out at all.
> 
> We want the path without $(XEN_ROOT)/$(d) so we can construct the same
> tree under $(d). It is better to just look at the tree structure after
> running the linkfarm rune.

Fair enough.  But I think my other points still stand.

FYI I am going to ship the patch as is to "comet" users via an XSA-254
update.  I'm just quibbling about this for #staging.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 6/6] firmware/shim: fix build process to use POSIX find options

[Wei Liu]

On Wed, Jan 17, 2018 at 04:55:01PM +0000, Ian Jackson wrote:
> Wei Liu writes ("Re: [PATCH 6/6] firmware/shim: fix build process to use POSIX find options"):
> > On Wed, Jan 17, 2018 at 04:24:27PM +0000, Ian Jackson wrote:
> > > And finally, I don't really understand why it is necessary to strip
> > > $(XEN_ROOT)/$(d)/ out at all.
> > 
> > We want the path without $(XEN_ROOT)/$(d) so we can construct the same
> > tree under $(d). It is better to just look at the tree structure after
> > running the linkfarm rune.
> 
> Fair enough.  But I think my other points still stand.
> 
> FYI I am going to ship the patch as is to "comet" users via an XSA-254
> update.  I'm just quibbling about this for #staging.

Yeah, we can definitely change the rune to something better once we have
all the critical issues solved. The rune was the best I could come up
with under a short notice. And it wasn't critical so I moved on to
something else.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 6/6] firmware/shim: fix build process to use POSIX find options

[Ian Jackson]

Wei Liu writes ("Re: [PATCH 6/6] firmware/shim: fix build process to use POSIX find options"):
> On Wed, Jan 17, 2018 at 04:55:01PM +0000, Ian Jackson wrote:
> > FYI I am going to ship the patch as is to "comet" users via an XSA-254
> > update.  I'm just quibbling about this for #staging.
> 
> Yeah, we can definitely change the rune to something better once we have
> all the critical issues solved. The rune was the best I could come up
> with under a short notice. And it wasn't critical so I moved on to
> something else.

Right, thanks.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 6/6] firmware/shim: fix build process to use POSIX find options

[Roger Pau Monné]

On Wed, Jan 17, 2018 at 04:24:27PM +0000, Ian Jackson wrote:
> Roger Pau Monne writes ("[PATCH 6/6] firmware/shim: fix build process to use POSIX find options"):
> > The -printf find option is not POSIX compatible, so replace it with
> > another rune.
> ...
> >  		  cd $(D)/$(d); \
> > -		  find $(XEN_ROOT)/$(d)/ -type d -printf "./%P\n" |  xargs mkdir -p);)
> > +		  find $(XEN_ROOT)/$(d)/ -type d -exec sh -c \
> > +		      "echo {} | sed 's,^$(XEN_ROOT)/$(d)/,,g' | xargs mkdir -p" \;);)
> 
> This is now a pretty nasty shell construct.
> 
> If you're going to use sed, you could just
>    find ... -print | sed ... | xargs mkdir -p

I think I will go with this one...

> Substituting {} into the middle of the shell rune is bad practice in
> general because it might contain metacharacters or spaces.  In our
> build system this doesn't matter because that breaks anyway, but it's
> very poor style (and also it buries the actual data flow path into the
> middle of a complicated manglement).
> 
> If you wanted to use -exec and sh, you could do something like this
>    -exec sh -c 'exec mkdir "${1#$(XEN_ROOT}/$(d)/}"' x {}
> maybe.

... because I cannot really make #2 work, and I'm not sure I fully
understand it.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 6/6] firmware/shim: fix build process to use POSIX find options

[Andrew Cooper]

On 17/01/18 17:32, Roger Pau Monné wrote:
> On Wed, Jan 17, 2018 at 04:24:27PM +0000, Ian Jackson wrote:
>> Roger Pau Monne writes ("[PATCH 6/6] firmware/shim: fix build process to use POSIX find options"):
>>> The -printf find option is not POSIX compatible, so replace it with
>>> another rune.
>> ...
>>>  		  cd $(D)/$(d); \
>>> -		  find $(XEN_ROOT)/$(d)/ -type d -printf "./%P\n" |  xargs mkdir -p);)
>>> +		  find $(XEN_ROOT)/$(d)/ -type d -exec sh -c \
>>> +		      "echo {} | sed 's,^$(XEN_ROOT)/$(d)/,,g' | xargs mkdir -p" \;);)
>> This is now a pretty nasty shell construct.
>>
>> If you're going to use sed, you could just
>>    find ... -print | sed ... | xargs mkdir -p
> I think I will go with this one...
>
>> Substituting {} into the middle of the shell rune is bad practice in
>> general because it might contain metacharacters or spaces.  In our
>> build system this doesn't matter because that breaks anyway, but it's
>> very poor style (and also it buries the actual data flow path into the
>> middle of a complicated manglement).
>>
>> If you wanted to use -exec and sh, you could do something like this
>>    -exec sh -c 'exec mkdir "${1#$(XEN_ROOT}/$(d)/}"' x {}
>> maybe.
> ... because I cannot really make #2 work, and I'm not sure I fully
> understand it.

Probably because it needs to be 'exec mkdir "$${1 .... so shell sees ${1
when its passed through Make.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 6/6] firmware/shim: fix build process to use POSIX find options

[Roger Pau Monné]

On Wed, Jan 17, 2018 at 05:58:43PM +0000, Andrew Cooper wrote:
> On 17/01/18 17:32, Roger Pau Monné wrote:
> > On Wed, Jan 17, 2018 at 04:24:27PM +0000, Ian Jackson wrote:
> >> Roger Pau Monne writes ("[PATCH 6/6] firmware/shim: fix build process to use POSIX find options"):
> >>> The -printf find option is not POSIX compatible, so replace it with
> >>> another rune.
> >> ...
> >>>  		  cd $(D)/$(d); \
> >>> -		  find $(XEN_ROOT)/$(d)/ -type d -printf "./%P\n" |  xargs mkdir -p);)
> >>> +		  find $(XEN_ROOT)/$(d)/ -type d -exec sh -c \
> >>> +		      "echo {} | sed 's,^$(XEN_ROOT)/$(d)/,,g' | xargs mkdir -p" \;);)
> >> This is now a pretty nasty shell construct.
> >>
> >> If you're going to use sed, you could just
> >>    find ... -print | sed ... | xargs mkdir -p
> > I think I will go with this one...

What about using:

find $(XEN_ROOT)/$(d)/ -type d | sed 's,^$(XEN_ROOT)/$(d)/,,g' | xargs mkdir -p

This AFAICT works fine, and should be the one involving less forks
since the whole output is processed at once.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 6/6] firmware/shim: fix build process to use POSIX find options

[Ian Jackson]

Roger Pau Monné writes ("Re: [Xen-devel] [PATCH 6/6] firmware/shim: fix build process to use POSIX find options"):
> What about using:
> 
> find $(XEN_ROOT)/$(d)/ -type d | sed 's,^$(XEN_ROOT)/$(d)/,,g' | xargs mkdir -p
> 
> This AFAICT works fine, and should be the one involving less forks
> since the whole output is processed at once.

Much nicer, thanks.  That IMO makes it easy to see what's going on.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel