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

[Ross Philipson <Ross.Philipson@xxxxxxxxxx>]

> -----Original Message-----
> From: Ian Campbell
> Sent: Wednesday, April 04, 2012 5:30 AM
> To: Ross Philipson
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxx
> Subject: Re: [Xen-devel] [PATCH 00/07] HVM firmware passthrough
> 
> Seems like this thread slipped through the cracks, sorry about that.
> Please do give us a prod (e.g. a ping message) after a few days/a week
> when this happens.

Ok, thanks, will do.

> 
> On Thu, 2012-03-22 at 14:03 +0000, Ross Philipson wrote:
> > -----Original Message-----
> > > From: Tim Deegan [mailto:tim@xxxxxxx]
> > > Sent: Thursday, March 22, 2012 7:26 AM
> > > To: Keir Fraser
> > > Cc: xen-devel@xxxxxxxxxxxxxxxxxxx; Ian Campbell; Ross Philipson
> > > Subject: Re: [Xen-devel] [PATCH 00/07] HVM firmware passthrough
> > >
> > > Hi,
> > >
> > > At 09:24 +0000 on 20 Mar (1332235452), Tim Deegan wrote:
> > > > My impression from the earlier discussions is that we're pasing
> > > > largish blobs of binary BIOS goop around, which aren't suitable to
> > > > go into xenstore.  Dropping them in memory where HVMloader can
> > > > pick them up seems reasonable.
> > > >
> > > > All the control-path stuff - what the blobs are, where they are
> > > > &c, should go through Xenstore, though.
> > >
> > > So having looked at the code, I think the module system is really
> > > overkill - AIUI all the things you're talking about passing through
> > > are ACPI tables, which have their own length fields internally. So
> > > all
> >
> > Ok well fair enough. I can go back and do it again, making something
> > smaller. For the record, I am passing in more than just ACPI tables; I
> > am passing in parts of the SMBIOS tables. Each of the SMBIOS types
> > does not actually report its length but rather you have to parse them
> > for the double terminator after the string section. I guess it seems a
> > shame to have to do that parse logic in two places.
> 
> I think Tim was suggesting that in xs this looks something like:
>       .../hvmloader/acpi/address
>       (no .../acpi/length length? encoded in tables)
>       .../hvmloader/smbios/address
>       .../hvmloader/smbios/length
>       .../hvmloader/some-other/{address,length?}...
> 
> with the data at the referenced addresses.
> 
> 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. 
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.

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.

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?

Thanks
Ross

> 
> 
> Ian.
> 

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