| Message ID | 20210909163441.44418-2-kevin.stefanov@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix alignment of FACS in guests | expand |
On 09/09/2021 17:34, Kevin Stefanov wrote: > The spec requires 64-byte alignment, not 16. > > Signed-off-by: Kevin Stefanov <kevin.stefanov@citrix.com> > --- > CC: Jan Beulich <jbeulich@suse.com> > CC: Andrew Cooper <andrew.cooper3@citrix.com> > > Note: This does not fix the FACS alignment inside guests yet. See next > patch. The history here is complex. c/s 938cee9d41d3 in 2006 deleted a comment saying // FACS: should be 64-bit alignment // so it is put at the start of buffer // as the buffer is 64 bit alignment Clearly it means byte rather than bit, but the change also introduced logic to the effect of buf += ROUNDUP(sizeof(facs), 16). This (incorrect) alignment survived several morphs of the logic and was moved from hvmloader into libacpi by c/s 73b72736e6ca. 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". ~Andrew
On 09.09.2021 18:34, Kevin Stefanov wrote: > The spec requires 64-byte alignment, not 16. > > Signed-off-by: Kevin Stefanov <kevin.stefanov@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com>
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 ?
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 ?
Thanks,
Ian.
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
diff --git a/tools/libacpi/build.c b/tools/libacpi/build.c index a61dd5583a..fe2db66a62 100644 --- a/tools/libacpi/build.c +++ b/tools/libacpi/build.c @@ -532,7 +532,7 @@ int acpi_build_tables(struct acpi_ctxt *ctxt, struct acpi_config *config) * Fill in high-memory data structures, starting at @buf. */ - facs = ctxt->mem_ops.alloc(ctxt, sizeof(struct acpi_20_facs), 16); + facs = ctxt->mem_ops.alloc(ctxt, sizeof(struct acpi_20_facs), 64); if (!facs) goto oom; memcpy(facs, &Facs, sizeof(struct acpi_20_facs));
The spec requires 64-byte alignment, not 16. Signed-off-by: Kevin Stefanov <kevin.stefanov@citrix.com> --- CC: Jan Beulich <jbeulich@suse.com> CC: Andrew Cooper <andrew.cooper3@citrix.com> Note: This does not fix the FACS alignment inside guests yet. See next patch. --- tools/libacpi/build.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)