[PATCH] tools/xen-foreign: Update the size for vcpu_guest_{core_regs, context}

[PATCH] tools/xen-foreign: Update the size for vcpu_guest_{core_regs, context}

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

Commit 918b8842a852 ("arm64: Change type of hsr, cpsr, spsr_el1 to
uint64_t") updated the size of the structure vcpu_guest_core_regs and
indirectly vcpu_guest_context.

On Arm, the two structures are only accessible to the tools and the
hypervisor (and therefore stable). However, they are still checked
by the scripts in tools/include/xen-foreign are not able to understand
that.

Ideally we should rework the scripts so we don't have to update
the size for non-stable structure. But I don't have limited time
to spend on the issue. So chose the simple solution and update
the size accordingly.

Note that we need to keep vcpu_guest_core_regs around because
the structure is used by vcpu_guest_context and therefore the
scripts expects the generated header to contain it.

Fixes: 918b8842a852 ("arm64: Change type of hsr, cpsr, spsr_el1 to uint64_t")
Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Julien Grall <jgrall@amazon.com>

---

Cc: michal.orzel@arm.com
Cc: olaf@aepfle.de
---
 tools/include/xen-foreign/reference.size | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/include/xen-foreign/reference.size b/tools/include/xen-foreign/reference.size
index a0409db5c458..11a06a7a432c 100644
--- a/tools/include/xen-foreign/reference.size
+++ b/tools/include/xen-foreign/reference.size
@@ -4,8 +4,8 @@ structs                   |   arm32   arm64  x86_32  x86_64
 start_info                |       -       -    1112    1168
 trap_info                 |       -       -       8      16
 cpu_user_regs             |       -       -      68     200
-vcpu_guest_core_regs      |     304     304       -       -
-vcpu_guest_context        |     344     344    2800    5168
+vcpu_guest_core_regs      |     312     312       -       -
+vcpu_guest_context        |     352     352    2800    5168
 arch_vcpu_info            |       0       0      24      16
 vcpu_time_info            |      32      32      32      32
 vcpu_info                 |      48      48      64      64
-- 
2.17.1

Re: [PATCH] tools/xen-foreign: Update the size for vcpu_guest_{core_regs, context}

[Bertrand Marquis]

Hi Julien,

> On 6 Jul 2021, at 14:20, Julien Grall <julien@xen.org> wrote:
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> Commit 918b8842a852 ("arm64: Change type of hsr, cpsr, spsr_el1 to
> uint64_t") updated the size of the structure vcpu_guest_core_regs and
> indirectly vcpu_guest_context.
> 
> On Arm, the two structures are only accessible to the tools and the
> hypervisor (and therefore stable). However, they are still checked
> by the scripts in tools/include/xen-foreign are not able to understand
> that.
> 
> Ideally we should rework the scripts so we don't have to update
> the size for non-stable structure. But I don't have limited time
> to spend on the issue. So chose the simple solution and update
> the size accordingly.
> 
> Note that we need to keep vcpu_guest_core_regs around because
> the structure is used by vcpu_guest_context and therefore the
> scripts expects the generated header to contain it.
> 
> Fixes: 918b8842a852 ("arm64: Change type of hsr, cpsr, spsr_el1 to uint64_t")
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

But I have other issues now:
xg_dom_arm.c:143:15: error: format '%x' expects argument of type 'unsigned int', but argument 3 has type 'uint64_t' {aka 'long long unsigned int'} [-Werror=format=]
  143 |     DOMPRINTF("Initial state CPSR %#"PRIx32" PC %#"PRIx32,
      |               ^~~~~~~~~~~~~~~~~~~~~~~
  144 |            ctxt->user_regs.cpsr, ctxt->user_regs.pc32);
      |            ~~~~~~~~~~~~~~~~~~~~
      |                           |
      |                           uint64_t {aka long long unsigned int}

We will work on a patch to fix those.

Cheers
Bertrand

> 
> ---
> 
> Cc: michal.orzel@arm.com
> Cc: olaf@aepfle.de
> ---
> tools/include/xen-foreign/reference.size | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/include/xen-foreign/reference.size b/tools/include/xen-foreign/reference.size
> index a0409db5c458..11a06a7a432c 100644
> --- a/tools/include/xen-foreign/reference.size
> +++ b/tools/include/xen-foreign/reference.size
> @@ -4,8 +4,8 @@ structs                   |   arm32   arm64  x86_32  x86_64
> start_info                |       -       -    1112    1168
> trap_info                 |       -       -       8      16
> cpu_user_regs             |       -       -      68     200
> -vcpu_guest_core_regs      |     304     304       -       -
> -vcpu_guest_context        |     344     344    2800    5168
> +vcpu_guest_core_regs      |     312     312       -       -
> +vcpu_guest_context        |     352     352    2800    5168
> arch_vcpu_info            |       0       0      24      16
> vcpu_time_info            |      32      32      32      32
> vcpu_info                 |      48      48      64      64
> -- 
> 2.17.1
> 
> 

Re: [PATCH] tools/xen-foreign: Update the size for vcpu_guest_{core_regs, context}

[Jan Beulich]

On 06.07.2021 15:20, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> Commit 918b8842a852 ("arm64: Change type of hsr, cpsr, spsr_el1 to
> uint64_t") updated the size of the structure vcpu_guest_core_regs and
> indirectly vcpu_guest_context.
> 
> On Arm, the two structures are only accessible to the tools and the
> hypervisor (and therefore stable). However, they are still checked

I suppose s/stable/not stable/ and ...

> by the scripts in tools/include/xen-foreign are not able to understand
> that.
> 
> Ideally we should rework the scripts so we don't have to update
> the size for non-stable structure. But I don't have limited time

... s/don't/only/ ?

> to spend on the issue. So chose the simple solution and update
> the size accordingly.
> 
> Note that we need to keep vcpu_guest_core_regs around because
> the structure is used by vcpu_guest_context and therefore the
> scripts expects the generated header to contain it.

If vcpu_guest_context is also tools-only, why does it need keeping
(for Arm)? IOW can't you drop vcpu_guest_core_regs from the scripts
altogether, and mark vcpu_guest_context with valid sizes for x86
only?

Jan

Re: [PATCH] tools/xen-foreign: Update the size for vcpu_guest_{core_regs, context}

[Julien Grall]

Hi,

On 06/07/2021 15:07, Jan Beulich wrote:
> On 06.07.2021 15:20, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> Commit 918b8842a852 ("arm64: Change type of hsr, cpsr, spsr_el1 to
>> uint64_t") updated the size of the structure vcpu_guest_core_regs and
>> indirectly vcpu_guest_context.
>>
>> On Arm, the two structures are only accessible to the tools and the
>> hypervisor (and therefore stable). However, they are still checked
> 
> I suppose s/stable/not stable/ and ...
> 
>> by the scripts in tools/include/xen-foreign are not able to understand
>> that.
>>
>> Ideally we should rework the scripts so we don't have to update
>> the size for non-stable structure. But I don't have limited time
> 
> ... s/don't/only/ ?

Yes I will update.

> 
>> to spend on the issue. So chose the simple solution and update
>> the size accordingly.
>>
>> Note that we need to keep vcpu_guest_core_regs around because
>> the structure is used by vcpu_guest_context and therefore the
>> scripts expects the generated header to contain it.
> 
> If vcpu_guest_context is also tools-only, why does it need keeping
> (for Arm)? IOW can't you drop vcpu_guest_core_regs from the scripts
> altogether, and mark vcpu_guest_context with valid sizes for x86
> only?
I have tried to use "-" in place of the size but the checker is still 
not happy. I didn't find another way without modifying the script...

I don't know the script and I don't have a lot of time to work on the 
bug fix. So this is not something I could do this week.

So this leave us to the following options:
   1) Someone volunteer to fix the script
   2) Revert the patch that broke the build
   3) Accept this patch

I would prefer 3) so we have more time to work on making the script a 
lot smarter.

