Re: [Xen-devel] [PATCH v4 19/27] x86: assembly, make some functions local

[Mark Rutland]

On Wed, Oct 25, 2017 at 04:21:48PM +0200, Jiri Slaby wrote:
> Hi,
> 
> On 10/06/2017, 03:21 PM, Mark Rutland wrote:
> > If the aim of this series is to introduce something that architectures
> > use consistently, then can we please actually poke other architectures
> > about it? e.g. send this to linux-arch, with a cover letter explaining
> > the idea and asking maintainers to take a look.
> 
> Sure, with v5, I will.

Thanks; that's much appreciated, and I look forward to it!

Mark.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v4 19/27] x86: assembly, make some functions local

[Mark Rutland]

Hi Jiri,

I can see that these serve a useful purpose (as they are necessary for
asm validation encessary for livepatching), and I am not personally
averse to the new annotations.

However, I am concerned that as-is, this is going to create more
problems for !x86 architectures. More on that below.

On Fri, Oct 06, 2017 at 02:53:08PM +0200, Jiri Slaby wrote:
> On 10/04/2017, 09:33 AM, Ard Biesheuvel wrote:
> > On 4 October 2017 at 08:22, Jiri Slaby  wrote:
> >> On 10/02/2017, 02:48 PM, Ard Biesheuvel wrote:
> >>> On 2 October 2017 at 10:12, Jiri Slaby  wrote:
>  There is a couple of assembly functions, which are invoked only locally
>  in the file they are defined. In C, we mark them "static". In assembly,
>  annotate them using SYM_{FUNC,CODE}_START_LOCAL (and switch their
>  ENDPROC to SYM_{FUNC,CODE}_END too). Whether FUNC or CODE depends on
>  ENDPROC/END for a particular function (C or non-C).
> >>>
> >>> I wasn't cc'ed on the cover letter, so I am missing the rationale of
> >>> replacing ENTRY/ENDPROC with other macros.
> >>
> >> There was no cover letter. I am attaching what is in PATCH 1/27 instead:
> >> Introduce new C macros for annotations of functions and data in
> >> assembly. There is a long-standing mess in macros like ENTRY, END,
> >> ENDPROC and similar. They are used in different manners and sometimes
> >> incorrectly.
> > 
> > I must say, I don't share this sentiment.
> > 
> > In arm64, we use ENTRY/ENDPROC for functions with external linkage,
> > and the bare symbol name/ENDPROC for functions with local linkage. I
> > guess we could add ENDOBJECT if we wanted to, but we never really felt
> > the need.
> 
> Yes and this is exactly the reason for the new macros. Now, it's a
> complete mess. One arch does this, another does that. 

If the aim of this series is to introduce something that architectures
use consistently, then can we please actually poke other architectures
about it? e.g. send this to linux-arch, with a cover letter explaining
the idea and asking maintainers to take a look.

I think one reason that ENTRY/END/ENDPROC are used inconsistently is
that they're insufficiently documented. So people assume (inconsistent)
semantics, and often cargo-cult usage without thinking. To avoid that,
could we please document how these new macros should (and should not) be
used?

That way, we have a much better chance of consistency, and it's easier
to figure out if the intended semantics are necessary/sufficient.

Otherwise, I'm worried that bits of this will be inconsistently and
incorrectly cargo-culted into other architectures, making matters worse.

Thanks,
Mark.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] Duplicated memory node in the Device-Tree (WAS [XEN] Re: Duplicated memory nodes cause the BUG())

[Mark Rutland]

On Tue, Jul 25, 2017 at 06:27:31PM +0300, Andrii Anisov wrote:
> On 25.07.17 17:23, Julien Grall wrote:
> >I think this is by chance rather than by design. The first
> >question to answer is why a Firmware would specify twice the same
> >memory banks? Is it valid from the specification?
> Yep, that is the question.

I beleive that all memory regions described in any memory node's reg
entries should be disjoint (i.e. should not overlap). Per IEEE-1275, reg
entries (within a node) are supposed to be disjoint, and I would expect
this requirement to extend across nodes in the case of memory nodes.

Currently, the DT spec is somewhat sparse in wording, but this can be
corrected.

> >Regardless that, it looks like to me that the device-tree you give
> >to the board should not contain the memory nodes.
> The device-tree is provided by vendor in that form, and u-boot is
> theirs. It seems to me that they do not really care since the kernel
> tolerates duplication.
> 
> >> ps. Linux kernel does tolerate duplicated memory nodes by
> >>merging memory blocks. I.e. memblock_add_range() function is
> >>commented as following:
> >>/**
> >> * memblock_add_range - add new memblock region
> >> * @type: memblock type to add new region into
> >> * @base: base address of the new region
> >> * @size: size of the new region
> >> * @nid: nid of the new region
> >> * @flags: flags of the new region
> >> *
> >> * Add new memblock region [@base,@base+@size) into @type. The new
> >>region
> >> * is allowed to overlap with existing ones - overlaps don't affect
> >>already
> >> * existing regions.  @type is guaranteed to be minimal (all
> >>neighbouring
> >> * compatible regions are merged) after the addition.
> >> *
> >> * RETURNS:
> >> * 0 on success, -errno on failure.
> >> */
> IMO the function description is pretty straight-forward.
> But let us wait for device tree guys feedback.

This might be the designed of *memblock*, but that does not mean that it
is a deliberate part of the DT handling. I beleive this is simply an
implementation detail that happens to mask a DT bug.

Thanks,
Mark.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] arm64: xen: Implement EFI reset_system callback

[Mark Rutland]

[Adding the EFI maintainers]

Tl;DR: Xen's EFI wrappery doesn't implement reset_system, so when
invoked on arm64 we get a NULL dereference.

On Thu, Apr 06, 2017 at 04:39:13PM +0100, Julien Grall wrote:
> On 06/04/17 16:20, Daniel Kiper wrote:
> >On Thu, Apr 06, 2017 at 04:38:24PM +0200, Juergen Gross wrote:
> >>On 06/04/17 16:27, Daniel Kiper wrote:
> >>>On Thu, Apr 06, 2017 at 09:32:32AM +0100, Julien Grall wrote:
> Hi Juergen,
> 
> On 06/04/17 07:23, Juergen Gross wrote:
> >On 05/04/17 21:49, Boris Ostrovsky wrote:
> >>On 04/05/2017 02:14 PM, Julien Grall wrote:
> >>>The x86 code has theoritically a similar issue, altought EFI does not
> >>>seem to be the preferred method. I have left it unimplemented on x86 
> >>>and
> >>>CCed Linux Xen x86 maintainers to know their view here.
> >>
> >>(+Daniel)
> >>
> >>This could be a problem for x86 as well, at least theoretically.
> >>xen_machine_power_off() may call pm_power_off(), which is 
> >>efi.reset_system.
> >>
> >>So I think we should have a similar routine there.
> >
> >+1
> >
> >I don't see any problem with such a routine added, in contrast to
> >potential "reboots" instead of power off without it.
> >
> >So I think this dummy xen_efi_reset_system() should be added to
> >drivers/xen/efi.c instead.
> 
> I will resend the patch during day with xen_efi_reset_system moved
> to common code and implement the x86 counterpart (thought, I will
> not be able to test it).
> >>>
> >>>I think that this is ARM specific issue. On x86 machine_restart() calls
> >>>xen_restart(). Hence, everything works. So, I think that it should be
> >>>fixed only for ARM. Anyway, please CC me when you send a patch.
> >>
> >>What about xen_machine_power_off() (as stated in Boris' mail)?
> >
> >Guys what do you think about that:
> >
> >--- a/drivers/firmware/efi/reboot.c
> >+++ b/drivers/firmware/efi/reboot.c
> >@@ -55,7 +55,7 @@ static void efi_power_off(void)
> >
> > static int __init efi_shutdown_init(void)
> > {
> >-   if (!efi_enabled(EFI_RUNTIME_SERVICES))
> >+   if (!efi_enabled(EFI_RUNTIME_SERVICES) || efi_enabled(EFI_PARAVIRT))
> >return -ENODEV;
> >
> >if (efi_poweroff_required())
> >
> >
> >Julien, for ARM64 please take a look at 
> >arch/arm64/kernel/efi.c:efi_poweroff_required(void).
> >
> >I hope that tweaks for both files should solve our problem.
> 
> This sounds good for power off (I haven't tried to power off DOM0
> yet).

Please, let's keep the Xen knowledge constrained to the Xen EFI wrapper,
rather than spreading it further.

IMO, given reset_system is a *mandatory* function, the Xen wrapper
should provide an implementation.

I don't see why you can't implement a wrapper that calls the usual Xen
poweroff/reset functions.

Thanks,
Mark.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH V8 1/3] irq: Add flags to request_percpu_irq function

[Mark Rutland]

Hi Daniel,

On Thu, Mar 23, 2017 at 06:42:01PM +0100, Daniel Lezcano wrote:
> In the next changes, we track the interrupts but we discard the timers as
> that does not make sense. The next interrupt on a timer is predictable.

Sorry, but I could not parse this. 

[...]

> diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
> index 9612b84..0f5ab4a 100644
> --- a/drivers/perf/arm_pmu.c
> +++ b/drivers/perf/arm_pmu.c
> @@ -661,7 +661,7 @@ static int cpu_pmu_request_irq(struct arm_pmu *cpu_pmu, 
> irq_handler_t handler)
>  
>   irq = platform_get_irq(pmu_device, 0);
>   if (irq > 0 && irq_is_percpu(irq)) {
> - err = request_percpu_irq(irq, handler, "arm-pmu",
> + err = request_percpu_irq(irq, 0, handler, "arm-pmu",
>_events->percpu_pmu);
>   if (err) {
>   pr_err("unable to request IRQ%d for ARM PMU counters\n",

Please Cc myself and Will Deacon when modifying the arm_pmu driver, as
per MAINTAINERS. I only spotted this patch by chance.

This conflicts with arm_pmu changes I have queued for v4.12 [1].

So, can we leave the prototype of request_percpu_irq() as-is?

Why not add a new request_percpu_irq_flags() function, and leave
request_percpu_irq() as a wrapper for that? e.g.

static inline int
request_percpu_irq(unsigned int irq, irq_handler_t handler,
   const char *devname, void __percpu *percpu_dev_id)
{
return request_percpu_irq_flags(irq, handler, devname,
percpu_dev_id, 0);
}

... that would avoid having to touch any non-timer driver for now.

[...]

> -request_percpu_irq(unsigned int irq, irq_handler_t handler,
> -const char *devname, void __percpu *percpu_dev_id);
> +request_percpu_irq(unsigned int irq, unsigned long flags,
> +irq_handler_t handler,  const char *devname,
> +void __percpu *percpu_dev_id);
>  

Looking at request_irq, the prototype is:

int __must_check
request_irq(unsigned int irq, irq_handler_t handler,
unsigned long flags, const char *name,
void *dev);

... surely it would be better to share the same argument order? i.e.

int __must_check
request_percpu_irq(unsigned int irq, irq_handler_t handler,
   unsigned long flags, const char *devname,
   void __percpu *percpu_dev_id);

Thanks,
Mark.

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=arm/perf/refactoring

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 02/18] xen/arm: Restore HCR_EL2 register

[Mark Rutland]

On Wed, Mar 22, 2017 at 10:54:11AM -0700, Stefano Stabellini wrote:
> When we receive an SError in Xen, we determine if it should be injected
> into the guest or "handled" in Xen (by "handle" I mean crash the
> system). In case it should be injected into the guest, we set the
> relevant bit in vcpu->arch.hcr_el2 (the saved version of HCR_EL2). This
> patch moves the WRITE(vcpu->arch.hcr_el2, HCR_EL2) from context switch
> related functions (p2m_restore_state) to leave_hypervisor_tail, which is
> the last point we can move it to, before exiting Xen. That way, we are
> sure to inject an abort into the guest, no matter exactly when we
> receive the SError. So far so good, right?

I assume you're in the context of the guest at this point, so what's the
problem with doing:

vcpu->arch.hcr_el2 |= HCR_VA;
WRITE(vcpu->arch.hcr_el2, HCR_EL2);

That way you don't need to move the restoration of HCR_EL2, and it workd
regardless of whether you get preempted, at the cost of a potentially
redundant system register write on what should be an incredibly rare
path...

Within Linux we do similar when we manipulate system registers which are
context switched with the thread, e.g. when setting tpidrro_el0 in
tls_thread_flush() [1].

Surely similar applies for the manipulation of other system registers
while in the guest context?

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/kernel/process.c?h=v4.11-rc3#n217

Thanks,
Mark.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 02/18] xen/arm: Restore HCR_EL2 register

[Mark Rutland]

On Wed, Mar 22, 2017 at 12:16:20PM +, Julien Grall wrote:
> (CC Mark for the TLB question)

[Adding Marc since he should understand this better than I do]

I've trimmed a lot of context here, since it wasn't clear if it was
relevant to the question. If there's something I've missed, please point
that out explicitly.

> I am not entirely sure if we can only restore HCR_RW. My concern is
> some of the bits are cached in the TLBs. Looking at the ARM ARM
> (both v7 and v8 see D4.7 in DDI0487A.k_iss10775): "In these
> descriptions, TLB entries for a translation regime for a particular
> Exception level are out of context when executing at a higher
> Exception level.". Which I interpret as TLB result cannot be cached
> when translating a guest VA to guest PA at EL2. So I guess restoring
> only HCR_RW might be fine.

My understanding is that when a translation regime is out-of-context,
the only requirement is that the core does not begin speculative walks
for that translation regime. See ARM DDI 0487A.k_iss10775, page D4-1735,
"Use of out-of-context translation regimes".

I believe that an explicit address translation involving a translation
regime can validly make use of (any part of) that regime's state and/or
allocate new TLB entries for that regime.

It looks like you're asking if you can avoid installing all of the
registers related to the EL1&0 regime when performing an EL1&0
translation at EL2, right?

I do not believe that is valid; my understanding is that all of the
relevant registers need to be installed first.

Thanks,
Mark.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3 0/5] boot-wrapper: arm64: Xen support

[Mark Rutland]

On Thu, Dec 15, 2016 at 12:27:13PM +, Andre Przywara wrote:
> These patches allow to include a Xen hypervisor binary into a boot-wrapper
> ELF file, so that a Foundation Platform or a Fast Model can boot a Xen
> system (including a Dom0 kernel).

Thanks!

I've applied the series and pushed it out to git.kernel.org.

I made minor tweaks to patches 1 and 3 for consistency, as noted in the
commit logs, but neither of these should have a functional impact.

Thanks,
Mark.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3 4/5] Xen: Select correct dom0 console

[Mark Rutland]

On Thu, Dec 15, 2016 at 12:27:17PM +, Andre Przywara wrote:
> From: Ian Campbell 
> 
> If Xen is enabled, tell Dom0 to use the 'hvc0' console, and fall back to
> the usual ttyAMA0 otherwise.
> 
> Signed-off-by: Ian Campbell 
> Signed-off-by: Christoffer Dall 
> Signed-off-by: Andre Przywara 
> Reviewed-by: Julien Grall 
> Tested-by: Konrad Rzeszutek Wilk 
> ---
>  configure.ac | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/configure.ac b/configure.ac
> index ea02dca..d23cced 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -105,7 +105,8 @@ AC_ARG_WITH([initrd],
>  AC_SUBST([FILESYSTEM], [$USE_INITRD])
>  AM_CONDITIONAL([INITRD], [test "x$USE_INITRD" != "x"])
>  
> -C_CMDLINE="console=ttyAMA0 earlyprintk=pl011,0x1c09"
> +AS_IF([test "x$X_IMAGE" = "x"],[C_CONSOLE="ttyAMA0"],[C_CONSOLE="hvc0"])
> +C_CMDLINE="console=$C_CONSOLE earlyprintk=pl011,0x1c09"

Just to check: what happesns if Dom0 tries to write to 0x1c09?

Shouldn't we override/delete earlyprintk/earlycon here too?

I've applied this as-is, so if we do need to, I'll need a fixup patch.

Thanks,
Mark.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 4/4] Explicitly clean linux-system.axf and xen-system.axf

[Mark Rutland]

On Mon, Dec 12, 2016 at 02:50:20PM +, Andre Przywara wrote:
> Hi Konrad,
> 
> On 12/12/16 14:47, Konrad Rzeszutek Wilk wrote:
> > On Tue, Nov 22, 2016 at 03:09:17PM +, Andre Przywara wrote:
> >> From: Christoffer Dall 
> >>
> >> When doing a make clean, only the output image currently configured to
> >> build is being removed.  However, one would expect all build artifacts
> >> to be removed when doing a 'make clean' and when switching between Xen
> >> and Linux builds, it is easy to accidentally run an older build than
> >> intended.  Simply hardcode the axf image file names.
> >>
> >> Signed-off-by: Christoffer Dall 
> >> Signed-off-by: Andre Przywara 
> >> Reviewed-by: Julien Grall 
> > 
> > Tested-by: Konrad Rzeszutek Wilk 
> > Reviewed-by: Konrad Rzeszutek Wilk 
> 
> Thanks a lot!
> 
> I will try to poke Mark to see if we can get this merged eventually.

I'm happy to take these so long as this doesn't adversely affect non-Xen
usage. I assume they don't. :)

Could you fix this up as per Julien's comments, and fold in the tags?
I'm happy to take that from a v3 or a git repo.

Thanks,
Mark.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCHv5 00/11] CONFIG_DEBUG_VIRTUAL for arm64

