Re: [Xen-devel] [PULL 0/19] xen-2015-09-08-tag

[Peter Maydell]

On 10 September 2015 at 02:21, Chen, Tiejun  wrote:
> On 9/10/2015 12:10 AM, Stefano Stabellini wrote:
>> I found another issue introduced by the gfx passthrough series on
>> Windows:
>>
>> ../hw/pci-host/piix.o: In function `host_pci_config_read':
>> /root/qemu/hw/pci-host/piix.c:778: undefined reference to `_pread'
>>
>> It is introduced by:
>>
>> commit fdb70721ba0496a767137e5505dd27627d19c4a8
>> Author: Tiejun Chen 
>> Date:   Wed Jul 15 13:37:43 2015 +0800
>>
>>  piix: create host bridge to passthrough
>>
>>
>> You might have to replace the pread call with lseek and read.
>>
>
> This is also surprising to me. Just see xen_host_pci_config_() inside
> /hw/xen/xen-host-pci-device.c, there are so many this pread usage(). So I
> really don't understand what's difference between these two files.

xen-host-pci-device.c is only compiled if CONFIG_XEN_PCI_PASSTHROUGH
was set by configure. That won't be the case on OSX or Windows, where
the Xen headers don't exist.

thanks
-- PMM

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

Re: [Xen-devel] linux-3.4 broken on chardonnay and huxelrebe (Re: [linux-3.4 test] 61301: regressions - FAIL)

[Ian Campbell]