Cheers,

-- 
Julien Grall

Re: [PATCH] tools/xen-foreign: Update the size for vcpu_guest_{core_regs, context}

[Jan Beulich]

On 06.07.2021 16:24, Julien Grall wrote:
> On 06/07/2021 15:07, Jan Beulich wrote:
>> On 06.07.2021 15:20, Julien Grall wrote:
>>> From: Julien Grall <jgrall@amazon.com>
>>>
>>> Commit 918b8842a852 ("arm64: Change type of hsr, cpsr, spsr_el1 to
>>> uint64_t") updated the size of the structure vcpu_guest_core_regs and
>>> indirectly vcpu_guest_context.
>>>
>>> On Arm, the two structures are only accessible to the tools and the
>>> hypervisor (and therefore stable). However, they are still checked
>>
>> I suppose s/stable/not stable/ and ...
>>
>>> by the scripts in tools/include/xen-foreign are not able to understand
>>> that.
>>>
>>> Ideally we should rework the scripts so we don't have to update
>>> the size for non-stable structure. But I don't have limited time
>>
>> ... s/don't/only/ ?
> 
> Yes I will update.
> 
>>
>>> to spend on the issue. So chose the simple solution and update
>>> the size accordingly.
>>>
>>> Note that we need to keep vcpu_guest_core_regs around because
>>> the structure is used by vcpu_guest_context and therefore the
>>> scripts expects the generated header to contain it.
>>
>> If vcpu_guest_context is also tools-only, why does it need keeping
>> (for Arm)? IOW can't you drop vcpu_guest_core_regs from the scripts
>> altogether, and mark vcpu_guest_context with valid sizes for x86
>> only?
> I have tried to use "-" in place of the size but the checker is still 
> not happy. I didn't find another way without modifying the script...
> 
> I don't know the script and I don't have a lot of time to work on the 
> bug fix. So this is not something I could do this week.
> 
> So this leave us to the following options:
>    1) Someone volunteer to fix the script
>    2) Revert the patch that broke the build
>    3) Accept this patch
> 
> I would prefer 3) so we have more time to work on making the script a 
> lot smarter.