[Mark Rutland]

On Tue, Dec 06, 2016 at 03:50:46PM -0800, Laura Abbott wrote:
> Hi,
> 
> This is v5 of the series to add CONFIG_DEBUG_VIRTUAL for arm64. This mostly
> contains minor fixups including adding a few extra headers around and 
> splitting
> things out into a few more sub-patches.
> 
> With a few more acks I think this should be ready to go. More testing is
> always appreciated though.

I've given the whole series a go with kasan, kexec, and hibernate (using
test_resume with the disk target), and everything looks happy. So FWIW,
for the series:

Reviewed-by: Mark Rutland <mark.rutl...@arm.com>
Tested-by: Mark Rutland <mark.rutl...@arm.com>

Hopefully this can be queued soon for v4.11!

Thanks,
Mark.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [RFC 21/22] xen/arm: p2m: Re-implement p2m_set_mem_access using p2m_{set, get}_entry

[Mark Rutland]

On Tue, Aug 02, 2016 at 10:58:00AM +0100, Julien Grall wrote:
> On 01/08/2016 19:22, Mark Rutland wrote:
> >On Mon, Aug 01, 2016 at 06:26:54PM +0100, Mark Rutland wrote:
> >>On Mon, Aug 01, 2016 at 05:57:50PM +0100, Julien Grall wrote:
> >>>however we only need one TLBI instruction (assuming there is
> >>>no superpage shattering) per-batch rather than one per-entry in this
> >>>case.
> >>
> >>I got Cc'd to a reply without the original patch context, so I'm not
> >>sure what the case is here. I'm not exactly sure what you mean by
> >>"per-batch".
> 
> Sorry for that. I CCed in case I did not summarize correctly the
> conversation we had.
> 
> The page table handling code can be found in patch #18 [1].

If I've understood, you're asking if you can do a TLBI VMALLE1IS per
batch of leaf entry updates in p2m_set_entry?

As below, if only the AP and/or XN bits are changing, that should be
safe. If any other fields are being altered (inc. the output address,
even for intermediate entries), that may not be safe.

> >>Assuming that you've (only) changed the permissions (i.e. the AP bits
> >>and XN bits) of a number of stage-2 leaf entries, you need either:

> >>* Per entry, a TLBI IPAS2LE1IS
> >>
> >>  e.g.

> >  for_each_entry(x)
> >modify_ap_bits(x);
> >  dsb(ishst);
> >  for_each_entry(x)
> >tlbi(ipas2le1is, x);
> >  dsb(ish);
> 
> I have a question related to this example. Is there a threshold
> where invalidate all the TLB entry for a given VMID/ASID is worth?

Strictly speaking, "yes", but the value is going to depend on
implementation and workload, so there's no "good" value as such provided
by the architecture.

In Linux, we end up doing so in some cases to avoid softlockups. Look
for MAX_TLB_RANGE in arch/arm64/include/asm/tlbflush.h. There are some
more details in commit 05ac65305437e8ef ("arm64: fix soft lockup due to
large tlb flush range").

Thanks,
Mark.

> [1] https://lists.xen.org/archives/html/xen-devel/2016-07/msg02966.html

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [RFC 21/22] xen/arm: p2m: Re-implement p2m_set_mem_access using p2m_{set, get}_entry

[Mark Rutland]

Hi,

I realised I made a confusing mistake in my last reply; clarification below.

On Mon, Aug 01, 2016 at 06:26:54PM +0100, Mark Rutland wrote:
> On Mon, Aug 01, 2016 at 05:57:50PM +0100, Julien Grall wrote:
> > however we only need one TLBI instruction (assuming there is
> > no superpage shattering) per-batch rather than one per-entry in this
> > case.
> 
> I got Cc'd to a reply without the original patch context, so I'm not
> sure what the case is here. I'm not exactly sure what you mean by
> "per-batch".
> 
> Assuming that you've (only) changed the permissions (i.e. the AP bits
> and XN bits) of a number of stage-2 leaf entries, you need either:

[...]

> * Per entry, a TLBI IPAS2LE1IS
> 
>   e.g. 
> 
> for_each_entry(x)
>   modify_ap_bits(x);
> dsb(ishst);
> tlbi(ipas2le1is);
> dsb(ish);

Here I was trying to have the bare minimum barriers necessary, but in focussing
on that I failed to add the required loop to have a TLBI per entry.

The above should have been:

  for_each_entry(x)
modify_ap_bits(x);
  dsb(ishst);
  for_each_entry(x)
tlbi(ipas2le1is, x);
  dsb(ish);

Assuming the necessary bit fiddling for the TLBI to affect the IPA of x, with
the right VMID, etc.

Thanks,
Mark.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [RFC 21/22] xen/arm: p2m: Re-implement p2m_set_mem_access using p2m_{set, get}_entry

[Mark Rutland]

On Mon, Aug 01, 2016 at 05:57:50PM +0100, Julien Grall wrote:
> On 01/08/16 17:34, Mark Rutland wrote:
> >On Mon, Aug 01, 2016 at 04:40:50PM +0100, Julien Grall wrote:
> >>After I read again multiple time the ARM ARM (D4-1732 in ARM DDI
> >>0487A.i) and spoke with various ARM folks, changing the permission
> >>(i.e read, write, execute) does not require the break-before-make
> >>sequence.
> >
> >Regardless of whether Break-Before-Make (BBM) is required, TLB
> >invalidation is still required to ensure the new permissions have taken
> >effect after *any* modification to page tables, unless:
> >
> >* The prior entry was not permitted to be held in a TLB, and:
> >* No TLB held an entry for the address range.
> 
> Agreed, however we only need one TLBI instruction (assuming there is
> no superpage shattering) per-batch rather than one per-entry in this
> case.

I got Cc'd to a reply without the original patch context, so I'm not
sure what the case is here. I'm not exactly sure what you mean by
"per-batch".

Assuming that you've (only) changed the permissions (i.e. the AP bits
and XN bits) of a number of stage-2 leaf entries, you need either:

* A single TLBI VMALLE1IS

  Note that this removes *all* stage-2 or combined stage-1&2 entries
  from TLBs, and thus is stronger than necessary.

* Per entry, a TLBI IPAS2LE1IS

  e.g. 

for_each_entry(x)
  modify_ap_bits(x);
dsb(ishst);
tlbi(ipas2le1is);
dsb(ish);

> >>In the current implementation (i.e without this series), the TLB
> >>invalidation is deferred until the p2m is released. Until that time,
> >>a vCPU may still run with the previous permission and trap into the
> >>hypervisor the permission fault. However, as the permission as
> >>changed, p2m_memaccess_check may fail and we will inject an abort to
> >>the guest.
> >>
> >>The problem is very similar to [1]. This will still be true with
> >>this series applied if I relaxed the use of the break-before-make
> >>sequence. The two ways I can see to fix this are either try again
> >>the instruction (we would trap again if the permission was not
> >>correct) or keep the break-before-make.
> >
> >Why can you not have TLB invalidation without the full BBM sequence?
> 
> I agree that in general TLBI instruction does not require the full
> BBM sequence. If we ever need the TLB invalidation per entry, it
> will be better to keep BBM to keep the code streamlined.

If this is not performance-critical, this sounds fine.

This does unnecessarily penalise some cases, though.

e.g. a guest only performing reads if write permission is removed from
pages. 

> However, in this case I think it will be better to return to the
> guest and replay the instruction if a data/instruction abort has
> been received.

That sounds like what we do in Linux.

On a fault, if the page tables are such that the fault should not occur,
we assume we raced with concurrent modification, and return to the
faulting instruction. Eventually (once the TLB maintenance is
completed), the guest will make forward progress.

We have locking around page table manipulation, but only have to take
them in the (hopefully) unlikely case of a race on the affected memory
area.

Thanks,
Mark.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [RFC 21/22] xen/arm: p2m: Re-implement p2m_set_mem_access using p2m_{set, get}_entry

[Mark Rutland]

Hi Julien, 

On Mon, Aug 01, 2016 at 04:40:50PM +0100, Julien Grall wrote:
> Hi Razvan,
> 
> On 28/07/16 16:04, Razvan Cojocaru wrote:
> >On 07/28/2016 05:51 PM, Julien Grall wrote:
> >>The function p2m_set_mem_access can be re-implemented using the generic
> >>functions p2m_get_entry and __p2m_set_entry.
> >>
> >>Note that because of the implementation of p2m_get_entry, a TLB
> >>invalidation instruction will be issued for each 4KB page. Therefore the
> >>performance of memaccess will be impacted, however the function is now
> >>safe on all the processors.
> >>
> >>Also the function apply_p2m_changes is dropped completely as it is not
> >>unused anymore.
> >>
> >>Signed-off-by: Julien Grall 
> >>Cc: Razvan Cojocaru 
> >>Cc: Tamas K Lengyel 
> >>
> >>---
> >>I have not ran any performance test with memaccess for now, but I
> >>expect an important and unavoidable impact because of how memaccess
> >>has been designed to workaround hardware limitation. Note that might
> >>be possible to re-work memaccess work on superpage but this should
> >>be done in a separate patch.
> >>---
> >> xen/arch/arm/p2m.c | 329 
> >> +++--
> >> 1 file changed, 38 insertions(+), 291 deletions(-)
> >
> >Thanks for the CC!
> >
> >This seems to only impact ARM, are there any planned changes for x86
> >along these lines as well?
> 
> Actually, it might be possible to remove the TLB for each 4KB entry
> in the memaccess case.

Could you clarify what you mean by "remove the TLB"? Do you mean a TLBI
instruction?

> After I read again multiple time the ARM ARM (D4-1732 in ARM DDI
> 0487A.i) and spoke with various ARM folks, changing the permission
> (i.e read, write, execute) does not require the break-before-make
> sequence.

Regardless of whether Break-Before-Make (BBM) is required, TLB
invalidation is still required to ensure the new permissions have taken
effect after *any* modification to page tables, unless:

* The prior entry was not permitted to be held in a TLB, and:
* No TLB held an entry for the address range.

> However, I noticed a latent bug in the memaccess code when the
> permission restriction are removed/changed.
> 
> In the current implementation (i.e without this series), the TLB
> invalidation is deferred until the p2m is released. Until that time,
> a vCPU may still run with the previous permission and trap into the
> hypervisor the permission fault. However, as the permission as
> changed, p2m_memaccess_check may fail and we will inject an abort to
> the guest.
> 
> The problem is very similar to [1]. This will still be true with
> this series applied if I relaxed the use of the break-before-make
> sequence. The two ways I can see to fix this are either try again
> the instruction (we would trap again if the permission was not
> correct) or keep the break-before-make.

Why can you not have TLB invalidation without the full BBM sequence?

Thanks,
Mark.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2] xen/arm: register clocks used by the hypervisor

[Mark Rutland]

On Wed, Jul 06, 2016 at 02:16:18PM +0100, Stefano Stabellini wrote:
> On Wed, 6 Jul 2016, Julien Grall wrote:
> > On 06/07/16 02:34, Michael Turquette wrote:
> > > Hi!
> > 
> > Hello Michael,
> > 
> > > Quoting Dirk Behme (2016-06-30 03:32:32)
> > > > Some clocks might be used by the Xen hypervisor and not by the Linux
> > > > kernel. If these are not registered by the Linux kernel, they might be
> > > > disabled by clk_disable_unused() as the kernel doesn't know that they
> > > > are used. The clock of the serial console handled by Xen is one
> > > > example for this. It might be disabled by clk_disable_unused() which
> > > > stops the whole serial output, even from Xen, then.
> > > 
> > > This whole thread had me confused until I realized that it all boiled
> > > down to some nomenclature issues (for me).
> > > 
> > > This code does not _register_ any clocks. It simply gets them and
> > > enables them, which is what every other clk consumer in the Linux kernel
> > > does. More details below.
> > > 
> > > > 
> > > > Up to now, the workaround for this has been to use the Linux kernel
> > > > command line parameter 'clk_ignore_unused'. See Xen bug
> > > > 
> > > > http://bugs.xenproject.org/xen/bug/45
> > > 
> > > clk_ignore_unused is a band-aid, not a proper medical solution. Setting
> > > that flag will not turn clocks on for you, nor will it guarantee that
> > > those clocks are never turned off in the future. It looks like you
> > > figured this out correctly in the patch below but it is worth repeating.
> > > 
> > > Also the new CLK_IS_CRITICAL flag might be of interest to you, but that
> > > flag only exists as a way to enable clocks that must be enabled for the
> > > system to function (hence, "critical") AND when those same clocks do not
> > > have an accompanying Linux driver to consume them and enable them.
> > 
> > I don't think we want the kernel to enable the clock for the hypervisor. We
> > want to tell the kernel "don't touch at all to this clock, it does not 
> > belong
> > to you".
> 
> Right, and that's why I was suggesting that another way to do this would
> be to set the "status" to "disabled" in Xen: so that Linux would leave
> the clock alone. But in that case Linux would not be happy to see
> disabled clocks which are actually supposed to be used by some devices.

If you were to do that, that would cover the entire clock-controller,
not necessarily for the individual clock line (as this does not
necessarily have a node of its own). So you'd prevent the use of other
clocks owned by that controller.

That's also not sufficient, as you'd have to do the same for resources
required to keep that clock active (parent clocks from different
controllers, regulators, GPIOs, etc).

I don't think that will work other than in very basic cases.

Thanks,
Mark.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2] xen/arm: register clocks used by the hypervisor

[Mark Rutland]

Hi,

On Tue, Jul 05, 2016 at 12:45:34PM +0200, Dirk Behme wrote:
> On 05.07.2016 12:39, Mark Rutland wrote:
> >On Tue, Jul 05, 2016 at 08:50:23AM +0200, Dirk Behme wrote:
> >>+- clocks: one or more clocks to be registered.
> >>+  Xen hypervisor drivers might replace native drivers, resulting in
> >>+  clocks not registered by these native drivers. To avoid that these
> >>+  unregistered clocks are disabled by the Linux kernel initialization
> >>+  register them in the hypervisor node.
> >>+  An example for this are the clocks of a serial driver already enabled
> >>+  by the firmware. If the clocks used by the serial hardware interface
> >>+  are not registered by the serial driver itself the serial output
> >>+  might stop once the Linux kernel initialization disables the 'unused'
> >>+  clocks.
> >
> >The above describes the set of problems, but doesn't set out the actual
> >contract. It also covers a number of Linux implementation details in
> >abstract.
> 
> Could you kindly be a little more specific which 'implementation
> details' you don't like?

The fact that we disable some clocks at init time is a driver model
thing that depends on various factors (e.g. cmdline options), and it's
something that could be moved around. We only mention disabling, and not
rate change (which could happen, even if it doesn't today).

I don't think that we need to describe the Linux behaviour at all.

> E.g. to my understanding, the 'implementation detail' that Linux
> disables unregistered clocks is needed for the description.
> 
> If you have a different wording in mind, could you kindly share that?

Something like:

- clocks: a list of phandle + clock-specifier pairs 
  Clocks described by this property are reserved for use by Xen, and the
  OS must not alter their state any way, such as disabling or gating a
  clock, or modifying its rate. Ensuring this may impose constraints on
  parent clocks or other resources used by the clock tree.

  Note: this property is used to proxy clocks for devices Xen has taken
  ownership of, such as UARTs, for which the associated clock
  controller(s) remain under the control of Dom0.

> >As I commented previously [1], the binding should describe the set of
> >guarantees that you rewquire (e.g. that the clocks must be left as-is,
> >not gated, and their rates left unchanged).
> >
> >Please describe the specific set of guarantees that you require.
> 
> To my understanding this is done, already: "avoid that these ...
> clocks are disabled"

My point of contention here is that while this might tell a dts author
what to place in this property, it doesn't specify what the OS should
do.

Thanks,
Mark.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2] xen/arm: register clocks used by the hypervisor

