On 8/2/19 2:48 PM, Jan Kiszka wrote:
> On 02.08.19 14:33, Ralf Ramsauer wrote:
>> ACPI tables aren't available if Linux runs as guest of the hypervisor
>> Jailhouse. This makes the 8250 driver probe for all platform UARTs as
>> it assumes that all platform are present in case of !ACPI. Jailhouse
>> will stop execution of Linux guest due to port access violation.
>>
>> So far, these access violations could be solved by tuning the
>> 8250.nr_uarts parameter but it has limitations: We can, e.g., only map
>> consecutive platform UARTs to Linux, and only in the sequence 0x3f8,
>> 0x2f8, 0x3e8, 0x2e8.
>>
>> Beginning from setup_data version 2, Jailhouse will place information of
>> available platform UARTs in setup_data. This allows for selective
>> activation of platform UARTs.
>>
>> This patch queries the setup_data version and activates only available
>> UARTS. It comes with backward compatibility, and will still support
>> older setup_data versions. In this case, Linux falls back to the old
>> behaviour.
>>
>> Signed-off-by: Ralf Ramsauer <[email protected]>
>> ---
>> arch/x86/include/uapi/asm/bootparam.h | 1 +
>> arch/x86/kernel/jailhouse.c | 74 ++++++++++++++++++++++++---
>> 2 files changed, 69 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/x86/include/uapi/asm/bootparam.h
>> b/arch/x86/include/uapi/asm/bootparam.h
>> index a06cbf019744..2d6a40cbf3df 100644
>> --- a/arch/x86/include/uapi/asm/bootparam.h
>> +++ b/arch/x86/include/uapi/asm/bootparam.h
>> @@ -146,6 +146,7 @@ struct jailhouse_setup_data {
>> __u32 apic_khz;
>> __u8 standard_ioapic;
>> __u8 cpu_ids[255];
>> + __u32 flags;
>> } __attribute__((packed));
>>
>> /* The so-called "zeropage" */
>> diff --git a/arch/x86/kernel/jailhouse.c b/arch/x86/kernel/jailhouse.c
>> index 108c48d0d40e..3b73647b2456 100644
>> --- a/arch/x86/kernel/jailhouse.c
>> +++ b/arch/x86/kernel/jailhouse.c
>> @@ -11,6 +11,7 @@
>> #include <linux/acpi_pmtmr.h>
>> #include <linux/kernel.h>
>> #include <linux/reboot.h>
>> +#include <linux/serial_8250.h>
>> #include <asm/apic.h>
>> #include <asm/cpu.h>
>> #include <asm/hypervisor.h>
>> @@ -20,6 +21,10 @@
>> #include <asm/reboot.h>
>> #include <asm/setup.h>
>>
>> +#define SETUP_DATA_FLAGS_PERMIT_PCUART(n) (1 << (n))
>> +#define SETUP_DATA_FLAGS_HAS_PCUART(flags, n) \
>> + !!(flags & SETUP_DATA_FLAGS_PERMIT_PCUART(n))
>> +
>> static __initdata struct jailhouse_setup_data setup_data;
>> static unsigned int precalibrated_tsc_khz;
>>
>> @@ -76,11 +81,13 @@ static void __init jailhouse_get_smp_config(unsigned int
>> early)
>> .type = IOAPIC_DOMAIN_STRICT,
>> .ops = &mp_ioapic_irqdomain_ops,
>> };
>> +#ifdef CONFIG_SERIAL_8250
>> struct mpc_intsrc mp_irq = {
>> .type = MP_INTSRC,
>> .irqtype = mp_INT,
>> .irqflag = MP_IRQPOL_ACTIVE_HIGH | MP_IRQTRIG_EDGE,
>> };
>> +#endif
>> unsigned int cpu;
>>
>> jailhouse_x2apic_init();
>> @@ -97,12 +104,16 @@ static void __init jailhouse_get_smp_config(unsigned
>> int early)
>> if (setup_data.standard_ioapic) {
>> mp_register_ioapic(0, 0xfec00000, gsi_top, &ioapic_cfg);
>>
>> - /* Register 1:1 mapping for legacy UART IRQs 3 and 4 */
>> - mp_irq.srcbusirq = mp_irq.dstirq = 3;
>> - mp_save_irq(&mp_irq);
>> +#ifdef CONFIG_SERIAL_8250
>> + if (setup_data.version < 2) {
>> + /* Register 1:1 mapping for legacy UART IRQs 3 and 4 */
>> + mp_irq.srcbusirq = mp_irq.dstirq = 3;
>> + mp_save_irq(&mp_irq);
>>
>> - mp_irq.srcbusirq = mp_irq.dstirq = 4;
>> - mp_save_irq(&mp_irq);
>> + mp_irq.srcbusirq = mp_irq.dstirq = 4;
>> + mp_save_irq(&mp_irq);
>> + }
>> +#endif
>> }
>> }
>>
>> @@ -135,6 +146,42 @@ static int __init jailhouse_pci_arch_init(void)
>> return 0;
>> }
>>
>> +#ifdef CONFIG_SERIAL_8250
>> +static const u16 pcuart_base[] = {
>> + 0x3f8,
>> + 0x2f8,
>> + 0x3e8,
>> + 0x2e8,
>> +};
>> +
>> +static void jailhouse_serial_fixup(int port, struct uart_port *up,
>> + u32 *capabilites)
>> +{
>> + struct mpc_intsrc mp_irq = {
>> + .type = MP_INTSRC,
>> + .irqtype = mp_INT,
>> + .irqflag = MP_IRQPOL_ACTIVE_HIGH | MP_IRQTRIG_EDGE,
>> + };
>> + unsigned int n;
>> +
>> + for (n = 0; n < ARRAY_SIZE(pcuart_base); n++) {
>> + if (pcuart_base[n] != up->iobase)
>> + continue;
>> +
>> + if (SETUP_DATA_FLAGS_HAS_PCUART(setup_data.flags, n)) {
>> + pr_info("Enabling UART%u (port 0x%lx)\n", n,
>> + up->iobase);
>> + mp_irq.srcbusirq = mp_irq.dstirq = up->irq;
>> + mp_save_irq(&mp_irq);
>> + } else {
>> + /* Deactivate UART if access isn't allowed */
>> + up->iobase = 0;
>> + }
>> + break;
>> + }
>> +}
>> +#endif
>> +
>> static void __init jailhouse_init_platform(void)
>> {
>> u64 pa_data = boot_params.hdr.setup_data;
>> @@ -162,8 +209,12 @@ static void __init jailhouse_init_platform(void)
>> memcpy(&header, mapping, sizeof(header));
>> early_memunmap(mapping, sizeof(header));
>>
>> + /*
>> + * Version 2 extends version 1 by four byte. We still do
>> + * support version 1.
>> + */
>> if (header.type == SETUP_JAILHOUSE &&
>> - header.len >= sizeof(setup_data)) {
>> + header.len >= (sizeof(setup_data) - 4)) {
>
> Hmm, looking at this again, maybe it's better to have struct setup_data_v1
> inside setup_data, instead of a magic "- 4".
Thanks for mentioning. But that's probably not enough. From now on, the
size we copy from setup_data depends on the version. I forgot to respect
that.
And this eventually brings me to the point that we need proper version
handling.
>
>> pa_data += offsetof(struct setup_data, data);
>>
>> mapping = early_memremap(pa_data, sizeof(setup_data));
>> @@ -195,6 +246,17 @@ static void __init jailhouse_init_platform(void)
>> * are none in a non-root cell.
>> */
>> disable_acpi();
>> +
>> +#ifdef CONFIG_SERIAL_8250
>> + /* Since setup_data.version 2 we have flags that indicate availability
>> + * of platform UARTs.
>> + *
>> + * In case of version 1, we don't know which UARTs belong Linux. In
>> + * this case, unconditionally register 1:1 mapping for legacy UART IRQs
>> + * 3 and 4 */
>> + if (setup_data.version > 1)
>> + serial8250_set_isa_configurator(jailhouse_serial_fixup);
>> +#endif
>> }
>>
>> bool jailhouse_paravirt(void)
>>
>
> When addressing the above, you can add my
>
> Reviewed-by: Jan Kiszka <[email protected]>
Thanks, but I guess this will now become two patches. The first one for
proper version handling, the second one that implements setup_data
version 2.
Logically, the second patch will stay the same, but there's probably too
much noise from the first patch to keep your Reviewed-by.
Ralf
--
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 [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/jailhouse-dev/d08c2103-f24b-4dd4-2f21-aec6fd068921%40oth-regensburg.de.
