Re: [Qemu-devel] [PATCH v4 7/8] hw: acpi: Export and share the ARM RSDP build

[Michael S. Tsirkin]

On Mon, Dec 17, 2018 at 05:46:48PM +0100, Samuel Ortiz wrote:
> On Mon, Dec 17, 2018 at 04:35:36PM +0100, Igor Mammedov wrote:
> > On Mon, 17 Dec 2018 14:49:59 +0100
> > Samuel Ortiz <address@hidden> wrote:
> > 
> > > On Mon, Dec 17, 2018 at 01:20:28PM +0100, Igor Mammedov wrote:
> > > > On Mon, 17 Dec 2018 11:48:37 +0100
> > > > Samuel Ortiz <address@hidden> wrote:
> > > > 
> > > > > Now that build_rsdp() supports building both legacy and current RSDP
> > > > > tables, we can move it to a generic folder (hw/acpi) and have the i386
> > > > > ACPI code reuse it in order to reduce code duplication.
> > > > > 
> > > > > Signed-off-by: Samuel Ortiz <address@hidden>
> > > > > Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
> > > > > Tested-by: Philippe Mathieu-Daudé <address@hidden>
> > > > > Reviewed-by: Andrew Jones <address@hidden>
> > > > > Reviewed-by: Igor Mammedov <address@hidden>
> > > > for future reference, if one changes patch in a significant way,
> > > > one is supposed to drop Tested/Reviewed-by tags so that reviewers
> > > > would look at it again and we by mistake won't merge not actually
> > > > reviewed changes.
> > > > 
> > > > [...]
> > > > 
> > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > > > index fb877648ac..846cb6d755 100644
> > > > > --- a/hw/i386/acpi-build.c
> > > > > +++ b/hw/i386/acpi-build.c
> > > > > @@ -2547,35 +2547,6 @@ build_amd_iommu(GArray *table_data, BIOSLinker 
> > > > > *linker)
> > > > >                   "IVRS", table_data->len - iommu_start, 1, NULL, 
> > > > > NULL);
> > > > >  }
> > > > >  
> > > > > -static void
> > > > > -build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned 
> > > > > rsdt_tbl_offset)
> > > > > -{
> > > > > -    /* AcpiRsdpDescriptor describes revision 2 RSDP table and as 
> > > > > result we
> > > > > -     * allocate extra 16 bytes for pc/q35 RSDP rev1 as well. Keep 
> > > > > extra 16 bytes
> > > > > -     * wasted to make sure we won't breake migration for machine 
> > > > > types older
> > > > > -     * than 2.3 due to size mismatch.
> > > > > -     */
> > > > > -    AcpiRsdpDescriptor *rsdp = acpi_data_push(rsdp_table, sizeof 
> > > > > *rsdp);
> > > > > -    unsigned rsdt_pa_size = sizeof(rsdp->rsdt_physical_address);
> > > > > -    unsigned rsdt_pa_offset =
> > > > > -        (char *)&rsdp->rsdt_physical_address - rsdp_table->data;
> > > > > -
> > > > > -    bios_linker_loader_alloc(linker, ACPI_BUILD_RSDP_FILE, 
> > > > > rsdp_table, 16,
> > > > > -                             true /* fseg memory */);
> > > > > -
> > > > > -    memcpy(&rsdp->signature, "RSD PTR ", 8);
> > > > > -    memcpy(rsdp->oem_id, ACPI_BUILD_APPNAME6, 6);
> > > > > -    /* Address to be filled by Guest linker */
> > > > > -    bios_linker_loader_add_pointer(linker,
> > > > > -        ACPI_BUILD_RSDP_FILE, rsdt_pa_offset, rsdt_pa_size,
> > > > > -        ACPI_BUILD_TABLE_FILE, rsdt_tbl_offset);
> > > > > -
> > > > > -    /* Checksum to be filled by Guest linker */
> > > > > -    bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
> > > > > -        (char *)rsdp - rsdp_table->data, 20 /* ACPI rev 1.0 RSDP 
> > > > > size */,
> > > > > -        (char *)&rsdp->checksum - rsdp_table->data);
> > > > > -}
> > > > > -
> > > > >  typedef
> > > > >  struct AcpiBuildState {
> > > > >      /* Copy of table in RAM (for patching). */
> > > > > @@ -2732,7 +2703,25 @@ void acpi_build(AcpiBuildTables *tables, 
> > > > > MachineState *machine)
> > > > >                 slic_oem.id, slic_oem.table_id);
> > > > >  
> > > > >      /* RSDP is in FSEG memory, so allocate it separately */
> > > > > -    build_rsdp(tables->rsdp, tables->linker, rsdt);
> > > > > +    {
> > > > > +        AcpiRsdpData rsdp_data = {
> > > > > +            .revision = 0,
> > > > > +            .oem_id = ACPI_BUILD_APPNAME6,
> > > > > +            .xsdt_tbl_offset = NULL,
> > > > > +            .rsdt_tbl_offset = &rsdt,
> > > > > +        };
> > > > > +        build_rsdp(tables->rsdp, tables->linker, &rsdp_data);
> > > > > +        if (!pcmc->rsdp_in_ram) {
> > > > > +            /*
> > > > > +             * Legacy machine types (2.2 and older) expect to get a 
> > > > > complete
> > > > > +             * revision 2 RSDP table, even though they only look at 
> > > > > the
> > > > not true, rev is set to 0 for pc machines,
> > > Rev is set to 0 but they effectively expect to get a structure which
> > > size is the rev 2 one. That's what I meant.
> > > 
> > > > the point of the original comment was
> > > > that we allocate extra 16 bytes but not actually using them and why 
> > > > it's bad
> > > > to drop it suddenly.
> > > >
> > > > > +             * revision 0 fields (xsdt pointer is not set). So in 
> > > > > order to
> > > > > +             * not break migration to those machine types we waste 
> > > > > 16 bytes
> > > > > +             * that we amend to the RSDP revision 0 structure.
> > > >                           ^^^ added
> > > > > +             */
> > > > Perhaps amended original comment would be clearer:
> > > > 
> > > >    /* We used to allocate extra space for RSDP rev 2 but used only 
> > > > space for
> > > >     * legacy RSDP and extra bytes were zeroed out. Keep wasting extra 
> > > > 16 bytes
> > > >     * to make sure we won't breake migration for machine types 2.2 and 
> > > > older
> > > >     * due to RSDP blob size mismatch.
> > > >     */
> > > > 
> > > > > +            build_append_int_noprefix(tables->rsdp, 0, 16);
> > Looks like I've haven't noticed, it might work but are you sure?
> > 0 here is uint64_t and then it's shifted 16 times which is sort of gray 
> > area (undefined behavior)
> AFAIK shifting by N bits when N > width(left operand) is undefined but
> that's not the case here. build_append_int_noprefix(foo, 0, 16) shift a 64 
> bits
> wide unsigned int by 8 bits, for each iterations of a 16 iterations
> loop. So we're doing 16 iterations of properly defined behaviour.
> 
> Cheers,
> Samuel.

It works fine but is IMHO unnecessarily tricky. Simple

        for (i = 0; i < 16; ++i)
                build_append_byte(tables->rsdp, 0)

is imho clearer.