Well, if we have no-one who knows the script, and the simple thing
of removing the one obvious line doesn't help, then I agree we want
to go with 3) for now, even if it looks odd to adjust number for
something that's not supposed to be checked in the first place. Of
course I also wouldn't mind 2), as it seems pretty clear that the
patch was insufficiently build-tested.

Jan

Re: [PATCH] tools/xen-foreign: Update the size for vcpu_guest_{core_regs, context}

[Julien Grall]

Hi Jan,

On 06/07/2021 16:10, Jan Beulich wrote:
> On 06.07.2021 16:24, Julien Grall wrote:
>> On 06/07/2021 15:07, Jan Beulich wrote:
>>> On 06.07.2021 15:20, Julien Grall wrote:
>>>> From: Julien Grall <jgrall@amazon.com>
>>>>
>>>> Commit 918b8842a852 ("arm64: Change type of hsr, cpsr, spsr_el1 to
>>>> uint64_t") updated the size of the structure vcpu_guest_core_regs and
>>>> indirectly vcpu_guest_context.
>>>>
>>>> On Arm, the two structures are only accessible to the tools and the
>>>> hypervisor (and therefore stable). However, they are still checked
>>>
>>> I suppose s/stable/not stable/ and ...
>>>
>>>> by the scripts in tools/include/xen-foreign are not able to understand
>>>> that.
>>>>
>>>> Ideally we should rework the scripts so we don't have to update
>>>> the size for non-stable structure. But I don't have limited time
>>>
>>> ... s/don't/only/ ?
>>
>> Yes I will update.
>>
>>>
>>>> to spend on the issue. So chose the simple solution and update
>>>> the size accordingly.
>>>>
>>>> Note that we need to keep vcpu_guest_core_regs around because
>>>> the structure is used by vcpu_guest_context and therefore the
>>>> scripts expects the generated header to contain it.
>>>
>>> If vcpu_guest_context is also tools-only, why does it need keeping
>>> (for Arm)? IOW can't you drop vcpu_guest_core_regs from the scripts
>>> altogether, and mark vcpu_guest_context with valid sizes for x86
>>> only?
>> I have tried to use "-" in place of the size but the checker is still
>> not happy. I didn't find another way without modifying the script...
>>
>> I don't know the script and I don't have a lot of time to work on the
>> bug fix. So this is not something I could do this week.
>>
>> So this leave us to the following options:
>>     1) Someone volunteer to fix the script
>>     2) Revert the patch that broke the build
>>     3) Accept this patch
>>
>> I would prefer 3) so we have more time to work on making the script a
>> lot smarter.
> 
> Well, if we have no-one who knows the script, and the simple thing
> of removing the one obvious line doesn't help, then I agree we want
> to go with 3) for now, even if it looks odd to adjust number for
> something that's not supposed to be checked in the first place. Of
> course I also wouldn't mind 2), as it seems pretty clear that the
> patch was insufficiently build-tested.
The series has dragged on for quite a while on the ML. So I would like 
to avoid a revert and wait for further rework in the tools side.

