Re: [Qemu-devel] [PATCH v2 6/8] hw: arm: Support both legacy and current RSDP build

[Igor Mammedov <imammedo@redhat.com>]

On Thu, 29 Nov 2018 15:59:42 +0100
Samuel Ortiz <sameo@linux.intel.com> wrote:

> On Thu, Nov 29, 2018 at 03:42:43PM +0100, Andrew Jones wrote:
> > On Thu, Nov 29, 2018 at 02:24:26PM +0100, Samuel Ortiz wrote:  
> > > We add the ability to build legacy or current RSDP tables, based on the
> > > AcpiRsdpData revision field passed to build_rsdp().
> > > Although arm/virt only uses RSDP v2, adding that capability to
> > > build_rsdp will allow us to share the RSDP build code between ARM and x86.
> > > 
> > > Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
> > > ---
> > >  hw/arm/virt-acpi-build.c | 37 ++++++++++++++++++++++++++-----------
> > >  1 file changed, 26 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > > index 4782aea4fe..e1338b6f5a 100644
> > > --- a/hw/arm/virt-acpi-build.c
> > > +++ b/hw/arm/virt-acpi-build.c
> > > @@ -378,23 +378,38 @@ build_rsdp(GArray *tbl, BIOSLinker *linker, AcpiRsdpData *rsdp_data)
> > >      g_array_append_vals(tbl, rsdp_data->oem_id, 6); /* OEMID */
> > >      build_append_int_noprefix(tbl, rsdp_data->revision, 1); /* Revision */
> > >      build_append_int_noprefix(tbl, 0, 4); /* RsdtAddress */
> > > -    build_append_int_noprefix(tbl, 36, 4); /* Length */
> > > -
> > > -    /* XSDT address to be filled by guest linker */
> > > -    build_append_int_noprefix(tbl, 0, 8); /* XsdtAddress */
> > > -    bios_linker_loader_add_pointer(linker, ACPI_BUILD_RSDP_FILE,
> > > -                                   24, 8,
> > > -                                   ACPI_BUILD_TABLE_FILE,
> > > -                                   *rsdp_data->xsdt_tbl_offset);
> > > -
> > > -    build_append_int_noprefix(tbl, 0, 1); /* Extended Checksum */
> > > -    build_append_int_noprefix(tbl, 0, 3); /* Reserved */
> > > +    if (rsdp_data->rsdt_tbl_offset) {  
> > 
> > I see why a pointer was used now. Using a pointer ensures a zero
> > offset won't fail this test. However the test could be replaced with
> > rsdp_data->revision == 0.
> >   
> > > +        /* RSDT address to be filled by guest linker */
> > > +        bios_linker_loader_add_pointer(linker,
> > > +                                       ACPI_BUILD_RSDP_FILE, 16, 4,
> > > +                                       ACPI_BUILD_TABLE_FILE,
> > > +                                       *rsdp_data->rsdt_tbl_offset);
> > > +    }
> > >  
> > >      /* Checksum to be filled by guest linker */
> > >      bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
> > >                                      0, 20, /* ACPI rev 1.0 RSDP size */
> > >                                      8);
> > >  
> > > +    if (rsdp_data->revision == 0) {
> > > +        /* ACPI 1.0 RSDP, we're done */
> > > +        return;
> > > +    }
> > > +
> > > +    /* The RSDP revision is 2 and later, we must have an XSDT pointer */
> > > +    g_assert(rsdp_data->xsdt_tbl_offset != NULL);  
> > 
> > So here's the justification for the pointers. We sanity check the callers.  
> We could sanity check the callers without pointers as well, I don't
> think there's a strong advantage for pointers here, except consistence.
I remember now,
we need pointer to RSDT offset here so that we could skip bios_linker_loader_add_pointer(rsdt)
when RSDT isn't provided (0 is also a valid offset), as for xsdt field it follows
rsdt one for consistency and we can do sanity checks on it.

assert here seems not really necessary as it guarantied to SIGSEGV
on NULL point dereference at bios_linker_loader_add_pointer(*xdst) callsite.
But I don't care here, so I'll leave it up to Samuel.

> > We're missing the (rsdp_data->revision == 0 && rsdp_data->rsdt_tbl_offset)
> > sanity check though.  
> I think there's nothing preventing a caller to include both rsdt and
> xsdt if it wants to be able to run on both < 2.0 and 2.0+ platforms with
> the same table. So if rsdt is set we should add it, regardless of the revision.
agreed,
we don't do this in QEMU but it's correct, there could be both and it's better
follow better document spec in this case.

> 
> Cheers,
> Samuel.