On 27/01/2020 14:39, Jan Kiszka wrote: > On 27.01.20 13:15, Jan Kiszka wrote: >> On 27.01.20 12:40, Ralf Ramsauer wrote: >>> >>> >>> On 27/01/2020 07:37, Jan Kiszka wrote: >>>> From: Jan Kiszka <[email protected]> >>>> >>>> Better pad than rely on both sides using the same compiler logic. >>> >>> Ack. But shouldn't we then, in turn, use __attribute__((unpacked)) to >>> avoid that the compiler accidentally does some other unintended >>> alignment / reordering? >> >> Do you mean "packed"? It shouldn't be needed at this stage, but it >> shouldn't harm as well. >> >>> >>>> >>>> Signed-off-by: Jan Kiszka <[email protected]> >>>> --- >>>> include/arch/arm-common/asm/jailhouse_hypercall.h | 1 + >>>> 1 file changed, 1 insertion(+) >>>> >>>> diff --git a/include/arch/arm-common/asm/jailhouse_hypercall.h >>>> b/include/arch/arm-common/asm/jailhouse_hypercall.h >>>> index 83cec97b..aeab2816 100644 >>>> --- a/include/arch/arm-common/asm/jailhouse_hypercall.h >>>> +++ b/include/arch/arm-common/asm/jailhouse_hypercall.h >>>> @@ -38,6 +38,7 @@ >>>> >>>> #define COMM_REGION_COMMON_PLATFORM_INFO \ >>>> __u8 gic_version; \ >>>> + __u8 padding[7]; \ >>>> __u64 gicd_base; \ >>>> __u64 gicc_base; \ >>>> __u64 gicr_base; \ >>> >>> BTW: It's really hard to trace how the structures are being defined. >>> >>> Instead of creating the structure in arch-specific parts for each >>> architecture, I think it would be nicer to introduce the structure for >>> all architectures, and then include arch-specific parts. >> >> ...as anonymous sub-structs - possibly. > > Not that easy: The anonymous struct thing does not work, and the doxygen > documentation would have to be refactored as well.
Ack, just realised that as well. Have a look at the attachment. If you agree on this idea, I'll make a proper patch out of it. 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/80ed86e4-9082-42ad-b38d-7f97d440076e%40oth-regensburg.de.
>From e0a5d08e6cd3044f52fc9a4401b761906cecd38e Mon Sep 17 00:00:00 2001 From: Ralf Ramsauer <[email protected]> Date: Mon, 27 Jan 2020 15:08:50 +0100 Subject: [PATCH] RFC: core: simplify definition of comm region Keeping everything at one place (as we do it in for cell-config.h) makes the definition of the ABI much more readable. --- .../arch/arm-common/asm/jailhouse_hypercall.h | 7 --- include/arch/arm/asm/jailhouse_hypercall.h | 5 -- include/arch/arm64/asm/jailhouse_hypercall.h | 5 -- include/arch/x86/asm/jailhouse_hypercall.h | 15 ----- include/jailhouse/hypercall.h | 62 ++++++++++++++----- 5 files changed, 46 insertions(+), 48 deletions(-) diff --git a/include/arch/arm-common/asm/jailhouse_hypercall.h b/include/arch/arm-common/asm/jailhouse_hypercall.h index aeab2816..0da658dd 100644 --- a/include/arch/arm-common/asm/jailhouse_hypercall.h +++ b/include/arch/arm-common/asm/jailhouse_hypercall.h @@ -36,10 +36,3 @@ * THE POSSIBILITY OF SUCH DAMAGE. */ -#define COMM_REGION_COMMON_PLATFORM_INFO \ - __u8 gic_version; \ - __u8 padding[7]; \ - __u64 gicd_base; \ - __u64 gicc_base; \ - __u64 gicr_base; \ - __u32 vpci_irq_base; diff --git a/include/arch/arm/asm/jailhouse_hypercall.h b/include/arch/arm/asm/jailhouse_hypercall.h index 275d4891..f943145d 100644 --- a/include/arch/arm/asm/jailhouse_hypercall.h +++ b/include/arch/arm/asm/jailhouse_hypercall.h @@ -57,11 +57,6 @@ #ifndef __ASSEMBLY__ -struct jailhouse_comm_region { - COMM_REGION_GENERIC_HEADER; - COMM_REGION_COMMON_PLATFORM_INFO; -} __attribute__((packed)); - static inline __u32 jailhouse_call(__u32 num) { register __u32 num_result asm(JAILHOUSE_CALL_NUM_RESULT) = num; diff --git a/include/arch/arm64/asm/jailhouse_hypercall.h b/include/arch/arm64/asm/jailhouse_hypercall.h index 9daa21fe..c12b9262 100644 --- a/include/arch/arm64/asm/jailhouse_hypercall.h +++ b/include/arch/arm64/asm/jailhouse_hypercall.h @@ -55,11 +55,6 @@ #ifndef __ASSEMBLY__ -struct jailhouse_comm_region { - COMM_REGION_GENERIC_HEADER; - COMM_REGION_COMMON_PLATFORM_INFO; -} __attribute__((packed)); - static inline __u64 jailhouse_call(__u64 num) { register __u64 num_result asm(JAILHOUSE_CALL_NUM_RESULT) = num; diff --git a/include/arch/x86/asm/jailhouse_hypercall.h b/include/arch/x86/asm/jailhouse_hypercall.h index 8dffb5c0..aaac5100 100644 --- a/include/arch/x86/asm/jailhouse_hypercall.h +++ b/include/arch/x86/asm/jailhouse_hypercall.h @@ -95,21 +95,6 @@ extern bool jailhouse_use_vmcall; #include <jailhouse/hypercall.h> #endif -/** Communication region between hypervisor and a cell. */ -struct jailhouse_comm_region { - COMM_REGION_GENERIC_HEADER; - - /** I/O port address of the PM timer (x86-specific). */ - __u16 pm_timer_address; - /** Number of CPUs available to the cell (x86-specific). */ - __u16 num_cpus; - /** Calibrated TSC frequency in kHz (x86-specific). */ - __u32 tsc_khz; - /** Calibrated APIC timer frequency in kHz or 0 if TSC deadline timer - * is available (x86-specific). */ - __u32 apic_khz; -} __attribute__((packed)); - /** * Invoke a hypervisor without additional arguments. * @param num Hypercall number. diff --git a/include/jailhouse/hypercall.h b/include/jailhouse/hypercall.h index 07574d3d..db6f89c1 100644 --- a/include/jailhouse/hypercall.h +++ b/include/jailhouse/hypercall.h @@ -105,24 +105,54 @@ #define COMM_REGION_ABI_REVISION 2 #define COMM_REGION_MAGIC "JHCOMM" -#define COMM_REGION_GENERIC_HEADER \ - /** Communication region magic JHCOMM */ \ - char signature[6]; \ - /** Communication region ABI revision */ \ - __u16 revision; \ - /** Cell state, initialized by hypervisor, updated by cell. */ \ - volatile __u32 cell_state; \ - /** Message code sent from hypervisor to cell. */ \ - volatile __u32 msg_to_cell; \ - /** Reply code sent from cell to hypervisor. */ \ - volatile __u32 reply_from_cell; \ - /** Holds static flags, see JAILHOUSE_COMM_FLAG_*. */ \ - __u32 flags; \ - /** Debug console that may be accessed by the inmate. */ \ - struct jailhouse_console console; \ - /** Base address of PCI memory mapped config. */ \ +/** Communication region between hypervisor and a cell. */ +struct jailhouse_comm_region { + /* Generic Platform Information */ + + /** Communication region magic JHCOMM */ + char signature[6]; + /** Communication region ABI revision */ + __u16 revision; + /** Cell state, initialized by hypervisor, updated by cell. */ + volatile __u32 cell_state; + /** Message code sent from hypervisor to cell. */ + volatile __u32 msg_to_cell; + /** Reply code sent from cell to hypervisor. */ + volatile __u32 reply_from_cell; + /** Holds static flags, see JAILHOUSE_COMM_FLAG_*. */ + __u32 flags; + /** Debug console that may be accessed by the inmate. */ + struct jailhouse_console console; + /** Base address of PCI memory mapped config. */ __u64 pci_mmconfig_base; + /* Architecture specific platform information */ + union { + /* x86 specific platform information */ + struct { + /** I/O port address of the PM timer (x86-specific). */ + __u16 pm_timer_address; + /** Number of CPUs available to the cell (x86-specific). */ + __u16 num_cpus; + /** Calibrated TSC frequency in kHz (x86-specific). */ + __u32 tsc_khz; + /** Calibrated APIC timer frequency in kHz or 0 if TSC deadline timer + * is available (x86-specific). */ + __u32 apic_khz; + }; + + /* ARMv7 and ARMv8 specific platform information */ + struct { + __u8 gic_version; + __u8 padding[7]; + __u64 gicd_base; + __u64 gicc_base; + __u64 gicr_base; + __u32 vpci_irq_base; + }; + }; +} __attribute__((packed)); + #include <asm/jailhouse_hypercall.h> #endif /* !_JAILHOUSE_HYPERCALL_H */ -- 2.25.0
