Re: [xen-unstable bisection] complete test-amd64-i386-xl-shadow

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.09.2020 11:20, Roger Pau Monné wrote:
> On Mon, Sep 07, 2020 at 10:21:36AM +0200, Jan Beulich wrote:
>> On 07.09.2020 00:48, osstest service owner wrote:
>>> branch xen-unstable
>>> xenbranch xen-unstable
>>> job test-amd64-i386-xl-shadow
>>> testid guest-saverestore
>>>
>>> Tree: linux git://xenbits.xen.org/linux-pvops.git
>>> Tree: linuxfirmware git://xenbits.xen.org/osstest/linux-firmware.git
>>> Tree: qemu git://xenbits.xen.org/qemu-xen-traditional.git
>>> Tree: qemuu git://xenbits.xen.org/qemu-xen.git
>>> Tree: xen git://xenbits.xen.org/xen.git
>>>
>>> *** Found and reproduced problem changeset ***
>>>
>>>   Bug is in tree:  xen git://xenbits.xen.org/xen.git
>>>   Bug introduced:  696c273f3d9a169911308fb7e0a702a3eb6a150d
>>>   Bug not present: a609b6577f7867db4be1470130b7b3c686398c4f
>>>   Last fail repro: http://logs.test-lab.xenproject.org/osstest/logs/153833/
>>>
>>>
>>>   commit 696c273f3d9a169911308fb7e0a702a3eb6a150d
>>>   Author: Jan Beulich <jbeulich@xxxxxxxx>
>>>   Date:   Fri Sep 4 11:13:01 2020 +0200
>>>   
>>>       x86: generalize padding field handling
>>>       
>>>       The original intention was to ignore padding fields, but the pattern
>>>       matched only ones whose names started with an underscore. Also match
>>>       fields whose names are in line with the C spec by not having a leading
>>>       underscore. (Note that the leading ^ in the sed regexps was pointless
>>>       and hence get dropped.)
>>
>> I conclude this needs to be reverted, and there was a thinko of mine
>> involved here: Avoiding translation of padding fields would be okay
>> only when their values don't get checked in the native handler. We
>> effectively have a not written down (afaict) rule here that _pad*
>> fields get ignored (and hence don't need translation), while pad*
>> fields may not be ignored and hence may need translation. I don't
>> like this state, but I also can't think of a good way out of it, at
>> least not just yet.
> 
> I think his stems from the fact that we don't have a rule whether
> explicit padding fields in structs should be zeroed. IIRC there are
> hypercalls that would check for padding fields to be 0, while others
> don't.

Right - we only have an almost-rule for new code.

> At this point I assume we can only implement the least restrictive
> one, which is to not force padding fields to be zeroed?

Well, we could formalize the (or ideally: some) naming model to
distinguish the cases. Question then is going to be in how many
cases neither contributor nor reviewers are(n't) going to pay
attention.

> This would have the side effect that they cannot be later used to
> introduce additional fields to the struct without signaling the
> version in use.

Exactly, so unilaterally going the "don't copy and hence don't
require zero" route is not an option imo.

Jan