Also Bertrand pointed out there is a second fix necessary (I think they 
should still live separately). I will wait him to confirm that the 
toolstack is building for 32-bit and 64-bit after the two patches are 
applied.

Cheers,

-- 
Julien Grall

Re: [PATCH] tools/xen-foreign: Update the size for vcpu_guest_{core_regs, context}

[Michal Orzel]

On 06.07.2021 15:20, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> Commit 918b8842a852 ("arm64: Change type of hsr, cpsr, spsr_el1 to
> uint64_t") updated the size of the structure vcpu_guest_core_regs and
> indirectly vcpu_guest_context.
> 
> On Arm, the two structures are only accessible to the tools and the
> hypervisor (and therefore stable). However, they are still checked
> by the scripts in tools/include/xen-foreign are not able to understand
> that.
> 
> Ideally we should rework the scripts so we don't have to update
> the size for non-stable structure. But I don't have limited time
> to spend on the issue. So chose the simple solution and update
> the size accordingly.
> 
> Note that we need to keep vcpu_guest_core_regs around because
> the structure is used by vcpu_guest_context and therefore the
> scripts expects the generated header to contain it.
> 
> Fixes: 918b8842a852 ("arm64: Change type of hsr, cpsr, spsr_el1 to uint64_t")

Sorry for this inconvenience due to the lack of xen-tools testing.
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
Reviewed-by: Michal Orzel <michal.orzel@arm.com>
Tested-by: Michal Orzel <michal.orzel@arm.com>
> ---
> 
> Cc: michal.orzel@arm.com
> Cc: olaf@aepfle.de
> ---
>  tools/include/xen-foreign/reference.size | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/include/xen-foreign/reference.size b/tools/include/xen-foreign/reference.size
> index a0409db5c458..11a06a7a432c 100644
> --- a/tools/include/xen-foreign/reference.size
> +++ b/tools/include/xen-foreign/reference.size
> @@ -4,8 +4,8 @@ structs                   |   arm32   arm64  x86_32  x86_64
>  start_info                |       -       -    1112    1168
>  trap_info                 |       -       -       8      16
>  cpu_user_regs             |       -       -      68     200
> -vcpu_guest_core_regs      |     304     304       -       -
> -vcpu_guest_context        |     344     344    2800    5168
> +vcpu_guest_core_regs      |     312     312       -       -
> +vcpu_guest_context        |     352     352    2800    5168
>  arch_vcpu_info            |       0       0      24      16
>  vcpu_time_info            |      32      32      32      32
>  vcpu_info                 |      48      48      64      64
>