Re: [Qemu-devel] [PATCH v2] tests/bios-tables-test: Fix endianess problems when passing data to iasl

[Michael S. Tsirkin]

On Tue, Nov 21, 2017 at 05:22:36PM +0100, Igor Mammedov wrote:
> > > > > > > probably it's been discussed but, why not do
> > > > > > >  leXX_to_cpu()
> > > > > > > here, instead of making each place that access read field
> > > > > > > to do leXX_to_cpu() manually.?
> > > > > > > 
> > > > > > > Beside of keeping access to structure in natural host order,
> > > > > > > it should also be less error-prone as field users don't
> > > > > > > have to worry about endianness.      
> > > > > > 
> > > > > > Yes, I suggested that.
> > > > > > The issue is that we don't byte-swap all of the tables
> > > > > > (we can't, that would require a full ACPI parser).
> > > > > > So when byte-swapping we end up with a mixed host/LE
> > > > > > structures.    
> > > ...  
> > > > > What tables/use-cases do you have in mind where we'd need full
> > > > > ACPI parser /whatever it is/?    
> > > > 
> > > > We dump ACPI tables for parsing by iasl.  
> > > I don't really see a problem here (i.e. how dumping blobs for
> > > iasl would require byte-swap all of the tables).  
> > 
> > It's not all the tables. We would need to track what is and
> > what isn't swapped.
> Why we need track swapping at all?
> Why not just convert everything to host order and work with that
> in C code, so we won't have to deal with mixed endianness anywhere
> in code beside of accessors wrappers? 

It's a tooling issue really.

Consider any structure, e.g. AcpiRsdtDescriptorRev1.  We want its fields
to have specific endian-ness so we can use static checking for that down
the line.

So it must be LE since we use it to format guest tables.

Therefore this change:
-    uint32_t addr = data->rsdp_table.rsdt_physical_address;
+    uint32_t addr = le32_to_cpu(data->rsdp_table.rsdt_physical_address);

is really a requirement, otherwise we end up with converting
field format in-place and static checkers can't support that
right now.


>  
> > > Where bios-tables-test.c is concerned we should just read
> > > a blob from qemu (get its location/len/name) and dump it to
> > > disk without changing anything in it.  
> > 
> > Exactly.
> > 
> > > > >     
> > > > > > Keeping it all LE seems cleaner.
> > > > > > 
> > > > > >