On Wed, 2015-09-09 at 10:44 +0100, Ian Campbell wrote:
> On Fri, 2015-09-04 at 08:58 +, osstest service owner wrote:
> > flight 61301 linux-3.4 real [real]
> > http://logs.test-lab.xenproject.org/osstest/logs/61301/
> > 
> > Regressions :-(
> > 
> > Tests which did not succeed and are blocking,
> > including tests which could not be run:
> >  test-amd64-amd64-xl-qemut-win7-amd64  6 xen-boot  fail REGR.
> > vs. 30511
> 
> linux-3.4 seems to have trouble booting on chardonnay and huxelrebe,

I ran some adhoc tests on some jobs selected because I could see they
passed on these hosts (or their partners) on later Linux versions:

Tag RevisionFlight  huxelrebe0  
chardonnay1

^test-amd64-amd64-libvirt

^test-amd64-amd64-xl

v3.476e10d158efb6d4516018846f60c2ab5501900bc61710   FAIL
PASS
v3.528a33cbc24e4256c143dce96c7d93bf423229f9261713   PASS
FAIL
v3.6a0d271cbfed1dd50278c6b06bead3d00ba0a88f961714   PASS
PASS
v3.729594404d7fe73cd80eaa4ee8c43dcc53970c60e61715   PASS
PASS
v3.819f949f52599ba7c3f67a5897ac6be14bfcb120061716   PASS
FAIL
v3.9c1be5a5b1b355d40e6cf79cc979eb66dafa24ad161717   PASS
PASS
v3.10   8bb495e3f02401ee6f76d1b1d77f3ac9f079e37661703   PASS
PASS

(unfortunately due to a configuration issue I have no logs from these, just
the overall result :-(. Luckily I think for now they aren't necessary for
the analysis)

The Chardonnay case suggests that either something has been backported into
3.4.x which has broken things (current real flights, which reliably fail,
are running on 3.4.108) or that it is simply unreliable (or both). I think
I need to repeat things a few times to confirm.

The Huxelrebe case is apparently more obvious, I noticed that the Ethernet
device from [0] us a "Intel Corporation I210 Gigabit Network Connection"
which the Debian kernel detects (in [1]) as:
Aug 18 01:38:56.085002 [2.386215] igb :03:00.0: Intel(R) Gigabit 
Ethernet Network Connection
Aug 18 01:38:56.133067 [2.393591] igb :03:00.0: eth1: 
(PCIe:2.5Gb/s:Width x1) 44:39:c4:6d:20:60
Aug 18 01:38:56.141009 [2.401248] igb :03:00.0: eth1: PBA No: 000300-000

But which 3.4 doesn't pick up at all in [2]. In the logs between 3.4 and
3.5 I see:

commit f96a8a0b78548c0ec06b0b4b438db6ee895d67e9
Author: Carolyn Wyborny 
Date:   Fri Apr 6 23:25:19 2012 +

igb: Add Support for new i210/i211 devices.

This patch adds new initialization functions and device support
for i210 and i211 devices.

Signed-off-by: Carolyn Wyborny 
Tested-by: Jeff Pieper 
Signed-off-by: Jeff Kirsher 

Which seems pretty straight forwards.

These systems actually have a pair of these devices (the other is
 :02:00.0/eth0) and another which is detected by Debian as:

Aug 18 01:38:56.149025 [2.403785] e1000e :00:19.0: eth2: (PCI 
Express:2.5GT/s:Width x1) 44:39:c4:39:a1:19
Aug 18 01:38:56.157078 [2.403787] e1000e :00:19.0: eth2: Intel(R) 
PRO/1000 Network Connection
Aug 18 01:38:56.165003 [2.403862] e1000e :00:19.0: eth2: MAC: 11, PHY: 
12, PBA No: FF-0FF

But which linux-3.4 also doesn't see.

I've not confirmed whether f96a8a0b78 is the only required patch and it is
not entirely trivial so I don't know if it would be considered a valid
backport. We could perhaps consider arranging that these machines are only
used for newer kernels somehow?

Ian.

[0] 
http://logs.test-lab.xenproject.org/osstest/logs/60711/test-amd64-amd64-xl/huxelrebe1-output-lspci_-tv
[1] 
http://logs.test-lab.xenproject.org/osstest/logs/60711/test-amd64-amd64-xl/serial-huxelrebe1.log
[2] 
http://logs.test-lab.xenproject.org/osstest/logs/61301/test-amd64-amd64-xl/serial-huxelrebe0.log

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

Re: [Xen-devel] Asus X99-A VT-d problems

[Jan Beulich]

>>> On 09.09.15 at 19:02,  wrote:
> Well, now the system is booting but would you have any idea why the system
> only boots with iommu=verbose,debug but not with iommu=1? If I pass iommu=1
> as a boot parameter it fails detecting SATA drives (AHCI) with "ata1.00:
> failed command: IDENTIFY DEVICE" error. Also USB fails with fatal error,
> "ehci_hcd :xx:xxx:xx: HC died".

Short of logs thereof - no explanation other than the extra printing
possibly changing timing somewhere (albeit one would think that
while the printing happens timing shouldn't be that much of an issue).

> Also if I try to pass dom0pvh=1 it also crashes with iommu=verbose,debug -
> screen just goes completely black and system restarts itself - no way to
> see any error messages at all.

Likely a different problem, and definitely one where again we'd need
to see full logs of. You may want to try "noreboot" as a command line
option to see whether that makes the machine hang instead of reboot
(which would be an indication that proper logging will give us useful
insight).

Jan


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

Re: [Xen-devel] [PATCH v4 10/11] x86/intel_pstate: support the use of intel_pstate in pmstat.c

[Jan Beulich]

>>> On 10.09.15 at 07:35,  wrote:
> On 09/09/2015 23:55,  Jan Beulich wrote:
 On 09.09.15 at 17:16,  wrote:
>> On 09/09/2015 21:12,  Jan Beulich wrote:
> On 09.09.15 at 14:56,  wrote:
>>> Can you please explain more why it doesn't scale? 
>>> From my point of view, any other future value representation can be 
>>> passed from the producer to the related consumer through this method.
>> 
>>> Did you read all of my earlier replies? I already said there "Just 
>>> consider
>> what happens to the code when we end up gaining a few
>>> more drivers providing percentage values, and perhaps another one 
>>> providing
>> a third variant of output representation."
>> 
>> Yes, I have read that. I am not sure if I got your point, but my 
>> meaning was when we add new drivers to the code, e.g. xx_pstate 
>> driver, we can still have the name, "xx_pstate", assigned to 
>> "p_cpufreq->scaling_driver" to distinguish one another. If the driver 
>> uses a different variant of output representation, which cannot be 
>> held by " uint32_t scaling_max_perf" (it needs "uint64_t" for example, then 
> that driver developer needs to add a new field here like  "
>> uint64_t scaling_max_perf_xx").
>> What is the scaling problem? 
> 
>>  if (strcmp() == 0 ||
>>  strcmp() == 0 ||
>>  strcmp() == 0) {
>>  ...
>>  } else if (strcmp() == 0) {
>>  ...
>>  } else {
>>  ...
>>  }
> 
>> is just ugly, and gets all the uglier the more strcmp()s get added.
>> Have a boolean or enumeration indicating what kind of data there is, and the 
> above changes to
> 
>>  switch (kind) {
>>  case absolute: ...
>>  case percentage: ...
>>  }
> 
> Ok. I will replace the default "scaling_driver[CPUFREQ_NAME_LEN]" with an 
> enum type, like this following
> ...
> - char scaling_driver[CPUFREQ_NAME_LEN];
> + enum scaling_driver_flag scaling_driver;
> ...
> 
> We cannot keep both of the above two there, because there is a 128Byte size 
> limit. Then somewhere, we need to translate the character-represented 
> scaling_driver to our new enum-represented scaling_driver. For example, in 
> pmstat.c, the following:
> 
> if ( cpufreq_driver->name[0] )
> strlcpy(op->u.get_para.scaling_driver,
> cpufreq_driver->name, CPUFREQ_NAME_LEN);
> else
> strlcpy(op->u.get_para.scaling_driver, "Unknown", CPUFREQ_NAME_LEN);
> 
> needs to be changed to:
> if ( strncmp(cpufreq_driver->name[0], "intel_pstate", CPUFREQ_NAME_LEN) == 0 )
> op->u.get_para.scaling_driver = INTEL_PSTATE;
> else if ( strncmp(cpufreq_driver->name[0], "acpi_cpufreq", CPUFREQ_NAME_LEN) 
> == 0 )
> op->u.get_para.scaling_driver = ACPI_CPUFREQ;
> ...
> 
> Seems we still cannot get rid of these strncmp()s. Is this acceptable, or 
> should we change "struct cpufreq_driver" to use enum represented driver name 
> as well, or do you have a better suggestion?

The one I explained before: Express the data representation type
in an enum, not the driver kind. But even if we went with the above,
the strcmp() ugliness would at least be limited to the producer, not
enforced onto any number of consumers.

Jan


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

Re: [Xen-devel] [PATCH] xen/domctl: lower loglevel of XEN_DOMCTL_memory_mapping

[Chen, Tiejun]

Sort of (the patch has the intended effect, but for its size very
many rough edges).



I guess we need to amend the original parameter, once_mapping_mfns, like 
this,


/* xen_once_mapping_mfns: memory mapping mfn bumbers once. */
unsigned int xen_once_mapping_mfns;
size_param("once_mapping_mfns", xen_once_mapping_mfns);

static void xen_once_mapping_mfns_setup(void)
{
if ( once_mapping_mfns < 64 )
xen_once_mapping_mfns = 64;
else if ( once_mapping_mfns > 1024 )
xen_once_mapping_mfns = 1024;
else
xen_once_mapping_mfns = once_mapping_mfns;
}

Or what is your expected rule?

Thanks
Tiejun

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

Re: [Xen-devel] [PATCH net v2 0/2] xen-net{front, back}: respect user provided max_queues

[Wei Liu]

On Wed, Sep 09, 2015 at 10:05:53PM -0700, David Miller wrote:
> From: David Miller 
> Date: Wed, 09 Sep 2015 21:53:22 -0700 (PDT)
> 
> > From: Wei Liu 
> > Date: Wed, 9 Sep 2015 11:23:04 +0100
> > 
> >> Wei Liu (2):
> >>   xen-netback: respect user provided max_queues
> >>   xen-netfront: respect user provided max_queues
> > 
> > Both applied, thanks.
> 
> Yo, this doesn't even compile!
> 
> drivers/net/xen-netfront.c: In function ‘netif_init’:
> drivers/net/xen-netfront.c:2138:6: error: ‘xenvif_max_queues’ undeclared 
> (first use in this function)
>   if (xenvif_max_queues == 0)
>   ^
> drivers/net/xen-netfront.c:2138:6: note: each undeclared identifier is 
> reported only once for each function it appears in
> 

Sorry about this.

I will send an updated version.

Wei.

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

Re: [Xen-devel] [Qemu-devel] [edk2] EDK II & GPL - Re: OVMF BoF @ KVM Forum 2015

[Daniel P. Berrange]

On Wed, Sep 09, 2015 at 05:41:59PM -0700, Jordan Justen wrote:
> On 2015-09-09 16:05:20, Andrew Fish wrote:
> > 
> > > On Sep 9, 2015, at 3:24 PM, Jordan Justen  
> > > wrot> > > FWIW, I don't mind if the consensus is that GplDriverPkg must 
> > > live in
> > > a separate repo. But, it would be nice to hear a good reason why it
> > > must live elsewhere.
> > 
> > Because GPL is not a permissive license. An accidental git grep and
> > copying some code can change the license of the code that gets the
> > GPL code pasted into it.
> 
> I like this argument. It is slightly tempered by the fact that git
> grep always shows the source path, and thus 'GplDriverPkg' would be
> obviously visible.

Plenty of projects have a scenario in which different parts of their
codebase are under different licenses, without there being undue
problems. If you make it clear by having a separate directory, then
I think you can ultimately credit the developers with having enough
intelligence to do the right thing here. If not, then I'd probably
question whether you can trust them to submit any code at all, as
they could equally have blindly copied it from a 3rd party project
under an incompatible license.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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

Re: [Xen-devel] [PATCH v6 16/18] vmx: Add some scheduler hooks for VT-d posted interrupts

[Jan Beulich]

>>> On 10.09.15 at 10:59,  wrote:
>> From: Jan Beulich [mailto:jbeul...@suse.com]
>> Sent: Thursday, September 10, 2015 4:28 PM
>> >>> On 10.09.15 at 04:07,  wrote:
>> >> From: Jan Beulich [mailto:jbeul...@suse.com]
>> >> Sent: Wednesday, September 09, 2015 6:27 PM
>> >> >>> On 09.09.15 at 10:56,  wrote:
>> >> >> From: Jan Beulich [mailto:jbeul...@suse.com]
>> >> >> Sent: Monday, September 07, 2015 8:55 PM
>> >> >> >>> On 25.08.15 at 03:57,  wrote:
>> >> >> > +
>> >> >> > +/*
>> >> >> > + * Delete the vCPU from the related block list
>> >> >> > + * if we are resuming from blocked state.
>> >> >> > + */
>> >> >> > +if ( v->arch.hvm_vmx.pi_block_cpu != -1 )
>> >> >> > +{
>> >> >> > +spin_lock_irqsave(_cpu(pi_blocked_vcpu_lock,
>> >> >> > +  v->arch.hvm_vmx.pi_block_cpu),
>> >> >> flags);
>> >> >> > +list_del_init(>arch.hvm_vmx.pi_blocked_vcpu_list);
>> >> >>
>> >> >> Shouldn't you set .pi_block_cpu back to -1 along with removing
>> >> >> the vCPU from the list? Which then ought to allow using just
>> >> >> list_del() here?
>> >> >
>> >> > Here is the story about using list_del_init() instead of list_del(), 
>> >> > (the
>> >> > same reason for using list_del_init() in pi_wakeup_interrupt() ).
>> >> > If we use list_del(), that means we need to set .pi_block_cpu to
>> >> > -1 after removing the vCPU from the block list, there are two
>> >> > places where the vCPU is removed from the list:
>> >> > - arch_vcpu_wake_prepare()
>> >> > - pi_wakeup_interrupt()
>> >> >
>> >> > That means we also need to set .pi_block_cpu to -1 in
>> >> > pi_wakeup_interrupt(), which seems problematic to me, since
>> >> > .pi_block_cpu is used to get the per-CPU spin lock, it is not safe
>> >> > to change it in pi_wakeup_interrupt(), maybe someone is using
>> >> > it at the same time.
>> >> >
>> >> > And the check "if ( v->arch.hvm_vmx.pi_block_cpu != -1 )" here
>> >> > is only used when .pi_block_cpu is set to -1 at the initial time.
>> >> >
>> >> > If you have any good suggestions about this, I will be all ears.
>> >>
>> >> Latching pi_block_cpu into a local variable would take care of that
>> >> part of the problem. Of course you then may also need to check
>> >> that while waiting to acquire the lock pi_block_cpu didn't change.
>> >
>> > Thanks for the suggestion! But I think it is not easy to "check
>> > "that while waiting to acquire the lock pi_block_cpu didn't change".
>> > Let's take the following pseudo code as an example:
>> >
>> > int local_pi_block_cpu = v->arch.hvm_vmx.pi_block_cpu;
>> >
>> > ..
>> >
>> > spin_lock_irqsave(_cpu(pi_blocked_vcpu_lock,
>> >local_pi_block_cpu), flags);
>> > list_del(>arch.hvm_vmx.pi_blocked_vcpu_list);
>> > spin_unlock_irqrestore(_cpu(pi_blocked_vcpu_lock,
>> >local_pi_block_cpu), flags);
>> >
>> > If .pi_block_cpu is changed to -1 by others during calling list_del()
>> > above, that means the vCPU is removed by others, then calling
>> > list_del() here again would be problematic. I think there might be
>> > other corner cases for this. So I suggest adding some comments
>> > for list_del_init() as you mentioned below.
>> 
>> That's why I said "check that while waiting to acquire the lock
>> pi_block_cpu didn't change." The code you present does not do
>> this.
> 
> I didn't see we need implement it as the above code, I just list it
> here the show it is hard to do that. 
> First, how to check it while waiting to acquire the lock .pi_block_cpu
> didn't change?

Note the difference between "check while waiting" and "check that
while waiting": The former is indeed hard to implement, while the
latter is pretty straightforward (and we do so elsewhere).

> Secondly, even if we can check it, what should we do if .pi_block_cpu
> is changed after acquiring the lock as I mentioned above?

Drop the lock and start over. I.e. (taking your pseudo code)

restart:
local_pi_block_cpu = ...;
bail-if-invalid (e.g. -1 in current model)
spin_lock_irqsave(_cpu(, local_pi_block_cpu), flags);
if(local_pi_block_cpu != actual_pi_block_cpu) {
spin_unlock_irqrestore(_cpu(,local_pi_block_cpu), flags);
goto restart;
}
list_del(>arch.hvm_vmx.pi_blocked_vcpu_list);
spin_unlock_irqrestore(_cpu(,local_pi_block_cpu), flags);

>> > Anyway, I go through the main path of the code as below, I really don't 
>> > find
>> > anything unsafe here.
>> >
>> > context_switch() -->
>> > pi_ctxt_switch_from() -->
>> > vmx_pre_ctx_switch_pi() -->
>> > vcpu_unblock() -->
>> > vcpu_wake() -->
>> > SCHED_OP(VCPU2OP(v), wake, v) -->
>> > csched_vcpu_wake() -->
>> > __runq_insert()
>> > __runq_tickle()
>> >
>> > If you continue to think it is unsafe, 

Re: [Xen-devel] [PULL 0/19] xen-2015-09-08-tag

[Stefano Stabellini]

On Thu, 10 Sep 2015, Chen, Tiejun wrote:
> > xen-host-pci-device.c is only compiled if CONFIG_XEN_PCI_PASSTHROUGH
> > was set by configure. That won't be the case on OSX or Windows, where
> > the Xen headers don't exist.
> > 
> 
> Okay. This actually shouldn't be enabled on Windows so what about this?

I think it would be nicer to replace the pread than introducing ifdefs.


> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index 58a33fb..9a1fcb9 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -739,6 +739,7 @@ static const TypeInfo i440fx_info = {
>  .class_init= i440fx_class_init,
>  };
> 
> +#ifndef _WIN32
>  /* IGD Passthrough Host Bridge. */
>  typedef struct {
>  uint8_t offset;
> @@ -819,6 +820,7 @@ static const TypeInfo igd_passthrough_i440fx_info = {
>  .instance_size = sizeof(PCII440FXState),
>  .class_init= igd_passthrough_i440fx_class_init,
>  };
> +#endif
> 
>  static const char *i440fx_pcihost_root_bus_path(PCIHostState *host_bridge,
>  PCIBus *rootbus)
> @@ -861,7 +863,9 @@ static const TypeInfo i440fx_pcihost_info = {
>  static void i440fx_register_types(void)
>  {
>  type_register_static(_info);
> +#ifndef _WIN32
>  type_register_static(_passthrough_i440fx_info);
> +#endif
>  type_register_static(_pci_type_info);
>  type_register_static(_info);
>  type_register_static(_xen_info);
> 
> Thanks
> Tiejun
> 

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

Re: [Xen-devel] [PATCH] xen/domctl: lower loglevel of XEN_DOMCTL_memory_mapping

[Jan Beulich]

>>> On 10.09.15 at 07:28,  wrote:
>> > If the 64 limit was arbitrary then I would suggest increasing it to at 
>> > least
>>> 1024 so that
>>> at least 4M of BAR can be mapped in one go and it reduces the overhead by a
>>> factor of 16.
>>
>> 1024 may be a little much, but 256 is certainly a possibility, plus
>> Konrad's suggestion to allow this limit to be controlled via command
>> line option.
> 
> Are you guys talking this way?

Sort of (the patch has the intended effect, but for its size very
many rough edges).

Jan

> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 3946e4c..a9671bb 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -88,6 +88,10 @@ boolean_param("noapic", skip_ioapic_setup);
>   s8 __read_mostly xen_cpuidle = -1;
>   boolean_param("cpuidle", xen_cpuidle);
> 
> +/* once_mapping_mfns: memory mapping mfn bumbers once. */
> +unsigned int xen_once_mapping_mfns;
> +integer_param("once_mapping_mfns", xen_once_mapping_mfns);
> +
>   #ifndef NDEBUG
>   unsigned long __initdata highmem_start;
>   size_param("highmem-start", highmem_start);
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index 3bf39f1..82c85e3 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -33,6 +33,8 @@
>   #include 
>   #include 
> 
> +extern unsigned int xen_once_mapping_mfns;
> +
>   static DEFINE_SPINLOCK(domctl_lock);
>   DEFINE_SPINLOCK(vcpu_alloc_lock);
> 
> @@ -1035,7 +1037,7 @@ long 
> do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> 
>   ret = -E2BIG;
>   /* Must break hypercall up as this could take a while. */
> -if ( nr_mfns > 64 )
> +if ( nr_mfns > xen_once_mapping_mfns )
>   break;
> 
>   ret = -EPERM;
> 
> Thanks
> Tiejun




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

Re: [Xen-devel] OVMF/Xen, Debian wheezy can't boot with NX on stack (Was: Re: [edk2] [PATCH] OvmfPkg: prevent code execution from DXE stack)

[Laszlo Ersek]

On 09/10/15 05:05, Zeng, Star wrote:
> On 2015/9/9 18:48, Laszlo Ersek wrote:
>> me neither :)
>>
>>> but if this (executable code on stack) is
>>> happening in grub is there something which is explicitly forbidden to
>>> UEFI
>>> apps by the UEFI spec?
>>
>> Yes, there is. This small OvmfPkg patch only enables the edk2 feature
>> added by Star Zeng in
>>  for OVMF. That patch
>> (also referenced in my commit message by SVN rev) says,
>>
>>  This feature is added for UEFI spec that says
>>  "Stack may be marked as non-executable in identity mapped page
>>  tables".
>>  A PCD PcdSetNxForStack is added to turn on/off this feature, and it
>>  is FALSE by default.
>>
>> A UEFI app runs (well, *starts*, anyway) before ExitBootServices() /
>> SetVirtualAddressMap(), so it's bound by the above.
>>
>> The spec passage above is quoted from "2.3.2 IA-32 Platforms", and
>> "2.3.4 x64 Platforms", in chapter "2.3 Calling Conventions", where the
>> boot services time environment is specified.
>>
>> This is new in UEFI-2.5, and it comes from Mantis ticket 1224: "Adding
>> support for No executable data areas".
>>
>> ... The question could be then if grub (in Wheezy) should be adapted to
>> UEFI-2.5 (if that's possible) or if OVMF should be built without this
>> feature.
>>
>> Hmmm. Actually, I'm torn about the default for PcdSetNxForStack.
>>
>> Namely, Mantis ticket 1224 has come up before. There's another edk2
>> sub-feature related to this UEFI spec feature / Mantis ticket; the
>> properties table (controlled by "PcdPropertiesTableEnable"), and the
>> effects it has on the UEFI memory map, and the requirements it presents
>> for UEFI OSes.
>>
>> *That* sub-feature is extremely intrusive.
>> "MdeModulePkg/MdeModulePkg.dec" sets "PcdPropertiesTableEnable" TRUE by
>> default, and OvmfPkg inherits it. I have not overridden that default
>> just yet in OvmfPkg because the properties table feature depends on
>> something *else* too: sections in runtime DXE driver binaries must be
>> aligned at 4KB, and that is not ensured for OvmfPkg (yet?). Therefore,
>> the properties table feature is not active in OVMF at the moment due to
>> a "random build tools circumstance" -- and not because the feature flag
>> is actually disabled.
>>
>> Now, the properties table feature absolutely cannot be (effectively)
>> enabled in OVMF as a default. It would break Windows 7 / Windows Server
>> 2008 R2. A huge number of users run such guests for GPU passthrough.
>>
>> The idea that just crossed my mind was to introduce a new build flag for
>> OVMF, like -D NOEXEC_DATA_ENABLE. And this would then control
>> PcdSetNxForStack *and* PcdPropertiesTableEnable *together*:
>>
>> if NOEXEC_DATA_ENABLE:
>>PcdSetNxForStack := TRUE
>>PcdPropertiesTableEnable := TRUE
>> else
>>PcdSetNxForStack := FALSE
>>PcdPropertiesTableEnable := FALSE
>>
>> However, I think that's a bad idea.
>>
>> "PcdPropertiesTableEnable" breaks a very widely used guest OS for which
>> there is no update in sight (ie. no Service Pack 2 for Windows 7 /
>> Windows Server 2008 R2), but is otherwise supposed to be supported for
>> years to come.
>>
>> Whereas Debian Wheezy is not as widely used as a guest (for one: it
>> "competes" with all the other Linux distros from the same timeframe).
>> Debian Wheezy also provides a quite non-intrusive upgrade path (to
>> Jessie), which is not the case for Windows 7. So the breakage casued by
>> setting PcdSetNxForStack has much smaller impact, I think, and the
>> benefits might outweigh the breakage.
>>
>> Which just means that a heavy-handed -D NOEXEC_DATA_ENABLE, like above,
>> would not be appropriate. PcdSetNxForStack set, and
>> PcdPropertiesTableEnable clear, is a valid configuration.
>>
>> In any case, I sort of convinced myself that Wheezy's grub is not at
>> fault; it was simply written against an earlier release of the UEFI spec.
>>
>> How about this:
>>
>> - (somewhat unrelatedly, but for completeness):
>>I post a patch that exposes PcdPropertiesTableEnable with a build
>>time flag (NX_PROP_TABLE_ENABLE) with an OVMF-default of FALSE,
>>
>> - I post another patch that exposes PcdSetNxForStack with a build time
>>flag (NX_STACK_ENABLE) with an OVMF-default of *TRUE*. You could then
>>pass -D NX_STACK_ENABLE=FALSE when you build OVMF for any environment
>>where Wheezy guests are expected.
>>
>> Jordan?
>>
>> Yet another (quite complex) possibility:
>>
>> - According to "MdeModulePkg/MdeModulePkg.dec",
>>a platform DSC can already type PcdPropertiesTableEnable as a dynamic
>>PCD. We could allow the same for PcdSetNxForStack. Star?
> 
> I think PcdSetNxForStack could be extended to support dynamic type if
> the platform really needs to set the PCD dynamically after some
> condition check.

Thank you. (Also for the PDF reference in your other email.)

Laszlo

> 
>>
>>Both PCDs are consumed "acceptably late" (the first in the 

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

[Shannon Zhao]

From: Shannon Zhao 

These EFI stub parameters are used to internal communication between EFI
stub and Linux kernel and EFI stub creates these parameters. But for Xen
on ARM when booting with UEFI, Xen will create a minimal DT providing
these parameters for Dom0 and Dom0 is not only Linux kernel, but also
other OS (such as FreeBSD) which will be used in the future. So here
we plan to standardize the names by dropping the prefix "linux," and
make them common to other OS. Also this will not break the compatibility
since these parameters are used to internal communication between EFI
stub and kernel.

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

[1]http://lists.xenproject.org/archives/html/xen-devel/2015-08/msg02250.html

 Documentation/arm/uefi.txt | 10 +-
 drivers/firmware/efi/efi.c | 10 +-
 drivers/firmware/efi/libstub/fdt.c | 10 +-
 3 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/Documentation/arm/uefi.txt b/Documentation/arm/uefi.txt
index d60030a..8c83243 100644
--- a/Documentation/arm/uefi.txt
+++ b/Documentation/arm/uefi.txt
@@ -45,18 +45,18 @@ following parameters:
 

 Name  | Size   | Description
 

-linux,uefi-system-table   | 64-bit | Physical address of the UEFI System Table.
+uefi-system-table | 64-bit | Physical address of the UEFI System Table.
 

-linux,uefi-mmap-start | 64-bit | Physical address of the UEFI memory map,
+uefi-mmap-start   | 64-bit | Physical address of the UEFI memory map,
   || populated by the UEFI GetMemoryMap() call.
 

-linux,uefi-mmap-size  | 32-bit | Size in bytes of the UEFI memory map
+uefi-mmap-size| 32-bit | Size in bytes of the UEFI memory map
   || pointed to in previous entry.
 

-linux,uefi-mmap-desc-size | 32-bit | Size in bytes of each entry in the UEFI
+uefi-mmap-desc-size   | 32-bit | Size in bytes of each entry in the UEFI
   || memory map.
 

-linux,uefi-mmap-desc-ver  | 32-bit | Version of the mmap descriptor format.
+uefi-mmap-desc-ver| 32-bit | Version of the mmap descriptor format.
 

 linux,uefi-stub-kern-ver  | string | Copy of linux_banner from build.
 

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index d6144e3..3878715 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -481,11 +481,11 @@ static __initdata struct {
int offset;
int size;
 } dt_params[] = {
-   UEFI_PARAM("System Table", "linux,uefi-system-table", system_table),
-   UEFI_PARAM("MemMap Address", "linux,uefi-mmap-start", mmap),
-   UEFI_PARAM("MemMap Size", "linux,uefi-mmap-size", mmap_size),
-   UEFI_PARAM("MemMap Desc. Size", "linux,uefi-mmap-desc-size", desc_size),
-   UEFI_PARAM("MemMap Desc. Version", "linux,uefi-mmap-desc-ver", desc_ver)
+   UEFI_PARAM("System Table", "uefi-system-table", system_table),
+   UEFI_PARAM("MemMap Address", "uefi-mmap-start", mmap),
+   UEFI_PARAM("MemMap Size", "uefi-mmap-size", mmap_size),
+   UEFI_PARAM("MemMap Desc. Size", "uefi-mmap-desc-size", desc_size),
+   UEFI_PARAM("MemMap Desc. Version", "uefi-mmap-desc-ver", desc_ver)
 };
 
 struct param_info {
diff --git a/drivers/firmware/efi/libstub/fdt.c 
b/drivers/firmware/efi/libstub/fdt.c
index ef5d764..e94589a 100644
--- a/drivers/firmware/efi/libstub/fdt.c
+++ b/drivers/firmware/efi/libstub/fdt.c
@@ -118,31 +118,31 @@ efi_status_t update_fdt(efi_system_table_t *sys_table, 
void *orig_fdt,
/* Add FDT entries for EFI runtime services in chosen node. */
node = fdt_subnode_offset(fdt, 0, "chosen");
fdt_val64 = cpu_to_fdt64((u64)(unsigned long)sys_table);
-   status = fdt_setprop(fdt, node, "linux,uefi-system-table",
+   status = fdt_setprop(fdt, node, "uefi-system-table",
 _val64, sizeof(fdt_val64));
if (status)
goto fdt_set_fail;
 
fdt_val64 = 

Re: [Xen-devel] [PATCH] xen/domctl: lower loglevel of XEN_DOMCTL_memory_mapping

[Jan Beulich]

>>> On 10.09.15 at 10:55,  wrote:
>>  Sort of (the patch has the intended effect, but for its size very
>> many rough edges).
>>
> 
> I guess we need to amend the original parameter, once_mapping_mfns, like 
> this,
> 
> /* xen_once_mapping_mfns: memory mapping mfn bumbers once. */
> unsigned int xen_once_mapping_mfns;
> size_param("once_mapping_mfns", xen_once_mapping_mfns);
> 
> static void xen_once_mapping_mfns_setup(void)
> {
>  if ( once_mapping_mfns < 64 )
>  xen_once_mapping_mfns = 64;
>  else if ( once_mapping_mfns > 1024 )
>  xen_once_mapping_mfns = 1024;
>  else
>  xen_once_mapping_mfns = once_mapping_mfns;
> }

Right, that's one of the things that would need taking care of.
(Whether enforcing an upper limit is actually needed I'm not
sure - we generally allow the admin to shoot himself in the foot
if he wants to. And whether the lower limit should be 64 instead
of just ensuring the limit is not zero is another question.)

Jan


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

Re: [Xen-devel] [PULL 0/19] xen-2015-09-08-tag

[Peter Maydell]

On 10 September 2015 at 02:12, Chen, Tiejun  wrote:
> On 9/9/2015 9:06 PM, Stefano Stabellini wrote:
>>
>> On Tue, 8 Sep 2015, Peter Maydell wrote:
>>>
>>> On 8 September 2015 at 18:21, Stefano Stabellini
>>>  wrote:
>>> > The following changes since commit
>>> > 8611280505119e296757a60711a881341603fa5a:
>>> >
>>> >   target-microblaze: Use setcond for pcmp* (2015-09-08 08:49:33 +0200)
>>> >
>>> > are available in the git repository at:
>>> >
>>> >   git://xenbits.xen.org/people/sstabellini/qemu-dm.git
>>> > tags/xen-2015-09-08-tag
>>> >
>>> > for you to fetch changes up to
>>> > ba2250ad148997b1352aba976aac66b55410e7e4:
>>> >
>>> >   xen/pt: Use XEN_PT_LOG properly to guard against compiler warnings.
>>> > (2015-09-08 15:21:56 +)
>>> >
>>> > 
>>> > Xen branch xen-2015-09-08
>>> >
>>> > 
>>>
>>> Hi. I'm afraid this fails to build on OSX (and probably Windows too,
>>> though that build hasn't run yet):
>>>
>>>   CCi386-softmmu/hw/i386/pci-assign-load-rom.o
>>> /Users/pm215/src/qemu/hw/i386/pci-assign-load-rom.c:6:10: fatal error:
>>>   'sys/io.h' file not found
>>> #include 
>>>  ^
>>>   CCalpha-softmmu/hw/alpha/pci.o
>>> 1 error generated.
>>
>>
>> Tiejun,
>>
>> this is caused by 33d33242b7d802e6c994f3d56ecba96a66465dc3,
>> "hw/pci-assign: split pci-assign.c". Could you please double-check
>> non-Linux builds?
>
>
> Its interesting.
>
> As you see this short log, "hw/pci-assign: split pci-assign.c", so this
> means I just extract something from the original hw/i386/kvm/pci-assign.c,
> and here so I just keep those original head files residing
> hw/i386/kvm/pci-assign.c, and I didn't introduce anything new.

hw/i386/kvm/pci-assign.c is only built if configure set CONFIG_KVM,
which it won't do on Windows or OSX builds.

It sounds like your patch has incorrectly moved code out of files
which are compiled only if KVM is present, or only if we're doing
Xen PCI passthrough, and into compiled-for-everything files.

> So its very probably that you still can't compile successfully even without
> my commit on OSX/Windows, right?

OSX and Windows build fine on master at the moment; I check this
for every pull request.

thanks
-- PMM

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

[Xen-devel] [distros-debian-jessie test] 37919: regressions - FAIL

[Platform Team regression test user]

flight 37919 distros-debian-jessie real [real]
http://osstest.xs.citrite.net/~osstest/testlogs/logs/37919/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 build-armhf-pvops 5 kernel-build  fail REGR. vs. 37850
 test-amd64-amd64-i386-jessie-netboot-pygrub 9 debian-di-install fail REGR. vs. 
37850
 build-i3865 xen-build fail REGR. vs. 37850
 test-amd64-amd64-amd64-jessie-netboot-pvgrub 9 debian-di-install fail REGR. 
vs. 37850

Tests which did not succeed, but are not blocking:
 test-amd64-i386-i386-jessie-netboot-pvgrub  1 build-check(1)   blocked n/a
 test-amd64-i386-amd64-jessie-netboot-pygrub  1 build-check(1)  blocked n/a
 test-armhf-armhf-armhf-jessie-netboot-pygrub  1 build-check(1) blocked n/a

baseline version:
 flight   37850

jobs:
 build-amd64  pass
 build-armhf  pass
 build-i386   fail
 build-amd64-pvopspass
 build-armhf-pvopsfail
 build-i386-pvops pass
 test-amd64-amd64-amd64-jessie-netboot-pvgrub fail
 test-amd64-i386-i386-jessie-netboot-pvgrub   blocked 
 test-amd64-i386-amd64-jessie-netboot-pygrub  blocked 
 test-armhf-armhf-armhf-jessie-netboot-pygrub blocked 
 test-amd64-amd64-i386-jessie-netboot-pygrub  fail



sg-report-flight on osstest.xs.citrite.net
logs: /home/osstest/logs
images: /home/osstest/images

Logs, config files, etc. are available at
http://osstest.xs.citrite.net/~osstest/testlogs/logs

Test harness code can be found at
http://xenbits.xensource.com/gitweb?p=osstest.git;a=summary


Push not applicable.


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

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

[Mark Rutland]

Hi,

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

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

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

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

That at least includes the following:

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

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

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

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

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

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

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

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

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

Thanks,
Mark.

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

Re: [Xen-devel] [v2][PATCH] xen/vtd/iommu: permit group devices to passthrough in relaxed mode

[Wei Liu]

On Thu, Sep 10, 2015 at 02:09:39AM -0600, Jan Beulich wrote:
> >>> On 10.09.15 at 07:23,  wrote:
> >>  From: Jan Beulich [mailto:jbeul...@suse.com]
> >> Sent: Wednesday, September 09, 2015 2:55 PM
> >> 
> >> >>> On 09.09.15 at 03:59,  wrote:
> >> > @@ -2310,12 +2312,16 @@ static int intel_iommu_assign_device(
> >> >   PCI_DEVFN2(bdf) == devfn &&
> >> >   rmrr->scope.devices_cnt > 1 )
> >> >  {
> >> > -printk(XENLOG_G_ERR VTDPREFIX
> >> > -   " cannot assign %04x:%02x:%02x.%u"
> >> > +bool_t relaxed = !!(flag & XEN_DOMCTL_DEV_RDM_RELAXED);
> >> > +
> >> > +printk(XENLOG_G_WARNING VTDPREFIX
> >> 
> >> Well, I can live with this always being a warning, but it's not what I
> >> had asked for. The VT-d maintainers will have to judge.
> >> 
> > 
> > Need to have separate warning/error level for relax/strict.
> > 
> > However I don't think this patch is a right fix. So far relax/strict policy
> > is per-domain. what about one VM specifies relax while another VM
> > specifies strict when each is assigned with a device sharing rmrr
> > with the other? In that case it becomes a system-wide security hole.
> 
> The one specifying "strict" won't gets its device assigned (due to
> the code above, taking the path that was there already without
> the patch), so I don't see the security issue.
> 

Agreed. A VM can't get such device assigned in the first place, so the
hypothetical scenario doesn't exist.

Wei.

> Jan

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

Re: [Xen-devel] [PATCH v4 10/11] x86/intel_pstate: support the use of intel_pstate in pmstat.c

[Wang, Wei W]

On 10/09/2015 17:55,  Jan Beulich wrote:
>>> On 10.09.15 at 11:33,  wrote:
> On 09/09/2015 16:17,  Jan Beulich wrote:
 On 10.09.15 at 07:35,  wrote:
>> Seems we still cannot get rid of these strncmp()s. Is this 
>> acceptable, or should we change "struct cpufreq_driver" to use enum 
>> represented driver name as well, or do you have a better suggestion?
> 
>> The one I explained before: Express the data representation type in 
>> an enum,
> not the driver kind. But even if we went with the
>> above, the strcmp() ugliness would at least be limited to the 
>> producer, not
> enforced onto any number of consumers.
> 
> No.  I think in our previous discussion, there is no problem with "the 
> data representation type", any future data representation, as long as 
> it is in "uint32_t", it can use "uint32_t scaling_max_perf" to hold 
> that value representation.

> Okay, "representation" was a badly chose term. "Meaning of the data"
> would have been better.

> Your concern was that the following doesn't scale well.
> 
> +if (!strncmp(p_cpufreq->scaling_driver,
> +  "intel_pstate", CPUFREQ_NAME_LEN) )
> 
> So we are trying to change the driver name thing to be in enum. 

> No, that was never what I've been asking for. I've always said that the 
> consumer of the data ought to have a way to know what 
> kind of data it is dealing with. I.e. the enum ought to represent what 
> meaning the data has (frequency vs percentage).

Ok. If we add this "enum meaning_of_data", the xen_get_cpufreq_para will exceed 
128Byte, which cannot even pass the compilation. I am not sure how to deal with 
this nicely. Do you have a suggestion? 

Best,
Wei

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

Re: [Xen-devel] [edk2] EDK II & GPL - Re: OVMF BoF @ KVM Forum 2015

[Sharma Bhupesh]

> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Jordan Justen
> Sent: Thursday, September 10, 2015 11:03 AM
> On 2015-09-09 20:26:54, Andrew Fish wrote:
> > > On Sep 9, 2015, at 5:41 PM, Jordan Justen 
> wrote:
> > > On 2015-09-09 16:05:20, Andrew Fish wrote:
> > >> So you have a legal degree and are speaking on behalf of your
> > >> employer on this subject?
> > >
> > > No and no. How about you? :)
> >
> > No but I have to review any code contributed to the open source
> > project to make sure it follows the corporate polices.
> 
> Is Apple corporate policy that you could never contribute to a project
> that has a GPL directory in the tree?
> 
> > > Nevertheless, I have not heard the interpretation that just having
> > > GPL in a source tree would impact your code, even if you do not
> > > include, nor link to it. Is this Apple's interpretation of how GPL
> works?
> >
> > No but thanks for making my point for me. 1st off the rules are made
> > by lawyers and managers so you trying to argue logic is kind of funny.
> > What does logic have to do with it.
> 
> Whoa! What's next in this crazy world? Dogs and cats living together!
> Mass hysteria! How can we be sure that the lawyers won't decide that BSD
> means GPL and vice versa? ;)
> 
> > Your company started this edk2 project as a BSD project, and I assume
> > there was a reason for that.
> 
> And then more licenses were added.
> 
> > The reasons rules like this end up getting made is that developers
> > like you are confused about the company policy regarding open source,
> > closed source and protecting intellectual property rights.
> > So your very smart and well versed and you are confused, so
> 
> I don't think I'm confused (or smart :), but you are trying hard to make
> it seem confusing and scary.
> 
> Anyway, you are correct. We do have rules. But, I don't think those rules
> prevent us from discussing *possible* changes to those rules.
> 
> > some more jr. engineer has no hope of getting it right and would copy
> > the GPL code and be clueless to what he just did. As I always say a
> > development process exists to slow down the best developer, at the
> > price of preventing the most jr. developers from doing something
> > stupid.
> 
> If we have another repo with GplDriverPkg, then I guess the same jr.
> developer might just go find the code over there and copy it.
> 
> > > I would be more worried about the GPL based drivers becoming too
> > > featureful over time, and the permissively licensed code not being
> > > very useful. For example, I'm worried that the non-GPL OVMF may end
> > > up missing a lot of features.
> >
> > Then logically you should just make OVMF a GPL project?
> 
> Not if you are someone that prefers permissive source licenses, such as
> myself.
> 
> I'm basically trying to argue the other side of this to not let the GPL
> FUD go by unimpeded.
> 
> Laszlo's email raised the GPL question, but I was not sure what the EDK
> II community would accept with regards to GPL. Thus ... I asked. I guess
> I'm getting a better idea with regards to Apple and HP. :)
> 
> In your opinion, would we be able to discuss patches for a *separate*
> repo with GplDriverPkg on edk2-devel?
 
Sorry to chime-in into the discussion out-of-turn, but we at Freescale were 
struggling
for some time with the different licensing models/downloading privileges  of 
EDK2, FatDriverPkg
and SCT 2.4.

So based on my limited understanding, can't the OVMF driver which uses features 
from some GPL based code,
carry a dual license (GPL + x11 [MIT]), like most device trees in Linux do now
(including the Freescale ARMv8 DTS, see [1] for reference), thus allowing its 
usage
across both GPL and BSD based projects (like EDK2).

We already have examples of such dual-licensed code (libfdt) being used in 
BSD-licensed EDK2
(see [2] for details).

In such a case we might not be required to create a separate GIT repo for the 
same.

[1] https://patchwork.ozlabs.org/patch/385248/
[2] 
http://sourceforge.net/p/tianocore/edk2/ci/3e8576dd363fe516ceec1ddc4aff51bc5a3d4bd7/

Regards,
Bhupesh

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

Re: [Xen-devel] [PATCH v6 16/18] vmx: Add some scheduler hooks for VT-d posted interrupts

[Wu, Feng]

> -Original Message-
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: Thursday, September 10, 2015 4:28 PM
> To: Wu, Feng
> Cc: Andrew Cooper; Dario Faggioli; George Dunlap; Tian, Kevin;
> xen-devel@lists.xen.org; Keir Fraser
> Subject: RE: [PATCH v6 16/18] vmx: Add some scheduler hooks for VT-d posted
> interrupts
> 
> >>> On 10.09.15 at 04:07,  wrote:
> >> From: Jan Beulich [mailto:jbeul...@suse.com]
> >> Sent: Wednesday, September 09, 2015 6:27 PM
> >> >>> On 09.09.15 at 10:56,  wrote:
> >> >> From: Jan Beulich [mailto:jbeul...@suse.com]
> >> >> Sent: Monday, September 07, 2015 8:55 PM
> >> >> >>> On 25.08.15 at 03:57,  wrote:
> >> >> > +
> >> >> > +/*
> >> >> > + * Delete the vCPU from the related block list
> >> >> > + * if we are resuming from blocked state.
> >> >> > + */
> >> >> > +if ( v->arch.hvm_vmx.pi_block_cpu != -1 )
> >> >> > +{
> >> >> > +spin_lock_irqsave(_cpu(pi_blocked_vcpu_lock,
> >> >> > +  v->arch.hvm_vmx.pi_block_cpu),
> >> >> flags);
> >> >> > +list_del_init(>arch.hvm_vmx.pi_blocked_vcpu_list);
> >> >>
> >> >> Shouldn't you set .pi_block_cpu back to -1 along with removing
> >> >> the vCPU from the list? Which then ought to allow using just
> >> >> list_del() here?
> >> >
> >> > Here is the story about using list_del_init() instead of list_del(), (the
> >> > same reason for using list_del_init() in pi_wakeup_interrupt() ).
> >> > If we use list_del(), that means we need to set .pi_block_cpu to
> >> > -1 after removing the vCPU from the block list, there are two
> >> > places where the vCPU is removed from the list:
> >> > - arch_vcpu_wake_prepare()
> >> > - pi_wakeup_interrupt()
> >> >
> >> > That means we also need to set .pi_block_cpu to -1 in
> >> > pi_wakeup_interrupt(), which seems problematic to me, since
> >> > .pi_block_cpu is used to get the per-CPU spin lock, it is not safe
> >> > to change it in pi_wakeup_interrupt(), maybe someone is using
> >> > it at the same time.
> >> >
> >> > And the check "if ( v->arch.hvm_vmx.pi_block_cpu != -1 )" here
> >> > is only used when .pi_block_cpu is set to -1 at the initial time.
> >> >
> >> > If you have any good suggestions about this, I will be all ears.
> >>
> >> Latching pi_block_cpu into a local variable would take care of that
> >> part of the problem. Of course you then may also need to check
> >> that while waiting to acquire the lock pi_block_cpu didn't change.
> >
> > Thanks for the suggestion! But I think it is not easy to "check
> > "that while waiting to acquire the lock pi_block_cpu didn't change".
> > Let's take the following pseudo code as an example:
> >
> > int local_pi_block_cpu = v->arch.hvm_vmx.pi_block_cpu;
> >
> > ..
> >
> > spin_lock_irqsave(_cpu(pi_blocked_vcpu_lock,
> >local_pi_block_cpu), flags);
> > list_del(>arch.hvm_vmx.pi_blocked_vcpu_list);
> > spin_unlock_irqrestore(_cpu(pi_blocked_vcpu_lock,
> >local_pi_block_cpu), flags);
> >
> > If .pi_block_cpu is changed to -1 by others during calling list_del()
> > above, that means the vCPU is removed by others, then calling
> > list_del() here again would be problematic. I think there might be
> > other corner cases for this. So I suggest adding some comments
> > for list_del_init() as you mentioned below.
> 
> That's why I said "check that while waiting to acquire the lock
> pi_block_cpu didn't change." The code you present does not do
> this.

I didn't see we need implement it as the above code, I just list it
here the show it is hard to do that. 
First, how to check it while waiting to acquire the lock .pi_block_cpu
didn't change?
Secondly, even if we can check it, what should we do if .pi_block_cpu
is changed after acquiring the lock as I mentioned above?

> 
> >> >> > +do {
> >> >> > +old.control = new.control = pi_desc->control;
> >> >> > +
> >> >> > +/* Should not block the vCPU if an interrupt was posted
> for
> >> it.
> >> >> */
> >> >> > +if ( pi_test_on() )
> >> >> > +{
> >> >> > +
> spin_unlock_irqrestore(>arch.hvm_vmx.pi_lock,
> >> >> gflags);
> >> >> > +vcpu_unblock(v);
> >> >>
> >> >> Calling vcpu_unblock() in the middle of context_switch()? Why? And
> >> >> is this safe?
> >> >
> >> > I cannot see anything unsafe so far, can some scheduler maintainer
> >> > help to confirm it? Dario? George?
> >>
> >> But first and foremost you ought to answer the "why".
> >
> > I think if you think this solution is unsafe, you need point out where it
> > is, not
> > just ask "is this safe ?", I don't think this is an effective comments.
> 
> It is my general understanding that people wanting code to be
> included have to prove their code is okay, not reviewers to prove
> the code is broken.
> 
> > Anyway, I go through the main path of the code as below, I really don't find
> > 

Re: [Xen-devel] [PULL 0/19] xen-2015-09-08-tag

[Chen, Tiejun]

As you see this short log, "hw/pci-assign: split pci-assign.c", so this
means I just extract something from the original hw/i386/kvm/pci-assign.c,
and here so I just keep those original head files residing
hw/i386/kvm/pci-assign.c, and I didn't introduce anything new.


hw/i386/kvm/pci-assign.c is only built if configure set CONFIG_KVM,
which it won't do on Windows or OSX builds.

It sounds like your patch has incorrectly moved code out of files
which are compiled only if KVM is present, or only if we're doing
Xen PCI passthrough, and into compiled-for-everything files.



Yes, we want to share this chunk of codes between Xen and Kvm. Just to 
this error, could we remove #include ? As I mentioned I still 
can compile this file without this head file.


Thanks
Tiejun

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

Re: [Xen-devel] [PATCH V2] x86/hvm: fix saved pmtimer value

[Jan Beulich]

>>> On 10.09.15 at 08:06,  wrote:
> The ACPI PM timer is sometimes broken on live migration.
> Since vcpu->arch.hvm_vcpu.guest_time is always zero in other than
> "delay for missed ticks mode". Even in "delay for missed ticks mode",
> vcpu's guest_time field is not valid (i.e. zero) when
> the state of vcpu is "blocked". (see pt_save_timer function)

Better, but still leaving open why we should not use
v->arch.hvm_vcpu.guest_time when non-zero. In particular your
reference to a blocked vCPU is bogus, as in that case the field will
continue to be zero.

The fear I have with completely ignoring the field is that we may
lose guest time precision in the delay-for-missed-ticks mode. Otoh
I appreciate that you patch gets things in line with vHPET; the
question here is whether the vHPET save logic should also take
that field into account when non-zero.

> The original author (Tim Deegan) of pmtimer_save() must have intended
> that it saves the last scheduled time of the vcpu. Unfortunately it was
> already implied this bug. FYI, there is no other timer mode than 
> "delay for missed ticks mode" then.

I'm not sure how to interpret this paragraph. It would have been
helpful anyway if you had Cc-ed Tim right away...

Jan


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

[Xen-devel] [PATCH net v3 1/2] xen-netback: respect user provided max_queues

[Wei Liu]

Originally that parameter was always reset to num_online_cpus during
module initialisation, which renders it useless.

The fix is to only set max_queues to num_online_cpus when user has not
provided a value.

Reported-by: Johnny Strom 
Signed-off-by: Wei Liu 
Reviewed-by: David Vrabel 
Acked-by: Ian Campbell 
---
 drivers/net/xen-netback/netback.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/xen-netback/netback.c 
b/drivers/net/xen-netback/netback.c
index 42569b9..f585c6a 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -2105,8 +2105,11 @@ static int __init netback_init(void)
if (!xen_domain())
return -ENODEV;
 
-   /* Allow as many queues as there are CPUs, by default */
-   xenvif_max_queues = num_online_cpus();
+   /* Allow as many queues as there are CPUs if user has not
+* specified a value.
+*/
+   if (xenvif_max_queues == 0)
+   xenvif_max_queues = num_online_cpus();
 
if (fatal_skb_slots < XEN_NETBK_LEGACY_SLOTS_MAX) {
pr_info("fatal_skb_slots too small (%d), bump it to 
XEN_NETBK_LEGACY_SLOTS_MAX (%d)\n",
-- 
2.1.4


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

Re: [Xen-devel] [PULL 0/19] xen-2015-09-08-tag

[Chen, Tiejun]

xen-host-pci-device.c is only compiled if CONFIG_XEN_PCI_PASSTHROUGH
was set by configure. That won't be the case on OSX or Windows, where
the Xen headers don't exist.



Okay. This actually shouldn't be enabled on Windows so what about this?

diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index 58a33fb..9a1fcb9 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -739,6 +739,7 @@ static const TypeInfo i440fx_info = {
 .class_init= i440fx_class_init,
 };

+#ifndef _WIN32
 /* IGD Passthrough Host Bridge. */
 typedef struct {
 uint8_t offset;
@@ -819,6 +820,7 @@ static const TypeInfo igd_passthrough_i440fx_info = {
 .instance_size = sizeof(PCII440FXState),
 .class_init= igd_passthrough_i440fx_class_init,
 };
+#endif

 static const char *i440fx_pcihost_root_bus_path(PCIHostState *host_bridge,
 PCIBus *rootbus)
@@ -861,7 +863,9 @@ static const TypeInfo i440fx_pcihost_info = {
 static void i440fx_register_types(void)
 {
 type_register_static(_info);
+#ifndef _WIN32
 type_register_static(_passthrough_i440fx_info);
+#endif
 type_register_static(_pci_type_info);
 type_register_static(_info);
 type_register_static(_xen_info);

Thanks
Tiejun

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

[Xen-devel] [linux-3.14 test] 61641: regressions - FAIL

[osstest service owner]

flight 61641 linux-3.14 real [real]
http://logs.test-lab.xenproject.org/osstest/logs/61641/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-amd64-i386-xl-vhd   13 guest-saverestore fail REGR. vs. 60666
 test-amd64-i386-xl-qcow2 13 guest-saverestore fail REGR. vs. 60666
 test-amd64-amd64-xl-vhd  19 guest-start.2fail in 61263 REGR. vs. 60666
 test-amd64-amd64-xl-qcow219 guest-start.2fail in 61263 REGR. vs. 60666

Tests which are failing intermittently (not blocking):
 test-armhf-armhf-libvirt-xsm  3 host-install(3)  broken in 61522 pass in 61641
 test-amd64-amd64-xl-vhd  14 guest-localmigrate fail in 60949 pass in 61263
 test-amd64-amd64-libvirt-raw 17 guest-start/debian.repeat fail in 60949 pass 
in 61641
 test-amd64-amd64-libvirt 17 guest-start.2  fail in 60949 pass in 61641
 test-amd64-amd64-xl-qemuu-debianhvm-amd64 14 guest-saverestore.2 fail in 60949 
pass in 61641
 test-amd64-amd64-xl-qemuu-ovmf-amd64 19 guest-start/debianhvm.repeat fail in 
60949 pass in 61641
 test-amd64-i386-libvirt 18 guest-start/debian.repeat fail in 61263 pass in 
61641
 test-amd64-i386-xl-qemuu-ovmf-amd64 12 guest-saverestore fail in 61263 pass in 
61641
 test-amd64-i386-xl-qemuu-debianhvm-amd64 19 guest-start/debianhvm.repeat fail 
in 61263 pass in 61641
 test-amd64-i386-xl-qemut-debianhvm-amd64 18 guest-start.2 fail in 61263 pass 
in 61641
 test-amd64-i386-xl-qemuu-winxpsp3-vcpus1 19 guest-start/win.repeat fail in 
61263 pass in 61641
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 16 
guest-start/debianhvm.repeat fail in 61522 pass in 61641
 test-amd64-amd64-xl-qcow210 guest-startfail in 61522 pass in 61641
 test-amd64-i386-rumpuserxen-i386 15 
rumpuserxen-demo-xenstorels/xenstorels.repeat fail pass in 60949
 test-amd64-amd64-xl-vhd  13 guest-saverestore   fail pass in 61263
 test-amd64-amd64-xl-qcow213 guest-saverestore   fail pass in 61263
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 16 
guest-start/debianhvm.repeat fail pass in 61263
 test-armhf-armhf-xl-vhd   5 xen-install fail pass in 61522
 test-amd64-amd64-libvirt-xsm 17 guest-start.2   fail pass in 61522
 test-amd64-amd64-libvirt-qcow2 13 guest-saverestore fail pass in 61522
 test-amd64-i386-qemut-rhel6hvm-amd 12 guest-start/redhat.repeat fail pass in 
61522
 test-amd64-i386-xl-qemut-debianhvm-amd64-xsm 19 guest-start/debianhvm.repeat 
fail pass in 61522
 test-amd64-amd64-xl-qemut-stubdom-debianhvm-amd64-xsm 13 guest-localmigrate 
fail pass in 61522

Regressions which are regarded as allowable (not blocking):
 test-amd64-amd64-libvirt-vhd 16 guest-start.2 fail REGR. vs. 60666
 test-amd64-i386-libvirt-vhd  13 guest-saverestore fail REGR. vs. 60666
 test-amd64-i386-libvirt-qcow2 13 guest-saverestorefail REGR. vs. 60666
 test-amd64-amd64-libvirt-qcow2 14 guest-saverestore.2 fail in 61522 REGR. vs. 
60666
 test-amd64-amd64-xl-qemut-stubdom-debianhvm-amd64-xsm 16 
guest-localmigrate/x10 fail in 61522 blocked in 60666
 test-armhf-armhf-xl   6 xen-boot fail   like 60666
 test-armhf-armhf-xl-multivcpu  6 xen-boot fail  like 60666
 test-armhf-armhf-libvirt  6 xen-boot fail   like 60666
 test-armhf-armhf-xl-credit2   6 xen-boot fail   like 60666
 test-amd64-amd64-rumpuserxen-amd64 15 
rumpuserxen-demo-xenstorels/xenstorels.repeat fail like 60666
 test-amd64-i386-xl-qemut-win7-amd64 17 guest-stop  fail like 60666
 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stop fail like 60666
 test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stop fail like 60666

Tests which did not succeed, but are not blocking:
 test-armhf-armhf-xl-vhd   6 xen-boot  fail in 61522 never pass
 test-armhf-armhf-xl-raw   6 xen-boot fail   never pass
 test-armhf-armhf-xl-qcow2 6 xen-boot fail   never pass
 test-armhf-armhf-libvirt-vhd  6 xen-boot fail   never pass
 test-armhf-armhf-xl-arndale   6 xen-boot fail   never pass
 test-armhf-armhf-xl-rtds  6 xen-boot fail   never pass
 test-armhf-armhf-libvirt-qcow2  6 xen-boot fail never pass
 test-armhf-armhf-libvirt-raw  6 xen-boot fail   never pass
 test-armhf-armhf-xl-cubietruck  6 xen-boot fail never pass
 test-armhf-armhf-xl-xsm   6 xen-boot fail   never pass
 test-amd64-amd64-libvirt-xsm 12 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check 
fail never pass
 test-armhf-armhf-libvirt-xsm  6 xen-boot fail   never pass
 test-amd64-amd64-libvirt-qcow2 11 migrate-support-checkfail never pass
 

Re: [Xen-devel] [PATCH v4 10/11] x86/intel_pstate: support the use of intel_pstate in pmstat.c

[Wang, Wei W]

On 09/09/2015 16:17,  Jan Beulich wrote:
>>> On 10.09.15 at 07:35,  wrote:
> On 09/09/2015 23:55,  Jan Beulich wrote:
 On 09.09.15 at 17:16,  wrote:
>> On 09/09/2015 21:12,  Jan Beulich wrote:
> On 09.09.15 at 14:56,  wrote:
>>> Can you please explain more why it doesn't scale? 
>>> From my point of view, any other future value representation can be 
>>> passed from the producer to the related consumer through this method.
>> 
>>> Did you read all of my earlier replies? I already said there "Just 
>>> consider
>> what happens to the code when we end up gaining a few
>>> more drivers providing percentage values, and perhaps another one 
>>> providing
>> a third variant of output representation."
>> 
>> Yes, I have read that. I am not sure if I got your point, but my 
>> meaning was when we add new drivers to the code, e.g. xx_pstate 
>> driver, we can still have the name, "xx_pstate", assigned to 
>> "p_cpufreq->scaling_driver" to distinguish one another. If the driver 
>> uses a different variant of output representation, which cannot be 
>> held by " uint32_t scaling_max_perf" (it needs "uint64_t" for 
>> example, then
> that driver developer needs to add a new field here like  "
>> uint64_t scaling_max_perf_xx").
>> What is the scaling problem? 
> 
>>  if (strcmp() == 0 ||
>>  strcmp() == 0 ||
>>  strcmp() == 0) {
>>  ...
>>  } else if (strcmp() == 0) {
>>  ...
>>  } else {
>>  ...
>>  }
> 
>> is just ugly, and gets all the uglier the more strcmp()s get added.
>> Have a boolean or enumeration indicating what kind of data there is, 
>> and the
> above changes to
> 
>>  switch (kind) {
>>  case absolute: ...
>>  case percentage: ...
>>  }
> 
> Ok. I will replace the default "scaling_driver[CPUFREQ_NAME_LEN]" with 
> an enum type, like this following ...
> - char scaling_driver[CPUFREQ_NAME_LEN];
> + enum scaling_driver_flag scaling_driver;
> ...
> 
> We cannot keep both of the above two there, because there is a 128Byte 
> size limit. Then somewhere, we need to translate the 
> character-represented scaling_driver to our new enum-represented 
> scaling_driver. For example, in pmstat.c, the following:
> 
> if ( cpufreq_driver->name[0] )
> strlcpy(op->u.get_para.scaling_driver,
> cpufreq_driver->name, CPUFREQ_NAME_LEN); else
> strlcpy(op->u.get_para.scaling_driver, "Unknown", 
> CPUFREQ_NAME_LEN);
> 
> needs to be changed to:
> if ( strncmp(cpufreq_driver->name[0], "intel_pstate", CPUFREQ_NAME_LEN) == 0 )
> op->u.get_para.scaling_driver = INTEL_PSTATE; else if ( 
> strncmp(cpufreq_driver->name[0], "acpi_cpufreq", CPUFREQ_NAME_LEN) == 
> 0 )
> op->u.get_para.scaling_driver = ACPI_CPUFREQ; ...
> 
> Seems we still cannot get rid of these strncmp()s. Is this acceptable, 
> or should we change "struct cpufreq_driver" to use enum represented 
> driver name as well, or do you have a better suggestion?

> The one I explained before: Express the data representation type in an enum, 
> not the driver kind. But even if we went with the 
> above, the strcmp() ugliness would at least be limited to the producer, not 
> enforced onto any number of consumers.

No.  I think in our previous discussion, there is no problem with "the data 
representation type", any future data representation, as long as it is in 
"uint32_t", it can use "uint32_t scaling_max_perf" to hold that value 
representation. Your concern was that the following doesn't scale well.

+if (!strncmp(p_cpufreq->scaling_driver,
+  "intel_pstate", CPUFREQ_NAME_LEN) )

So we are trying to change the driver name thing to be in enum. 

Best,
Wei



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

Re: [Xen-devel] [PATCH v6 16/18] vmx: Add some scheduler hooks for VT-d posted interrupts

[Wu, Feng]

> -Original Message-
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: Thursday, September 10, 2015 5:26 PM
> To: Wu, Feng
> Cc: Andrew Cooper; Dario Faggioli; George Dunlap; Tian, Kevin;
> xen-devel@lists.xen.org; Keir Fraser
> Subject: RE: [PATCH v6 16/18] vmx: Add some scheduler hooks for VT-d posted
> interrupts
> 
> >>> On 10.09.15 at 10:59,  wrote:
> >> From: Jan Beulich [mailto:jbeul...@suse.com]
> >> Sent: Thursday, September 10, 2015 4:28 PM
> >> >>> On 10.09.15 at 04:07,  wrote:
> >> >> From: Jan Beulich [mailto:jbeul...@suse.com]
> >> >> Sent: Wednesday, September 09, 2015 6:27 PM
> >> >> >>> On 09.09.15 at 10:56,  wrote:
> >> >> >> From: Jan Beulich [mailto:jbeul...@suse.com]
> >> >> >> Sent: Monday, September 07, 2015 8:55 PM
> >> >> >> >>> On 25.08.15 at 03:57,  wrote:
> >> >> >> > +
> >> >> >> > +/*
> >> >> >> > + * Delete the vCPU from the related block list
> >> >> >> > + * if we are resuming from blocked state.
> >> >> >> > + */
> >> >> >> > +if ( v->arch.hvm_vmx.pi_block_cpu != -1 )
> >> >> >> > +{
> >> >> >> > +spin_lock_irqsave(_cpu(pi_blocked_vcpu_lock,
> >> >> >> > +
> v->arch.hvm_vmx.pi_block_cpu),
> >> >> >> flags);
> >> >> >> > +
> list_del_init(>arch.hvm_vmx.pi_blocked_vcpu_list);
> >> >> >>
> >> >> >> Shouldn't you set .pi_block_cpu back to -1 along with removing
> >> >> >> the vCPU from the list? Which then ought to allow using just
> >> >> >> list_del() here?
> >> >> >
> >> >> > Here is the story about using list_del_init() instead of list_del(), 
> >> >> > (the
> >> >> > same reason for using list_del_init() in pi_wakeup_interrupt() ).
> >> >> > If we use list_del(), that means we need to set .pi_block_cpu to
> >> >> > -1 after removing the vCPU from the block list, there are two
> >> >> > places where the vCPU is removed from the list:
> >> >> > - arch_vcpu_wake_prepare()
> >> >> > - pi_wakeup_interrupt()
> >> >> >
> >> >> > That means we also need to set .pi_block_cpu to -1 in
> >> >> > pi_wakeup_interrupt(), which seems problematic to me, since
> >> >> > .pi_block_cpu is used to get the per-CPU spin lock, it is not safe
> >> >> > to change it in pi_wakeup_interrupt(), maybe someone is using
> >> >> > it at the same time.
> >> >> >
> >> >> > And the check "if ( v->arch.hvm_vmx.pi_block_cpu != -1 )" here
> >> >> > is only used when .pi_block_cpu is set to -1 at the initial time.
> >> >> >
> >> >> > If you have any good suggestions about this, I will be all ears.
> >> >>
> >> >> Latching pi_block_cpu into a local variable would take care of that
> >> >> part of the problem. Of course you then may also need to check
> >> >> that while waiting to acquire the lock pi_block_cpu didn't change.
> >> >
> >> > Thanks for the suggestion! But I think it is not easy to "check
> >> > "that while waiting to acquire the lock pi_block_cpu didn't change".
> >> > Let's take the following pseudo code as an example:
> >> >
> >> > int local_pi_block_cpu = v->arch.hvm_vmx.pi_block_cpu;
> >> >
> >> > ..
> >> >
> >> > spin_lock_irqsave(_cpu(pi_blocked_vcpu_lock,
> >> >local_pi_block_cpu), flags);
> >> > list_del(>arch.hvm_vmx.pi_blocked_vcpu_list);
> >> > spin_unlock_irqrestore(_cpu(pi_blocked_vcpu_lock,
> >> >local_pi_block_cpu), flags);
> >> >
> >> > If .pi_block_cpu is changed to -1 by others during calling list_del()
> >> > above, that means the vCPU is removed by others, then calling
> >> > list_del() here again would be problematic. I think there might be
> >> > other corner cases for this. So I suggest adding some comments
> >> > for list_del_init() as you mentioned below.
> >>
> >> That's why I said "check that while waiting to acquire the lock
> >> pi_block_cpu didn't change." The code you present does not do
> >> this.
> >
> > I didn't see we need implement it as the above code, I just list it
> > here the show it is hard to do that.
> > First, how to check it while waiting to acquire the lock .pi_block_cpu
> > didn't change?
> 
> Note the difference between "check while waiting" and "check that
> while waiting": The former is indeed hard to implement, while the
> latter is pretty straightforward (and we do so elsewhere).
> 
> > Secondly, even if we can check it, what should we do if .pi_block_cpu
> > is changed after acquiring the lock as I mentioned above?
> 
> Drop the lock and start over. I.e. (taking your pseudo code)
> 
> restart:
> local_pi_block_cpu = ...;
> bail-if-invalid (e.g. -1 in current model)
> spin_lock_irqsave(_cpu(, local_pi_block_cpu), flags);
> if(local_pi_block_cpu != actual_pi_block_cpu) {
> spin_unlock_irqrestore(_cpu(,local_pi_block_cpu), flags);
> goto restart;
> }

Thanks a lot for showing me this pseudo code! My concern is if
.pi_block_vcpu is changed to -1 at this point, it doesn't work.
.pi_block_vcpu being -1 here means the vCPU is 

Re: [Xen-devel] [PATCH v6 16/18] vmx: Add some scheduler hooks for VT-d posted interrupts

[Jan Beulich]

>>> On 10.09.15 at 11:41,  wrote:
>> From: Jan Beulich [mailto:jbeul...@suse.com]
>> Sent: Thursday, September 10, 2015 5:26 PM
>> >>> On 10.09.15 at 10:59,  wrote:
>> > First, how to check it while waiting to acquire the lock .pi_block_cpu
>> > didn't change?
>> 
>> Note the difference between "check while waiting" and "check that
>> while waiting": The former is indeed hard to implement, while the
>> latter is pretty straightforward (and we do so elsewhere).
>> 
>> > Secondly, even if we can check it, what should we do if .pi_block_cpu
>> > is changed after acquiring the lock as I mentioned above?
>> 
>> Drop the lock and start over. I.e. (taking your pseudo code)
>> 
>> restart:
>> local_pi_block_cpu = ...;
>> bail-if-invalid (e.g. -1 in current model)
>> spin_lock_irqsave(_cpu(, local_pi_block_cpu), flags);
>> if(local_pi_block_cpu != actual_pi_block_cpu) {
>> spin_unlock_irqrestore(_cpu(,local_pi_block_cpu), flags);
>> goto restart;
>> }
> 
> Thanks a lot for showing me this pseudo code! My concern is if
> .pi_block_vcpu is changed to -1 at this point, it doesn't work.
> .pi_block_vcpu being -1 here means the vCPU is remove from
> the blocking list by others, then we cannot delete it again via
> list_del() here.

Did you miss the "bail-if-invalid" above?

> BTW, I cannot see performance overhead for list_del_init()
> compared to list_del().
> 
> list_del():
> static inline void list_del(struct list_head *entry)
> {
> ASSERT(entry->next->prev == entry);
> ASSERT(entry->prev->next == entry);
> __list_del(entry->prev, entry->next);
> entry->next = LIST_POISON1;
> entry->prev = LIST_POISON2;
> }
> 
> list_del_init():
> static inline void list_del_init(struct list_head *entry)
> {
> __list_del(entry->prev, entry->next);
> INIT_LIST_HEAD(entry);
> }

Well, yes, both do two stores (I forgot about the poisoning), but
arguably the poisoning could become a debug-build-only thing. I.e.
it is an implementation detail that the number of stores currently
is the same. From an abstract perspective one should still prefer
list_del() when the re-init isn't really needed. And in the specific
case here asking you to use list_del() makes sure the code ends
up not even trying the deletion when not needed.

Jan


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

Re: [Xen-devel] [PULL 0/19] xen-2015-09-08-tag

[Stefano Stabellini]

On Thu, 10 Sep 2015, Chen, Tiejun wrote:
> > > As you see this short log, "hw/pci-assign: split pci-assign.c", so this
> > > means I just extract something from the original hw/i386/kvm/pci-assign.c,
> > > and here so I just keep those original head files residing
> > > hw/i386/kvm/pci-assign.c, and I didn't introduce anything new.
> > 
> > hw/i386/kvm/pci-assign.c is only built if configure set CONFIG_KVM,
> > which it won't do on Windows or OSX builds.
> > 
> > It sounds like your patch has incorrectly moved code out of files
> > which are compiled only if KVM is present, or only if we're doing
> > Xen PCI passthrough, and into compiled-for-everything files.
> > 
> 
> Yes, we want to share this chunk of codes between Xen and Kvm. Just to this
> error, could we remove #include ? As I mentioned I still can compile
> this file without this head file.

And #include , but it does seem to still work on Linux after
removing them

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

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

[Stefano Stabellini]

On Thu, 10 Sep 2015, Mark Rutland wrote:
> Hi,
> 
> I'm not necessarily opposed to the renaming, but I think that this is
> the least important thing to standardize for this to work.
> 
> On Thu, Sep 10, 2015 at 09:41:56AM +0100, Shannon Zhao wrote:
> > From: Shannon Zhao 
> > 
> > These EFI stub parameters are used to internal communication between EFI
> > stub and Linux kernel and EFI stub creates these parameters. But for Xen
> > on ARM when booting with UEFI, Xen will create a minimal DT providing
> > these parameters for Dom0 and Dom0 is not only Linux kernel, but also
> > other OS (such as FreeBSD) which will be used in the future.
> 
> Currently the Linux EFI stub and kernel have a symbiotic relationship,
> because they're intimately coupled and we don't have kexec (yet) on EFI
> platforms to loosen that coupling.
> 
> If an agent other than the (kernel-specific) stub is going to provide
> this to the kernel, then we need more specified than just the property
> names.
> 
> That at least includes the following:
> 
> * The state of boot services (we currently have the EFI stub call
>   ExitBootServices(), and I believe this is crucial to the plan for
>   kexec).
> 
> * The state of the address map (we currently have the EFI stub call
>   SetVirtualAddressMap()).
> 
> * The virtual address range(s) that SetVirtualAddressMap() may map
>   elements to (this logic is currently in the EFI stub, and this matches
>   the expectations of the kernel that it is tied to).
> 
> > So here we plan to standardize the names by dropping the prefix
> > "linux," and make them common to other OS. Also this will not break
> > the compatibility since these parameters are used to internal
> > communication between EFI stub and kernel.
> 
> For the moment this is true, but will not be once we have kexec, so
> there's a dependency (or anti-dependency) there.
> 
> > Signed-off-by: Shannon Zhao 
> > ---
> > Look at [1] for the discussion about this in Xen ML. The purpose of this
> > patch is to standardize the names to make Linux ARM kernel work on Xen
> > with UEFI. Also it hopes other OS(e.g. FreeBSD), which will be used as
> > Dom0 on Xen, could support this mechanism as well.
> > 
> > [1]http://lists.xenproject.org/archives/html/xen-devel/2015-08/msg02250.html
> 
> Per this post, it looks like to pass a DTB to the kernel Xen already
> needs to know it's a Linux kernel...
> 
> I wasn't aware that there was a common standard for arm(64) kernels
> other than a PE/COFF EFI application.
> 
> Does Xen not talk to EFI itself and/or give the kernel a virtual EFI
> interface?

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

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

Re: [Xen-devel] [PATCH v4 10/11] x86/intel_pstate: support the use of intel_pstate in pmstat.c

[Jan Beulich]

>>> On 10.09.15 at 12:10,  wrote:
> Ok. If we add this "enum meaning_of_data", the xen_get_cpufreq_para will 
> exceed 128Byte, which cannot even pass the compilation. I am not sure how to 
> deal with this nicely. Do you have a suggestion? 

Currently afaict the structure occupies 14 8-byte slots, with two
unused 32-bit padding fields. Are you saying that the additions
you need to make don't fit in two 8-byte plus two 4-byte slots?
And if so, the first thing to recover space from would be
"turbo_enabled", which iirc is a boolean and hence one bit would
suffice to represent it. 31 bits then would surely be enough to
store your (two value) enum into?

Jan


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

Re: [Xen-devel] [v2][PATCH] xen/vtd/iommu: permit group devices to passthrough in relaxed mode

[Jan Beulich]

>>> On 10.09.15 at 07:23,  wrote:
>>  From: Jan Beulich [mailto:jbeul...@suse.com]
>> Sent: Wednesday, September 09, 2015 2:55 PM
>> 
>> >>> On 09.09.15 at 03:59,  wrote:
>> > @@ -2310,12 +2312,16 @@ static int intel_iommu_assign_device(
>> >   PCI_DEVFN2(bdf) == devfn &&
>> >   rmrr->scope.devices_cnt > 1 )
>> >  {
>> > -printk(XENLOG_G_ERR VTDPREFIX
>> > -   " cannot assign %04x:%02x:%02x.%u"
>> > +bool_t relaxed = !!(flag & XEN_DOMCTL_DEV_RDM_RELAXED);
>> > +
>> > +printk(XENLOG_G_WARNING VTDPREFIX
>> 
>> Well, I can live with this always being a warning, but it's not what I
>> had asked for. The VT-d maintainers will have to judge.
>> 
> 
> Need to have separate warning/error level for relax/strict.
> 
> However I don't think this patch is a right fix. So far relax/strict policy
> is per-domain. what about one VM specifies relax while another VM
> specifies strict when each is assigned with a device sharing rmrr
> with the other? In that case it becomes a system-wide security hole.

The one specifying "strict" won't gets its device assigned (due to
the code above, taking the path that was there already without
the patch), so I don't see the security issue.

Jan


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

[Xen-devel] [linux-3.18 bisection] complete test-amd64-amd64-xl-pvh-intel

[osstest service owner]

branch xen-unstable
xen branch xen-unstable
job test-amd64-amd64-xl-pvh-intel
test guest-start

Tree: linux 
git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git
Tree: linuxfirmware git://xenbits.xen.org/osstest/linux-firmware.git
Tree: qemu git://xenbits.xen.org/staging/qemu-xen-unstable.git
Tree: qemuu git://xenbits.xen.org/staging/qemu-upstream-unstable.git
Tree: xen git://xenbits.xen.org/xen.git

*** Found and reproduced problem changeset ***

  Bug is in tree:  linux 
git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git
  Bug introduced:  63753fac67e11fb6bac3d5c6a48bd319d9e612c2
  Bug not present: 8ca6fadf220ab0a1369a667462d8d8c0c5d6ac29


  commit 63753fac67e11fb6bac3d5c6a48bd319d9e612c2
  Author: Andy Lutomirski 
  Date:   Fri Oct 24 15:58:08 2014 -0700
  
  x86: Store a per-cpu shadow copy of CR4
  
  [ Upstream commit 1e02ce4cccdcb9688386e5b8d2c9fa4660b45389 ]
  
  Context switches and TLB flushes can change individual bits of CR4.
  CR4 reads take several cycles, so store a shadow copy of CR4 in a
  per-cpu variable.
  
  To avoid wasting a cache line, I added the CR4 shadow to
  cpu_tlbstate, which is already touched in switch_mm.  The heaviest
  users of the cr4 shadow will be switch_mm and __switch_to_xtra, and
  __switch_to_xtra is called shortly after switch_mm during context
  switch, so the cacheline is likely to be hot.
  
  Signed-off-by: Andy Lutomirski 
  Reviewed-by: Thomas Gleixner 
  Signed-off-by: Peter Zijlstra (Intel) 
  Cc: Kees Cook 
  Cc: Andrea Arcangeli 
  Cc: Vince Weaver 
  Cc: "hillf.zj" 
  Cc: Valdis Kletnieks 
  Cc: Paul Mackerras 
  Cc: Arnaldo Carvalho de Melo 
  Cc: Linus Torvalds 
  Link: 
http://lkml.kernel.org/r/3a54dd3353fffbf84804398e00dfdc5b7c1afd7d.1414190806.git.l...@amacapital.net
  Signed-off-by: Ingo Molnar 
  Signed-off-by: Sasha Levin 


For bisection revision-tuple graph see:
   
http://logs.test-lab.xenproject.org/osstest/results/bisect/linux-3.18/test-amd64-amd64-xl-pvh-intel.guest-start.html
Revision IDs in each graph node refer, respectively, to the Trees above.


Running cs-bisection-step 
--graph-out=/home/logs/results/bisect/linux-3.18/test-amd64-amd64-xl-pvh-intel.guest-start
 --summary-out=tmp/61740.bisection-summary --basis-template=58581 
--blessings=real,real-bisect linux-3.18 test-amd64-amd64-xl-pvh-intel 
guest-start
Searching for failure / basis pass:
 61524 fail [host=huxelrebe0] / 58558 ok.
Failure / basis pass flights: 61524 / 58558
(tree with no url: ovmf)
(tree with no url: seabios)
Tree: linux 
git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git
Tree: linuxfirmware git://xenbits.xen.org/osstest/linux-firmware.git
Tree: qemu git://xenbits.xen.org/staging/qemu-xen-unstable.git
Tree: qemuu git://xenbits.xen.org/staging/qemu-upstream-unstable.git
Tree: xen git://xenbits.xen.org/xen.git
Latest fcd9bfdb9d884f1aab89124dee894e7d821bb5dc 
c530a75c1e6a472b0eb9558310b518f0dfcd8860 
5cdde31eacdd288359746019ad05cac8ed5d9f70 
b05befcbea71a979509ce04f02929969a790c923 
801ab48e5556cb54f67e3cb57f077f47e8663ced
Basis pass d048c068d00da7d4cfa5ea7651933b99026958cf 
c530a75c1e6a472b0eb9558310b518f0dfcd8860 
38609ae72b0a9e09b42be94f469fef928a1049fa 
579e90432e995d6cb6f8520aca557fa6646f94ec 
12e817e281034f5881f46e0e4f1d127820101a78
Generating revisions with ./adhoc-revtuple-generator  
git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git#d048c068d00da7d4cfa5ea7651933b99026958cf-fcd9bfdb9d884f1aab89124dee894e7d821bb5dc
 
git://xenbits.xen.org/osstest/linux-firmware.git#c530a75c1e6a472b0eb9558310b518f0dfcd8860-c530a75c1e6a472b0eb9558310b518f0dfcd8860
 
git://xenbits.xen.org/staging/qemu-xen-unstable.git#38609ae72b0a9e09b42be94f469fef928a1049fa-5cdde31eacdd288359746019ad05cac8ed5d9f70
 
git://xenbits.xen.org/staging/qemu-upstream-unstable.git#579e90432e995d6cb6f8520aca557fa6646f94ec-b05befcbea71a979509ce04f02929969a790c923
 
git://xenbits.xen.org/xen.git#12e817e281034f5881f46e0e4f1d127820101a78-801ab48e5556cb54f67e3cb57f077f47e8663ced
Loaded 19677 nodes in revision graph
Searching for test results:
 58558 pass d048c068d00da7d4cfa5ea7651933b99026958cf 
c530a75c1e6a472b0eb9558310b518f0dfcd8860 
38609ae72b0a9e09b42be94f469fef928a1049fa 
579e90432e995d6cb6f8520aca557fa6646f94ec 
12e817e281034f5881f46e0e4f1d127820101a78
 58987 fail ea5dd38e93b3bec3427e5d3eef000bbf5d637e76 
c530a75c1e6a472b0eb9558310b518f0dfcd8860 
3e2e51ecc1120bd59537ed19b6bc7066511c7e2e 
c4a962ec0c61aa9b860a3635c8424472e6c2cc2c 
c40317f11b3f05e7c06a2213560c8471081f2662
 58976 

[Xen-devel] [PULL 2/7] pc: Remove redundant arguments from xen_hvm_init()

[Michael S. Tsirkin]

From: Eduardo Habkost 

Remove arguments that can be found in PCMachineState.

Signed-off-by: Eduardo Habkost 
Reviewed-by: Michael S. Tsirkin 
Signed-off-by: Michael S. Tsirkin 
---
 include/hw/xen/xen.h |  4 ++--
 hw/i386/pc_piix.c|  4 +---
 hw/i386/pc_q35.c |  4 +---
 xen-hvm-stub.c   |  3 +--
 xen-hvm.c| 25 -
 5 files changed, 17 insertions(+), 23 deletions(-)

diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
index 4356af4..e90931a 100644
--- a/include/hw/xen/xen.h
+++ b/include/hw/xen/xen.h
@@ -10,6 +10,7 @@
 
 #include "hw/irq.h"
 #include "qemu-common.h"
+#include "qemu/typedefs.h"
 
 /* xen-machine.c */
 enum xen_mode {
@@ -38,8 +39,7 @@ qemu_irq *xen_interrupt_controller_init(void);
 void xenstore_store_pv_console_info(int i, struct CharDriverState *chr);
 
 #if defined(NEED_CPU_H) && !defined(CONFIG_USER_ONLY)
-int xen_hvm_init(ram_addr_t *below_4g_mem_size, ram_addr_t *above_4g_mem_size,
- MemoryRegion **ram_memory);
+int xen_hvm_init(PCMachineState *pcms, MemoryRegion **ram_memory);
 void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size,
struct MemoryRegion *mr);
 void xen_modified_memory(ram_addr_t start, ram_addr_t length);
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index b82921d..117f8dc 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -134,9 +134,7 @@ static void pc_init1(MachineState *machine)
 pcms->below_4g_mem_size = machine->ram_size;
 }
 
-if (xen_enabled() && xen_hvm_init(>below_4g_mem_size,
-  >above_4g_mem_size,
-  _memory) != 0) {
+if (xen_enabled() && xen_hvm_init(pcms, _memory) != 0) {
 fprintf(stderr, "xen hardware virtual machine initialisation 
failed\n");
 exit(1);
 }
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 7217cbf..4b38dee 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -125,9 +125,7 @@ static void pc_q35_init(MachineState *machine)
 pcms->below_4g_mem_size = machine->ram_size;
 }
 
-if (xen_enabled() && xen_hvm_init(>below_4g_mem_size,
-  >above_4g_mem_size,
-  _memory) != 0) {
+if (xen_enabled() && xen_hvm_init(pcms, _memory) != 0) {
 fprintf(stderr, "xen hardware virtual machine initialisation 
failed\n");
 exit(1);
 }
diff --git a/xen-hvm-stub.c b/xen-hvm-stub.c
index 46867d8..6a39425 100644
--- a/xen-hvm-stub.c
+++ b/xen-hvm-stub.c
@@ -47,8 +47,7 @@ void xen_modified_memory(ram_addr_t start, ram_addr_t length)
 {
 }
 
-int xen_hvm_init(ram_addr_t *below_4g_mem_size, ram_addr_t *above_4g_mem_size,
- MemoryRegion **ram_memory)
+int xen_hvm_init(PCMachineState *pcms, MemoryRegion **ram_memory)
 {
 return 0;
 }
diff --git a/xen-hvm.c b/xen-hvm.c
index 0408462..55bce3a 100644
--- a/xen-hvm.c
+++ b/xen-hvm.c
@@ -180,8 +180,7 @@ qemu_irq *xen_interrupt_controller_init(void)
 
 /* Memory Ops */
 
-static void xen_ram_init(ram_addr_t *below_4g_mem_size,
- ram_addr_t *above_4g_mem_size,
+static void xen_ram_init(PCMachineState *pcms,
  ram_addr_t ram_size, MemoryRegion **ram_memory_p)
 {
 MemoryRegion *sysmem = get_system_memory();
@@ -198,20 +197,20 @@ static void xen_ram_init(ram_addr_t *below_4g_mem_size,
 }
 
 if (ram_size >= user_lowmem) {
-*above_4g_mem_size = ram_size - user_lowmem;
-*below_4g_mem_size = user_lowmem;
+pcms->above_4g_mem_size = ram_size - user_lowmem;
+pcms->below_4g_mem_size = user_lowmem;
 } else {
-*above_4g_mem_size = 0;
-*below_4g_mem_size = ram_size;
+pcms->above_4g_mem_size = 0;
+pcms->below_4g_mem_size = ram_size;
 }
-if (!*above_4g_mem_size) {
+if (!pcms->above_4g_mem_size) {
 block_len = ram_size;
 } else {
 /*
  * Xen does not allocate the memory continuously, it keeps a
  * hole of the size computed above or passed in.
  */
-block_len = (1ULL << 32) + *above_4g_mem_size;
+block_len = (1ULL << 32) + pcms->above_4g_mem_size;
 }
 memory_region_init_ram(_memory, NULL, "xen.ram", block_len,
_abort);
@@ -229,12 +228,12 @@ static void xen_ram_init(ram_addr_t *below_4g_mem_size,
  */
 memory_region_init_alias(_lo, NULL, "xen.ram.lo",
  _memory, 0xc,
- *below_4g_mem_size - 0xc);
+ pcms->below_4g_mem_size - 0xc);
 memory_region_add_subregion(sysmem, 0xc, _lo);
-if (*above_4g_mem_size > 0) {
+if (pcms->above_4g_mem_size > 0) {
 memory_region_init_alias(_hi, NULL, "xen.ram.hi",
  _memory, 0x1ULL,
- 

[Xen-devel] [GIT PULL] xen: MFN/GFN/BFN terminology changes for 4.3-rc0

[David Vrabel]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

Linus,

Please git pull the following tag:

 git://git.kernel.org/pub/scm/linux/kernel/git/xen/tip.git 
for-linus-4.3-rc0b-tag

xen: MFN/GFN/BFN terminology changes for 4.3-rc0

- - Use the correct GFN/BFN terms more consistently.

Thanks.

David

 arch/arm/include/asm/xen/page.h | 28 +++--
 arch/arm/xen/enlighten.c| 18 +++---
 arch/arm/xen/mm.c   |  4 +--
 arch/x86/include/asm/xen/page.h | 39 +++--
 arch/x86/xen/mmu.c  | 32 
 arch/x86/xen/smp.c  |  2 +-
 drivers/block/xen-blkfront.c|  6 ++---
 drivers/input/misc/xen-kbdfront.c   |  4 +--
 drivers/net/xen-netback/netback.c   |  4 +--
 drivers/net/xen-netfront.c  | 12 +
 drivers/scsi/xen-scsifront.c| 10 
 drivers/tty/hvc/hvc_xen.c   | 18 ++
 drivers/video/fbdev/xen-fbfront.c   | 20 +++
 drivers/xen/balloon.c   |  2 +-
 drivers/xen/biomerge.c  |  6 ++---
 drivers/xen/events/events_base.c|  2 +-
 drivers/xen/events/events_fifo.c|  4 +--
 drivers/xen/gntalloc.c  |  3 ++-
 drivers/xen/manage.c|  2 +-
 drivers/xen/privcmd.c   | 44 -
 drivers/xen/swiotlb-xen.c   | 16 ++--
 drivers/xen/tmem.c  | 24 ++
 drivers/xen/xenbus/xenbus_client.c  |  2 +-
 drivers/xen/xenbus/xenbus_dev_backend.c |  2 +-
 drivers/xen/xenbus/xenbus_probe.c   | 16 ++--
 drivers/xen/xlate_mmu.c | 18 +++---
 include/uapi/xen/privcmd.h  |  4 +++
 include/xen/page.h  |  4 +--
 include/xen/xen-ops.h   | 10 
 29 files changed, 198 insertions(+), 158 deletions(-)

Julien Grall (8):
  xen: Make clear that swiotlb and biomerge are dealing with DMA address
  arm/xen: implement correctly pfn_to_mfn
  xen: Use correctly the Xen memory terminologies
  xen/tmem: Use xen_page_to_gfn rather than pfn_to_gfn
  video/xen-fbfront: Further s/MFN/GFN clean-up
  hvc/xen: Further s/MFN/GFN clean-up
  xen/privcmd: Further s/MFN/GFN/ clean-up
  xen/xenbus: Rename the variable xen_store_mfn to xen_store_gfn
-BEGIN PGP SIGNATURE-
Version: GnuPG v1

iQEcBAEBAgAGBQJV8VdlAAoJEFxbo/MsZsTRVfAH/jP2E4h+NiNwyrEOc0FIFnNb
DTQW2SSpy49tLBP1AZwBgalCr3sdy3Obws8zn4PwaWqxBsjiFG+BUVp7uIxVS9pg
KjxhZzwvPWVGKczT+cKp+bqY8RRukLx8PezDQ3lZmHu3KHjowMMJmp6aboVG2TZF
RROY0yWReA6rKNQi610PEEmAaWlkRFph6SX2dp9KnnI29oPIlJcbEmTBwvc1G0pz
imEIXJGOvVD5Soikzgh/s1mLGnyMq2ZqYrOFjYagvvR7w6v0ZJqQyNKEhdsM3ITK
bVcGZwIAupG130RdqQPx8laPBfiGO/Bw4sXdKwCWB0lV/iIX09vTLpC9fF1kTO4=
=XDTK
-END PGP SIGNATURE-

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

Re: [Xen-devel] [PATCH v6 16/18] vmx: Add some scheduler hooks for VT-d posted interrupts

[Jan Beulich]

>>> On 10.09.15 at 04:07,  wrote:
>> From: Jan Beulich [mailto:jbeul...@suse.com]
>> Sent: Wednesday, September 09, 2015 6:27 PM
>> >>> On 09.09.15 at 10:56,  wrote:
>> >> From: Jan Beulich [mailto:jbeul...@suse.com]
>> >> Sent: Monday, September 07, 2015 8:55 PM
>> >> >>> On 25.08.15 at 03:57,  wrote:
>> >> > +
>> >> > +/*
>> >> > + * Delete the vCPU from the related block list
>> >> > + * if we are resuming from blocked state.
>> >> > + */
>> >> > +if ( v->arch.hvm_vmx.pi_block_cpu != -1 )
>> >> > +{
>> >> > +spin_lock_irqsave(_cpu(pi_blocked_vcpu_lock,
>> >> > +  v->arch.hvm_vmx.pi_block_cpu),
>> >> flags);
>> >> > +list_del_init(>arch.hvm_vmx.pi_blocked_vcpu_list);
>> >>
>> >> Shouldn't you set .pi_block_cpu back to -1 along with removing
>> >> the vCPU from the list? Which then ought to allow using just
>> >> list_del() here?
>> >
>> > Here is the story about using list_del_init() instead of list_del(), (the
>> > same reason for using list_del_init() in pi_wakeup_interrupt() ).
>> > If we use list_del(), that means we need to set .pi_block_cpu to
>> > -1 after removing the vCPU from the block list, there are two
>> > places where the vCPU is removed from the list:
>> > - arch_vcpu_wake_prepare()
>> > - pi_wakeup_interrupt()
>> >
>> > That means we also need to set .pi_block_cpu to -1 in
>> > pi_wakeup_interrupt(), which seems problematic to me, since
>> > .pi_block_cpu is used to get the per-CPU spin lock, it is not safe
>> > to change it in pi_wakeup_interrupt(), maybe someone is using
>> > it at the same time.
>> >
>> > And the check "if ( v->arch.hvm_vmx.pi_block_cpu != -1 )" here
>> > is only used when .pi_block_cpu is set to -1 at the initial time.
>> >
>> > If you have any good suggestions about this, I will be all ears.
>> 
>> Latching pi_block_cpu into a local variable would take care of that
>> part of the problem. Of course you then may also need to check
>> that while waiting to acquire the lock pi_block_cpu didn't change.
> 
> Thanks for the suggestion! But I think it is not easy to "check
> "that while waiting to acquire the lock pi_block_cpu didn't change".
> Let's take the following pseudo code as an example:
> 
> int local_pi_block_cpu = v->arch.hvm_vmx.pi_block_cpu;
> 
> ..
> 
> spin_lock_irqsave(_cpu(pi_blocked_vcpu_lock,
>local_pi_block_cpu), flags);
> list_del(>arch.hvm_vmx.pi_blocked_vcpu_list);
> spin_unlock_irqrestore(_cpu(pi_blocked_vcpu_lock,
>local_pi_block_cpu), flags);
> 
> If .pi_block_cpu is changed to -1 by others during calling list_del()
> above, that means the vCPU is removed by others, then calling
> list_del() here again would be problematic. I think there might be
> other corner cases for this. So I suggest adding some comments
> for list_del_init() as you mentioned below.

That's why I said "check that while waiting to acquire the lock
pi_block_cpu didn't change." The code you present does not do
this.

>> >> > +do {
>> >> > +old.control = new.control = pi_desc->control;
>> >> > +
>> >> > +/* Should not block the vCPU if an interrupt was posted for
>> it.
>> >> */
>> >> > +if ( pi_test_on() )
>> >> > +{
>> >> > +spin_unlock_irqrestore(>arch.hvm_vmx.pi_lock,
>> >> gflags);
>> >> > +vcpu_unblock(v);
>> >>
>> >> Calling vcpu_unblock() in the middle of context_switch()? Why? And
>> >> is this safe?
>> >
>> > I cannot see anything unsafe so far, can some scheduler maintainer
>> > help to confirm it? Dario? George?
>> 
>> But first and foremost you ought to answer the "why".
> 
> I think if you think this solution is unsafe, you need point out where it 
> is, not
> just ask "is this safe ?", I don't think this is an effective comments.

It is my general understanding that people wanting code to be
included have to prove their code is okay, not reviewers to prove
the code is broken.

> Anyway, I go through the main path of the code as below, I really don't find
> anything unsafe here.
> 
> context_switch() -->
> pi_ctxt_switch_from() -->
> vmx_pre_ctx_switch_pi() -->
> vcpu_unblock() -->
> vcpu_wake() -->
> SCHED_OP(VCPU2OP(v), wake, v) -->
> csched_vcpu_wake() -->
> __runq_insert()
> __runq_tickle()
> 
> If you continue to think it is unsafe, pointing out the place will be 
> welcome!

Once again - I didn't say it's unsafe (and hence can't point at a
particular place). What bothers me is that vcpu_unblock() affects
scheduler state, and altering scheduler state from inside the
context switch path looks problematic at the first glance. I'd be
happy if I was told there is no such problem, but be aware that
this would have to include latent ones: Even if 

Re: [Xen-devel] [PATCH v4 10/11] x86/intel_pstate: support the use of intel_pstate in pmstat.c

[Jan Beulich]

>>> On 10.09.15 at 11:33,  wrote:
> On 09/09/2015 16:17,  Jan Beulich wrote:
 On 10.09.15 at 07:35,  wrote:
>> Seems we still cannot get rid of these strncmp()s. Is this acceptable, 
>> or should we change "struct cpufreq_driver" to use enum represented 
>> driver name as well, or do you have a better suggestion?
> 
>> The one I explained before: Express the data representation type in an enum, 
> not the driver kind. But even if we went with the 
>> above, the strcmp() ugliness would at least be limited to the producer, not 
> enforced onto any number of consumers.
> 
> No.  I think in our previous discussion, there is no problem with "the data 
> representation type", any future data representation, as long as it is in 
> "uint32_t", it can use "uint32_t scaling_max_perf" to hold that value 
> representation.

Okay, "representation" was a badly chose term. "Meaning of the data"
would have been better.

> Your concern was that the following doesn't scale well.
> 
> +if (!strncmp(p_cpufreq->scaling_driver,
> +  "intel_pstate", CPUFREQ_NAME_LEN) )
> 
> So we are trying to change the driver name thing to be in enum. 

No, that was never what I've been asking for. I've always said
that the consumer of the data ought to have a way to know what
kind of data it is dealing with. I.e. the enum ought to represent
what meaning the data has (frequency vs percentage).

Jan


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

Re: [Xen-devel] [edk2] EDK II & GPL - Re: OVMF BoF @ KVM Forum 2015

[Laszlo Ersek]

On 09/10/15 08:19, Alexander Graf wrote:
> 
> 
>> Am 10.09.2015 um 07:32 schrieb Jordan Justen :

>> Laszlo's email raised the GPL question, but I was not sure what the
>> EDK II community would accept with regards to GPL. Thus ... I asked. I
>> guess I'm getting a better idea with regards to Apple and HP. :)
>>
>> In your opinion, would we be able to discuss patches for a *separate*
>> repo with GplDriverPkg on edk2-devel?
> 
> In fact, could we just make the non-free FAT source and GPL FAT
> source both be git submodules?

We've discussed submodules in the past (for other purposes). The
consensus seemed to be that most people dislike them (me included).

UEFI drivers are supposed to be modular / well separable (for one, they
can be shipped by third parties in binary-only form; which was a design
goal of UEFI). And specifically in the FAT driver's case, the source
doesn't even live inside the main repo at the moment, so turning it into
a source submodule might not be a step back.

But... I just don't like it. We should be moving towards a grand unified
repo, where cross-module changes and dependencies are possible to
implement with carefully segmented patch sets. The FAT driver's source
lives outside for non-technical reasons. Rather than codifying that
situation forever with a git submodule, I'd prefer some solution that
leaves us with a standalone repo.

I think I'm fuzzy on the details of the earlier git-submodule
discussion. In any case here's the link (I hope this is the right one):

http://thread.gmane.org/gmane.comp.bios.tianocore.devel/15168

> Then whoever clones the repo can get
> the license flavor he's least scared about.

I think for many companies it is important that a developer of theirs
who is "blissfully ignorant" of licensing questions simply *cannot* make
a mistake (eg. by copying code from the "wrong" directory, or by using
the "wrong" submodule). It should be foolproof.

> Or alternatively instead
> of pulling in a GPL licensed FAT driver we use a BSD licensed one.
> I'm sure someone has one of those too ;).

I'm not sure at all. Do you have a pointer? :)

> Also for the record, the FAT driver problem is that the source
> license states that it can not be used in certain circumstances,
> which by common interpretation makes it non-free.

I agree. The restriction on use conflicts both the FSF's defintion of
Free Software, and the OSI's definition of Open Source Software.

> The fact that there
> is a binary or source doesn't matter fwiw.

Right.

Laszlo

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

[Xen-devel] [PATCH net v3 0/2] xen-net{front, back}: respect user provided max_queues

[Wei Liu]

Wei Liu (2):
  xen-netback: respect user provided max_queues
  xen-netfront: respect user provided max_queues

 drivers/net/xen-netback/netback.c | 7 +--
 drivers/net/xen-netfront.c| 7 +--
 2 files changed, 10 insertions(+), 4 deletions(-)

-- 
2.1.4


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

[Xen-devel] [PATCH net v3 2/2] xen-netfront: respect user provided max_queues

[Wei Liu]

Originally that parameter was always reset to num_online_cpus during
module initialisation, which renders it useless.

The fix is to only set max_queues to num_online_cpus when user has not
provided a value.

Signed-off-by: Wei Liu 
Cc: David Vrabel 
---
v3: fix copy-n-paste error to make netfront compile
---
 drivers/net/xen-netfront.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index e27e6d2..b9c637a 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -2132,8 +2132,11 @@ static int __init netif_init(void)
 
pr_info("Initialising Xen virtual ethernet driver\n");
 
-   /* Allow as many queues as there are CPUs, by default */
-   xennet_max_queues = num_online_cpus();
+   /* Allow as many queues as there are CPUs if user has not
+* specified a value.
+*/
+   if (xennet_max_queues == 0)
+   xennet_max_queues = num_online_cpus();
 
return xenbus_register_frontend(_driver);
 }
-- 
2.1.4


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

Re: [Xen-devel] [PULL 0/19] xen-2015-09-08-tag

[Stefano Stabellini]

CC Michael

On Thu, 10 Sep 2015, Stefano Stabellini wrote:
> On Thu, 10 Sep 2015, Chen, Tiejun wrote:
> > > xen-host-pci-device.c is only compiled if CONFIG_XEN_PCI_PASSTHROUGH
> > > was set by configure. That won't be the case on OSX or Windows, where
> > > the Xen headers don't exist.
> > > 
> > 
> > Okay. This actually shouldn't be enabled on Windows so what about this?
> 
> I think it would be nicer to replace the pread than introducing ifdefs.

Something like:

---
Replace pread with read to avoid build breakages on Windows

Signed-off-by: Stefano Stabellini 

diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index 58a33fb..9a40429 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -774,8 +774,11 @@ static int host_pci_config_read(int pos, int len, uint32_t 
val)
 return -ENODEV;
 }
 
+if (lseek(config_fd, pos, SEEK_SET) != pos) {
+return -errno;
+}
 do {
-rc = pread(config_fd, (uint8_t *), len, pos);
+rc = read(config_fd, (uint8_t *), len);
 } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
 if (rc != len) {
 return -errno;

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

Re: [Xen-devel] [PATCH for 4.6] configure: don't silently disable systemd support

[Wei Liu]

On Thu, Sep 10, 2015 at 12:04:10PM +0100, Wei Liu wrote:
> On Thu, Sep 10, 2015 at 11:54:16AM +0100, Ian Campbell wrote:
> > On Wed, 2015-09-09 at 23:35 +0100, Wei Liu wrote:
> > > Originally when user runs ./configure --enable-systemd and systemd
> > > development library is not available the build system silently disables
> > > systemd support. This is not in line with normal expectation.
> > > 
> > > Instead, configure should error out when user has asked for systemd
> > > support but development libraries can't be found.
> > > 
> > > Reported-by: George Dunlap 
> > > Signed-off-by: Wei Liu 
> > > ---
> > > Please rerun ./autogen.sh.
> > > ---
> > >  m4/systemd.m4 | 6 +-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/m4/systemd.m4 b/m4/systemd.m4
> > > index 8284993..84508a1 100644
> > > --- a/m4/systemd.m4
> > > +++ b/m4/systemd.m4
> > > @@ -85,7 +85,11 @@ AC_DEFUN([AX_CHECK_SYSTEMD], [
> > >   AC_DEFINE([HAVE_SYSTEMD], [1], [Systemd available and
> > > enabled])
> > >   systemd=y
> > >   AX_CHECK_SYSTEMD_LIBS()
> > > - ],[systemd=n])
> > > + ],[
> > > +>>   > AS_IF([test "x$enable_systemd" != "x"],
> > 
> > At this point the value of enable_systemd is either empty or ...what...? I
> > think "yes"?, with "no" having been ruled out earlier?
> > 
> > Or can it be just "y"?
> > 
> 
> At this point, it can't be no. It's either empty or "yes" (or "y"?). I

For the record, it was "yes".

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

[Xen-devel] [PATCH for 4.6 v2] configure: don't silently disable systemd support

[Wei Liu]

Originally when user runs ./configure --enable-systemd and systemd
development library is not available the build system silently disables
systemd support. This is not in line with normal expectation.

Instead, configure should error out when user has asked for systemd
support but development libraries can't be found.

Reported-by: George Dunlap 
Signed-off-by: Wei Liu 
---
v2: invert the test to check for explicit "yes" value.

Please rerun ./autogen.sh.
---
 m4/systemd.m4 | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/m4/systemd.m4 b/m4/systemd.m4
index 8284993..e4b1aa5 100644
--- a/m4/systemd.m4
+++ b/m4/systemd.m4
@@ -85,7 +85,11 @@ AC_DEFUN([AX_CHECK_SYSTEMD], [
AC_DEFINE([HAVE_SYSTEMD], [1], [Systemd available and enabled])
systemd=y
AX_CHECK_SYSTEMD_LIBS()
-   ],[systemd=n])
+   ],[
+   AS_IF([test "x$enable_systemd" = "xyes"],
+   [AC_MSG_ERROR([Unable to find systemd development 
library])],
+   [systemd=n])
+   ])
],[systemd=n])
 ])
 
-- 
2.1.4


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

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

[Mark Rutland]

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

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

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

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

Mark.

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

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

[Julien Grall]

On 10/09/15 12:32, Andrew Turner wrote:
> On Thu, 10 Sep 2015 16:41:56 +0800
> Shannon Zhao  wrote:
> 
>> From: Shannon Zhao 
>>
>> These EFI stub parameters are used to internal communication between
>> EFI stub and Linux kernel and EFI stub creates these parameters. But
>> for Xen on ARM when booting with UEFI, Xen will create a minimal DT
>> providing these parameters for Dom0 and Dom0 is not only Linux
>> kernel, but also other OS (such as FreeBSD) which will be used in the
>> future. So here we plan to standardize the names by dropping the
>> prefix "linux," and make them common to other OS. Also this will not
>> break the compatibility since these parameters are used to internal
>> communication between EFI stub and kernel.
> 
> It is unlikely FreeBSD will use this property when booting ad Xen dom0.
> The kernel expects to be passed a data structure to find boot this
> information.
> 
> My preference would be to have the FreeBSD loader load the xen binary,
> the FreeBSD kernel, and set up these data structs before passing
> control to Xen, with the information it needs to boot the kernel. My
> understanding is this is how it is done on x86.

Well, AFAICT, there is no FreeBSD specific code in Xen for x86. We are
faking a multiboot module which contains all the informations.

I know that the bootloader is at least involved, I don't remember if
there is some code in FreeBSD to read this boot module. I've CCed Roger
to confirm.

> I can see a few issues with this where, for example, the device tree or
> ACPI tables need to be modified, but these should be solvable.

Xen has to modify the firmware table in order to remove everything that
it's used by itself (UART, virtual GIC...).

So we would need to modify the FreeBSD data structure to pass the new
DT/ACPI. Does the metadata are standardize? I.e is it stable from
accross FreeBSD version?

Anyway, I'd like to avoid any FreeBSD specific code in Xen, and even any
OS specific code in Xen. It's better if we keep a common interface for
everyone.

Regards,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH v6 16/18] vmx: Add some scheduler hooks for VT-d posted interrupts

[Wu, Feng]

> -Original Message-
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: Thursday, September 10, 2015 8:45 PM
> To: Wu, Feng
> Cc: Andrew Cooper; Dario Faggioli; George Dunlap; Tian, Kevin;
> xen-devel@lists.xen.org; Keir Fraser
> Subject: RE: [PATCH v6 16/18] vmx: Add some scheduler hooks for VT-d posted
> interrupts
> 
> >>> On 10.09.15 at 14:34,  wrote:
> 
> >
> >> -Original Message-
> >> From: Jan Beulich [mailto:jbeul...@suse.com]
> >> Sent: Thursday, September 10, 2015 6:01 PM
> >> To: Wu, Feng
> >> Cc: Andrew Cooper; Dario Faggioli; George Dunlap; Tian, Kevin;
> >> xen-devel@lists.xen.org; Keir Fraser
> >> Subject: RE: [PATCH v6 16/18] vmx: Add some scheduler hooks for VT-d
> posted
> >> interrupts
> >>
> >> >>> On 10.09.15 at 11:41,  wrote:
> >> >> From: Jan Beulich [mailto:jbeul...@suse.com]
> >> >> Sent: Thursday, September 10, 2015 5:26 PM
> >> >> >>> On 10.09.15 at 10:59,  wrote:
> >> >> > First, how to check it while waiting to acquire the lock .pi_block_cpu
> >> >> > didn't change?
> >> >>
> >> >> Note the difference between "check while waiting" and "check that
> >> >> while waiting": The former is indeed hard to implement, while the
> >> >> latter is pretty straightforward (and we do so elsewhere).
> >> >>
> >> >> > Secondly, even if we can check it, what should we do if .pi_block_cpu
> >> >> > is changed after acquiring the lock as I mentioned above?
> >> >>
> >> >> Drop the lock and start over. I.e. (taking your pseudo code)
> >> >>
> >> >> restart:
> >> >> local_pi_block_cpu = ...;
> >> >> bail-if-invalid (e.g. -1 in current model)
> >> >> spin_lock_irqsave(_cpu(, local_pi_block_cpu), flags);
> >> >> if(local_pi_block_cpu != actual_pi_block_cpu) {
> >> >> spin_unlock_irqrestore(_cpu(,local_pi_block_cpu), flags);
> >> >> goto restart;
> >> >> }
> >> >
> >> > Thanks a lot for showing me this pseudo code! My concern is if
> >> > .pi_block_vcpu is changed to -1 at this point, it doesn't work.
> >> > .pi_block_vcpu being -1 here means the vCPU is remove from
> >> > the blocking list by others, then we cannot delete it again via
> >> > list_del() here.
> >>
> >> Did you miss the "bail-if-invalid" above?
> >
> > I am sorry, do I miss something here? If .pi_block_cpu becomes
> > -1 here (after the above 'if' statement is finished with
> > local_pi_block_cpu == actual_pi_block_cpu ), how can "bail-if-invalid"
> > above help?
> 
> The (obvious I thought) implication is that all assignments to
> pi_block_cpu (along with all list manipulations) now need to happen
> with the lock held.

If all the assignment to pi_block_cpu is with one lock held, I don't think
we need to above checkout, we can safely use .pi_block_cpu, right?

Thanks,
Feng

> 
> Jan


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

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

[Ian Campbell]

On Thu, 2015-09-10 at 13:15 +0100, Mark Rutland wrote:

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

Taking a step back, the reason for using the EFI stub parameters is only
(AFAIK) in order to be able to locate the ACPI RDSP (the root table
pointer), which as it happens is normally passed via one of the EFI
firmware tables.

If there was a way to achieve that goal (i.e. another way to find the RSDP)
without opening the can of UEFI worms then we could consider that opiton
too.

a way != the legacy x86 thing of scanning low memory of the signature, of
course.

Ian.

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

Re: [Xen-devel] [PULL 0/19] xen-2015-09-08-tag

[Michael S. Tsirkin]

On Thu, Sep 10, 2015 at 11:29:18AM +0100, Stefano Stabellini wrote:
> CC Michael
> 
> On Thu, 10 Sep 2015, Stefano Stabellini wrote:
> > On Thu, 10 Sep 2015, Chen, Tiejun wrote:
> > > > xen-host-pci-device.c is only compiled if CONFIG_XEN_PCI_PASSTHROUGH
> > > > was set by configure. That won't be the case on OSX or Windows, where
> > > > the Xen headers don't exist.
> > > > 
> > > 
> > > Okay. This actually shouldn't be enabled on Windows so what about this?
> > 
> > I think it would be nicer to replace the pread than introducing ifdefs.
> 
> Something like:
> 
> ---
> Replace pread with read to avoid build breakages on Windows
> 
> Signed-off-by: Stefano Stabellini 

I'd prefer a wrapper that does the right thing.
No sense in doubling the # of system calls for everyone.

> 
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index 58a33fb..9a40429 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -774,8 +774,11 @@ static int host_pci_config_read(int pos, int len, 
> uint32_t val)
>  return -ENODEV;
>  }
>  
> +if (lseek(config_fd, pos, SEEK_SET) != pos) {
> +return -errno;
> +}
>  do {
> -rc = pread(config_fd, (uint8_t *), len, pos);
> +rc = read(config_fd, (uint8_t *), len);
>  } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
>  if (rc != len) {
>  return -errno;

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

Re: [Xen-devel] [PULL 0/19] xen-2015-09-08-tag

[Stefano Stabellini]

On Thu, 10 Sep 2015, Michael S. Tsirkin wrote:
> On Thu, Sep 10, 2015 at 12:26:21PM +0100, Stefano Stabellini wrote:
> > On Thu, 10 Sep 2015, Michael S. Tsirkin wrote:
> > > On Thu, Sep 10, 2015 at 11:29:18AM +0100, Stefano Stabellini wrote:
> > > > CC Michael
> > > > 
> > > > On Thu, 10 Sep 2015, Stefano Stabellini wrote:
> > > > > On Thu, 10 Sep 2015, Chen, Tiejun wrote:
> > > > > > > xen-host-pci-device.c is only compiled if 
> > > > > > > CONFIG_XEN_PCI_PASSTHROUGH
> > > > > > > was set by configure. That won't be the case on OSX or Windows, 
> > > > > > > where
> > > > > > > the Xen headers don't exist.
> > > > > > > 
> > > > > > 
> > > > > > Okay. This actually shouldn't be enabled on Windows so what about 
> > > > > > this?
> > > > > 
> > > > > I think it would be nicer to replace the pread than introducing 
> > > > > ifdefs.
> > > > 
> > > > Something like:
> > > > 
> > > > ---
> > > > Replace pread with read to avoid build breakages on Windows
> > > > 
> > > > Signed-off-by: Stefano Stabellini 
> > > 
> > > I'd prefer a wrapper that does the right thing.
> > > No sense in doubling the # of system calls for everyone.
> > 
> > If this was done on an hot path I would agree with you, but it is just
> > one call at initialization time (igd_pt_i440fx_initfn).
> 
> I missed this fact. OK then.

Thanks! I'll fold it the offending patch
(http://marc.info/?l=qemu-devel=144174596628052=2) and resend.


> > > > diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> > > > index 58a33fb..9a40429 100644
> > > > --- a/hw/pci-host/piix.c
> > > > +++ b/hw/pci-host/piix.c
> > > > @@ -774,8 +774,11 @@ static int host_pci_config_read(int pos, int len, 
> > > > uint32_t val)
> > > >  return -ENODEV;
> > > >  }
> > > >  
> > > > +if (lseek(config_fd, pos, SEEK_SET) != pos) {
> > > > +return -errno;
> > > > +}
> > > >  do {
> > > > -rc = pread(config_fd, (uint8_t *), len, pos);
> > > > +rc = read(config_fd, (uint8_t *), len);
> > > >  } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
> > > >  if (rc != len) {
> > > >  return -errno;
> > > 
> 

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

[Xen-devel] [BUG] xenctrl.h : error with xc_error_code declaration

[Sébastien Frémal]

Hello,

I just write to signal a bug and its solution. I installed the 14.04 LTS
ubuntu version and installed the xen version through synaptic. As I'm
developping modules for Xen I also installed the xen dev package. The
installed Xen version is 4.4.2.

I tried to compile one of my C files including xenctrl.h but I got the
following errors :
In file included from ../modules/gntring/libgntring4.c:12:0:
/usr/include/xenctrl.h:122:14: error: use of enum ‘xc_error_code’ without
previous declaration
 typedef enum xc_error_code xc_error_code;
  ^
/usr/include/xenctrl.h:122:41: error: invalid type in declaration before
‘;’ token
 typedef enum xc_error_code xc_error_code;
 ^
/usr/include/xenctrl.h:1759:6: error: using typedef-name ‘xc_error_code’
after ‘enum’
 enum xc_error_code {
  ^
/usr/include/xenctrl.h:122:28: note: ‘xc_error_code’ has a previous
declaration here
 typedef enum xc_error_code xc_error_code;
^
/usr/include/xenctrl.h:1770:8: error: using typedef-name ‘xc_error_code’
after ‘enum’
   enum xc_error_code code;
^
/usr/include/xenctrl.h:122:28: note: ‘xc_error_code’ has a previous
declaration here
 typedef enum xc_error_code xc_error_code;




I looked at xenctrl.h and the typedef is put before the declaration of the
enum :

typedef enum xc_error_code xc_error_code;

...

enum xc_error_code {
  XC_ERROR_NONE = 0,
  XC_INTERNAL_ERROR = 1,
  XC_INVALID_KERNEL = 2,
  XC_INVALID_PARAM = 3,
  XC_OUT_OF_MEMORY = 4,
  /* new codes need to be added to xc_error_level_to_desc too */
};




I swapped the two declarations and that works just fine :

enum xc_error_code {
  XC_ERROR_NONE = 0,
  XC_INTERNAL_ERROR = 1,
  XC_INVALID_KERNEL = 2,
  XC_INVALID_PARAM = 3,
  XC_OUT_OF_MEMORY = 4,
  /* new codes need to be added to xc_error_level_to_desc too */
};

typedef enum xc_error_code xc_error_code;




Best regards,

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

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

[Ian Campbell]

On Thu, 2015-09-10 at 07:08 -0600, Jan Beulich wrote:
> > > > On 10.09.15 at 14:58,  wrote:
> > On Thu, 2015-09-10 at 13:15 +0100, Mark Rutland wrote:
> > 
> > > > In any case this should be separate from the shim ABI discussion.
> > > 
> > > I disagree; I think this is very much relevant to the ABI discussion.
> > > That's not to say that I insist on a particular approach, but I think
> > > that they need to be considered together.
> > 
> > Taking a step back, the reason for using the EFI stub parameters is
> > only
> > (AFAIK) in order to be able to locate the ACPI RDSP (the root table
> > pointer), which as it happens is normally passed via one of the EFI
> > firmware tables.
> > 
> > If there was a way to achieve that goal (i.e. another way to find the
> > RSDP)
> > without opening the can of UEFI worms then we could consider that
> > opiton
> > too.
> > 
> > a way != the legacy x86 thing of scanning low memory of the signature,
> > of
> > course.
> 
> But even x86 doesn't do that (other than as a fallback) on EFI.

Indeed, I was referring legacy (non-EFI) mode.

>  The
> configuration table is available to Dom0 (via XENPF_firmware_info:
> XEN_FW_EFI_INFO:XEN_FW_EFI_CONFIG_TABLE).

Under ARM we find out we are running under Xen from the ACPI tables, so
there is a chicken and egg situation there.

Not insoluble I'm sure, if we want to go down this route.
Ian.

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

Re: [Xen-devel] [PATCH net v3 2/2] xen-netfront: respect user provided max_queues

[David Vrabel]

On 10/09/15 11:18, Wei Liu wrote:
> Originally that parameter was always reset to num_online_cpus during
> module initialisation, which renders it useless.
> 
> The fix is to only set max_queues to num_online_cpus when user has not
> provided a value.

Reviewed-by: David Vrabel 
Tested-by: David Vrabel 

David

> --- a/drivers/net/xen-netfront.c
> +++ b/drivers/net/xen-netfront.c
> @@ -2132,8 +2132,11 @@ static int __init netif_init(void)
>  
>   pr_info("Initialising Xen virtual ethernet driver\n");
>  
> - /* Allow as many queues as there are CPUs, by default */
> - xennet_max_queues = num_online_cpus();
> + /* Allow as many queues as there are CPUs if user has not
> +  * specified a value.
> +  */
> + if (xennet_max_queues == 0)
> + xennet_max_queues = num_online_cpus();
>  
>   return xenbus_register_frontend(_driver);
>  }
> 


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

Re: [Xen-devel] [v2][PATCH] xen/vtd/iommu: permit group devices to passthrough in relaxed mode

[Håkon Alstadheim]

Den 10. sep. 2015 08:06, skrev Tian, Kevin:
>> From: Chen, Tiejun
>> Sent: Thursday, September 10, 2015 1:47 PM
>>
>> But recently someone was encountering this problem.
>>
>> http://www.gossamer-threads.com/lists/xen/devel/391684?page=last
>>
>> We'd better figure out a simple way to this regression.
>>
> I'm not sure how popular that motherboard is used...
I've got the same devices on Asus Z10PE-D8 WS. Probably used
by several motherboard-manufacturers.


>  To me security is
> important so having some limitation for this purpose is acceptable.
PCIE add-in cards for USB are hard to work with, see [1],  so maximum use
of the on-board USB is a high priority for me, and I'd think others aswell.
>  But
> I'd also like to hear comments from Jan and Wei. If they think regression
> is more important (anyway we're not causing new security problem, it's
> there before), I'm OK with this patch (besides you need fix print level)
>
>
[1]


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

Re: [Xen-devel] [PATCH for 4.6 v2 3/3] xl/libxl: disallow saving a guest with vNUMA configured

[Ian Campbell]

On Wed, 2015-09-09 at 21:21 +0100, Wei Liu wrote:
> On Wed, Sep 09, 2015 at 07:49:16PM +0100, Andrew Cooper wrote:
> > On 09/09/15 18:03, Wei Liu wrote:
> > > This is due to migration v2 frame record doesn't contain node
> > > information.
> > 
> > This isn't a migration v2 bug, and it was similarly non-functional with
> > legacy migration.  It was yet another oversight with the vNUMA work.
> > 
> 
> Indeed. I mentioned that in v1 patch but dropped it in v2 -- legacy
> migration is gone anyway.

I think it is important in these cases to understand if this is a
functional regression vs 4.5 or not, so it ought to be mentioned.

> > Isn't this information available in the domain configuration for the
> > domain
> > sent in the xl header? (That layer violation also need removing).
> > 
> 
> Yes. But restoring a guest takes a path  different from guest creation
> to populate guest pages.
> 
> > Finally, your phrasing is somewhat unclear.  I would recommend "The
> > migration stream does not preserve node information" as a clearer
> > alternative.
> > 
> 
> Fine by me.

I'm expecting a v3 then.


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

Re: [Xen-devel] [PATCH for 4.6] configure: don't silently disable systemd support

[Wei Liu]

On Thu, Sep 10, 2015 at 11:54:16AM +0100, Ian Campbell wrote:
> On Wed, 2015-09-09 at 23:35 +0100, Wei Liu wrote:
> > Originally when user runs ./configure --enable-systemd and systemd
> > development library is not available the build system silently disables
> > systemd support. This is not in line with normal expectation.
> > 
> > Instead, configure should error out when user has asked for systemd
> > support but development libraries can't be found.
> > 
> > Reported-by: George Dunlap 
> > Signed-off-by: Wei Liu 
> > ---
> > Please rerun ./autogen.sh.
> > ---
> >  m4/systemd.m4 | 6 +-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/m4/systemd.m4 b/m4/systemd.m4
> > index 8284993..84508a1 100644
> > --- a/m4/systemd.m4
> > +++ b/m4/systemd.m4
> > @@ -85,7 +85,11 @@ AC_DEFUN([AX_CHECK_SYSTEMD], [
> > AC_DEFINE([HAVE_SYSTEMD], [1], [Systemd available and
> > enabled])
> > systemd=y
> > AX_CHECK_SYSTEMD_LIBS()
> > -   ],[systemd=n])
> > +   ],[
> > +>  >   > AS_IF([test "x$enable_systemd" != "x"],
> 
> At this point the value of enable_systemd is either empty or ...what...? I
> think "yes"?, with "no" having been ruled out earlier?
> 
> Or can it be just "y"?
> 

At this point, it can't be no. It's either empty or "yes" (or "y"?). I
printed it out when I wrote this patch but could remember exactly what
it was.

> I could live with this version, but I think it would be less of a brain ben
> der if the check was inverted to be "x$enable_systemd" = "xyes" i.e. more
> explicit.
> 

I can do that as well. V2 coming soon.

Wei.

> Ian.

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

Re: [Xen-devel] [v2][PATCH] xen/vtd/iommu: permit group devices to passthrough in relaxed mode

[Håkon Alstadheim]

Den 10. sep. 2015 13:04, skrev Håkon Alstadheim:
> Den 10. sep. 2015 08:06, skrev Tian, Kevin:
>>> From: Chen, Tiejun
>>> Sent: Thursday, September 10, 2015 1:47 PM
>>>
>>> But recently someone was encountering this problem.
>>>
>>> http://www.gossamer-threads.com/lists/xen/devel/391684?page=last
>>>
>>> We'd better figure out a simple way to this regression.
>>>
>> I'm not sure how popular that motherboard is used...
> I've got the same devices on Asus Z10PE-D8 WS. Probably used
> by several motherboard-manufacturers.

To be very precice: I've got _similar_ devices in my motherboard chipset.
Thus we know Intel chipset C200 has this issue, and I am fairly certain
that C610/X99 also has this issue.



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

Re: [Xen-devel] [Qemu-devel] [edk2] EDK II & GPL - Re: OVMF BoF @ KVM Forum 2015

[Dr. David Alan Gilbert]

* Daniel P. Berrange (berra...@redhat.com) wrote:
> On Wed, Sep 09, 2015 at 05:41:59PM -0700, Jordan Justen wrote:
> > On 2015-09-09 16:05:20, Andrew Fish wrote:
> > > 
> > > > On Sep 9, 2015, at 3:24 PM, Jordan Justen  
> > > > wrot> > > FWIW, I don't mind if the consensus is that GplDriverPkg must 
> > > > live in
> > > > a separate repo. But, it would be nice to hear a good reason why it
> > > > must live elsewhere.
> > > 
> > > Because GPL is not a permissive license. An accidental git grep and
> > > copying some code can change the license of the code that gets the
> > > GPL code pasted into it.
> > 
> > I like this argument. It is slightly tempered by the fact that git
> > grep always shows the source path, and thus 'GplDriverPkg' would be
> > obviously visible.
> 
> Plenty of projects have a scenario in which different parts of their
> codebase are under different licenses, without there being undue
> problems. If you make it clear by having a separate directory, then
> I think you can ultimately credit the developers with having enough
> intelligence to do the right thing here. If not, then I'd probably
> question whether you can trust them to submit any code at all, as
> they could equally have blindly copied it from a 3rd party project
> under an incompatible license.

Many companies dont trust their engineers to do that, and have painful
review processes to stop their engineers stupidly copying closed
code into open projects; and in general they're needed because the
engineers would do it if they weren't stopped.

Dave

> 
> Regards,
> Daniel
> -- 
> |: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
> |: http://libvirt.org  -o- http://virt-manager.org :|
> |: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
> |: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

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

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

[Mark Rutland]

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

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

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

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

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

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

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

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

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

Thanks,
Mark.

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

Re: [Xen-devel] [PATCH v6 16/18] vmx: Add some scheduler hooks for VT-d posted interrupts

[Jan Beulich]

>>> On 10.09.15 at 14:34,  wrote:

> 
>> -Original Message-
>> From: Jan Beulich [mailto:jbeul...@suse.com]
>> Sent: Thursday, September 10, 2015 6:01 PM
>> To: Wu, Feng
>> Cc: Andrew Cooper; Dario Faggioli; George Dunlap; Tian, Kevin;
>> xen-devel@lists.xen.org; Keir Fraser
>> Subject: RE: [PATCH v6 16/18] vmx: Add some scheduler hooks for VT-d posted
>> interrupts
>> 
>> >>> On 10.09.15 at 11:41,  wrote:
>> >> From: Jan Beulich [mailto:jbeul...@suse.com]
>> >> Sent: Thursday, September 10, 2015 5:26 PM
>> >> >>> On 10.09.15 at 10:59,  wrote:
>> >> > First, how to check it while waiting to acquire the lock .pi_block_cpu
>> >> > didn't change?
>> >>
>> >> Note the difference between "check while waiting" and "check that
>> >> while waiting": The former is indeed hard to implement, while the
>> >> latter is pretty straightforward (and we do so elsewhere).
>> >>
>> >> > Secondly, even if we can check it, what should we do if .pi_block_cpu
>> >> > is changed after acquiring the lock as I mentioned above?
>> >>
>> >> Drop the lock and start over. I.e. (taking your pseudo code)
>> >>
>> >> restart:
>> >> local_pi_block_cpu = ...;
>> >> bail-if-invalid (e.g. -1 in current model)
>> >> spin_lock_irqsave(_cpu(, local_pi_block_cpu), flags);
>> >> if(local_pi_block_cpu != actual_pi_block_cpu) {
>> >> spin_unlock_irqrestore(_cpu(,local_pi_block_cpu), flags);
>> >> goto restart;
>> >> }
>> >
>> > Thanks a lot for showing me this pseudo code! My concern is if
>> > .pi_block_vcpu is changed to -1 at this point, it doesn't work.
>> > .pi_block_vcpu being -1 here means the vCPU is remove from
>> > the blocking list by others, then we cannot delete it again via
>> > list_del() here.
>> 
>> Did you miss the "bail-if-invalid" above?
> 
> I am sorry, do I miss something here? If .pi_block_cpu becomes
> -1 here (after the above 'if' statement is finished with
> local_pi_block_cpu == actual_pi_block_cpu ), how can "bail-if-invalid"
> above help?

The (obvious I thought) implication is that all assignments to
pi_block_cpu (along with all list manipulations) now need to happen
with the lock held.

Jan


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

Re: [Xen-devel] [edk2] EDK II & GPL - Re: OVMF BoF @ KVM Forum 2015

[Alexander Graf]

> Am 10.09.2015 um 14:17 schrieb Andrew Fish :
> 
> 
>> On Sep 10, 2015, at 4:40 AM, Alexander Graf  wrote:
>> 
>> 
>> 
>>> On 10.09.15 12:04, Laszlo Ersek wrote:
 On 09/10/15 08:19, Alexander Graf wrote:
 
 
> Am 10.09.2015 um 07:32 schrieb Jordan Justen :
>>> 
> Laszlo's email raised the GPL question, but I was not sure what the
> EDK II community would accept with regards to GPL. Thus ... I asked. I
> guess I'm getting a better idea with regards to Apple and HP. :)
> 
> In your opinion, would we be able to discuss patches for a *separate*
> repo with GplDriverPkg on edk2-devel?
 
 In fact, could we just make the non-free FAT source and GPL FAT
 source both be git submodules?
>>> 
>>> We've discussed submodules in the past (for other purposes). The
>>> consensus seemed to be that most people dislike them (me included).
>>> 
>>> UEFI drivers are supposed to be modular / well separable (for one, they
>>> can be shipped by third parties in binary-only form; which was a design
>>> goal of UEFI). And specifically in the FAT driver's case, the source
>>> doesn't even live inside the main repo at the moment, so turning it into
>>> a source submodule might not be a step back.
>>> 
>>> But... I just don't like it. We should be moving towards a grand unified
>>> repo, where cross-module changes and dependencies are possible to
>>> implement with carefully segmented patch sets. The FAT driver's source
>>> lives outside for non-technical reasons. Rather than codifying that
>>> situation forever with a git submodule, I'd prefer some solution that
>>> leaves us with a standalone repo.
>>> 
>>> I think I'm fuzzy on the details of the earlier git-submodule
>>> discussion. In any case here's the link (I hope this is the right one):
>>> 
>>> https://urldefense.proofpoint.com/v2/url?u=http-3A__thread.gmane.org_gmane.comp.bios.tianocore.devel_15168=BQID-g=eEvniauFctOgLOKGJOplqw=1HnUuXD1wDvw67rut5_idw=6S8FigY338Lo73J1DVLkCR-sshwa_iC7dw0Ng-S8U4o=VQx-tNIhMX_CI4DXORkNq10jBTMAXyaFT332GWZhfRQ=
>>>  
>>> 
 Then whoever clones the repo can get
 the license flavor he's least scared about.
>>> 
>>> I think for many companies it is important that a developer of theirs
>>> who is "blissfully ignorant" of licensing questions simply *cannot* make
>>> a mistake (eg. by copying code from the "wrong" directory, or by using
>>> the "wrong" submodule). It should be foolproof.
>>> 
 Or alternatively instead
 of pulling in a GPL licensed FAT driver we use a BSD licensed one.
 I'm sure someone has one of those too ;).
>>> 
>>> I'm not sure at all. Do you have a pointer? :)
>> 
>> Well, the BSDs definitely have drivers, but I find the BSD VFS layer
>> quite confusing to be honest ;).
>> 
>> Then there is 
>> https://urldefense.proofpoint.com/v2/url?u=http-3A__elm-2Dchan.org_fsw_ff_00index-5Fe.html=BQID-g=eEvniauFctOgLOKGJOplqw=1HnUuXD1wDvw67rut5_idw=6S8FigY338Lo73J1DVLkCR-sshwa_iC7dw0Ng-S8U4o=l2F5kQrmSHyEb7D4po7B0A-vQK1l7rCkw79eJCddVmQ=
>>   which from my
>> gut feeling has a compatible license (read: needs verification).
>> 
>> I'm sure with some extensive search one can find a workable driver. Or
>> for example Apple could just contribute theirs as BSD licensed.
> 
> They are talking about an EFI FAT driver with a BSD compatible license, not a 
> BSD driver. 

We're talking about replacing the non-free FAT driver with a free one for OVMF. 
The easiest path to get there isvto reuse an existing driver - the original 
proposal was to take the one from grub and fit it into uEFI's interfaces.

An alternative to the GPL grub driver would be a BSD licensed driver from 
somewhere else or a rewrite. Rewrite sounds harder. If we can throw out the 
non-free FAT driver and put in a BSD licensed FAT driver based on known working 
code into edk2, I suppose it's a win for everyone involved and we wouldn't even 
need a fork for OVMF imho but keep it as reference implementation in the tree, 
like Duet.


Alex


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

Re: [Xen-devel] [PATCH v6 16/18] vmx: Add some scheduler hooks for VT-d posted interrupts

[Wu, Feng]

> -Original Message-
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: Thursday, September 10, 2015 9:15 PM
> To: Wu, Feng
> Cc: Andrew Cooper; Dario Faggioli; George Dunlap; Tian, Kevin;
> xen-devel@lists.xen.org; Keir Fraser
> Subject: RE: [PATCH v6 16/18] vmx: Add some scheduler hooks for VT-d posted
> interrupts
> 
> >>> On 10.09.15 at 14:58,  wrote:
> 
> >
> >> -Original Message-
> >> From: Jan Beulich [mailto:jbeul...@suse.com]
> >> Sent: Thursday, September 10, 2015 8:45 PM
> >> To: Wu, Feng
> >> Cc: Andrew Cooper; Dario Faggioli; George Dunlap; Tian, Kevin;
> >> xen-devel@lists.xen.org; Keir Fraser
> >> Subject: RE: [PATCH v6 16/18] vmx: Add some scheduler hooks for VT-d
> posted
> >> interrupts
> >>
> >> >>> On 10.09.15 at 14:34,  wrote:
> >>
> >> >
> >> >> -Original Message-
> >> >> From: Jan Beulich [mailto:jbeul...@suse.com]
> >> >> Sent: Thursday, September 10, 2015 6:01 PM
> >> >> To: Wu, Feng
> >> >> Cc: Andrew Cooper; Dario Faggioli; George Dunlap; Tian, Kevin;
> >> >> xen-devel@lists.xen.org; Keir Fraser
> >> >> Subject: RE: [PATCH v6 16/18] vmx: Add some scheduler hooks for VT-d
> >> posted
> >> >> interrupts
> >> >>
> >> >> >>> On 10.09.15 at 11:41,  wrote:
> >> >> >> From: Jan Beulich [mailto:jbeul...@suse.com]
> >> >> >> Sent: Thursday, September 10, 2015 5:26 PM
> >> >> >> >>> On 10.09.15 at 10:59,  wrote:
> >> >> >> > First, how to check it while waiting to acquire the lock 
> >> >> >> > .pi_block_cpu
> >> >> >> > didn't change?
> >> >> >>
> >> >> >> Note the difference between "check while waiting" and "check that
> >> >> >> while waiting": The former is indeed hard to implement, while the
> >> >> >> latter is pretty straightforward (and we do so elsewhere).
> >> >> >>
> >> >> >> > Secondly, even if we can check it, what should we do if 
> >> >> >> > .pi_block_cpu
> >> >> >> > is changed after acquiring the lock as I mentioned above?
> >> >> >>
> >> >> >> Drop the lock and start over. I.e. (taking your pseudo code)
> >> >> >>
> >> >> >> restart:
> >> >> >> local_pi_block_cpu = ...;
> >> >> >> bail-if-invalid (e.g. -1 in current model)
> >> >> >> spin_lock_irqsave(_cpu(, local_pi_block_cpu), flags);
> >> >> >> if(local_pi_block_cpu != actual_pi_block_cpu) {
> >> >> >> spin_unlock_irqrestore(_cpu(,local_pi_block_cpu),
> flags);
> >> >> >> goto restart;
> >> >> >> }
> >> >> >
> >> >> > Thanks a lot for showing me this pseudo code! My concern is if
> >> >> > .pi_block_vcpu is changed to -1 at this point, it doesn't work.
> >> >> > .pi_block_vcpu being -1 here means the vCPU is remove from
> >> >> > the blocking list by others, then we cannot delete it again via
> >> >> > list_del() here.
> >> >>
> >> >> Did you miss the "bail-if-invalid" above?
> >> >
> >> > I am sorry, do I miss something here? If .pi_block_cpu becomes
> >> > -1 here (after the above 'if' statement is finished with
> >> > local_pi_block_cpu == actual_pi_block_cpu ), how can "bail-if-invalid"
> >> > above help?
> >>
> >> The (obvious I thought) implication is that all assignments to
> >> pi_block_cpu (along with all list manipulations) now need to happen
> >> with the lock held.
> >
> > If all the assignment to pi_block_cpu is with one lock held, I don't think
> > we need to above checkout, we can safely use .pi_block_cpu, right?
> 
> No. In order to use it you need to make sure it's valid (or else
> there's no valid lock to acquire). And once you acquired the
> lock, you have to check whether changed / became invalid.

If all the assignment to .pi_block_cpu is with one lock held,
how can it get changed / become invalid then once I acquired
the lock?

Thanks,
Feng

> Plus the up front check allows to avoid acquiring the lock in the
> first place when the field is invalid anyway.
> 
> Jan


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

Re: [Xen-devel] [PULL 0/19] xen-2015-09-08-tag

[Michael S. Tsirkin]

On Thu, Sep 10, 2015 at 12:26:21PM +0100, Stefano Stabellini wrote:
> On Thu, 10 Sep 2015, Michael S. Tsirkin wrote:
> > On Thu, Sep 10, 2015 at 11:29:18AM +0100, Stefano Stabellini wrote:
> > > CC Michael
> > > 
> > > On Thu, 10 Sep 2015, Stefano Stabellini wrote:
> > > > On Thu, 10 Sep 2015, Chen, Tiejun wrote:
> > > > > > xen-host-pci-device.c is only compiled if CONFIG_XEN_PCI_PASSTHROUGH
> > > > > > was set by configure. That won't be the case on OSX or Windows, 
> > > > > > where
> > > > > > the Xen headers don't exist.
> > > > > > 
> > > > > 
> > > > > Okay. This actually shouldn't be enabled on Windows so what about 
> > > > > this?
> > > > 
> > > > I think it would be nicer to replace the pread than introducing ifdefs.
> > > 
> > > Something like:
> > > 
> > > ---
> > > Replace pread with read to avoid build breakages on Windows
> > > 
> > > Signed-off-by: Stefano Stabellini 
> > 
> > I'd prefer a wrapper that does the right thing.
> > No sense in doubling the # of system calls for everyone.
> 
> If this was done on an hot path I would agree with you, but it is just
> one call at initialization time (igd_pt_i440fx_initfn).

I missed this fact. OK then.

> 
> > > diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> > > index 58a33fb..9a40429 100644
> > > --- a/hw/pci-host/piix.c
> > > +++ b/hw/pci-host/piix.c
> > > @@ -774,8 +774,11 @@ static int host_pci_config_read(int pos, int len, 
> > > uint32_t val)
> > >  return -ENODEV;
> > >  }
> > >  
> > > +if (lseek(config_fd, pos, SEEK_SET) != pos) {
> > > +return -errno;
> > > +}
> > >  do {
> > > -rc = pread(config_fd, (uint8_t *), len, pos);
> > > +rc = read(config_fd, (uint8_t *), len);
> > >  } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
> > >  if (rc != len) {
> > >  return -errno;
> > 

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

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

[Jan Beulich]

>>> On 10.09.15 at 14:58,  wrote:
> On Thu, 2015-09-10 at 13:15 +0100, Mark Rutland wrote:
> 
>> > In any case this should be separate from the shim ABI discussion.
>> 
>> I disagree; I think this is very much relevant to the ABI discussion.
>> That's not to say that I insist on a particular approach, but I think
>> that they need to be considered together.
> 
> Taking a step back, the reason for using the EFI stub parameters is only
> (AFAIK) in order to be able to locate the ACPI RDSP (the root table
> pointer), which as it happens is normally passed via one of the EFI
> firmware tables.
> 
> If there was a way to achieve that goal (i.e. another way to find the RSDP)
> without opening the can of UEFI worms then we could consider that opiton
> too.
> 
> a way != the legacy x86 thing of scanning low memory of the signature, of
> course.

But even x86 doesn't do that (other than as a fallback) on EFI. The
configuration table is available to Dom0 (via XENPF_firmware_info:
XEN_FW_EFI_INFO:XEN_FW_EFI_CONFIG_TABLE).

Jan


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

Re: [Xen-devel] [edk2] EDK II & GPL - Re: OVMF BoF @ KVM Forum 2015

[Andrew Fish]

> On Sep 10, 2015, at 4:40 AM, Alexander Graf  wrote:
> 
> 
> 
> On 10.09.15 12:04, Laszlo Ersek wrote:
>> On 09/10/15 08:19, Alexander Graf wrote:
>>> 
>>> 
 Am 10.09.2015 um 07:32 schrieb Jordan Justen :
>> 
 Laszlo's email raised the GPL question, but I was not sure what the
 EDK II community would accept with regards to GPL. Thus ... I asked. I
 guess I'm getting a better idea with regards to Apple and HP. :)
 
 In your opinion, would we be able to discuss patches for a *separate*
 repo with GplDriverPkg on edk2-devel?
>>> 
>>> In fact, could we just make the non-free FAT source and GPL FAT
>>> source both be git submodules?
>> 
>> We've discussed submodules in the past (for other purposes). The
>> consensus seemed to be that most people dislike them (me included).
>> 
>> UEFI drivers are supposed to be modular / well separable (for one, they
>> can be shipped by third parties in binary-only form; which was a design
>> goal of UEFI). And specifically in the FAT driver's case, the source
>> doesn't even live inside the main repo at the moment, so turning it into
>> a source submodule might not be a step back.
>> 
>> But... I just don't like it. We should be moving towards a grand unified
>> repo, where cross-module changes and dependencies are possible to
>> implement with carefully segmented patch sets. The FAT driver's source
>> lives outside for non-technical reasons. Rather than codifying that
>> situation forever with a git submodule, I'd prefer some solution that
>> leaves us with a standalone repo.
>> 
>> I think I'm fuzzy on the details of the earlier git-submodule
>> discussion. In any case here's the link (I hope this is the right one):
>> 
>> https://urldefense.proofpoint.com/v2/url?u=http-3A__thread.gmane.org_gmane.comp.bios.tianocore.devel_15168=BQID-g=eEvniauFctOgLOKGJOplqw=1HnUuXD1wDvw67rut5_idw=6S8FigY338Lo73J1DVLkCR-sshwa_iC7dw0Ng-S8U4o=VQx-tNIhMX_CI4DXORkNq10jBTMAXyaFT332GWZhfRQ=
>>  
>> 
>>> Then whoever clones the repo can get
>>> the license flavor he's least scared about.
>> 
>> I think for many companies it is important that a developer of theirs
>> who is "blissfully ignorant" of licensing questions simply *cannot* make
>> a mistake (eg. by copying code from the "wrong" directory, or by using
>> the "wrong" submodule). It should be foolproof.
>> 
>>> Or alternatively instead
>>> of pulling in a GPL licensed FAT driver we use a BSD licensed one.
>>> I'm sure someone has one of those too ;).
>> 
>> I'm not sure at all. Do you have a pointer? :)
> 
> Well, the BSDs definitely have drivers, but I find the BSD VFS layer
> quite confusing to be honest ;).
> 
> Then there is 
> https://urldefense.proofpoint.com/v2/url?u=http-3A__elm-2Dchan.org_fsw_ff_00index-5Fe.html=BQID-g=eEvniauFctOgLOKGJOplqw=1HnUuXD1wDvw67rut5_idw=6S8FigY338Lo73J1DVLkCR-sshwa_iC7dw0Ng-S8U4o=l2F5kQrmSHyEb7D4po7B0A-vQK1l7rCkw79eJCddVmQ=
>   which from my
> gut feeling has a compatible license (read: needs verification).
> 
> I'm sure with some extensive search one can find a workable driver. Or
> for example Apple could just contribute theirs as BSD licensed.
> 

They are talking about an EFI FAT driver with a BSD compatible license, not a 
BSD driver. 
The edk2 EFI FAT driver has a license that matches the FAT32 spec it was coded 
against, but that license restricts the usage of the code to EFI. This is not 
deemed a GPL compatible license, so that causes issues with down stream GPL 
projects of the edk2 as there is a binary for the EFI FAT driver checked into 
the main branch of the edk2. The source to the edk2 EFI FAT driver is separate 
from the edk2 based on its funky license. 

Thanks,

Andrew Fish

> 
> Alex


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

Re: [Xen-devel] [PATCH for 4.6 v2 1/3] libxl: set ret to non-zero value in failure path

[Ian Campbell]

On Wed, 2015-09-09 at 18:03 +0100, Wei Liu wrote:
> ... otherwise we have something like:
> 
> xl: libxl_create.c:968: initiate_domain_create: Assertion `ret' failed.
> 
> Signed-off-by: Wei Liu 

Acked-by: Ian Campbell 


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

Re: [Xen-devel] [edk2] EDK II & GPL - Re: OVMF BoF @ KVM Forum 2015

[Alexander Graf]

On 10.09.15 12:04, Laszlo Ersek wrote:
> On 09/10/15 08:19, Alexander Graf wrote:
>>
>>
>>> Am 10.09.2015 um 07:32 schrieb Jordan Justen :
> 
>>> Laszlo's email raised the GPL question, but I was not sure what the
>>> EDK II community would accept with regards to GPL. Thus ... I asked. I
>>> guess I'm getting a better idea with regards to Apple and HP. :)
>>>
>>> In your opinion, would we be able to discuss patches for a *separate*
>>> repo with GplDriverPkg on edk2-devel?
>>
>> In fact, could we just make the non-free FAT source and GPL FAT
>> source both be git submodules?
> 
> We've discussed submodules in the past (for other purposes). The
> consensus seemed to be that most people dislike them (me included).
> 
> UEFI drivers are supposed to be modular / well separable (for one, they
> can be shipped by third parties in binary-only form; which was a design
> goal of UEFI). And specifically in the FAT driver's case, the source
> doesn't even live inside the main repo at the moment, so turning it into
> a source submodule might not be a step back.
> 
> But... I just don't like it. We should be moving towards a grand unified
> repo, where cross-module changes and dependencies are possible to
> implement with carefully segmented patch sets. The FAT driver's source
> lives outside for non-technical reasons. Rather than codifying that
> situation forever with a git submodule, I'd prefer some solution that
> leaves us with a standalone repo.
> 
> I think I'm fuzzy on the details of the earlier git-submodule
> discussion. In any case here's the link (I hope this is the right one):
> 
> http://thread.gmane.org/gmane.comp.bios.tianocore.devel/15168
> 
>> Then whoever clones the repo can get
>> the license flavor he's least scared about.
> 
> I think for many companies it is important that a developer of theirs
> who is "blissfully ignorant" of licensing questions simply *cannot* make
> a mistake (eg. by copying code from the "wrong" directory, or by using
> the "wrong" submodule). It should be foolproof.
> 
>> Or alternatively instead
>> of pulling in a GPL licensed FAT driver we use a BSD licensed one.
>> I'm sure someone has one of those too ;).
> 
> I'm not sure at all. Do you have a pointer? :)

Well, the BSDs definitely have drivers, but I find the BSD VFS layer
quite confusing to be honest ;).

Then there is http://elm-chan.org/fsw/ff/00index_e.html which from my
gut feeling has a compatible license (read: needs verification).

I'm sure with some extensive search one can find a workable driver. Or
for example Apple could just contribute theirs as BSD licensed.


Alex

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

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

[Stefano Stabellini]

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


> Why can't Xen give a virtual EFI interface to Dom0 / guests? e.g.
> create pages of RuntimeServicesCode that are trivial assembly shims
> doing hypercalls, and plumb these into the virtual EFI memory map and
> tables?
> 
> That would keep things sane for any guest, allow for easy addition of
> EFI features, and you could even enter the usual EFI entry point,
> simulate ExitBootServices(), SetVirtualAddressMap(), and allow the guest
> to make things sane for itself...

That's the way it was done on x86 and now we have common code both in
Linux (drivers/xen/efi.c) and Xen (xen/common/efi) which implement this
scheme.  Switching to a different solution for ARM, would mean diverging
with x86, which is not nice, or reimplementing the x86 solution too,
which is expensive.

BTW I think that the idea you proposed was actually considered at the
time and deemed hard to implement, if I recall correctly.


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

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

Re: [Xen-devel] [PULL 0/19] xen-2015-09-08-tag

[Michael S. Tsirkin]

On Thu, Sep 10, 2015 at 01:00:35PM +0100, Stefano Stabellini wrote:
> On Thu, 10 Sep 2015, Michael S. Tsirkin wrote:
> > On Thu, Sep 10, 2015 at 12:26:21PM +0100, Stefano Stabellini wrote:
> > > On Thu, 10 Sep 2015, Michael S. Tsirkin wrote:
> > > > On Thu, Sep 10, 2015 at 11:29:18AM +0100, Stefano Stabellini wrote:
> > > > > CC Michael
> > > > > 
> > > > > On Thu, 10 Sep 2015, Stefano Stabellini wrote:
> > > > > > On Thu, 10 Sep 2015, Chen, Tiejun wrote:
> > > > > > > > xen-host-pci-device.c is only compiled if 
> > > > > > > > CONFIG_XEN_PCI_PASSTHROUGH
> > > > > > > > was set by configure. That won't be the case on OSX or Windows, 
> > > > > > > > where
> > > > > > > > the Xen headers don't exist.
> > > > > > > > 
> > > > > > > 
> > > > > > > Okay. This actually shouldn't be enabled on Windows so what about 
> > > > > > > this?
> > > > > > 
> > > > > > I think it would be nicer to replace the pread than introducing 
> > > > > > ifdefs.
> > > > > 
> > > > > Something like:
> > > > > 
> > > > > ---
> > > > > Replace pread with read to avoid build breakages on Windows
> > > > > 
> > > > > Signed-off-by: Stefano Stabellini 
> > > > 
> > > > I'd prefer a wrapper that does the right thing.
> > > > No sense in doubling the # of system calls for everyone.
> > > 
> > > If this was done on an hot path I would agree with you, but it is just
> > > one call at initialization time (igd_pt_i440fx_initfn).
> > 
> > I missed this fact. OK then.
> 
> Thanks! I'll fold it the offending patch
> (http://marc.info/?l=qemu-devel=144174596628052=2) and resend.
> 

Reviewed-by: Michael S. Tsirkin 


> > > > > diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> > > > > index 58a33fb..9a40429 100644
> > > > > --- a/hw/pci-host/piix.c
> > > > > +++ b/hw/pci-host/piix.c
> > > > > @@ -774,8 +774,11 @@ static int host_pci_config_read(int pos, int 
> > > > > len, uint32_t val)
> > > > >  return -ENODEV;
> > > > >  }
> > > > >  
> > > > > +if (lseek(config_fd, pos, SEEK_SET) != pos) {
> > > > > +return -errno;
> > > > > +}
> > > > >  do {
> > > > > -rc = pread(config_fd, (uint8_t *), len, pos);
> > > > > +rc = read(config_fd, (uint8_t *), len);
> > > > >  } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
> > > > >  if (rc != len) {
> > > > >  return -errno;
> > > > 
> > 

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

[Xen-devel] [PATCH] libxl: slightly refine pci-assignable-{add, remove} handling

[Jan Beulich]

While it appears to be intentional for "xl pci-assignable-remove" to
not re-bind the original driver by default (requires the -r option),
permanently losing the information which driver was originally used
seems bad. Make "add; remove; add; remove -r" re-bind the original
driver by allowing "remove" to delete the information only upon
successful re-bind.

In the course of this I also noticed that binding information is lost
when upon first "add" pciback isn't loaded yet, due to its presence not
being checked for early enough. Adjust pciback_dev_is_assigned()
accordingly, and properly distinguish "yes" and "error" returns in the
"add" case (removing a redundant error message from the "remove" path
for consistency).

Signed-off-by: Jan Beulich 
---
As to 4.6 I'm not overly fussed: It'd be nice, but it could easily be
backported later on.

--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -543,6 +543,17 @@ static int pciback_dev_is_assigned(libxl
 int rc;
 struct stat st;
 
+if ( access(SYSFS_PCIBACK_DRIVER, F_OK) < 0 ) {
+if ( errno == ENOENT ) {
+LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
+   "Looks like pciback driver is not loaded");
+} else {
+LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
+ "Can't access "SYSFS_PCIBACK_DRIVER);
+}
+return -1;
+}
+
 spath = libxl__sprintf(gc, SYSFS_PCIBACK_DRIVER"/"PCI_BDF,
pcidev->domain, pcidev->bus,
pcidev->dev, pcidev->func);
@@ -658,6 +669,7 @@ static int libxl__device_pci_assignable_
 libxl_ctx *ctx = libxl__gc_owner(gc);
 unsigned dom, bus, dev, func;
 char *spath, *driver_path = NULL;
+int rc;
 struct stat st;
 
 /* Local copy for convenience */
@@ -674,7 +686,11 @@ static int libxl__device_pci_assignable_
 }
 
 /* Check to see if it's already assigned to pciback */
-if ( pciback_dev_is_assigned(gc, pcidev) ) {
+rc = pciback_dev_is_assigned(gc, pcidev);
+if ( rc < 0 ) {
+return ERROR_FAIL;
+}
+if ( rc ) {
 LIBXL__LOG(ctx, LIBXL__LOG_WARNING, PCI_BDF" already assigned to 
pciback",
dom, bus, dev, func);
 return 0;
@@ -692,11 +708,18 @@ static int libxl__device_pci_assignable_
 if ( rebind ) {
 if ( driver_path ) {
 pci_assignable_driver_path_write(gc, pcidev, driver_path);
+} else if ( (driver_path =
+ pci_assignable_driver_path_read(gc, pcidev)) != NULL ) {
+LIBXL__LOG(ctx, LIBXL__LOG_INFO,
+   PCI_BDF" not bound to a driver, will be rebound to %s",
+   dom, bus, dev, func, driver_path);
 } else {
 LIBXL__LOG(ctx, LIBXL__LOG_WARNING,
PCI_BDF" not bound to a driver, will not be rebound.",
dom, bus, dev, func);
 }
+} else {
+pci_assignable_driver_path_remove(gc, pcidev);
 }
 
 if ( pciback_dev_assign(gc, pcidev) ) {
@@ -717,7 +740,6 @@ static int libxl__device_pci_assignable_
 
 /* Unbind from pciback */
 if ( (rc=pciback_dev_is_assigned(gc, pcidev)) < 0 ) {
-LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Checking if pciback was assigned");
 return ERROR_FAIL;
 } else if ( rc ) {
 pciback_dev_unassign(gc, pcidev);
@@ -741,9 +763,9 @@ static int libxl__device_pci_assignable_
  "Couldn't bind device to %s", driver_path);
 return -1;
 }
-}
 
-pci_assignable_driver_path_remove(gc, pcidev);
+pci_assignable_driver_path_remove(gc, pcidev);
+}
 } else {
 if ( rebind ) {
 LIBXL__LOG(ctx, LIBXL__LOG_WARNING,



libxl: slightly refine pci-assignable-{add,remove} handling

While it appears to be intentional for "xl pci-assignable-remove" to
not re-bind the original driver by default (requires the -r option),
permanently losing the information which driver was originally used
seems bad. Make "add; remove; add; remove -r" re-bind the original
driver by allowing "remove" to delete the information only upon
successful re-bind.

In the course of this I also noticed that binding information is lost
when upon first "add" pciback isn't loaded yet, due to its presence not
being checked for early enough. Adjust pciback_dev_is_assigned()
accordingly, and properly distinguish "yes" and "error" returns in the
"add" case (removing a redundant error message from the "remove" path
for consistency).

Signed-off-by: Jan Beulich 
---
As to 4.6 I'm not overly fussed: It'd be nice, but it could easily be
backported later on.

--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -543,6 +543,17 @@ static int pciback_dev_is_assigned(libxl
 int rc;
 struct stat st;
 
+if ( access(SYSFS_PCIBACK_DRIVER, F_OK) < 0 ) {
+if ( errno 

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

[Jan Beulich]

>>> On 10.09.15 at 13:37,  wrote:
> On Thu, 10 Sep 2015, Mark Rutland wrote:
>> Why can't Xen give a virtual EFI interface to Dom0 / guests? e.g.
>> create pages of RuntimeServicesCode that are trivial assembly shims
>> doing hypercalls, and plumb these into the virtual EFI memory map and
>> tables?
>> 
>> That would keep things sane for any guest, allow for easy addition of
>> EFI features, and you could even enter the usual EFI entry point,
>> simulate ExitBootServices(), SetVirtualAddressMap(), and allow the guest
>> to make things sane for itself...
> 
> That's the way it was done on x86 and now we have common code both in
> Linux (drivers/xen/efi.c) and Xen (xen/common/efi) which implement this
> scheme.  Switching to a different solution for ARM, would mean diverging
> with x86, which is not nice, or reimplementing the x86 solution too,
> which is expensive.
> 
> BTW I think that the idea you proposed was actually considered at the
> time and deemed hard to implement, if I recall correctly.

Considering that the EFI support is just for Dom0, and Dom0 (at
the time) had to be PV anyway, it was the more natural solution to
expose the interface via hypercalls, the more that this allows better
control over what is and primarily what is not being exposed to
Dom0. With the wrapper approach we'd be back to the same
problem (discussed elsewhere) of which EFI version to surface: The
host one would impose potentially missing extensions, while the
most recent hypervisor known one might imply hiding valuable
information from Dom0. Plus there are incompatible changes like
the altered meaning of EFI_MEMORY_WP in 2.5.

Jan


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

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

[Julien Grall]

On 10/09/15 13:05, Roger Pau Monné wrote:
> El 10/09/15 a les 13.48, Julien Grall ha escrit:
>> On 10/09/15 12:32, Andrew Turner wrote:
>>> On Thu, 10 Sep 2015 16:41:56 +0800
>>> Shannon Zhao  wrote:
>>>
 From: Shannon Zhao 

 These EFI stub parameters are used to internal communication between
 EFI stub and Linux kernel and EFI stub creates these parameters. But
 for Xen on ARM when booting with UEFI, Xen will create a minimal DT
 providing these parameters for Dom0 and Dom0 is not only Linux
 kernel, but also other OS (such as FreeBSD) which will be used in the
 future. So here we plan to standardize the names by dropping the
 prefix "linux," and make them common to other OS. Also this will not
 break the compatibility since these parameters are used to internal
 communication between EFI stub and kernel.
>>>
>>> It is unlikely FreeBSD will use this property when booting ad Xen dom0.
>>> The kernel expects to be passed a data structure to find boot this
>>> information.
>>>
>>> My preference would be to have the FreeBSD loader load the xen binary,
>>> the FreeBSD kernel, and set up these data structs before passing
>>> control to Xen, with the information it needs to boot the kernel. My
>>> understanding is this is how it is done on x86.
>>
>> Well, AFAICT, there is no FreeBSD specific code in Xen for x86. We are
>> faking a multiboot module which contains all the informations.
>>
>> I know that the bootloader is at least involved, I don't remember if
>> there is some code in FreeBSD to read this boot module. I've CCed Roger
>> to confirm.
> 
> The metadata/modules needed by the FreeBSD Dom0 kernel is generated by
> the native FreeBSD bootloader (as would be done when booting bare
> metal). Then this blob is passed as a multiboot module in the same place
> that Linux puts it's initramfs. Xen simply copies this blob straight
> into guest memory and sets start_info.mod_start to point to the start
> memory address of this blob.
> 
> I've done it this way because I don't see many other options. Xen is
> tailored for Linux and only allows passing one module to the Dom0 kernel
> (initramfs). For FreeBSD it would be good to be able to pass at least
> two modules to the Dom0 kernel, one containing the metadata, and the
> other one containing the modules themselves. The new PVH work (HVMlite
> or whatever) will hopefully allow passing more than one module to the
> Dom0 kernel, and should make the code in the FreeBSD loader simpler.

We have a multiboot based on device tree to boot Xen on ARM [1].

It may be possible to create another kind of module storing the FreeBSD
metadata and make a trampoline for Xen in FreeBSD to pass the
information correctly to the DOM0 kernel.

But the main problem is we have to modify those metadata because,
AFAICT, the device tree/ACPI blob is part of them. It also seems to be
the same for UEFI.

That means we need some knowledge in Xen to parse/modify those metadata.
Unless they are stable (i.e clearly defined, not changing, place in
memory known...), we can't use them in Xen as we would need to adapt for
every changes.

If it's stable, we have to know that we are using FreeBSD (could be done
with the multiboot string).

Although, I would be more in favor of the loader using the Xen boot
interface with a new multiboot module, Xen jumping on a trampoline in
the FreeBSD kernel still using the Guest Boot ABI (similar the the Linux
ABI). The trampoline would recreate the metadata with the new DT and
jump into the kernel.

I will give a look to this when I'm done with the FreeBSD ARM guest support.

Regards,

[1] http://xenbits.xen.org/docs/unstable/misc/arm/device-tree/booting.txt

-- 
Julien Grall

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

[Xen-devel] [PATCH 2/2] xen/swiotlb: Add support for 64KB page granularity

[Julien Grall]

Swiotlb is used on ARM64 to support DMA on platform where devices are
not protected by an SMMU. Furthermore it's only enabled for DOM0.

While Xen is always using 4KB page granularity in the stage-2 page table,
Linux ARM64 may either use 4KB or 64KB. This means that a Linux page
can be spanned accross multiple Xen page.

The Swiotlb code has to validate that the buffer used for DMA is
physically contiguous in the memory. As a Linux page can't be shared
between local memory and foreign page by design (the balloon code always
removing entirely a Linux page), the changes in the code are very
minimal because we only need to check the first Xen PFN.

Note that it may be possible to optimize the function
check_page_physically_contiguous to avoid looping over every Xen PFN
for local memory. Although I will let this optimization for a follow-up.

Signed-off-by: Julien Grall 
Cc: Stefano Stabellini 
Cc: Russell King 
Cc: Konrad Rzeszutek Wilk 
Cc: Boris Ostrovsky 
Cc: David Vrabel 
---
 arch/arm/include/asm/xen/page-coherent.h | 26 +
 arch/arm/xen/mm.c| 38 ++-
 drivers/xen/swiotlb-xen.c| 39 
 3 files changed, 63 insertions(+), 40 deletions(-)

diff --git a/arch/arm/include/asm/xen/page-coherent.h 
b/arch/arm/include/asm/xen/page-coherent.h
index efd5624..0375c8c 100644
--- a/arch/arm/include/asm/xen/page-coherent.h
+++ b/arch/arm/include/asm/xen/page-coherent.h
@@ -35,11 +35,15 @@ static inline void xen_dma_map_page(struct device *hwdev, 
struct page *page,
 dma_addr_t dev_addr, unsigned long offset, size_t size,
 enum dma_data_direction dir, struct dma_attrs *attrs)
 {
-   bool local = PFN_DOWN(dev_addr) == page_to_pfn(page);
-   /* Dom0 is mapped 1:1, so if pfn == mfn the page is local otherwise
-* is a foreign page grant-mapped in dom0. If the page is local we
-* can safely call the native dma_ops function, otherwise we call
-* the xen specific function. */
+   bool local = XEN_PFN_DOWN(dev_addr) == page_to_xen_pfn(page);
+   /*
+* Dom0 is mapped 1:1, while the Linux page can be spanned accross
+* multiple Xen page, it's not possible to have a mix of local and
+* foreign Xen page. So if the first xen_pfn == mfn the page is local
+* otherwise it's a foreign page grant-mapped in dom0. If the page is
+* local we can safely call the native dma_ops function, otherwise we
+* call the xen specific function.
+*/
if (local)
__generic_dma_ops(hwdev)->map_page(hwdev, page, offset, size, 
dir, attrs);
else
@@ -51,10 +55,14 @@ static inline void xen_dma_unmap_page(struct device *hwdev, 
dma_addr_t handle,
struct dma_attrs *attrs)
 {
unsigned long pfn = PFN_DOWN(handle);
-   /* Dom0 is mapped 1:1, so calling pfn_valid on a foreign mfn will
-* always return false. If the page is local we can safely call the
-* native dma_ops function, otherwise we call the xen specific
-* function. */
+   /*
+* Dom0 is mapped 1:1, while the Linux page can be spanned accross
+* multiple Xen page, it's not possible to have a mix of local and
+* foreign Xen page. Dom0 is mapped 1:1, so calling pfn_valid on a
+* foreign mfn will always return false. If the page is local we can
+* safely call the native dma_ops function, otherwise we call the xen
+* specific function.
+*/
if (pfn_valid(pfn)) {
if (__generic_dma_ops(hwdev)->unmap_page)
__generic_dma_ops(hwdev)->unmap_page(hwdev, handle, 
size, dir, attrs);
diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
index 7b517e91..7c34f71 100644
--- a/arch/arm/xen/mm.c
+++ b/arch/arm/xen/mm.c
@@ -48,22 +48,22 @@ static void dma_cache_maint(dma_addr_t handle, unsigned 
long offset,
size_t size, enum dma_data_direction dir, enum dma_cache_op op)
 {
struct gnttab_cache_flush cflush;
-   unsigned long pfn;
+   unsigned long xen_pfn;
size_t left = size;
 
-   pfn = (handle >> PAGE_SHIFT) + offset / PAGE_SIZE;
-   offset %= PAGE_SIZE;
+   xen_pfn = (handle >> XEN_PAGE_SHIFT) + offset / XEN_PAGE_SIZE;
+   offset %= XEN_PAGE_SIZE;
 
do {
size_t len = left;

/* buffers in highmem or foreign pages cannot cross page
 * boundaries */
-   if (len + offset > PAGE_SIZE)
-   len = PAGE_SIZE - offset;
+   if (len + offset > XEN_PAGE_SIZE)
+   len = XEN_PAGE_SIZE - offset;
 
cflush.op = 0;
-   cflush.a.dev_bus_addr = pfn << PAGE_SHIFT;
+

[Xen-devel] [PATCH 1/2] xen/swiotlb: Pass addresses rather than frame numbers to xen_arch_need_swiotlb

[Julien Grall]

With 64KB page granularity support, the frame number will be different.

It will be easier to modify the behavior in a single place rather than
in each caller.

Signed-off-by: Julien Grall 

---
Cc: Stefano Stabellini 
Cc: Russell King 
Cc: Konrad Rzeszutek Wilk 
Cc: Boris Ostrovsky 
Cc: David Vrabel 
Cc: Thomas Gleixner 
Cc: Ingo Molnar 
Cc: "H. Peter Anvin" 
Cc: x...@kernel.org
---
 arch/arm/include/asm/xen/page.h | 4 ++--
 arch/arm/xen/mm.c   | 7 +--
 arch/x86/include/asm/xen/page.h | 4 ++--
 drivers/xen/swiotlb-xen.c   | 4 ++--
 4 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/arch/arm/include/asm/xen/page.h b/arch/arm/include/asm/xen/page.h
index e3d94cf..415dbc6 100644
--- a/arch/arm/include/asm/xen/page.h
+++ b/arch/arm/include/asm/xen/page.h
@@ -115,8 +115,8 @@ static inline bool set_phys_to_machine(unsigned long pfn, 
unsigned long mfn)
 #define xen_unmap(cookie) iounmap((cookie))
 
 bool xen_arch_need_swiotlb(struct device *dev,
-  unsigned long pfn,
-  unsigned long bfn);
+  phys_addr_t phys,
+  dma_addr_t dev_addr);
 unsigned long xen_get_swiotlb_free_pages(unsigned int order);
 
 #endif /* _ASM_ARM_XEN_PAGE_H */
diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
index 6dd911d..7b517e91 100644
--- a/arch/arm/xen/mm.c
+++ b/arch/arm/xen/mm.c
@@ -138,9 +138,12 @@ void __xen_dma_sync_single_for_device(struct device *hwdev,
 }
 
 bool xen_arch_need_swiotlb(struct device *dev,
-  unsigned long pfn,
-  unsigned long bfn)
+  phys_addr_t phys,
+  dma_addr_t dev_addr)
 {
+   unsigned long pfn = PFN_DOWN(phys);
+   unsigned long bfn = PFN_DOWN(dev_addr);
+
return (!hypercall_cflush && (pfn != bfn) && 
!is_device_dma_coherent(dev));
 }
 
diff --git a/arch/x86/include/asm/xen/page.h b/arch/x86/include/asm/xen/page.h
index 2a05e691..84c4757 100644
--- a/arch/x86/include/asm/xen/page.h
+++ b/arch/x86/include/asm/xen/page.h
@@ -296,8 +296,8 @@ void make_lowmem_page_readwrite(void *vaddr);
 #define xen_unmap(cookie) iounmap((cookie))
 
 static inline bool xen_arch_need_swiotlb(struct device *dev,
-unsigned long pfn,
-unsigned long bfn)
+phys_addr_t phys,
+dma_addr_t dev_addr);
 {
return false;
 }
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index d757a3e..cfe755d 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -398,7 +398,7 @@ dma_addr_t xen_swiotlb_map_page(struct device *dev, struct 
page *page,
 */
if (dma_capable(dev, dev_addr, size) &&
!range_straddles_page_boundary(phys, size) &&
-   !xen_arch_need_swiotlb(dev, PFN_DOWN(phys), PFN_DOWN(dev_addr)) 
&&
+   !xen_arch_need_swiotlb(dev, phys, dev_addr) &&
!swiotlb_force) {
/* we are not interested in the dma_addr returned by
 * xen_dma_map_page, only in the potential cache flushes 
executed
@@ -557,7 +557,7 @@ xen_swiotlb_map_sg_attrs(struct device *hwdev, struct 
scatterlist *sgl,
dma_addr_t dev_addr = xen_phys_to_bus(paddr);
 
if (swiotlb_force ||
-   xen_arch_need_swiotlb(hwdev, PFN_DOWN(paddr), 
PFN_DOWN(dev_addr)) ||
+   xen_arch_need_swiotlb(hwdev, paddr, dev_addr) ||
!dma_capable(hwdev, dev_addr, sg->length) ||
range_straddles_page_boundary(paddr, sg->length)) {
phys_addr_t map = swiotlb_tbl_map_single(hwdev,
-- 
2.1.4


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

Re: [Xen-devel] [PATCH V2] x86/hvm: fix saved pmtimer value

[Tim Deegan]

Hi,

At 15:06 +0900 on 10 Sep (1441897575), Kouya Shimura wrote:
> The ACPI PM timer is sometimes broken on live migration.
> Since vcpu->arch.hvm_vcpu.guest_time is always zero in other than
> "delay for missed ticks mode". Even in "delay for missed ticks mode",
> vcpu's guest_time field is not valid (i.e. zero) when
> the state of vcpu is "blocked". (see pt_save_timer function)

Nice catch, thanks!

> The original author (Tim Deegan) of pmtimer_save() must have
> intended that it saves the last scheduled time of the
> vcpu. Unfortunately it was already implied this bug. FYI, there is
> no other timer mode than "delay for missed ticks mode" then.  For
> consistency with HPET, pmtimer_save() should refer
> hvm_get_guest_time() to update the counter as well as hpet_save()
> does.

Using hvm_get_guest_time() here means the guest will see a time jump
for the delay between being paused and the HVM state being saved.
OTOH, fixing it so that this always uses the pause time (e.g. by
making pt_freeze_time() always save the time as it used to) means the
guest will see a time difference between PMTIMER and other HVM state
(e.g. HPET).

So I think this change is fine.  But please update the comment right
above it that talks about using the saved time!

Cheers,

Tim.

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

Re: [Xen-devel] [PATCH for 4.6 v2 2/3] libxc: introduce xc_domain_getvnuma

[Ian Campbell]

On Wed, 2015-09-09 at 18:03 +0100, Wei Liu wrote:
> A simple wrapper for XENMEM_get_vnumainfo.
> 
> Signed-off-by: Wei Liu 

Acked-by: Ian Campbell 

Some comments which are not specific to this patch:
 
> +int xc_domain_getvnuma(xc_interface *xch,
> +   uint32_t domid,
> +   uint32_t *nr_vnodes,
> +   uint32_t *nr_vmemranges,
> +   uint32_t *nr_vcpus,
> +   xen_vmemrange_t *vmemrange,
> +   unsigned int *vdistance,
> +   unsigned int *vcpu_to_vnode)
> +{
> +int rc;
> +DECLARE_HYPERCALL_BOUNCE(vmemrange, sizeof(*vmemrange) * *nr_vmemranges,
> + XC_HYPERCALL_BUFFER_BOUNCE_OUT);
> +DECLARE_HYPERCALL_BOUNCE(vdistance, sizeof(*vdistance) *
> + *nr_vnodes * *nr_vnodes,
> + XC_HYPERCALL_BUFFER_BOUNCE_OUT);
> +DECLARE_HYPERCALL_BOUNCE(vcpu_to_vnode, sizeof(*vcpu_to_vnode) * 
> *nr_vcpus,
> + XC_HYPERCALL_BUFFER_BOUNCE_OUT);
> +
> +struct xen_vnuma_topology_info vnuma_topo;
> +
> +if ( xc_hypercall_bounce_pre(xch, vmemrange)  ||
> + xc_hypercall_bounce_pre(xch, vdistance)  ||
> + xc_hypercall_bounce_pre(xch, vcpu_to_vnode) )
> +{
> +rc = -1;
> +errno = ENOMEM;

xc_hypercall_bounce_pre really ought to set errno, but it (to my surprise)
doesn't appear to do so...

> +goto vnumaget_fail;
> +}
> +
> +set_xen_guest_handle(vnuma_topo.vmemrange.h, vmemrange);
> +set_xen_guest_handle(vnuma_topo.vdistance.h, vdistance);
> +set_xen_guest_handle(vnuma_topo.vcpu_to_vnode.h, vcpu_to_vnode);
> +
> +vnuma_topo.nr_vnodes = *nr_vnodes;
> +vnuma_topo.nr_vcpus = *nr_vcpus;
> +vnuma_topo.nr_vmemranges = *nr_vmemranges;
> +vnuma_topo.domid = domid;
> +vnuma_topo.pad = 0;
> +
> +rc = do_memory_op(xch, XENMEM_get_vnumainfo, _topo,
> +  sizeof(vnuma_topo));
> +
> +*nr_vnodes = vnuma_topo.nr_vnodes;
> +*nr_vcpus = vnuma_topo.nr_vcpus;
> +*nr_vmemranges = vnuma_topo.nr_vmemranges;

We're a bit inconsistent, it seems, about a) tolerating such parameters
being NULL and b) whether or not we update them when the hypercall failed.

Anyway, neither of those are anything to block this patch over, so ack as
above.

> +
> + vnumaget_fail:
> +xc_hypercall_bounce_post(xch, vmemrange);
> +xc_hypercall_bounce_post(xch, vdistance);
> +xc_hypercall_bounce_post(xch, vcpu_to_vnode);
> +
> +return rc;
> +}
> +
>  /*
>   * Local variables:
>   * mode: C,

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

Re: [Xen-devel] [PULL 0/19] xen-2015-09-08-tag

[Stefano Stabellini]

On Thu, 10 Sep 2015, Michael S. Tsirkin wrote:
> On Thu, Sep 10, 2015 at 11:29:18AM +0100, Stefano Stabellini wrote:
> > CC Michael
> > 
> > On Thu, 10 Sep 2015, Stefano Stabellini wrote:
> > > On Thu, 10 Sep 2015, Chen, Tiejun wrote:
> > > > > xen-host-pci-device.c is only compiled if CONFIG_XEN_PCI_PASSTHROUGH
> > > > > was set by configure. That won't be the case on OSX or Windows, where
> > > > > the Xen headers don't exist.
> > > > > 
> > > > 
> > > > Okay. This actually shouldn't be enabled on Windows so what about this?
> > > 
> > > I think it would be nicer to replace the pread than introducing ifdefs.
> > 
> > Something like:
> > 
> > ---
> > Replace pread with read to avoid build breakages on Windows
> > 
> > Signed-off-by: Stefano Stabellini 
> 
> I'd prefer a wrapper that does the right thing.
> No sense in doubling the # of system calls for everyone.

If this was done on an hot path I would agree with you, but it is just
one call at initialization time (igd_pt_i440fx_initfn).


> > diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> > index 58a33fb..9a40429 100644
> > --- a/hw/pci-host/piix.c
> > +++ b/hw/pci-host/piix.c
> > @@ -774,8 +774,11 @@ static int host_pci_config_read(int pos, int len, 
> > uint32_t val)
> >  return -ENODEV;
> >  }
> >  
> > +if (lseek(config_fd, pos, SEEK_SET) != pos) {
> > +return -errno;
> > +}
> >  do {
> > -rc = pread(config_fd, (uint8_t *), len, pos);
> > +rc = read(config_fd, (uint8_t *), len);
> >  } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
> >  if (rc != len) {
> >  return -errno;
> 

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

Re: [Xen-devel] [PATCH for 4.6] configure: don't silently disable systemd support

[Ian Campbell]

On Wed, 2015-09-09 at 23:35 +0100, Wei Liu wrote:
> Originally when user runs ./configure --enable-systemd and systemd
> development library is not available the build system silently disables
> systemd support. This is not in line with normal expectation.
> 
> Instead, configure should error out when user has asked for systemd
> support but development libraries can't be found.
> 
> Reported-by: George Dunlap 
> Signed-off-by: Wei Liu 
> ---
> Please rerun ./autogen.sh.
> ---
>  m4/systemd.m4 | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/m4/systemd.m4 b/m4/systemd.m4
> index 8284993..84508a1 100644
> --- a/m4/systemd.m4
> +++ b/m4/systemd.m4
> @@ -85,7 +85,11 @@ AC_DEFUN([AX_CHECK_SYSTEMD], [
>   AC_DEFINE([HAVE_SYSTEMD], [1], [Systemd available and
> enabled])
>   systemd=y
>   AX_CHECK_SYSTEMD_LIBS()
> - ],[systemd=n])
> + ],[
> +>>   > AS_IF([test "x$enable_systemd" != "x"],

At this point the value of enable_systemd is either empty or ...what...? I
think "yes"?, with "no" having been ruled out earlier?

Or can it be just "y"?

I could live with this version, but I think it would be less of a brain ben
der if the check was inverted to be "x$enable_systemd" = "xyes" i.e. more
explicit.

Ian.


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

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

[Roger Pau Monné]

El 10/09/15 a les 13.48, Julien Grall ha escrit:
> On 10/09/15 12:32, Andrew Turner wrote:
>> On Thu, 10 Sep 2015 16:41:56 +0800
>> Shannon Zhao  wrote:
>>
>>> From: Shannon Zhao 
>>>
>>> These EFI stub parameters are used to internal communication between
>>> EFI stub and Linux kernel and EFI stub creates these parameters. But
>>> for Xen on ARM when booting with UEFI, Xen will create a minimal DT
>>> providing these parameters for Dom0 and Dom0 is not only Linux
>>> kernel, but also other OS (such as FreeBSD) which will be used in the
>>> future. So here we plan to standardize the names by dropping the
>>> prefix "linux," and make them common to other OS. Also this will not
>>> break the compatibility since these parameters are used to internal
>>> communication between EFI stub and kernel.
>>
>> It is unlikely FreeBSD will use this property when booting ad Xen dom0.
>> The kernel expects to be passed a data structure to find boot this
>> information.
>>
>> My preference would be to have the FreeBSD loader load the xen binary,
>> the FreeBSD kernel, and set up these data structs before passing
>> control to Xen, with the information it needs to boot the kernel. My
>> understanding is this is how it is done on x86.
> 
> Well, AFAICT, there is no FreeBSD specific code in Xen for x86. We are
> faking a multiboot module which contains all the informations.
> 
> I know that the bootloader is at least involved, I don't remember if
> there is some code in FreeBSD to read this boot module. I've CCed Roger
> to confirm.

The metadata/modules needed by the FreeBSD Dom0 kernel is generated by
the native FreeBSD bootloader (as would be done when booting bare
metal). Then this blob is passed as a multiboot module in the same place
that Linux puts it's initramfs. Xen simply copies this blob straight
into guest memory and sets start_info.mod_start to point to the start
memory address of this blob.

I've done it this way because I don't see many other options. Xen is
tailored for Linux and only allows passing one module to the Dom0 kernel
(initramfs). For FreeBSD it would be good to be able to pass at least
two modules to the Dom0 kernel, one containing the metadata, and the
other one containing the modules themselves. The new PVH work (HVMlite
or whatever) will hopefully allow passing more than one module to the
Dom0 kernel, and should make the code in the FreeBSD loader simpler.

Roger.


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

Re: [Xen-devel] [PATCH v6 16/18] vmx: Add some scheduler hooks for VT-d posted interrupts

[Wu, Feng]

> -Original Message-
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: Thursday, September 10, 2015 6:01 PM
> To: Wu, Feng
> Cc: Andrew Cooper; Dario Faggioli; George Dunlap; Tian, Kevin;
> xen-devel@lists.xen.org; Keir Fraser
> Subject: RE: [PATCH v6 16/18] vmx: Add some scheduler hooks for VT-d posted
> interrupts
> 
> >>> On 10.09.15 at 11:41,  wrote:
> >> From: Jan Beulich [mailto:jbeul...@suse.com]
> >> Sent: Thursday, September 10, 2015 5:26 PM
> >> >>> On 10.09.15 at 10:59,  wrote:
> >> > First, how to check it while waiting to acquire the lock .pi_block_cpu
> >> > didn't change?
> >>
> >> Note the difference between "check while waiting" and "check that
> >> while waiting": The former is indeed hard to implement, while the
> >> latter is pretty straightforward (and we do so elsewhere).
> >>
> >> > Secondly, even if we can check it, what should we do if .pi_block_cpu
> >> > is changed after acquiring the lock as I mentioned above?
> >>
> >> Drop the lock and start over. I.e. (taking your pseudo code)
> >>
> >> restart:
> >> local_pi_block_cpu = ...;
> >> bail-if-invalid (e.g. -1 in current model)
> >> spin_lock_irqsave(_cpu(, local_pi_block_cpu), flags);
> >> if(local_pi_block_cpu != actual_pi_block_cpu) {
> >> spin_unlock_irqrestore(_cpu(,local_pi_block_cpu), flags);
> >> goto restart;
> >> }
> >
> > Thanks a lot for showing me this pseudo code! My concern is if
> > .pi_block_vcpu is changed to -1 at this point, it doesn't work.
> > .pi_block_vcpu being -1 here means the vCPU is remove from
> > the blocking list by others, then we cannot delete it again via
> > list_del() here.
> 
> Did you miss the "bail-if-invalid" above?

I am sorry, do I miss something here? If .pi_block_cpu becomes
-1 here (after the above 'if' statement is finished with
local_pi_block_cpu == actual_pi_block_cpu ), how can "bail-if-invalid"
above help?

Thanks,
Feng

> 
> > BTW, I cannot see performance overhead for list_del_init()
> > compared to list_del().
> >
> > list_del():
> > static inline void list_del(struct list_head *entry)
> > {
> > ASSERT(entry->next->prev == entry);
> > ASSERT(entry->prev->next == entry);
> > __list_del(entry->prev, entry->next);
> > entry->next = LIST_POISON1;
> > entry->prev = LIST_POISON2;
> > }
> >
> > list_del_init():
> > static inline void list_del_init(struct list_head *entry)
> > {
> > __list_del(entry->prev, entry->next);
> > INIT_LIST_HEAD(entry);
> > }
> 
> Well, yes, both do two stores (I forgot about the poisoning), but
> arguably the poisoning could become a debug-build-only thing. I.e.
> it is an implementation detail that the number of stores currently
> is the same. From an abstract perspective one should still prefer
> list_del() when the re-init isn't really needed. And in the specific
> case here asking you to use list_del() makes sure the code ends
> up not even trying the deletion when not needed.
> 
> Jan


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

Re: [Xen-devel] [PATCH v6 16/18] vmx: Add some scheduler hooks for VT-d posted interrupts

[Jan Beulich]

>>> On 10.09.15 at 15:27,  wrote:

> 
>> -Original Message-
>> From: Jan Beulich [mailto:jbeul...@suse.com]
>> Sent: Thursday, September 10, 2015 9:15 PM
>> To: Wu, Feng
>> Cc: Andrew Cooper; Dario Faggioli; George Dunlap; Tian, Kevin;
>> xen-devel@lists.xen.org; Keir Fraser
>> Subject: RE: [PATCH v6 16/18] vmx: Add some scheduler hooks for VT-d posted
>> interrupts
>> 
>> >>> On 10.09.15 at 14:58,  wrote:
>> 
>> >
>> >> -Original Message-
>> >> From: Jan Beulich [mailto:jbeul...@suse.com]
>> >> Sent: Thursday, September 10, 2015 8:45 PM
>> >> To: Wu, Feng
>> >> Cc: Andrew Cooper; Dario Faggioli; George Dunlap; Tian, Kevin;
>> >> xen-devel@lists.xen.org; Keir Fraser
>> >> Subject: RE: [PATCH v6 16/18] vmx: Add some scheduler hooks for VT-d
>> posted
>> >> interrupts
>> >>
>> >> >>> On 10.09.15 at 14:34,  wrote:
>> >>
>> >> >
>> >> >> -Original Message-
>> >> >> From: Jan Beulich [mailto:jbeul...@suse.com]
>> >> >> Sent: Thursday, September 10, 2015 6:01 PM
>> >> >> To: Wu, Feng
>> >> >> Cc: Andrew Cooper; Dario Faggioli; George Dunlap; Tian, Kevin;
>> >> >> xen-devel@lists.xen.org; Keir Fraser
>> >> >> Subject: RE: [PATCH v6 16/18] vmx: Add some scheduler hooks for VT-d
>> >> posted
>> >> >> interrupts
>> >> >>
>> >> >> >>> On 10.09.15 at 11:41,  wrote:
>> >> >> >> From: Jan Beulich [mailto:jbeul...@suse.com]
>> >> >> >> Sent: Thursday, September 10, 2015 5:26 PM
>> >> >> >> >>> On 10.09.15 at 10:59,  wrote:
>> >> >> >> > First, how to check it while waiting to acquire the lock 
>> >> >> >> > .pi_block_cpu
>> >> >> >> > didn't change?
>> >> >> >>
>> >> >> >> Note the difference between "check while waiting" and "check that
>> >> >> >> while waiting": The former is indeed hard to implement, while the
>> >> >> >> latter is pretty straightforward (and we do so elsewhere).
>> >> >> >>
>> >> >> >> > Secondly, even if we can check it, what should we do if 
>> >> >> >> > .pi_block_cpu
>> >> >> >> > is changed after acquiring the lock as I mentioned above?
>> >> >> >>
>> >> >> >> Drop the lock and start over. I.e. (taking your pseudo code)
>> >> >> >>
>> >> >> >> restart:
>> >> >> >> local_pi_block_cpu = ...;
>> >> >> >> bail-if-invalid (e.g. -1 in current model)
>> >> >> >> spin_lock_irqsave(_cpu(, local_pi_block_cpu), flags);
>> >> >> >> if(local_pi_block_cpu != actual_pi_block_cpu) {
>> >> >> >> spin_unlock_irqrestore(_cpu(,local_pi_block_cpu),
>> flags);
>> >> >> >> goto restart;
>> >> >> >> }
>> >> >> >
>> >> >> > Thanks a lot for showing me this pseudo code! My concern is if
>> >> >> > .pi_block_vcpu is changed to -1 at this point, it doesn't work.
>> >> >> > .pi_block_vcpu being -1 here means the vCPU is remove from
>> >> >> > the blocking list by others, then we cannot delete it again via
>> >> >> > list_del() here.
>> >> >>
>> >> >> Did you miss the "bail-if-invalid" above?
>> >> >
>> >> > I am sorry, do I miss something here? If .pi_block_cpu becomes
>> >> > -1 here (after the above 'if' statement is finished with
>> >> > local_pi_block_cpu == actual_pi_block_cpu ), how can "bail-if-invalid"
>> >> > above help?
>> >>
>> >> The (obvious I thought) implication is that all assignments to
>> >> pi_block_cpu (along with all list manipulations) now need to happen
>> >> with the lock held.
>> >
>> > If all the assignment to pi_block_cpu is with one lock held, I don't think
>> > we need to above checkout, we can safely use .pi_block_cpu, right?
>> 
>> No. In order to use it you need to make sure it's valid (or else
>> there's no valid lock to acquire). And once you acquired the
>> lock, you have to check whether changed / became invalid.
> 
> If all the assignment to .pi_block_cpu is with one lock held,
> how can it get changed / become invalid then once I acquired
> the lock?

Again - if pi_block_cpu is invalid, which lock do you want to acquire?

Jan


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

[Xen-devel] [PATCH for 4.6 v3 1/3] libxc: introduce xc_domain_getvnuma

[Wei Liu]

A simple wrapper for XENMEM_get_vnumainfo.

Signed-off-by: Wei Liu 
Acked-by: Ian Campbell 
---
 tools/libxc/include/xenctrl.h | 18 +++
 tools/libxc/xc_domain.c   | 54 +++
 2 files changed, 72 insertions(+)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 2000f12..37205c2 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1287,6 +1287,24 @@ int xc_domain_setvnuma(xc_interface *xch,
 unsigned int *vdistance,
 unsigned int *vcpu_to_vnode,
 unsigned int *vnode_to_pnode);
+/*
+ * Retrieve vnuma configuration
+ * domid: IN, target domid
+ * nr_vnodes: IN/OUT, number of vnodes, not NULL
+ * nr_vmemranges: IN/OUT, number of vmemranges, not NULL
+ * nr_vcpus: IN/OUT, number of vcpus, not NULL
+ * vmemranges: OUT, an array which has length of nr_vmemranges
+ * vdistance: OUT, an array which has length of nr_vnodes * nr_vnodes
+ * vcpu_to_vnode: OUT, an array which has length of nr_vcpus
+ */
+int xc_domain_getvnuma(xc_interface *xch,
+   uint32_t domid,
+   uint32_t *nr_vnodes,
+   uint32_t *nr_vmemranges,
+   uint32_t *nr_vcpus,
+   xen_vmemrange_t *vmemrange,
+   unsigned int *vdistance,
+   unsigned int *vcpu_to_vnode);
 
 #if defined(__i386__) || defined(__x86_64__)
 /*
diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
index 780797f..09ef748 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -2493,6 +2493,60 @@ int xc_domain_setvnuma(xc_interface *xch,
 return rc;
 }
 
+int xc_domain_getvnuma(xc_interface *xch,
+   uint32_t domid,
+   uint32_t *nr_vnodes,
+   uint32_t *nr_vmemranges,
+   uint32_t *nr_vcpus,
+   xen_vmemrange_t *vmemrange,
+   unsigned int *vdistance,
+   unsigned int *vcpu_to_vnode)
+{
+int rc;
+DECLARE_HYPERCALL_BOUNCE(vmemrange, sizeof(*vmemrange) * *nr_vmemranges,
+ XC_HYPERCALL_BUFFER_BOUNCE_OUT);
+DECLARE_HYPERCALL_BOUNCE(vdistance, sizeof(*vdistance) *
+ *nr_vnodes * *nr_vnodes,
+ XC_HYPERCALL_BUFFER_BOUNCE_OUT);
+DECLARE_HYPERCALL_BOUNCE(vcpu_to_vnode, sizeof(*vcpu_to_vnode) * *nr_vcpus,
+ XC_HYPERCALL_BUFFER_BOUNCE_OUT);
+
+struct xen_vnuma_topology_info vnuma_topo;
+
+if ( xc_hypercall_bounce_pre(xch, vmemrange)  ||
+ xc_hypercall_bounce_pre(xch, vdistance)  ||
+ xc_hypercall_bounce_pre(xch, vcpu_to_vnode) )
+{
+rc = -1;
+errno = ENOMEM;
+goto vnumaget_fail;
+}
+
+set_xen_guest_handle(vnuma_topo.vmemrange.h, vmemrange);
+set_xen_guest_handle(vnuma_topo.vdistance.h, vdistance);
+set_xen_guest_handle(vnuma_topo.vcpu_to_vnode.h, vcpu_to_vnode);
+
+vnuma_topo.nr_vnodes = *nr_vnodes;
+vnuma_topo.nr_vcpus = *nr_vcpus;
+vnuma_topo.nr_vmemranges = *nr_vmemranges;
+vnuma_topo.domid = domid;
+vnuma_topo.pad = 0;
+
+rc = do_memory_op(xch, XENMEM_get_vnumainfo, _topo,
+  sizeof(vnuma_topo));
+
+*nr_vnodes = vnuma_topo.nr_vnodes;
+*nr_vcpus = vnuma_topo.nr_vcpus;
+*nr_vmemranges = vnuma_topo.nr_vmemranges;
+
+ vnumaget_fail:
+xc_hypercall_bounce_post(xch, vmemrange);
+xc_hypercall_bounce_post(xch, vdistance);
+xc_hypercall_bounce_post(xch, vcpu_to_vnode);
+
+return rc;
+}
+
 /*
  * Local variables:
  * mode: C
-- 
2.1.4


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

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

[Jan Beulich]

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

Because there could be new entries in SystemTable->RuntimeServices
(expected and blindly but validly called by the kernel). Even worse
(because likely harder to deal with) would be new fields in other
structures.

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

Not necessarily - we don't expose the memory map (we'd have to
if we were to mimic EFI for Dom0), and hence the mentioned issue
doesn't exist in our model.

Jan


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

Re: [Xen-devel] [PATCH for 4.6 v2 3/3] xl/libxl: disallow saving a guest with vNUMA configured

[Wei Liu]

On Wed, Sep 09, 2015 at 01:33:15PM -0400, Boris Ostrovsky wrote:
> On 09/09/2015 01:29 PM, Wei Liu wrote:
> >On Wed, Sep 09, 2015 at 01:17:40PM -0400, Boris Ostrovsky wrote:
> >>On 09/09/2015 01:03 PM, Wei Liu wrote:
> >>>This is due to migration v2 frame record doesn't contain node
> >>>information.
> >>>
> >>>Signed-off-by: Wei Liu 
> >>>---
> >>>Cc: andrew.coop...@citrix.com
> >>>---
> >>>  docs/man/xl.cfg.pod.5   |  2 ++
> >>>  tools/libxl/libxl_dom.c | 14 ++
> >>>  2 files changed, 16 insertions(+)
> >>>
> >>>diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> >>>index 80e51bb..dbd0700 100644
> >>>--- a/docs/man/xl.cfg.pod.5
> >>>+++ b/docs/man/xl.cfg.pod.5
> >>>@@ -263,6 +263,8 @@ virtual node.
> >>>  Note that virtual NUMA for PV guest is not yet supported, because
> >>>  there is an issue with cpuid handling that affects PV virtual NUMA.
> >>>+Further more, guest with virtual NUMA cannot be saved or migrated
> >>>+because node information of guest frames is not preserved.
> >>Should we also issue a warning during startup if nomigrate is not set?
> >>
> >
> >
> >Ah, there is such option. I can certainly give a warning. The only
> >problem is it seems to be stale in libxl, there is no code that checks
> >that! I can still migrate a guest even with nomigrate set to true.
> >
> >Maybe someone with more knowledge about that option can lecture me on
> >what that does? The manpage says it *enables* certain feature? Does that
> >actually mean *make available*?
> 
> IIRC it was added to allow/control certain TSC modes (see domain_cpuid()).
> 

To the best of my knowledge the nomigrate option is both semantically
and functionally broken.

I'm not very inclined to print a warning because setting that option
won't provide any protection / added value.

Wei.

> -boris
> 

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

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

[Mark Rutland]

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

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

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

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

Thanks,
Mark.

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

Re: [Xen-devel] [PATCH 7/7] tools/hotplug: add wrapper to start xenstored

[Wei Liu]

On Thu, Sep 10, 2015 at 03:19:35PM +0100, George Dunlap wrote:
> On Wed, Jan 7, 2015 at 9:40 AM, Olaf Hering  wrote:
> > On Tue, Jan 06, Ian Campbell wrote:
> >
> >> On Fri, 2014-12-19 at 12:25 +0100, Olaf Hering wrote:
> >> > The shell wrapper in xenstored.service does not handle XENSTORE_TRACE.
> >> >
> >> > Create a separate wrapper script which is used in the sysv runlevel
> >> > script and in systemd xenstored.service. It preserves existing
> >> > behaviour by handling the XENSTORE_TRACE boolean. It also implements
> >> > the handling of XENSTORED_ARGS=. This variable has to be added to
> >> > sysconfig/xencommons.
> >>
> >> Why don't we just drop XENSTORED_* in favour of XENSTORED_ARGS (with an
> >> example in the sysconfig file of enabling tracing if you like)?
> >
> > After having two weeks to think about this I came to the same
> > conclusion. I think whatever the outcome is, the boolean should be
> > removed. The sysconfig file should get a XENSTORED_ARGS="" along with a
> > help text which mentions "-T /path" and "xenstored --help" to get other
> > options because there is no man page.
> >
> >> Going to a wrapper script just to make some fairly uncommon debugging
> >> option marginally more convenient seems like overkill to me, plus
> >> XENSTORED_ARGS would allow for passing other useful options to
> >> xenstored.
> >
> > If I recall correctly the point of the current 'sh -c "exec ..."' stunt
> > was to expand the XENSTORE variable from the sysconfig file. But this
> > approach leads to failures with SELinux because the socket passing does
> > not work this way. Up to now I have not seen a success report for
> > selinux+systemd+xenstored. Maybe its already somewhere in the other
> > unread mails.
> >
> > In my cover letter I provided some possible ways to handle
> > selinux+systemd+xenstored. Ideally the approach "Exec=/usr/bin/env
> > $XENSTORED --no-fork $XENSTORED_ARGS" works because it means its
> > possible to select a binary via the sysconfig file. But it also means
> > the XENSTORE_TRACE boolean has to be removed in favour of the plain
> > XENSTORED_ARGS= approach mentioned above. Finally this would avoid the
> > need for a wrapper script.
> >
> > Hopefully someone with access to a SELinux enabled system will report
> > which approach actually works.
> 
> I can confirm that
> 
> ExecStart=/bin/sh -c "exec $XENSTORED --no-fork $XENSTORED_ARGS"
> 
> does not work on a CentOS7 system with selinux enabled, but that
> 
> ExecStart=/usr/bin/env $XENSTORED --no-fork $XENSTORED_ARGS
> 

And the difference between these two approaches is /usr/bin/env
preserves envars while sh -c doesn't?

Wei.

> does work.
> 
> A patch is on its way.
> 
>  -George

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

Re: [Xen-devel] [PATCH 7/7] tools/hotplug: add wrapper to start xenstored

[Wei Liu]

On Thu, Sep 10, 2015 at 04:01:06PM +0100, M A Young wrote:
> On Thu, 10 Sep 2015, Wei Liu wrote:
> 
> > On Thu, Sep 10, 2015 at 03:19:35PM +0100, George Dunlap wrote:
> > > On Wed, Jan 7, 2015 at 9:40 AM, Olaf Hering  wrote:
> > > > On Tue, Jan 06, Ian Campbell wrote:
> > > >
> > > >> On Fri, 2014-12-19 at 12:25 +0100, Olaf Hering wrote:
> > > >> > The shell wrapper in xenstored.service does not handle 
> > > >> > XENSTORE_TRACE.
> > > >> >
> > > >> > Create a separate wrapper script which is used in the sysv runlevel
> > > >> > script and in systemd xenstored.service. It preserves existing
> > > >> > behaviour by handling the XENSTORE_TRACE boolean. It also implements
> > > >> > the handling of XENSTORED_ARGS=. This variable has to be added to
> > > >> > sysconfig/xencommons.
> > > >>
> > > >> Why don't we just drop XENSTORED_* in favour of XENSTORED_ARGS (with an
> > > >> example in the sysconfig file of enabling tracing if you like)?
> > > >
> > > > After having two weeks to think about this I came to the same
> > > > conclusion. I think whatever the outcome is, the boolean should be
> > > > removed. The sysconfig file should get a XENSTORED_ARGS="" along with a
> > > > help text which mentions "-T /path" and "xenstored --help" to get other
> > > > options because there is no man page.
> > > >
> > > >> Going to a wrapper script just to make some fairly uncommon debugging
> > > >> option marginally more convenient seems like overkill to me, plus
> > > >> XENSTORED_ARGS would allow for passing other useful options to
> > > >> xenstored.
> > > >
> > > > If I recall correctly the point of the current 'sh -c "exec ..."' stunt
> > > > was to expand the XENSTORE variable from the sysconfig file. But this
> > > > approach leads to failures with SELinux because the socket passing does
> > > > not work this way. Up to now I have not seen a success report for
> > > > selinux+systemd+xenstored. Maybe its already somewhere in the other
> > > > unread mails.
> > > >
> > > > In my cover letter I provided some possible ways to handle
> > > > selinux+systemd+xenstored. Ideally the approach "Exec=/usr/bin/env
> > > > $XENSTORED --no-fork $XENSTORED_ARGS" works because it means its
> > > > possible to select a binary via the sysconfig file. But it also means
> > > > the XENSTORE_TRACE boolean has to be removed in favour of the plain
> > > > XENSTORED_ARGS= approach mentioned above. Finally this would avoid the
> > > > need for a wrapper script.
> > > >
> > > > Hopefully someone with access to a SELinux enabled system will report
> > > > which approach actually works.
> > > 
> > > I can confirm that
> > > 
> > > ExecStart=/bin/sh -c "exec $XENSTORED --no-fork $XENSTORED_ARGS"
> > > 
> > > does not work on a CentOS7 system with selinux enabled, but that
> > > 
> > > ExecStart=/usr/bin/env $XENSTORED --no-fork $XENSTORED_ARGS
> > > 
> > 
> > And the difference between these two approaches is /usr/bin/env
> > preserves envars while sh -c doesn't?
> 
> /bin/sh doesn't give the right selinux permissions so xenstored doesn't 
> start. Presumably /usr/bin/env does.
> 

That is "what" but not "why". It could be env is special to selinux, it
could be the policy itself is written that way, it could be sh -c throws
away something that it shouldn't have.

Anyway, I'm just curious. Not going to block this effort just because I
don't understand this. I'm pretty sure if this approach is buggy / not
universal people who care enough will come back. :-)

Wei.

>   Michael Young

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

Re: [Xen-devel] [PATCH 7/7] tools/hotplug: add wrapper to start xenstored

[George Dunlap]

On Thu, Sep 10, 2015 at 4:01 PM, M A Young  wrote:
> On Thu, 10 Sep 2015, Wei Liu wrote:
>
>> On Thu, Sep 10, 2015 at 03:19:35PM +0100, George Dunlap wrote:
>> > On Wed, Jan 7, 2015 at 9:40 AM, Olaf Hering  wrote:
>> > > On Tue, Jan 06, Ian Campbell wrote:
>> > >
>> > >> On Fri, 2014-12-19 at 12:25 +0100, Olaf Hering wrote:
>> > >> > The shell wrapper in xenstored.service does not handle XENSTORE_TRACE.
>> > >> >
>> > >> > Create a separate wrapper script which is used in the sysv runlevel
>> > >> > script and in systemd xenstored.service. It preserves existing
>> > >> > behaviour by handling the XENSTORE_TRACE boolean. It also implements
>> > >> > the handling of XENSTORED_ARGS=. This variable has to be added to
>> > >> > sysconfig/xencommons.
>> > >>
>> > >> Why don't we just drop XENSTORED_* in favour of XENSTORED_ARGS (with an
>> > >> example in the sysconfig file of enabling tracing if you like)?
>> > >
>> > > After having two weeks to think about this I came to the same
>> > > conclusion. I think whatever the outcome is, the boolean should be
>> > > removed. The sysconfig file should get a XENSTORED_ARGS="" along with a
>> > > help text which mentions "-T /path" and "xenstored --help" to get other
>> > > options because there is no man page.
>> > >
>> > >> Going to a wrapper script just to make some fairly uncommon debugging
>> > >> option marginally more convenient seems like overkill to me, plus
>> > >> XENSTORED_ARGS would allow for passing other useful options to
>> > >> xenstored.
>> > >
>> > > If I recall correctly the point of the current 'sh -c "exec ..."' stunt
>> > > was to expand the XENSTORE variable from the sysconfig file. But this
>> > > approach leads to failures with SELinux because the socket passing does
>> > > not work this way. Up to now I have not seen a success report for
>> > > selinux+systemd+xenstored. Maybe its already somewhere in the other
>> > > unread mails.
>> > >
>> > > In my cover letter I provided some possible ways to handle
>> > > selinux+systemd+xenstored. Ideally the approach "Exec=/usr/bin/env
>> > > $XENSTORED --no-fork $XENSTORED_ARGS" works because it means its
>> > > possible to select a binary via the sysconfig file. But it also means
>> > > the XENSTORE_TRACE boolean has to be removed in favour of the plain
>> > > XENSTORED_ARGS= approach mentioned above. Finally this would avoid the
>> > > need for a wrapper script.
>> > >
>> > > Hopefully someone with access to a SELinux enabled system will report
>> > > which approach actually works.
>> >
>> > I can confirm that
>> >
>> > ExecStart=/bin/sh -c "exec $XENSTORED --no-fork $XENSTORED_ARGS"
>> >
>> > does not work on a CentOS7 system with selinux enabled, but that
>> >
>> > ExecStart=/usr/bin/env $XENSTORED --no-fork $XENSTORED_ARGS
>> >
>>
>> And the difference between these two approaches is /usr/bin/env
>> preserves envars while sh -c doesn't?
>
> /bin/sh doesn't give the right selinux permissions so xenstored doesn't
> start. Presumably /usr/bin/env does.

Or to be more precise, /bin/sh exec from that context doesn't allow
xenstored to make the accept() system call, so you get a log full of
these:

type=PROCTITLE msg=audit(1441892166.173:22242):
proctitle=2F7573722F7362696E2F78656E73746F726564002D2D6E6F2D666F726B
type=AVC msg=audit(1441892166.173:22243): avc:  denied  { accept } for
 pid=615 comm="xenstored" path="/run/xenstored/socket"
scontext=system_u:system_r:xenstored_t:s0
tcontext=system_u:system_r:initrc_t:s0 tclass=unix_stream_socket
permissive=0
type=SYSCALL msg=audit(1441892166.173:22243): arch=c03e syscall=43
success=no exit=-13 a0=3 a1=0 a2=0 a3=0 items=0 ppid=1 pid=615
auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0
fsgid=0 tty=(none) ses=4294967295 comm="xenstored"
exe="/usr/sbin/xenstored" subj=system_u:system_r:xenstored_t:s0
key=(null)

At least, I assume the denied {accept} means the accept system call...
I haven't verified that syscall 43 is in fact accept.

I assume that "env" since was designed to execute other programs,
selinux on CentOS 7 has been programmed to retain the context.

 -George

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

Re: [Xen-devel] [PATCH for 4.6 v2] configure: don't silently disable systemd support

[Ian Campbell]

On Thu, 2015-09-10 at 12:18 +0100, Wei Liu wrote:
> Originally when user runs ./configure --enable-systemd and systemd
> development library is not available the build system silently disables
> systemd support. This is not in line with normal expectation.
> 
> Instead, configure should error out when user has asked for systemd
> support but development libraries can't be found.
> 
> Reported-by: George Dunlap 
> Signed-off-by: Wei Liu 

Acked + applied to staging + staging-4.6

> ---
> v2: invert the test to check for explicit "yes" value.
> 
> Please rerun ./autogen.sh.
> ---
>  m4/systemd.m4 | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/m4/systemd.m4 b/m4/systemd.m4
> index 8284993..e4b1aa5 100644
> --- a/m4/systemd.m4
> +++ b/m4/systemd.m4
> @@ -85,7 +85,11 @@ AC_DEFUN([AX_CHECK_SYSTEMD], [
>   AC_DEFINE([HAVE_SYSTEMD], [1], [Systemd available and
> enabled])
>   systemd=y
>   AX_CHECK_SYSTEMD_LIBS()
> - ],[systemd=n])
> + ],[
> + AS_IF([test "x$enable_systemd" = "xyes"],
> + [AC_MSG_ERROR([Unable to find systemd
> development library])],
> + [systemd=n])
> + ])
>   ],[systemd=n])
>  ])
>  

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

Re: [Xen-devel] [PATCH for 4.6 v2 1/3] libxl: set ret to non-zero value in failure path

[Ian Campbell]

On Thu, 2015-09-10 at 11:55 +0100, Ian Campbell wrote:
> On Wed, 2015-09-09 at 18:03 +0100, Wei Liu wrote:
> > ... otherwise we have something like:
> > 
> > xl: libxl_create.c:968: initiate_domain_create: Assertion `ret' failed.
> > 
> > Signed-off-by: Wei Liu 
> 
> Acked-by: Ian Campbell 

APplied to dev and 4.6.


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

Re: [Xen-devel] [edk2] EDK II & GPL - Re: OVMF BoF @ KVM Forum 2015

[Paolo Bonzini]

On 10/09/2015 16:24, Kevin Davis wrote:
> Further leading me to guess that any actual use of those
> implementations could lead to you actually needing to hire a real
> attorney and not one that you find on YouTube.

The good thing is that attorneys have already figured it out.  IBM
figured out a few years ago how to work around Microsoft's patents, and
that's how FAT32 (and more specifically long file names) are implemented
in Linux.

Paolo

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

Re: [Xen-devel] [PATCH 1/7] tools/hotplug: remove SELinux options from var-lib-xenstored.mount

[M A Young]

On Thu, 10 Sep 2015, George Dunlap wrote:

> On Fri, Dec 19, 2014 at 11:25 AM, Olaf Hering  wrote:
> > Using SELinux mount options per default breaks several systems.
> > Either the context= mount option is not known at all to the kernel,
> > as reported for ArchLinux. Or the default value "none" is unknown to
> > SELinux, as reported for Fedora. In both cases the unit will fail.
> >
> > The proper place to specify mount options is /etc/fstab. Appearently
> > systemd is kind enough to use values from there even if Options= or
> > What= is specified in a .mount file.
> 
> For the benefit of someone moonlighting as a CentOS package
> maintainer, could you tell me how adding such an entry in a package is
> normally done?  Or alternately, how you would recommend a package
> maintainer to add the appropriate context?

I suspect it is actually easier to put the selinux context back into 
systemd file rather than trying to edit /etc/fstab which could get messy.
If that is what you want to do you could look at 
http://pkgs.fedoraproject.org/cgit/xen.git/tree/xen.fedora.systemd.patch
for ideas on how to do it.

Michael Young

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

Re: [Xen-devel] [PATCH 1/7] tools/hotplug: remove SELinux options from var-lib-xenstored.mount

[George Dunlap]

On 09/10/2015 03:13 PM, M A Young wrote:
> On Thu, 10 Sep 2015, George Dunlap wrote:
> 
>> On Fri, Dec 19, 2014 at 11:25 AM, Olaf Hering  wrote:
>>> Using SELinux mount options per default breaks several systems.
>>> Either the context= mount option is not known at all to the kernel,
>>> as reported for ArchLinux. Or the default value "none" is unknown to
>>> SELinux, as reported for Fedora. In both cases the unit will fail.
>>>
>>> The proper place to specify mount options is /etc/fstab. Appearently
>>> systemd is kind enough to use values from there even if Options= or
>>> What= is specified in a .mount file.
>>
>> For the benefit of someone moonlighting as a CentOS package
>> maintainer, could you tell me how adding such an entry in a package is
>> normally done?  Or alternately, how you would recommend a package
>> maintainer to add the appropriate context?
> 
> I suspect it is actually easier to put the selinux context back into 
> systemd file rather than trying to edit /etc/fstab which could get messy.
> If that is what you want to do you could look at 
> http://pkgs.fedoraproject.org/cgit/xen.git/tree/xen.fedora.systemd.patch
> for ideas on how to do it.

Right, well manually modifying the upstream source file is not a good
"interface" to provide.  If modifying /etc/fstab is not "the right
solution" for packages, then much better solution would have been to do
what IanC suggested later in this thread, and do something like this
instead:

Options=mode=755,$XENSTORED_MOUNT_OPTIONS

 -George

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

[Xen-devel] [linux-3.18 test] 61652: regressions - FAIL

[osstest service owner]

flight 61652 linux-3.18 real [real]
http://logs.test-lab.xenproject.org/osstest/logs/61652/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-amd64-amd64-xl-pvh-intel 11 guest-start  fail REGR. vs. 58581

Tests which are failing intermittently (not blocking):
 test-amd64-i386-xl-qemut-debianhvm-amd64-xsm 19 guest-start/debianhvm.repeat 
fail pass in 61524

Regressions which are regarded as allowable (not blocking):
 test-amd64-amd64-rumpuserxen-amd64 15 
rumpuserxen-demo-xenstorels/xenstorels.repeat fail REGR. vs. 58558
 test-armhf-armhf-libvirt  6 xen-boot  fail REGR. vs. 58581
 test-amd64-i386-xl-qemut-stubdom-debianhvm-amd64-xsm 9 debian-hvm-install fail 
baseline untested
 test-armhf-armhf-xl-rtds 11 guest-start fail baseline untested
 test-amd64-amd64-xl-qemut-stubdom-debianhvm-amd64-xsm 13 guest-localmigrate 
fail baseline untested
 test-armhf-armhf-xl-credit2   6 xen-boot fail   like 58581
 test-armhf-armhf-libvirt-xsm  6 xen-boot fail   like 58581
 test-armhf-armhf-xl-multivcpu  6 xen-boot fail  like 58581
 test-armhf-armhf-xl   6 xen-boot fail   like 58581
 test-armhf-armhf-xl-xsm   6 xen-boot fail   like 58581
 test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stop fail like 58581
 test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop  fail like 58581
 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stop fail like 58581

Tests which did not succeed, but are not blocking:
 test-armhf-armhf-xl-rtds 13 saverestore-support-check fail in 61524 never pass
 test-armhf-armhf-xl-rtds 12 migrate-support-check fail in 61524 never pass
 test-armhf-armhf-xl-rtds 16 guest-start/debian.repeat fail in 61524 never pass
 test-armhf-armhf-xl-qcow2 9 debian-di-installfail   never pass
 test-armhf-armhf-libvirt-qcow2  9 debian-di-installfail never pass
 test-armhf-armhf-libvirt-raw  9 debian-di-installfail   never pass
 test-amd64-amd64-xl-pvh-amd  11 guest-start  fail   never pass
 test-armhf-armhf-libvirt-vhd  9 debian-di-installfail   never pass
 test-amd64-amd64-libvirt-pair 21 guest-migrate/src_host/dst_host fail never 
pass
 test-armhf-armhf-xl-cubietruck  6 xen-boot fail never pass
 test-amd64-amd64-libvirt 12 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt-xsm  12 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-xsm 12 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt  12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  13 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-qcow2 11 migrate-support-checkfail never pass
 test-amd64-i386-libvirt-pair 21 guest-migrate/src_host/dst_host fail never pass
 test-amd64-i386-libvirt-raw  11 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-vhd 11 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check 
fail never pass
 test-amd64-i386-libvirt-vhd  11 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-vhd   9 debian-di-installfail   never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check 
fail never pass
 test-armhf-armhf-xl-raw   9 debian-di-installfail   never pass
 test-amd64-i386-libvirt-qcow2 11 migrate-support-checkfail  never pass
 test-amd64-amd64-libvirt-raw 11 migrate-support-checkfail   never pass
 test-amd64-i386-xl-qemut-win7-amd64 17 guest-stop  fail never pass

version targeted for testing:
 linuxfcd9bfdb9d884f1aab89124dee894e7d821bb5dc
baseline version:
 linuxd048c068d00da7d4cfa5ea7651933b99026958cf

Last test of basis58581  2015-06-15 09:42:22 Z   87 days
Failing since 58976  2015-06-29 19:43:23 Z   72 days   56 attempts
Testing same since61524  2015-09-07 11:48:03 Z3 days2 attempts


462 people touched revisions under test,
not listing them all

jobs:
 build-amd64-xsm  pass
 build-armhf-xsm  pass
 build-i386-xsm   pass
 build-amd64  pass
 build-armhf  pass
 build-i386   pass
 build-amd64-libvirt  pass
 build-armhf-libvirt  

[Xen-devel] [PATCH v3] x86/hvm: fix saved pmtimer value

[Jan Beulich]

From: Kouya Shimura 

The ACPI PM timer is sometimes broken on live migration.
Since vcpu->arch.hvm_vcpu.guest_time is always zero in other than
"delay for missed ticks mode". Even in "delay for missed ticks mode",
vcpu's guest_time field is not valid (i.e. zero) when
the state of vcpu is "blocked". (see pt_save_timer function)

The original author (Tim Deegan) of pmtimer_save() must have intended
that it saves the last scheduled time of the vcpu. Unfortunately it was
already implied this bug. FYI, there is no other timer mode than 
"delay for missed ticks mode" then.

For consistency with HPET, pmtimer_save() should refer hvm_get_guest_time()
to update the counter as well as hpet_save() does. 

Without this patch, the clock of windows server 2012R2 without HPET
might leap forward several minutes on live migration.

Signed-off-by: Kouya Shimura 

Retain use of ->arch.hvm_vcpu.guest_time when non-zero. Do the inverse
adjustment for vHPET.

Signed-off-by: Jan Beulich 
Release-acked-by: Wei Liu 

--- a/xen/arch/x86/hvm/hpet.c
+++ b/xen/arch/x86/hvm/hpet.c
@@ -513,11 +513,13 @@ static const struct hvm_mmio_ops hpet_mm
 static int hpet_save(struct domain *d, hvm_domain_context_t *h)
 {
 HPETState *hp = domain_vhpet(d);
+struct vcpu *v = pt_global_vcpu_target(d);
 int rc;
 uint64_t guest_time;
 
 write_lock(>lock);
-guest_time = guest_time_hpet(hp);
+guest_time = (v->arch.hvm_vcpu.guest_time ?: hvm_get_guest_time(v)) /
+ STIME_PER_HPET_TICK;
 
 /* Write the proper value into the main counter */
 if ( hpet_enabled(hp) )
--- a/xen/arch/x86/hvm/pmtimer.c
+++ b/xen/arch/x86/hvm/pmtimer.c
@@ -249,10 +249,12 @@ static int pmtimer_save(struct domain *d
 
 spin_lock(>lock);
 
-/* Update the counter to the guest's current time.  We always save
- * with the domain paused, so the saved time should be after the
- * last_gtime, but just in case, make sure we only go forwards */
-x = ((s->vcpu->arch.hvm_vcpu.guest_time - s->last_gtime) * s->scale) >> 32;
+/*
+ * Update the counter to the guest's current time.  Make sure it only
+ * goes forwards.
+ */
+x = (((s->vcpu->arch.hvm_vcpu.guest_time ?: hvm_get_guest_time(s->vcpu)) -
+  s->last_gtime) * s->scale) >> 32;
 if ( x < 1UL<<31 )
 s->pm.tmr_val += x;
 if ( (s->pm.tmr_val & TMR_VAL_MSB) != msb )



x86/hvm: fix saved pmtimer value

The ACPI PM timer is sometimes broken on live migration.
Since vcpu->arch.hvm_vcpu.guest_time is always zero in other than
"delay for missed ticks mode". Even in "delay for missed ticks mode",
vcpu's guest_time field is not valid (i.e. zero) when
the state of vcpu is "blocked". (see pt_save_timer function)

The original author (Tim Deegan) of pmtimer_save() must have intended
that it saves the last scheduled time of the vcpu. Unfortunately it was
already implied this bug. FYI, there is no other timer mode than 
"delay for missed ticks mode" then.

For consistency with HPET, pmtimer_save() should refer hvm_get_guest_time()
to update the counter as well as hpet_save() does. 

Without this patch, the clock of windows server 2012R2 without HPET
might leap forward several minutes on live migration.

Signed-off-by: Kouya Shimura 

Retain use of ->arch.hvm_vcpu.guest_time when non-zero. Do the inverse
adjustment for vHPET.

Signed-off-by: Jan Beulich 
Release-acked-by: Wei Liu 

--- a/xen/arch/x86/hvm/hpet.c
+++ b/xen/arch/x86/hvm/hpet.c
@@ -513,11 +513,13 @@ static const struct hvm_mmio_ops hpet_mm
 static int hpet_save(struct domain *d, hvm_domain_context_t *h)
 {
 HPETState *hp = domain_vhpet(d);
+struct vcpu *v = pt_global_vcpu_target(d);
 int rc;
 uint64_t guest_time;
 
 write_lock(>lock);
-guest_time = guest_time_hpet(hp);
+guest_time = (v->arch.hvm_vcpu.guest_time ?: hvm_get_guest_time(v)) /
+ STIME_PER_HPET_TICK;
 
 /* Write the proper value into the main counter */
 if ( hpet_enabled(hp) )
--- a/xen/arch/x86/hvm/pmtimer.c
+++ b/xen/arch/x86/hvm/pmtimer.c
@@ -249,10 +249,12 @@ static int pmtimer_save(struct domain *d
 
 spin_lock(>lock);
 
-/* Update the counter to the guest's current time.  We always save
- * with the domain paused, so the saved time should be after the
- * last_gtime, but just in case, make sure we only go forwards */
-x = ((s->vcpu->arch.hvm_vcpu.guest_time - s->last_gtime) * s->scale) >> 32;
+/*
+ * Update the counter to the guest's current time.  Make sure it only
+ * goes forwards.
+ */
+x = (((s->vcpu->arch.hvm_vcpu.guest_time ?: hvm_get_guest_time(s->vcpu)) -
+  s->last_gtime) * s->scale) >> 32;
 if ( x < 1UL<<31 )
 s->pm.tmr_val += x;
 if ( (s->pm.tmr_val & TMR_VAL_MSB) != msb )
___
Xen-devel mailing list

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

[Leif Lindholm]

On Thu, Sep 10, 2015 at 02:52:25PM +0100, Stefano Stabellini wrote:
> > > In any case this should be separate from the shim ABI discussion.
> > 
> > I disagree; I think this is very much relevant to the ABI discussion.
> > That's not to say that I insist on a particular approach, but I think
> > that they need to be considered together.
> 
> Let's suppose Xen didn't expose any RuntimeServices at all, would that
> make it easier to discuss about the EFI stub parameters?

Possibly :)

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

So, we have discussed in the past having the ability to get at
configuration tables when UEFI is not available. Say, for example,
that we wanted SMBIOS support on a platform with U-Boot firmware.

Since all that is needed then is a UEFI System Table with a pointer to
a configuration table array, this should be fairly straightforward to
implement statically. The other parameters would not be necessary.

It would however require minor changes to the arm64 kernel UEFI support.

/
Leif

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

Re: [Xen-devel] [PATCH 7/7] tools/hotplug: add wrapper to start xenstored

[George Dunlap]

On Wed, Jan 7, 2015 at 9:40 AM, Olaf Hering  wrote:
> On Tue, Jan 06, Ian Campbell wrote:
>
>> On Fri, 2014-12-19 at 12:25 +0100, Olaf Hering wrote:
>> > The shell wrapper in xenstored.service does not handle XENSTORE_TRACE.
>> >
>> > Create a separate wrapper script which is used in the sysv runlevel
>> > script and in systemd xenstored.service. It preserves existing
>> > behaviour by handling the XENSTORE_TRACE boolean. It also implements
>> > the handling of XENSTORED_ARGS=. This variable has to be added to
>> > sysconfig/xencommons.
>>
>> Why don't we just drop XENSTORED_* in favour of XENSTORED_ARGS (with an
>> example in the sysconfig file of enabling tracing if you like)?
>
> After having two weeks to think about this I came to the same
> conclusion. I think whatever the outcome is, the boolean should be
> removed. The sysconfig file should get a XENSTORED_ARGS="" along with a
> help text which mentions "-T /path" and "xenstored --help" to get other
> options because there is no man page.
>
>> Going to a wrapper script just to make some fairly uncommon debugging
>> option marginally more convenient seems like overkill to me, plus
>> XENSTORED_ARGS would allow for passing other useful options to
>> xenstored.
>
> If I recall correctly the point of the current 'sh -c "exec ..."' stunt
> was to expand the XENSTORE variable from the sysconfig file. But this
> approach leads to failures with SELinux because the socket passing does
> not work this way. Up to now I have not seen a success report for
> selinux+systemd+xenstored. Maybe its already somewhere in the other
> unread mails.
>
> In my cover letter I provided some possible ways to handle
> selinux+systemd+xenstored. Ideally the approach "Exec=/usr/bin/env
> $XENSTORED --no-fork $XENSTORED_ARGS" works because it means its
> possible to select a binary via the sysconfig file. But it also means
> the XENSTORE_TRACE boolean has to be removed in favour of the plain
> XENSTORED_ARGS= approach mentioned above. Finally this would avoid the
> need for a wrapper script.
>
> Hopefully someone with access to a SELinux enabled system will report
> which approach actually works.

I can confirm that

ExecStart=/bin/sh -c "exec $XENSTORED --no-fork $XENSTORED_ARGS"

does not work on a CentOS7 system with selinux enabled, but that

ExecStart=/usr/bin/env $XENSTORED --no-fork $XENSTORED_ARGS

does work.

A patch is on its way.

 -George

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

[Xen-devel] [PATCH for 4.6 v3 0/3] More vNUMA patches

[Wei Liu]

Wei Liu (3):
  libxc: introduce xc_domain_getvnuma
  xl/libxl: disallow saving a guest with vNUMA configured
  xl: handle empty vnuma configuration

 docs/man/xl.cfg.pod.5 |  2 ++
 tools/libxc/include/xenctrl.h | 18 +++
 tools/libxc/xc_domain.c   | 54 +++
 tools/libxl/libxl_dom.c   | 14 +++
 tools/libxl/xl_cmdimpl.c  |  3 +++
 5 files changed, 91 insertions(+)

-- 
2.1.4


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

[Xen-devel] [PATCH for 4.6 v3 2/3] xl/libxl: disallow saving a guest with vNUMA configured

[Wei Liu]

This is because the migration stream does not preserve node information.

Note this is not a regression for migration v2 vs legacy migration
because neither of them preserve node information.

Signed-off-by: Wei Liu 
---
Cc: andrew.coop...@citrix.com

v3:
1. Update manpage, code comment and commit message.
2. *Don't* check if nomigrate is set.
---
 docs/man/xl.cfg.pod.5   |  2 ++
 tools/libxl/libxl_dom.c | 14 ++
 2 files changed, 16 insertions(+)

diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index 80e51bb..555f8ba 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -263,6 +263,8 @@ virtual node.
 
 Note that virtual NUMA for PV guest is not yet supported, because
 there is an issue with cpuid handling that affects PV virtual NUMA.
+Further more, guest with virtual NUMA cannot be saved or migrated
+because migration stream does not preserve node information.
 
 Each B is a list, which has a form of
 "[VNODE_CONFIG_OPTION,VNODE_CONFIG_OPTION, ... ]"  (without quotes).
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index c2518a3..a4d37dc 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -24,6 +24,7 @@
 #include 
 #include 
 #include 
+#include 
 
 libxl_domain_type libxl__domain_type(libxl__gc *gc, uint32_t domid)
 {
@@ -1612,6 +1613,7 @@ void libxl__domain_save(libxl__egc *egc, 
libxl__domain_suspend_state *dss)
 const libxl_domain_remus_info *const r_info = dss->remus;
 libxl__srm_save_autogen_callbacks *const callbacks =
 >sws.shs.callbacks.save.a;
+unsigned int nr_vnodes = 0, nr_vmemranges = 0, nr_vcpus = 0;
 
 dss->rc = 0;
 logdirty_init(>logdirty);
@@ -1636,6 +1638,18 @@ void libxl__domain_save(libxl__egc *egc, 
libxl__domain_suspend_state *dss)
   | (debug ? XCFLAGS_DEBUG : 0)
   | (dss->hvm ? XCFLAGS_HVM : 0);
 
+/* Disallow saving a guest with vNUMA configured because migration
+ * stream does not preserve node information.
+ */
+rc = xc_domain_getvnuma(CTX->xch, domid, _vnodes, _vmemranges,
+_vcpus, NULL, NULL, NULL);
+assert(rc == -1 && (errno == XEN_ENOBUFS || errno == XEN_EOPNOTSUPP));
+if (errno == XEN_ENOBUFS && nr_vnodes) {
+LOG(ERROR, "Cannot save a guest with vNUMA configured");
+rc = ERROR_FAIL;
+goto out;
+}
+
 dss->guest_evtchn.port = -1;
 dss->guest_evtchn_lockfd = -1;
 dss->guest_responded = 0;
-- 
2.1.4


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

[Xen-devel] [PATCH for 4.6 v3 3/3] xl: handle empty vnuma configuration

[Wei Liu]

When user specifies vnuma = [], we need to skip the whole parser
function, otherwise the parser sets b_info->max_memkb to garbage value.

Signed-off-by: Wei Liu 
---
 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 ebbb9a5..7dbf37f 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1093,6 +1093,9 @@ static void parse_vnuma_config(const XLU_Config *config,
 if (xlu_cfg_get_list(config, "vnuma", , _vnuma, 1))
 return;
 
+if (!num_vnuma)
+return;
+
 b_info->num_vnuma_nodes = num_vnuma;
 b_info->vnuma_nodes = xcalloc(num_vnuma, sizeof(libxl_vnode_info));
 vcpu_parsed = xcalloc(num_vnuma, sizeof(libxl_bitmap));
-- 
2.1.4


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

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

[Mark Rutland]

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

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

Consider:

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

   DT -> Discover Xen -> Xen-specific stuff


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

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


C) When you could go:

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


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


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


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

It would simply the protocol specific to Xen, certainly.

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

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

Thanks,
Mark.

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