Re: [PATCH 1/3] tools/libacpi: Use 64-byte alignment for FACS [and 1 more messages]

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.09.2021 16:50, Ian Jackson wrote:
> Andrew Cooper writes ("Re: [PATCH 1/3] tools/libacpi: Use 64-byte alignment 
> for FACS"):
>> The current code is clearly wrong, but happens to work correctly in
>> hvmloader because FACS is the first table written and it starts on a
>> page boundary.  The logic does not work correctly when libxl passes a
>> buffer which doesn't start on a page boundary.
>>
>> As a consequence, I'm not sure what to suggest as a Fixes tag here,
>> except "the logic has been wrong for 15 years - patch everything".
> 
> Jan Beulich writes ("Re: [PATCH 2/3] tools/libxl: Correctly aligned buffer 
> for ACPI tables"):
>> But the buffers obtained via libxl__malloc() have no association with
>> guest address space layout (or at least they aren't supposed to have).
>> That's solely determined by mem_alloc(). I think it is a bug of
>> mem_alloc() to determine padding from alloc_currp; it should
>> rather/also maintain a current address in guest address space (e.g.
>> by having separate alloc_currp and alloc_currv). Not doing so leaves
>> us prone to encountering the same issue again down the road. For
>> example if higher than page alignment was requested from somewhere in
>> libacpi.
> 
> I think the two of you are saying roughly the same thing here ?

That's my view of it, indeed.

> There was a question about how (and if) this should be backported.
> I'm afraid I don't quite follow all the implications, but I doubt that
> a a substantial overhaul as described would be a good idea for a
> backport.  What is the impact for backports, and can we have a more
> targeted fix there ?  Are, in fact, Kevin's original patches, suitable
> to fix the issue for stable trees ?

I think they are; the risk with not (also) backporting the more complete
fix to do away with the bad assumptions is that eventual subsequent
backports may then run into the same issue again. Like Andrew suggested,
I think we want to first see how intrusive a "proper" fix is. If it's
really "bad", we can still decide to backport only the original change
(which I'd rather view as a workaround) while trying to make sure future
backports won't end up hitting the underlying problem.

Of course there's also the option to simply declare "future backports
here are unlikely"; I wouldn't really like such, though.

Jan