[PATCH 0/2] xen: Arch Teardown

[PATCH 0/2] xen: Arch Teardown

[Andrew Cooper]

Sorry this is late...  Patch 1 ought to be ready in this form.

I'll leave it up to the ARM maintainers as to whether they want to take patch
2 in that form (with TEE in patch 3), or whether to merge the two.

Compile tested on all architectures, functionally tested on x86 only.

Branch available from:

  https://xenbits.xen.org/gitweb/?p=people/andrewcoop/xen.git;a=shortlog;h=refs/heads/arch-teardown

Full Gitlab CI run in progress:

  https://gitlab.com/xen-project/people/andyhhp/xen/-/pipelines/889759838

Andrew Cooper (2):
  x86: Introduce arch_domain_teardown()
  arm: Boilerpate arch_domain_teardown()

 xen/arch/arm/domain.c    | 32 ++++++++++++++++++++++++++++++++
 xen/arch/x86/domain.c    |  5 +++++
 xen/common/domain.c      |  6 ++++++
 xen/include/xen/domain.h |  1 +
 xen/include/xen/sched.h  |  1 +
 5 files changed, 45 insertions(+)


base-commit: b3880c365db89051728e1de6b6889c750cbdd915
-- 
2.30.2

[PATCH 1/2] x86: Introduce arch_domain_teardown()

[Andrew Cooper]

Plumb it into domain_teardown().  Provide arch_val in the teardown
continuation information for use by arch_domain_teardown().

No practical change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Bertrand Marquis <bertrand.marquis@arm.com>
CC: Jens Wiklander <jens.wiklander@linaro.org>
---
 xen/arch/arm/domain.c    | 5 +++++
 xen/arch/x86/domain.c    | 5 +++++
 xen/common/domain.c      | 6 ++++++
 xen/include/xen/domain.h | 1 +
 xen/include/xen/sched.h  | 1 +
 5 files changed, 18 insertions(+)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index d8ef6501ff8e..b3981d70a442 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -750,6 +750,11 @@ int arch_domain_create(struct domain *d,
     return rc;
 }
 
