| Message ID | 20210706132000.29892-1-julien@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tools/xen-foreign: Update the size for vcpu_guest_{core_regs, context} | expand |
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 > >
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
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,
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
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,
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 >
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