Re: [Xen-devel] Re: [RFC] [PATCH 0/2] Basic SeaBIOS support for Xen HVM

[Ian Campbell <Ian.Campbell@eu.citrix.com>]

[-- Attachment #1: Type: text/plain, Size: 5189 bytes --]

On Tue, 2011-05-24 at 01:17 +0100, Kevin O'Connor wrote:
> On Mon, May 23, 2011 at 11:44:38AM +0100, Ian Campbell wrote:
> > On Tue, 2011-05-17 at 16:59 +0100, Ian Campbell wrote:
> > > The following implements all your feedback (I hope). I have squashed it
> > > down into a single commit which supports direct boot.
> 
> BTW, in general, the above patch looks okay to me.
> 
> > I've gotten all the PCI setup and ACPI stuff etc etc working but,
> > frankly, the patch to SeaBIOS is getting to be pretty enormous and
> > intrusive.
> 
> Is that due to incompleteness / innacuracies in the current SeaBIOS
> code, or due to requirements specific to Xen?

A mixture of both but mainly requirements specific to Xen, I think.

PCI setup is one of the main things, the interrupt routing in particular
is different which has knock on effects on chipset setup (e.g. legacy
PCI ISA IRQ routing) and the BIOS tables (e.g. ACPI _PRT entries). This
strikes me as being highly "mainboard" specific, IOW the stuff I would
expect to find in coreboot rather than SeaBIOS.

Another difference is that the existing hvmloader tables are ACPI 2.0
while SeaBIOS only creates 1.0 tables, I'm not sure that strictly
speaking counts as a incompleteness in SeaBIOS since it's not obvious
that Xen actually needs/uses any of the functionality of 2.0. Also it
might be something SeaBIOS would like to grow in any case.

> If you have test code, I'd be curious to see a patch on the mailing
> list - it may help later to understand the use cases for SeaBIOS.

It's in a pretty embarrassing state at the moment, but OK. Please note
that I was nowhere near suggesting this patch was in any way suitable to
be applied or even near to becoming an RFC!

I initially took a rather gung-ho approach in the interests of matching
as closely as possible how hvmloader+rombios sets things up, rather than
evaluating the actual useful meaning of the differences. Having made
things match my intention was to go back and evaluate each change and
attempt to get rid of it unless there was an extremely pressing reason
not to, but I didn't reach that phase yet. So I think this is an upper
bound (possibly by a large-ish order of magnitude) on the level of
change.

I hardcoded a few things here and there too (e.g. numbers of CPUs), in
the interests of getting something working before going back and
cleaning it up.

The diff (vs. the patches I already posted) is attached, it's diffstat
is:

 src/Kconfig          |   34 +
 src/acpi-dsdt.dsl    | 9482 +++++++++++++++++++++++++++++++++++++++++++++-----
 src/acpi-dsdt.hex    | 9331 +++++++++++++++++++++++++++++++++++++++++++-------
 src/acpi.c           |  152 +-
 src/acpi.h           |   93 +
 src/config.h         |    6 +-
 src/dev-i440fx.c     |   16 +-
 src/pciinit.c        |    6 +-
 src/pmm.c            |    5 +-
 src/post.c           |    5 +
 src/util.h           |    5 +
 src/xen.c            |  128 +-
 src/xen.h            |    3 +
 src/xen/hvm/hvm_op.h |  245 ++
 src/xen/hvm/params.h |  145 +
 src/xen/version.h    |   94 +
 src/xen/xen.h        |  739 ++++
 src/xen_hypercall.h  |  187 +
 18 files changed, 18591 insertions(+), 2085 deletions(-)

The DSDT change rather dominates, dropping it gives a more sensible:
 11 files changed, 414 insertions(+), 39 deletions(-)

I just ripped the DSDT out of hvmloader, I'm sure with analysis the
changes could be reduced to something much more sane, although I have a
feeling it would still end up as a subtly different variant of the table
(_PRT and such).

> > The more I look at it the more I am coming to the conclusion that it
> > would be better to have hvmloader setup all this platform level stuff
> > and pass details onto SeaBIOS as necessary, following the model used
> > with coreboot->SeaBIOS rather than the emulator's way of doing things.
> > hvmloader basically already fulfils the same role for Xen HVM guests as
> > coreboot does for physical hardware so I think that makes a certain
> > amount of sense.
> 
> I'm okay with that approach as well

The SeaBIOS patch for this approach seems likely to be an awful lot
smaller. Basically most "if (COREBOOT)" become effectively "if
(COREBOOT||XEN)" + a Xen equivalent to coreboot_copy_biostable and
coreboot_fill_map.

> - there are pros and cons to each method.

Apart from avoiding bloating SeaBIOS with a load of Xen specific stuff
(which would be duplicated in hvmloader and also needed by the tianocore
firmware variant etc) I think the big thing pulling me towards this
approach is the idea that the BIOS tables describe the hardware, and
since the "hardware" in this case is defined by Xen the tables ought to
be supplied by Xen. I don't know if that's a distinction which people
who really know about things at this level actually make though.

>  (At various points, it's been discussed whether SeaBIOS
> should generate ACPI tables for coreboot,

Would that involve pulling a bunch of mainboard specific stuff from
coreboot into SeaBIOS?

>  and it has also been
> discussed if QEmu should just pass in ACPI tables to SeaBIOS..)

That sounds plausible to me, at least based on my current understanding
of what I think is going on ;-)

Ian.

[-- Attachment #2: xen-hack.patch.bz2 --]
[-- Type: application/x-bzip, Size: 90631 bytes --]

[-- Attachment #3: Type: text/plain, Size: 137 bytes --]

_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
http://www.seabios.org/mailman/listinfo/seabios