On 2017-01-16 19:04, Ralf Ramsauer wrote: > On 01/16/2017 05:48 PM, Jan Kiszka wrote: >> On 2017-01-16 17:40, Jan Kiszka wrote: >>> On 2017-01-16 17:07, Ralf Ramsauer wrote: >>>> The console page is implemented as ring buffer, and will _not_ contain a >>>> trailing \0 for string termination. >>>> >>>> We're changing the semantics of the configuration: increment revision >>>> counter. >>>> >>>> Signed-off-by: Ralf Ramsauer <[email protected]> >>>> --- >>>> hypervisor/hypervisor.lds.S | 6 ++++++ >>>> hypervisor/include/jailhouse/cell-config.h | 13 +++++++++---- >>>> hypervisor/include/jailhouse/header.h | 8 ++++++++ >>>> hypervisor/include/jailhouse/printk.h | 2 ++ >>>> hypervisor/printk.c | 2 ++ >>>> tools/jailhouse-cell-linux | 2 +- >>>> tools/jailhouse-hardware-check | 2 +- >>>> 7 files changed, 29 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/hypervisor/hypervisor.lds.S b/hypervisor/hypervisor.lds.S >>>> index 09b1a0b2..f8178282 100644 >>>> --- a/hypervisor/hypervisor.lds.S >>>> +++ b/hypervisor/hypervisor.lds.S >>>> @@ -35,6 +35,12 @@ SECTIONS >>>> . = ALIGN(16); >>>> .bss : { *(.bss) } >>>> >>>> + /* The console section shall only contain the hypervisor console. This >>>> + * section and the next section must be aligned to PAGE_SIZE, as we >>>> + * want the console section to reserve a whole page */ >>>> + . = ALIGN(PAGE_SIZE); >>>> + .console : { *(.console) } >>>> + >>>> . = ALIGN(PAGE_SIZE); >>>> __page_pool = .; >>>> >>>> diff --git a/hypervisor/include/jailhouse/cell-config.h >>>> b/hypervisor/include/jailhouse/cell-config.h >>>> index 80fa5a78..ad894cb6 100644 >>>> --- a/hypervisor/include/jailhouse/cell-config.h >>>> +++ b/hypervisor/include/jailhouse/cell-config.h >>>> @@ -40,7 +40,7 @@ >>>> #define _JAILHOUSE_CELL_CONFIG_H >>>> >>>> /* Incremented on any layout or semantic change of system or cell config. >>>> */ >>>> -#define JAILHOUSE_CONFIG_REVISION 3 >>>> +#define JAILHOUSE_CONFIG_REVISION 4 >>>> >>>> #define JAILHOUSE_CELL_NAME_MAXLEN 31 >>>> >>>> @@ -186,9 +186,14 @@ struct jailhouse_iommu { >>>> >>>> #define CON1_TYPE(flags) ((flags) & JAILHOUSE_CON1_TYPE_MASK) >>>> >>>> -/* We use bit 4..5 to differentiate between PIO and MMIO access */ >>>> -#define JAILHOUSE_CON1_FLAG_PIO 0x0010 >>>> -#define JAILHOUSE_CON1_FLAG_MMIO 0x0020 >>>> +#define JAILHOUSE_CON2_TYPE_ROOTPAGE 0x0010 >>>> +#define JAILHOUSE_CON2_TYPE_MASK 0x00f0 >>>> + >>>> +#define CON2_TYPE(flags) ((flags) & JAILHOUSE_CON2_TYPE_MASK) >>>> + >>>> +/* We use bit 8..9 to differentiate between PIO and MMIO access */ >>>> +#define JAILHOUSE_CON1_FLAG_PIO 0x0100 >>>> +#define JAILHOUSE_CON1_FLAG_MMIO 0x0200 >>> >>> If you do not shift these flags around, there is also no need for the >>> revision bump. Actually, it would make at least equal sense to keep all >>> CON1 related bits together, maybe moving the CON2 related bits up to 16..31. >>> >>>> >>>> #define CON1_IS_MMIO(flags) ((flags) & JAILHOUSE_CON1_FLAG_MMIO) >>>> >>>> diff --git a/hypervisor/include/jailhouse/header.h >>>> b/hypervisor/include/jailhouse/header.h >>>> index 4fe159c6..072a37b2 100644 >>>> --- a/hypervisor/include/jailhouse/header.h >>>> +++ b/hypervisor/include/jailhouse/header.h >>>> @@ -24,6 +24,14 @@ >>>> */ >>>> typedef int (*jailhouse_entry)(unsigned int); >>>> >>>> +#define CONSOLE_SIZE 0x1000 >>> >>> This is no longer the actual size, is it? >>> >>>> + >>>> +struct jailhouse_console { >>>> + volatile unsigned int lock; >>>> + unsigned int tail; >> >> Oh, and tail is volatile as well, isn't it? > This is the code where I copy the console page: > > +static void copy_console_page(struct jailhouse_console *dst) > +{ > +retry: > + /* spin while hypervisor is writing to console */ > + while (console_page->lock); > + /* copy console page */ > + memcpy(dst, console_page, sizeof(struct jailhouse_console)); > + /* check consistency of console_page */ > + if (dst->tail != console_page->tail) > + goto retry; > +} > > I'm spinning on lock, copy the whole page and comparing tails. So is it > really necessary that the tail gets volatile? But sure, it won't hurt, > if it is volatile.
The avoid is not using any compiler barrier (typically via cpu_relax, which is a good idea anyway), and then the volatile is definitely missing because the compiler could theoretically get the idea to read console_page-> tail only once. With barriers, the volatile is not strictly required, right. Jan -- Siemens AG, Corporate Technology, CT RDA ITP SES-DE Corporate Competence Center Embedded Linux -- 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]. For more options, visit https://groups.google.com/d/optout.