+int arch_domain_teardown(struct domain *d)
+{
+    return 0;
+}
+
 void arch_domain_destroy(struct domain *d)
 {
     /* IOMMU page table is shared with P2M, always call
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 39c215316546..5f66c2ae33d7 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -888,6 +888,11 @@ int arch_domain_create(struct domain *d,
     return rc;
 }
 
+int arch_domain_teardown(struct domain *d)
+{
+    return 0;
+}
+
 void arch_domain_destroy(struct domain *d)
 {
     if ( is_hvm_domain(d) )
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 6a440590fe2a..b0d850e3595b 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -416,6 +416,7 @@ static int domain_teardown(struct domain *d)
             PROG_none,
             PROG_gnttab_mappings,
             PROG_vcpu_teardown,
+            PROG_arch_teardown,
             PROG_done,
         };
 
@@ -436,6 +437,11 @@ static int domain_teardown(struct domain *d)
                 return rc;
         }
 
+    PROGRESS(arch_teardown):
+        rc = arch_domain_teardown(d);
+        if ( rc )
+            return rc;
+
     PROGRESS(done):
         break;
 
diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
index 26f9c4f6dd5b..c3348c90748f 100644
--- a/xen/include/xen/domain.h
+++ b/xen/include/xen/domain.h
@@ -80,6 +80,7 @@ int arch_domain_create(struct domain *d,
                        struct xen_domctl_createdomain *config,
                        unsigned int flags);
 
+int arch_domain_teardown(struct domain *d);
 void arch_domain_destroy(struct domain *d);
 
 void arch_domain_shutdown(struct domain *d);
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 85242a73d374..854f3e32c00e 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -589,6 +589,7 @@ struct domain
      */
     struct {
         unsigned int val;
+        unsigned int arch_val;
         struct vcpu *vcpu;
     } teardown;
 
-- 
2.30.2

[PATCH 2/2] arm: Boilerpate arch_domain_teardown()

[Andrew Cooper]

XXX to be filled in with TEE teardown.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Bertrand Marquis <bertrand.marquis@arm.com>
CC: Jens Wiklander <jens.wiklander@linaro.org>

Jens: In the same was as the previous patch in the common path, you want to
add a PROG_tee(?) here, and rearrange the right function(s).
---
 xen/arch/arm/domain.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index b3981d70a442..b00d0e4f30b7 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -752,6 +752,33 @@ int arch_domain_create(struct domain *d,
 
 int arch_domain_teardown(struct domain *d)
 {
+    BUG_ON(!d->is_dying);
+
+    /* See domain_teardown() for an explanation of all of this magic. */
+    switch ( d->teardown.arch_val )
+    {
+#define PROGRESS(x)                             \
+        d->teardown.arch_val = PROG_ ## x;      \
+        fallthrough;                            \
+    case PROG_ ## x
+
+        enum {
+            PROG_none,
+            PROG_done,
+        };
+
+    case PROG_none:
+        BUILD_BUG_ON(PROG_none != 0);
+
+    PROGRESS(done):
+        break;
+
+#undef PROGRESS
+
+    default:
+        BUG();
+    }
+
     return 0;
 }
 
-- 
2.30.2

Re: [PATCH 0/2] xen: Arch Teardown

[Bertrand Marquis]

Hi Andrew,

> On 5 Jun 2023, at 16:43, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> 
> Sorry this is late...  Patch 1 ought to be ready in this form.
> 
> I'll leave it up to the ARM maintainers as to whether they want to take patch
> 2 in that form (with TEE in patch 3), or whether to merge the two.
> 
> Compile tested on all architectures, functionally tested on x86 only.
> 
> Branch available from:
> 
>  https://xenbits.xen.org/gitweb/?p=people/andrewcoop/xen.git;a=shortlog;h=refs/heads/arch-teardown
> 
> Full Gitlab CI run in progress:
> 
>  https://gitlab.com/xen-project/people/andyhhp/xen/-/pipelines/889759838


Thanks a lot for that.

I will sync up with Jens and we will handle the rest :-)

Cheers
Bertrand

> 
> Andrew Cooper (2):
>  x86: Introduce arch_domain_teardown()
>  arm: Boilerpate arch_domain_teardown()
> 
> xen/arch/arm/domain.c    | 32 ++++++++++++++++++++++++++++++++
> xen/arch/x86/domain.c    |  5 +++++
> xen/common/domain.c      |  6 ++++++
> xen/include/xen/domain.h |  1 +
> xen/include/xen/sched.h  |  1 +
> 5 files changed, 45 insertions(+)
> 
> 
> base-commit: b3880c365db89051728e1de6b6889c750cbdd915
> -- 
> 2.30.2
> 

Re: [PATCH 1/2] x86: Introduce arch_domain_teardown()

[Roger Pau Monné]

On Mon, Jun 05, 2023 at 03:43:30PM +0100, Andrew Cooper wrote:
> Plumb it into domain_teardown().  Provide arch_val in the teardown
> continuation information for use by arch_domain_teardown().
> 
> No practical change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien@xen.org>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> CC: Bertrand Marquis <bertrand.marquis@arm.com>
> CC: Jens Wiklander <jens.wiklander@linaro.org>
> ---
>  xen/arch/arm/domain.c    | 5 +++++
>  xen/arch/x86/domain.c    | 5 +++++
>  xen/common/domain.c      | 6 ++++++
>  xen/include/xen/domain.h | 1 +
>  xen/include/xen/sched.h  | 1 +
>  5 files changed, 18 insertions(+)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index d8ef6501ff8e..b3981d70a442 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -750,6 +750,11 @@ int arch_domain_create(struct domain *d,
>      return rc;
>  }
>  
> +int arch_domain_teardown(struct domain *d)
> +{
> +    return 0;
> +}
> +
>  void arch_domain_destroy(struct domain *d)
>  {
>      /* IOMMU page table is shared with P2M, always call
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 39c215316546..5f66c2ae33d7 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -888,6 +888,11 @@ int arch_domain_create(struct domain *d,
>      return rc;
>  }
>  
> +int arch_domain_teardown(struct domain *d)
> +{
> +    return 0;
> +}
> +
>  void arch_domain_destroy(struct domain *d)
>  {
>      if ( is_hvm_domain(d) )
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 6a440590fe2a..b0d850e3595b 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -416,6 +416,7 @@ static int domain_teardown(struct domain *d)
>              PROG_none,
>              PROG_gnttab_mappings,
>              PROG_vcpu_teardown,
> +            PROG_arch_teardown,
>              PROG_done,
>          };
>  
> @@ -436,6 +437,11 @@ static int domain_teardown(struct domain *d)
>                  return rc;
>          }
>  
> +    PROGRESS(arch_teardown):
> +        rc = arch_domain_teardown(d);
> +        if ( rc )
> +            return rc;
> +
>      PROGRESS(done):
>          break;
>  
> diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
> index 26f9c4f6dd5b..c3348c90748f 100644
> --- a/xen/include/xen/domain.h
> +++ b/xen/include/xen/domain.h
> @@ -80,6 +80,7 @@ int arch_domain_create(struct domain *d,
>                         struct xen_domctl_createdomain *config,
>                         unsigned int flags);
>  
> +int arch_domain_teardown(struct domain *d);
>  void arch_domain_destroy(struct domain *d);
>  
>  void arch_domain_shutdown(struct domain *d);
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index 85242a73d374..854f3e32c00e 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -589,6 +589,7 @@ struct domain
>       */
>      struct {
>          unsigned int val;
> +        unsigned int arch_val;

While I haven't looked at patch 2, wouldn't such continuation
information better be encoded in arch_domain in whatever way is more
suitable for each architecture?

Thanks, Roger.

Re: [PATCH 1/2] x86: Introduce arch_domain_teardown()

[Andrew Cooper]

On 05/06/2023 4:19 pm, Roger Pau Monné wrote:
> On Mon, Jun 05, 2023 at 03:43:30PM +0100, Andrew Cooper wrote:
>> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
>> index 85242a73d374..854f3e32c00e 100644
>> --- a/xen/include/xen/sched.h
>> +++ b/xen/include/xen/sched.h
>> @@ -589,6 +589,7 @@ struct domain
>>       */
>>      struct {
>>          unsigned int val;
>> +        unsigned int arch_val;
> While I haven't looked at patch 2, wouldn't such continuation
> information better be encoded in arch_domain in whatever way is more
> suitable for each architecture?

Well, it's filling a hole here on 64bit builds, which it couldn't do in
arch_domain.

And it's contained inside teardown{} which already signals it as fairly
magic.

~Andrew

Re: [PATCH 1/2] x86: Introduce arch_domain_teardown()

[Jan Beulich]

On 05.06.2023 17:23, Andrew Cooper wrote:
> On 05/06/2023 4:19 pm, Roger Pau Monné wrote:
>> On Mon, Jun 05, 2023 at 03:43:30PM +0100, Andrew Cooper wrote:
>>> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
>>> index 85242a73d374..854f3e32c00e 100644
>>> --- a/xen/include/xen/sched.h
>>> +++ b/xen/include/xen/sched.h
>>> @@ -589,6 +589,7 @@ struct domain
>>>       */
>>>      struct {
>>>          unsigned int val;
>>> +        unsigned int arch_val;
>> While I haven't looked at patch 2, wouldn't such continuation
>> information better be encoded in arch_domain in whatever way is more
>> suitable for each architecture?
> 
> Well, it's filling a hole here on 64bit builds, which it couldn't do in
> arch_domain.
> 
> And it's contained inside teardown{} which already signals it as fairly
> magic.

Plus why have each architecture duplicate the field? I expect none of
the arch_domain_teardown() instances will remain without an actual
use of the new field, mid to long term.

I don't want to override Roger's concern, but from my pov
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH 1/2] x86: Introduce arch_domain_teardown()

[Roger Pau Monné]

On Wed, Jun 07, 2023 at 10:43:24AM +0200, Jan Beulich wrote:
> On 05.06.2023 17:23, Andrew Cooper wrote:
> > On 05/06/2023 4:19 pm, Roger Pau Monné wrote:
> >> On Mon, Jun 05, 2023 at 03:43:30PM +0100, Andrew Cooper wrote:
> >>> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> >>> index 85242a73d374..854f3e32c00e 100644
> >>> --- a/xen/include/xen/sched.h
> >>> +++ b/xen/include/xen/sched.h
> >>> @@ -589,6 +589,7 @@ struct domain
> >>>       */
> >>>      struct {
> >>>          unsigned int val;
> >>> +        unsigned int arch_val;
> >> While I haven't looked at patch 2, wouldn't such continuation
> >> information better be encoded in arch_domain in whatever way is more
> >> suitable for each architecture?
> > 
> > Well, it's filling a hole here on 64bit builds, which it couldn't do in
> > arch_domain.
> > 
> > And it's contained inside teardown{} which already signals it as fairly
> > magic.
> 
> Plus why have each architecture duplicate the field? I expect none of
> the arch_domain_teardown() instances will remain without an actual
> use of the new field, mid to long term.
> 
> I don't want to override Roger's concern, but from my pov
> Acked-by: Jan Beulich <jbeulich@suse.com>

Oh, no worries, I was meaning to reply I was fine with Andrews
justification, but forgot.

Thanks, Roger