Re: [Xen-devel] Critique of the Xen Security Process

[chris]

+1... so many great points here that ive thought many times its almost as
if i could have written it

great post!

chris

On Fri, Nov 6, 2015 at 12:22 PM, Joanna Rutkowska <
joa...@invisiblethingslab.com> wrote:

> -BEGIN PGP SIGNED MESSAGE-
> Hash: SHA1
>
> Hello,
>
> Recently Xen has released the XSA-148 advisory [1] addressing a fatal bug
> in the
> hypervisor. The bug has been lurking there for the last 7 years! We, the
> Qubes
> OS Project, have commented on this in our Security Bulletin #22 [2]. And
> far
> from enthusiastic commentary that was (FWIW, it was me who wrote this QSB,
> as
> evidenced in the commits log, in case some from the Xen community would
> like to
> direct their rage towards a particular human being ;) Ian Jackson then
> wrote a
> response on the Xen blog [3]. I was then asked to share some more thoughts
> about
> how I thought Xen could actually improve its security process [4]. So, I
> share
> some these below:
>
> 1. First of all, I wish Xen was somehow more defensively coded. To provide
> some
> examples:
>
> a. In XSA-109 [5] there was a problem with the hypervisor dereferencing a
> NULL
> pointer. The problem was fixed by the Xen Security Team by applying a patch
> which (hopefully) made sure the execution path that lead to this NULL
> pointer
> dereferencing code was never taken. Back then I suggested (on the Xen
> pre-disclosure list) to make this patch more explicit though:
>
> > On Wed, Jan 21, 2015 at 02:31:51PM +0100, Joanna Rutkowska wrote:
> > (...)
> >>
> >> Wouldn't it be prudent to also check if:
> >>
> >> (v->arch.paging.mode>{write_guest_entry,cmpxchg_guest_entry} != NULL)
> >>
> >> ... in the two affected functions, just before derefing these function
> >> pointers?
> >>
> >> Going even a step further: how about replacing all
> >> function-pointer-based calls with macros that first validates the
> >> pointer before derefing it? At least when the system doesn't have SMEP?
> >>
>
> ...to which I got a reply from one of the Xen Security Team engineers that
> the
> above might perhaps be justified in debug builds only, followed by a
> standard:
> "feel free to contribute a patch".
>
> b. The XSA-123 [6] was another critical security bug in Xen, this time
> resulting
> from one of the hypervisor developer's fetish to use an absolutely
> confusing
> construct in order to save a few modest bytes in a structure which might
> have
> been allocated by the system maybe a few tens of times at best. Even more
> worrying was the way how Xen Security Team decided to fix the bug: again by
> modifying some condition in the code further up the execution path, with
> the
> hopes that this time they would ensure this puzzling construct would
> always be
> used properly. We wrote more about this in our QSB #18 [7].
>
> c. Finally, the way how Xen fixed the recent XSA-148 looks also very
> reactive,
> IMHO. With a bug of this calibre, I would expect Xen to carefully review
> and
> augment all its PV memory virtualization code with additional checks
> (ASSERTs),
> ensuring certain invariants are always satisfied. Such as e.g. that none
> of the
> pages containing PDEs or PTEs are becoming writeable by the VM.
>
> I can't help but have a feeling that some of the Xen developers seem to be
> overconfident in their belief they can fully understand all the possible
> execution paths in their code. Well, the XSAs quoted above are an
> indisputable
> prove that this is not quite always the case. Realizing that, each
> developer by
> themselves, might be a great step towards a more secure hypervisor...
>
> 2. Another security-related aspect of the Xen project is how it totally
> ignores
> problems related to the build process security. Those who don't believe me
> should grep the sources for wget, which is now disguised as "FETCHER" shell
> variable... (so grep for "FETCHER" string)
>
> I feel embarrassed that I need to explain, at the end of 2015, why the
> build
> process of any serious software project should not blindly download
> unsigned
> components (sources) from the Internet, especially if it is about to
> execute
> Makefiles from these components a moment later... Come on, guys!
>
> (Of course we have been forced to get around this gapping security whole in
> Qubes OS [8] ourselves, sadly with a method that is not well suited for
> upstreaming).
>
> 3. Another thing is, of course: stop adding features to the core
> hypervisor. We
> really need Xen to finally mature, stabilize, and for its development
>

Re: [Xen-devel] [qubes-devel] Re: Critique of the Xen Security Process

[chris]

This is true but I can see her point of view as someone with a product that
is based on the open source core. There have been a lot of very drastic
changes that would cause someone with a product to have to do a significant
amount of re-engineering. Also how many different toolstacks have their
been?

I also see both sides but Joanna definitely has some very good points

chris

On Mon, Nov 9, 2015 at 9:46 PM, Radoslaw Szkodzinski 
wrote:

> As usual. Security, performance, convenience, price. Pick any mixture.
>
> As is usual for most software, developer convenience trumps most other
> considerations. I include ease of generating nice papers and jobs
> under developer convenience.
> Big players are much more concerned about performance, which saves
> money on machines you need to buy. (Note the resources poured into
> tmem, which is very iffy from security standpoint.)
> Even if there was a sudden security drive for Xen, it's a huge and
> probably fruitless task. Xen is not designed top down so you get no
> nice overview to check main assumptions. This is also why Xen is
> written in C and assembly, not in anything easier to use.
>
> Likewise security-oriented microkernels such as seL4 push drivers to
> "userspace" making it convenient for their developers, at the cost of
> performance and security, since they are now Somebody Else's Problem.
> Surprisingly, seL4 seems to be the only active and working
> non-research project that happens to have security as main aim, and
> they don't even implement many of features that are "required" for
> modern secure design, say, IOMMU...
> They want you to write your drivers as automated proofs instead.
> Meaning cheap developers will not be able to even lift a finger in
> writing those, since you need to learn Isabelle DSL and understand
> what is required in order to write a working driver with a good API,
> then also understand and model features of underlying hardware. This
> requires good mathematical background, as in actual CS, not just
> writing code.
>
> What seL4 folks are actually doing (have done, even) is writing a
> potent compiler between both C and theorem proving language, plus a
> set of theorems describing assumptions made on underlying hardware and
> security features. Very simple, just some 250k lines of theorems to
> prove correctness of 7k lines of C code.
>
> Good luck trying to port Qubes to their architecture. It's not
> impossible, but it is quite a task. So many theorems to wade through
> and understand, not to mention a whole new programming language.
> You'd get to add well defined isolation primitives to their theorem
> prover, then write a parallel proof to capDL kernel (ouch) or extend
> capDL with such isolation features and requisite APIs.
> Again, developer convenience suffers and have too high barrier of
> entry, and you won't get any code written or it will get too expensive
> in terms of time, skill and therefore money.
>
> As to other attempts...
> Python code? Forget about it, Python is thoroughly undefined behavior
> with no truly defined memory model. :) Even C++11 is better there.
>
> Rust? Rust mechanisms are undefined in terms of timing, which will
> result in unbounded execution time (hangs) when actual hardware is
> involved - there is no way to describe timing behavior of hardware in
> it in such a way that these properties are verified at higher layers.
> They attempt concurrency but the primitives are not bound to actual
> real time and hardware. There is no library for bare metal
> programming. Memory allocations are assumed to never fail - and many
> other things as well. (Does this sound like some other "never fails"
> software we're depending upon?)
>
> Trying to write a secure kernel in an unsafe language is akin to
> trying to bail a sinking ship with a spoon. It can theoretically be
> done, just not in practice. And actual low-level and safe languages
> are surprisingly rare.
>
> Best regards,
> R.
>
> ___
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH] kernel-parameters: document earlycon=xenboot

[Chris Patterson]

Add earlycon=xenboot option to Documentation/kernel-parameters.txt.

Signed-off-by: Chris Patterson 
---
 Documentation/kernel-parameters.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/kernel-parameters.txt 
b/Documentation/kernel-parameters.txt
index ecc74fa..e01ec39 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -1078,6 +1078,8 @@ bytes respectively. Such letter suffixes can also be 
entirely omitted.
address. The serial port must already be setup
and configured. Options are not yet supported.
 
+   xenboot Use Xen hypervisor console for early console.
+
earlyprintk=[X86,SH,BLACKFIN,ARM,M68k]
earlyprintk=vga
earlyprintk=efi
-- 
2.1.4


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

Re: [Xen-devel] [PATCH] kernel-parameters: document earlycon=xenboot

[Chris Patterson]

On Tue, Apr 5, 2016 at 9:49 AM, Konrad Rzeszutek Wilk
 wrote:
> On Mon, Apr 04, 2016 at 10:48:11PM -0400, Chris Patterson wrote:
>> Add earlycon=xenboot option to Documentation/kernel-parameters.txt.
>>
>
> But it is not true.
>
> I tried this on x86: "earlycon=xenboot console=tty0"
>

I can investigate further, but hopefully Stefano can chime in more
definitively on what is supported.

I used "earlycon=xenboot console=hvc0" with success for dom0 on my
ARM64 platform.

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

Re: [Xen-devel] [PATCH] kernel-parameters: document earlycon=xenboot

[Chris Patterson]

On Tue, Apr 5, 2016 at 12:42 PM, Konrad Rzeszutek Wilk
 wrote:
> On Tue, Apr 05, 2016 at 11:43:36AM -0400, Chris Patterson wrote:
>> On Tue, Apr 5, 2016 at 9:49 AM, Konrad Rzeszutek Wilk
>>  wrote:
>> > On Mon, Apr 04, 2016 at 10:48:11PM -0400, Chris Patterson wrote:
>> >> Add earlycon=xenboot option to Documentation/kernel-parameters.txt.
>> >>
>> >
>> > But it is not true.
>> >
>> > I tried this on x86: "earlycon=xenboot console=tty0"
>> >
>>
>> I can investigate further, but hopefully Stefano can chime in more
>> definitively on what is supported.
>>
>> I used "earlycon=xenboot console=hvc0" with success for dom0 on my
>> ARM64 platform.
>
> Perhaps the documentation should also have [ARM], like some of the
> other ones?

Absolutely.  I will test some combinations and be explicit about what
should work in v2.  Thanks for the feedback.

Cheers,
-Chris

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

Re: [Xen-devel] Question about running Xen on NVIDIA Jetson-TK1

[Chris Patterson]

On Tue, May 17, 2016 at 9:37 AM, Konrad Rzeszutek Wilk
 wrote:
