Re: ACPI: usage of sandbox virtual addresses
Hi Heinrich, On Fri, Dec 29, 2023 at 8:58 AM Heinrich Schuchardt wrote: > > > > Am 29. Dezember 2023 15:31:24 MEZ schrieb Simon Glass : > >Hi Heinrich, > > > >On Fri, Dec 29, 2023 at 11:14 AM Heinrich Schuchardt > >wrote: > >> > >> On 12/29/23 09:26, Simon Glass wrote: > >> > Hi Heinrich, > >> > > >> > On Tue, Dec 26, 2023 at 10:03 AM Heinrich Schuchardt > >> > wrote: > >> >> > >> >> On 12/26/23 10:50, Simon Glass wrote: > >> >>> Hi Heinrich, > >> >>> > >> >>> On Tue, Dec 26, 2023 at 8:56 AM Heinrich Schuchardt > >> >>> wrote: > >> > >> Hello Simon, > >> > >> currently we use sandbox virtual addresses in all ACPI tables. This > >> means that an application started by the U-Boot sandbox consuming > >> these > >> tables will crash due to accessing invalid addresses. > >> > >> Shouldn't the ACPI tables be migrated to use valid pointers? > >> >>> > >> >>> The confusion arises due to the difference between addresses and > >> >>> pointers. If we store addresses in the ACPI tables, then things should > >> >>> work. > >> >>> > >> >>> My approach has been to avoid using casts, but instead use > >> >>> map_sysmem() and mem_to_sysmem(). > >> >>> > >> >>> Which particular piece of code is wrong in this case? > >> >> > >> >> Tables DSDT, XSDT, RSDP, FADT in the sandbox contain sandbox virtual > >> >> addresses instead of pointers to real host memory. > >> >> > >> >> All code referring to these tables should be changed.433: > >> >> *gnvs = map_to_sysmem(ctx->current); > >> > > >> > I'm still not quite clear on this...can you point to functions or > >> > lines of code? When I look at acpi_add_table() it does use memmap, but > >> > perhaps some parts are wrong? > >> > > >> > Regards, > >> > Simon > >> > >> Here are some examples where the wrong values are set. We must get rid > >> of all map_to_sysmem() calls where writing ACPI tables. > >> > >> lib/acpi/acpi_table.c: > >> 170 rsdt->entry[i] = map_to_sysmem(table); > >> 188 xsdt->entry[i] = map_to_sysmem(table); > >> > >> lib/acpi/base.c: > >> 27: rsdp->rsdt_address = map_to_sysmem(rsdt); > >> 30: rsdp->xsdt_address = map_to_sysmem(xsdt); > >> > >> arch/x86/cpu/baytrail/acpi.c: > >> 81: fadt->x_firmware_ctrl = map_to_sysmem(ctx->facs); > >> 82: fadt->x_dsdt = map_to_sysmem(ctx->dsdt); > >> > >> arch/x86/cpu/quark/acpi.c: > >> 76: fadt->x_firmware_ctrl = map_to_sysmem(ctx->facs); > >> 77: fadt->x_dsdt = map_to_sysmem(ctx->dsdt); > >> > >> arch/x86/cpu/tangier/acpi.c: > >> 47: fadt->x_firmware_ctrl = map_to_sysmem(ctx->facs); > >> 48: fadt->x_dsdt = map_to_sysmem(ctx->dsdt); > >> > >> arch/x86/lib/acpi_table.c: > >> 200:tcpa->lasa = map_to_sysmem(log); > >> 271:tpm2->lasa = map_to_sysmem(lasa); > >> 433:*gnvs = map_to_sysmem(ctx->current); > >> 575:fadt->x_firmware_ctrl = map_to_sysmem(facs); > >> 576:fadt->x_dsdt = map_to_sysmem(dsdt); > > > >OK, I see. But at least within sandbox, the address is what we want to > >store, not the pointer. Are you worried about what an EFI app will do > >in that case, when we run an app from sandbox? If so, then yes it is > >definitely a problem. > > > >Regards, > >Simon > > The sandbox is an environment to run EFI binaries inside an OS. So the ACPI > tables must contain pointers. > > The acpi command should display sandbox virtual addresses. All conversions > should be moved to the command. OK, it makes sense to me. For sandbox we want addresses, but in this case (with an external program using the addresses) we are going to have to use pointers cast to addresses. Regards, Simon
Re: ACPI: usage of sandbox virtual addresses
Am 29. Dezember 2023 15:31:24 MEZ schrieb Simon Glass : >Hi Heinrich, > >On Fri, Dec 29, 2023 at 11:14 AM Heinrich Schuchardt >wrote: >> >> On 12/29/23 09:26, Simon Glass wrote: >> > Hi Heinrich, >> > >> > On Tue, Dec 26, 2023 at 10:03 AM Heinrich Schuchardt >> > wrote: >> >> >> >> On 12/26/23 10:50, Simon Glass wrote: >> >>> Hi Heinrich, >> >>> >> >>> On Tue, Dec 26, 2023 at 8:56 AM Heinrich Schuchardt >> >>> wrote: >> >> Hello Simon, >> >> currently we use sandbox virtual addresses in all ACPI tables. This >> means that an application started by the U-Boot sandbox consuming these >> tables will crash due to accessing invalid addresses. >> >> Shouldn't the ACPI tables be migrated to use valid pointers? >> >>> >> >>> The confusion arises due to the difference between addresses and >> >>> pointers. If we store addresses in the ACPI tables, then things should >> >>> work. >> >>> >> >>> My approach has been to avoid using casts, but instead use >> >>> map_sysmem() and mem_to_sysmem(). >> >>> >> >>> Which particular piece of code is wrong in this case? >> >> >> >> Tables DSDT, XSDT, RSDP, FADT in the sandbox contain sandbox virtual >> >> addresses instead of pointers to real host memory. >> >> >> >> All code referring to these tables should be changed.433: >> >>*gnvs = map_to_sysmem(ctx->current); >> > >> > I'm still not quite clear on this...can you point to functions or >> > lines of code? When I look at acpi_add_table() it does use memmap, but >> > perhaps some parts are wrong? >> > >> > Regards, >> > Simon >> >> Here are some examples where the wrong values are set. We must get rid >> of all map_to_sysmem() calls where writing ACPI tables. >> >> lib/acpi/acpi_table.c: >> 170 rsdt->entry[i] = map_to_sysmem(table); >> 188 xsdt->entry[i] = map_to_sysmem(table); >> >> lib/acpi/base.c: >> 27: rsdp->rsdt_address = map_to_sysmem(rsdt); >> 30: rsdp->xsdt_address = map_to_sysmem(xsdt); >> >> arch/x86/cpu/baytrail/acpi.c: >> 81: fadt->x_firmware_ctrl = map_to_sysmem(ctx->facs); >> 82: fadt->x_dsdt = map_to_sysmem(ctx->dsdt); >> >> arch/x86/cpu/quark/acpi.c: >> 76: fadt->x_firmware_ctrl = map_to_sysmem(ctx->facs); >> 77: fadt->x_dsdt = map_to_sysmem(ctx->dsdt); >> >> arch/x86/cpu/tangier/acpi.c: >> 47: fadt->x_firmware_ctrl = map_to_sysmem(ctx->facs); >> 48: fadt->x_dsdt = map_to_sysmem(ctx->dsdt); >> >> arch/x86/lib/acpi_table.c: >> 200:tcpa->lasa = map_to_sysmem(log); >> 271:tpm2->lasa = map_to_sysmem(lasa); >> 433:*gnvs = map_to_sysmem(ctx->current); >> 575:fadt->x_firmware_ctrl = map_to_sysmem(facs); >> 576:fadt->x_dsdt = map_to_sysmem(dsdt); > >OK, I see. But at least within sandbox, the address is what we want to >store, not the pointer. Are you worried about what an EFI app will do >in that case, when we run an app from sandbox? If so, then yes it is >definitely a problem. > >Regards, >Simon The sandbox is an environment to run EFI binaries inside an OS. So the ACPI tables must contain pointers. The acpi command should display sandbox virtual addresses. All conversions should be moved to the command. Best regards Heinrich
Re: ACPI: usage of sandbox virtual addresses
Hi Heinrich, On Fri, Dec 29, 2023 at 11:14 AM Heinrich Schuchardt wrote: > > On 12/29/23 09:26, Simon Glass wrote: > > Hi Heinrich, > > > > On Tue, Dec 26, 2023 at 10:03 AM Heinrich Schuchardt > > wrote: > >> > >> On 12/26/23 10:50, Simon Glass wrote: > >>> Hi Heinrich, > >>> > >>> On Tue, Dec 26, 2023 at 8:56 AM Heinrich Schuchardt > >>> wrote: > > Hello Simon, > > currently we use sandbox virtual addresses in all ACPI tables. This > means that an application started by the U-Boot sandbox consuming these > tables will crash due to accessing invalid addresses. > > Shouldn't the ACPI tables be migrated to use valid pointers? > >>> > >>> The confusion arises due to the difference between addresses and > >>> pointers. If we store addresses in the ACPI tables, then things should > >>> work. > >>> > >>> My approach has been to avoid using casts, but instead use > >>> map_sysmem() and mem_to_sysmem(). > >>> > >>> Which particular piece of code is wrong in this case? > >> > >> Tables DSDT, XSDT, RSDP, FADT in the sandbox contain sandbox virtual > >> addresses instead of pointers to real host memory. > >> > >> All code referring to these tables should be changed.433: > >> *gnvs = map_to_sysmem(ctx->current); > > > > I'm still not quite clear on this...can you point to functions or > > lines of code? When I look at acpi_add_table() it does use memmap, but > > perhaps some parts are wrong? > > > > Regards, > > Simon > > Here are some examples where the wrong values are set. We must get rid > of all map_to_sysmem() calls where writing ACPI tables. > > lib/acpi/acpi_table.c: > 170 rsdt->entry[i] = map_to_sysmem(table); > 188 xsdt->entry[i] = map_to_sysmem(table); > > lib/acpi/base.c: > 27: rsdp->rsdt_address = map_to_sysmem(rsdt); > 30: rsdp->xsdt_address = map_to_sysmem(xsdt); > > arch/x86/cpu/baytrail/acpi.c: > 81: fadt->x_firmware_ctrl = map_to_sysmem(ctx->facs); > 82: fadt->x_dsdt = map_to_sysmem(ctx->dsdt); > > arch/x86/cpu/quark/acpi.c: > 76: fadt->x_firmware_ctrl = map_to_sysmem(ctx->facs); > 77: fadt->x_dsdt = map_to_sysmem(ctx->dsdt); > > arch/x86/cpu/tangier/acpi.c: > 47: fadt->x_firmware_ctrl = map_to_sysmem(ctx->facs); > 48: fadt->x_dsdt = map_to_sysmem(ctx->dsdt); > > arch/x86/lib/acpi_table.c: > 200:tcpa->lasa = map_to_sysmem(log); > 271:tpm2->lasa = map_to_sysmem(lasa); > 433:*gnvs = map_to_sysmem(ctx->current); > 575:fadt->x_firmware_ctrl = map_to_sysmem(facs); > 576:fadt->x_dsdt = map_to_sysmem(dsdt); OK, I see. But at least within sandbox, the address is what we want to store, not the pointer. Are you worried about what an EFI app will do in that case, when we run an app from sandbox? If so, then yes it is definitely a problem. Regards, Simon
Re: ACPI: usage of sandbox virtual addresses
On 12/29/23 09:26, Simon Glass wrote: Hi Heinrich, On Tue, Dec 26, 2023 at 10:03 AM Heinrich Schuchardt wrote: On 12/26/23 10:50, Simon Glass wrote: Hi Heinrich, On Tue, Dec 26, 2023 at 8:56 AM Heinrich Schuchardt wrote: Hello Simon, currently we use sandbox virtual addresses in all ACPI tables. This means that an application started by the U-Boot sandbox consuming these tables will crash due to accessing invalid addresses. Shouldn't the ACPI tables be migrated to use valid pointers? The confusion arises due to the difference between addresses and pointers. If we store addresses in the ACPI tables, then things should work. My approach has been to avoid using casts, but instead use map_sysmem() and mem_to_sysmem(). Which particular piece of code is wrong in this case? Tables DSDT, XSDT, RSDP, FADT in the sandbox contain sandbox virtual addresses instead of pointers to real host memory. All code referring to these tables should be changed.433: *gnvs = map_to_sysmem(ctx->current); I'm still not quite clear on this...can you point to functions or lines of code? When I look at acpi_add_table() it does use memmap, but perhaps some parts are wrong? Regards, Simon Here are some examples where the wrong values are set. We must get rid of all map_to_sysmem() calls where writing ACPI tables. lib/acpi/acpi_table.c: 170 rsdt->entry[i] = map_to_sysmem(table); 188 xsdt->entry[i] = map_to_sysmem(table); lib/acpi/base.c: 27: rsdp->rsdt_address = map_to_sysmem(rsdt); 30: rsdp->xsdt_address = map_to_sysmem(xsdt); arch/x86/cpu/baytrail/acpi.c: 81: fadt->x_firmware_ctrl = map_to_sysmem(ctx->facs); 82: fadt->x_dsdt = map_to_sysmem(ctx->dsdt); arch/x86/cpu/quark/acpi.c: 76: fadt->x_firmware_ctrl = map_to_sysmem(ctx->facs); 77: fadt->x_dsdt = map_to_sysmem(ctx->dsdt); arch/x86/cpu/tangier/acpi.c: 47: fadt->x_firmware_ctrl = map_to_sysmem(ctx->facs); 48: fadt->x_dsdt = map_to_sysmem(ctx->dsdt); arch/x86/lib/acpi_table.c: 200:tcpa->lasa = map_to_sysmem(log); 271:tpm2->lasa = map_to_sysmem(lasa); 433:*gnvs = map_to_sysmem(ctx->current); 575:fadt->x_firmware_ctrl = map_to_sysmem(facs); 576:fadt->x_dsdt = map_to_sysmem(dsdt); Best regards Heinrich Best regards Heinrich
Re: ACPI: usage of sandbox virtual addresses
Hi Heinrich, On Tue, Dec 26, 2023 at 10:03 AM Heinrich Schuchardt wrote: > > On 12/26/23 10:50, Simon Glass wrote: > > Hi Heinrich, > > > > On Tue, Dec 26, 2023 at 8:56 AM Heinrich Schuchardt > > wrote: > >> > >> Hello Simon, > >> > >> currently we use sandbox virtual addresses in all ACPI tables. This > >> means that an application started by the U-Boot sandbox consuming these > >> tables will crash due to accessing invalid addresses. > >> > >> Shouldn't the ACPI tables be migrated to use valid pointers? > > > > The confusion arises due to the difference between addresses and > > pointers. If we store addresses in the ACPI tables, then things should > > work. > > > > My approach has been to avoid using casts, but instead use > > map_sysmem() and mem_to_sysmem(). > > > > Which particular piece of code is wrong in this case? > > Tables DSDT, XSDT, RSDP, FADT in the sandbox contain sandbox virtual > addresses instead of pointers to real host memory. > > All code referring to these tables should be changed. I'm still not quite clear on this...can you point to functions or lines of code? When I look at acpi_add_table() it does use memmap, but perhaps some parts are wrong? Regards, Simon
Re: ACPI: usage of sandbox virtual addresses
On 12/26/23 10:50, Simon Glass wrote: Hi Heinrich, On Tue, Dec 26, 2023 at 8:56 AM Heinrich Schuchardt wrote: Hello Simon, currently we use sandbox virtual addresses in all ACPI tables. This means that an application started by the U-Boot sandbox consuming these tables will crash due to accessing invalid addresses. Shouldn't the ACPI tables be migrated to use valid pointers? The confusion arises due to the difference between addresses and pointers. If we store addresses in the ACPI tables, then things should work. My approach has been to avoid using casts, but instead use map_sysmem() and mem_to_sysmem(). Which particular piece of code is wrong in this case? Tables DSDT, XSDT, RSDP, FADT in the sandbox contain sandbox virtual addresses instead of pointers to real host memory. All code referring to these tables should be changed. Best regards Heinrich
Re: ACPI: usage of sandbox virtual addresses
Hi Heinrich, On Tue, Dec 26, 2023 at 8:56 AM Heinrich Schuchardt wrote: > > Hello Simon, > > currently we use sandbox virtual addresses in all ACPI tables. This > means that an application started by the U-Boot sandbox consuming these > tables will crash due to accessing invalid addresses. > > Shouldn't the ACPI tables be migrated to use valid pointers? The confusion arises due to the difference between addresses and pointers. If we store addresses in the ACPI tables, then things should work. My approach has been to avoid using casts, but instead use map_sysmem() and mem_to_sysmem(). Which particular piece of code is wrong in this case? Regards, Simon
ACPI: usage of sandbox virtual addresses
Hello Simon, currently we use sandbox virtual addresses in all ACPI tables. This means that an application started by the U-Boot sandbox consuming these tables will crash due to accessing invalid addresses. Shouldn't the ACPI tables be migrated to use valid pointers? Best regards Heinrich