[Mark Rutland]

Hi,

On Tue, Jul 05, 2016 at 08:50:23AM +0200, Dirk Behme wrote:
> Some clocks might be used by the Xen hypervisor and not by the Linux
> kernel. If these are not registered by the Linux kernel, they might be
> disabled by clk_disable_unused() as the kernel doesn't know that they
> are used. The clock of the serial console handled by Xen is one
> example for this. It might be disabled by clk_disable_unused() which
> stops the whole serial output, even from Xen, then.
> 
> Up to now, the workaround for this has been to use the Linux kernel
> command line parameter 'clk_ignore_unused'. See Xen bug
> 
> http://bugs.xenproject.org/xen/bug/45
> 
> too.
> 
> To fix this, we will add the "unused" clocks in Xen to the hypervisor
> node. The Linux kernel has to register the clocks from the hypervisor
> node, then.
> 
> Therefore, check if there is a "clocks" entry in the hypervisor node
> and if so register the given clocks to the Linux kernel clock
> framework and with this mark them as used. This prevents the clocks
> from being disabled.
> 
> Signed-off-by: Dirk Behme 
> ---
> Changes in v2: Drop the Linux implementation details like clk_disable_unused
>  in xen.txt.

Thanks for doing this.

>  Documentation/devicetree/bindings/arm/xen.txt | 13 
>  arch/arm/xen/enlighten.c  | 47 
> +++
>  2 files changed, 60 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/xen.txt 
> b/Documentation/devicetree/bindings/arm/xen.txt
> index c9b9321..21fd469 100644
> --- a/Documentation/devicetree/bindings/arm/xen.txt
> +++ b/Documentation/devicetree/bindings/arm/xen.txt
> @@ -17,6 +17,19 @@ the following properties:
>A GIC node is also required.
>This property is unnecessary when booting Dom0 using ACPI.
>  
> +Optional properties:
> +
> +- clocks: one or more clocks to be registered.
> +  Xen hypervisor drivers might replace native drivers, resulting in
> +  clocks not registered by these native drivers. To avoid that these
> +  unregistered clocks are disabled by the Linux kernel initialization
> +  register them in the hypervisor node.
> +  An example for this are the clocks of a serial driver already enabled
> +  by the firmware. If the clocks used by the serial hardware interface
> +  are not registered by the serial driver itself the serial output
> +  might stop once the Linux kernel initialization disables the 'unused'
> +  clocks.

The above describes the set of problems, but doesn't set out the actual
contract. It also covers a number of Linux implementation details in
abstract.

As I commented previously [1], the binding should describe the set of
guarantees that you rewquire (e.g. that the clocks must be left as-is,
not gated, and their rates left unchanged).

Please describe the specific set of guarantees that you require.

Thanks,
Mark.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-June/440434.html

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2] xen/arm: register clocks used by the hypervisor

[Mark Rutland]

Hi,

On Thu, Jun 30, 2016 at 04:56:40PM +0200, Dirk Behme wrote:
> On 30.06.2016 16:21, Mark Rutland wrote:
> >On Thu, Jun 30, 2016 at 12:32:32PM +0200, Dirk Behme wrote:
> >>+- clocks: one or more clocks to be registered.
> >>+  Xen hypervisor drivers might replace native drivers, resulting in
> >>+  clocks not registered by these native drivers. To avoid that these
> >>+  unregistered clocks are disabled, then, e.g. by clk_disable_unused(),
> >>+  register them in the hypervisor node.
> >>+  An example for this are the clocks of the serial driver. If the clocks
> >>+  used by the serial hardware interface are not registered by the serial
> >>+  driver the serial output might stop once clk_disable_unused() is called.
> >
> >This shouldn't be described in terms of the Linux implementation details
> >like clk_disable_unused. Those can change at any time, and don't define
> >the contract as such.
> 
> Ok. I remove that from the description. Thanks!

Great, thanks.

> >What exactly is the contract here? I assume that you don't want the
> >kernel to alter the state of these clocks in any way,
> 
> I don't think so. I think what we want is that the kernel just don't
> disable the (from kernel's point of view) "unused" clocks. I think
> we get this from the fact that using 'clk_ignore_unused' is a
> working workaround for this issue.

What if the clock (or a parent thereof) is shared with another device?

Surely you don't want that other driver to change the rate (changing the
rate of the UART behind Xen's back)?

I appreciate that clk_ignore_unused might work on platforms today, but
that relies on the behaviour of drivers, which might change. It may also
not work on other platforms in future, in cases like the above.

> >e.g. the rate cannot be changed, they must be left always on, parent
> >clocks cannot be altered if that would result in momentary jitter.
> >
> >I suspect it may be necessary to do more to ensure that, but I'm not
> >familiar enough with the clocks subsystem to say.
> >
> >Are we likely to need to care about regulators, GPIOs, resets, etc? I
> >worry that this may be the tip of hte iceberg
> 
> I don't think so. If there is a user in the kernel of the clock,
> fine. Then hopefully the user in the kernel knows what he is doing
> with the clock and then he should do what ever he wants.

I'm not sure that's true. A user of the clock may know nothing about
other users. As far as I can see, nothing prevents it from changing the
clock rate.

While drivers in the same kernel can co-ordinate to avoid problems such
as that, we can't do that if we know nothing about the user hidden by
Xen.

From looking at the Xen bug tracker, it's not clear which
platforms/devices this is necessary for, so it's still not clear to me
if we need to care about regulators, GPIOs, resets, and so on.

Do we know which platforms in particular we need this for?

> As there is a user in the kernel, we don't have to care about
> 'accidentally' disabling it.
> 
> Just let us care about the clocks there is no user.

As above, I don't believe that alone is sufficient in general, though it
may happen to be for a particular platform. Without information
regarding the affected platform(s), it's rather difficult to tell.

Thanks,
Mark.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2] xen/arm: register clocks used by the hypervisor

[Mark Rutland]

On Thu, Jun 30, 2016 at 12:32:32PM +0200, Dirk Behme wrote:
> Some clocks might be used by the Xen hypervisor and not by the Linux
> kernel. If these are not registered by the Linux kernel, they might be
> disabled by clk_disable_unused() as the kernel doesn't know that they
> are used. The clock of the serial console handled by Xen is one
> example for this. It might be disabled by clk_disable_unused() which
> stops the whole serial output, even from Xen, then.
> 
> Up to now, the workaround for this has been to use the Linux kernel
> command line parameter 'clk_ignore_unused'. See Xen bug
> 
> http://bugs.xenproject.org/xen/bug/45
> 
> too.
> 
> To fix this, we will add the "unused" clocks in Xen to the hypervisor
> node. The Linux kernel has to register the clocks from the hypervisor
> node, then.
> 
> Therefore, check if there is a "clocks" entry in the hypervisor node
> and if so register the given clocks to the Linux kernel clock
> framework and with this mark them as used. This prevents the clocks
> from being disabled.
> 
> Signed-off-by: Dirk Behme 
> ---
> Changes in v2:
>  - Rebase against git://git.kernel.org/pub/scm/linux/kernel/git/xen/tip.git 
> for-linus-4.8
>  - Add changes to Documentation/devicetree/bindings/arm/xen.txt
> 
>  Documentation/devicetree/bindings/arm/xen.txt | 11 +++
>  arch/arm/xen/enlighten.c  | 47 
> +++
>  2 files changed, 58 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/xen.txt 
> b/Documentation/devicetree/bindings/arm/xen.txt
> index c9b9321..55dfd3b 100644
> --- a/Documentation/devicetree/bindings/arm/xen.txt
> +++ b/Documentation/devicetree/bindings/arm/xen.txt
> @@ -17,6 +17,17 @@ the following properties:
>A GIC node is also required.
>This property is unnecessary when booting Dom0 using ACPI.
>  
> +Optional properties:
> +
> +- clocks: one or more clocks to be registered.
> +  Xen hypervisor drivers might replace native drivers, resulting in
> +  clocks not registered by these native drivers. To avoid that these
> +  unregistered clocks are disabled, then, e.g. by clk_disable_unused(),
> +  register them in the hypervisor node.
> +  An example for this are the clocks of the serial driver. If the clocks
> +  used by the serial hardware interface are not registered by the serial
> +  driver the serial output might stop once clk_disable_unused() is called.

This shouldn't be described in terms of the Linux implementation details
like clk_disable_unused. Those can change at any time, and don't define
the contract as such.

What exactly is the contract here? I assume that you don't want the
kernel to alter the state of these clocks in any way, e.g. the rate
cannot be changed, they must be left always on, parent clocks cannot be
altered if that would result in momentary jitter.

I suspect it may be necessary to do more to ensure that, but I'm not
familiar enough with the clocks subsystem to say.

Are we likely to need to care about regulators, GPIOs, resets, etc? I
worry that this may be the tip of hte iceberg, and we might need a
better way of catering for the general case of resources Xen wants to
own.

Thanks,
Mark.

