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

[Igor Mammedov <imammedo@redhat.com>]

On Tue, 21 Nov 2017 18:31:19 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> 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.

I didn't really used static checker for this purpose but in my humble
experience of writing portable cross platform code, mixed endianness
always was a source of bugs and reviewing corresponding code is much
harder and a way to reduce/simplify code was isolate byte-swapping to
accessors while the the rest of the code works with native encoding.

it seems a bit wrong to make code twisted (when we don't have) for
the sake of some tooling to work.

anyways, if you prefer having mixed endianness here, I won't mind much,
as impact of test case is much smaller so we'll see down the road
if it became any better. (as far as we continue going in direction
of using host byteorder within main qemu acpi code, which we were
doing by gradually converting acpi structures to build_append_foo API)

> 
> >    
> > > > 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.
> > > > > > > 
> > > > > > >