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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxxxx>
>>>
>>> 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