On 8/20/19 9:25 PM, Jan Kiszka wrote:
> On 19.08.19 20:34, Ralf Ramsauer wrote:
>> We will soon introduce a new setup_data version and extend the
>> structure. This requires some preparational work for the sanity check of
>> the header and the check of the version.
>>
>> Use the following strategy:
>>
>> 1. Ensure that the header declares at least enough space for the version
>> and the compatible_version as we must hold that fields for any
>> version. Furthermore, the location and semantics of those fields will
>> never change.
>>
>> 2. Copy over data -- as much as we can. The length is either limited by
>> the header length, or the length of setup_data.
>>
>> 3. Things are now in place -- sanity check if the header length complies
>> the actual version.
>>
>> For future versions of the setup_data, only step 3 requires alignment.
>>
>> Signed-off-by: Ralf Ramsauer <ralf.ramsa...@oth-regensburg.de>
>> ---
>> arch/x86/include/uapi/asm/bootparam.h | 22 +++++++-----
>> arch/x86/kernel/jailhouse.c | 50 +++++++++++++++++----------
>> 2 files changed, 44 insertions(+), 28 deletions(-)
>>
>> diff --git a/arch/x86/include/uapi/asm/bootparam.h
>> b/arch/x86/include/uapi/asm/bootparam.h
>> index a06cbf019744..6163b1afa7b3 100644
>> --- a/arch/x86/include/uapi/asm/bootparam.h
>> +++ b/arch/x86/include/uapi/asm/bootparam.h
>> @@ -137,15 +137,19 @@ struct boot_e820_entry {
>> * setup data structure.
>> */
>> struct jailhouse_setup_data {
>> - __u16 version;
>> - __u16 compatible_version;
>> - __u16 pm_timer_address;
>> - __u16 num_cpus;
>> - __u64 pci_mmconfig_base;
>> - __u32 tsc_khz;
>> - __u32 apic_khz;
>> - __u8 standard_ioapic;
>> - __u8 cpu_ids[255];
>> + struct {
>> + __u16 version;
>> + __u16 compatible_version;
>> + } __attribute__((packed)) hdr;
>> + struct {
>> + __u16 pm_timer_address;
>> + __u16 num_cpus;
>> + __u64 pci_mmconfig_base;
>> + __u32 tsc_khz;
>> + __u32 apic_khz;
>> + __u8 standard_ioapic;
>> + __u8 cpu_ids[255];
>> + } __attribute__((packed)) v1;
>> } __attribute__((packed));
>> /* The so-called "zeropage" */
>> diff --git a/arch/x86/kernel/jailhouse.c b/arch/x86/kernel/jailhouse.c
>> index 108c48d0d40e..fc65ed3346ac 100644
>> --- a/arch/x86/kernel/jailhouse.c
>> +++ b/arch/x86/kernel/jailhouse.c
>> @@ -21,6 +21,8 @@
>> #include <asm/setup.h>
>> static __initdata struct jailhouse_setup_data setup_data;
>> +#define SETUP_DATA_V1_LEN (sizeof(setup_data.hdr) +
>> sizeof(setup_data.v1))
>> +
>> static unsigned int precalibrated_tsc_khz;
>> static uint32_t jailhouse_cpuid_base(void)
>> @@ -44,7 +46,7 @@ static void jailhouse_get_wallclock(struct
>> timespec64 *now)
>> static void __init jailhouse_timer_init(void)
>> {
>> - lapic_timer_frequency = setup_data.apic_khz * (1000 / HZ);
>> + lapic_timer_frequency = setup_data.v1.apic_khz * (1000 / HZ);
>> }
>> static unsigned long jailhouse_get_tsc(void)
>> @@ -87,14 +89,14 @@ static void __init
>> jailhouse_get_smp_config(unsigned int early)
>> register_lapic_address(0xfee00000);
>> - for (cpu = 0; cpu < setup_data.num_cpus; cpu++) {
>> - generic_processor_info(setup_data.cpu_ids[cpu],
>> + for (cpu = 0; cpu < setup_data.v1.num_cpus; cpu++) {
>> + generic_processor_info(setup_data.v1.cpu_ids[cpu],
>> boot_cpu_apic_version);
>> }
>> smp_found_config = 1;
>> - if (setup_data.standard_ioapic) {
>> + if (setup_data.v1.standard_ioapic) {
>> mp_register_ioapic(0, 0xfec00000, gsi_top, &ioapic_cfg);
>> /* Register 1:1 mapping for legacy UART IRQs 3 and 4 */
>> @@ -125,9 +127,9 @@ static int __init jailhouse_pci_arch_init(void)
>> pcibios_last_bus = 0xff;
>> #ifdef CONFIG_PCI_MMCONFIG
>> - if (setup_data.pci_mmconfig_base) {
>> + if (setup_data.v1.pci_mmconfig_base) {
>> pci_mmconfig_add(0, 0, pcibios_last_bus,
>> - setup_data.pci_mmconfig_base);
>> + setup_data.v1.pci_mmconfig_base);
>> pci_mmcfg_arch_init();
>> }
>> #endif
>> @@ -138,6 +140,7 @@ static int __init jailhouse_pci_arch_init(void)
>> static void __init jailhouse_init_platform(void)
>> {
>> u64 pa_data = boot_params.hdr.setup_data;
>> + unsigned long setup_data_len;
>> struct setup_data header;
>> void *mapping;
>> @@ -162,16 +165,8 @@ static void __init jailhouse_init_platform(void)
>> memcpy(&header, mapping, sizeof(header));
>> early_memunmap(mapping, sizeof(header));
>> - if (header.type == SETUP_JAILHOUSE &&
>> - header.len >= sizeof(setup_data)) {
>> - pa_data += offsetof(struct setup_data, data);
>> -
>> - mapping = early_memremap(pa_data, sizeof(setup_data));
>> - memcpy(&setup_data, mapping, sizeof(setup_data));
>> - early_memunmap(mapping, sizeof(setup_data));
>> -
>> + if (header.type == SETUP_JAILHOUSE)
>> break;
>> - }
>> pa_data = header.next;
>> }
>> @@ -179,13 +174,26 @@ static void __init jailhouse_init_platform(void)
>> if (!pa_data)
>> panic("Jailhouse: No valid setup data found");
>> - if (setup_data.compatible_version >
>> JAILHOUSE_SETUP_REQUIRED_VERSION)
>> - panic("Jailhouse: Unsupported setup data structure");
>> + /* setup data must at least contain the header */
>> + if (header.len < sizeof(setup_data.hdr))
>> + goto unsupported;
>> - pmtmr_ioport = setup_data.pm_timer_address;
>> + pa_data += offsetof(struct setup_data, data);
>> + setup_data_len = min(sizeof(setup_data), (unsigned long)header.len);
>> + mapping = early_memremap(pa_data, setup_data_len);
>> + memcpy(&setup_data.hdr, mapping, setup_data_len);
>
> This is now nitpicking, but was there a particular reason to target
> setup_data.hdr with the copy, rather than just setup_data?
Hmm, not really. Must be a fragment of a earlier version of the patch.
Can be changed to &setup_data.
Ralf
>
>> + early_memunmap(mapping, setup_data_len);
>> +
>> + if (setup_data.hdr.version == 0 ||
>> + setup_data.hdr.compatible_version !=
>> + JAILHOUSE_SETUP_REQUIRED_VERSION ||
>> + (setup_data.hdr.version >= 1 && header.len < SETUP_DATA_V1_LEN))
>> + goto unsupported;
>> +
>> + pmtmr_ioport = setup_data.v1.pm_timer_address;
>> pr_debug("Jailhouse: PM-Timer IO Port: %#x\n", pmtmr_ioport);
>> - precalibrated_tsc_khz = setup_data.tsc_khz;
>> + precalibrated_tsc_khz = setup_data.v1.tsc_khz;
>> setup_force_cpu_cap(X86_FEATURE_TSC_KNOWN_FREQ);
>> pci_probe = 0;
>> @@ -195,6 +203,10 @@ static void __init jailhouse_init_platform(void)
>> * are none in a non-root cell.
>> */
>> disable_acpi();
>> + return;
>> +
>> +unsupported:
>> + panic("Jailhouse: Unsupported setup data structure");
>> }
>> bool jailhouse_paravirt(void)
>>
>
> In any case:
>
> Reviewed-by: Jan Kiszka <jan.kis...@siemens.com>
>
> Jan
>
--
You received this message because you are subscribed to the Google Groups
"Jailhouse" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to jailhouse-dev+unsubscr...@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/jailhouse-dev/b7b1ee8b-5363-0f9c-0419-357f042edcc5%40oth-regensburg.de.