>> - The serial controller on the Tegra SoCs doesn't behave in the same
>> was as most NS16550-compatibles; it actually adheres to the NS16550
>> spec a little more rigidly than most compatible controllers. A
>> coworker (Chris Patterson, cc'd) figured out what was going on; from
>> what I understand, most 16550s generate the "transmit ready" interrupt
>> once, when the device first can accept new FIFO entries. Both the
>> original 16550 and the Tegra implementation generate the "transmit
>> ready" interrupt /continuously/ when there's space available in the
>> FIFO, slewing the CPU with a stream of constant interrupts.
>
> That may also be an issue on x86 I would think.
>

On the x86 serial I have tested, it appears that the THRE interrupt
goes low after the IIR read reset, and doesn't re-assert until
UART_IER is flipped (or data is transmitted).  On the Tegra, the
interrupt persists (or immediately re-asserts) after the IIR read
reset.  I suppose this could be a problem on x86 if this behaviour
exists in another uart.

> Is there some simple 16550 'fix' for this? As in reprogram it
> to not be soo ready?

Yes, I do have a patch in my queue to address this. It simply
implements the start_tx and stop_tx hooks and sets or masks
UART_IER_ETHREI accordingly.  I believe this behaviour is consistent
with the Linux kernel.  I have done some limited testing on it with
both x86 and the Tegra (with additional patches for Tegra support) and
things appear to work well. I can RFC it when our employer gives us
approval.  Apologies for the delay.

Have a good day!
-Chris

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

Re: [Xen-devel] [PATCH 4/6] xen/arm: platforms: Add Tegra platform to support basic IRQ routing

[Chris Patterson]

>> +static const char * const tegra_dt_compat[] __initconst =
>> +{
>> +"nvidia,tegra120",  /* Tegra K1 */
>
> This is still tegra120 (not tegra124), is that intended? If so, it is
> still missing from arch/arm*/boot/dts. Do you have a pointer?

It was not intended; thank you for catching it. I must have lost that
fixup somewhere along the way...

> Also, do we need both tegra_dt_compat and tegra_interrupt_compat? Can we
> keep only one?

The purpose of tegra_interrupt_compat is to maintain a tegra-specific
whitelist of interrupt controllers we know how to route.  Presumably,
there may be custom boards out there that may have additional
interrupt routing capabilities that this patch set would not support
as-is.  I'm not sure of an appropriate way to maintain that logic and
merge them.  However, I am certainly open to suggestion, if you have
any ideas.

Thanks for the review!
-Chris

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

Re: [Xen-devel] [PATCH 1/6] xen/arm: platforms: Add earlyprintk and serial support for Tegra boards.

[Chris Patterson]

Will split patches & fix for v2, thanks!

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

Re: [Xen-devel] [refpolicy] [refpolicy SELinux PATCH 2/2] kernel/xen: Add map permission to the dev_rw_xen

[Chris PeBenito]

On 10/09/2017 11:53 AM, Konrad Rzeszutek Wilk via refpolicy wrote:

From: Konrad Rzeszutek Wilk 

type=AVC msg=audit(1504637347.487:280): avc:  denied  { map } for  pid=857 comm="xenconsoled" 
path="/dev/xen/privcmd" dev="devtmpfs" ino=16289 
scontext=system_u:system_r:xenconsoled_t:s0

Without this we can't use xenconsole (client) to
talk to xenconsoled (server).

Signed-off-by: Konrad Rzeszutek Wilk 
---
  policy/modules/kernel/devices.if | 1 +
  1 file changed, 1 insertion(+)

diff --git a/policy/modules/kernel/devices.if b/policy/modules/kernel/devices.if
index 4c47709ff..c9071df8f 100644
--- a/policy/modules/kernel/devices.if
+++ b/policy/modules/kernel/devices.if
@@ -4984,6 +4984,7 @@ interface(`dev_rw_xen',`
')
  
  	rw_chr_files_pattern($1, device_t, xen_device_t)

+   allow $1 xen_device_t:chr_file map;
  ')
  
  ####


Merged.

--
Chris PeBenito

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

Re: [Xen-devel] [refpolicy] [refpolicy SELinux PATCH 1/2] kernel/xen: Update for Xen 4.6

[Chris PeBenito]

On 10/09/2017 11:53 AM, Konrad Rzeszutek Wilk via refpolicy wrote:

From: Konrad Rzeszutek Wilk 

libxenstored since git commit 9c89dc95201ffed5fead17b35754bf9440fdbdc0
prefers to use "/dev/xen/xenbus" over the "/proc/xen/xenbus".

Signed-off-by: Konrad Rzeszutek Wilk 
---
  policy/modules/kernel/devices.fc | 1 +
  1 file changed, 1 insertion(+)

diff --git a/policy/modules/kernel/devices.fc b/policy/modules/kernel/devices.fc
index f4093434b..e206720be 100644
--- a/policy/modules/kernel/devices.fc
+++ b/policy/modules/kernel/devices.fc
@@ -183,6 +183,7 @@ ifdef(`distro_suse', `
  /dev/xen/gntdev   -c  
gen_context(system_u:object_r:xen_device_t,s0)
  /dev/xen/gntalloc -c  gen_context(system_u:object_r:xen_device_t,s0)
  /dev/xen/privcmd  -c  gen_context(system_u:object_r:xen_device_t,s0)
+/dev/xen/xenbus-c  
gen_context(system_u:object_r:xen_device_t,s0)
  
  ifdef(`distro_debian',`

  # this is a static /dev dir "backup mount"


Merged.

--
Chris PeBenito

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

[Xen-devel] XSA 243 v5 is missing the second patch for xen 4.8

[Chris Brannon]

Hello,
Looking at
https://xenbits.xen.org/xsa/advisory-243.html,
I cannot find the second patch for xen 4.8, xsa243-4.8-2.patch.
The text of the advisory leads me to believe that it should be there, so
it seems to be missing.

-- Chris

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

Re: [Xen-devel] [PATCH 4/6] xen/arm: platforms: Add Tegra platform to support basic IRQ routing

[Chris Patterson]

On Fri, Jul 7, 2017 at 2:53 PM, Chris Patterson  wrote:
> On Fri, Jul 7, 2017 at 12:30 PM, Julien Grall  wrote:
>> Hi Chris,
>>
>> On 07/07/17 00:12, Chris Patterson wrote:
>>>>
>>>>
>>>> So why do you want the hardware domain to interact with the ictlr? Could
>>>> not
>>>> you hide it completely?
>>>>
>>>
>>> snip
>>>
>>>> What would happen if you enable the interrupt here for the guest? Should
>>>> not
>>>> you do it when the guest is requesting to enable (see vgic_enable_irqs).
>>>>
>>>>
>>>> Also, how about EOI an interrupt?
>>>
>>>
>>> We could possibly hide the legacy controller, but that has its own
>>> challenges. Notably, the LIC allows configuration for forwarding FIQ
>>> vs IRQ, and setting wake-up sources.
>>
>>
>> FIQ are not supported for domain. So I am not sure why you would want a
>> guest to configure that.
>>
>
> Fair point, I did not know that and didn't want to assume there was
> not a case otherwise...
>
>> Furthermore, could you explain what is wake-up sources and why a guest would
>> need it?
>>
>
> I would expect any driver using irq_set_irq_wake()? A little more
> background from the reference manual
> (http://developer.nvidia.com/embedded/dlc/tegra-x1-technical-reference-manual):
>
> "
> The Legacy Interrupt Controller (LIC) is primarily used for BPMP
> (ARM7). But it is also used for generating interrupts as wake
> events for CPUs. This is an important use case when the core is in
> retention. All of the device hardware interrupt signals are
> sent to the LIC first, which routes them to the ARM7 BPMP-Lite as well
> as forwards them to the GIC. The LIC also provides a
> software set/clear mechanism for all of the interrupts."
>
> ...
>
> 3.3.1.5 Interrupt Blocking to Support Retention
>
> The Tegra X1 device implements blocking of interrupts routed to the
> GIC, which supports the CPU retention state. The block
> implementing this feature is represented in Figure 4 as Blocking.
>
> To support retention, the LIC contains a one shot disable for all the
> interrupts. When the system goes into retention, BPMP
> software sets this bit to disable the interrupts.
>
> The Flow Controller watches all the interrupts triggered and triggers
> the BPMP to bring the core out of retention when any
> interrupt bit is asserted. Once the system is out of retention, the
> BLOCK _CCPLEX_GIC_INTR bit is cleared, and the interrupt is
> serviced by the GIC.
> "
>
> Anything using GPIO to wake (e.g. wifi?) is routed through the LIC. I
> did not find anything obvious with a quick scan, but perhaps other
> devices directly wired to the LIC may require it as well.
>
> Thoughts?

Hey Julien. Just a quick ping. I was going to submit an updated patch
series, just wanted to see if you had any further thoughts or desired
changes with regards to the above. Thanks!

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

Re: [Xen-devel] [refpolicy] [SELINUX POLICY PATCH] Update for Xen 4.7

[Chris PeBenito]

On 07/28/2017 10:16 AM, Anthony PERARD via refpolicy wrote:

Since Xen 4.7, /dev/xen/privcmd is used instead of /proc/xen/privcmd.
Add the device into the policy so `xenstored` can work.

Signed-off-by: Anthony PERARD 
---
 policy/modules/kernel/devices.fc | 1 +
 1 file changed, 1 insertion(+)

diff --git a/policy/modules/kernel/devices.fc b/policy/modules/kernel/devices.fc
index e3a5ebc9..f4093434 100644
--- a/policy/modules/kernel/devices.fc
+++ b/policy/modules/kernel/devices.fc
@@ -182,6 +182,7 @@ ifdef(`distro_suse', `
 /dev/xen/evtchn-c  
gen_context(system_u:object_r:xen_device_t,s0)
 /dev/xen/gntdev-c  
gen_context(system_u:object_r:xen_device_t,s0)
 /dev/xen/gntalloc  -c  gen_context(system_u:object_r:xen_device_t,s0)
+/dev/xen/privcmd   -c  gen_context(system_u:object_r:xen_device_t,s0)

 ifdef(`distro_debian',`
 # this is a static /dev dir "backup mount"


Merged.

--
Chris PeBenito

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

Re: [Xen-devel] [PATCH 4/6] xen/arm: platforms: Add Tegra platform to support basic IRQ routing

[Chris Patterson]

>> The purpose of tegra_interrupt_compat is to maintain a tegra-specific
>> whitelist of interrupt controllers we know how to route.  Presumably,
>> there may be custom boards out there that may have additional
>> interrupt routing capabilities that this patch set would not support
>> as-is.  I'm not sure of an appropriate way to maintain that logic and
>> merge them.  However, I am certainly open to suggestion, if you have
>> any ideas.
>
>
> Those custom boards would likely have a different machine compatible string
> because the SOC would be different. So I think the tegra_interrupt_compat is
> not necessary.
>

Sorry about the delayed response...

This check effectively filters out GPIO controllers.  Any GPIO-sourced
interrupts route through the GPIO controller's interrupt, and need not
apply for map_irq_to_domain().  Devices requiring GPIO-based
interrupts would need to be passed through with the associated
controller (or maybe virtualize the GPIO routing?).

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

Re: [Xen-devel] [PATCH 4/6] xen/arm: platforms: Add Tegra platform to support basic IRQ routing

[Chris Patterson]

>
> So why do you want the hardware domain to interact with the ictlr? Could not
> you hide it completely?
>

snip

> What would happen if you enable the interrupt here for the guest? Should not
> you do it when the guest is requesting to enable (see vgic_enable_irqs).
>
>
> Also, how about EOI an interrupt?

We could possibly hide the legacy controller, but that has its own
challenges. Notably, the LIC allows configuration for forwarding FIQ
vs IRQ, and setting wake-up sources.

If we accept limitations to those configurations, we could possibly
hide it entirely (or just for non-Dom0 guests with device
passthrough?), and then I think we would need platform hooks for
accomplishing the masking/unmasking/eoi alongside the vgic.  If we
don't want to limit the configuration options - we would need to
surface the mediated LIC in the device trees for guests with device
pass-through.

>

Will incorporate rest of review into next version, thanks!

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

Re: [Xen-devel] [PATCH 6/6] xen/arm: platforms/tegra: Ensure the hwdom can only affect its own interrupts

[Chris Patterson]

Will fix, thanks!

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

Re: [Xen-devel] [PATCH 4/6] xen/arm: platforms: Add Tegra platform to support basic IRQ routing

[Chris Patterson]

On Fri, Jul 7, 2017 at 12:25 PM, Julien Grall  wrote:
> Hi Chris,
>
>
> On 06/07/17 23:00, Chris Patterson wrote:
>>>>
>>>> The purpose of tegra_interrupt_compat is to maintain a tegra-specific
>>>> whitelist of interrupt controllers we know how to route.  Presumably,
>>>> there may be custom boards out there that may have additional
>>>> interrupt routing capabilities that this patch set would not support
>>>> as-is.  I'm not sure of an appropriate way to maintain that logic and
>>>> merge them.  However, I am certainly open to suggestion, if you have
>>>> any ideas.
>>>
>>>
>>>
>>> Those custom boards would likely have a different machine compatible
>>> string
>>> because the SOC would be different. So I think the tegra_interrupt_compat
>>> is
>>> not necessary.
>>>
>>
>> Sorry about the delayed response...
>>
>> This check effectively filters out GPIO controllers.  Any GPIO-sourced
>> interrupts route through the GPIO controller's interrupt, and need not
>> apply for map_irq_to_domain().  Devices requiring GPIO-based
>> interrupts would need to be passed through with the associated
>> controller (or maybe virtualize the GPIO routing?).
>
>
> I am a bit confused. Could you give a concrete example based on tegra
> platform?
>

I'm thinking of devices where interrupt-parent is &gpio (as opposed to
&lic or &gic). Here is an example on tegra-smaug:

wifi: wifi_bcm4354 {
compatible = "bcm,bcm4354";
interrupt-parent = <&gpio>;
interrupts = ;
wl_reg_on = <&gpio TEGRA_GPIO(H, 1) GPIO_ACTIVE_HIGH>;
wl_host_wake = <&gpio TEGRA_GPIO(H, 2) IRQ_TYPE_LEVEL_HIGH>;
nvidia,pmc-wakeup = <&pmc
PMC_WAKE_TYPE_EVENT 8 PMC_TRIGGER_TYPE_HIGH>;
};

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

Re: [Xen-devel] [PATCH 4/6] xen/arm: platforms: Add Tegra platform to support basic IRQ routing

[Chris Patterson]

On Fri, Jul 7, 2017 at 12:30 PM, Julien Grall  wrote:
> Hi Chris,
>
> On 07/07/17 00:12, Chris Patterson wrote:
>>>
>>>
>>> So why do you want the hardware domain to interact with the ictlr? Could
>>> not
>>> you hide it completely?
>>>
>>
>> snip
>>
>>> What would happen if you enable the interrupt here for the guest? Should
>>> not
>>> you do it when the guest is requesting to enable (see vgic_enable_irqs).
>>>
>>>
>>> Also, how about EOI an interrupt?
>>
>>
>> We could possibly hide the legacy controller, but that has its own
>> challenges. Notably, the LIC allows configuration for forwarding FIQ
>> vs IRQ, and setting wake-up sources.
>
>
> FIQ are not supported for domain. So I am not sure why you would want a
> guest to configure that.
>

Fair point, I did not know that and didn't want to assume there was
not a case otherwise...

> Furthermore, could you explain what is wake-up sources and why a guest would
> need it?
>

I would expect any driver using irq_set_irq_wake()? A little more
background from the reference manual
(http://developer.nvidia.com/embedded/dlc/tegra-x1-technical-reference-manual):

"
The Legacy Interrupt Controller (LIC) is primarily used for BPMP
(ARM7). But it is also used for generating interrupts as wake
events for CPUs. This is an important use case when the core is in
retention. All of the device hardware interrupt signals are
sent to the LIC first, which routes them to the ARM7 BPMP-Lite as well
as forwards them to the GIC. The LIC also provides a
software set/clear mechanism for all of the interrupts."

...

3.3.1.5 Interrupt Blocking to Support Retention

The Tegra X1 device implements blocking of interrupts routed to the
GIC, which supports the CPU retention state. The block
implementing this feature is represented in Figure 4 as Blocking.

To support retention, the LIC contains a one shot disable for all the
interrupts. When the system goes into retention, BPMP
software sets this bit to disable the interrupts.

The Flow Controller watches all the interrupts triggered and triggers
the BPMP to bring the core out of retention when any
interrupt bit is asserted. Once the system is out of retention, the
BLOCK _CCPLEX_GIC_INTR bit is cleared, and the interrupt is
serviced by the GIC.
"

Anything using GPIO to wake (e.g. wifi?) is routed through the LIC. I
did not find anything obvious with a quick scan, but perhaps other
devices directly wired to the LIC may require it as well.

Thoughts?

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

[Xen-devel] [PATCH 2/6] xen/arm: domain_build: Inherit GIC's interrupt-parent from host device tree

[Chris Patterson]

From: "Chris Patterson" 

Currently, the interrupt parent is left undefined during creation in
make_gic_node().  In cases where a non-GIC interrupt controller is present,
this can lead to incorrect assignment of interrupt parents.

On the Tegra, the gic's interrupt parent is set to itself:

gic: interrupt-controller@0,50041000 {
compatible = "arm,gic-400";
#interrupt-cells = <3>;
interrupt-controller;
reg = <0x0 0x50041000 0x0 0x1000>,
  <0x0 0x50042000 0x0 0x2000>,
  <0x0 0x50044000 0x0 0x2000>,
  <0x0 0x50046000 0x0 0x2000>;
interrupts = ;
interrupt-parent = <&gic>;
};

To prevent the hardware domain from assuming the Legacy Interrupt Controller
(lic) as the GIC's interrupt-parent, this change explicitly assigns
the interrupt-parent property from the host device tree.

Authored-by: Kyle Temkin 
Signed-off-by: Kyle Temkin 
Signed-off-by: Chris Patterson 
Reviewed-by: Stefano Stabellini 
---

changes from rfc:
- commit message documentation improvements

---
 xen/arch/arm/domain_build.c | 17 +++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index de59e5f..cb66304 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -778,8 +778,8 @@ static int make_gic_node(const struct domain *d, void *fdt,
 {
 const struct dt_device_node *gic = dt_interrupt_controller;
 int res = 0;
-const void *addrcells, *sizecells;
-u32 addrcells_len, sizecells_len;
+const void *addrcells, *sizecells, *iparent;
+u32 addrcells_len, sizecells_len, iparent_len;
 
 /*
  * Xen currently supports only a single GIC. Discard any secondary
@@ -809,6 +809,19 @@ static int make_gic_node(const struct domain *d, void *fdt,
 return res;
 }
 
+/*
+ * If available, explicitly inherit interrupt-parent property from host
+ * device tree.  This will prevent the risk of incorrect identification
+ * of the parent on platforms with more than one interrupt controller.
+ */
+iparent = dt_get_property(gic, "interrupt-parent", &iparent_len);
+if ( iparent )
+{
+res = fdt_property(fdt, "interrupt-parent", iparent, iparent_len);
+if ( res )
+  return res;
+}
+
 addrcells = dt_get_property(gic, "#address-cells", &addrcells_len);
 if ( addrcells )
 {
-- 
2.1.4


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

[Xen-devel] [PATCH 5/6] xen/arm: Add function to query IRQ 'ownership'

[Chris Patterson]

From: "Chris Patterson" 

The addition of new IRQ-related platform hooks now allow platforms to
perform platform-specific interrupt logic, such as allowing virtualization
of platform-specific interrupt controller hardware.

This commit adds the ability for the platform to identify the domain
a given IRQ is routed to, allowing platform logic to deny access to
registers associated with a given IRQ unless the requesting domain
'owns' the IRQ. This will be used on Tegra platforms, where the hardware
domain needs access to its legacy interrupt controller, but should not
be able to control registers that correspond to other domains' IRQs, or
sections associated with IRQs routed to Xen.

Authored-by: Kyle Temkin 
Signed-off-by: Kyle Temkin 
Signed-off-by: Chris Patterson 
Reviewed-by: Stefano Stabellini 
---

changes since rfc:
- formatting & code style cleanup

---

 xen/arch/arm/irq.c| 10 ++
 xen/include/asm-arm/irq.h |  2 ++
 2 files changed, 12 insertions(+)

diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index 0b4eaa9..51bce58 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -143,6 +143,16 @@ static inline struct domain *irq_get_domain(struct 
irq_desc *desc)
 return irq_get_guest_info(desc)->d;
 }
 
+domid_t irq_get_domain_id(struct irq_desc *desc)
+{
+/* If this domain isn't routed to a guest, return DOMID_XEN. */
+if ( !test_bit(_IRQ_GUEST, &desc->status) )
+return DOMID_XEN;
+
+/* Otherise, get the guest domain's information. */
+return irq_get_domain(desc)->domain_id;
+}
+
 void irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask)
 {
 if ( desc != NULL )
diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
index 4849f16..d0fd6db 100644
--- a/xen/include/asm-arm/irq.h
+++ b/xen/include/asm-arm/irq.h
@@ -44,6 +44,8 @@ int route_irq_to_guest(struct domain *d, unsigned int virq,
unsigned int irq, const char *devname);
 int release_guest_irq(struct domain *d, unsigned int irq);
 
+domid_t irq_get_domain_id(struct irq_desc *desc);
+
 void arch_move_irqs(struct vcpu *v);
 
 #define arch_evtchn_bind_pirq(d, pirq) ((void)((d) + (pirq)))
-- 
2.1.4


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

[Xen-devel] [PATCH 4/6] xen/arm: platforms: Add Tegra platform to support basic IRQ routing

[Chris Patterson]

From: "Chris Patterson" 

Tegra devices have a legacy interrupt controller (lic, or ictlr) that
must be programmed in parallel with their primary GIC. For all intents
and purposes, we treat these devices attached to this controller as
connected to the primary GIC, as it will be handling their interrupts.

This commit adds support for exposing the ictlr to the hardware domain;
but a future commit will extend this to support exposing a virtualized
version of the ictlr to the hardware domain, and to ensure that
interrupts are unmasked properly when routed to a Xen, or to a domain
other than the hardware domain.

Authored-by: Kyle Temkin 
Signed-off-by: Kyle Temkin 
Signed-off-by: Chris Patterson 
---

changes since rfc:
- use bool instead of bool_t
- formatting & code style cleanup
- fix dt compat label (nvidia,tegra120 -> nvidia,tegra124) for K1
- separate mediated legacy interrupt controller into its own module
- split tegra_ictlr_set_interrupt_enable() into
  tegra_lic_set_interrupt_type_normal() and
  tegra_lic_set_interrupt_enable()
- added a couple helper functions to reduce duplicated logic
- added wrapper tegra_lic_readl and writel functions for external use (mlic)
- re-order defines in tegra.h
- cleanup tegra_init() that was previously in patch 6

---

 xen/arch/arm/platforms/Makefile   |   2 +
 xen/arch/arm/platforms/tegra.c| 313 ++
 xen/include/asm-arm/platforms/tegra.h |  54 ++
 3 files changed, 369 insertions(+)
 create mode 100644 xen/arch/arm/platforms/tegra.c
 create mode 100644 xen/include/asm-arm/platforms/tegra.h

diff --git a/xen/arch/arm/platforms/Makefile b/xen/arch/arm/platforms/Makefile
index 49fa683..d7033d2 100644
--- a/xen/arch/arm/platforms/Makefile
+++ b/xen/arch/arm/platforms/Makefile
@@ -6,5 +6,7 @@ obj-$(CONFIG_ARM_32) += omap5.o
 obj-$(CONFIG_ARM_32) += rcar2.o
 obj-$(CONFIG_ARM_64) += seattle.o
 obj-$(CONFIG_ARM_32) += sunxi.o
+obj-$(CONFIG_ARM_32) += tegra.o
+obj-$(CONFIG_ARM_64) += tegra.o
 obj-$(CONFIG_ARM_64) += xgene-storm.o
 obj-$(CONFIG_ARM_64) += xilinx-zynqmp.o
diff --git a/xen/arch/arm/platforms/tegra.c b/xen/arch/arm/platforms/tegra.c
new file mode 100644
index 000..bdd9966
--- /dev/null
+++ b/xen/arch/arm/platforms/tegra.c
@@ -0,0 +1,312 @@
+/*
+ * NVIDIA Tegra specific settings
+ *
+ * Ian Campbell; Copyright (c) 2014 Citrix Systems
+ * Kyle Temkin; Copyright (c) 2016 Assured Information Security, Inc.
+ * Chris Patterson; Copyright (c) 2016 Assured Information Security, Inc.
+ *
+ * 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.
+ */
+
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+
+/* Permanent mapping to the Tegra legacy interrupt controller. */
+static void __iomem *tegra_ictlr_base;
+
+/*
+ * List of legacy interrupt controllers that can be used to route
+ * Tegra interrupts.
+ */
+static const char * const tegra_interrupt_compat[] __initconst =
+{
+"nvidia,tegra124-ictlr",  /* Tegra K1 controllers */
+"nvidia,tegra210-ictlr"   /* Tegra X1 controllers */
+};
+
+/*
+ * Returns true iff the given IRQ belongs to a supported tegra interrupt
+ * controller.
+ */
+static bool tegra_irq_belongs_to_ictlr(const struct dt_raw_irq * rirq)  {
+int i;
+
+for ( i = 0; i < ARRAY_SIZE(tegra_interrupt_compat); i++ ) {
+if ( dt_device_is_compatible(rirq->controller, 
tegra_interrupt_compat[i]) )
+return true;
+}
+
+return false;
+}
+
+/*
+ * Returns true iff the given IRQ is routable -- that is, if it is descended
+ * from the platform's primary GIC.
+ */
+static bool tegra_irq_is_routable(const struct dt_raw_irq * rirq)
+{
+/* If the IRQ connects directly to our GIC, it's trivially routable. */
+if ( rirq->controller == dt_interrupt_controller )
+return true;
+
+/*
+ * If the IRQ belongs to a legacy interrupt controller, then it's
+ * effectively owned by the GIC, and is routable.
+ */
+if ( tegra_irq_belongs_to_ictlr(rirq) )
+return true;
+
+return false;
+}
+
+/*
+ * Platform-specific reset code for the Tegra devices.
+ * Should not return.
+ */
+static void tegra_reset(void)
+{
+void __iomem *addr;
+u32 val;
+
+addr = ioremap_nocache(TEGRA_RESET_BASE, TEGRA_RESET_SIZE);
+if ( !addr )
+{
+printk(XENLOG_ERR "Tegra: Unable to map tegra reset address. Reset 
failed!\n");
+return;
+}
+
+/* Write into the reset devi

[Xen-devel] [PATCH 0/6] Initial Tegra platform support

[Chris Patterson]

The attached patch-set adds support for 32-bit and 64-bit Tegra SoCs; including
support for the Jetson TK1 and Jetson TX1 boards, as well as the Pixel C tablet.
Previous revisions have been tested against the TK1, TX1, and Pixel C.  This
current patch set has only been tested against the TX1 due to my curent hardware
availability, but should be OK on the other platforms.

Many thanks to Ian Campbell, whose original Jetson TK1 patchset contained a lot
of pointers in the right direction, and helped us to get started on this one!

This patch set includes:
  - A minor serial quirk to get the NS16550 on the Tegra working.
  - Support for the Tegra Legacy Interrupt Controller, which is necessary to get
interrupt routing working correctly on Tegra devices. In this version of the
patchset, the interrupt controller is supported via platform quirks.
  - A few additional minor features and logic fixes to support the Tegra. An
example would be the Tegra-specific reset logic.

This patch set does NOT include:
  - Support for the Tegra-specific IOMMU. This means this platform doesn't yet
support device passthrough.

This patch set includes fixes and cleanup requested from the original RFC
posted by Kyle Temkin. Kyle has done most of the authoring on this, but I am
picking it up to address the RFC reviews and push it though.

Chris Patterson (6):
  xen/arm: platforms: Add earlyprintk and serial support for Tegra
boards.
  xen/arm: domain_build: Inherit GIC's interrupt-parent from host device
tree
  xen/arm: Allow platforms to hook IRQ routing
  xen/arm: platforms: Add Tegra platform to support basic IRQ routing
  xen/arm: Add function to query IRQ 'ownership'
  xen/arm: platforms/tegra: Ensure the hwdom can only affect its own
interrupts

 xen/arch/arm/Rules.mk  |   1 +
 xen/arch/arm/domain_build.c|  29 ++-
 xen/arch/arm/irq.c |  15 +-
 xen/arch/arm/platform.c|  30 +++
 xen/arch/arm/platforms/Makefile|   4 +
 xen/arch/arm/platforms/tegra-mlic.c| 261 +++
 xen/arch/arm/platforms/tegra.c | 326 +
 xen/common/device_tree.c   |   8 +-
 xen/drivers/char/ns16550.c |  28 ++-
 xen/include/asm-arm/irq.h  |   2 +
 xen/include/asm-arm/platform.h |  12 ++
 xen/include/asm-arm/platforms/tegra-mlic.h |  34 +++
 xen/include/asm-arm/platforms/tegra.h  |  54 +
 xen/include/xen/8250-uart.h|   1 +
 14 files changed, 788 insertions(+), 17 deletions(-)
 create mode 100644 xen/arch/arm/platforms/tegra-mlic.c
 create mode 100644 xen/arch/arm/platforms/tegra.c
 create mode 100644 xen/include/asm-arm/platforms/tegra-mlic.h
 create mode 100644 xen/include/asm-arm/platforms/tegra.h

-- 
2.1.4


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

[Xen-devel] [PATCH 3/6] xen/arm: Allow platforms to hook IRQ routing

[Chris Patterson]

From: "Chris Patterson" 

Some common platforms (e.g. Tegra) have non-traditional IRQ controllers
that must be programmed in addition to their primary GICs-- and which
can come in unusual topologies. Device trees for targets that feature
these controllers often deviate from the conventions that Xen expects.

This commit provides a foundation for support of these platforms, by
allowing the platform to decide which IRQs can be routed by Xen, rather
than assuming that only GIC-connected IRQs can be routed.  This enables
platform specific logic to routing the IRQ to Xen and Guest.

As dt_irq_translate() is presently hard-coded to just support the
primary interrupt controller, instead rely on the newly added
platform_irq_is_routable() check instead.  The default behaviour of this
new function should be consistent with the previous checks for platforms
that do not implement it.

Authored-by: Kyle Temkin 
Signed-off-by: Kyle Temkin 
Signed-off-by: Chris Patterson 
---

changes since rfc:
- use bool instead of bool_t
- formatting & code style cleanup
- reuse dt_irq_translate() path and drop platform_irq_for_device() approach
- use const qualifier for platform_irq_is_routable rirq argument

---

 xen/arch/arm/domain_build.c| 12 
 xen/arch/arm/irq.c |  5 +++--
 xen/arch/arm/platform.c| 30 ++
 xen/common/device_tree.c   |  8 +++-
 xen/include/asm-arm/platform.h | 12 
 5 files changed, 56 insertions(+), 11 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index cb66304..92536dd 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1120,12 +1120,16 @@ static int handle_device(struct domain *d, struct 
dt_device_node *dev,
 
 /*
  * Don't map IRQ that have no physical meaning
- * ie: IRQ whose controller is not the GIC
+ * ie: IRQ that does not wind up being controlled by the GIC
+ * (Note that we can't just check to see if an IRQ is owned by the GIC,
+ *  as some platforms have a controller between the device irq and the 
GIC,
+ *  such as the Tegra legacy interrupt controller.)
  */
-if ( rirq.controller != dt_interrupt_controller )
+if ( !platform_irq_is_routable(&rirq) )
 {
-dt_dprintk("irq %u not connected to primary controller. Connected 
to %s\n",
-  i, dt_node_full_name(rirq.controller));
+dt_dprintk("irq %u not (directly or indirectly) connected to 
primary"
+"controller. Connected to %s\n", i,
+dt_node_full_name(rirq.controller));
 continue;
 }
 
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index f3f20a6..0b4eaa9 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -26,6 +26,7 @@
 
 #include 
 #include 
+#include 
 
 static unsigned int local_irqs_type[NR_LOCAL_IRQS];
 static DEFINE_SPINLOCK(local_irqs_type_lock);
@@ -369,7 +370,7 @@ int setup_irq(unsigned int irq, unsigned int irqflags, 
struct irqaction *new)
 /* First time the IRQ is setup */
 if ( disabled )
 {
-gic_route_irq_to_xen(desc, GIC_PRI_IRQ);
+platform_route_irq_to_xen(desc, GIC_PRI_IRQ);
 /* It's fine to use smp_processor_id() because:
  * For PPI: irq_desc is banked
  * For SPI: we don't care for now which CPU will receive the
@@ -506,7 +507,7 @@ int route_irq_to_guest(struct domain *d, unsigned int virq,
 if ( retval )
 goto out;
 
-retval = gic_route_irq_to_guest(d, virq, desc, GIC_PRI_IRQ);
+retval = platform_route_irq_to_guest(d, virq, desc, GIC_PRI_IRQ);
 
 spin_unlock_irqrestore(&desc->lock, flags);
 
diff --git a/xen/arch/arm/platform.c b/xen/arch/arm/platform.c
index 0af6d57..539ee3b 100644
--- a/xen/arch/arm/platform.c
+++ b/xen/arch/arm/platform.c
@@ -147,6 +147,36 @@ bool_t platform_device_is_blacklisted(const struct 
dt_device_node *node)
 return (dt_match_node(blacklist, node) != NULL);
 }
 
+int platform_route_irq_to_guest(struct domain *d, unsigned int virq,
+struct irq_desc *desc, unsigned int priority)
+{
+if ( platform && platform->route_irq_to_guest )
+return platform->route_irq_to_guest(d, virq, desc, priority);
+else
+return gic_route_irq_to_guest(d, virq, desc, priority);
+}
+
+void platform_route_irq_to_xen(struct irq_desc *desc, unsigned int priority)
+{
+if ( platform && platform->route_irq_to_xen )
+platform->route_irq_to_xen(desc, priority);
+else
+gic_route_irq_to_xen(desc, priority);
+}
+
+bool platform_irq_is_routable(const struct dt_raw_irq * rirq)
+{
+/*
+ * If we have a platform-specific method to determine if an IRQ is 
routable,
+ * check that; otherwise fall back to checkin

[Xen-devel] [PATCH 1/6] xen/arm: platforms: Add earlyprintk and serial support for Tegra boards.

[Chris Patterson]

From: Chris Patterson 

Tegra boards feature a NS16550-compatible serial mapped into the MMIO
space. Add support for its use both as a full-featured serial port and
as an earlyprintk driver.

This patch adds a quirk for platforms, such as the Tegra, which require
require the NS16550 Rx timeout interrupt to be enabled for receive to
function properly. The same quirk is applied in the eqvuialent Linux
driver [1].

This quirk is selectively enabled for the platform using a new "hw_quirks"
member with a corresponding set of bitmasks.  The existing quirk,
dw_usr_bsy was updated to match this approach as well.

[1] 
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=4539c24fe4f92c09ee668ef959d3e8180df619b9

Signed-off-by: Kyle Temkin 
Signed-off-by: Chris Patterson 
---

changes from rfc:
- use bitmask for quirks in ns1660, including dw_usr_bsy

---
 xen/arch/arm/Rules.mk   |  1 +
 xen/drivers/char/ns16550.c  | 28 
 xen/include/xen/8250-uart.h |  1 +
 3 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/Rules.mk b/xen/arch/arm/Rules.mk
index 569a0ba..43b32d0 100644
--- a/xen/arch/arm/Rules.mk
+++ b/xen/arch/arm/Rules.mk
@@ -44,6 +44,7 @@ EARLY_PRINTK_vexpress   := pl011,0x1c09
 EARLY_PRINTK_xgene-mcdivitt := 8250,0x1c021000,2
 EARLY_PRINTK_xgene-storm:= 8250,0x1c02,2
 EARLY_PRINTK_zynqmp := cadence,0xff00
+EARLY_PRINTK_tegra  := 8250,0x70006000,2
 
 ifneq ($(EARLY_PRINTK_$(CONFIG_EARLY_PRINTK)),)
 EARLY_PRINTK_CFG := $(subst $(comma), ,$(EARLY_PRINTK_$(CONFIG_EARLY_PRINTK)))
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index e4de3b4..1b75e89 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -62,7 +62,7 @@ static struct ns16550 {
 struct timer resume_timer;
 unsigned int timeout_ms;
 bool_t intr_works;
-bool_t dw_usr_bsy;
+uint8_t hw_quirks;
 #ifdef CONFIG_HAS_PCI
 /* PCI card parameters. */
 bool_t pb_bdf_enable;   /* if =1, pb-bdf effective, port behind bridge */
@@ -414,6 +414,10 @@ static const struct ns16550_config __initconst 
uart_config[] =
 };
 #endif
 
+/* Various hardware quirks/features that may be need be enabled per device */
+#define HW_QUIRKS_DW_USR_BSY (1<<0)
+#define HW_QUIRKS_USE_RTOIE  (1<<1)
+
 static void ns16550_delayed_resume(void *data);
 
 static u8 ns_read_reg(struct ns16550 *uart, unsigned int reg)
@@ -578,7 +582,7 @@ static void ns16550_setup_preirq(struct ns16550 *uart)
 /* No interrupts. */
 ns_write_reg(uart, UART_IER, 0);
 
-if ( uart->dw_usr_bsy &&
+if ( (uart->hw_quirks & HW_QUIRKS_DW_USR_BSY) &&
  (ns_read_reg(uart, UART_IIR) & UART_IIR_BSY) == UART_IIR_BSY )
 {
 /* DesignWare 8250 detects if LCR is written while the UART is
@@ -651,12 +655,23 @@ static void ns16550_setup_postirq(struct ns16550 *uart)
 {
 if ( uart->irq > 0 )
 {
+u8 ier_value = 0;
+
 /* Master interrupt enable; also keep DTR/RTS asserted. */
 ns_write_reg(uart,
  UART_MCR, UART_MCR_OUT2 | UART_MCR_DTR | UART_MCR_RTS);
 
 /* Enable receive interrupts. */
-ns_write_reg(uart, UART_IER, UART_IER_ERDAI);
+ier_value = UART_IER_ERDAI;
+
+/*
+ * If we're on a platform that needs Rx timeouts enabled, also
+ * set Rx TimeOut Interrupt Enable (RTOIE).
+ */
+if ( uart->hw_quirks & HW_QUIRKS_USE_RTOIE )
+  ier_value |= UART_IER_RTOIE;
+
+ns_write_reg(uart, UART_IER, ier_value);
 }
 
 if ( uart->irq >= 0 )
@@ -1271,7 +1286,11 @@ static int __init ns16550_uart_dt_init(struct 
dt_device_node *dev,
 return -EINVAL;
 uart->irq = res;
 
-uart->dw_usr_bsy = dt_device_is_compatible(dev, "snps,dw-apb-uart");
+if ( dt_device_is_compatible(dev, "snps,dw-apb-uart") )
+uart->hw_quirks |= HW_QUIRKS_DW_USR_BSY;
+
+if ( dt_device_is_compatible(dev, "nvidia,tegra20-uart") )
+uart->hw_quirks |= HW_QUIRKS_USE_RTOIE;
 
 uart->vuart.base_addr = uart->io_base;
 uart->vuart.size = uart->io_size;
@@ -1292,6 +1311,7 @@ static const struct dt_device_match ns16550_dt_match[] 
__initconst =
 DT_MATCH_COMPATIBLE("ns16550"),
 DT_MATCH_COMPATIBLE("ns16550a"),
 DT_MATCH_COMPATIBLE("snps,dw-apb-uart"),
+DT_MATCH_COMPATIBLE("nvidia,tegra20-uart"),
 { /* sentinel */ },
 };
 
diff --git a/xen/include/xen/8250-uart.h b/xen/include/xen/8250-uart.h
index c6b62c8..2ad0ee6 100644
--- a/xen/include/xen/8250-uart.h
+++ b/xen/include/xen/8250-uart.h
@@ -41,6 +41,7 @@
 #define UART_IER_ETHREI   0x02/* tx reg. empty*/
 #define UART_IER_ELSI 0x04/* rx line status   */
 #define UART_IER_EMSI 0x08/* MODEM status */
+#

[Xen-devel] [PATCH 6/6] xen/arm: platforms/tegra: Ensure the hwdom can only affect its own interrupts

[Chris Patterson]

From: Chris Patterson 

Several Tegra hardware devices, and the Tegra device tree, expect
the presence of a Tegra Legacy Interrupt Controller (LIC) in the hardware
domain. Accordingly, we'll need to expose (most of) the LIC's registers
to the hardware domain.

As the Tegra LIC provides the ability to modify interrupt delivery (e.g.
by masking interrupts, forcing asserting/clearing them, or adjusting
their prority), it's important that the hardware domain's access be
mediated. This commit adds read/write handlers that prohibit
modification of register sections corresponding to interrupts not owned
by the hardware domain.

Note that this is written to be domain agnostic; this allows the
potential to e.g. map the ictlr into multiple domains if this is desired
for passthrough in the future.

Authored-by: Kyle Temkin 
Signed-off-by: Kyle Temkin 
Signed-off-by: Chris Patterson 
---

changes since rfc:
- documentation, formatting & code style cleanup
- drop tegra_init changes (folded into patch 4)

---
 xen/arch/arm/platforms/Makefile|   2 +
 xen/arch/arm/platforms/tegra-mlic.c| 261 +
 xen/arch/arm/platforms/tegra.c |  13 ++
 xen/include/asm-arm/platforms/tegra-mlic.h |  34 
 4 files changed, 310 insertions(+)
 create mode 100644 xen/arch/arm/platforms/tegra-mlic.c
 create mode 100644 xen/include/asm-arm/platforms/tegra-mlic.h

diff --git a/xen/arch/arm/platforms/Makefile b/xen/arch/arm/platforms/Makefile
index d7033d2..5701e62 100644
--- a/xen/arch/arm/platforms/Makefile
+++ b/xen/arch/arm/platforms/Makefile
@@ -7,6 +7,8 @@ obj-$(CONFIG_ARM_32) += rcar2.o
 obj-$(CONFIG_ARM_64) += seattle.o
 obj-$(CONFIG_ARM_32) += sunxi.o
 obj-$(CONFIG_ARM_32) += tegra.o
+obj-$(CONFIG_ARM_32) += tegra-mlic.o
 obj-$(CONFIG_ARM_64) += tegra.o
+obj-$(CONFIG_ARM_64) += tegra-mlic.o
 obj-$(CONFIG_ARM_64) += xgene-storm.o
 obj-$(CONFIG_ARM_64) += xilinx-zynqmp.o
diff --git a/xen/arch/arm/platforms/tegra-mlic.c 
b/xen/arch/arm/platforms/tegra-mlic.c
new file mode 100644
index 000..ad77766
--- /dev/null
+++ b/xen/arch/arm/platforms/tegra-mlic.c
@@ -0,0 +1,260 @@
+/*
+ * xen/arch/arm/tegra_mlic.c
+ *
+ * Mediator for Tegra Legacy Interrupt Controller
+ *
+ * This module allow the hardware domain to have access to the sections of
+ * the legacy interrupt controller that correspond to its devices,
+ * but disallow access to the sections controlled by other domains
+ * or by Xen.
+ *
+ * Kyle Temkin; Copyright (c) 2016 Assured Information Security, Inc.
+ * Chris Patterson; Copyright (c) 2016 Assured Information Security, Inc.
+ *
+ * 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.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+static int tegra_mlic_mmio_read(struct vcpu *v, mmio_info_t *info,
+   register_t *r, void *priv);
+static int tegra_mlic_mmio_write(struct vcpu *v, mmio_info_t *info,
+register_t r, void *priv);
+
+static const struct mmio_handler_ops tegra_mlic_mmio_handler = {
+.read  = tegra_mlic_mmio_read,
+.write = tegra_mlic_mmio_write,
+};
+
+/*
+ * Parses a LIC MMIO read or write, and extracts the information needed to
+ * complete the request.
+ *
+ * info: Information describing the MMIO read/write being performed
+ * ictlr_index: The interrupt controller number in the ictlr (e.g. 0-5)
+ * register_offset: The register offset into the specified interrupt controller
+ *(e.g. TEGRA_ICTLR_CPU_IER_SET)
+ * irq_base: The number of the first IRQ represented by the given ictlr.
+ */
+static void tegra_mlic_parse_mmio_request(mmio_info_t *info,
+uint32_t *ictlr_index, uint32_t *register_offset, uint32_t *irq_base)
+{
+/* Determine the offset of the access into the ICTLR region. */
+uint32_t offset = info->gpa - TEGRA_ICTLR_BASE;
+uint32_t ictlr = offset / TEGRA_ICTLR_SIZE;
+uint32_t reg = offset % TEGRA_ICTLR_SIZE;
+
+if ( ictlr_index )
+*ictlr_index = ictlr;
+
+if ( register_offset )
+*register_offset = reg;
+
+if ( irq_base )
+*irq_base = (ictlr * TEGRA_IRQS_PER_ICTLR) + NR_LOCAL_IRQS;
+
+/* Ensure that we've only been handed a valid offset within our region. */
+BUG_ON(ictlr >= TEGRA_ICTLR_COUNT);
+BUG_ON(offset >= (TEGRA_ICTLR_COUNT * TEGRA_ICTLR_SIZE));
+BUG_ON((ictlr * TEGRA_ICTLR_SIZE + reg) != offset);
+}
+
+/*
+ * Returns true iff th

[Xen-devel] stack size limit issues with xen + qemu + rbd

[Chris Patterson]

I have spent some time investigating a case where qemu is failing to
register xenstore watches for a PV guest once I enable vfb (and
thereby triggering the creation of a qemu instance).

The qemu logs show something along the lines of:
xen be core: xen be core: xen be: watching backend path
(backend/console/3) failed
xen be: watching backend path (backend/console/3) failed
xen be core: xen be core: xen be: watching backend path (backend/vkbd/3) failed
xen be: watching backend path (backend/vkbd/3) failed
xen be core: xen be core: xen be: watching backend path (backend/qdisk/3) failed
xen be: watching backend path (backend/qdisk/3) failed
xen be core: xen be core: xen be: watching backend path (backend/qusb/3) failed
xen be: watching backend path (backend/qusb/3) failed
xen be core: xen be core: xen be: watching backend path (backend/vfb/3) failed
xen be: watching backend path (backend/vfb/3) failed
xen be core: xen be core: xen be: watching backend path (backend/qnic/3) failed
xen be: watching backend path (backend/qnic/3) failed

I have tested qemu master, qemu-xen in the master xen tree, as well as
a few tags all with the same issue.

I came across a similar issue reported by Juergen Gross:
https://lists.nongnu.org/archive/html/qemu-devel/2016-07/msg03341.html

Sure enough, the thread stack size was the culprit.  I had been
testing with qemu with the associated fix "vnc-tight: fix regression
with libxenstore" as it is in master, so that wasn't it...

I did some basic analysis of the qemu binary and the libraries it is pulling in:

for lib in $(ldd /usr/local/bin/qemu-system-i386 | grep -o '/.* '); do
echo "lib=$lib"; readelf -S "$lib" | grep -e tbss -e tdata -A1 ; done

The largest consumers were:
lib=/usr/lib/x86_64-linux-gnu/librbd.so.1
  [17] .tbss NOBITS   0088fed0  0068fed0
   1820   WAT   0 0 8
lib=/usr/lib/x86_64-linux-gnu/librados.so.2
  [17] .tbss NOBITS   00718600  00518600
   0aa0   WAT   0 0 8

IIUC, librbd & librados are using nearly 9k of the 16k alone (I am
assuming this thread-local storage must be consumed as part of the
thread's stack)?

I narrowed that down to Ceph's usage of __thread in stringify() in
src/include/stringify.h.

To make things functional, the options were either to:
(a) disable rbd at configure time for qemu
(b) reduce the level of thread-local storage in dependencies
(particularly ceph's stringify)
(c) increase the stack size specified in xenstore's xs.c

Is there is any precedent/policy with regards to expected TLS and/or
stack usage for dependencies?  Is the best course of action (b)? Or
perhaps reconsider the default size for (c)?

Thoughts? :)

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

[Xen-devel] [RFC PATCH] xs: use system's default stack size for xs_watch's reader thread

[Chris Patterson]

From: Chris Patterson 

xs_watch() creates a thread to listen to xenstore events.  Currently, the
thread is created with the greater of 16K or PTHREAD_MIN_SIZE.

There have been several bug reports and workarounds related to the issue
where xs_watch() fails because its attempt to create the reader thread with
pthread_create() fails. This is due to insufficient stack space size
given the requirements for thread-local storage usage in the applications
and libraries that are linked against libxenstore.  [1,2,3,4].

Specifying the stack size appears to have been added to reduce memory
footprint (1d00c73b983b09fbee4d9dc0f58f6663c361c345).

This has already been bumped up once to the greater of 16k and
PTHREAD_STACK_MIN (da6a0e86d6a079102abdd0858a19f1e1fae584fc).

This patch reverts to sticking with the system's default stack size and
removes the code used to set the thread's stack size.

Of course, the alternative is to bump it to another arbitrary value, but the
requirements may change depending on the application and its libraries that
are linking against xenstore.

[1] https://lists.nongnu.org/archive/html/qemu-devel/2016-07/msg03341.html
[2] https://lists.xenproject.org/archives/html/xen-users/2016-07/msg00012.html
[3] https://lists.xenproject.org/archives/html/xen-users/2016-07/msg00085.html
[4] https://bugzilla.redhat.com/show_bug.cgi?id=1350264

Signed-off-by: Chris Patterson 
---
 tools/xenstore/xs.c | 10 --
 1 file changed, 10 deletions(-)

diff --git a/tools/xenstore/xs.c b/tools/xenstore/xs.c
index d1e01ba..c62b120 100644
--- a/tools/xenstore/xs.c
+++ b/tools/xenstore/xs.c
@@ -723,11 +723,6 @@ bool xs_watch(struct xs_handle *h, const char *path, const 
char *token)
struct iovec iov[2];
 
 #ifdef USE_PTHREAD
-#define DEFAULT_THREAD_STACKSIZE (16 * 1024)
-#define READ_THREAD_STACKSIZE  \
-   ((DEFAULT_THREAD_STACKSIZE < PTHREAD_STACK_MIN) ?   \
-   PTHREAD_STACK_MIN : DEFAULT_THREAD_STACKSIZE)
-
/* We dynamically create a reader thread on demand. */
mutex_lock(&h->request_mutex);
if (!h->read_thr_exists) {
@@ -738,11 +733,6 @@ bool xs_watch(struct xs_handle *h, const char *path, const 
char *token)
mutex_unlock(&h->request_mutex);
return false;
}
-   if (pthread_attr_setstacksize(&attr, READ_THREAD_STACKSIZE) != 
0) {
-   pthread_attr_destroy(&attr);
-   mutex_unlock(&h->request_mutex);
-   return false;
-   }
 
sigfillset(&set);
pthread_sigmask(SIG_SETMASK, &set, &old_set);
-- 
2.1.4


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

Re: [Xen-devel] [RFC PATCH] xs: use system's default stack size for xs_watch's reader thread

[Chris Patterson]

On Wed, Sep 21, 2016 at 9:00 AM, Wei Liu  wrote:
> On Wed, Sep 21, 2016 at 08:51:07AM -0400, Konrad Rzeszutek Wilk wrote:
>> On Tue, Sep 20, 2016 at 05:29:39PM -0400, Chris Patterson wrote:
>> > From: Chris Patterson 
>> >
>> > xs_watch() creates a thread to listen to xenstore events.  Currently, the
>> > thread is created with the greater of 16K or PTHREAD_MIN_SIZE.
>> >
>> > There have been several bug reports and workarounds related to the issue
>> > where xs_watch() fails because its attempt to create the reader thread with
>> > pthread_create() fails. This is due to insufficient stack space size
>> > given the requirements for thread-local storage usage in the applications
>> > and libraries that are linked against libxenstore.  [1,2,3,4].
>> >
>> > Specifying the stack size appears to have been added to reduce memory
>> > footprint (1d00c73b983b09fbee4d9dc0f58f6663c361c345).
>>
>> Ugh. 8MB.
>
> OOI isn't that 8MB virtual memory, which means it shouldn't have real
> impact unless it is used?
>

From what I understand, that is correct.  At least in the Linux/glibc
case, I believe the stack is allocated using anonymous mmap() and that
resident memory usage shouldn't be greater than what you actually end
up writing.  However, I do not know if this holds true universally...

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

Re: [Xen-devel] [RFC PATCH] xs: use system's default stack size for xs_watch's reader thread

[Chris Patterson]

On Wed, Sep 21, 2016 at 10:07 AM, Konrad Rzeszutek Wilk
 wrote:
> On Wed, Sep 21, 2016 at 09:50:30AM -0400, Chris Patterson wrote:
>> On Wed, Sep 21, 2016 at 9:00 AM, Wei Liu  wrote:
>> > On Wed, Sep 21, 2016 at 08:51:07AM -0400, Konrad Rzeszutek Wilk wrote:
>> >> On Tue, Sep 20, 2016 at 05:29:39PM -0400, Chris Patterson wrote:
>> >> > From: Chris Patterson 
>> >> >
>> >> > xs_watch() creates a thread to listen to xenstore events.  Currently, 
>> >> > the
>> >> > thread is created with the greater of 16K or PTHREAD_MIN_SIZE.
>> >> >
>> >> > There have been several bug reports and workarounds related to the issue
>> >> > where xs_watch() fails because its attempt to create the reader thread 
>> >> > with
>> >> > pthread_create() fails. This is due to insufficient stack space size
>> >> > given the requirements for thread-local storage usage in the 
>> >> > applications
>> >> > and libraries that are linked against libxenstore.  [1,2,3,4].
>> >> >
>> >> > Specifying the stack size appears to have been added to reduce memory
>> >> > footprint (1d00c73b983b09fbee4d9dc0f58f6663c361c345).
>> >>
>> >> Ugh. 8MB.
>> >
>> > OOI isn't that 8MB virtual memory, which means it shouldn't have real
>> > impact unless it is used?
>
> /me nods. That is my recollection too. But it does mean that 'top'
> shows the application as bigger (by 8MB).
>
>> >
>>
>> >From what I understand, that is correct.  At least in the Linux/glibc
>> case, I believe the stack is allocated using anonymous mmap() and that
>> resident memory usage shouldn't be greater than what you actually end
>> up writing.  However, I do not know if this holds true universally...
>
> That should be faily easy to find out. One just needs to check
> the RSS size. Not sure how to do that on FreeBSD thought..

I did validate that yesterday (using pmap -x ) and it appeared to
hold true (for that case anyways).

If using the default stack size (as this patch reverts to), the stack
size can be controlled with ulimit -s 

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

Re: [Xen-devel] Xen ARM community call

[Chris Patterson]

> I would suggest to start with a 1 hour meeting on the Wednesday 23rd
> November. I know that people are spread across different timezones, so I
> would like to gather thought before choosing a time.
>

Sounds good! EST (GMT-5).

Cheers,
-Chris

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

[Xen-devel] [PATCH] libxl: compilation warning fix for arm & aarch64

[Chris Patterson]

From: Chris Patterson 

GCC 6 will warn on unused static const variables in c modules:
https://gcc.gnu.org/ml/gcc-patches/2015-09/msg00847.html

When compiling with LIBXL_HAVE_NO_SUSPEND_RESUME set (arm & aarch64),
the compiler emits the following errors:
  xl_cmdimpl.c:101:19: error: 'migrate_report'
  defined but not used [-Werror=unused-const-variable=]
  xl_cmdimpl.c:99:19: error: 'migrate_permission_to_go'
  defined but not used [-Werror=unused-const-variable=]
  xl_cmdimpl.c:97:19: error: 'migrate_receiver_ready'
  defined but not used [-Werror=unused-const-variable=]
  xl_cmdimpl.c:95:19: error: 'migrate_receiver_banner'
  defined but not used [-Werror=unused-const-variable=]

These unused const variables are only used in functions which exist between
the ifndef block:
   #ifndef LIBXL_HAVE_NO_SUSPEND_RESUME
   ...
   #endif

Wrap the same ifndef around these variables.

Signed-off-by: Chris Patterson 
---
 tools/libxl/xl_cmdimpl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index d1fcfa4..ada8178 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -92,6 +92,7 @@ static int fd_lock = -1;
 static const char savefileheader_magic[32]=
 "Xen saved domain, xl format\n \0 \r";
 
+#ifndef LIBXL_HAVE_NO_SUSPEND_RESUME
 static const char migrate_receiver_banner[]=
 "xl migration receiver ready, send binary domain data.\n";
 static const char migrate_receiver_ready[]=
@@ -100,6 +101,8 @@ static const char migrate_permission_to_go[]=
 "domain is yours, you are cleared to unpause";
 static const char migrate_report[]=
 "my copy unpause results are as follows";
+#endif
+
   /* followed by one byte:
* 0: everything went well, domain is running
*next thing is we all exit
-- 
2.1.4


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

[Xen-devel] [PATCH] ns16550: mask transmit holding register empty interrupt when tx is stopped

[Chris Patterson]

From: Chris Patterson 

The uart generates an interrupt whenever the transmit holding register is
empty and UART_IER_ETHREI is set in UART_IER.  Currently, Xen's ns16550
driver does not currently mask this interrupt when transmit is stopped,
unlike other platforms such as Linux [1].

Toggle UART_IER_ETHREI flag in the UART_IER according to the state dictated
by stop_tx and start_tx hooks.

On the Tegra platform (forthcoming series), the reset via reading IIR does not
prevent re-assertion of THRE.  This causes Xen to hang in the interrupt
handler's while loop whenever there is no data to transmit.  This behavior 
(bug?)
is addressed by utilizing the start & stop tx hooks.

This has been tested on various x86 PCs for any obvious signs of regressions.

[1] 
http://lxr.free-electrons.com/source/drivers/tty/serial/8250/8250_port.c?v=4.7#L1518

Signed-off-by: Chris Patterson 
---
 xen/drivers/char/ns16550.c | 26 --
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index b2b5f56..cae7f1b 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -656,8 +656,8 @@ static void ns16550_setup_postirq(struct ns16550 *uart)
 ns_write_reg(uart,
  UART_MCR, UART_MCR_OUT2 | UART_MCR_DTR | UART_MCR_RTS);
 
-/* Enable receive and transmit interrupts. */
-ns_write_reg(uart, UART_IER, UART_IER_ERDAI | UART_IER_ETHREI);
+/* Enable receive interrupts. */
+ns_write_reg(uart, UART_IER, UART_IER_ERDAI);
 }
 
 if ( uart->irq >= 0 )
@@ -813,6 +813,26 @@ static int __init ns16550_irq(struct serial_port *port)
 return ((uart->irq > 0) ? uart->irq : -1);
 }
 
+static void ns16550_start_tx(struct serial_port *port)
+{
+struct ns16550 *uart = port->uart;
+unsigned int ier = ns_read_reg(uart, UART_IER);
+
+/* unmask transmit holding register empty interrupt if currently masked */
+if (!(ier & UART_IER_ETHREI))
+ns_write_reg(uart, UART_IER, ier | UART_IER_ETHREI);
+}
+
+static void ns16550_stop_tx(struct serial_port *port)
+{
+struct ns16550 *uart = port->uart;
+unsigned int ier = ns_read_reg(uart, UART_IER);
+
+/* mask off transmit holding register empty interrupt if currently 
unmasked */
+if (ier & UART_IER_ETHREI)
+ns_write_reg(uart, UART_IER, ier & ~UART_IER_ETHREI);
+}
+
 #ifdef CONFIG_ARM
 static const struct vuart_info *ns16550_vuart_info(struct serial_port *port)
 {
@@ -832,6 +852,8 @@ static struct uart_driver __read_mostly ns16550_driver = {
 .putc = ns16550_putc,
 .getc = ns16550_getc,
 .irq  = ns16550_irq,
+.start_tx = ns16550_start_tx,
+.stop_tx  = ns16550_stop_tx,
 #ifdef CONFIG_ARM
 .vuart_info   = ns16550_vuart_info,
 #endif
-- 
2.1.4


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

Re: [Xen-devel] [PATCH] ns16550: mask transmit holding register empty interrupt when tx is stopped

[Chris Patterson]

On Wed, Aug 17, 2016 at 10:29 AM, Jan Beulich  wrote:
>>>> On 17.08.16 at 16:02,  wrote:
>> From: Chris Patterson 
>>
>> The uart generates an interrupt whenever the transmit holding register is
>> empty and UART_IER_ETHREI is set in UART_IER.  Currently, Xen's ns16550
>> driver does not currently mask this interrupt when transmit is stopped,
>> unlike other platforms such as Linux [1].
>>
>> Toggle UART_IER_ETHREI flag in the UART_IER according to the state dictated
>> by stop_tx and start_tx hooks.
>>
>> On the Tegra platform (forthcoming series), the reset via reading IIR does 
>> not
>> prevent re-assertion of THRE.  This causes Xen to hang in the interrupt
>> handler's while loop whenever there is no data to transmit.  This behavior 
>> (bug?)
>> is addressed by utilizing the start & stop tx hooks.
>
> Looks mostly okay from a technical pov, but there are a few minor
> (mostly style) issues.
>
>> [1] 
>> http://lxr.free-electrons.com/source/drivers/tty/serial/8250/8250_port.c?v=4.7#L1518
>
> I'd appreciate for such references to be to the canonical (i.e. Linus'es)
> tree.
>
>> @@ -813,6 +813,26 @@ static int __init ns16550_irq(struct serial_port *port)
>>  return ((uart->irq > 0) ? uart->irq : -1);
>>  }
>>
>> +static void ns16550_start_tx(struct serial_port *port)
>> +{
>> +struct ns16550 *uart = port->uart;
>> +unsigned int ier = ns_read_reg(uart, UART_IER);
>
> Please use u8 or unsigned char here, as is done elsewhere in the file.
>
>> +/* unmask transmit holding register empty interrupt if currently masked 
>> */
>
> Comment style: Upper case at the beginning; the full stop at the
> end has become optional recently.
>
>> +if (!(ier & UART_IER_ETHREI))
>
> Blanks missing inside the parentheses.
>
> Jan
>

ACK, will fix these in v2.  Thank you for the review.

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

[Xen-devel] [PATCH v2] ns16550: mask transmit holding register empty interrupt when tx is stopped

[Chris Patterson]

From: Chris Patterson 

The uart generates an interrupt whenever the transmit holding register is
empty and UART_IER_ETHREI is set in UART_IER.  Currently, Xen's ns16550
driver does not currently mask this interrupt when transmit is stopped,
unlike other platforms such as Linux [1].

Toggle UART_IER_ETHREI flag in the UART_IER according to the state dictated
by stop_tx and start_tx hooks.

On the Tegra platform (forthcoming series), the reset via reading IIR does not
prevent re-assertion of THRE.  This causes Xen to hang in the interrupt
handler's while loop whenever there is no data to transmit.  This behavior 
(bug?)
is addressed by utilizing the start & stop tx hooks.

This has been tested on various x86 PCs for any obvious signs of regressions.

[1] 
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/tty/serial/8250/8250_port.c?id=refs/tags/v4.8-rc2#n1518

Signed-off-by: Chris Patterson 
---
 xen/drivers/char/ns16550.c | 26 --
 1 file changed, 24 insertions(+), 2 deletions(-)

Changes in v2:
- unsigned int to u8
- formatting fixes

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index b2b5f56..1da103a 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -656,8 +656,8 @@ static void ns16550_setup_postirq(struct ns16550 *uart)
 ns_write_reg(uart,
  UART_MCR, UART_MCR_OUT2 | UART_MCR_DTR | UART_MCR_RTS);
 
-/* Enable receive and transmit interrupts. */
-ns_write_reg(uart, UART_IER, UART_IER_ERDAI | UART_IER_ETHREI);
+/* Enable receive interrupts. */
+ns_write_reg(uart, UART_IER, UART_IER_ERDAI);
 }
 
 if ( uart->irq >= 0 )
@@ -813,6 +813,26 @@ static int __init ns16550_irq(struct serial_port *port)
 return ((uart->irq > 0) ? uart->irq : -1);
 }
 
+static void ns16550_start_tx(struct serial_port *port)
+{
+struct ns16550 *uart = port->uart;
+u8 ier = ns_read_reg(uart, UART_IER);
+
+/* Unmask transmit holding register empty interrupt if currently masked. */
+if ( !(ier & UART_IER_ETHREI) )
+ns_write_reg(uart, UART_IER, ier | UART_IER_ETHREI);
+}
+
+static void ns16550_stop_tx(struct serial_port *port)
+{
+struct ns16550 *uart = port->uart;
+u8 ier = ns_read_reg(uart, UART_IER);
+
+/* Mask off transmit holding register empty interrupt if currently 
unmasked. */
+if ( ier & UART_IER_ETHREI )
+ns_write_reg(uart, UART_IER, ier & ~UART_IER_ETHREI);
+}
+
 #ifdef CONFIG_ARM
 static const struct vuart_info *ns16550_vuart_info(struct serial_port *port)
 {
@@ -832,6 +852,8 @@ static struct uart_driver __read_mostly ns16550_driver = {
 .putc = ns16550_putc,
 .getc = ns16550_getc,
 .irq  = ns16550_irq,
+.start_tx = ns16550_start_tx,
+.stop_tx  = ns16550_stop_tx,
 #ifdef CONFIG_ARM
 .vuart_info   = ns16550_vuart_info,
 #endif
-- 
2.1.4


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

Re: [Xen-devel] [Intel-gfx] [PATCH] drm/i915: fix i915 running as dom0 under Xen

[Chris Wilson]

On Thu, Feb 02, 2017 at 11:48:21AM +0100, Daniel Vetter wrote:
> On Thu, Feb 02, 2017 at 10:47:11AM +0100, Juergen Gross wrote:
> > Commit 920cf4194954ec ("drm/i915: Introduce an internal allocator for
> > disposable private objects") introduced a regression for the kernel
> > running as Xen dom0: when switching to graphics mode a GPU HANG
> > occurred.
> > 
> > Reason seems to be a missing adaption similar to that done in
> > commit 7453c549f5f648 ("swiotlb: Export swiotlb_max_segment to users")
> > to i915_gem_object_get_pages_internal().
> > 
> > So limit the maximum page order to be used according to the maximum
> > swiotlb segment size instead to the complete swiotlb size.
> > 
> > Signed-off-by: Juergen Gross 
> Fixes: 920cf4194954 ("drm/i915: Introduce an internal allocator for 
> disposable private objects")
> Cc: Chris Wilson 
> Cc: Tvrtko Ursulin 
> Cc: Daniel Vetter 
> Cc: Jani Nikula 
> Cc: intel-...@lists.freedesktop.org
> Cc:  # v4.10-rc1+
> 
> We have a nice script for these :-)

Pffifle. 7453c549f5f648 allowed Xen to change it and silently conflicted
with those that already used the previous limits, which didn't land in
our tree until v4.10-rc3.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

Re: [Xen-devel] [Intel-gfx] [PATCH] drm/i915: fix i915 running as dom0 under Xen

[Chris Wilson]

On Thu, Feb 02, 2017 at 12:11:29PM +, Tvrtko Ursulin wrote:
> 
> On 02/02/2017 09:47, Juergen Gross wrote:
> >Commit 920cf4194954ec ("drm/i915: Introduce an internal allocator for
> >disposable private objects") introduced a regression for the kernel
> >running as Xen dom0: when switching to graphics mode a GPU HANG
> >occurred.
> >
> >Reason seems to be a missing adaption similar to that done in
> >commit 7453c549f5f648 ("swiotlb: Export swiotlb_max_segment to users")
> >to i915_gem_object_get_pages_internal().
> >
> >So limit the maximum page order to be used according to the maximum
> >swiotlb segment size instead to the complete swiotlb size.
> >
> >Signed-off-by: Juergen Gross 
> >---
> >Please consider for 4.10 as otherwise 4.10 will be unusable as Xen dom0
> >with i915 graphics.
> >---
> > drivers/gpu/drm/i915/i915_gem_internal.c | 12 ++--
> > 1 file changed, 10 insertions(+), 2 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_gem_internal.c 
> >b/drivers/gpu/drm/i915/i915_gem_internal.c
> >index 4b3ff3e..d09c749 100644
> >--- a/drivers/gpu/drm/i915/i915_gem_internal.c
> >+++ b/drivers/gpu/drm/i915/i915_gem_internal.c
> >@@ -66,8 +66,16 @@ i915_gem_object_get_pages_internal(struct 
> >drm_i915_gem_object *obj)
> >
> > max_order = MAX_ORDER;
> > #ifdef CONFIG_SWIOTLB
> >-if (swiotlb_nr_tbl()) /* minimum max swiotlb size is IO_TLB_SEGSIZE */
> >-max_order = min(max_order, ilog2(IO_TLB_SEGPAGES));
> >+if (swiotlb_nr_tbl()) {
> >+unsigned int max_segment;
> >+
> >+max_segment = swiotlb_max_segment();
> >+if (max_segment) {
> >+max_segment = max_t(unsigned int, max_segment,
> >+PAGE_SIZE) >> PAGE_SHIFT;
> >+max_order = min(max_order, ilog2(max_segment));
> >+}
> >+}
> > #endif
> >
> > gfp = GFP_KERNEL | __GFP_HIGHMEM | __GFP_RECLAIMABLE;
> >
> 
> Looks OK to me. We could bikeshed it to only use
> swiotlb_max_segment() which I think was the intention when that API
> was added but can leave that for the future.
> 
> Reviewed-by: Tvrtko Ursulin 

Pushed, I imagine this has been added to the list of sg cleanups you
have :)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

Re: [Xen-devel] [PATCH 22/29] drivers, scsi: convert iscsi_task.refcount from atomic_t to refcount_t

[Chris Leech]

On Mon, Mar 06, 2017 at 04:21:09PM +0200, Elena Reshetova wrote:
> refcount_t type and corresponding API should be
> used instead of atomic_t when the variable is used as
> a reference counter. This allows to avoid accidental
> refcounter overflows that might lead to use-after-free
> situations.
> 
> Signed-off-by: Elena Reshetova 
> Signed-off-by: Hans Liljestrand 
> Signed-off-by: Kees Cook 
> Signed-off-by: David Windsor 

This looks OK to me.

Acked-by: Chris Leech 

> ---
>  drivers/scsi/libiscsi.c| 8 
>  drivers/scsi/qedi/qedi_iscsi.c | 2 +-
>  include/scsi/libiscsi.h| 3 ++-
>  3 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index 834d121..7eb1d2c 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c
> @@ -516,13 +516,13 @@ static void iscsi_free_task(struct iscsi_task *task)
>  
>  void __iscsi_get_task(struct iscsi_task *task)
>  {
> - atomic_inc(&task->refcount);
> + refcount_inc(&task->refcount);
>  }
>  EXPORT_SYMBOL_GPL(__iscsi_get_task);
>  
>  void __iscsi_put_task(struct iscsi_task *task)
>  {
> - if (atomic_dec_and_test(&task->refcount))
> + if (refcount_dec_and_test(&task->refcount))
>   iscsi_free_task(task);
>  }
>  EXPORT_SYMBOL_GPL(__iscsi_put_task);
> @@ -744,7 +744,7 @@ __iscsi_conn_send_pdu(struct iscsi_conn *conn, struct 
> iscsi_hdr *hdr,
>* released by the lld when it has transmitted the task for
>* pdus we do not expect a response for.
>*/
> - atomic_set(&task->refcount, 1);
> + refcount_set(&task->refcount, 1);
>   task->conn = conn;
>   task->sc = NULL;
>   INIT_LIST_HEAD(&task->running);
> @@ -1616,7 +1616,7 @@ static inline struct iscsi_task 
> *iscsi_alloc_task(struct iscsi_conn *conn,
>   sc->SCp.phase = conn->session->age;
>   sc->SCp.ptr = (char *) task;
>  
> - atomic_set(&task->refcount, 1);
> + refcount_set(&task->refcount, 1);
>   task->state = ISCSI_TASK_PENDING;
>   task->conn = conn;
>   task->sc = sc;
> diff --git a/drivers/scsi/qedi/qedi_iscsi.c b/drivers/scsi/qedi/qedi_iscsi.c
> index b9f79d3..3895bd5 100644
> --- a/drivers/scsi/qedi/qedi_iscsi.c
> +++ b/drivers/scsi/qedi/qedi_iscsi.c
> @@ -1372,7 +1372,7 @@ static void qedi_cleanup_task(struct iscsi_task *task)
>  {
>   if (!task->sc || task->state == ISCSI_TASK_PENDING) {
>   QEDI_INFO(NULL, QEDI_LOG_IO, "Returning ref_cnt=%d\n",
> -   atomic_read(&task->refcount));
> +   refcount_read(&task->refcount));
>   return;
>   }
>  
> diff --git a/include/scsi/libiscsi.h b/include/scsi/libiscsi.h
> index b0e275d..24d74b5 100644
> --- a/include/scsi/libiscsi.h
> +++ b/include/scsi/libiscsi.h
> @@ -29,6 +29,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -139,7 +140,7 @@ struct iscsi_task {
>  
>   /* state set/tested under session->lock */
>   int state;
> - atomic_trefcount;
> + refcount_t  refcount;
>   struct list_headrunning;/* running cmd list */
>   void*dd_data;   /* driver/transport data */
>  };
> -- 
> 2.7.4
> 

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

Re: [Xen-devel] [qubes-devel] Re: Critique of the Xen Security Process

[Chris Laprise]

Hello...

On 11/10/2015 05:52 AM, Lars Kurth wrote:

Hi everyone,

firstly I wanted to thank everyone for raising this issue. I wanted to 
point out that we are not talking about a security process here, but 
the development process. Or more accurately the cost of writing more 
secure code and the relative importance of security compared to 
features. And of course the recent increase of relative importance of 
"built-in security as a feature" since Snowden.


On 9 Nov 2015, at 16:31, Franz <169...@gmail.com 
> wrote:


(Please note that all of the above are my personal views, i.e. I'm
explicitly not speaking on behalf of the project.)


It seems positions are antithetic with no possible compromise in 
view. Sadly this is a general problem of FOSS software: developers  
tend to do what they like and not what users request. And who can 
blame developers for that? After all they are working as volunteers 
so they deserve to do what they like. No possible compromise.


I don't think this is a fair statement: more than 95% of developers 
working on Xen are employees of large organisations. And they follow 
the priorities that their employers set. The same is true in Linux and 
for comparable projects and of course for proprietary software 
developers. Blaming open source developers, is simply wrong and not 
constructive.


Nevertheless, Xen is a creature of interfaces that purport to uphold 
contracts. These (software engineering, not legal) contracts are 
explicitly security-themed... the software is promising to isolate 
processes from each other and protect against privilege escalation. The 
Xen project itself asserts that it is focused on the security benefits 
of the hypervisor (to the point of invoking "microkernel" in Xen's 
description), not mere administrative convenience.


What seems to be missing from the defense of Xen project so far in this 
thread is a level of acknowledgement of this very important aspect of 
twenty-first century software development: Can you honor what your 
interfaces communicate to your audience? Its true that some FOSS 
projects do not care for modern methodologies (which are greatly about 
concept and mindset), but should Xen be that way?


Should Xen project also be a blanket absolution of "blame"? And is the 
corporate status of some of its contributors a proper justification for 
being blame-less (perhaps we can think of them like members of 'The 
Borg')? I think not.


This is the trap that FOSS projects most commonly fall into: 'We are 
free to publish because of liberty, but you are not free to criticize.' 
Its odd when you think about it. This is the squandering of user 
motivation and valuable feedback.





In fact, throughout 2014 and 2015, the project has received complaints 
from several large vendors that the Xen Project today is too rigorous 
with code reviews compared to Linux, KVM...



Famous last words before a Heartbleed-scale media circus ensues?




I can certainly raise this suggestion with the Advisory Board and see 
whether we can make some funds available. However, the board has 
already invested nearly 50% of its entire budget in Test 
Infrastructure and is planning to continue to spend in this area at 
roughly this proportion of the projects budget. However, we do not 
have huge amounts of funds: thus, what we could do with bounties would 
necessarily be limited.


I personally also looked at other ways to change the cost-benefit 
equation. One example is the Feature Maturity Lifecycle (see 
http://lists.xen.org/archives/html/xen-devel/2015-11/msg00609.html & 
http://lists.xenproject.org/archives/html/xen-devel/2015-06/msg01992.html) 
which aims to use better classification to change behaviour of 
contributors.


I do hope, that this discussion can remain constructive. De-motivating 
the good work many of our developers (and in particular code 
reviewers) are doing, is really not helpful in this context.




It does seem to me that the suggestion of a Long-Term Support (LTS) 
release is a constructive one, among the other suggestions that Joanna 
made. The FML you cite is interesting, but seems to be aimed squarely at 
developers. You are bound to get better results if the expectations of 
users change along with developers, so LTS releases may be the better idea.



Best Regards
Lars





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

Re: [Xen-devel] [Intel-gfx] [Announcement] 2015-Q3 release of XenGT - a Mediated Graphics Passthrough Solution from Intel

[Chris Wilson]

On Tue, Nov 24, 2015 at 12:19:18PM +0100, Daniel Vetter wrote:
> Downside: Tracking mapping changes on the guest side won't be any easier.
> This is mostly a problem for integrated gpus, since discrete ones usually
> require contiguous vram for scanout. I think saying "don't do that" is a
> valid option though, i.e. we're assuming that page mappings for a in-use
> scanout range never changes on the guest side. That is true for at least
> all the current linux drivers.

Apart from we already suffer limitations of fixed mappings and have patches
that want to change the page mapping of active scanouts.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Xen-devel] [PATCH 1/2] libfsimage: replace deprecated readdir_r() with readdir()

[Chris Patterson]

From: Chris Patterson 

Replace the usage of readdir_r() with readdir() to address
a compilation error due to the deprecation of readdir_r.

glibc has deprecated this for their next release (2.24):
https://sourceware.org/bugzilla/show_bug.cgi?id=19056

Signed-off-by: Chris Patterson 
---
 tools/libfsimage/common/fsimage_plugin.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libfsimage/common/fsimage_plugin.c 
b/tools/libfsimage/common/fsimage_plugin.c
index 3fa06c7..03212e1 100644
--- a/tools/libfsimage/common/fsimage_plugin.c
+++ b/tools/libfsimage/common/fsimage_plugin.c
@@ -147,7 +147,7 @@ static int load_plugins(void)
 
bzero(dp, sizeof (struct dirent) + name_max + 1);
 
-   while (readdir_r(dir, dp, &dpp) == 0 && dpp != NULL) {
+   while ((dpp = readdir(dir)) != NULL) {
if (strcmp(dpp->d_name, ".") == 0)
continue;
if (strcmp(dpp->d_name, "..") == 0)
-- 
2.1.4


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

[Xen-devel] [PATCH 2/2] libxl: replace deprecated readdir_r() with readdir()

[Chris Patterson]

From: Chris Patterson 

Replace the usage of readdir_r() with readdir() to address
a compilation error due to the deprecation of readdir_r.

glibc has deprecated this for their next release (2.24):
https://sourceware.org/bugzilla/show_bug.cgi?id=19056

This also removes the zalloc_dirent() helper function
which is no longer required.

Signed-off-by: Chris Patterson 
---
 tools/libxl/libxl_pvusb.c | 26 --
 tools/libxl/libxl_utils.c |  9 ++---
 2 files changed, 6 insertions(+), 29 deletions(-)

diff --git a/tools/libxl/libxl_pvusb.c b/tools/libxl/libxl_pvusb.c
index 9f1e842..d1397c4 100644
--- a/tools/libxl/libxl_pvusb.c
+++ b/tools/libxl/libxl_pvusb.c
@@ -508,19 +508,10 @@ int libxl_devid_to_device_usbctrl(libxl_ctx *ctx,
 return rc;
 }
 
-static void *zalloc_dirent(libxl__gc *gc, const char *dirpath)
-{
-size_t need = offsetof(struct dirent, d_name) +
-  pathconf(dirpath, _PC_NAME_MAX) + 1;
-
-return libxl__zalloc(gc, need);
-}
-
 static char *usbdev_busaddr_to_busid(libxl__gc *gc, int bus, int addr)
 {
 DIR *dir;
 char *busid = NULL;
-struct dirent *de_buf;
 struct dirent *de;
 
 /* invalid hostbus or hostaddr */
@@ -533,21 +524,17 @@ static char *usbdev_busaddr_to_busid(libxl__gc *gc, int 
bus, int addr)
 return NULL;
 }
 
-de_buf = zalloc_dirent(gc, SYSFS_USB_DEV);
-
 for (;;) {
 char *filename;
 void *buf;
 int busnum = -1;
 int devnum = -1;
 
-int r = readdir_r(dir, de_buf, &de);
-if (r) {
+de = readdir(dir);
+if (!de) {
 LOGE(ERROR, "failed to readdir %s", SYSFS_USB_DEV);
 break;
 }
-if (!de)
-break;
 
 if (!strcmp(de->d_name, ".") ||
 !strcmp(de->d_name, ".."))
@@ -1157,7 +1144,6 @@ static int usbdev_get_all_interfaces(libxl__gc *gc, const 
char *busid,
 {
 DIR *dir;
 char *buf;
-struct dirent *de_buf;
 struct dirent *de;
 int rc;
 
@@ -1172,18 +1158,14 @@ static int usbdev_get_all_interfaces(libxl__gc *gc, 
const char *busid,
 return ERROR_FAIL;
 }
 
-de_buf = zalloc_dirent(gc, SYSFS_USB_DEV);
-
 for (;;) {
-int r = readdir_r(dir, de_buf, &de);
+de = readdir(dir);
 
-if (r) {
+if (!de) {
 LOGE(ERROR, "failed to readdir %s", SYSFS_USB_DEV);
 rc = ERROR_FAIL;
 goto out;
 }
-if (!de)
-break;
 
 if (!strcmp(de->d_name, ".") ||
 !strcmp(de->d_name, ".."))
diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
index ceb8825..613a9d6 100644
--- a/tools/libxl/libxl_utils.c
+++ b/tools/libxl/libxl_utils.c
@@ -548,20 +548,15 @@ int libxl__remove_directory(libxl__gc *gc, const char 
*dirpath)
 goto out;
 }
 
-size_t need = offsetof(struct dirent, d_name) +
-pathconf(dirpath, _PC_NAME_MAX) + 1;
-struct dirent *de_buf = libxl__zalloc(gc, need);
 struct dirent *de;
 
 for (;;) {
-int r = readdir_r(d, de_buf, &de);
-if (r) {
+de = readdir(d);
+if (!de) {
 LOGE(ERROR, "failed to readdir %s for removal", dirpath);
 rc = ERROR_FAIL;
 break;
 }
-if (!de)
-break;
 
 if (!strcmp(de->d_name, ".") ||
 !strcmp(de->d_name, ".."))
-- 
2.1.4


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

[Xen-devel] [PATCH] xen/Makefile: quote HOSTCC and HOSTCXX args

[Chris Patterson]

From: Chris Patterson 

In some cross-compilation environments, the CC/CXX variables may
expand out to more than one argument (to include things
like --sysroot=...).  Quote these to safely pass along.

Signed-off-by: Chris Patterson 
---
 xen/Makefile | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/xen/Makefile b/xen/Makefile
index 0d5f240..b59f95d 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -246,14 +246,14 @@ kconfig := silentoldconfig oldconfig config menuconfig 
defconfig \
randconfig
 .PHONY: $(kconfig)
 $(kconfig):
-   $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig ARCH=$(ARCH) 
SRCARCH=$(SRCARCH) HOSTCC=$(HOSTCC) HOSTCXX=$(HOSTCXX) $@
+   $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig ARCH=$(ARCH) 
SRCARCH=$(SRCARCH) HOSTCC="$(HOSTCC)" HOSTCXX="$(HOSTCXX)" $@
 
 include/config/%.conf: include/config/auto.conf.cmd $(KCONFIG_CONFIG)
-   $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig ARCH=$(ARCH) 
SRCARCH=$(SRCARCH) HOSTCC=$(HOSTCC) HOSTCXX=$(HOSTCXX) silentoldconfig
+   $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig ARCH=$(ARCH) 
SRCARCH=$(SRCARCH) HOSTCC="$(HOSTCC)" HOSTCXX="$(HOSTCXX)" silentoldconfig
 
 # Allow people to just run `make` as before and not force them to configure
 $(KCONFIG_CONFIG):
-   $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig ARCH=$(ARCH) 
SRCARCH=$(SRCARCH) HOSTCC=$(HOSTCC) HOSTCXX=$(HOSTCXX) defconfig
+   $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig ARCH=$(ARCH) 
SRCARCH=$(SRCARCH) HOSTCC="$(HOSTCC)" HOSTCXX="$(HOSTCXX)" defconfig
 
 # Break the dependency chain for the first run
 include/config/auto.conf.cmd: ;
-- 
2.1.4


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

Re: [Xen-devel] [PATCH 1/2] libfsimage: replace deprecated readdir_r() with readdir()

[Chris Patterson]

On Tue, May 31, 2016 at 6:42 AM, George Dunlap  wrote:
> On Mon, May 30, 2016 at 3:32 AM, Chris Patterson  wrote:
>> From: Chris Patterson 
>>
>> Replace the usage of readdir_r() with readdir() to address
>> a compilation error due to the deprecation of readdir_r.
>>
>> glibc has deprecated this for their next release (2.24):
>> https://sourceware.org/bugzilla/show_bug.cgi?id=19056
>>
>> Signed-off-by: Chris Patterson 
>
> Thanks for the patch, Chris.  A bit more background would have been
> helpful -- I did some searching and found a description[1] which says:
>

Thank you. I should have added these details in the commit message or cover.

> In the current POSIX.1 specification (POSIX.1-2008), readdir(3) is
> not required to be thread-safe.  However, in modern
> implementations (including the glibc implementation), concurrent
> calls to readdir(3) that specify different directory streams are
> thread-safe.  Therefore, the use of readdir_r() is generally
> unnecessary in multithreaded programs.  In cases where multiple
> threads must read from the same directory stream, using readdir(3)
> with external synchronization is still preferable to the use of
> readdir_r(), for the reasons given in the points above.
>
> The use of the specific directory stream is single-threaded, so for
> glibc, it looks like using readdir() will be safe.  But libxl needs to
> be able to build on a number of libc's that are not glibc and still be
> thread-safe.  So we need to either 1) verify that readdir() is thread
> safe on all libc's against which we may compile, or 2) add some

Is there a list of supported libc libraries?  I can look into it and
provide more definitive answers if there are.

> #ifdef-ery to switch to readdir_r() on systems unless we know it's
> thread-safe.
>
> I'm less familiar with best practices for this kind of thing -- Ian,
> do you have an idea?
>
> Thanks again for raising this, Chris.
>
>  -George
>
> [1] http://man7.org/linux/man-pages/man3/readdir_r.3.html
>

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

Re: [Xen-devel] [PATCH 1/2] libfsimage: replace deprecated readdir_r() with readdir()

[Chris Patterson]

On Tue, May 31, 2016 at 1:53 PM, Wei Liu  wrote:
> On Tue, May 31, 2016 at 11:43:13AM -0400, Chris Patterson wrote:
>> On Tue, May 31, 2016 at 6:42 AM, George Dunlap  
>> wrote:
>> > On Mon, May 30, 2016 at 3:32 AM, Chris Patterson  wrote:
>> >> From: Chris Patterson 
>> >>
>> >> Replace the usage of readdir_r() with readdir() to address
>> >> a compilation error due to the deprecation of readdir_r.
>> >>
>> >> glibc has deprecated this for their next release (2.24):
>> >> https://sourceware.org/bugzilla/show_bug.cgi?id=19056
>> >>
>> >> Signed-off-by: Chris Patterson 
>> >
>> > Thanks for the patch, Chris.  A bit more background would have been
>> > helpful -- I did some searching and found a description[1] which says:
>> >
>>
>> Thank you. I should have added these details in the commit message or cover.
>>
>> > In the current POSIX.1 specification (POSIX.1-2008), readdir(3) is
>> > not required to be thread-safe.  However, in modern
>> > implementations (including the glibc implementation), concurrent
>> > calls to readdir(3) that specify different directory streams are
>> > thread-safe.  Therefore, the use of readdir_r() is generally
>> > unnecessary in multithreaded programs.  In cases where multiple
>> > threads must read from the same directory stream, using readdir(3)
>> > with external synchronization is still preferable to the use of
>> > readdir_r(), for the reasons given in the points above.
>> >
>> > The use of the specific directory stream is single-threaded, so for
>> > glibc, it looks like using readdir() will be safe.  But libxl needs to
>> > be able to build on a number of libc's that are not glibc and still be
>> > thread-safe.  So we need to either 1) verify that readdir() is thread
>> > safe on all libc's against which we may compile, or 2) add some
>>
>> Is there a list of supported libc libraries?  I can look into it and
>> provide more definitive answers if there are.
>>
>
> We at least care about FreeBSD and NetBSD. Sadly their manuaks don't
> provide statement regarding thread safety for readdir and readdir_r.
> It's better to err on the safe side.
>

I'm far from the expert here, but it would appear that both NetBSD's
and FreeBSD's libc readdir()/readdir_r() implementations are
consistent in their locking mechanisms [1,2].  As such, I would expect
readdir() to be equally as safe as readdir_r() for their users.  As
you noted, there does not appear to be any documented distinction
within their man pages [3,4] with regards to thread safety and it
seems reasonable that they would have documented these limitations, if
present.

[1] FreeBSD: 
https://svnweb.freebsd.org/base/head/lib/libc/gen/readdir.c?view=markup#l98
[2] NetBSD: 
http://cvsweb.netbsd.org/bsdweb.cgi/src/lib/libc/gen/readdir.c?rev=1.25.6.1&content-type=text/x-cvsweb-markup
[3] FreeBSD readdir manpage:
https://www.freebsd.org/cgi/man.cgi?query=readdir§ion=3
[4] NetBSD readdir manpage: http://netbsd.gw.com/cgi-bin/man-cgi?readdir

Are there other known supported libc implementations?

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

Re: [Xen-devel] [PATCH 1/2] libfsimage: replace deprecated readdir_r() with readdir()

[Chris Patterson]

On Wed, Jun 1, 2016 at 8:06 AM, Ian Jackson  wrote:
> Ian Jackson writes ("Re: [Xen-devel] [PATCH 1/2] libfsimage: replace 
> deprecated readdir_r() with readdir()"):
>> Ian Jackson writes ("Re: [Xen-devel] [PATCH 1/2] libfsimage: replace 
>> deprecated readdir_r() with readdir()"):
>> > 2. There may be good reasons to deviate from a formal specification.
>> > Formal specifications can be wrong (for example, they can differ from
>> > established practice, or unuseable, or incoherent).  But there has
>> > been no discussion (at least in this thread on xen-devel) which might
>> > suggest that the POSIX specification is wrongheaded here.
>>
>> I have been helpfully referred by a local irc channel to the following
>> attempt to change posix to require that readdir() is threadsafe in the
>> senses required by libx, and to deprecate readdir_r():
>>
>>  http://austingroupbugs.net/view.php?id=696
>>
>> I find the comment 0001606 by "dalias" (et seq) totally convincing.
>> The published specification of readdir_r is indeed incoherent.  And
>> only contrived implementations of readdir will not be threadsafe in
>> the required sense.
> ...
>> Accordingly, I think all occurrences of readdir_r in our codebase
>> should be replaced by readdir, as proposed by Chris.
>>
>> However, I think the patch is not quite complete, as the change from
>> readdir_r to readdir should also involve removing the local dirent
>> variables associated with each call site.
>
> Also, the commit message needs to be expanded to provide the
> rationale.  It should restate the reasoning provided by "dalias" and
> provide links to the austingroupbugs thread and references to the
> comment numbers.
>
> Ian.

Agreed, will post a v2.  Thanks for the feedback.

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

[Xen-devel] [[PATCH v2 2/2] libxl: replace deprecated readdir_r() with readdir()

[Chris Patterson]

From: Chris Patterson 

Replace the usage of readdir_r() with readdir() to address a
compilation error under glibc due to the deprecation of readdir_r
for their next release (2.24) [1, 2].

Remove code specific to usage of readdir_r which is no longer required,
such as zalloc_dirent().

--

From the GNU libc manual [3]:
"
 It is expected that future versions of POSIX will obsolete readdir_r and
 mandate the level of thread safety for readdir which is provided by the
 GNU C Library and other implementations today.
"

There is a filed bug in the Austin Group Defect Tracker [4]  in which 'dalias'
proposes (in comment 0001632) that:
"
   I would like to propose an alternate solution. For readdir, replace the text:
"The readdir() function need not be thread-safe."
   with:
"If multiple threads call the readdir() function with the same directory
stream argument and without synchronization to preclude simultaneous
access, then the behavior is undefined."

   With this change, the clunky readdir_r function is no longer needed or
   useful, and should probably be deprecated. As the only reasonable way
   to meet the implementation requirements for readdir is to have the dirent
   buffer in the DIR structure, this change should not require any change to
   existing implementations.
"

[1] https://sourceware.org/ml/libc-alpha/2016-02/msg00093.html
[2] https://sourceware.org/bugzilla/show_bug.cgi?id=19056
[3] 
https://www.gnu.org/software/libc/manual/html_node/Reading_002fClosing-Directory.html
[4] http://austingroupbugs.net/view.php?id=696

--

v2:
- Additional detail in commit message
- Fix readdir loop logic (do not treat NULL as error)
- Cleanup additional related (no longer used) code

Signed-off-by: Chris Patterson 
---
 tools/libxl/libxl_pvusb.c | 42 +++---
 tools/libxl/libxl_utils.c | 14 +-
 2 files changed, 4 insertions(+), 52 deletions(-)

diff --git a/tools/libxl/libxl_pvusb.c b/tools/libxl/libxl_pvusb.c
index 9f1e842..01819bd 100644
--- a/tools/libxl/libxl_pvusb.c
+++ b/tools/libxl/libxl_pvusb.c
@@ -508,19 +508,10 @@ int libxl_devid_to_device_usbctrl(libxl_ctx *ctx,
 return rc;
 }
 
-static void *zalloc_dirent(libxl__gc *gc, const char *dirpath)
-{
-size_t need = offsetof(struct dirent, d_name) +
-  pathconf(dirpath, _PC_NAME_MAX) + 1;
-
-return libxl__zalloc(gc, need);
-}
-
 static char *usbdev_busaddr_to_busid(libxl__gc *gc, int bus, int addr)
 {
 DIR *dir;
 char *busid = NULL;
-struct dirent *de_buf;
 struct dirent *de;
 
 /* invalid hostbus or hostaddr */
@@ -533,22 +524,12 @@ static char *usbdev_busaddr_to_busid(libxl__gc *gc, int 
bus, int addr)
 return NULL;
 }
 
-de_buf = zalloc_dirent(gc, SYSFS_USB_DEV);
-
-for (;;) {
+while ((de = readdir(dir)) != NULL) {
 char *filename;
 void *buf;
 int busnum = -1;
 int devnum = -1;
 
-int r = readdir_r(dir, de_buf, &de);
-if (r) {
-LOGE(ERROR, "failed to readdir %s", SYSFS_USB_DEV);
-break;
-}
-if (!de)
-break;
-
 if (!strcmp(de->d_name, ".") ||
 !strcmp(de->d_name, ".."))
 continue;
@@ -1157,9 +1138,7 @@ static int usbdev_get_all_interfaces(libxl__gc *gc, const 
char *busid,
 {
 DIR *dir;
 char *buf;
-struct dirent *de_buf;
 struct dirent *de;
-int rc;
 
 *intfs = NULL;
 *num = 0;
@@ -1172,19 +1151,7 @@ static int usbdev_get_all_interfaces(libxl__gc *gc, 
const char *busid,
 return ERROR_FAIL;
 }
 
-de_buf = zalloc_dirent(gc, SYSFS_USB_DEV);
-
-for (;;) {
-int r = readdir_r(dir, de_buf, &de);
-
-if (r) {
-LOGE(ERROR, "failed to readdir %s", SYSFS_USB_DEV);
-rc = ERROR_FAIL;
-goto out;
-}
-if (!de)
-break;
-
+while ((de = readdir(dir)) != NULL) {
 if (!strcmp(de->d_name, ".") ||
 !strcmp(de->d_name, ".."))
 continue;
@@ -1196,11 +1163,8 @@ static int usbdev_get_all_interfaces(libxl__gc *gc, 
const char *busid,
 }
 }
 
-rc = 0;
-
-out:
 closedir(dir);
-return rc;
+return 0;
 }
 
 /* Encode usb interface so that it could be written to xenstore as a key.
diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
index ceb8825..5730774 100644
--- a/tools/libxl/libxl_utils.c
+++ b/tools/libxl/libxl_utils.c
@@ -548,21 +548,9 @@ int libxl__remove_directory(libxl__gc *gc, const char 
*dirpath)
 goto out;
 }
 
-size_t need = offsetof(struct dirent, d_name) +
-pathconf(dirpath, _PC_NAME_MAX) + 1;
-struct dirent *de_buf = libxl__zalloc(gc, need);
 struct dirent *de;
 
-for (;;) {
-int r = readdir_r(d, de_buf, &de);
-  

[Xen-devel] [[PATCH v2 1/2] libfsimage: replace deprecated readdir_r() with readdir()

[Chris Patterson]

From: Chris Patterson 

Replace the usage of readdir_r() with readdir() to address a
compilation error under glibc due to the deprecation of readdir_r
for their next release (2.24) [1, 2].

--

From the GNU libc manual [3]:
"
 It is expected that future versions of POSIX will obsolete readdir_r and
 mandate the level of thread safety for readdir which is provided by the
 GNU C Library and other implementations today.
"

There is a filed bug in the Austin Group Defect Tracker [4]  in which 'dalias'
proposes (in comment 0001632) that:
"
   I would like to propose an alternate solution. For readdir, replace the text:
"The readdir() function need not be thread-safe."
   with:
"If multiple threads call the readdir() function with the same directory
stream argument and without synchronization to preclude simultaneous
access, then the behavior is undefined."

   With this change, the clunky readdir_r function is no longer needed or
   useful, and should probably be deprecated. As the only reasonable way
   to meet the implementation requirements for readdir is to have the dirent
   buffer in the DIR structure, this change should not require any change to
   existing implementations.
"

[1] https://sourceware.org/ml/libc-alpha/2016-02/msg00093.html
[2] https://sourceware.org/bugzilla/show_bug.cgi?id=19056
[3] 
https://www.gnu.org/software/libc/manual/html_node/Reading_002fClosing-Directory.html
[4] http://austingroupbugs.net/view.php?id=696

--

v2:
- Additional detail in commit message
- Cleanup additional related (no longer used) code

Signed-off-by: Chris Patterson 
---
 tools/libfsimage/common/fsimage_plugin.c | 17 +
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/tools/libfsimage/common/fsimage_plugin.c 
b/tools/libfsimage/common/fsimage_plugin.c
index 3fa06c7..5ab8d93 100644
--- a/tools/libfsimage/common/fsimage_plugin.c
+++ b/tools/libfsimage/common/fsimage_plugin.c
@@ -122,8 +122,7 @@ fail:
 static int load_plugins(void)
 {
const char *fsdir = getenv("FSIMAGE_FSDIR");
-   struct dirent *dp = NULL;
-   struct dirent *dpp;
+   struct dirent *de;
DIR *dir = NULL;
char *tmp = NULL;
size_t name_max;
@@ -139,22 +138,17 @@ static int load_plugins(void)
if ((tmp = malloc(name_max + 1)) == NULL)
goto fail;
 
-   if ((dp = malloc(sizeof (struct dirent) + name_max + 1)) == NULL)
-   goto fail;
-
if ((dir = opendir(fsdir)) == NULL)
goto fail;
 
-   bzero(dp, sizeof (struct dirent) + name_max + 1);
-
-   while (readdir_r(dir, dp, &dpp) == 0 && dpp != NULL) {
-   if (strcmp(dpp->d_name, ".") == 0)
+   while ((de = readdir(dir)) != NULL) {
+   if (strcmp(de->d_name, ".") == 0)
continue;
-   if (strcmp(dpp->d_name, "..") == 0)
+   if (strcmp(de->d_name, "..") == 0)
continue;
 
(void) snprintf(tmp, name_max, "%s/%s/fsimage.so", fsdir,
-   dpp->d_name);
+   de->d_name);
 
if (init_plugin(tmp) != 0)
goto fail;
@@ -167,7 +161,6 @@ fail:
if (dir != NULL)
(void) closedir(dir);
free(tmp);
-   free(dp);
errno = err;
return (ret);
 }
-- 
2.1.4


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

Re: [Xen-devel] [[PATCH v2 2/2] libxl: replace deprecated readdir_r() with readdir()

[Chris Patterson]

On Thu, Jun 2, 2016 at 6:11 AM, Ian Jackson  wrote:
> Chris Patterson writes ("[[PATCH v2 2/2] libxl: replace deprecated 
> readdir_r() with readdir()"):
>> -for (;;) {
>> +while ((de = readdir(dir)) != NULL) {
> ...
>> -int r = readdir_r(dir, de_buf, &de);
>> -if (r) {
>> -LOGE(ERROR, "failed to readdir %s", SYSFS_USB_DEV);
>> -break;
>
> Sadly this is not right because it mishandles errors when reading the
> directory, treating them all as EOF.  See the error handling info
> in the specification for readdir:
>   http://pubs.opengroup.org/onlinepubs/9699919799/functions/readdir.html
>

You're right, it should check for the error afterwards.

How about something along the lines of:

int saved_errno = errno;
errno = 0;
while ((de = readdir(dir)) != NULL) {
...
}

if (errno) {
   LOGE(ERROR, "readdir failed: %s", strerror(errno));
   rc = ERROR_FAIL;
}
errno = saved_errno;
...

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

Re: [Xen-devel] [[PATCH v2 2/2] libxl: replace deprecated readdir_r() with readdir()

[Chris Patterson]

On Thu, Jun 2, 2016 at 12:13 PM, Ian Jackson  wrote:
> Chris Patterson writes ("Re: [[PATCH v2 2/2] libxl: replace deprecated 
> readdir_r() with readdir()"):
>> You're right, it should check for the error afterwards.
>>
>> How about something along the lines of:
>>
>> int saved_errno = errno;
>> errno = 0;
>> while ((de = readdir(dir)) != NULL) {
>> ...
>
> Wrong because you need to set errno=0 before each call to readdir.
> I really think you should abandon your efforts to keep the readdir
> call inside the while() condition :-).
>

I agree.  How does something like this look?

-int r = readdir_r(dir, de_buf, &de);
-
-if (r) {
+   errno = 0;
+   de = readdir(dir);
+
+   if (!de && errno) {

And I'll apply the same construct for tools/libfsimage/common/fsimage_plugin.c.

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

[Xen-devel] [PATCH v3 2/2] libfsimage: replace deprecated readdir_r() with readdir()

[Chris Patterson]

From: Chris Patterson 

Replace the usage of readdir_r() with readdir() to address a
compilation error under glibc due to the deprecation of readdir_r
for their next release (2.24) [1, 2].

Add new error checking on readdir(), and fail if error occurs.

--

From the GNU libc manual [3]:
"
 It is expected that future versions of POSIX will obsolete readdir_r and
 mandate the level of thread safety for readdir which is provided by the
 GNU C Library and other implementations today.
"

There is a filed bug in the Austin Group Defect Tracker [4]  in which 'dalias'
proposes (in comment 0001632) that:
"
   I would like to propose an alternate solution. For readdir, replace the text:
"The readdir() function need not be thread-safe."
   with:
"If multiple threads call the readdir() function with the same directory
stream argument and without synchronization to preclude simultaneous
access, then the behavior is undefined."

   With this change, the clunky readdir_r function is no longer needed or
   useful, and should probably be deprecated. As the only reasonable way
   to meet the implementation requirements for readdir is to have the dirent
   buffer in the DIR structure, this change should not require any change to
   existing implementations.
"

[1] https://sourceware.org/ml/libc-alpha/2016-02/msg00093.html
[2] https://sourceware.org/bugzilla/show_bug.cgi?id=19056
[3] 
https://www.gnu.org/software/libc/manual/html_node/Reading_002fClosing-Directory.html
[4] http://austingroupbugs.net/view.php?id=696

Signed-off-by: Chris Patterson 
---

v2:
- Additional detail in commit message
- Cleanup additional (no longer used) code

v3:
- Add error check and fail if readdir() fails.

 tools/libfsimage/common/fsimage_plugin.c | 21 -
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/tools/libfsimage/common/fsimage_plugin.c 
b/tools/libfsimage/common/fsimage_plugin.c
index 3fa06c7..0744e7b 100644
--- a/tools/libfsimage/common/fsimage_plugin.c
+++ b/tools/libfsimage/common/fsimage_plugin.c
@@ -123,7 +123,6 @@ static int load_plugins(void)
 {
const char *fsdir = getenv("FSIMAGE_FSDIR");
struct dirent *dp = NULL;
-   struct dirent *dpp;
DIR *dir = NULL;
char *tmp = NULL;
size_t name_max;
@@ -139,22 +138,26 @@ static int load_plugins(void)
if ((tmp = malloc(name_max + 1)) == NULL)
goto fail;
 
-   if ((dp = malloc(sizeof (struct dirent) + name_max + 1)) == NULL)
-   goto fail;
-
if ((dir = opendir(fsdir)) == NULL)
goto fail;
 
-   bzero(dp, sizeof (struct dirent) + name_max + 1);
+   for (;;) {
+   errno = 0;
+   dp = readdir(dir);
+
+   if (dp == NULL && errno != 0)
+   goto fail;
+
+   if (dp == NULL)
+   break;
 
-   while (readdir_r(dir, dp, &dpp) == 0 && dpp != NULL) {
-   if (strcmp(dpp->d_name, ".") == 0)
+   if (strcmp(dp->d_name, ".") == 0)
continue;
-   if (strcmp(dpp->d_name, "..") == 0)
+   if (strcmp(dp->d_name, "..") == 0)
continue;
 
(void) snprintf(tmp, name_max, "%s/%s/fsimage.so", fsdir,
-   dpp->d_name);
+   dp->d_name);
 
if (init_plugin(tmp) != 0)
goto fail;
-- 
2.1.4


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

[Xen-devel] [PATCH v3 1/2] libxl: replace deprecated readdir_r() with readdir()

[Chris Patterson]

From: Chris Patterson 

Replace the usage of readdir_r() with readdir() to address a
compilation error under glibc due to the deprecation of readdir_r
for their next release (2.24) [1, 2].

Remove code specific to usage of readdir_r which is no longer required,
such as zalloc_dirent().

--

From the GNU libc manual [3]:
"
 It is expected that future versions of POSIX will obsolete readdir_r and
 mandate the level of thread safety for readdir which is provided by the
 GNU C Library and other implementations today.
"

There is a filed bug in the Austin Group Defect Tracker [4]  in which 'dalias'
proposes (in comment 0001632) that:
"
   I would like to propose an alternate solution. For readdir, replace the text:
"The readdir() function need not be thread-safe."
   with:
"If multiple threads call the readdir() function with the same directory
stream argument and without synchronization to preclude simultaneous
access, then the behavior is undefined."

   With this change, the clunky readdir_r function is no longer needed or
   useful, and should probably be deprecated. As the only reasonable way
   to meet the implementation requirements for readdir is to have the dirent
   buffer in the DIR structure, this change should not require any change to
   existing implementations.
"

[1] https://sourceware.org/ml/libc-alpha/2016-02/msg00093.html
[2] https://sourceware.org/bugzilla/show_bug.cgi?id=19056
[3] 
https://www.gnu.org/software/libc/manual/html_node/Reading_002fClosing-Directory.html
[4] http://austingroupbugs.net/view.php?id=696

Signed-off-by: Chris Patterson 
---

v2:
- Additional detail in commit message
- Fix readdir loop logic (do not treat NULL as error)
- Cleanup additional (no longer used) code

v3:
- Restore original loop & error handling

 tools/libxl/libxl_pvusb.c | 24 ++--
 tools/libxl/libxl_utils.c |  8 +++-
 2 files changed, 9 insertions(+), 23 deletions(-)

diff --git a/tools/libxl/libxl_pvusb.c b/tools/libxl/libxl_pvusb.c
index 9f1e842..ec97ff8 100644
--- a/tools/libxl/libxl_pvusb.c
+++ b/tools/libxl/libxl_pvusb.c
@@ -508,19 +508,10 @@ int libxl_devid_to_device_usbctrl(libxl_ctx *ctx,
 return rc;
 }
 
-static void *zalloc_dirent(libxl__gc *gc, const char *dirpath)
-{
-size_t need = offsetof(struct dirent, d_name) +
-  pathconf(dirpath, _PC_NAME_MAX) + 1;
-
-return libxl__zalloc(gc, need);
-}
-
 static char *usbdev_busaddr_to_busid(libxl__gc *gc, int bus, int addr)
 {
 DIR *dir;
 char *busid = NULL;
-struct dirent *de_buf;
 struct dirent *de;
 
 /* invalid hostbus or hostaddr */
@@ -533,16 +524,15 @@ static char *usbdev_busaddr_to_busid(libxl__gc *gc, int 
bus, int addr)
 return NULL;
 }
 
-de_buf = zalloc_dirent(gc, SYSFS_USB_DEV);
-
 for (;;) {
 char *filename;
 void *buf;
 int busnum = -1;
 int devnum = -1;
 
-int r = readdir_r(dir, de_buf, &de);
-if (r) {
+errno = 0;
+de = readdir(dir);
+if (!de && errno) {
 LOGE(ERROR, "failed to readdir %s", SYSFS_USB_DEV);
 break;
 }
@@ -1157,7 +1147,6 @@ static int usbdev_get_all_interfaces(libxl__gc *gc, const 
char *busid,
 {
 DIR *dir;
 char *buf;
-struct dirent *de_buf;
 struct dirent *de;
 int rc;
 
@@ -1172,12 +1161,11 @@ static int usbdev_get_all_interfaces(libxl__gc *gc, 
const char *busid,
 return ERROR_FAIL;
 }
 
-de_buf = zalloc_dirent(gc, SYSFS_USB_DEV);
-
 for (;;) {
-int r = readdir_r(dir, de_buf, &de);
+errno = 0;
+de = readdir(dir);
 
-if (r) {
+if (!de && errno) {
 LOGE(ERROR, "failed to readdir %s", SYSFS_USB_DEV);
 rc = ERROR_FAIL;
 goto out;
diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
index ceb8825..4ca6bcb 100644
--- a/tools/libxl/libxl_utils.c
+++ b/tools/libxl/libxl_utils.c
@@ -548,14 +548,12 @@ int libxl__remove_directory(libxl__gc *gc, const char 
*dirpath)
 goto out;
 }
 
-size_t need = offsetof(struct dirent, d_name) +
-pathconf(dirpath, _PC_NAME_MAX) + 1;
-struct dirent *de_buf = libxl__zalloc(gc, need);
 struct dirent *de;
 
 for (;;) {
-int r = readdir_r(d, de_buf, &de);
-if (r) {
+errno = 0;
+de = readdir(d);
+if (!de && errno) {
 LOGE(ERROR, "failed to readdir %s for removal", dirpath);
 rc = ERROR_FAIL;
 break;
-- 
2.1.4


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

[Xen-devel] VGA Passthrough Tested Adatpers

[Chris Law]

Hey can someone add AMD Radeon R9 390 to the Tested Adapters list here 
http://wiki.xenproject.org/wiki/Xen_VGA_Passthrough_Tested_Adapters
 
I've requested edit access, but don't yet have it.
 
Thanks,
 
Chris. 
  ___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [Wg-test-framework] osstest going live in new test colo

[Chris Shepherd]

And thanks Ian for your vision and dogged persistence on this. Great to have 
this up and running!


-Original Message-
From: wg-test-framework-boun...@lists.xenproject.org 
[mailto:wg-test-framework-boun...@lists.xenproject.org] On Behalf Of Ian Jackson
Sent: 26 March 2015 13:25
To: wg-test-framew...@lists.xenproject.org; xen-de...@lists.xenproject.org
Cc: Ian Campbell
Subject: [Wg-test-framework] osstest going live in new test colo

Based on our commissioning tests, the new facility now has greater capacity and 
better reliability than the existing setup in the Citrix Cambridge office.

I'm therefore going to set the new system going in production mode, and shut 
down the old one.

There are a few loose ends to get the cron jobs working right which I expect to 
have sorted out very soon.  In the meantime I have already instructed the old 
system to drain its queue, with the expectation of going fully live with the 
new system on Friday.

You will be able to tell when test reports are coming from the new system 
because they will have flight numbers >=5.


There are still some problems with the new facility.  Of the 24 x86 test hosts, 
only 12 are currently live.  Of the remainder:

 * 2 pairs (4 hosts, chardonnay[01] and elbling[01]) are affected by 2
   different kernel/hypervisor bugs.  I have not yet reported these
   properly, but will do so shortly.  I hope to get community help
   with resolving them.

 * 1 pair (2 hosts, oseleta[01]) are affected by what appears to be a
   BIOS bug that sometimes prevents the machine rebooting; this is
   with our supplier.

 * 1 pair (2 hosts, godello[01]) cannot boot Debian wheezy i386, nor
   boot under Xen.  I suspect buggy ACPI tables in the BIOS but have
   not yet investigated in detail.

 * For 2 pairs (4 hosts, huxelrebe[01] and rimava[01]) we have yet to
   gain useable remote access to the BIOS configuration.  This is
   being worked on by our supplier with advice from hardware vendors.

However, the existing setup has only x86 10 hosts, so the new lab is bigger.  
The new colo has better (more, and more reliable) ARM hosts
(cubietruck-* and arndale-*).  And the existing setup is affected by various 
problems with the underlying infrastructure.


Thanks very much to Ian Campbell and Lars Kurth for their help, and for the 
support and patience of the Xen Project Test Working Group and the community in 
general.

Ian.

___
Wg-test-framework mailing list
wg-test-framew...@lists.xenproject.org
http://lists.xenproject.org/cgi-bin/mailman/listinfo/wg-test-framework

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

[Xen-devel] [PATCH v3 0/2] xen: arm: Give PTE bits explict values

[Chris Brand]

This is more-or-less what Julien requested. He noted that pt.config
was never set to zero. When I looked further, I found other bits that
were also never given a value. Looking at the callsites, they almost
all seem to assume a value of zero, so that's what I went with.

Patch 1 re-orders the assignments to match the declaration, making it
clearer which ones are missing. Patch 2 then adds the missing bits.

Chris

Chris Brand (2):
  xen: arm re-order assignments in mfn_to_xen_entry()
  xen: arm: Be explicit about bit values in mfn_to_xen_entry()

 xen/include/asm-arm/page.h | 17 ++---
 1 file changed, 10 insertions(+), 7 deletions(-)

-- 
1.9.1


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

[Xen-devel] [PATCH v3 2/2] xen: arm: Be explicit about bit values in mfn_to_xen_entry()

[Chris Brand]

Ensure that every relevant bit is given an explicit value.
This has no effect on the generated code, but makes it
a little easier to follow.

Reported-by: Julien Grall 
Signed-off-by: Chris Brand 
---
v3 trims down the list of bits given explicit values
v2 adds comments on pxn and avail

 xen/include/asm-arm/page.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index 01628f3e96cb..a94e978a9995 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -202,9 +202,12 @@ static inline lpae_t mfn_to_xen_entry(unsigned long mfn, 
unsigned attr)
 .ai = attr,
 .ns = 1,  /* Hyp mode is in the non-secure world */
 .user = 1,/* See below */
+.ro = 0,  /* Assume read-write */
 .af = 1,  /* No need for access tracking */
 .ng = 1,  /* Makes TLB flushes easier */
+.contig = 0,  /* Assume non-contiguous */
 .xn = 1,  /* No need to execute outside .text */
+.avail = 0,   /* Reference count for domheap mapping */
 }};;
 /* Setting the User bit is strange, but the ATS1H[RW] instructions
  * don't seem to work otherwise, and since we never run on Xen
-- 
1.9.1


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

[Xen-devel] [PATCH v3 1/2] xen: arm re-order assignments in mfn_to_xen_entry()

[Chris Brand]

Shuffle lines around so that the assignments in mfn_to_xen_entry()
occur in the same order as the bits are declared in lpae_pt_t.
This makes it easier to see which ones are never given a value.
No change in behaviour.

Also fix a minor comment typo.

Signed-off-by: Chris Brand 
Reviewed-by: Julien Grall 
Acked-by: Ian Campbell 
---
 xen/include/asm-arm/page.h | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index 5ecfd0705e07..01628f3e96cb 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -197,18 +197,18 @@ static inline lpae_t mfn_to_xen_entry(unsigned long mfn, 
unsigned attr)
 paddr_t pa = ((paddr_t) mfn) << PAGE_SHIFT;
 lpae_t e = (lpae_t) {
 .pt = {
-.xn = 1,  /* No need to execute outside .text */
-.ng = 1,  /* Makes TLB flushes easier */
-.af = 1,  /* No need for access tracking */
+.valid = 1,   /* Mappings are present */
+.table = 0,   /* Set to 1 for links and 4k maps */
+.ai = attr,
 .ns = 1,  /* Hyp mode is in the non-secure world */
 .user = 1,/* See below */
-.ai = attr,
-.table = 0,   /* Set to 1 for links and 4k maps */
-.valid = 1,   /* Mappings are present */
+.af = 1,  /* No need for access tracking */
+.ng = 1,  /* Makes TLB flushes easier */
+.xn = 1,  /* No need to execute outside .text */
 }};;
 /* Setting the User bit is strange, but the ATS1H[RW] instructions
  * don't seem to work otherwise, and since we never run on Xen
- * pagetables un User mode it's OK.  If this changes, remember
+ * pagetables in User mode it's OK.  If this changes, remember
  * to update the hard-coded values in head.S too */
 
 switch ( attr )
-- 
1.9.1


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

[Xen-devel] [PATCH 1/2 v2] xen: arm re-order assignments in mfn_to_xen_entry()

[Chris Brand]

Shuffle lines around so that the assignments in mfn_to_xen_entry()
occur in the same order as the bits are declared in lpae_pt_t.
This makes it easier to see which ones are never given a value.
No change in behaviour.

Also fix a minor comment typo.

Signed-off-by: Chris Brand 
Reviewed-by: Julien Grall 
---
 xen/include/asm-arm/page.h | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index 5ecfd0705e07..01628f3e96cb 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -197,18 +197,18 @@ static inline lpae_t mfn_to_xen_entry(unsigned long mfn, 
unsigned attr)
 paddr_t pa = ((paddr_t) mfn) << PAGE_SHIFT;
 lpae_t e = (lpae_t) {
 .pt = {
-.xn = 1,  /* No need to execute outside .text */
-.ng = 1,  /* Makes TLB flushes easier */
-.af = 1,  /* No need for access tracking */
+.valid = 1,   /* Mappings are present */
+.table = 0,   /* Set to 1 for links and 4k maps */
+.ai = attr,
 .ns = 1,  /* Hyp mode is in the non-secure world */
 .user = 1,/* See below */
-.ai = attr,
-.table = 0,   /* Set to 1 for links and 4k maps */
-.valid = 1,   /* Mappings are present */
+.af = 1,  /* No need for access tracking */
+.ng = 1,  /* Makes TLB flushes easier */
+.xn = 1,  /* No need to execute outside .text */
 }};;
 /* Setting the User bit is strange, but the ATS1H[RW] instructions
  * don't seem to work otherwise, and since we never run on Xen
- * pagetables un User mode it's OK.  If this changes, remember
+ * pagetables in User mode it's OK.  If this changes, remember
  * to update the hard-coded values in head.S too */
 
 switch ( attr )
-- 
1.9.1


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

[Xen-devel] [PATCH 0/2 v2] xen: arm: Ensure all PTE bits have a known value

[Chris Brand]

This is more-or-less what Julien requested. He noted that pt.config
was never set to zero. When I looked further, I found other bits that
were also never given a value. Looking at the callsites, they almost
all seem to assume a value of zero, so that's what I went with.

Patch 1 re-orders the assignments to match the declaration, making it
clearer which ones are missing. Patch 2 then adds the missing bits.

Chris

Chris Brand (2):
  xen: arm re-order assignments in mfn_to_xen_entry()
  xen: arm: Set all bits in mfn_to_xen_entry()

 xen/include/asm-arm/page.h | 19 ---
 1 file changed, 12 insertions(+), 7 deletions(-)

-- 
1.9.1


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

[Xen-devel] [PATCH 2/2 v2] xen: arm: Set all bits in mfn_to_xen_entry()

[Chris Brand]

Ensure that every bit has a specific value.

Reported-by: Julien Grall 
Signed-off-by: Chris Brand 
---
v2 adds comments on pxn and avail.

 xen/include/asm-arm/page.h | 5 +
 1 file changed, 5 insertions(+)

diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index 01628f3e96cb..4f430a5ff4fa 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -202,9 +202,14 @@ static inline lpae_t mfn_to_xen_entry(unsigned long mfn, 
unsigned attr)
 .ai = attr,
 .ns = 1,  /* Hyp mode is in the non-secure world */
 .user = 1,/* See below */
+.ro = 0,  /* Assume read-write */
 .af = 1,  /* No need for access tracking */
 .ng = 1,  /* Makes TLB flushes easier */
+.sbz = 0,
+.contig = 0,  /* Assume non-contiguous */
+.pxn = 0, /* Reserved for PL2 stage 1 page table */
 .xn = 1,  /* No need to execute outside .text */
+.avail = 0,   /* Reference count for domheap mapping */
 }};;
 /* Setting the User bit is strange, but the ATS1H[RW] instructions
  * don't seem to work otherwise, and since we never run on Xen
-- 
1.9.1


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

[Xen-devel] [PATCH v3] xen: arm: Support <32MB frametables

[Chris Brand]

setup_frametable_mappings() rounds frametable_size up to a multiple
of 32MB. This is wasteful on systems with less than 4GB of RAM,
although it does allow the "contig" bit to be set in the PTEs.

Where the frametable is less than 32MB in size, instead round up
to a multiple of 2MB, not setting the "contig" bit in the PTEs.

Signed-off-by: Chris Brand 
---
Changed in v2: merged create_32mb_mappings() and create_2mb_mappings()
into create_mappings(). A side-effect is to fix the bug in v1 for
ARM64 systems with <4GB RAM.

Changed in v3: Ensure that the granularity variable is used even
in a non-debug build.

 xen/arch/arm/mm.c | 37 ++---
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index ae0f34c3c480..36300d4402b2 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -628,25 +628,31 @@ void __cpuinit mmu_init_secondary_cpu(void)
 }
 
 /* Create Xen's mappings of memory.
- * Base and virt must be 32MB aligned and size a multiple of 32MB.
+ * Mapping_size must be either 2MB or 32MB.
+ * Base and virt must be mapping_size aligned.
+ * Size must be a multiple of mapping_size.
  * second must be a contiguous set of second level page tables
  * covering the region starting at virt_offset. */
-static void __init create_32mb_mappings(lpae_t *second,
-unsigned long virt_offset,
-unsigned long base_mfn,
-unsigned long nr_mfns)
+static void __init create_mappings(lpae_t *second,
+   unsigned long virt_offset,
+   unsigned long base_mfn,
+   unsigned long nr_mfns,
+   unsigned int mapping_size)
 {
 unsigned long i, count;
+const unsigned long granularity = mapping_size >> PAGE_SHIFT;
 lpae_t pte, *p;
 
-ASSERT(!((virt_offset >> PAGE_SHIFT) % (16 * LPAE_ENTRIES)));
-ASSERT(!(base_mfn % (16 * LPAE_ENTRIES)));
-ASSERT(!(nr_mfns % (16 * LPAE_ENTRIES)));
+ASSERT((mapping_size == MB(2)) || (mapping_size == MB(32)));
+ASSERT(!((virt_offset >> PAGE_SHIFT) % granularity));
+ASSERT(!(base_mfn % granularity));
+ASSERT(!(nr_mfns % granularity));
 
 count = nr_mfns / LPAE_ENTRIES;
 p = second + second_linear_offset(virt_offset);
 pte = mfn_to_xen_entry(base_mfn, WRITEALLOC);
-pte.pt.contig = 1;  /* These maps are in 16-entry contiguous chunks. */
+if ( granularity == 16 * LPAE_ENTRIES )
+pte.pt.contig = 1;  /* These maps are in 16-entry contiguous chunks. */
 for ( i = 0; i < count; i++ )
 {
 write_pte(p + i, pte);
@@ -660,7 +666,7 @@ static void __init create_32mb_mappings(lpae_t *second,
 void __init setup_xenheap_mappings(unsigned long base_mfn,
unsigned long nr_mfns)
 {
-create_32mb_mappings(xen_second, XENHEAP_VIRT_START, base_mfn, nr_mfns);
+create_mappings(xen_second, XENHEAP_VIRT_START, base_mfn, nr_mfns, MB(32));
 
 /* Record where the xenheap is, for translation routines. */
 xenheap_virt_end = XENHEAP_VIRT_START + nr_mfns * PAGE_SIZE;
@@ -749,6 +755,7 @@ void __init setup_frametable_mappings(paddr_t ps, paddr_t 
pe)
 unsigned long nr_pdxs = pfn_to_pdx(nr_pages);
 unsigned long frametable_size = nr_pdxs * sizeof(struct page_info);
 unsigned long base_mfn;
+const unsigned long mapping_size = frametable_size < MB(32) ? MB(2) : 
MB(32);
 #ifdef CONFIG_ARM_64
 lpae_t *second, pte;
 unsigned long nr_second, second_base;
@@ -756,9 +763,8 @@ void __init setup_frametable_mappings(paddr_t ps, paddr_t 
pe)
 #endif
 
 frametable_base_pdx = pfn_to_pdx(ps >> PAGE_SHIFT);
-
-/* Round up to 32M boundary */
-frametable_size = (frametable_size + 0x1ff) & ~0x1ff;
+/* Round up to 2M or 32M boundary, as appropriate. */
+frametable_size = ROUNDUP(frametable_size, mapping_size);
 base_mfn = alloc_boot_pages(frametable_size >> PAGE_SHIFT, 32<<(20-12));
 
 #ifdef CONFIG_ARM_64
@@ -771,9 +777,10 @@ void __init setup_frametable_mappings(paddr_t ps, paddr_t 
pe)
 pte.pt.table = 1;
 write_pte(&xen_first[first_table_offset(FRAMETABLE_VIRT_START)+i], 
pte);
 }
-create_32mb_mappings(second, 0, base_mfn, frametable_size >> PAGE_SHIFT);
+create_mappings(second, 0, base_mfn, frametable_size >> PAGE_SHIFT, 
mapping_size);
 #else
-create_32mb_mappings(xen_second, FRAMETABLE_VIRT_START, base_mfn, 
frametable_size >> PAGE_SHIFT);
+create_mappings(xen_second, FRAMETABLE_VIRT_START,
+base_mfn, frametable_size >> PAGE_SHIFT, mapping_size);
 #endif
 
 memset(&frame_table[0], 0, nr_pdxs * sizeof(struct page_info));
-- 
1.9.1


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

Re: [Xen-devel] [PATCH] xen/arm: Propagate clock-frequency to DOMU if present in the DT timer node

[Chris (Christopher) Brand]

Hi Julien,

>When the property "clock-frequency" is present in the DT timer node, it means 
>that the bootloader/firmware didn't correctly configured the
>CNTFRQ/CNTFRQ_EL0 on each processor.

I will test this patch, but it doesn't apply cleanly to the version of Xen I'm 
currently using, so I need to update that first.

I also looked at whether it would be possible to set the CNTFRQ register in the 
other cores when they come up. Eventually, I think we should do this in the 
(platform-specific) PSCI code. There doesn't seem to be a suitable hook in the 
platform-specific Xen code - it looks like all the code there related to 
bringing up secondary cores runs on the primary.

Chris


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

Re: [Xen-devel] [PATCH] xen/arm: Propagate clock-frequency to DOMU if present in the DT timer node

[Chris (Christopher) Brand]

Hi Julien,

>When the property "clock-frequency" is present in the DT timer node, it means 
>that the bootloader/firmware didn't correctly configured the
CNTFRQ/CNTFRQ_EL0 on each processor.

I did try this out, and it didn't affect my results. I don't understand why, 
though :-)

What I see is that in preinit_xen_time(), the call to dt_property_read_u32() 
returns zero. When I built Xen, I set CONFIG_DTB_FILE, and looking at the 
corresponding dts file it has a timer node with a clock-frequency property. I 
know that our bootloader also creates a DTB, though, and it looks like that one 
does *not* have a clock-frequency property in the timer node, so I guess Xen 
ends up using that one somehow. CNTFRQ on core 0 (only) is also set to the 
correct frequency, so I end up with the correct frequency in my Dom0 kernel 
anyway.

I'm guessing this is me not understanding what's happening in my system rather 
than anything wrong with the patch.

Chris



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

Re: [Xen-devel] [PATCH] xen/arm: Propagate clock-frequency to DOMU if present in the DT timer node

[Chris (Christopher) Brand]

Hi Julien,

>My test was limited as I don't have a platform where CNTFRQ/CNTFRQ_EL0 is not 
>valid. I may have done a mistake in the code.

Understood. That's why I thought it would be worthwhile posting my results :-)

>> What I see is that in preinit_xen_time(), the call to dt_property_read_u32() 
>> returns zero. When I built Xen, I set CONFIG_DTB_FILE, and looking at the 
>> corresponding dts file it has a timer node with a clock-frequency property. 
>> I know that our bootloader also creates a DTB, though, and it looks like 
>> that one does *not* have a clock-frequency property in the timer node, so I 
>> guess Xen ends up using that one somehow. CNTFRQ on core 0 (only) is also 
>> set to the correct frequency, so I end up with the correct frequency in my 
>> Dom0 kernel anyway.
>
>dt_property_read_u32 returns 0 when it cannot find a property or because the 
>size of the value is not valid.
>
>The device tree provided via CONFIG_DTB_FILE will always take precedence to 
>the one pass by the bootloader.
>
>How do you set CONFIG_DTB_FILE?

A simple "export CONFIG_DTB_FILE=...".

>I would also look to see if by any chance the wrong device tree is set via 
>CONFIG_DTB_FILE.

I did double-check before I posted, and everything looks right to me. I 
certainly could have missed something. It does look like it's *somehow* getting 
the DT generated by the bootloader.

>You can check what is the device tree used by dumping it from DOM0. 
>Thought, it may be slightly different (some nodes are rewritten). You can dump 
>it using dtc -I fs /proc/device-tree -O dts

When I do that (at a shell prompt in Dom0), the timer node does not have a 
clock-frequency property. So your patch isn't designed to help in this case 
(CNTFRQ set on core 0 only, no clock-frequency property in timer node of DT).

Chris


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

Re: [Xen-devel] [PATCH] xen/arm: Propagate clock-frequency to DOMU if present in the DT timer node

[Chris (Christopher) Brand]

Hi Julien,

>> A simple "export CONFIG_DTB_FILE=...".
>
>Are you using an absolute path?

Absolute path, yes.

>> So your patch isn't designed to help in this case (CNTFRQ set on core 0 
>> only, no clock-frequency property in timer node of DT).
>
>Well, my patch is designed to propagate the "clock-frequency" property in DOMU 
>via the toolstack. The propagation for Dom0 is already present in Xen since 
>last year [1].

Understood. In my case, the DomU kernel dies during boot if it happens to be 
scheduled on a core other than core 0 (it comes up fine if it happens to run on 
core 0). Core 0 has CNTFRQ set, but the other cores don't. We will fix this for 
real by writing to CNTFRQ on the other cores in the PSCI code in the SMC.

>Does your bootloader create Xen nodes (multiboot, chosen, ...) or the device 
>tree provided by the bootloader contain such nodes? If not, then you are using 
>an appended DT (via CONFIG_DTB_FILE) to Xen.

It doesn't do that, no. In that case the mystery is why Xen apparently doesn't 
see the clock-frequency property. I'll keep investigating.

>If you didn't yet do it, I would try to clean Xen repository and try to 
>rebuild Xen to see if the build system was using a stall DTB by mistake.

I have, but I'll do it again.

Chris


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

Re: [Xen-devel] [PATCH] xen/arm: Propagate clock-frequency to DOMU if present in the DT timer node

[Chris (Christopher) Brand]

Hi Julien,

The patch does work exactly as advertised.

When I used dtc to convert CONFIG_DTB_FILE from dtb to dts, I could see that it 
didn't in fact have a timer clock-frequency node. After re-creating the dtb and 
rebuilding Xen, "ls /proc/device-tree/timer/" shows a clock-frequency file. 
When I then fire up DomU and do the same command, it too has a clock-frequency 
node.

Thanks,

Chris


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

Re: [Xen-devel] [PATCH] xen/arm: Propagate clock-frequency to DOMU if present in the DT timer node

[Chris (Christopher) Brand]

>Can I get your Tested-by on this patch?

Tested-by: Chris Brand 

Chris


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

[Xen-devel] Bug in devicetree_for_each_node() in xen/arch/arm/bootfdt.c ?

[Chris (Christopher) Brand]

I've been trying to figure out why Xen only reports 2GB on my ARM platform that 
actually has 3GB, and I think I've found a bug, but I'm not familiar enough 
with the Xen code to fix it.

The relevant parts of my dts are:
/dts-v1/;

/ {
 model = "Broadcom STB (7445d0)";
 compatible = "brcm,bcm7445d0", "brcm,brcmstb";
 #address-cells = <0x2>;
 #size-cells = <0x2>;
 interrupt-parent = <0x1>;

 memory {
   #address-cells = <0x1>;
   #size-cells = <0x1>;
   device_type = "memory";
   reg = <0x0 0x0 0x0 0x4000 0x0 0x4000 0x0 0x4000>;

   region@1000 {
contiguous-region;
reg = <0x1000 0x1f80>;
linux,phandle = <0x2>;
phandle = <0x2>;
   };

   region@3000 {
contiguous-region;
reg = <0x3000 0x1000>;
linux,phandle = <0x3>;
phandle = <0x3>;
   };

   region@4000 {
contiguous-region;
reg = <0x4000 0x4000>;
linux,phandle = <0x4>;
phandle = <0x4>;
   };

   region@8000 {
contiguous-region;
reg = <0x8000 0x4000>;
linux,phandle = <0x5>;
phandle = <0x5>;
   };
 };

As you can see, it specifies 0 + 1GB + 1GB + 1GB. When I run "xl info" in Dom0, 
though, it reports "Total 2048".

Digging into the code, I found that in bootinfo.mem.nr_banks is 2 rather than 
the expected 3 (or 4?). That turned out to be because in process_memory_node(), 
address_cells and size_cells were both 2 and so the prop_len of 32 was 
resulting in "banks" being set to 2.

Looking at device_tree_for_each_node(), it looks like something is wrong 
because it contains this loop:
for ( node = 0, depth = 0;
  node >=0 && depth >= 0;
  node = fdt_next_node(fdt, node, &depth) )
{
[...]
ret = func(fdt, node, name, depth,
   address_cells[depth-1], size_cells[depth-1], data);
which looks like it will read before the start of the array for the first node, 
when depth=0. My first instinct was to remove those two "-1"s, but the 
resulting code didn't run, so I figured I'd try to enlist some help :) Of 
course it's possible that my problem is unrelated to this, but reading before 
the start of the array definitely seems like a bug that should be fixed 
(although in practice those values are never used for node 0). Looking through 
the history, it seems to have been like that since the function was first 
introduced 
(http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=9cf4a9a467171c8a2c3d97c2bfadb1bc8b15a3d6).

I'm happy to test out any patches.

Chris
(Not subscribed to the list)

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

Re: [Xen-devel] Bug in devicetree_for_each_node() in xen/arch/arm/bootfdt.c ?

[Chris (Christopher) Brand]

>As said by David, Xen will parse "reg" using the #address-cells and 
>#size-cells of the parent. So it's normal to see 2GB. Does the same device 
>tree reports 3GB on Linux? I suspect no.

I'm not in the office today, so I can't test this, but I believe the answer is 
"yes". I have this log from an earlier run where I ran Linux without Xen. I 
*believe* used that same DT:
[0.00] Memory: 208924K/3145728K available (5557K kernel code, 280K 
rwdata, 1740K rodata, 3892K init, 213K bss, 36836K reserved, 2899968K 
cma-reserved, 0K highmem)

Chris


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

[Xen-devel] [PATCH] xen: arm: Support <32MB frametables

[Chris (Christopher) Brand]

setup_frametable_mappings() rounds frametable_size up to a multiple
of 32MB. This is wasteful on systems with less than 4GB of RAM,
although it does allow the "contig" bit to be set in the PTEs.

Where the frametable is less than 32MB in size, instead round up
to a multiple of 2MB, not setting the "contig" bit in the PTEs.

Signed-off-by: Chris Brand 
---
 xen/arch/arm/mm.c | 39 ---
 1 file changed, 36 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index a91ea774f1f9..47b6d5d44563 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -656,6 +656,29 @@ static void __init create_32mb_mappings(lpae_t *second,
 }
 
 #ifdef CONFIG_ARM_32
+static void __init create_2mb_mappings(lpae_t *second,
+   unsigned long virt_offset,
+   unsigned long base_mfn,
+   unsigned long nr_mfns)
+{
+unsigned long i, count;
+lpae_t pte, *p;
+
+ASSERT(!((virt_offset >> PAGE_SHIFT) % LPAE_ENTRIES));
+ASSERT(!(base_mfn % LPAE_ENTRIES));
+ASSERT(!(nr_mfns % LPAE_ENTRIES));
+
+count = nr_mfns / LPAE_ENTRIES;
+p = second + second_linear_offset(virt_offset);
+pte = mfn_to_xen_entry(base_mfn, WRITEALLOC);
+for ( i = 0; i < count; i++ )
+{
+write_pte(p + i, pte);
+pte.pt.base += 1 << LPAE_SHIFT;
+}
+flush_xen_data_tlb_local();
+}
+
 /* Set up the xenheap: up to 1GB of contiguous, always-mapped memory. */
 void __init setup_xenheap_mappings(unsigned long base_mfn,
unsigned long nr_mfns)
@@ -749,6 +772,7 @@ void __init setup_frametable_mappings(paddr_t ps, paddr_t 
pe)
 unsigned long nr_pdxs = pfn_to_pdx(nr_pages);
 unsigned long frametable_size = nr_pdxs * sizeof(struct page_info);
 unsigned long base_mfn;
+unsigned long mask;
 #ifdef CONFIG_ARM_64
 lpae_t *second, pte;
 unsigned long nr_second, second_base;
@@ -757,8 +781,12 @@ void __init setup_frametable_mappings(paddr_t ps, paddr_t 
pe)
 
 frametable_base_pdx = pfn_to_pdx(ps >> PAGE_SHIFT);
 
-/* Round up to 32M boundary */
-frametable_size = (frametable_size + 0x1ff) & ~0x1ff;
+/* Round up to 2M or 32M boundary, as appropriate. */
+if ( frametable_size < MB(32) )
+mask = MB(2) - 1;
+else
+mask = MB(32) - 1;
+frametable_size = (frametable_size + mask) & ~mask;
 base_mfn = alloc_boot_pages(frametable_size >> PAGE_SHIFT, 32<<(20-12));
 
 #ifdef CONFIG_ARM_64
@@ -773,7 +801,12 @@ void __init setup_frametable_mappings(paddr_t ps, paddr_t 
pe)
 }
 create_32mb_mappings(second, 0, base_mfn, frametable_size >> PAGE_SHIFT);
 #else
-create_32mb_mappings(xen_second, FRAMETABLE_VIRT_START, base_mfn, 
frametable_size >> PAGE_SHIFT);
+if ( frametable_size < MB(32) )
+create_2mb_mappings(xen_second, FRAMETABLE_VIRT_START,
+base_mfn, frametable_size >> PAGE_SHIFT);
+else
+create_32mb_mappings(xen_second, FRAMETABLE_VIRT_START,
+ base_mfn, frametable_size >> PAGE_SHIFT);
 #endif
 
 memset(&frame_table[0], 0, nr_pdxs * sizeof(struct page_info));
-- 
1.9.1


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

Re: [Xen-devel] [PATCH] xen: arm: Support <32MB frametables

[Chris (Christopher) Brand]

Hi Julien,

Thanks for the review.

> OOI, you win 30MB of RAM but how does this affect the performance?

Fair question. :-) All I can say is that I don't see any noticeable difference 
on my
system. Are there performance tests that you suggest I run ? Also, note that
the new code is only executed if you specify a previously-invalid value for
xenheap_megabytes on the command-line, so it won't affect any existing
systems. Is it worth adding a sentence mentioning performance to the
documentation, do you think ?

> Can you rework create_32mb_mappings to take the size of the mappings in 
> parameters?

Yeah, I have a version like that here somewhere, but it wasn't as clean as I'd 
hoped.
I'll re-visit it and send as v2.

>> -/* Round up to 32M boundary */
>> -frametable_size = (frametable_size + 0x1ff) & ~0x1ff;
>> +/* Round up to 2M or 32M boundary, as appropriate. */
>> +if ( frametable_size < MB(32) )
>> +mask = MB(2) - 1;
>> +else
>> +mask = MB(32) - 1;
>> +frametable_size = (frametable_size + mask) & ~mask;
>
>You can use ROUNDUP(frametable_size, size) to avoid open-coding the mask.

Will do.

> Also, this code is common with ARM64. If we happen to have a board with a
> frametable smaller than 32MB, you will round up to 2MB and crash later in
> create_32mb_mappings because you don't support 2MB mapping for ARM64.
> 
> It might be good to support 2MB for ARM64 too.

Whoops! Yes, I've messed the ARM64 path up at some point while cleaning
up the code. Will fix.

Thanks,

Chris


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

Re: [Xen-devel] [PATCH] xen: arm: Support <32MB frametables

[Chris (Christopher) Brand]

>> OOI, you win 30MB of RAM but how does this affect the performance?
>
> Fair question. :-) All I can say is that I don't see any noticeable 
> difference on my system.
> Are there performance tests that you suggest I run ?
> Also, note that the new code is
> only executed if you specify a previously-invalid value for xenheap_megabytes 
> on the
> command-line, so it won't affect any existing systems.

This sentence is a lie, sorry. Please ignore it. :-)

> Is it worth adding a sentence
> mentioning performance to the documentation, do you think ?

Chris



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

[Xen-devel] [PATCH v2] xen: arm: Support <32MB frametables

[Chris (Christopher) Brand]

setup_frametable_mappings() rounds frametable_size up to a multiple
of 32MB. This is wasteful on systems with less than 4GB of RAM,
although it does allow the "contig" bit to be set in the PTEs.

Where the frametable is less than 32MB in size, instead round up
to a multiple of 2MB, not setting the "contig" bit in the PTEs.

Signed-off-by: Chris Brand 
---
Changed in v2: merged create_32mb_mappings() and create_2mb_mappings()
into create_mappings(). A side-effect is to fix the bug in v1 for
ARM64 systems with <4GB RAM.

 xen/arch/arm/mm.c | 37 ++---
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index ae0f34c3c480..fd64fbfdfb93 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -628,25 +628,31 @@ void __cpuinit mmu_init_secondary_cpu(void)
 }
 
 /* Create Xen's mappings of memory.
- * Base and virt must be 32MB aligned and size a multiple of 32MB.
+ * Mapping_size must be either 2MB or 32MB.
+ * Base and virt must be mapping_size aligned.
+ * Size must be a multiple of mapping_size.
  * second must be a contiguous set of second level page tables
  * covering the region starting at virt_offset. */
-static void __init create_32mb_mappings(lpae_t *second,
-unsigned long virt_offset,
-unsigned long base_mfn,
-unsigned long nr_mfns)
+static void __init create_mappings(lpae_t *second,
+   unsigned long virt_offset,
+   unsigned long base_mfn,
+   unsigned long nr_mfns,
+   unsigned int mapping_size)
 {
 unsigned long i, count;
+const unsigned long granularity = mapping_size >> PAGE_SHIFT;
 lpae_t pte, *p;
 
-ASSERT(!((virt_offset >> PAGE_SHIFT) % (16 * LPAE_ENTRIES)));
-ASSERT(!(base_mfn % (16 * LPAE_ENTRIES)));
-ASSERT(!(nr_mfns % (16 * LPAE_ENTRIES)));
+ASSERT((mapping_size == MB(2)) || (mapping_size == MB(32)));
+ASSERT(!((virt_offset >> PAGE_SHIFT) % granularity));
+ASSERT(!(base_mfn % granularity));
+ASSERT(!(nr_mfns % granularity));
 
 count = nr_mfns / LPAE_ENTRIES;
 p = second + second_linear_offset(virt_offset);
 pte = mfn_to_xen_entry(base_mfn, WRITEALLOC);
-pte.pt.contig = 1;  /* These maps are in 16-entry contiguous chunks. */
+if ( mapping_size == MB(32) )
+pte.pt.contig = 1;  /* These maps are in 16-entry contiguous chunks. */
 for ( i = 0; i < count; i++ )
 {
 write_pte(p + i, pte);
@@ -660,7 +666,7 @@ static void __init create_32mb_mappings(lpae_t *second,
 void __init setup_xenheap_mappings(unsigned long base_mfn,
unsigned long nr_mfns)
 {
-create_32mb_mappings(xen_second, XENHEAP_VIRT_START, base_mfn, nr_mfns);
+create_mappings(xen_second, XENHEAP_VIRT_START, base_mfn, nr_mfns, MB(32));
 
 /* Record where the xenheap is, for translation routines. */
 xenheap_virt_end = XENHEAP_VIRT_START + nr_mfns * PAGE_SIZE;
@@ -749,6 +755,7 @@ void __init setup_frametable_mappings(paddr_t ps, paddr_t 
pe)
 unsigned long nr_pdxs = pfn_to_pdx(nr_pages);
 unsigned long frametable_size = nr_pdxs * sizeof(struct page_info);
 unsigned long base_mfn;
+const unsigned long mapping_size = frametable_size < MB(32) ? MB(2) : 
MB(32);
 #ifdef CONFIG_ARM_64
 lpae_t *second, pte;
 unsigned long nr_second, second_base;
@@ -756,9 +763,8 @@ void __init setup_frametable_mappings(paddr_t ps, paddr_t 
pe)
 #endif
 
 frametable_base_pdx = pfn_to_pdx(ps >> PAGE_SHIFT);
-
-/* Round up to 32M boundary */
-frametable_size = (frametable_size + 0x1ff) & ~0x1ff;
+/* Round up to 2M or 32M boundary, as appropriate. */
+frametable_size = ROUNDUP(frametable_size, mapping_size);
 base_mfn = alloc_boot_pages(frametable_size >> PAGE_SHIFT, 32<<(20-12));
 
 #ifdef CONFIG_ARM_64
@@ -771,9 +777,10 @@ void __init setup_frametable_mappings(paddr_t ps, paddr_t 
pe)
 pte.pt.table = 1;
 write_pte(&xen_first[first_table_offset(FRAMETABLE_VIRT_START)+i], 
pte);
 }
-create_32mb_mappings(second, 0, base_mfn, frametable_size >> PAGE_SHIFT);
+create_mappings(second, 0, base_mfn, frametable_size >> PAGE_SHIFT, 
mapping_size);
 #else
-create_32mb_mappings(xen_second, FRAMETABLE_VIRT_START, base_mfn, 
frametable_size >> PAGE_SHIFT);
+create_mappings(xen_second, FRAMETABLE_VIRT_START,
+base_mfn, frametable_size >> PAGE_SHIFT, mapping_size);
 #endif
 
 memset(&frame_table[0], 0, nr_pdxs * sizeof(struct page_info));
-- 
1.9.1


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

Re: [Xen-devel] [PATCH v2] xen: arm: Support <32MB frametables

[Chris (Christopher) Brand]

Any thoughts on v2 ?

Chris

> -Original Message-
> From: Chris (Christopher) Brand
> Sent: Friday, 07 August, 2015 1:41 PM
> To: 'Julien Grall'; xen-devel@lists.xen.org
> Cc: Stefano Stabellini; Ian Campbell (ian.campb...@citrix.com)
> Subject: [PATCH v2] xen: arm: Support <32MB frametables
> 
> setup_frametable_mappings() rounds frametable_size up to a multiple of
> 32MB. This is wasteful on systems with less than 4GB of RAM, although it
> does allow the "contig" bit to be set in the PTEs.
> 
> Where the frametable is less than 32MB in size, instead round up to a
> multiple of 2MB, not setting the "contig" bit in the PTEs.
> 
> Signed-off-by: Chris Brand 
> ---
> Changed in v2: merged create_32mb_mappings() and
> create_2mb_mappings() into create_mappings(). A side-effect is to fix the
> bug in v1 for
> ARM64 systems with <4GB RAM.
> 
>  xen/arch/arm/mm.c | 37 ++---
>  1 file changed, 22 insertions(+), 15 deletions(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c index
> ae0f34c3c480..fd64fbfdfb93 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -628,25 +628,31 @@ void __cpuinit mmu_init_secondary_cpu(void)  }
> 
>  /* Create Xen's mappings of memory.
> - * Base and virt must be 32MB aligned and size a multiple of 32MB.
> + * Mapping_size must be either 2MB or 32MB.
> + * Base and virt must be mapping_size aligned.
> + * Size must be a multiple of mapping_size.
>   * second must be a contiguous set of second level page tables
>   * covering the region starting at virt_offset. */ -static void __init
> create_32mb_mappings(lpae_t *second,
> -unsigned long virt_offset,
> -unsigned long base_mfn,
> -unsigned long nr_mfns)
> +static void __init create_mappings(lpae_t *second,
> +   unsigned long virt_offset,
> +   unsigned long base_mfn,
> +   unsigned long nr_mfns,
> +   unsigned int mapping_size)
>  {
>  unsigned long i, count;
> +const unsigned long granularity = mapping_size >> PAGE_SHIFT;
>  lpae_t pte, *p;
> 
> -ASSERT(!((virt_offset >> PAGE_SHIFT) % (16 * LPAE_ENTRIES)));
> -ASSERT(!(base_mfn % (16 * LPAE_ENTRIES)));
> -ASSERT(!(nr_mfns % (16 * LPAE_ENTRIES)));
> +ASSERT((mapping_size == MB(2)) || (mapping_size == MB(32)));
> +ASSERT(!((virt_offset >> PAGE_SHIFT) % granularity));
> +ASSERT(!(base_mfn % granularity));
> +ASSERT(!(nr_mfns % granularity));
> 
>  count = nr_mfns / LPAE_ENTRIES;
>  p = second + second_linear_offset(virt_offset);
>  pte = mfn_to_xen_entry(base_mfn, WRITEALLOC);
> -pte.pt.contig = 1;  /* These maps are in 16-entry contiguous chunks. */
> +if ( mapping_size == MB(32) )
> +pte.pt.contig = 1;  /* These maps are in 16-entry contiguous
> + chunks. */
>  for ( i = 0; i < count; i++ )
>  {
>  write_pte(p + i, pte);
> @@ -660,7 +666,7 @@ static void __init create_32mb_mappings(lpae_t
> *second,  void __init setup_xenheap_mappings(unsigned long base_mfn,
> unsigned long nr_mfns)  {
> -create_32mb_mappings(xen_second, XENHEAP_VIRT_START, base_mfn,
> nr_mfns);
> +create_mappings(xen_second, XENHEAP_VIRT_START, base_mfn,
> nr_mfns,
> + MB(32));
> 
>  /* Record where the xenheap is, for translation routines. */
>  xenheap_virt_end = XENHEAP_VIRT_START + nr_mfns * PAGE_SIZE; @@
> -749,6 +755,7 @@ void __init setup_frametable_mappings(paddr_t ps,
> paddr_t pe)
>  unsigned long nr_pdxs = pfn_to_pdx(nr_pages);
>  unsigned long frametable_size = nr_pdxs * sizeof(struct page_info);
>  unsigned long base_mfn;
> +const unsigned long mapping_size = frametable_size < MB(32) ? MB(2)
> + : MB(32);
>  #ifdef CONFIG_ARM_64
>  lpae_t *second, pte;
>  unsigned long nr_second, second_base; @@ -756,9 +763,8 @@ void __init
> setup_frametable_mappings(paddr_t ps, paddr_t pe)  #endif
> 
>  frametable_base_pdx = pfn_to_pdx(ps >> PAGE_SHIFT);
> -
> -/* Round up to 32M boundary */
> -frametable_size = (frametable_size + 0x1ff) & ~0x1ff;
> +/* Round up to 2M or 32M boundary, as appropriate. */
> +frametable_size = ROUNDUP(frametable_size, mapping_size);
>  base_mfn = alloc_boot_pages(frametable_size >> PAGE_SHIFT, 32<<(20-
> 12));
> 
>  #ifdef CONFIG_ARM_64
> @@ -771,9 +777,10 @@ void __init setup_frametable_mappings(p

Re: [Xen-devel] [PATCH v2] xen: arm: Support <32MB frametables

[Chris (Christopher) Brand]

Hi Julien,

Thanks for the review.

> >  /* Create Xen's mappings of memory.
> > - * Base and virt must be 32MB aligned and size a multiple of 32MB.
> > + * Mapping_size must be either 2MB or 32MB.
> 
> I would have generalize the function to support any mapping size. But I'm
> also fine with the current solution.

The code would have to be (somewhat) more complex to do that. I think
I prefer to keep it simple for now.

> > +const unsigned long granularity = mapping_size >> PAGE_SHIFT;
> 
> This variable is only used in the ASSERT. On non-debug build the compiler will
> throw an error because the variable will be unused.

Good point. Will fix.

> > +if ( mapping_size == MB(32) )
> > +pte.pt.contig = 1;  /* These maps are in 16-entry contiguous
> > + chunks. */
> 
> mfn_to_xen_entry never set the contig bit to 0 (or anything else). So the
> value will be undefined for MB(2) mapping.
> 
> It looks like to me that mfn_to_xen_entry should always set the contig bit to
> 0. I'm not sure why we didn't see any issue until now. Can you please send a
> patch for this?

Will do.

Chris


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

[Xen-devel] [PATCH 0/2] xen: arm: Ensure all PTE bits have a known value

[Chris (Christopher) Brand]

Hi,

This is more-or-less what Julien requested. He noted that pt.contig was never
set to zero. When I looked further, I found other bits that were also never
given a value. Looking at the callsites, they almost all seem to assume a value
of zero, so that's what I went with.

Patch 1 re-orders the assignments to match the declaration, making it clearer
which ones are missing. Patch 2 then adds the missing bits.

Chris
P.S. Apologies for any threading issues - I have not yet managed to get git
send-email working properly.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 0/2] xen: arm: Ensure all PTE bits have a known value

[Chris (Christopher) Brand]

Hi,

This is more-or-less what Julien requested. He noted that pt.contig was never
set to zero. When I looked further, I found other bits that were also never
given a value. Looking at the callsites, they almost all seem to assume a value
of zero, so that's what I went with.

Patch 1 re-orders the assignments to match the declaration, making it clearer
which ones are missing. Patch 2 then adds the missing bits.

Chris
P.S. Apologies for any threading issues - I have not yet managed to get git
send-email working properly.

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

[Xen-devel] [PATCH 1/2] xen: arm re-order assignments in mfn_to_xen_entry()

[Chris (Christopher) Brand]

Shuffle lines around so that the assignments in mfn_to_xen_entry()
occur in the same order as the bits are declared in lpae_pt_t.
This makes it easier to see which ones are never given a value.
No change in behaviour.

Also fix a minor comment typo.

Signed-off-by: Chris Brand 
---
 xen/include/asm-arm/page.h | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index 5ecfd0705e07..01628f3e96cb 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -197,18 +197,18 @@ static inline lpae_t mfn_to_xen_entry(unsigned long mfn, 
unsigned attr)
 paddr_t pa = ((paddr_t) mfn) << PAGE_SHIFT;
 lpae_t e = (lpae_t) {
 .pt = {
-.xn = 1,  /* No need to execute outside .text */
-.ng = 1,  /* Makes TLB flushes easier */
-.af = 1,  /* No need for access tracking */
+.valid = 1,   /* Mappings are present */
+.table = 0,   /* Set to 1 for links and 4k maps */
+.ai = attr,
 .ns = 1,  /* Hyp mode is in the non-secure world */
 .user = 1,/* See below */
-.ai = attr,
-.table = 0,   /* Set to 1 for links and 4k maps */
-.valid = 1,   /* Mappings are present */
+.af = 1,  /* No need for access tracking */
+.ng = 1,  /* Makes TLB flushes easier */
+.xn = 1,  /* No need to execute outside .text */
 }};;
 /* Setting the User bit is strange, but the ATS1H[RW] instructions
  * don't seem to work otherwise, and since we never run on Xen
- * pagetables un User mode it's OK.  If this changes, remember
+ * pagetables in User mode it's OK.  If this changes, remember
  * to update the hard-coded values in head.S too */
 
 switch ( attr )
-- 
1.9.1


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

[Xen-devel] [PATCH 2/2] xen: arm: Set all bits in mfn_to_xen_entry()

[Chris (Christopher) Brand]

Ensure that every bit has a specific value.

Reported-by: Julien Grall 
Signed-off-by: Chris Brand 
---
 xen/include/asm-arm/page.h | 5 +
 1 file changed, 5 insertions(+)

diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index 01628f3e96cb..7a56b2cb463a 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -202,9 +202,14 @@ static inline lpae_t mfn_to_xen_entry(unsigned long mfn, 
unsigned attr)
 .ai = attr,
 .ns = 1,  /* Hyp mode is in the non-secure world */
 .user = 1,/* See below */
+.ro = 0,  /* Assume read-write */
 .af = 1,  /* No need for access tracking */
 .ng = 1,  /* Makes TLB flushes easier */
+.sbz = 0,
+.contig = 0,  /* Assume non-contiguous */
+.pxn = 0,
 .xn = 1,  /* No need to execute outside .text */
+.avail = 0,
 }};;
 /* Setting the User bit is strange, but the ATS1H[RW] instructions
  * don't seem to work otherwise, and since we never run on Xen
-- 
1.9.1


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

Re: [Xen-devel] [PATCH 2/2] xen: arm: Set all bits in mfn_to_xen_entry()

[Chris (Christopher) Brand]

Hi Julien,

Thanks for the review.

> > +.pxn = 0,
> 
> I would add a comment to explain that this bit is reserved for PL2 stage
> 1 page table.

Will do.

> > +.avail = 0,
> 
> I don't think this one is necessary. avail is not used by the hardware neither
> Xen.

"grep -rF pt.avail xen" gives 7 matches in xen/arch/arm/mm.c, so it is used by
Xen (I'll fully admit that I didn't dig in to the "how and why" of its use).
 
> what about *t fields (pxnt, xnt, apt,...)?

I figured that as we're setting table to 0, these are ignored, and any code 
setting
table to 1 should then set them. I can obviously easily set them here (I guess 
all
to zero would make sense) if you think it's worthwhile ...

Chris


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

Re: [Xen-devel] [PATCH 0/2] xen: arm: Ensure all PTE bits have a known value

[Chris (Christopher) Brand]

> > P.S. Apologies for any threading issues - I have not yet managed to
> > get git send-email working properly.
> 
> I'm usually using git format-patch to get a list of files (one per
> patch) and my cover letter (--cover-letter).

Thanks.

> Then I use git send-email *.patch which will send the patch correctly
> threading.

Yeah, the difficulty is finding the right set of --smtp* options to get it
through our corporate mail server. I had it working once upon a time
when I was working for Linaro, but something somewhere seems to have
changed :-)

Chris


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

Re: [Xen-devel] [PATCH 2/2 v2] xen: arm: Set all bits in mfn_to_xen_entry()

[Chris (Christopher) Brand]

Hi Andrew,

> On 21/08/15 00:33, Chris Brand wrote:
> > Ensure that every bit has a specific value.
> >
> > Reported-by: Julien Grall 
> > Signed-off-by: Chris Brand 
> > ---
> > v2 adds comments on pxn and avail.
> 
> This is no functional change, if the compiler is conforming to the C spec.
> 
> The spec guarantees that structure initialisation like this causes unspecified
> names to gain their default value.  As these are integer bitfields, the 
> default
> value is 0.
> 
> What compiler is in use?  It would appear that it is buggy, or at least has
> buggy scalar replacement optimisations.

That's right. I'd forgotten about that. This was actually suggested by Julien in
a review of another patch I sent. I haven't seen any problems this fixes.

Chris


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

Re: [Xen-devel] Bug in devicetree_for_each_node() in xen/arch/arm/bootfdt.c ?

[Chris (Christopher) Brand]

>>As said by David, Xen will parse "reg" using the #address-cells and 
>>#size-cells of the parent. So it's normal to see 2GB. Does the same device 
>>tree reports 3GB on Linux? I suspect no.
>
>I'm not in the office today, so I can't test this, but I believe the answer is 
>"yes".

But in fact the answer is indeed "no". The log that I found was from a 
different device tree.

I still think the code shouldn't be reading before the start of the two arrays, 
but it does function correctly.

Chris


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

[Xen-devel] [PATCH] xen: arm: Avoid reading beyond the last module

[Chris (Christopher) Brand]

nr_mods is set in add_boot_module() to the number of module
array elements used. This function also ensures that nr_mods
never exceeds MAX_MODULES (the size of the array). When looping
through the array, the correct maximum index is "nr_mods-1",
not "nr_mods". If the array is full, using the latter will in
fact access beyond the end of the array.
This was done correctly in boot_module_find_by_kind() and
consider_modules() but incorrectly in discard_initial_modules()
and next_module().

Signed-off-by: Chris Brand 
---
xen/arch/arm/setup.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 06f8e54b1f04..5daa6db919ac 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -250,7 +250,7 @@ void __init discard_initial_modules(void)
 struct bootmodules *mi = &bootinfo.modules;
 int i;
-for ( i = 0; i <= mi->nr_mods; i++ )
+for ( i = 0; i < mi->nr_mods; i++ )
 {
 paddr_t s = mi->module[i].start;
 paddr_t e = s + PAGE_ALIGN(mi->module[i].size);
@@ -350,7 +350,7 @@ static paddr_t __init next_module(paddr_t s, paddr_t *end)
 paddr_t lowest = ~(paddr_t)0;
 int i;
-for ( i = 0; i <= mi->nr_mods; i++ )
+for ( i = 0; i < mi->nr_mods; i++ )
 {
 paddr_t mod_s = mi->module[i].start;
 paddr_t mod_e = mod_s + mi->module[i].size;
--
1.9.1



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

[Xen-devel] [PATCH] xen: arm: Avoid reading beyond the last module

[Chris (Christopher) Brand]

nr_mods is set in add_boot_module() to the number of module
array elements used. This function also ensures that nr_mods
never exceeds MAX_MODULES (the size of the array). When looping
through the array, the correct maximum index is "nr_mods-1",
not "nr_mods". If the array is full, using the latter will in
fact access beyond the end of the array.
This was done correctly in boot_module_find_by_kind() and
consider_modules() but incorrectly in discard_initial_modules()
and next_module().

Signed-off-by: Chris Brand 
---
 xen/arch/arm/setup.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 06f8e54b1f04..5daa6db919ac 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -250,7 +250,7 @@ void __init discard_initial_modules(void)
 struct bootmodules *mi = &bootinfo.modules;
 int i;
 
-for ( i = 0; i <= mi->nr_mods; i++ )
+for ( i = 0; i < mi->nr_mods; i++ )
 {
 paddr_t s = mi->module[i].start;
 paddr_t e = s + PAGE_ALIGN(mi->module[i].size);
@@ -350,7 +350,7 @@ static paddr_t __init next_module(paddr_t s, paddr_t *end)
 paddr_t lowest = ~(paddr_t)0;
 int i;
 
-for ( i = 0; i <= mi->nr_mods; i++ )
+for ( i = 0; i < mi->nr_mods; i++ )
 {
 paddr_t mod_s = mi->module[i].start;
 paddr_t mod_e = mod_s + mi->module[i].size;
-- 
1.9.1


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

[Xen-devel] ARM - why does setup_frametable_size() round frametable_size to 32MB ?

[Chris (Christopher) Brand]

Hi,

I'm working on a platform with a mere 2GB of RAM, and trying to
trim the Xen footprint down as much as possible. I've found two
places where Xen uses more memory than it seems it needs to,
one of which is the frametable. On a 2GB system, frametable_size
is initially calculated as 16MB, but is then rounded up to 32MB.
can somebody tell me why this is done, and therefore whether
it can be avoided ? I assume it's because the code then calls
create_32mb_mappings(), in which case I guess my question
is what's special about 32MB ?

unsigned long frametable_size = nr_pdxs * sizeof(struct page_info);
[...]
/* Round up to 32M boundary */
frametable_size = (frametable_size + 0x1ff) & ~0x1ff;

Thanks,

Chris


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

Re: [Xen-devel] [PATCH] xen: arm: Avoid reading beyond the last module

[Chris (Christopher) Brand]

> It looks like the mail as been sent in HTML. Can you resend it in plain text?

Re-sent, hopefully correctly this time.

Chris


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

Re: [Xen-devel] ARM - why does setup_frametable_size() round frametable_size to 32MB ?

[Chris (Christopher) Brand]

Thanks, Ian. I tried that, and it does seem to work (everything boots, I can 
still bring up VMs, and I see an extra 16MB of free memory). The patch I came 
up with follows (it would be nice to share code between create_32mb_mappings() 
and create_2mb_mappings(), but the setting of the contig bit is right in the 
middle, and the functions are pretty short).

Chris

From: Chris Brand 
Date: Mon, 20 Jul 2015 13:38:15 -0700
Subject: [PATCH] xen: arm: Support <32MB frametables

setup_frametable_mappings() rounds frametable_size up to a multiple
of 32MB. This is wasteful on systemes with less than 4GB of RAM,
although it does allow the "contig" bit to be set in the PTEs.

Where the frametable is less than 32MB in size, instead round up
to a multiple of 2MB, not setting the "contig" bit in the PTEs.

Signed-off-by: Chris Brand 
---
 xen/arch/arm/mm.c | 39 ---
 1 file changed, 36 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index a91ea774f1f9..a7f4864f8d8f 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -656,6 +656,29 @@ static void __init create_32mb_mappings(lpae_t *second,
 }
 
 #ifdef CONFIG_ARM_32
+static void __init create_2mb_mappings(lpae_t *second,
+   unsigned long virt_offset,
+   unsigned long base_mfn,
+   unsigned long nr_mfns)
+{
+unsigned long i, count;
+lpae_t pte, *p;
+
+ASSERT(!((virt_offset >> PAGE_SHIFT) % LPAE_ENTRIES));
+ASSERT(!(base_mfn % LPAE_ENTRIES));
+ASSERT(!(nr_mfns % LPAE_ENTRIES));
+
+count = nr_mfns / LPAE_ENTRIES;
+p = second + second_linear_offset(virt_offset);
+pte = mfn_to_xen_entry(base_mfn, WRITEALLOC);
+for ( i = 0; i < count; i++ )
+{
+write_pte(p + i, pte);
+pte.pt.base += 1 << LPAE_SHIFT;
+}
+flush_xen_data_tlb_local();
+}
+
 /* Set up the xenheap: up to 1GB of contiguous, always-mapped memory. */
 void __init setup_xenheap_mappings(unsigned long base_mfn,
unsigned long nr_mfns)
@@ -749,6 +772,7 @@ void __init setup_frametable_mappings(paddr_t ps, paddr_t 
pe)
 unsigned long nr_pdxs = pfn_to_pdx(nr_pages);
 unsigned long frametable_size = nr_pdxs * sizeof(struct page_info);
 unsigned long base_mfn;
+unsigned long mask;
 #ifdef CONFIG_ARM_64
 lpae_t *second, pte;
 unsigned long nr_second, second_base;
@@ -757,8 +781,12 @@ void __init setup_frametable_mappings(paddr_t ps, paddr_t 
pe)
 
 frametable_base_pdx = pfn_to_pdx(ps >> PAGE_SHIFT);
 
-/* Round up to 32M boundary */
-frametable_size = (frametable_size + 0x1ff) & ~0x1ff;
+/* Round up to 2M or 32M boundary, as appropriate */
+if (frametable_size < MB(32))
+mask = MB(2) - 1;
+else
+mask = MB(32) - 1;
+frametable_size = (frametable_size + mask) & ~mask;
 base_mfn = alloc_boot_pages(frametable_size >> PAGE_SHIFT, 32<<(20-12));
 
 #ifdef CONFIG_ARM_64
@@ -773,7 +801,12 @@ void __init setup_frametable_mappings(paddr_t ps, paddr_t 
pe)
 }
 create_32mb_mappings(second, 0, base_mfn, frametable_size >> PAGE_SHIFT);
 #else
-create_32mb_mappings(xen_second, FRAMETABLE_VIRT_START, base_mfn, 
frametable_size >> PAGE_SHIFT);
+if (frametable_size < MB(32))
+create_2mb_mappings(xen_second, FRAMETABLE_VIRT_START,
+base_mfn, frametable_size >> PAGE_SHIFT);
+else
+create_32mb_mappings(xen_second, FRAMETABLE_VIRT_START,
+ base_mfn, frametable_size >> PAGE_SHIFT);
 #endif
 
 memset(&frame_table[0], 0, nr_pdxs * sizeof(struct page_info));
-- 
1.9.1



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

[Xen-devel] [PATCH] xen: arm: Document xenheap_megabytes limitation

[Chris (Christopher) Brand]

In setup_mm(), the value passed as xenheap_megabytes gets
converted to pages and passed to setup_xenheap_mappings(),
which in turn passes it to create_32mb_mappings(), which
contains an ASSERT that the value passed is a multiple of
32MB. So specifying any value that is not an integer multiple
of 32 will cause Xen to hit this assert and fail to boot.

Signed-off-by: Chris Brand 
---
 docs/misc/xen-command-line.markdown | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/misc/xen-command-line.markdown 
b/docs/misc/xen-command-line.markdown
index 4889e27626d4..f3d3bd6ce56a 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -242,7 +242,7 @@ the NMI watchdog is also enabled.
 
 > Default: `0` (1/32 of RAM)
 
-Amount of RAM to set aside for the Xenheap.
+Amount of RAM to set aside for the Xenheap. Must be an integer multiple of 32.
 
 By default will use 1/32 of the RAM up to a maximum of 1GB and with a
 minimum of 32M, subject to a suitably aligned and sized contiguous
-- 
1.9.1


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

Re: [Xen-devel] [PATCH 4/8] xen: Use the correctly the Xen memory terminologies

[Chris (Christopher) Brand]

> Based on include/xen/mm.h [1], Linux is mistakenly using MFN when GFN is 
> meant,
> I suspect this is because the first support for Xen was for PV. This brough 
> some
Typo : "brought"
Perhaps "resulted in" would be better ?

> misimplementation of helpers on ARM and make the developper confused the 
> expected behavior.
Typo: "developer".
I'd also suggest "...and confused developers about the...".

[...]

> For clarity and avoid new confusion, replace any reference of mfn into gnf in 
> any helpers used by PV drivers.
Typo : "gfn"
I'd suggest "...replace any reference to mfn with gfn..."

[...]

Chris

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