> +
>  To support UEFI on Xen ARM virtual platforms, Xen populates the FDT "uefi" 
> node
>  under /hypervisor with following parameters:
>  
> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> index 47acb36..5c546d0 100644
> --- a/arch/arm/xen/enlighten.c
> +++ b/arch/arm/xen/enlighten.c
> @@ -24,6 +24,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -444,6 +445,52 @@ static int __init xen_pm_init(void)
>  }
>  late_initcall(xen_pm_init);
>  
> +/*
> + * Check if we want to register some clocks, that they
> + * are not freed because unused by clk_disable_unused().
> + * E.g. the serial console clock.
> + */
> +static int __init xen_arm_register_clks(void)
> +{
> + struct clk *clk;
> + struct device_node *xen_node;
> + unsigned int i, count;
> + int ret = 0;
> +
> + xen_node = of_find_compatible_node(NULL, NULL, "xen,xen");
> + if (!xen_node) {
> + pr_err("Xen support was detected before, but it has 
> disappeared\n");
> + return -EINVAL;
> + }
> +
> + count = of_clk_get_parent_count(xen_node);
> + if (!count)
> + goto out;
> +
> + for (i = 0; i < count; i++) {
> + clk = of_clk_get(xen_node, i);
> + if (IS_ERR(clk)) {
> + pr_err("Xen failed to register clock %i. Error: %li\n",
> +i, PTR_ERR(clk));
> + ret = PTR_ERR(clk);
> + goto out;
> + }
> +
> + ret = clk_prepare_enable(clk);
> + if (ret < 0) {
> + pr_err("Xen failed to enable clock %i. Error: %i\n",
> +i, ret);
> + goto out;
> + 

Re: [Xen-devel] [PATCH] xen/arm: register clocks used by the hypervisor

[Mark Rutland]

On Wed, Jun 22, 2016 at 04:26:46PM +0100, Julien Grall wrote:
> Hello Dirk,
> 
> On 21/06/16 11:16, Dirk Behme wrote:
> >Some clocks might be used by the Xen hypervisor and not by the Linux
> >kernel. If these are not registered by the Linux kernel, they might be
> >disabled by clk_disable_unused() as the kernel doesn't know that they
> >are used. The clock of the serial console handled by Xen is one
> >example for this. It might be disabled by clk_disable_unused() which
> >stops the whole serial output, even from Xen, then.
> >
> >Up to now, the workaround for this has been to use the Linux kernel
> >command line parameter 'clk_ignore_unused'. See Xen bug
> >
> >http://bugs.xenproject.org/xen/bug/45
> >
> >too.
> >
> >To fix this, we will add the "unused" clocks in Xen to the hypervisor
> >node. The Linux kernel has to register the clocks from the hypervisor
> >node, then.
> >
> >Therefore, check if there is a "clocks" entry in the hypervisor node
> >and if so register the given clocks to the Linux kernel clock
> >framework and with this mark them as used. This prevents the clocks
> >from being disabled.
> 
> This new property would need to be documented in:
>- linux/Documentation/devicetree/bindings/arm/xen.txt
>- xen/docs/misc/arm/device-tree/guest.txt

This (series) should also be CC'd to devicet...@vger.kernel.org, and to
the clock framework maintainers.

I have further questions, but I will wait for that posting.

Thanks,
Mark.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v11 12/17] ARM64: ACPI: Check if it runs on Xen to enable or disable ACPI

[Mark Rutland]

On Wed, Apr 20, 2016 at 10:34:41AM +0100, Stefano Stabellini wrote:
> Hello Mark,
> 
> do you think that this patch addresses your previous comments
> (http://marc.info/?l=devicetree=145926913008544=2) appropriately?
> 
> Thanks,
> 
> Stefano
> 
> On Thu, 7 Apr 2016, Shannon Zhao wrote:
> > From: Shannon Zhao <shannon.z...@linaro.org>
> > 
> > When it's a Xen domain0 booting with ACPI, it will supply a /chosen and
> > a /hypervisor node in DT. So check if it needs to enable ACPI.
> > 
> > Signed-off-by: Shannon Zhao <shannon.z...@linaro.org>
> > Reviewed-by: Stefano Stabellini <stefano.stabell...@eu.citrix.com>
> > Acked-by: Hanjun Guo <hanjun@linaro.org>
> > ---
> >  arch/arm64/kernel/acpi.c | 14 ++
> >  1 file changed, 10 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
> > index d1ce8e2..57ee317 100644
> > --- a/arch/arm64/kernel/acpi.c
> > +++ b/arch/arm64/kernel/acpi.c
> > @@ -67,10 +67,15 @@ static int __init dt_scan_depth1_nodes(unsigned long 
> > node,
> >  {
> > /*
> >  * Return 1 as soon as we encounter a node at depth 1 that is
> > -* not the /chosen node.
> > +* not the /chosen node, or /hypervisor node with compatible
> > +* string "xen,xen".
> >  */
> > -   if (depth == 1 && (strcmp(uname, "chosen") != 0))
> > -   return 1;
> > +   if (depth == 1 && (strcmp(uname, "chosen") != 0)) {
> > +   if (strcmp(uname, "hypervisor") != 0 ||
> > +   !of_flat_dt_is_compatible(node, "xen,xen"))
> > +   return 1;
> > +   }
> > +
> > return 0;
> >  }

Is the duplicate node checking logic I mentioned in that review gone?
i.e. do we not need an is_xen_node() helper?

Additionally, IMO, this would be easier to follow without the nested
conditionals, e.g.

static int __init dt_scan_depth1_nodes(unsigned long node,
   const char *uname, int depth,
   void *data)
{
/*
 * Ignore anything not directly under the root node; we'll
 * catch its parent instead.
 */
if (depth != 1)
return 0;

if (strcmp(uname, "chosen") == 0)
return 0;

    if (strcmp(uname, "hypervisor") == 0 &&
of_flat_dt_is_compatible(node, "xen,xen"))
return 0;

/*
 * This node at depth 1 is neither a chosen node nor a xen node,
 * which we do not expect.
 */
return 1;
}

Otherwise, this looks fine to me. FWIW, either way:

Acked-by: Mark Rutland <mark.rutl...@arm.com>

As this is core arm64 code, I believe you'll need acks from Catalin
and/or Will (and likewise for patch 15), unless I've missed those.

Thanks,
Mark.

> >  
> > @@ -184,7 +189,8 @@ void __init acpi_boot_table_init(void)
> > /*
> >  * Enable ACPI instead of device tree unless
> >  * - ACPI has been disabled explicitly (acpi=off), or
> > -* - the device tree is not empty (it has more than just a /chosen node)
> > +* - the device tree is not empty (it has more than just a /chosen node,
> > +*   and a /hypervisor node when running on Xen)
> >  *   and ACPI has not been force enabled (acpi=force)
> >  */
> > if (param_acpi_off ||
> > -- 
> > 2.0.4
> > 
> > 
> 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v7 12/17] ARM64: ACPI: Check if it runs on Xen to enable or disable ACPI

[Mark Rutland]

On Thu, Mar 31, 2016 at 01:44:08PM +0200, Ard Biesheuvel wrote:
> The heuristic is there to decide whether some DTB image contains a
> complete description of the platform, or only some data handed over by
> the bootloader. Arguably, a DT containing both /chosen and /hypervisor
> but nothing else can still not describe an actual platform, and
> whether we execute under Xen or not is completely irrelevant.

I disagree somewhat.

In general, a /hypervisor node may not be a Xen node, and could
potentially imply some platform description. As /hypervisor is a generic
name up for grabs by any hypervisor, we simply cannot make assumptions
about it.

As /chosen is a special reserved path that implies a particular binding
and has no compatible string, so checking its path alone is correct.

While we do check that the /hypervisor node is "xen,xen" compatible
elsewhere, the canonical mechanism of checking for a Xen node (as
opposed to any hypervisor's node) is to check the compatible string.

If we are going to handle nodes for other hypervisors while treating the
DTB as empty, we need code and discussion regarding said hypervisor.

Hence, for checking for a Xen /hypervisor node, I would prefer we
checked the compatible string rather than the path.

An is_xen_node() helper (which could also check that the path is
"/hypervisor") would avoid having redundant, subtly distinct ways of
checking, and would explicitly document precisely what we are checking
for.

Thanks,
Mark.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v7 12/17] ARM64: ACPI: Check if it runs on Xen to enable or disable ACPI

[Mark Rutland]

On Tue, Mar 29, 2016 at 05:18:38PM +0100, Will Deacon wrote:
> On Thu, Mar 24, 2016 at 10:44:31PM +0800, Shannon Zhao wrote:
> > When it's a Xen domain0 booting with ACPI, it will supply a /chosen and
> > a /hypervisor node in DT. So check if it needs to enable ACPI.
> > 
> > Signed-off-by: Shannon Zhao 
> > Reviewed-by: Stefano Stabellini 
> > Acked-by: Hanjun Guo 
> > ---
> >  arch/arm64/kernel/acpi.c | 12 
> >  1 file changed, 8 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
> > index d1ce8e2..4e92be0 100644
> > --- a/arch/arm64/kernel/acpi.c
> > +++ b/arch/arm64/kernel/acpi.c
> > @@ -67,10 +67,13 @@ static int __init dt_scan_depth1_nodes(unsigned long 
> > node,
> >  {
> > /*
> >  * Return 1 as soon as we encounter a node at depth 1 that is
> > -* not the /chosen node.
> > +* not the /chosen node, or /hypervisor node when running on Xen.
> >  */
> > -   if (depth == 1 && (strcmp(uname, "chosen") != 0))
> > -   return 1;
> > +   if (depth == 1 && (strcmp(uname, "chosen") != 0)) {
> > +   if (!xen_initial_domain() || (strcmp(uname, "hypervisor") != 0))
> > +   return 1;
> > +   }
> 
> Hmm, but xen_initial_domain() is false when xen isn't being used at all,
> so it feels to me like this is a bit too far-reaching and is basically
> claiming the "/hypervisor" namespace for Xen. Couldn't it be renamed to
> "xen,hypervisor" or something?
> 
> Mark, got any thoughts on this?

The node has a compatible string, "xen,xen" per [1], which would tell us
absolutely that xen is present. I'd be happy checking for that
explicitly.

In patch 11 fdt_find_hyper_node checks the compatible string. We could
factor that out into a helper like is_xen_node(node) and use it here
too.

If in future we want to do the same for other hypervisors we can add
explicit checks for their compatible strings and/or wrap that in a more
generic helper to cater for any hypervisor-specific details.

Thanks,
Mark.

[1] Documentation/devicetree/bindings/arm/xen.txt

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3 13/17] ARM: Xen: Document UEFI support on Xen ARM virtual platforms

[Mark Rutland]

On Mon, Jan 25, 2016 at 03:50:52PM +, Stefano Stabellini wrote:
> On Sat, 23 Jan 2016, Shannon Zhao wrote:
> > From: Shannon Zhao 
> > 
> > Add a "uefi" node under /hypervisor node in FDT, then Linux kernel could
> > scan this to get the UEFI information.
> > 
> > Signed-off-by: Shannon Zhao 
> > Acked-by: Rob Herring 
> > ---
> > CC: Rob Herring 
> > ---
> >  Documentation/devicetree/bindings/arm/xen.txt | 34 
> > +++
> >  1 file changed, 34 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/arm/xen.txt 
> > b/Documentation/devicetree/bindings/arm/xen.txt
> > index 0f7b9c2..aa69405 100644
> > --- a/Documentation/devicetree/bindings/arm/xen.txt
> > +++ b/Documentation/devicetree/bindings/arm/xen.txt
> > @@ -15,6 +15,26 @@ the following properties:
> >  - interrupts: the interrupt used by Xen to inject event notifications.
> >A GIC node is also required.
> >  
> > +To support UEFI on Xen ARM virtual platforms, Xen populates the FDT "uefi" 
> > node
> > +under /hypervisor with following parameters:
> > +
> > +
> > +Name  | Size   | Description
> > +
> > +xen,uefi-system-table | 64-bit | Guest physical address of the UEFI 
> > System
> > + || Table.
> > +
> > +xen,uefi-mmap-start   | 64-bit | Guest physical address of the UEFI 
> > memory
> > + || map.
> > +
> > +xen,uefi-mmap-size| 32-bit | Size in bytes of the UEFI memory map
> > +  || pointed to in previous entry.
> > +
> > +xen,uefi-mmap-desc-size   | 32-bit | Size in bytes of each entry in the 
> > UEFI
> > +  || memory map.
> > +
> > +xen,uefi-mmap-desc-ver| 32-bit | Version of the mmap descriptor format.
> > +
> >  
> >  Example (assuming #address-cells = <2> and #size-cells = <2>):
> >  
> > @@ -22,4 +42,18 @@ hypervisor {
> > compatible = "xen,xen-4.3", "xen,xen";
> > reg = <0 0xb000 0 0x2>;
> > interrupts = <1 15 0xf08>;
> > +   uefi {
> > +   xen,uefi-system-table = <0x>;
> > +   xen,uefi-mmap-start = <0x>;
> > +   xen,uefi-mmap-size = <0x>;
> > +   xen,uefi-mmap-desc-size = <0x>;
> > +   xen,uefi-mmap-desc-ver = <0x>;
> > +};
> >  };
> > +
> > +The format and meaning of these "xen,uefi-*" parameters are similar to 
> > those in
> > +Documentation/arm/uefi.txt which are used by normal UEFI. But to 
> > distinguish
> > +from normal UEFI, for Xen ARM virtual platforms it needs to introduce a Xen
> > +specific UEFI which requires Xen hypervisor to provide hypercalls for Dom0 
> > to
> > +make use of the runtime services. Therefore, it defines these parameters 
> > under
> > +/hypervisor node.
> 
> Please replace the paragraph with the following which mentions
> include/xen/interface/platform.h and improves the wording:
> 
> "The format and meaning of the "xen,uefi-*" parameters are similar to
> those in Documentation/arm/uefi.txt, which are provided by the regular
> UEFI stub. However they differ because they are provided by the Xen
> hypervisor, together with a set of UEFI runtime services implemented via
> hypercalls, see include/xen/interface/platform.h."

I would prefer if we didn't refer to Linux paths here, for the usual
rationale of keeping bindings distinct from Linux implementation
details.

Could we either drop the "see include/xen/interface/platform.h" part, or
refer to some Xen documentation?

Thanks,
Mark.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 12/16] ARM: Xen: Document UEFI support on Xen ARM virtual platforms

[Mark Rutland]

On Tue, Jan 19, 2016 at 09:43:59PM +0800, Shannon Zhao wrote:
> 
> On 2016/1/19 21:13, Mark Rutland wrote:
> >On Tue, Jan 19, 2016 at 12:23:17PM +, Stefano Stabellini wrote:
> >>>On Tue, 19 Jan 2016, Mark Rutland wrote:
> >>>> >On Tue, Jan 19, 2016 at 06:25:25PM +0800, Shannon Zhao wrote:
> >>>>>>> > > >>We don't do this in Documentation/arm/uefi.txt, and I don't see 
> >>>>>>> > > >>why we
> >>>>>>> > > >>should do so here.
> >>>>>>> > > >>
> >>>>>>> > > >>Does Xen handle arbitrary size memory map descriptors? I'm not 
> >>>>>>> > > >>sure what
> >>>>>>> > > >>new information might be passed in future additions to the 
> >>>>>>> > > >>descriptor
> >>>>>>> > > >>format, and I'm not sure what should happen in the Dom0 case.
> >>>>>> > > >
> >>>>>> > > >Xen passes to Dom0 the memory map in the same format as the native
> >>>>>> > > >memory map.
> >>>> >
> >>>> >Does Xen parse or modify the EFI memory map in any way?
> >>>
> >>>Xen:
> >>>- calls EFI_BOOT_SERVICES.GetMemoryMap()
> >>>- takes note of the memory regions for its own usage
> >>>- create the fdt notes, including efi-mmap-start, with a pointer to it
> >>>
> >>>
> >>>> >Does it pass the raw values returned by EFI_BOOT_SERVICES.GetMemoryMap()
> >>>> >through to the xen,uefi-* properties, or does is make any static
> >>>> >assumptions about what the values will be?
> >>>
> >>>It just passes the raw values.
> >I take it that means that any memory carved out for Xen itself is
> >described/discovered via a separate mechanism? How does that work
> 
> For Xen hypervisor booting on UEFI, it get the EFI memory map
> through the similar way like Linux, e.g. call
> EFI_BOOT_SERVICES.GetMemoryMap().
> For Dom0, Xen will create a new EFI memory map for Dom0.
> 
> See [PATCH v3 52/62] arm/acpi: Prepare EFI memory descriptor for Dom0
> http://lists.xen.org/archives/html/xen-devel/2015-11/msg01884.html

Ok. So Xen will parse the EFI memory map, and will create a new memory
map to pass to the Dom0 kernel, presumably with some memory having been
carved out for Xen itself, and never described to Dom0.

So if there's any extension to that in future, Dom0 may see problems.
There's not much that can be done about that, however, and extensions to
the descriptors seem unlikely at present.

Thanks,
Mark.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 12/16] ARM: Xen: Document UEFI support on Xen ARM virtual platforms

[Mark Rutland]

On Tue, Jan 19, 2016 at 06:25:25PM +0800, Shannon Zhao wrote:
> 
> 
> On 2016/1/19 1:34, Stefano Stabellini wrote:
> > On Mon, 18 Jan 2016, Mark Rutland wrote:
> >> On Fri, Jan 15, 2016 at 02:55:25PM +0800, Shannon Zhao wrote:
> >>> From: Shannon Zhao <shannon.z...@linaro.org>
> >>>
> >>> Add a "uefi" node under /hypervisor node in FDT, then Linux kernel could
> >>> scan this to get the UEFI information.
> >>>
> >>> Signed-off-by: Shannon Zhao <shannon.z...@linaro.org>
> >>> ---
> >>>  Documentation/devicetree/bindings/arm/xen.txt | 42 
> >>> +++
> >>>  1 file changed, 42 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/arm/xen.txt 
> >>> b/Documentation/devicetree/bindings/arm/xen.txt
> >>> index 0f7b9c2..fbc17ae 100644
> >>> --- a/Documentation/devicetree/bindings/arm/xen.txt
> >>> +++ b/Documentation/devicetree/bindings/arm/xen.txt
> >>> @@ -15,6 +15,36 @@ the following properties:
> >>>  - interrupts: the interrupt used by Xen to inject event notifications.
> >>>A GIC node is also required.
> >>>  
> >>> +To support UEFI on Xen ARM virtual platforms, Xen pupulates the FDT 
> >>> "uefi" node
> >>> +under /hypervisor with following parameters:
> >>
> >> s/pupulates/populates/
> >>
> >>> +
> >>> +
> >>> +Name  | Size   | Description
> >>> +
> >>> +xen,uefi-system-table | 64-bit | Guest physical address of the UEFI 
> >>> System
> >>> +   || Table.
> >>> +
> >>> +xen,uefi-mmap-start   | 64-bit | Guest physical address of the UEFI 
> >>> memory
> >>> +   || map.
> >>> +
> >>> +xen,uefi-mmap-size| 32-bit | Size in bytes of the UEFI memory map
> >>> +  || pointed to in previous entry.
> >>> +
> >>> +xen,uefi-mmap-desc-size   | 32-bit | Size in bytes of each entry in the 
> >>> UEFI
> >>> +  || memory map.
> >>> +
> >>> +xen,uefi-mmap-desc-ver| 32-bit | Version of the mmap descriptor 
> >>> format.
> >>> +
> >>> +
> >>> +Below is the format of the mmap descriptor.
> >>> +typedef struct {
> >>> + u32 type;
> >>> + u32 pad;
> >>> + u64 phys_addr;
> >>> + u64 virt_addr;
> >>> + u64 num_pages;
> >>> + u64 attribute;
> >>> +} efi_memory_desc_t;
> >>
> >> I don't think we should describe this here, as it duplicates the UEFI
> >> spec, and is techincally incorrect the above is only guaranteed to be
> >> the prefix of each memory descriptor -- that's why the
> >> uefi-mmap-desc-size property exists.
> >>
> Oh, this format is suggested to describe here at previous patch set.

We can describe it by referring to the definition in the UEFI
specification (i.e. state the properties represent the return values of
EFI_BOOT_SERVICES.GetMemoryMap()).

If that's necessary at all, fix that in the usual
Documentation/arm/uefi.txt, and state here that the format and meaning
of each property here follows its unprefixed cousin, with the caveat
that Xen-specific assumptions also apply (e.g. runtime services must be
indirected via hypercalls).

Anything else is redundant and risks being wrong.

> >> We don't do this in Documentation/arm/uefi.txt, and I don't see why we
> >> should do so here.
> >>
> >> Does Xen handle arbitrary size memory map descriptors? I'm not sure what
> >> new information might be passed in future additions to the descriptor
> >> format, and I'm not sure what should happen in the Dom0 case.
> > 
> > Xen passes to Dom0 the memory map in the same format as the native
> > memory map.

Does Xen p

Re: [Xen-devel] [PATCH v2 16/16] ARM64: XEN: Initialize Xen specific UEFI runtime services

[Mark Rutland]

On Tue, Jan 19, 2016 at 12:03:59PM +, Stefano Stabellini wrote:
> On Mon, 18 Jan 2016, Mark Rutland wrote:
> > On Mon, Jan 18, 2016 at 05:45:24PM +, Stefano Stabellini wrote:
> > > On Mon, 18 Jan 2016, Mark Rutland wrote:
> > > > On Fri, Jan 15, 2016 at 02:55:29PM +0800, Shannon Zhao wrote:
> > > > > +void __init xen_efi_runtime_setup(void)
> > > > > +{
> > > > > + efi.get_time = xen_efi_get_time;
> > > > > + efi.set_time = xen_efi_set_time;
> > > > > + efi.get_wakeup_time  = xen_efi_get_wakeup_time;
> > > > > + efi.set_wakeup_time  = xen_efi_set_wakeup_time;
> > > > > + efi.get_variable = xen_efi_get_variable;
> > > > > + efi.get_next_variable= xen_efi_get_next_variable;
> > > > > + efi.set_variable = xen_efi_set_variable;
> > > > > + efi.query_variable_info  = xen_efi_query_variable_info;
> > > > > + efi.update_capsule   = xen_efi_update_capsule;
> > > > > + efi.query_capsule_caps   = xen_efi_query_capsule_caps;
> > > > > + efi.get_next_high_mono_count = xen_efi_get_next_high_mono_count;
> > > > > + efi.reset_system = NULL;
> > > > > +}
> > > > 
> > > > How do capsules work in the absence of an EFI system reset?
> > > 
> > > Actually I don't think that capsules are available in Xen on ARM64 yet,
> > > see "TODO - disabled until implemented on ARM" in
> > > xen/common/efi/runtime.c.
> > > 
> > > FYI system reset is available, but it is provided via a different
> > > mechanism (HYPERVISOR_sched_op(xen_restart...)
> > 
> > Will that trigger Xen to do the right thing to trigger capsule updates
> > when implemented in Xen? Or do we need a xen_efi_reset_system?
> 
> On ARM, to reboot the hardware, Xen calls the native PSCI system_reset
> method. On x86, Xen calls efi_reset_system on EFI systems, and has
> several fall backs if that doesn't work as expected (see
> xen/arch/x86/shutdown.c:machine_restart).
> 
> But on a second look it doesn't look like that the capsule hypercalls
> are implemented correctly even on x86 (there is an "XXX fall through for
> now" comment in the code). I guess they are not available on Xen at all
> unfortunately.

That is incredibly unfortunate. It effectively renders the firmware
non-updateable when using Xen.

> > Does that override PSCI?
> 
> It does not, HYPERVISOR_sched_op(xen_restart,) is in addition to it. It
> ends up calling the same function within Xen as PSCI system_reset.

I meant within Dom0.

Presumably Dom0 calls HYPERVISOR_sched_op(xen_restart,), and doesn't
ever call PSCI SYSTEM_RESET?

> > In machine_restart we try efi_reboot first specifically to allow for
> > capsule updates. Similarly drivers/firmware/efi/reboot.c registers
> > efi_power_off late in order to override anything else, though that's
> > best-effort at present.
> 
> That's very interesting. I think that Xen on ARM should follow what
> Linux does and what Xen already does on x86 and try efi_reset_system
> first on efi systems.

I would agree.

Thanks,
Mark.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 14/16] Xen: EFI: Parse DT parameters for Xen specific UEFI

[Mark Rutland]

On Tue, Jan 19, 2016 at 09:19:05PM +0800, Shannon Zhao wrote:
> 
> 
> On 2016/1/18 23:41, Stefano Stabellini wrote:
> >CC'ing Matt Fleming
> >
> >On Fri, 15 Jan 2016, Shannon Zhao wrote:
> >>From: Shannon Zhao 
> >>@@ -520,15 +531,28 @@ static int __init fdt_find_uefi_params(unsigned long 
> >>node, const char *uname,
> >>   int depth, void *data)
> >>  {
> >>struct param_info *info = data;
> >>+   struct params *dt_params;
> >>+   unsigned int size;
> >>const void *prop;
> >>void *dest;
> >>u64 val;
> >>int i, len;
> >>
> >>-   if (depth != 1 || strcmp(uname, "chosen") != 0)
> >>-   return 0;
> >>+   if (efi_enabled(EFI_PARAVIRT)) {
> >>+   if (depth != 2 || strcmp(uname, "uefi") != 0)
> >
> >You are already introducing this check in the previous patch when
> >setting EFI_PARAVIRT, why do this again now?  But if we need to do this
> >check again, then, like Mark suggested, it should be done against the
> >full path.
> >
> This check just wants to confirm that the current node is the "uefi"
> node and we can parse it with xen_fdt_params now.

There is no single "uefi" node as many nodes can share that name. There
is at most a single, /hypervisor/uefi node, as that is qualified by a
full path.

Checking the leaf node name, as above, is insufficient. For example, the
below will be accepted:

* /chosen/uefi
* /foo/uefi
* /not-a-hypervisor/uefi

Any of these could exist in addition to a /hypervisor/uefi node, and
could appear ealier or later in the DTB.

There may be reasons to add such nodes in future, and regardless we
should not read properties from an invalid/wrong node.

Thanks,
Mark.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 12/16] ARM: Xen: Document UEFI support on Xen ARM virtual platforms

[Mark Rutland]

On Tue, Jan 19, 2016 at 12:23:17PM +, Stefano Stabellini wrote:
> On Tue, 19 Jan 2016, Mark Rutland wrote:
> > On Tue, Jan 19, 2016 at 06:25:25PM +0800, Shannon Zhao wrote:
> > > >> We don't do this in Documentation/arm/uefi.txt, and I don't see why we
> > > >> should do so here.
> > > >>
> > > >> Does Xen handle arbitrary size memory map descriptors? I'm not sure 
> > > >> what
> > > >> new information might be passed in future additions to the descriptor
> > > >> format, and I'm not sure what should happen in the Dom0 case.
> > > > 
> > > > Xen passes to Dom0 the memory map in the same format as the native
> > > > memory map.
> > 
> > Does Xen parse or modify the EFI memory map in any way?
> 
> Xen:
> - calls EFI_BOOT_SERVICES.GetMemoryMap()
> - takes note of the memory regions for its own usage
> - create the fdt notes, including efi-mmap-start, with a pointer to it
> 
> 
> > Does it pass the raw values returned by EFI_BOOT_SERVICES.GetMemoryMap()
> > through to the xen,uefi-* properties, or does is make any static
> > assumptions about what the values will be?
> 
> It just passes the raw values.

I take it that means that any memory carved out for Xen itself is
described/discovered via a separate mechanism? How does that work?

> > I'm trying to get a feeling for what the behaviour will be if/when a
> > version of the EFI spec expands the memory map format.
> 
> Linux is likely to get the memory map in the same format as it's
> provided on native.

Ok.

The only fragility I forsee there would be if some extended memory
descriptor format defined things the kernel understood but Xen did not.

Given that would require an update to the kernel, at that point code is
added for that we can figure out how to determine whether Xen handled
any of said information, and whether any further negotiation with Xen is
necessary. I assume Xen exposes some feature probing mechanism to guests
that can cater for that if necessary.

Ard, Leif, I trust you two can keep an eye out for anything of that
sort on the UEFI side. :)

Thanks,
Mark.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 12/16] ARM: Xen: Document UEFI support on Xen ARM virtual platforms

[Mark Rutland]

On Fri, Jan 15, 2016 at 02:55:25PM +0800, Shannon Zhao wrote:
> From: Shannon Zhao 
> 
> Add a "uefi" node under /hypervisor node in FDT, then Linux kernel could
> scan this to get the UEFI information.
> 
> Signed-off-by: Shannon Zhao 
> ---
>  Documentation/devicetree/bindings/arm/xen.txt | 42 
> +++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/xen.txt 
> b/Documentation/devicetree/bindings/arm/xen.txt
> index 0f7b9c2..fbc17ae 100644
> --- a/Documentation/devicetree/bindings/arm/xen.txt
> +++ b/Documentation/devicetree/bindings/arm/xen.txt
> @@ -15,6 +15,36 @@ the following properties:
>  - interrupts: the interrupt used by Xen to inject event notifications.
>A GIC node is also required.
>  
> +To support UEFI on Xen ARM virtual platforms, Xen pupulates the FDT "uefi" 
> node
> +under /hypervisor with following parameters:

s/pupulates/populates/

> +
> +
> +Name  | Size   | Description
> +
> +xen,uefi-system-table | 64-bit | Guest physical address of the UEFI 
> System
> +   || Table.
> +
> +xen,uefi-mmap-start   | 64-bit | Guest physical address of the UEFI 
> memory
> +   || map.
> +
> +xen,uefi-mmap-size| 32-bit | Size in bytes of the UEFI memory map
> +  || pointed to in previous entry.
> +
> +xen,uefi-mmap-desc-size   | 32-bit | Size in bytes of each entry in the UEFI
> +  || memory map.
> +
> +xen,uefi-mmap-desc-ver| 32-bit | Version of the mmap descriptor format.
> +
> +
> +Below is the format of the mmap descriptor.
> +typedef struct {
> + u32 type;
> + u32 pad;
> + u64 phys_addr;
> + u64 virt_addr;
> + u64 num_pages;
> + u64 attribute;
> +} efi_memory_desc_t;

I don't think we should describe this here, as it duplicates the UEFI
spec, and is techincally incorrect the above is only guaranteed to be
the prefix of each memory descriptor -- that's why the
uefi-mmap-desc-size property exists.

We don't do this in Documentation/arm/uefi.txt, and I don't see why we
should do so here.

Does Xen handle arbitrary size memory map descriptors? I'm not sure what
new information might be passed in future additions to the descriptor
format, and I'm not sure what should happen in the Dom0 case.

>  Example (assuming #address-cells = <2> and #size-cells = <2>):
>  
> @@ -22,4 +52,16 @@ hypervisor {
>   compatible = "xen,xen-4.3", "xen,xen";
>   reg = <0 0xb000 0 0x2>;
>   interrupts = <1 15 0xf08>;
> + uefi {
> + xen,uefi-system-table = <0x>;
> + xen,uefi-mmap-start = <0x>;
> + xen,uefi-mmap-size = <0x>;
> + xen,uefi-mmap-desc-size = <0x>;
> + xen,uefi-mmap-desc-ver = <0x>;
> +};
>  };
> +
> +These "xen,uefi-*" parameters are similar to those in 
> Documentation/arm/uefi.txt
> +which are used by normal UEFI. But to Xen ARM virtual platforms, it needs to
> +introduce a Xen specific UEFI and it doesn't want to mix with normal UEFI.
> +Therefore, it defines these parameters under /hypervisor node.

Could we please describe what that actual difference is?

I know that the OS must handle a system table differently under Xen, but
this doesn't describe what it should do.

I assume that the OS can handle the memory map in an identical fashion
to when it is native. Is that true?

Mark.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 01/16] Xen: ACPI: Hide UART used by Xen

[Mark Rutland]

On Fri, Jan 15, 2016 at 02:55:14PM +0800, Shannon Zhao wrote:
> From: Shannon Zhao 
> 
> ACPI 6.0 introduces a new table STAO to list the devices which are used
> by Xen and can't be used by Dom0. On Xen virtual platforms, the physical
> UART is used by Xen. So here it hides UART from Dom0.
> 
> Signed-off-by: Shannon Zhao 
> ---
> CC: "Rafael J. Wysocki"  (supporter:ACPI)
> CC: Len Brown  (supporter:ACPI)
> CC: linux-a...@vger.kernel.org (open list:ACPI)
> ---
>  drivers/acpi/bus.c | 30 ++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index a212cef..d7a559f 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -46,6 +46,7 @@ ACPI_MODULE_NAME("bus");
>  struct acpi_device *acpi_root;
>  struct proc_dir_entry *acpi_root_dir;
>  EXPORT_SYMBOL(acpi_root_dir);
> +static u64 spcr_uart_addr;
>  
>  #ifdef CONFIG_X86
>  #ifdef CONFIG_ACPI_CUSTOM_DSDT
> @@ -93,6 +94,17 @@ acpi_status acpi_bus_get_status_handle(acpi_handle handle,
>  {
>   acpi_status status;
>  
> + if (spcr_uart_addr != 0x) {

The SPCR spec says that the Base Address fields being zero means that
console redirection is disabled (though I'm not clear on whether or not
that requires the whole acpi_generic_address to be zero).

Can we not use that here?

Mark.

> + u64 addr;
> +
> + status = acpi_evaluate_integer(handle, METHOD_NAME__ADR, NULL,
> +);
> + if (ACPI_SUCCESS(status) && (addr == spcr_uart_addr)) {
> + *sta = 0;
> + return AE_OK;
> + }
> + }
> +
>   status = acpi_evaluate_integer(handle, "_STA", NULL, sta);
>   if (ACPI_SUCCESS(status))
>   return AE_OK;
> @@ -1069,6 +1081,8 @@ EXPORT_SYMBOL_GPL(acpi_kobj);
>  static int __init acpi_init(void)
>  {
>   int result;
> + acpi_status status;
> + struct acpi_table_stao *stao_ptr;
>  
>   if (acpi_disabled) {
>   printk(KERN_INFO PREFIX "Interpreter disabled.\n");
> @@ -1081,6 +1095,22 @@ static int __init acpi_init(void)
>   acpi_kobj = NULL;
>   }
>  
> + /* If there is STAO table, check whether it needs to ignore the UART
> +  * device in SPCR table.
> +  */
> + spcr_uart_addr = 0x;
> + status = acpi_get_table(ACPI_SIG_STAO, 0,
> + (struct acpi_table_header **)_ptr);
> + if (ACPI_SUCCESS(status)) {
> + if (stao_ptr->ignore_uart) {
> + struct acpi_table_spcr *spcr_ptr;
> +
> + acpi_get_table(ACPI_SIG_SPCR, 0,
> +(struct acpi_table_header **)_ptr);
> + spcr_uart_addr = spcr_ptr->serial_port.address;
> + }
> + }
> +
>   init_acpi_device_notify();
>   result = acpi_bus_init();
>   if (result) {
> -- 
> 2.0.4
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 11/16] ARM64: ACPI: Check if it runs on Xen to enable or disable ACPI

[Mark Rutland]

On Fri, Jan 15, 2016 at 02:55:24PM +0800, Shannon Zhao wrote:
> From: Shannon Zhao 
> 
> When it's a Xen domain0 booting with ACPI, it will supply a /chosen and
> a /hypervisor node in DT. So check if it needs to enable ACPI.
> 
> Signed-off-by: Shannon Zhao 
> ---
>  arch/arm64/kernel/acpi.c | 12 
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
> index d1ce8e2..4e92be0 100644
> --- a/arch/arm64/kernel/acpi.c
> +++ b/arch/arm64/kernel/acpi.c
> @@ -67,10 +67,13 @@ static int __init dt_scan_depth1_nodes(unsigned long node,
>  {
>   /*
>* Return 1 as soon as we encounter a node at depth 1 that is
> -  * not the /chosen node.
> +  * not the /chosen node, or /hypervisor node when running on Xen.
>*/
> - if (depth == 1 && (strcmp(uname, "chosen") != 0))
> - return 1;
> + if (depth == 1 && (strcmp(uname, "chosen") != 0)) {
> + if (!xen_initial_domain() || (strcmp(uname, "hypervisor") != 0))
> + return 1;
> + }
> +
>   return 0;
>  }

As this is changing the semantic of an "empty" DT, we should consider
now if there's anything else that might also need to exist in an "empty"
DT. We don't want to change this again in future if we don't have to,
given the compatiblity nightmare that's sure to result.

We should also consider if the "hypervisor" node name is sufficient (I
think it is, but let's not assume anything).

Mark.

>  
> @@ -184,7 +187,8 @@ void __init acpi_boot_table_init(void)
>   /*
>* Enable ACPI instead of device tree unless
>* - ACPI has been disabled explicitly (acpi=off), or
> -  * - the device tree is not empty (it has more than just a /chosen node)
> +  * - the device tree is not empty (it has more than just a /chosen node,
> +  *   and a /hypervisor node when running on Xen)
>*   and ACPI has not been force enabled (acpi=force)
>*/
>   if (param_acpi_off ||
> -- 
> 2.0.4
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-efi" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 16/16] ARM64: XEN: Initialize Xen specific UEFI runtime services

[Mark Rutland]

On Mon, Jan 18, 2016 at 05:45:24PM +, Stefano Stabellini wrote:
> On Mon, 18 Jan 2016, Mark Rutland wrote:
> > On Fri, Jan 15, 2016 at 02:55:29PM +0800, Shannon Zhao wrote:
> > > +void __init xen_efi_runtime_setup(void)
> > > +{
> > > + efi.get_time = xen_efi_get_time;
> > > + efi.set_time = xen_efi_set_time;
> > > + efi.get_wakeup_time  = xen_efi_get_wakeup_time;
> > > + efi.set_wakeup_time  = xen_efi_set_wakeup_time;
> > > + efi.get_variable = xen_efi_get_variable;
> > > + efi.get_next_variable= xen_efi_get_next_variable;
> > > + efi.set_variable = xen_efi_set_variable;
> > > + efi.query_variable_info  = xen_efi_query_variable_info;
> > > + efi.update_capsule   = xen_efi_update_capsule;
> > > + efi.query_capsule_caps   = xen_efi_query_capsule_caps;
> > > + efi.get_next_high_mono_count = xen_efi_get_next_high_mono_count;
> > > + efi.reset_system = NULL;
> > > +}
> > 
> > How do capsules work in the absence of an EFI system reset?
> 
> Actually I don't think that capsules are available in Xen on ARM64 yet,
> see "TODO - disabled until implemented on ARM" in
> xen/common/efi/runtime.c.
> 
> FYI system reset is available, but it is provided via a different
> mechanism (HYPERVISOR_sched_op(xen_restart...)

Will that trigger Xen to do the right thing to trigger capsule updates
when implemented in Xen? Or do we need a xen_efi_reset_system?

Does that override PSCI?

In machine_restart we try efi_reboot first specifically to allow for
capsule updates. Similarly drivers/firmware/efi/reboot.c registers
efi_power_off late in order to override anything else, though that's
best-effort at present.

Thanks,
Mark.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 14/16] Xen: EFI: Parse DT parameters for Xen specific UEFI

[Mark Rutland]

On Fri, Jan 15, 2016 at 02:55:27PM +0800, Shannon Zhao wrote:
> From: Shannon Zhao 
> 
> Add a new function to parse DT parameters for Xen specific UEFI just
> like the way for normal UEFI. Then it could reuse the existing codes.
> 
> Signed-off-by: Shannon Zhao 
> ---
>  drivers/firmware/efi/efi.c | 45 ++---
>  1 file changed, 38 insertions(+), 7 deletions(-)

> @@ -520,15 +531,28 @@ static int __init fdt_find_uefi_params(unsigned long 
> node, const char *uname,
>  int depth, void *data)
>  {
>   struct param_info *info = data;
> + struct params *dt_params;
> + unsigned int size;
>   const void *prop;
>   void *dest;
>   u64 val;
>   int i, len;
>  
> - if (depth != 1 || strcmp(uname, "chosen") != 0)
> - return 0;
> + if (efi_enabled(EFI_PARAVIRT)) {
> + if (depth != 2 || strcmp(uname, "uefi") != 0)
> + return 0;

Please check the full path rather than the leaf node name alone.

Mark.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 16/16] ARM64: XEN: Initialize Xen specific UEFI runtime services

[Mark Rutland]

On Fri, Jan 15, 2016 at 02:55:29PM +0800, Shannon Zhao wrote:
> From: Shannon Zhao 
> 
> When running on Xen hypervisor, runtime services are supported through
> hypercall. So call Xen specific function to initialize runtime services.
> 
> Signed-off-by: Shannon Zhao 
> ---
>  arch/arm/xen/enlighten.c |  5 +
>  arch/arm64/xen/Makefile  |  1 +
>  arch/arm64/xen/efi.c | 36 
>  drivers/xen/Kconfig  |  2 +-
>  include/xen/xen-ops.h|  1 +
>  5 files changed, 44 insertions(+), 1 deletion(-)
>  create mode 100644 arch/arm64/xen/efi.c
> 
> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> index 485e117..84f27ec 100644
> --- a/arch/arm/xen/enlighten.c
> +++ b/arch/arm/xen/enlighten.c
> @@ -414,6 +414,11 @@ static int __init xen_guest_init(void)
>   if (xen_initial_domain())
>   pvclock_gtod_register_notifier(_pvclock_gtod_notifier);
>  
> + if (IS_ENABLED(CONFIG_XEN_EFI)) {
> + if (efi_enabled(EFI_PARAVIRT))
> + xen_efi_runtime_setup();
> + }
> +
>   return 0;
>  }
>  early_initcall(xen_guest_init);
> diff --git a/arch/arm64/xen/Makefile b/arch/arm64/xen/Makefile
> index 74a8d87..62e6fe2 100644
> --- a/arch/arm64/xen/Makefile
> +++ b/arch/arm64/xen/Makefile
> @@ -1,2 +1,3 @@
>  xen-arm-y+= $(addprefix ../../arm/xen/, enlighten.o grant-table.o p2m.o 
> mm.o)
>  obj-y:= xen-arm.o hypercall.o
> +obj-$(CONFIG_XEN_EFI) += efi.o
> diff --git a/arch/arm64/xen/efi.c b/arch/arm64/xen/efi.c
> new file mode 100644
> index 000..33046b0
> --- /dev/null
> +++ b/arch/arm64/xen/efi.c
> @@ -0,0 +1,36 @@
> +/*
> + * Copyright (c) 2015, Linaro Limited, Shannon Zhao
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program.  If not, see .
> + */
> +
> +#include 
> +#include 
> +
> +void __init xen_efi_runtime_setup(void)
> +{
> + efi.get_time = xen_efi_get_time;
> + efi.set_time = xen_efi_set_time;
> + efi.get_wakeup_time  = xen_efi_get_wakeup_time;
> + efi.set_wakeup_time  = xen_efi_set_wakeup_time;
> + efi.get_variable = xen_efi_get_variable;
> + efi.get_next_variable= xen_efi_get_next_variable;
> + efi.set_variable = xen_efi_set_variable;
> + efi.query_variable_info  = xen_efi_query_variable_info;
> + efi.update_capsule   = xen_efi_update_capsule;
> + efi.query_capsule_caps   = xen_efi_query_capsule_caps;
> + efi.get_next_high_mono_count = xen_efi_get_next_high_mono_count;
> + efi.reset_system = NULL;
> +}

How do capsules work in the absence of an EFI system reset?

Are there any other mandatory features that are missing in a
Xen-provided pseudo-EFI?

Mark.

> +EXPORT_SYMBOL_GPL(xen_efi_runtime_setup);
> diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
> index 73708ac..27d216a 100644
> --- a/drivers/xen/Kconfig
> +++ b/drivers/xen/Kconfig
> @@ -268,7 +268,7 @@ config XEN_HAVE_PVMMU
>  
>  config XEN_EFI
>   def_bool y
> - depends on X86_64 && EFI
> + depends on (ARM64 || X86_64) && EFI
>  
>  config XEN_AUTO_XLATE
>   def_bool y
> diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
> index c83a338..36ff8e4 100644
> --- a/include/xen/xen-ops.h
> +++ b/include/xen/xen-ops.h
> @@ -107,6 +107,7 @@ efi_status_t xen_efi_update_capsule(efi_capsule_header_t 
> **capsules,
>  efi_status_t xen_efi_query_capsule_caps(efi_capsule_header_t **capsules,
>   unsigned long count, u64 *max_size,
>   int *reset_type);
> +void xen_efi_runtime_setup(void);
>  
>  #ifdef CONFIG_PREEMPT
>  
> -- 
> 2.0.4
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-efi" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 13/16] ARM: XEN: Set EFI_PARAVIRT if Xen supports EFI

[Mark Rutland]

On Fri, Jan 15, 2016 at 02:55:26PM +0800, Shannon Zhao wrote:
> From: Shannon Zhao 
> 
> Check if there is "uefi" node in the DT. If so, set EFI_PARAVIRT flag.
> 
> Signed-off-by: Shannon Zhao 
> ---
>  arch/arm/xen/enlighten.c | 23 +++
>  arch/arm64/kernel/efi.c  |  5 +
>  2 files changed, 28 insertions(+)
> 
> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> index 5d0fe68..485e117 100644
> --- a/arch/arm/xen/enlighten.c
> +++ b/arch/arm/xen/enlighten.c
> @@ -31,6 +31,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  
> @@ -248,6 +249,19 @@ static int __init fdt_find_xen_node(unsigned long node, 
> const char *uname,
>   return 0;
>  }
>  
> +static int __init fdt_find_uefi_node(unsigned long node, const char *uname,
> +  int depth, void *data)
> +{
> + bool *found = data;
> +
> + if (depth != 2 || strcmp(uname, "uefi") != 0)
> + return 0;
> +
> + *found = true;
> +
> + return 1;
> +}

I don't like this. What if we had to add a uefi node in the !Xen case
for some reason?

You want to look for /hypervisor/uefi, specifically when the hypervisor
compatible contains "xen,xen".

It would be better to find the "/hypervisor" node, checking for the
compatible string, then walk within that in the Xen-specific init
routine.

> +
>  /*
>   * see Documentation/devicetree/bindings/arm/xen.txt for the
>   * documentation of the Xen Device Tree format.
> @@ -255,6 +269,8 @@ static int __init fdt_find_xen_node(unsigned long node, 
> const char *uname,
>  #define GRANT_TABLE_PHYSADDR 0
>  void __init xen_early_init(void)
>  {
> + bool uefi_found = false;
> +
>   of_scan_flat_dt(fdt_find_xen_node, NULL);
>   if (!xen_node.found) {
>   pr_debug("No Xen support\n");
> @@ -279,6 +295,13 @@ void __init xen_early_init(void)
>  
>   if (!console_set_on_cmdline && !xen_initial_domain())
>   add_preferred_console("hvc", 0, NULL);
> +
> + if (IS_ENABLED(CONFIG_XEN_EFI)) {
> + /* Check if Xen support UEFI */
> + of_scan_flat_dt(fdt_find_uefi_node, _found);
> + if (uefi_found)
> + set_bit(EFI_PARAVIRT, );
> + }
>  }

This alone is insufficient given that we haven't parsed the rest of the
/hypervisor/uefi properties. Is the kernel resilient such that this
patch alone will not result in a panic?

I think it would be best for this to be in the same patch as the rest of
the hypervisor UEFI property parsing, being unified with that.

Mark.

>  static int __init xen_guest_init(void)
> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
> index 4eeb171..16c6b72 100644
> --- a/arch/arm64/kernel/efi.c
> +++ b/arch/arm64/kernel/efi.c
> @@ -288,6 +288,11 @@ static int __init arm64_enable_runtime_services(void)
>   return 0;
>   }
>  
> + if (efi_enabled(EFI_PARAVIRT)) {
> + pr_info("EFI runtime services access via paravirt.\n");
> + return -1;
> + }
> +
>   pr_info("Remapping and enabling EFI services.\n");
>  
>   mapsize = memmap.map_end - memmap.map;
> -- 
> 2.0.4
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3 20/62] arm/acpi: Add ACPI support for SMP initialization

[Mark Rutland]

On Mon, Jan 04, 2016 at 02:51:51PM +, Stefano Stabellini wrote:
> On Wed, 30 Dec 2015, Shannon Zhao wrote:
> > On 2015/11/30 22:57, Julien Grall wrote:
> > > Hi Shannon,
> > > 
> > > On 17/11/15 09:40, shannon.z...@linaro.org wrote:
> > >> > diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
> > >> > index d800cb6..dede0e1 100644
> > >> > --- a/xen/arch/arm/psci.c
> > >> > +++ b/xen/arch/arm/psci.c
> > >> > @@ -22,6 +22,7 @@
> > >> >  #include 
> > >> >  #include 
> > >> >  #include 
> > >> > +#include 
> > >> >  
> > >> >  /*
> > >> >   * While a 64-bit OS can make calls with SMC32 calling conventions, 
> > >> > for
> > >> > @@ -86,6 +87,9 @@ int __init psci_init_0_1(void)
> > >> >  int ret;
> > >> >  const struct dt_device_node *psci;
> > >> >  
> > >> > +if ( !acpi_disabled )
> > >> > +return -EINVAL;
> > > Please explain in the commit message why PSCI 0.1 is not supported on 
> > > ACPI.
> > 
> > Hi,
> > 
> > I check this again. There are not limitations of supporting PSCI version
> > in ACPI SPEC. It should support PSCI 0.1 as well. But look at the code
> > of linux kernel, it says it only supports PSCI 0.2+.
> > 
> > #define ACPI_FADT_PSCI_COMPLIANT(1) /* 00: [V5+] PSCI 0.2+ is
> > implemented */
> > 
> > So does it need to be consistent with Linux or support PSCI 0.1 in Xen
> > as well?
> 
> I don't think it needs to be consistent with Linux. I would support PSCI
> 0.1 too.

That's not possible, so I don't follow. Prior to 0.2 the function IDs
are not defined.

The FADT has a single bit which describes PSCI 0.2+ being implemented,
and does not describe function IDs.

You must assume PSCI 0.2+ in order to have a set of function IDs to use.

You should also assume the presence of the rest of the mandatory
portions of PSCI 0.2, as these are also required per the combination of
the PSCI spec and the ACPI spec.

Mark.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3 20/62] arm/acpi: Add ACPI support for SMP initialization

[Mark Rutland]

On Mon, Jan 04, 2016 at 03:00:45PM +, Mark Rutland wrote:
> On Mon, Jan 04, 2016 at 02:51:51PM +, Stefano Stabellini wrote:
> > On Wed, 30 Dec 2015, Shannon Zhao wrote:
> > > I check this again. There are not limitations of supporting PSCI version
> > > in ACPI SPEC. It should support PSCI 0.1 as well. But look at the code
> > > of linux kernel, it says it only supports PSCI 0.2+.
> > > 
> > > #define ACPI_FADT_PSCI_COMPLIANT(1)   /* 00: [V5+] PSCI 0.2+ is
> > > implemented */
> > > 
> > > So does it need to be consistent with Linux or support PSCI 0.1 in Xen
> > > as well?
> > 
> > I don't think it needs to be consistent with Linux. I would support PSCI
> > 0.1 too.
> 
> That's not possible, so I don't follow. Prior to 0.2 the function IDs
> are not defined.
> 
> The FADT has a single bit which describes PSCI 0.2+ being implemented,
> and does not describe function IDs.

I've now spotted that this wording is indeed missing from the ACPI
documentation. I believe this is a documentation bug, as the intent was
always for the bit to imply PSCI 0.2+.

Mark.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3 20/62] arm/acpi: Add ACPI support for SMP initialization

[Mark Rutland]

On Wed, Dec 30, 2015 at 11:11:08AM +0800, Shannon Zhao wrote:
> 
> 
> On 2015/11/30 22:57, Julien Grall wrote:
> > Hi Shannon,
> > 
> > On 17/11/15 09:40, shannon.z...@linaro.org wrote:
> >> > diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
> >> > index d800cb6..dede0e1 100644
> >> > --- a/xen/arch/arm/psci.c
> >> > +++ b/xen/arch/arm/psci.c
> >> > @@ -22,6 +22,7 @@
> >> >  #include 
> >> >  #include 
> >> >  #include 
> >> > +#include 
> >> >  
> >> >  /*
> >> >   * While a 64-bit OS can make calls with SMC32 calling conventions, for
> >> > @@ -86,6 +87,9 @@ int __init psci_init_0_1(void)
> >> >  int ret;
> >> >  const struct dt_device_node *psci;
> >> >  
> >> > +if ( !acpi_disabled )
> >> > +return -EINVAL;
> > Please explain in the commit message why PSCI 0.1 is not supported on ACPI.
> 
> Hi,
> 
> I check this again. There are not limitations of supporting PSCI version
> in ACPI SPEC. It should support PSCI 0.1 as well.

I believe that's an oversight in the ACPI documentation, which should
state 0.2+.

There was a deliberate decision that the FADT PSCI flag implied PSCI
0.2+, since prior to PSCI 0.2 function IDs were not well-defined, and
functions crticial for some uses cases did not exist (e.g. AFFINITY_INFO
for kexec-type things).

I don't know why that did not make it into the documentation.

Charles, thoughts?

Mark.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 08/13] Xen: EFI: Parse DT parameters for Xen specific UEFI

[Mark Rutland]

On Tue, Nov 17, 2015 at 12:25:58PM +0100, Ard Biesheuvel wrote:
> On 17 November 2015 at 10:57,   wrote:
> > From: Shannon Zhao 
> >
> > Add a new function to parse DT parameters for Xen specific UEFI just
> > like the way for normal UEFI. Then it could reuse the existing codes.
> >
> > Signed-off-by: Shannon Zhao 
> > ---
> >  drivers/firmware/efi/efi.c | 67 
> > ++
> >  1 file changed, 62 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > index d6144e3..629bd06 100644
> > --- a/drivers/firmware/efi/efi.c
> > +++ b/drivers/firmware/efi/efi.c
> > @@ -24,6 +24,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >
> >  struct efi __read_mostly efi = {
> > .mps= EFI_INVALID_TABLE_ADDR,
> > @@ -488,12 +489,60 @@ static __initdata struct {
> > UEFI_PARAM("MemMap Desc. Version", "linux,uefi-mmap-desc-ver", 
> > desc_ver)
> >  };
> >
> > +static __initdata struct {
> > +   const char name[32];
> > +   const char propname[32];
> > +   int offset;
> > +   int size;
> > +} xen_dt_params[] = {
> > +   UEFI_PARAM("System Table", "xen,uefi-system-table", system_table),
> > +   UEFI_PARAM("MemMap Address", "xen,uefi-mmap-start", mmap),
> > +   UEFI_PARAM("MemMap Size", "xen,uefi-mmap-size", mmap_size),
> > +   UEFI_PARAM("MemMap Desc. Size", "xen,uefi-mmap-desc-size", 
> > desc_size),
> > +   UEFI_PARAM("MemMap Desc. Version", "xen,uefi-mmap-desc-ver", 
> > desc_ver)
> > +};
> > +
> 
> We discussed (and agreed afair) that dropping the "linux," prefix from
> the DT properties was an acceptable change. If we do that, and reuse
> the same names in the xen version (i.e., drop the "xen," prefix), we
> could make this change a *lot* simpler.

Regardless of if we drop the "linux," prefix from the existing strings,
I think we need the "xen," prefix here.

The xen EFI interface comes with additional caveats, and we need to
treat it separately.

Unless you're suggesting that /hypervisor/uefi-* is handled differently
to /chosen/uefi-*?

I think I'd still prefer the "xen," prefix regardless.

Thanks,
Mark.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 1/3] xen/arm: introduce xen_read_wallclock

[Mark Rutland]

> + delta = arch_timer_read_counter();  /* time since system boot */
> + delta += now.tv_sec * (u64)NSEC_PER_SEC + now.tv_nsec;

The arch counter value is not a number of nanoseconds (unless CNTFRQ
reads as 100), so this doesn't look right; the units don't match.

Thanks,
Mark.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v11 5/5] xen/arm: account for stolen ticks

[Mark Rutland]

On Fri, Nov 06, 2015 at 11:41:49AM +, David Vrabel wrote:
> On 06/11/15 11:39, Stefano Stabellini wrote:
> > On Thu, 5 Nov 2015, Mark Rutland wrote:
> >>>  static void xen_percpu_init(void)
> >>>  {
> >>>   struct vcpu_register_vcpu_info info;
> >>> @@ -104,6 +120,8 @@ static void xen_percpu_init(void)
> >>>   BUG_ON(err);
> >>>   per_cpu(xen_vcpu, cpu) = vcpup;
> >>>  
> >>> + xen_setup_runstate_info(cpu);
> >>
> >> Does the runstate memory area get unregsitered when a kernel tears
> >> things down, or is kexec somehow inhibited for xen guests?
> >>
> >> i couldn't spot either happening, but I may have missed it.
> > 
> > I don't think that the runstate memory area needs to be unregistered for
> > kexec, but I am not very knowledgeble on kexec and Xen, CC'ing Vitaly
> > and David.
> 
> There's a whole pile of other state needing to be reset for kexec (event
> channels and grant tables for example).  The guest needs to soft reset
> itself (available in Xen 4.6) before kexec'ing another kernel.
> 
> This soft reset would also including cleaning up this shared memory region.

Ok. So we don't currently have the code kernel-side, but it looks like
it would be relatively simple to add (having just spotted [1]), and
everything should be ready on the Xen side.`

Thanks,
Mark.

[1] https://lkml.org/lkml/2015/9/25/152

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v11 1/5] xen: move xen_setup_runstate_info and get_runstate_snapshot to drivers/xen/time.c

[Mark Rutland]

On Fri, Nov 06, 2015 at 11:11:40AM +, Stefano Stabellini wrote:
> On Thu, 5 Nov 2015, Mark Rutland wrote:
> > Hi,
> > 
> > > +static u64 get64(const u64 *p)
> > > +{
> > > + u64 ret;
> > > +
> > > + if (BITS_PER_LONG < 64) {
> > > + u32 *p32 = (u32 *)p;
> > > + u32 h, l;
> > > +
> > > + /*
> > > +  * Read high then low, and then make sure high is
> > > +  * still the same; this will only loop if low wraps
> > > +  * and carries into high.
> > > +  * XXX some clean way to make this endian-proof?
> > > +  */
> > > + do {
> > > + h = p32[1];
> > > + barrier();
> > > + l = p32[0];
> > > + barrier();
> > > + } while (p32[1] != h);
> > 
> > I realise this is simply a move of existing code, but it may be better
> > to instead have:
> > 
> > do {
> > h = READ_ONCE(p32[1]);
> > l = READ_ONCE(p32[0]);
> > } while (READ_ONCE(p32[1] != h);
> > 
> > Which ensures that each load is a single access (though it almost
> > certainly would be anyway), and prevents the compiler from having to
> > reload any other memory locations (which the current barrier() usage
> > forces).
> 
> I am happy to make these changes, however for code clarity and review
> simplicity I'll keep them on a separate patch (I like code movement to
> remain code movement). I can squash the two patches together when
> committing, if necessary.

Sure, I also prefer to separate code movement from code rework, so that
makes sense to me.

Thanks,
Mark.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v11 5/5] xen/arm: account for stolen ticks

[Mark Rutland]

>  static void xen_percpu_init(void)
>  {
>   struct vcpu_register_vcpu_info info;
> @@ -104,6 +120,8 @@ static void xen_percpu_init(void)
>   BUG_ON(err);
>   per_cpu(xen_vcpu, cpu) = vcpup;
>  
> + xen_setup_runstate_info(cpu);

Does the runstate memory area get unregsitered when a kernel tears
things down, or is kexec somehow inhibited for xen guests?

i couldn't spot either happening, but I may have missed it.

Mark.

> +
>  after_register_vcpu_info:
>   enable_percpu_irq(xen_events_irq, 0);
>   put_cpu();
> @@ -271,6 +289,9 @@ static int __init xen_guest_init(void)
>  
>   register_cpu_notifier(_cpu_notifier);
>  
> + pv_time_ops.steal_clock = xen_stolen_accounting;
> + static_key_slow_inc(_steal_enabled);
> +
>   return 0;
>  }
>  early_initcall(xen_guest_init);
> -- 
> 1.7.10.4
> 
> 
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v11 1/5] xen: move xen_setup_runstate_info and get_runstate_snapshot to drivers/xen/time.c

[Mark Rutland]

Hi,

> +static u64 get64(const u64 *p)
> +{
> + u64 ret;
> +
> + if (BITS_PER_LONG < 64) {
> + u32 *p32 = (u32 *)p;
> + u32 h, l;
> +
> + /*
> +  * Read high then low, and then make sure high is
> +  * still the same; this will only loop if low wraps
> +  * and carries into high.
> +  * XXX some clean way to make this endian-proof?
> +  */
> + do {
> + h = p32[1];
> + barrier();
> + l = p32[0];
> + barrier();
> + } while (p32[1] != h);

I realise this is simply a move of existing code, but it may be better
to instead have:

do {
h = READ_ONCE(p32[1]);
l = READ_ONCE(p32[0]);
} while (READ_ONCE(p32[1] != h);

Which ensures that each load is a single access (though it almost
certainly would be anyway), and prevents the compiler from having to
reload any other memory locations (which the current barrier() usage
forces).

> +
> + ret = (((u64)h) << 32) | l;
> + } else
> + ret = *p;

Likewise, this would be better as READ_ONCE(*p), to force a single
access.

> +
> + return ret;
> +}

> + do {
> + state_time = get64(>state_entry_time);
> + barrier();
> + *res = *state;
> + barrier();

You can also have:

*res = READ_ONCE(*state);

That will which will handle the barriers implicitly.

Thanks,
Mark.

> + } while (get64(>state_entry_time) != state_time);
> +}

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v5 00/22] xen/arm64: Add support for 64KB page in Linux

[Mark Rutland]

> > Just to check, would this be expected to work with a 16K DomU (e.g.
> > [2])?
> > 
> > From a quick scan it looks like the relaxations provided by this series
> > should work so long as PAGE_SIZE % XEN_PAGE_SIZE == 0, assuming I
> > haven't missed something.
> 
> Correct, this series is able to cope with any PAGE_SIZE as long as it's
> a multiple of the granularity used by Xen (i.e 4KB on ARM).

[...]

> > Would any of these require more work to also handle 16K?
> 
> No. It should just boot on Xen as long as the CPU is support 16K
> granularity.

Great to hear, thanks!

Mark.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] efi/libstub/fdt: Standardize the names of EFI stub parameters

[Mark Rutland]

On Sat, Sep 12, 2015 at 12:36:55PM +0100, Daniel Kiper wrote:
> On Fri, Sep 11, 2015 at 05:25:59PM +0100, Mark Rutland wrote:
> > On Fri, Sep 11, 2015 at 01:46:43PM +0100, Daniel Kiper wrote:
> > > On Thu, Sep 10, 2015 at 05:23:02PM +0100, Mark Rutland wrote:
> 
> [...]
> 
> > > > What's troublesome with the boot services?
> > > >
> > > > What can't be simulated?
> > >
> > > How do you want to access bare metal EFI boot services from dom0 if they
> > > were shutdown long time ago before loading dom0 image?
> >
> > I don't want to.
> >
> > I asked "What can't be simulated?" because I assumed everything
> > necessary/mandatory could be simulated without needinng access to any
> > real EFI boot services.
> >
> > As far as I can see all that's necessary is to provide a compatible
> > interface.
> 
> Could you be more precise what do you need? Please enumerate. UEFI spec has
> more than 2500 pages and I do not think that we need all stuff in dom0.
> 
> > > What do you need from EFI boot services in dom0?
> >
> > The ability to call ExitBootServices() and SetVirtualAddressMap() on a
> > _virtual_ address map for _virtual_ services provided by the hypervisor.
> 
> I am confused. Why do you need that? Please remember, EFI is owned and
> operated by Xen hypervisor. dom0 does not have direct access to EFI.

Let's take a step back.

My objection here is to passing the Dom0 kernel properties as if it were
booted with direct access to a full UEFI, then later fixing that up
(when Xen is detected and we apply its hypercall EFI implementation).

If the kernel cannot use EFI natively, why pretend to the kernel that it
can? The hypercall implementation is _not_ EFI (though it provides
access to some services).

The two ways I can see providing Dom0 with EFI services are:

* Have Xen create shims for any services, in which any hypercalls live,
  and pass these to the kernel with a virtual system table. This keeps
  the interface to the kernel the same regardless of Xen.

* Have the kernel detect Xen EFI capability via Xen, without passing the
  usual native EFI parameters. This can then be installed into the
  kernel in a Xen-specific manner, and we know from the outset that
  Xen-specific caveats apply.

As per my original email, I'm not against the renaming of the stub
parameters if we standardise the rest of the details, but I believe
that's orthogonal to the Xen Dom0 case.

Thanks,
Mark.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] efi/libstub/fdt: Standardize the names of EFI stub parameters

[Mark Rutland]

On Fri, Sep 11, 2015 at 01:46:43PM +0100, Daniel Kiper wrote:
> On Thu, Sep 10, 2015 at 05:23:02PM +0100, Mark Rutland wrote:
> > > > C) When you could go:
> > > >
> > > >DT -> Discover Xen -> Xen-specific stuff -> Xen-specific EFI/ACPI 
> > > > discovery
> > >
> > > I take you mean discovering Xen with the usual Xen hypervisor node on
> > > device tree. I think that C) is a good option actually. I like it. Not
> > > sure why we didn't think about this earlier. Is there anything EFI or
> > > ACPI which is needed before Xen support is discovered by
> > > arch/arm64/kernel/setup.c:setup_arch -> xen_early_init()?
> >
> > Currently lots (including the memory map). With the stuff to support
> > SPCR, the ACPI discovery would be moved before xen_early_init().
> >
> > > If not, we could just go for this. A lot of complexity would go away.
> >
> > I suspect this would still be fairly complex, but would at least prevent
> > the Xen-specific EFI handling from adversely affecting the native case.
> >
> > > > D) If you want to be generic:
> > > >EFI -> EFI application -> EFI tables -> ACPI tables -> Xen-specific 
> > > > stuff
> > > >   \--/
> > > >(virtualize these, provide shims to Dom0, but handle
> > > > everything in Xen itself)
> > >
> > > I think that this is good in theory but could turn out to be a lot of
> > > work in practice. We could probably virtualize the RuntimeServices but
> > > the BootServices are troublesome.
> >
> > What's troublesome with the boot services?
> >
> > What can't be simulated?
> 
> How do you want to access bare metal EFI boot services from dom0 if they
> were shutdown long time ago before loading dom0 image?

I don't want to.

I asked "What can't be simulated?" because I assumed everything
necessary/mandatory could be simulated without needinng access to any
real EFI boot services.

As far as I can see all that's necessary is to provide a compatible
interface.

> What do you need from EFI boot services in dom0?

The ability to call ExitBootServices() and SetVirtualAddressMap() on a
_virtual_ address map for _virtual_ services provided by the hypervisor.
A console so that I can log things early on.

Mark.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] efi/libstub/fdt: Standardize the names of EFI stub parameters

[Mark Rutland]

> It feels like this discussion is going in circles.
> 
> When we discussed this six months ago, we already concluded that,
> since UEFI is the only specified way that the presence of ACPI is
> advertised on an ARM system, we need to emulate UEFI to some extent.

My understanding from the last time I was present at such a discussion
was that the emulation was complete from the kernel's PoV (i.e. no
special case handling was required). 

Evidently I misunderstood.

One of the main points of rationale for requiring EFI was that we'd have
a well-defined system state as per the EFI (and ACPI) standards. We'd
know we had the EFI memory map, services, etc (with the memory map and
configuration tables being the most important elements). We didn't want
to have to try to reconcile a DT memory map and ACPI, for instance.

That is somewhat (though admitedly not entirely) broken if we require a
set of rules to be applied beyond what the standards mandate.  That is
broken if we require a non-standard set of rules to be applied, and
limits what we can do in the !Xen case.

> So we need the EFI system table to expose the UEFI configuration table
> that carries the ACPI root pointer.
> 
> Since ACPI support also relies on the UEFI memory map (I think?), we
> need that as well.
> 
> These two items are exactly what we pass via the UEFI DT properties,
> so we should indeed promote the current de-facto binding to a proper
> binding, and renaming the properties makes sense in that context.

I agree that we need to sort these out.

> I agree that this should also include a description of the expected
> state of the firmware, i.e., that ExitBootServices() has been called,
> and that the memory map has been populated with virtual address, which
> have been installed using SetVirtualAddressMap() if they differ from
> the physical addresses. (The current implementation on the kernel side
> is perfectly capable of dealing with a 1:1 mapping).
> 
> Beyond that, there is no point in pretending to be a full UEFI
> implementation, imo. Boot services are not required, nor are runtime
> services (only the current EFI init code on arm needs to be modified
> to deal with a NULL runtime services pointer)

I'm not keen on this because it involves applying Xen-specific caveats
atop of what the UEFI spec says (e.g. as runtime services might be NULL
under Xen). As the spec and Xen evolve, those caveats shift, and that's
going to be fragile for all users regardleses of Xen.

If Xen needs special-casing, then I'd rather that Xen were detected
first and provided us with what it knows is safe for us to use, rather
than we tip-toe around until we're sure Xen isn't present and/or doesn't
need additional constraints met.

Thanks,
Mark.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] efi/libstub/fdt: Standardize the names of EFI stub parameters

[Mark Rutland]

> >> Considering that the EFI support is just for Dom0, and Dom0 (at
> >> the time) had to be PV anyway, it was the more natural solution to
> >> expose the interface via hypercalls, the more that this allows better
> >> control over what is and primarily what is not being exposed to
> >> Dom0. With the wrapper approach we'd be back to the same
> >> problem (discussed elsewhere) of which EFI version to surface: The
> >> host one would impose potentially missing extensions, while the
> >> most recent hypervisor known one might imply hiding valuable
> >> information from Dom0. Plus there are incompatible changes like
> >> the altered meaning of EFI_MEMORY_WP in 2.5.
> > 
> > I'm not sure I follow how hypercalls solve any impedance mismatch here;
> > you're still expecting Dom0 to call up to Xen in order to perform calls,
> > and all I suggested was a different location for those hypercalls.
> > 
> > If Xen is happy to make such calls blindly, why does it matter if the
> > hypercall was in the kernel binary or an external shim?
> 
> Because there could be new entries in SystemTable->RuntimeServices
> (expected and blindly but validly called by the kernel). Even worse
> (because likely harder to deal with) would be new fields in other
> structures.

Any of these could cause Xen to blow up, while Xen could always provide
a known-safe (but potentially sub-optimal) view to the kernel by
default.

> > Incompatible changes are a spec problem regardless of how this is
> > handled.
> 
> Not necessarily - we don't expose the memory map (we'd have to
> if we were to mimic EFI for Dom0), and hence the mentioned issue
> doesn't exist in our model.

We have to expose _some_ memory map, so I don't follow this point.

Mark.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] efi/libstub/fdt: Standardize the names of EFI stub parameters

[Mark Rutland]

Hi,

I'm not necessarily opposed to the renaming, but I think that this is
the least important thing to standardize for this to work.

On Thu, Sep 10, 2015 at 09:41:56AM +0100, Shannon Zhao wrote:
> From: Shannon Zhao 
> 
> These EFI stub parameters are used to internal communication between EFI
> stub and Linux kernel and EFI stub creates these parameters. But for Xen
> on ARM when booting with UEFI, Xen will create a minimal DT providing
> these parameters for Dom0 and Dom0 is not only Linux kernel, but also
> other OS (such as FreeBSD) which will be used in the future.

Currently the Linux EFI stub and kernel have a symbiotic relationship,
because they're intimately coupled and we don't have kexec (yet) on EFI
platforms to loosen that coupling.

If an agent other than the (kernel-specific) stub is going to provide
this to the kernel, then we need more specified than just the property
names.

That at least includes the following:

* The state of boot services (we currently have the EFI stub call
  ExitBootServices(), and I believe this is crucial to the plan for
  kexec).

* The state of the address map (we currently have the EFI stub call
  SetVirtualAddressMap()).

* The virtual address range(s) that SetVirtualAddressMap() may map
  elements to (this logic is currently in the EFI stub, and this matches
  the expectations of the kernel that it is tied to).

> So here we plan to standardize the names by dropping the prefix
> "linux," and make them common to other OS. Also this will not break
> the compatibility since these parameters are used to internal
> communication between EFI stub and kernel.

For the moment this is true, but will not be once we have kexec, so
there's a dependency (or anti-dependency) there.

> Signed-off-by: Shannon Zhao 
> ---
> Look at [1] for the discussion about this in Xen ML. The purpose of this
> patch is to standardize the names to make Linux ARM kernel work on Xen
> with UEFI. Also it hopes other OS(e.g. FreeBSD), which will be used as
> Dom0 on Xen, could support this mechanism as well.
> 
> [1]http://lists.xenproject.org/archives/html/xen-devel/2015-08/msg02250.html

Per this post, it looks like to pass a DTB to the kernel Xen already
needs to know it's a Linux kernel...

I wasn't aware that there was a common standard for arm(64) kernels
other than a PE/COFF EFI application.

Does Xen not talk to EFI itself and/or give the kernel a virtual EFI
interface?

Thanks,
Mark.

>  Documentation/arm/uefi.txt | 10 +-
>  drivers/firmware/efi/efi.c | 10 +-
>  drivers/firmware/efi/libstub/fdt.c | 10 +-
>  3 files changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/Documentation/arm/uefi.txt b/Documentation/arm/uefi.txt
> index d60030a..8c83243 100644
> --- a/Documentation/arm/uefi.txt
> +++ b/Documentation/arm/uefi.txt
> @@ -45,18 +45,18 @@ following parameters:
>  
> 
>  Name  | Size   | Description
>  
> 
> -linux,uefi-system-table   | 64-bit | Physical address of the UEFI System 
> Table.
> +uefi-system-table | 64-bit | Physical address of the UEFI System 
> Table.
>  
> 
> -linux,uefi-mmap-start | 64-bit | Physical address of the UEFI memory map,
> +uefi-mmap-start   | 64-bit | Physical address of the UEFI memory map,
>|| populated by the UEFI GetMemoryMap() 
> call.
>  
> 
> -linux,uefi-mmap-size  | 32-bit | Size in bytes of the UEFI memory map
> +uefi-mmap-size| 32-bit | Size in bytes of the UEFI memory map
>|| pointed to in previous entry.
>  
> 
> -linux,uefi-mmap-desc-size | 32-bit | Size in bytes of each entry in the UEFI
> +uefi-mmap-desc-size   | 32-bit | Size in bytes of each entry in the UEFI
>|| memory map.
>  
> 
> -linux,uefi-mmap-desc-ver  | 32-bit | Version of the mmap descriptor format.
> +uefi-mmap-desc-ver| 32-bit | Version of the mmap descriptor format.
>  
> 
>  linux,uefi-stub-kern-ver  | string | Copy of linux_banner from build.
>  
> 
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index d6144e3..3878715 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -481,11 +481,11 @@ static __initdata struct {
>  

Re: [Xen-devel] [PATCH] efi/libstub/fdt: Standardize the names of EFI stub parameters

[Mark Rutland]

> > Does Xen not talk to EFI itself and/or give the kernel a virtual EFI
> > interface?
> 
> Xen talks to EFI itself but the interface provided to dom0 is somewhat
> different: there are no BootServices (Xen calls ExitBootServices before
> running the kernel), and the RuntimeServices go via hypercalls (see
> drivers/xen/efi.c).

That's somewhat hideous; a non Xen-aware OS wouild presumably die if
trying to use any runtime services the normal way? I'm not keen on
describing things that the OS cannot use.

Why can't Xen give a virtual EFI interface to Dom0 / guests? e.g.
create pages of RuntimeServicesCode that are trivial assembly shims
doing hypercalls, and plumb these into the virtual EFI memory map and
tables?

That would keep things sane for any guest, allow for easy addition of
EFI features, and you could even enter the usual EFI entry point,
simulate ExitBootServices(), SetVirtualAddressMap(), and allow the guest
to make things sane for itself...

Mark.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] efi/libstub/fdt: Standardize the names of EFI stub parameters

[Mark Rutland]

On Thu, Sep 10, 2015 at 12:37:57PM +0100, Stefano Stabellini wrote:
> On Thu, 10 Sep 2015, Mark Rutland wrote:
> > > > Does Xen not talk to EFI itself and/or give the kernel a virtual EFI
> > > > interface?
> > > 
> > > Xen talks to EFI itself but the interface provided to dom0 is somewhat
> > > different: there are no BootServices (Xen calls ExitBootServices before
> > > running the kernel), and the RuntimeServices go via hypercalls (see
> > > drivers/xen/efi.c).
> > 
> > That's somewhat hideous; a non Xen-aware OS wouild presumably die if
> > trying to use any runtime services the normal way? I'm not keen on
> > describing things that the OS cannot use.
>  
> I agree that is somewhat hideous, but a non-Xen aware OS traditionally
> has never been able to even boot as Dom0. On ARM it can, but it still
> wouldn't be very useful (one couldn't use it to start other guests).

Sure, but it feels odd to provide the usual information in this manner
if it cannot be used. If you require Xen-specific code to make things
work, I would imagine this information could be dciscovered in a
Xen-specific manner.

> > Why can't Xen give a virtual EFI interface to Dom0 / guests? e.g.
> > create pages of RuntimeServicesCode that are trivial assembly shims
> > doing hypercalls, and plumb these into the virtual EFI memory map and
> > tables?
> > 
> > That would keep things sane for any guest, allow for easy addition of
> > EFI features, and you could even enter the usual EFI entry point,
> > simulate ExitBootServices(), SetVirtualAddressMap(), and allow the guest
> > to make things sane for itself...
> 
> That's the way it was done on x86 and now we have common code both in
> Linux (drivers/xen/efi.c) and Xen (xen/common/efi) which implement this
> scheme.

This code is not currently used on arm. It might live in a location
where it may be shared, but that doesn't mean that it's common code yet.

> Switching to a different solution for ARM, would mean diverging
> with x86, which is not nice, or reimplementing the x86 solution too,
> which is expensive.
> 
> BTW I think that the idea you proposed was actually considered at the
> time and deemed hard to implement, if I recall correctly.

I appreciate that divergence is painful. We already diverge in other
respects (e.g. lack of PV page tables) because things that used to be
the case on x86 never applied to ARM.

It would be interesting to see why that was the case for x86, and
whether that applies to ARM.

> In any case this should be separate from the shim ABI discussion.

I disagree; I think this is very much relevant to the ABI discussion.
That's not to say that I insist on a particular approach, but I think
that they need to be considered together.

Thanks,
Mark.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] efi/libstub/fdt: Standardize the names of EFI stub parameters

[Mark Rutland]

On Thu, Sep 10, 2015 at 01:55:25PM +0100, Jan Beulich wrote:
> >>> On 10.09.15 at 13:37, <stefano.stabell...@eu.citrix.com> wrote:
> > On Thu, 10 Sep 2015, Mark Rutland wrote:
> >> Why can't Xen give a virtual EFI interface to Dom0 / guests? e.g.
> >> create pages of RuntimeServicesCode that are trivial assembly shims
> >> doing hypercalls, and plumb these into the virtual EFI memory map and
> >> tables?
> >> 
> >> That would keep things sane for any guest, allow for easy addition of
> >> EFI features, and you could even enter the usual EFI entry point,
> >> simulate ExitBootServices(), SetVirtualAddressMap(), and allow the guest
> >> to make things sane for itself...
> > 
> > That's the way it was done on x86 and now we have common code both in
> > Linux (drivers/xen/efi.c) and Xen (xen/common/efi) which implement this
> > scheme.  Switching to a different solution for ARM, would mean diverging
> > with x86, which is not nice, or reimplementing the x86 solution too,
> > which is expensive.
> > 
> > BTW I think that the idea you proposed was actually considered at the
> > time and deemed hard to implement, if I recall correctly.
> 
> Considering that the EFI support is just for Dom0, and Dom0 (at
> the time) had to be PV anyway, it was the more natural solution to
> expose the interface via hypercalls, the more that this allows better
> control over what is and primarily what is not being exposed to
> Dom0. With the wrapper approach we'd be back to the same
> problem (discussed elsewhere) of which EFI version to surface: The
> host one would impose potentially missing extensions, while the
> most recent hypervisor known one might imply hiding valuable
> information from Dom0. Plus there are incompatible changes like
> the altered meaning of EFI_MEMORY_WP in 2.5.

I'm not sure I follow how hypercalls solve any impedance mismatch here;
you're still expecting Dom0 to call up to Xen in order to perform calls,
and all I suggested was a different location for those hypercalls.

If Xen is happy to make such calls blindly, why does it matter if the
hypercall was in the kernel binary or an external shim?

Incompatible changes are a spec problem regardless of how this is
handled.

Thanks,
Mark.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] efi/libstub/fdt: Standardize the names of EFI stub parameters

[Mark Rutland]

On Thu, Sep 10, 2015 at 02:52:25PM +0100, Stefano Stabellini wrote:
> On Thu, 10 Sep 2015, Mark Rutland wrote:
> > On Thu, Sep 10, 2015 at 12:37:57PM +0100, Stefano Stabellini wrote:
> > > On Thu, 10 Sep 2015, Mark Rutland wrote:
> > > > > > Does Xen not talk to EFI itself and/or give the kernel a virtual EFI
> > > > > > interface?
> > > > > 
> > > > > Xen talks to EFI itself but the interface provided to dom0 is somewhat
> > > > > different: there are no BootServices (Xen calls ExitBootServices 
> > > > > before
> > > > > running the kernel), and the RuntimeServices go via hypercalls (see
> > > > > drivers/xen/efi.c).
> > > > 
> > > > That's somewhat hideous; a non Xen-aware OS wouild presumably die if
> > > > trying to use any runtime services the normal way? I'm not keen on
> > > > describing things that the OS cannot use.
> > >  
> > > I agree that is somewhat hideous, but a non-Xen aware OS traditionally
> > > has never been able to even boot as Dom0. On ARM it can, but it still
> > > wouldn't be very useful (one couldn't use it to start other guests).
> > 
> > Sure, but it feels odd to provide the usual information in this manner
> > if it cannot be used. If you require Xen-specific code to make things
> > work, I would imagine this information could be dciscovered in a
> > Xen-specific manner.
> 
> We need ACPI (or Device Tree) to find that Xen is available on the
> platform, so we cannot use Xen-specific code to get the ACPI tables.

I don't understand. The proposition already involves passing a custom DT
to the OS, implying that Xen knows how to boot that OS, and how to pass
it a DTB.

Consider:

A) In the DT-only case, we go:

   DT -> Discover Xen -> Xen-specific stuff


B) The proposition is that un the ACPI case we go:

   DT -> EFI tables -> ACPI tables -> Discover Xen -> Xen-specific stuff -> 
override EFI/ACPI stuff
 \---/
  (be really cautious here)


C) When you could go:

   DT -> Discover Xen -> Xen-specific stuff -> Xen-specific EFI/ACPI discovery


D) If you want to be generic:
   EFI -> EFI application -> EFI tables -> ACPI tables -> Xen-specific stuff
  \--/
   (virtualize these, provide shims to Dom0, but handle
everything in Xen itself)


E) Partially-generic option:
   EFI -> EFI application -> Xen detected by registered GUID -> Xen-specific 
EFI bootloader stuff -> OS in Xen-specific configuration


> > > In any case this should be separate from the shim ABI discussion.
> > 
> > I disagree; I think this is very much relevant to the ABI discussion.
> > That's not to say that I insist on a particular approach, but I think
> > that they need to be considered together.
> 
> Let's suppose Xen didn't expose any RuntimeServices at all, would that
> make it easier to discuss about the EFI stub parameters?

It would simply the protocol specific to Xen, certainly.

> In the grant scheme of things, they are not that important, as Ian
> wrote what is important is how to pass the RSDP.

Unfortunately we're still going to have to care about this eventually,
even if for something like kexec. So we still need to spec out the state
of things if this is going to be truly generic.

Thanks,
Mark.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] efi/libstub/fdt: Standardize the names of EFI stub parameters

[Mark Rutland]

> > C) When you could go:
> > 
> >DT -> Discover Xen -> Xen-specific stuff -> Xen-specific EFI/ACPI 
> > discovery
> 
> I take you mean discovering Xen with the usual Xen hypervisor node on
> device tree. I think that C) is a good option actually. I like it. Not
> sure why we didn't think about this earlier. Is there anything EFI or
> ACPI which is needed before Xen support is discovered by
> arch/arm64/kernel/setup.c:setup_arch -> xen_early_init()?

Currently lots (including the memory map). With the stuff to support
SPCR, the ACPI discovery would be moved before xen_early_init().

> If not, we could just go for this. A lot of complexity would go away.

I suspect this would still be fairly complex, but would at least prevent
the Xen-specific EFI handling from adversely affecting the native case.

> > D) If you want to be generic:
> >EFI -> EFI application -> EFI tables -> ACPI tables -> Xen-specific stuff
> >   \--/
> >(virtualize these, provide shims to Dom0, but handle
> > everything in Xen itself)
> 
> I think that this is good in theory but could turn out to be a lot of
> work in practice. We could probably virtualize the RuntimeServices but
> the BootServices are troublesome.

What's troublesome with the boot services?

What can't be simulated?

> > E) Partially-generic option:
> >EFI -> EFI application -> Xen detected by registered GUID -> 
> > Xen-specific EFI bootloader stuff -> OS in Xen-specific configuration
> > 
> > 
> > > > > In any case this should be separate from the shim ABI discussion.
> > > > 
> > > > I disagree; I think this is very much relevant to the ABI discussion.
> > > > That's not to say that I insist on a particular approach, but I think
> > > > that they need to be considered together.
> > > 
> > > Let's suppose Xen didn't expose any RuntimeServices at all, would that
> > > make it easier to discuss about the EFI stub parameters?
> > 
> > It would simply the protocol specific to Xen, certainly.
> > 
> > > In the grant scheme of things, they are not that important, as Ian
> > > wrote what is important is how to pass the RSDP.
> > 
> > Unfortunately we're still going to have to care about this eventually,
> > even if for something like kexec. So we still need to spec out the state
> > of things if this is going to be truly generic.
>  
> Fair enough. My position is that if we restrict this to RuntimeServices,
> it might be possible, but I still prefer C).

Regardless of what we do we still need a well-defined state here, which
brings us back to the initial problem eventually.

Thanks,
Mark.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH for-4.6] efi: introduce efi_arch_flush_dcache_area

[Mark Rutland]

On Mon, Sep 07, 2015 at 05:06:36PM +0100, Ian Campbell wrote:
> On Mon, 2015-09-07 at 16:08 +0100, Stefano Stabellini wrote:
> > On Mon, 7 Sep 2015, Jan Beulich wrote:
> > > > > > On 07.09.15 at 16:13,  wrote:
> > > > Objects loaded by FileHandle->Read need to be flushed to dcache,
> > > > otherwise copy_from_paddr will read stale data when copying the 
> > > > kernel,
> > > > causing a failure to boot.
> > > 
> > > I have to admit that I'm a little puzzled by this description: If this
> > > was a general requirement (which is how it reads to me)
> > 
> > Yes, it is
> > 
> > 
> > > why does > ->Read() not do the necessary flushing? Or if it's not a
> > > general requirement, perhaps the description could be changed to make
> > > clear what exact dependency exists that does not exist for all users
> > > of ->Read()?
> > 
> > It is a general requirement: anything that could be accessed without a
> > cacheable mapping needs to be flushed. Unfortunately the UEFI spec is
> > not clear enough on who should do the flushing on what, and in fact the
> > Forum is working on improving it
> 
> I might be wrong and/or misremembering but I think this stems partly from
> the fact that the ARM UEFI stub in Xen needs to turn off caches (and
> paging?) before jumping to the usual Xen entry point.
> 
> Then when caches come back on you get inconsistencies because of stale
> stuff in the cache which suddenly reappears etc.

It's more to do with using inconsistent attributes than about whether
the caches are enabled -- at both points the kernel image is
manipulated, SCTLR_EL2.{C,M} == {1,1}.

EFI_FILE_PROTOCOL.Read() may place data into the caches without updating
memory because of coherent IO or simply because it copies to a cacheable
alias. If Xen were to modify the image in any way (e.g. decompressing
it), it would only update the cached copy.

Later kernel_zimage_load calls copy_from_paddr, which uses a
non-cacheable alias, bypassing the caches and going straight to memory.

Even if the two aliases were in use simultaneously, they wouldn't be
coherent with each other.

For more background, Marc Zyngier did a good talk at KVM Forum regarding
the usual behaviour of the caches [1,2].

Mark.

[1] http://events.linuxfoundation.org/sites/events/files/slides/slides_10.pdf
[2] https://www.youtube.com/watch?v=A_zCxzpxzmE

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel