Re: [Xen-devel] [PATCH 00/07] HVM firmware passthrough

[Ross Philipson <Ross.Philipson@xxxxxxxxxx>]

> -----Original Message-----
> From: Ian Campbell
> Sent: Thursday, April 05, 2012 6:08 AM
> To: Ross Philipson
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxx
> Subject: Re: [Xen-devel] [PATCH 00/07] HVM firmware passthrough
> 
> On Wed, 2012-04-04 at 20:25 +0100, Ross Philipson wrote:
> > > If it's convenient (or even just for consistency) there's no reason
> > > IMHO not to include the length even where the length is part of the
> data.
> > > Similarly you can add checksums etc if those are useful.
> > >
> >
> > Well actually it does not really do any good to have the length in
> > xenstore (at least for the types we are currently talking about) other
> > than knowing the overall extent of the blob. The problem is that we
> > are talking about multiple tables chained together. For ACPI this is
> > not a problem because the table length is specified in the table
> > header. For SMBIOS (where there will almost certainly be multiple
> > tables) the length of each table is not specified and so the set would
> > have to be re-parsed in hvmloader to find each individual table.
> 
> I see. So you need to be able to find the individual tables so that
> smbios_type_<N>_init can check for an overriding table <N> in the
> passed-through tables, it seems reasonable to try and avoid needing to
> parse a big lump of tables each time to see if the one you are
> interested in is there.
> 
> I think this can work by having .../smbios/<N>/{address,etc} in
> XenStore. You could also have .../smbios/OEM/{...} for the stuff for
> smbios_type_vendor_oem_init, which I think could easily just be a single
> lump?
> 
> I think you don't need the same thing for the ACPI side since there you
> just provide secondary tables?

There could be quite a few SMBIOS tables being passed in. On the order of 20 - 
30 of them and thet are pretty small. It seems a bit odd to break it up like 
this for lots of little chunks. There could be more than one ACPI table also 
(multiple SSDTs and/or other static tables). Also the OEM SMBIOS tables are all 
discrete and they could not go in as a single lump.

This approach will certainly work but I am still not sure about it being the 
best way. 

> 
> >  That was the motivation for a separate table header we define. I
> > still think some simple entry delimiter structure with some small
> > amount of information about each item is a good idea.
> 
> I think the content of .../smbios/<N>/* serves the same purpose as a
> delimiter?

Yes if we use this approach.

> 
> > But in general I don't have a problem with using xenstore for this
> > sort of thing so I am fine with the basic concept.
> >
> > > > > you'd need to do is have a type code and an address in xenstore
> > > > > somewhere, the same way we pass a type code and a string for the
> > > > > other BIOS customizations.
> > > > >
> > > >
> > > > So I am not exactly sure how to proceed here since libxc seems the
> > > > correct place to load the individual blobs (ACPI tables, SMBIOS
> > > > junk) but libxc seems too low level to be writing values to
> > > > xenstore (and it does not do that at present). Would it be better
> > > > to have a peer API to
> > > > xc_hvm_build() that write the chunks and returns the addresses
> > > > where it loaded them? Or should it be totally moved out of libxc?
> > > > Advice would be appreciated...
> > >
> > > The layering relationship between libxc and libxs here is a bit of a
> > > pain, but lets not try and solve that now.
> > >
> > > I think you can just add a guest_address field to your struct
> > > xc_hvm_module as an output field which the caller would then push
> > > into xenstore along with the (potentially updated) length field.
> > >
> > > Given the above XS layout then I think where you have a list of
> > > modules in xc_hvm_build_args you can just have "struct xc_hvm_module
> acpi".
> > > Similarly for smbios and whatever new table types come up in the
> future.
> > > I don't think having each module type explicitly here matters since
> > > each new table type needs a bunch of support code in at least
> > > hvmloader anyway so adding a few lines to libxc to expose it at the
> > > same time is fine.
> >
> > That seems like a reasonable way to do it. Especially if (wrt below)
> > we have the caller glom the like bits together.
> >
> > >
> > > > Finally, I had made this a generic framework because I thought it
> > > > could be extended in the future to pass in other types of
> > > > firmware-ish blobs at runtime. It also handles the case where the
> > > > passing of one set of tables/blobs is not tied to passing another
> > > > set. E.g. in our work we pass in some static ACPI tables to
> > > > satisfy one feature and construct/pass-in an SSDT to satisfy
> another unrelated feature.
> > >
> > > I think it is ok to have it be up to the caller to glom those
> > > together into a single "ACPI" blob which is processed by hvmloader
> > > into the right places. Or is there a problem with that which hasn't
> occurred to me?
> > > (Likewise in the SMBIOS case)
> > >
> > > If it is a problem then
> > >   .../acpi/0/...
> > >   .../acpi/1/...
> > > (or s/0/SSDT0/ and s/1/SSDT1/ etc) works for the XS side of things.
> > > Not sure about the libxc level, I'll worry about it when you tell me
> > > what I've missed which makes it necessary ;-)
> >
> > Well that approach was mostly for flexibility (as in the scenario I
> > pointed out). The only other issue might be the measuring of
> > components by a domain builder but I don't think that that prevents
> > glomming. Presumably the glomming code is part of the overall domain
> > builder code so a measure-then-glom operation can happen. And doing
> > the merge in the tools makes the code in hvmloader simpler so I am
> > good with that.
> 
> You've convinced me that having the SMBIOS tables each be presented
> separately makes sense.
> 
> At the libxc layer you could have "struct xc_hvm_module *smbios". I
> expect in the simple case this would just point into an array on the
> callers stack but a caller could also do something more dynamic if they
> wanted to. If you think it would be useful then a linked-list thing here
> would also work.

Again assuming the xenstore approach, I guess it would probably have to be a 
linked list because there could be a fair number of them (mainly SMBIOS).

> 
> > Given all that, the only real issue I see at the moment is how to deal
> > with multiple entries within a blob that don't readily specify their
> > own lengths. I am in favor of a delimiting struct with possibly a
> > terminating form (like a flag). Then all we put in xenstore is the gpe
> > for each type.
> >
> > Finally I wanted to bring up again the idea of another helper
> > component (lib maybe) that can be used to build and glom (I like that
> > word) modules. Note that in many cases the passed in firmware is going
> > to be pulled from the host firmware. I already have bunches of codes
> > that do that. Perhaps I should start an RFC thread for that as a
> > separate task? Thoughts on this?
> 
> You mean some sort of libacpi / libsmbios type things, helper routines
> for parsing and manipulating tables etc? (such a thing doesn't already
> exist somewhere?)
> 
> I suspect your usecases are the ones which would make most extensive use
> of such a thing but I also assume that at least some of it would be
> useful to any caller which was passing in these tables. If you want to
> maintain it within the Xen tree then that works for me.

Yea along those lines. I was thinking of maybe a libxenhvm that had utility 
routines for collecting the system firmware information that one wanted to pass 
to a guest. It could also provide an API that wrapped up the setting of the 
firmware values that hvmloader consumes including the bits that are on the 
shared info page you want to get rid of. The usefulness of this library is 
diminished if we go the all xenstore route above so I am going to shelf this 
until that is settled.

Oh and yea, there are tools to get this information (acpidump/dmidecode) but 
they unfortunately do not come in library form or necessarily give you the 
desired output. And in newer kernels ACPI/DMI is accessible through sysfs.

> 
> Ian.
> 
> >
> > Thanks
> > Ross
> >
> > >
> > >
> > > Ian.
> > >
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@xxxxxxxxxxxxx
> > http://lists.xen.org/xen-devel
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel