Re: [Xen-devel] Error booting Xen

[Shuai Ruan]

On Tue, Feb 02, 2016 at 10:39:09AM +, Andrew Cooper wrote:
> On 02/02/16 04:43, Shuai Ruan wrote:
> >
> >>> I tried booting staging branch but results were identical. Following
> >>> line repeats endlessly.
> >>> (XEN) traps.c:3290: GPF (): 82d0801c1cce -> 82d080252e5c
> >>>
> >>> $ 'addr2line -e xen-syms 82d0801c1cce' returns
> >>> 'xen/xen/arch/x86/xstate.c:387' which again points to
> >>> xsave. Also, adding 'xsave=0' makes it boot just fine.
> >> Ah, I think I see the issue: We're zeroing the entire save area in
> >> the fixup code, which will make XRSTORS fault unconditionally.
> >> Shuai, would you please look into possible ways of fixing this?
> > Ok. I will look into this problem.
> 
> Fixes for this have been committed already.  changesets 104a409^..d570d82
> 
> It would be useful if you could review them to see if there are any
> issues Jan and myself missed.
The patch is ok to me. 
And I test it on the skylake machine (with xsaves support), no boot error 
happen.

Thanks
> 
> ~Andrew
> 

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

Re: [Xen-devel] Error booting Xen

[Dario Faggioli]

Hey Harmandeep,

On Fri, 2016-01-29 at 03:13 -0700, Jan Beulich wrote:
> > > > On 28.01.16 at 20:17,  wrote:
> > Latest set looks good. No boot issues. No resets.
> > Full log at http://paste2.org/NxHNW4vn 
> 
> > Sorry I don't know much about source of last few
> > lines. I was just tracing in xen when these came.
> > I was unable to create them again. I will inform
> > you if I get these again.
> 
> The WRMSR ones are of no concern. What I'm curious about are the
> five instances of
> 
> (XEN) traps.c:3290: GPF (): 82d08019cb87 -> 82d080242e15
> 
> Could you check what these addresses correspond to in source?
> 
I guess you just happened to forgot to follow up on this question from
Jan?

> So far you've said you don't start any, but
> the logs clearly show otherwise. Are there perhaps any that
> get auto-started? In which case I'd be interested in you
> confirming that these come up fine.
> 
And this too?

Can you please do that? :-)

Regards,
Dario
-- 
<> (Raistlin Majere)
-
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)



signature.asc
Description: This is a digitally signed message part
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH V13 3/5] libxl: add pvusb API

[Chun Yan Liu]

>>> On 2/3/2016 at 12:48 AM, in message
, George
Dunlap  wrote: 
> On Tue, Jan 19, 2016 at 3:48 PM, Ian Jackson  
> wrote: 
> > Chunyan Liu writes ("[PATCH V13 3/5] libxl: add pvusb API"): 
> >> > Ought this function to really report success if these calls fail ? 
> >> 
> >> I think so. Till here, the USB device has already been unbound from 
> >> usbback and removed from xenstore. usb-list cannot list it any more. 
>  
> Also, I think we probably should return some sort of error, so that 
> scripts &c can at least know something out of the ordinary has 
> happened, even if it doesn't go back to the state we started in when 
> the function was called.

That makes sense. But do we need to differentiate failure at removing
xenstore information step and error in bind/unbind step? Or, treat 
them as same? That affects doing it within other process (like 
usbctrl-detach). For the former, xl usb-list can list usb device and
one can do usbdev-detach again; and if in a usbctrl-detach process,
will stop the process. For the later, one cannot do usbdev-detach, 
and if in usbctrl-detach process, in current codes, will ignore that.

Chunyan

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



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

Re: [Xen-devel] [RFC Design Doc] Add vNVDIMM support for Xen

[Haozhong Zhang]

On 02/02/16 14:15, Konrad Rzeszutek Wilk wrote:
> > 3. Design of vNVDIMM in Xen
> 
> Thank you for this design!
> 
> > 
> >  Similarly to that in KVM/QEMU, enabling vNVDIMM in Xen is composed of
> >  three parts:
> >  (1) Guest clwb/clflushopt/pcommit enabling,
> >  (2) Memory mapping, and
> >  (3) Guest ACPI emulation.
> 
> 
> .. MCE? and vMCE?
>

Specifications on my hand seem not mention much about MCE for NVDIMM,
but I remember that NVDIMM driver in Linux kernel does have MCE
code. I'll have a look at that code and add this part later.

> > 
> >  The rest of this section present the design of each part
> >  respectively. The basic design principle to reuse existing code in
> >  Linux NVDIMM driver and QEMU as much as possible. As recent
> >  discussions in the both Xen and QEMU mailing lists for the v1 patch
> >  series, alternative designs are also listed below.
> > 
> > 
> > 3.1 Guest clwb/clflushopt/pcommit Enabling
> > 
> >  The instruction enabling is simple and we do the same work as in KVM/QEMU.
> >  - All three instructions are exposed to guest via guest cpuid.
> >  - L1 guest pcommit is never intercepted by Xen.
> 
> I wish there was some watermarks like the PLE has.
> 
> My fear is that an unfriendly guest can issue sfence all day long
> flushing out other guests MMC queue (the writes followed by pcommits).
> Which means that an guest may have degraded performance as their
> memory writes are being flushed out immediately as if they were
> being written to UC instead of WB memory. 
>

pcommit takes no parameter and it seems hard to solve this problem
from hardware for now. And the current VMX does not provide mechanism
to limit the commit rate of pcommit like PLE for pause.

> In other words - the NVDIMM resource does not provide any resource
> isolation. However this may not be any different than what we had
> nowadays with CPU caches.
>

Does Xen have any mechanism to isolate multiple guests' operations on
CPU caches?

> 
> >  - L1 hypervisor is allowed to intercept L2 guest pcommit.
> 
> clwb?
>

VMX is not capable to intercept clwb. Any reason to intercept it?

> > 
> > 
> > 3.2 Address Mapping
> > 
> > 3.2.1 My Design
> > 
> >  The overview of this design is shown in the following figure.
> > 
> >  Dom0 |   DomU
> >   |
> >   |
> >  QEMU |
> >  +...++...+-+ |
> >   VA |   | Label Storage Area |   | buf | |
> >  +...++...+-+ |
> >  ^^ ^ |
> >  || | |
> >  V| | |
> >  +---+   +---+mmap(2) |
> >  | vACPI |   | v_DSM || | |+++
> >  +---+   +---+| | |   SPA  || /dev/pmem0 |
> >  ^   ^ +--+ | |+++
> >  |---|-||--   | ^^
> >  |   | || | ||
> >  |+--+ +~-~-+|
> >  |||| |
> > XEN_DOMCTL_memory_mapping
> >  |||+-~--+
> >  |||| |
> >  ||   +++ |
> >  Linux   ||   SPA || /dev/pmem0 | | +--+   +--+
> >  ||   +++ | | ACPI |   | _DSM |
> >  ||   ^   | +--+   +--+
> >  ||   |   | |  |
> >  ||   Dom0 Driver |   hvmloader/xl |
> >  
> > ||---|-|--|---
> >  |+---~-~--+
> >  Xen || |
> >  +~-+
> >  
> > -|
> >   ++
> >|
> > +-+
> >  HW |NVDIMM   |
> > +-+
> > 
> > 
> >  This design treats host NVDIMM devices as ordinary MMIO devices:
> 
> Nice.
> 
> But it also means you need Xen to 'share' the ranges of an MMIO device.
> 
> That is you may need dom0 _DSM method to access certain ranges
> (the AML code may need to poke there) - and the guest may want to access
> those as well.
>

Currently, we a

Re: [Xen-devel] [PATCH v3 3/3] tools: introduce parameter max_wp_ram_ranges.

[Jan Beulich]

>>> On 03.02.16 at 08:10,  wrote:
> On 2/2/2016 11:21 PM, Jan Beulich wrote:
> On 02.02.16 at 16:00,  wrote:
>>> The limit of 4G is to avoid the data missing from uint64 to uint32
>>> assignment. And I can accept the 8K limit for XenGT in practice.
>>> After all, it is vGPU page tables we are trying to trap and emulate,
>>> not normal page frames.
>>>
>>> And I guess the reason that one domain exhausting Xen's memory can
>>> affect another domain is because rangeset uses Xen heap, instead of the
>>> per-domain memory. So what about we use a 8K limit by now for XenGT,
>>> and in the future, if a per-domain memory allocation solution for
>>> rangeset is ready, we do need to limit the rangeset size. Does this
>>> sounds more acceptable?
>>
>> The lower the limit the better (but no matter how low the limit
>> it won't make this a pretty thing). Anyway I'd still like to wait
>> for what Ian may further say on this.
>>
> Hi Jan, I just had a discussion with my colleague. We believe 8K could
> be the biggest limit for the write-protected ram ranges. If in the
> future, number of vGPU page tables exceeds this limit, we will modify
> our back-end device model to find a trade-off method, instead of
> extending this limit. If you can accept this value as the upper bound
> of rangeset, maybe we do not need to add any tool stack parameters, but
> define a MAX_NR_WR_RAM_RANGES for the write-protected ram rangesset. As
> to other rangesets, we keep their limit as 256. Does this sounds OK? :)

I'm getting the impression that we're moving in circles. A blanket
limit above the 256 one for all domains is _not_ going to be
acceptable; going to 8k will still need host admin consent. With
your rangeset performance improvement patch, each range is
going to be tracked by a 40 byte structure (up from 32), which
already means an overhead increase for all the other ranges. 8k
of wp ranges implies an overhead beyond 448k (including the
xmalloc() overhead), which is not _that_ much, but also not
negligible.

Jan


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

[Xen-devel] [linux-4.1 test] 79950: regressions - FAIL

[osstest service owner]

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

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 build-amd64-rumpuserxen   6 xen-build fail REGR. vs. 66399
 build-i386-rumpuserxen6 xen-build fail REGR. vs. 66399
 test-armhf-armhf-xl-xsm  15 guest-start/debian.repeat fail REGR. vs. 66399
 test-armhf-armhf-xl  15 guest-start/debian.repeat fail REGR. vs. 66399
 build-amd64   5 xen-buildfail in 79642 REGR. vs. 66399
 test-armhf-armhf-xl-credit2 15 guest-start/debian.repeat fail in 79642 REGR. 
vs. 66399
 test-armhf-armhf-xl-cubietruck 15 guest-start/debian.repeat fail in 79817 
REGR. vs. 66399
 test-armhf-armhf-xl-multivcpu 15 guest-start/debian.repeat fail in 79817 REGR. 
vs. 66399

Tests which are failing intermittently (not blocking):
 test-armhf-armhf-xl-xsm  11 guest-startfail in 79642 pass in 79950
 test-armhf-armhf-xl  11 guest-startfail in 79817 pass in 79950
 test-armhf-armhf-xl-credit2  11 guest-start fail pass in 79642
 test-armhf-armhf-xl-cubietruck 11 guest-start   fail pass in 79817
 test-armhf-armhf-xl-multivcpu 11 guest-startfail pass in 79817
 test-armhf-armhf-xl-rtds 11 guest-start fail pass in 79817

Regressions which are regarded as allowable (not blocking):
 test-armhf-armhf-xl-rtds 15 guest-start/debian.repeat fail in 79642 like 66399
 test-amd64-i386-xl-qemut-win7-amd64 16 guest-stop fail in 79817 like 66399
 test-amd64-amd64-xl-qemuu-win7-amd64 16 guest-stop fail like 66399
 test-amd64-i386-xl-qemuu-win7-amd64 16 guest-stop  fail like 66399
 test-armhf-armhf-xl-vhd   9 debian-di-installfail   like 66399

Tests which did not succeed, but are not blocking:
 build-amd64-libvirt   1 build-check(1)blocked in 79642 n/a
 build-amd64-rumpuserxen   1 build-check(1)blocked in 79642 n/a
 test-amd64-amd64-xl-pvh-amd   1 build-check(1)blocked in 79642 n/a
 test-amd64-amd64-xl-pvh-intel  1 build-check(1)   blocked in 79642 n/a
 test-amd64-amd64-xl-multivcpu  1 build-check(1)   blocked in 79642 n/a
 test-amd64-amd64-xl-credit2   1 build-check(1)blocked in 79642 n/a
 test-amd64-i386-xl1 build-check(1)blocked in 79642 n/a
 test-amd64-amd64-libvirt-xsm  1 build-check(1)blocked in 79642 n/a
 test-amd64-i386-qemut-rhel6hvm-intel  1 build-check(1)blocked in 79642 n/a
 test-amd64-amd64-pair 1 build-check(1)blocked in 79642 n/a
 test-amd64-i386-libvirt-pair  1 build-check(1)blocked in 79642 n/a
 test-amd64-amd64-pygrub   1 build-check(1)blocked in 79642 n/a
 test-amd64-amd64-libvirt-pair  1 build-check(1)   blocked in 79642 n/a
 test-amd64-amd64-qemuu-nested-amd  1 build-check(1)   blocked in 79642 n/a
 test-amd64-i386-xl-qemut-debianhvm-amd64 1 build-check(1) blocked in 79642 n/a
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 1 build-check(1) blocked in 
79642 n/a
 test-amd64-i386-freebsd10-i386  1 build-check(1)  blocked in 79642 n/a
 test-amd64-amd64-xl-qcow2 1 build-check(1)blocked in 79642 n/a
 test-amd64-i386-libvirt   1 build-check(1)blocked in 79642 n/a
 test-amd64-i386-qemut-rhel6hvm-amd  1 build-check(1)  blocked in 79642 n/a
 test-amd64-amd64-xl   1 build-check(1)blocked in 79642 n/a
 test-amd64-amd64-libvirt  1 build-check(1)blocked in 79642 n/a
 test-amd64-amd64-xl-rtds  1 build-check(1)blocked in 79642 n/a
 test-amd64-amd64-amd64-pvgrub  1 build-check(1)   blocked in 79642 n/a
 test-amd64-i386-pair  1 build-check(1)blocked in 79642 n/a
 test-amd64-amd64-i386-pvgrub  1 build-check(1)blocked in 79642 n/a
 test-amd64-amd64-xl-qemut-win7-amd64  1 build-check(1)blocked in 79642 n/a
 test-amd64-amd64-qemuu-nested-intel  1 build-check(1) blocked in 79642 n/a
 test-amd64-i386-xl-raw1 build-check(1)blocked in 79642 n/a
 test-amd64-i386-freebsd10-amd64  1 build-check(1) blocked in 79642 n/a
 test-amd64-i386-xl-qemuu-ovmf-amd64  1 build-check(1) blocked in 79642 n/a
 test-amd64-amd64-xl-qemuu-win7-amd64  1 build-check(1)blocked in 79642 n/a
 test-amd64-amd64-xl-qemut-debianhvm-amd64 1 build-check(1) blocked in 79642 n/a
 test-amd64-i386-xl-qemuu-win7-amd64  1 build-check(1) blocked in 79642 n/a
 test-amd64-amd64-libvirt-vhd  1 build-check(1)blocked in 79642 n/a
 test-amd64-i386-xl-qemut-win7-amd64  1 build-check(1) blocked in 79642 n/a
 test-amd64-i386-xl-qemuu-debianhvm-amd64 1 build-check(1) blocked in 79642 n/a
 test-amd64-i386-qemuu-rhel6hvm-intel  1 build-check(1)blocked in 79642 n/a
 test-amd64-amd64-xl-qemuu-ovmf

Re: [Xen-devel] [PATCH] x86/hvm: Provide list of emulated features in HVM CPUID leaf

[Roger Pau Monné]

El 3/2/16 a les 1:17, Andrew Cooper ha escrit:
> On 02/02/2016 23:30, Boris Ostrovsky wrote:
>> On 02/02/2016 06:22 PM, Andrew Cooper wrote:
>>> On 02/02/2016 23:17, Boris Ostrovsky wrote:
 Hypervisor may choose which features to emulate for HVMlite guests.
 Guest will query the HVM CPUID leaf to find out what is available.

 Signed-off-by: Boris Ostrovsky 
>>> Roger also submitted a patch to do this.  However, it is not
>>> appropriate, so was dropped.
>>>
>>> An HVMLite domain should assume there are no emulated devices.  The very
>>> old legacy devices will never be implemented, and any others we care
>>> about possibly implementing in the future have APCI-based ways of
>>> indicating support.
>>
>> OK, so I wasn't the first one to come up with this ;-)

Hehe, no I think I've posted the exact same patch a week ago:

http://marc.info/?l=xen-devel&m=145339515912038

>> I think for now I mostly care about APIC and for that I can use HW
>> CPUID bit (which I believe is cleared for HVMlite guests).
> 
> The APIC bit in cpuid is magic and specified as a fast forward of the
> APICBASE_MSR enable bit.
> 
> Therefore, the correct architectural behaviour is for this bit to be
> clear if the local APIC is disabled, or indeed not implemented.
> 
> With my maintainers hat on, I will reject any attempt to introduce
> non-architectural behaviour; at the moment I am dealing with the
> stupidity that is the PV XSAVE interface, where broken bugfix piled on
> top of broken bugfix has resulted in a situation where many Linux PV
> guests crash if provided with architecturally correct behaviour of the
> OSXSAVE cpuid bit (yet another magic one).
> 
>> The trouble is that I need to present Linux as having APIC (boot code
>> doesn't feel good if !cpu_has_apic) so I'll need to keep no-APIC
>> emulation private to Xen-related code. Which is doable.

I have to do the same for FreeBSD, I have to manually switch the APIC
cpuid bit, or else FreeBSD refuses to do SMP initialization. IMHO, what
we currently do (no APIC cpuid bit) is correct, and when a local APIC is
available the bit will indeed be enabled.

> I see two choices.
> 
> 1) Require that Linux DMLite guests require a Local APIC, and we allow
> that to be a configured option.  Exposing APIC definitely makes sense
> longer term, because APICV hardware acceleration outperforms the
> hypercall-based method.

This is what I aim to do long term, that is provide an emulated local
APIC. The plan was to then also provide ACPI tables in order to notify
the presence of the local and IO APICs (we are going to need both if we
plan to do pci-passthrough of devices with PCI interrupts). Of course
the APIC cpuid bit will also be enabled in this case.

Roger.

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

Re: [Xen-devel] Xen 4.7 Development Update

[Haozhong Zhang]

On 02/01/16 10:45, Wei Liu wrote:
[...] 
> = Projects =
> 
> == Hypervisor == 
[...]
> === x86 === 
[...]
> *  VMX TSC scaling support
>   -  Haozhong Zhang

v4 patch series was sent out. Patch 1 - 3 were merged.

[...]

> == Toolstack == 
> 
> *  vNVDIMM support
>   -  Haozhong Zhang

v1 patch series and a design doc were sent out. Let's see how much I
can get before freeze.

Thanks,
Haozhong

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

Re: [Xen-devel] [RFC Design Doc] Add vNVDIMM support for Xen

[Jan Beulich]

>>> On 03.02.16 at 08:00,  wrote:
> On 02/02/16 17:11, Stefano Stabellini wrote:
>> Once upon a time somebody made the decision that ACPI tables
>> on Xen should be static and included in hvmloader. That might have been
>> a bad decision but at least it was coherent. Loading only *some* tables
>> from QEMU, but not others, it feels like an incomplete design to me.
>>
>> For example, QEMU is currently in charge of emulating the PCI bus, why
>> shouldn't it be QEMU that generates the PRT and MCFG?
>>
> 
> To Keir, Jan and Andrew:
> 
> Are there anything related to ACPI that must be done (or are better to
> be done) in hvmloader?

Some of the static tables (FADT and HPET come to mind) likely would
better continue to live in hvmloader. MCFG (for example) coming from
qemu, otoh, would be quite natural (and would finally allow MMCFG
support for guests in the first place). I.e. ...

>> I prefer switching to QEMU building all ACPI tables for devices that it
>> is emulating. However this alternative is good too because it is
>> coherent with the current design.
> 
> I would prefer to this one if the final conclusion is that only one
> agent should be allowed to build guest ACPI. As I said above, it looks
> like a big change to switch to QEMU for all ACPI tables and I'm afraid
> it would break some existing guests. 

... I indeed think that tables should come from qemu for components
living in qemu, and from hvmloader for components coming from Xen.

Jan


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

Re: [Xen-devel] [RFC Design Doc] Add vNVDIMM support for Xen

[Jan Beulich]

>>> On 03.02.16 at 09:28,  wrote:
> On 02/02/16 14:15, Konrad Rzeszutek Wilk wrote:
>> > 3.1 Guest clwb/clflushopt/pcommit Enabling
>> > 
>> >  The instruction enabling is simple and we do the same work as in KVM/QEMU.
>> >  - All three instructions are exposed to guest via guest cpuid.
>> >  - L1 guest pcommit is never intercepted by Xen.
>> 
>> I wish there was some watermarks like the PLE has.
>> 
>> My fear is that an unfriendly guest can issue sfence all day long
>> flushing out other guests MMC queue (the writes followed by pcommits).
>> Which means that an guest may have degraded performance as their
>> memory writes are being flushed out immediately as if they were
>> being written to UC instead of WB memory. 
> 
> pcommit takes no parameter and it seems hard to solve this problem
> from hardware for now. And the current VMX does not provide mechanism
> to limit the commit rate of pcommit like PLE for pause.
> 
>> In other words - the NVDIMM resource does not provide any resource
>> isolation. However this may not be any different than what we had
>> nowadays with CPU caches.
>>
> 
> Does Xen have any mechanism to isolate multiple guests' operations on
> CPU caches?

No. All it does is disallow wbinvd for guests not controlling any
actual hardware. Perhaps pcommit should at least be limited in
a similar way?

>> >  - L1 hypervisor is allowed to intercept L2 guest pcommit.
>> 
>> clwb?
> 
> VMX is not capable to intercept clwb. Any reason to intercept it?

I don't think so - otherwise normal memory writes might also need
intercepting. Bus bandwidth simply is shared (and CLWB operates
on a guest virtual address, so can only cause bus traffic for cache
lines the guest has managed to dirty).

Jan


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

Re: [Xen-devel] Nested virtualization off VMware vSphere 6.0 with EL6 guests crashes on Xen 4.6

[Jan Beulich]

>>> On 02.02.16 at 23:05,  wrote:
> This is getting more and more bizzare.
> 
> I realized that this machine has VMCS shadowing so Xen does not trap on
> any vmwrite or vmread. Unless I update the VMCS shadowing bitmap - which
> I did for vmwrite and vmread to get a better view of this. It never
> traps on VIRTUAL_APIC_PAGE_ADDR accesses. It does trap on: 
> VIRTUAL_PROCESSOR_ID,
> VM_EXIT_MSR_LOAD_ADDR and GUEST_[ES,DS,FS,GS,TR]_SELECTORS.
> 
> (It may also trap on IO_BITMAP_A,B but I didn't print that out).
> 
> To confirm that the VMCS that will be given to the L2 guest is correct
> I added some printking of some states that ought to be pretty OK such
> as HOST_RIP or HOST_RSP - which are all 0!

But did you also check what the field of interest starts out as?

> If I let the nvmx_update_virtual_apic_address keep on going without
> modifying the VIRTUAL_APIC_PAGE_ADDR it later on crashes the nested
> guest:
> 
> EN) nvmx_handle_vmwrite: 0   

The missing characters at the beginning may just be a copy-and-
paste mistake, but they could also indicate a truncated log. Can
you clarify which of the two it is?

> (XEN) nvmx_handle_vmwrite: 0 
> (XEN) nvmx_handle_vmwrite: 2008  
> (XEN) nvmx_handle_vmwrite: 2008  
> (XEN) nvmx_handle_vmwrite: 0 
> (XEN) nvmx_handle_vmwrite: 2008  
> (XEN) nvmx_handle_vmwrite: 0 
> (XEN) nvmx_handle_vmwrite: 2008  
> (XEN) nvmx_handle_vmwrite: 2008  
> (XEN) nvmx_handle_vmwrite: 2008  
> (XEN) nvmx_handle_vmwrite: 2008  
> (XEN) nvmx_handle_vmwrite: 2008  
> (XEN) nvmx_handle_vmwrite: 800   
> (XEN) nvmx_handle_vmwrite: 804   
> (XEN) nvmx_handle_vmwrite: 806   
> (XEN) nvmx_handle_vmwrite: 80a   
> (XEN) nvmx_handle_vmwrite: 80e   
> (XEN) nvmx_update_virtual_apic_address: vCPU1 0x(vAPIC) 
> 0x0(APIC), 0x0(TPR) ctrl=b5b9effe sec=0 

Assuming the field starts out as other than all ones, could you check
its value on each of the intercepted VMWRITEs, to at least narrow
when it changes.

Kevin, Jun - are there any cases where the hardware would alter
this field's value? Like during some guest side LAPIC manipulations?
(The same monitoring as suggested during VMWRITEs could of
course also be added to LAPIC accesses visible to the hypervisor,
but I guess there won't be too many of those.)

Jan


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

Re: [Xen-devel] Xen 4.7 Development Update

[Han, Huaitong]

On Mon, 2016-02-01 at 10:45 +, Wei Liu wrote:
> This email only tracks big items for xen.git tree. Please reply for
> items you
> woulk like to see in 4.7 so that people have an idea what is going on
> and
> prioritise accordingly.
> 
> You're welcome to provide description and use cases of the feature
> you're
> working on.
> 
> = Timeline =
> 
> We now adopt a fixed cut-off date scheme. We will release twice a
> year. The upcoming 4.7 timeline are as followed:
> 
> * Last posting date: March 18, 2016
> * Hard code freeze: April 1, 2016
> * RC1: TBD
> * Release: June 3, 2016
> 
> Note that we don't have freeze exception scheme anymore. All patches
> that wish to go into 4.7 must be posted no later than the last
> posting
> date. All patches posted after that date will be automatically queued
> into next release.
> 
> RCs will be arranged immediately after freeze.
> 
> = Projects =
> *  Memory protection-key support
>   -  Huaitong Han
V8 (7 patches) has been sent out, 2 patches have already been merged, 3
of the remaining 5 patches have been Acked/Reviewed by Andrew/Jan.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH V8 2/5] x86/hvm: pkeys, add pkeys support for guest_walk_tables

[Jan Beulich]

>>> On 02.02.16 at 08:35,  wrote:
> Protection keys define a new 4-bit protection key field(PKEY) in bits 62:59 
> of
> leaf entries of the page tables.
> 
> PKRU register defines 32 bits, there are 16 domains and 2 attribute bits per
> domain in pkru, for each i (0 ≤ i ≤ 15), PKRU[2i] is the access-disable bit 
> for
> protection key i (ADi); PKRU[2i+1] is the write-disable bit for protection 
> key
> i (WDi). PKEY is index to a defined domain.
> 
> A fault is considered as a PKU violation if all of the following conditions 
> are
> true:
> 1.CR4_PKE=1.
> 2.EFER_LMA=1.
> 3.Page is present with no reserved bit violations.
> 4.The access is not an instruction fetch.
> 5.The access is to a user page.
> 6.PKRU.AD=1
> or The access is a data write and PKRU.WD=1
> and either CR0.WP=1 or it is a user access.
> 
> Signed-off-by: Huaitong Han 

Reviewed-by: Jan Beulich 
albeit ...

> Changes in v8:
> *Abstract out _write_cr4.

... I'm not happy about the chose name and will try to remember
to change it to e.g. raw_write_cr4(). Names starting with an
underscore and a lower case letter are reserved to symbols
local to a given translation unit, which in my reading doesn't fit
with them getting placed in header files.

Jan

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

[Xen-devel] [PATCH 1/3] Move collectversions from ts-xen-build into Osstest::BuildSupport

[Ian Campbell]

I'm going to have a need for it elsewhere.

Signed-off-by: Ian Campbell 
---
 Osstest/BuildSupport.pm | 12 
 ts-xen-build| 13 +
 2 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/Osstest/BuildSupport.pm b/Osstest/BuildSupport.pm
index 933f6e1..a183546 100644
--- a/Osstest/BuildSupport.pm
+++ b/Osstest/BuildSupport.pm
@@ -42,6 +42,7 @@ BEGIN {
 
   xendist
   $xendist
+  collect_xen_built_versions
 
   submodulefixup submodule_have submodule_find
 
@@ -84,6 +85,17 @@ sub xendist () {
($ho, 'xendist', '', $r{"buildjob"});
 }
 
+sub collect_xen_built_versions () {
+my $tools="$builddir/xen/tools";
+my $extras="$builddir/xen/extras";
+store_revision($ho, 'qemu', "$tools/ioemu-dir", 1);
+store_revision($ho, 'qemu', "$tools/qemu-xen-traditional-dir", 1);
+store_revision($ho, 'qemuu', "$tools/qemu-xen-dir", 1);
+store_revision($ho, 'seabios', "$tools/firmware/seabios-dir", 1);
+store_revision($ho, 'ovmf', "$tools/firmware/ovmf-dir", 1);
+store_revision($ho, 'minios', "$extras/mini-os", 1);
+}
+
 #- submodules -
 
 sub submodulefixup () {
diff --git a/ts-xen-build b/ts-xen-build
index 8f92729..382fe62 100755
--- a/ts-xen-build
+++ b/ts-xen-build
@@ -151,17 +151,6 @@ END
 }
 }
 
-sub collectversions () {
-my $tools="$builddir/xen/tools";
-my $extras="$builddir/xen/extras";
-store_revision($ho, 'qemu', "$tools/ioemu-dir", 1);
-store_revision($ho, 'qemu', "$tools/qemu-xen-traditional-dir", 1);
-store_revision($ho, 'qemuu', "$tools/qemu-xen-dir", 1);
-store_revision($ho, 'seabios', "$tools/firmware/seabios-dir", 1);
-store_revision($ho, 'ovmf', "$tools/firmware/ovmf-dir", 1);
-store_revision($ho, 'minios', "$extras/mini-os", 1);
-}
-
 sub divide () {
 # Only move hv to xeninstall, so that we can have
 # xenpolicy in tools tarball.
@@ -248,7 +237,7 @@ sub trapping ($) {
 checkout();
 
 trapping(\&build);
-trapping(\&collectversions);
+trapping(\&collect_xen_built_versions);
 
 die "*** something failed:\n\n".(join "\n\n",@probs)."\n** something failed"
 if @probs;
-- 
2.6.1


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

[Xen-devel] [PATCH 3/3] make-coverity-flight: set coverity_upload=true

[Ian Campbell]

Signed-off-by: Ian Campbell 
---
Apply once the previous patch + manual upload lead to us being happy.
---
 make-coverity-flight | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/make-coverity-flight b/make-coverity-flight
index dafb61c..d086762 100755
--- a/make-coverity-flight
+++ b/make-coverity-flight
@@ -41,7 +41,7 @@ 
build_hostflags=share-build-$suite-$arch,arch-$arch,suite-$suite,purpose-build
arch=$arch host_hostflags=$build_hostflags \
tree_xen=$TREE_COVERITY \
revision_xen=$REVISION_COVERITY \
-   coverity_upload=false
+   coverity_upload=true
 
 echo $flight
 
-- 
2.6.1


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

[Xen-devel] [PATCH 2/3] Add a weekly coverity flight

[Ian Campbell]

This primarily consists of ts-coverity-scan and make-coverity-flight
which constructs the sole job.

The most recently scanned revision is pushed to a new coverity-scanned
branch in the usual xen.git, tests are run on the master branch.

For the cr-* integration we treat branch=coverity as a special case of
tree=xen. I didn't think tree=coverity made much sense, and would
probably reach tendrils into lots of other places (such as the
invocations of check_tested).

I initially thoughts that $c{CoverityEmail} would need to be an actual
account registered with scan, however a manual experiment using
email=secur...@xen.org was accepted by the service. An "analysis
complete" message was sent to security@ while individual results mails
were sent to each member of the coverity project who was configured to
receive them. I think this is what we want. The "analysis complete"
mail contained no sensitive data, but also no real information other
than "success" (or presumably "failure" if that were to be the case).
I think going to security@ is probably OK.

I have run this in non-uploading mode on the production infra and then
run the curl manually, adjusting the CLI until it works and updated
the script to match. I have not yet run in uploading mode but will do
so once another upload is allowed by the service,

In my experiments the curl command took ~35 minutes to complete (rate
in the 100-200k range). Not sure if this is a problem. Note that curl
is run on the controller (via system_checked) and consequently has no
timeout etc.

Note that the token must be supplied with 
Cc: Andrew Cooper 
---
v2:
 - Split move of collect_xen_built_versions() into separate patch
 - Implemented support for coverity_upload = true (but don't yet set
   it)
 - Add host_hostflags to the job so it can actually run somewhere.
 - Call tsreadconfig() before referencing $r{coverity_upload} so that
   $r is actually populated.
 - use token=http://www.gnu.org/licenses/>.
+
+
+set -e -o posix
+
+branch=$1
+xenbranch=$2
+blessing=$3
+buildflight=$4
+
+flight=`./cs-flight-create $blessing $branch`
+
+. ./cri-common
+. ./ap-common
+. ./mfi-common
+
+defsuite=`getconfig DebianSuite`
+
+arch=amd64
+suite=$defsuite
+
+build_hostflags=share-build-$suite-$arch,arch-$arch,suite-$suite,purpose-build
+
+./cs-job-create $flight coverity-$arch coverity \
+   arch=$arch host_hostflags=$build_hostflags \
+   tree_xen=$TREE_COVERITY \
+   revision_xen=$REVISION_COVERITY \
+   coverity_upload=false
+
+echo $flight
+
+# Local variables:
+# mode: sh
+# sh-basic-offset: 2
+# indent-tabs-mode: nil
+# End:
diff --git a/production-config b/production-config
index f2f0584..e67a253 100644
--- a/production-config
+++ b/production-config
@@ -100,6 +100,10 @@ TftpGrubVersion -XX-XX
 XenUsePath /usr/groups/xencore/systems/bin/xenuse
 XenUseUser osstest
 
+# Results might include potential vulnerabilities.
+CoverityEmail secur...@xen.org
+CoverityTools cov-analysis-linux64-7.7.0.4.tar.gz
+
 # We use the IP address because Citrix can't manage reliable nameservice
 #DebianMirrorHost debian.uk.xensource.com
 #DebianMirrorHost 10.80.16.196
diff --git a/sg-run-job b/sg-run-job
index 20ebb64..7e592dd 100755
--- a/sg-run-job
+++ b/sg-run-job
@@ -445,6 +445,11 @@ proc prepare-build-host {} {
 run-ts . host-build-prep ts-xen-build-prep
 }
 
+proc need-hosts/coverity {} { return BUILD }
+proc run-job/coverity {} {
+run-ts . = ts-coverity-scan + host
+}
+
 #-- main program --
 
 jobdb::set-flight
diff --git a/ts-coverity-scan b/ts-coverity-scan
new file mode 100755
index 000..f3cc497
--- /dev/null
+++ b/ts-coverity-scan
@@ -0,0 +1,110 @@
+#!/usr/bin/perl -w
+# This is part of "osstest", an automated testing framework for Xen.
+# Copyright (C) 2015 Citrix Inc.
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU Affero General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU Affero General Public License for more details.
+#
+# You should have received a copy of the GNU Affero General Public License
+# along with this program.  If not, see .
+
+use strict qw(vars);
+use DBI;
+use Osstest;
+use File::Path;
+use POSIX;
+use Osstest::TestSupport;
+use Osstest::BuildSupport;
+
+tsreadconfig();
+selectbuildhost(\@ARGV);
+# remaining arguments are passed as targets to "make"
+builddirsprops();
+
+# Require explicit opt in from flight construction
+my $coverity_upload = ($r{coverity_upload}//'false') =~ m/true/ ? 1 : 0;
+
+# This must contain exactly and only the token, for example there must
+# be no trailing "\n", otherwise it is included in the literal token,
+# whic

Re: [Xen-devel] [PATCH v7 9/9] vring: Use the DMA API on Xen

[David Vrabel]

On 03/02/16 05:46, Andy Lutomirski wrote:
> Signed-off-by: Andy Lutomirski 

You forgot the previous Reviewed-by tags.

David

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

[Xen-devel] [PATCH OSSTEST 5/5 v2] mg-show-flight-runvars: recurse on buildjobs upon request

[Ian Campbell]

By looping over @rows looking for buildjobs runvars and adding those
jobs to the output until nothing changes.

The output is resorted by runvar name which is the desired default
behaviour. As usual can be piped to sort(1) to sort by flight+job.

Signed-off-by: Ian Campbell 
---
v2:
 - Use $jobcond,@jobconfparams to avoid SQL injection
 - Only recurse if the option was given
 - Drop synth from ORDER BY
 - Use a Schwatzian transform for the sort, at the same time allowing
   retention of the sorting of synth runvars last.
---
 mg-show-flight-runvars | 44 +---
 1 file changed, 37 insertions(+), 7 deletions(-)

diff --git a/mg-show-flight-runvars b/mg-show-flight-runvars
index f96539f..faa0ea1 100755
--- a/mg-show-flight-runvars
+++ b/mg-show-flight-runvars
@@ -46,30 +46,56 @@ for (;;) {
 
 die unless @ARGV==1 && $ARGV[0] =~ m/^\w+$/;
 
-
 our @cols = qw(job name val);
 our @rows;
+our %jobs;
+
+sub collect ($;$@) {
+my ($flight,$jobcond,@jobcondparams) = @_;
 
-sub collect ($) {
-my ($flight) = @_;
+$jobcond //= "TRUE";
 
 $flight =~ m/^\d+/ or $flight = "'$flight'";
-my $qfrom = "FROM runvars WHERE flight=$flight AND $synthcond";
+my $qfrom = "FROM runvars WHERE flight=$flight AND $synthcond AND 
$jobcond";
 
 my $q = $dbh_tests->prepare
-   ("SELECT synth, ".(join ',', @cols)." $qfrom ORDER BY synth, name, 
job");
-$q->execute();
+   ("SELECT synth, ".(join ',', @cols)." $qfrom ORDER BY name, job");
+$q->execute(@jobcondparams);
 
 while (my (@row) = $q->fetchrow_array()) {
my $synth = shift @row;
$row[0] = "$flight.$row[0]" if $recurse;
$row[1] .= $synthsufx if $synth && $synth ne 'f'; # sqlite3 is typeless
push @rows, \@row;
+   $jobs{$row[0]} = 1;
 }
 }
 
 collect($ARGV[0]);
 
+if ($recurse) {
+foreach my $row (@rows) {
+   next unless $row->[1] =~ m/^(?:.*_)?([^_]*)buildjob$/;
+   next if $jobs{$row->[2]};
+
+   # parse this flight and job, which must be in $flight.$job
+   # form if $recurse is true (see collect())
+   my ($tflight, $tjob) = flight_otherjob(undef, $row->[0]);
+   die "$row->[1]" unless $tflight;
+
+   # parse the buildjob reference and recurse. might be a job in
+   # this flight, in which case we still recurse since it might
+   # be a chain from a non-top-level job which hasn't been
+   # included yet. %jobs will prevent us from duplicating or
+   # infinite loops.
+   my ($oflight, $ojob) = flight_otherjob($tflight, $row->[2]);
+   collect($oflight, "job = ?", $ojob);
+
+   # collect() appends to @rows, so we don't need any special
+   # handling to pickup anything which was newly added.
+}
+}
+
 our @colws;
 sub max ($$) { $_[$_[0] < $_[1]] }
 foreach my $row (@rows) {
@@ -77,7 +103,11 @@ foreach my $row (@rows) {
 }
 $colws[1] += length $synthsufx;
 
-foreach my $row (@rows) {
+# Sort by runvar name, then (flight+)job, synth runvars come last.
+foreach my $row (map { $_->[0] }
+sort { $a->[1] cmp $b->[1] }
+map { [ $_, ($_->[1] =~ m/~$/)." $_->[1] $_->[0]" ] }
+@rows) {
 printf "%-*s %-*s %-*s\n", map { $colws[$_], $row->[$_] } qw(0 1 2)
 or die $!;
 }
-- 
2.6.1


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

Re: [Xen-devel] [PATCH V8 5/5] x86/hvm: pkeys, add pkeys support for cpuid handling

[Jan Beulich]

>>> On 02.02.16 at 08:35,  wrote:
> This patch adds pkeys support for cpuid handing.
> 
> Pkeys hardware support is CPUID.7.0.ECX[3]:PKU. software support is
> CPUID.7.0.ECX[4]:OSPKE and it reflects the support setting of CR4.PKE.
> 
> X86_FEATURE_OSXSAVE depends on guest X86_FEATURE_XSAVE, but cpu_has_xsave
> function reflects hypervisor X86_FEATURE_XSAVE, it is fixed too.
> 
> Signed-off-by: Huaitong Han 
> ---
> Changes in v7:
> *Rebase in the latest tree.
> *Add a comment for cpu_has_xsave adjustment.
> *Adjust indentation.
> 
>  tools/libxc/xc_cpufeature.h |  3 +++
>  tools/libxc/xc_cpuid_x86.c  |  6 --
>  xen/arch/x86/hvm/hvm.c  | 18 +-
>  3 files changed, 20 insertions(+), 7 deletions(-)

This will need a tools maintainer's ack, so upon re-submission you
should Cc them.

> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4572,7 +4572,7 @@ void hvm_cpuid(unsigned int input, unsigned int *eax, 
> unsigned int *ebx,
>  __clear_bit(X86_FEATURE_APIC & 31, edx);
>  
>  /* Fix up OSXSAVE. */
> -if ( cpu_has_xsave )
> +if ( *ecx & cpufeat_mask(X86_FEATURE_XSAVE) )
>  *ecx |= (v->arch.hvm_vcpu.guest_cr[4] & X86_CR4_OSXSAVE) ?
>   cpufeat_mask(X86_FEATURE_OSXSAVE) : 0;

First of all I would have wished that since you're already touching it,
you would have brought it into the same shape as you're doing for
PKU below. And then here as well as ...

> @@ -4593,16 +4593,24 @@ void hvm_cpuid(unsigned int input, unsigned int *eax, 
> unsigned int *ebx,
>  if ( !cpu_has_smap )
>  *ebx &= ~cpufeat_mask(X86_FEATURE_SMAP);
>  
> -/* Don't expose MPX to hvm when VMX support is not available */
> +/* Don't expose MPX to hvm when VMX support is not available. */
>  if ( !(vmx_vmexit_control & VM_EXIT_CLEAR_BNDCFGS) ||
>   !(vmx_vmentry_control & VM_ENTRY_LOAD_BNDCFGS) )
>  *ebx &= ~cpufeat_mask(X86_FEATURE_MPX);
>  
> -/* Don't expose INVPCID to non-hap hvm. */
>  if ( !hap_enabled(d) )
> -*ebx &= ~cpufeat_mask(X86_FEATURE_INVPCID);
> +{
> + /* Don't expose INVPCID to non-hap hvm. */
> + *ebx &= ~cpufeat_mask(X86_FEATURE_INVPCID);
> + /* X86_FEATURE_PKU is not yet implemented for shadow 
> paging. */
> + *ecx &= ~cpufeat_mask(X86_FEATURE_PKU);
> +}
> +
> +if ( (*ecx & cpufeat_mask(X86_FEATURE_PKU)) &&
> + (v->arch.hvm_vcpu.guest_cr[4] & X86_CR4_PKE) )
> +*ecx |= cpufeat_mask(X86_FEATURE_OSPKE);

... here - shouldn't we also clear the respective flag in the "else"
case?

Jan


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

Re: [Xen-devel] [RFC] support more qdisk types

[Ian Campbell]

On Tue, 2016-02-02 at 15:06 -0700, Jim Fehlig wrote:
> 
> > And extending
> > the structure seems to be the right thing to do.
> 
> So what do you think of the approach in the RFC patch? It adds discrete knobs 
> in
> the disk config and extends the disk structure similarly. Before I can make
> progress on this we need to agree on extending the config and 
> libxl_device_disk
> structure.

My main concern is that this approach requires us to update libxl for each
new possible backend type.

The intention of the target= in the disk spec is that it consumes the rest
of the line so it can be used to encode pretty much anything. Is it not
possible (modulo bugs) to pass all the necessary information to qdisk in
this form? I thought Dave S had made it possible to use qdisk in this way
back in:

commit a8a1f236a2964506a22d1779648d8e1c8668cb1a
Author: David Scott 
Date:   Tue Apr 23 10:59:26 2013 +0100

libxl: Only call stat() when adding a disk if we expect a device to 
exist.

We consider calling stat() a helpful error check in the following
circumstances only:
 1. the disk backend type must be PHYsical
 2. the disk backend domain must be the same as the running libxl
code (ie LIBXL_TOOLSTACK_DOMID)
 3. there must not be a hotplug script because this would imply that
the device won't be created until after the hotplug script has
run.

With this fix, it is possible to use qemu's built-in block drivers
such as ceph/rbd, with a xl config disk spec like this:

disk=[ 
'backendtype=qdisk,format=raw,vdev=hda,access=rw,target=rbd:rbd/ubuntu1204.img' 
]

Signed-off-by: David Scott 
Acked-by: Ian Campbell 
Acked-by: Roger Pau Monné 

Of course things may have regressed again since then.

Since target is basically passed as is to qdisk does libvirt not already
have some helpers in the QEMU backend for constructing such things?

Ian.

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

Re: [Xen-devel] [PATCH 1/3] Move collectversions from ts-xen-build into Osstest::BuildSupport

[Ian Campbell]

On Wed, 2016-02-03 at 09:46 +, Ian Campbell wrote:

These 3 should all have been tagged "OSSTEST v2", sorry for the omission.


> I'm going to have a need for it elsewhere.
> 
> Signed-off-by: Ian Campbell 
> ---
>  Osstest/BuildSupport.pm | 12 
>  ts-xen-build| 13 +
>  2 files changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/Osstest/BuildSupport.pm b/Osstest/BuildSupport.pm
> index 933f6e1..a183546 100644
> --- a/Osstest/BuildSupport.pm
> +++ b/Osstest/BuildSupport.pm
> @@ -42,6 +42,7 @@ BEGIN {
>  
>    xendist
>    $xendist
> +  collect_xen_built_versions
>  
>    submodulefixup submodule_have submodule_find
>  
> @@ -84,6 +85,17 @@ sub xendist () {
>   ($ho, 'xendist', '', $r{"buildjob"});
>  }
>  
> +sub collect_xen_built_versions () {
> +my $tools="$builddir/xen/tools";
> +my $extras="$builddir/xen/extras";
> +store_revision($ho, 'qemu', "$tools/ioemu-dir", 1);
> +store_revision($ho, 'qemu', "$tools/qemu-xen-traditional-dir", 1);
> +store_revision($ho, 'qemuu', "$tools/qemu-xen-dir", 1);
> +store_revision($ho, 'seabios', "$tools/firmware/seabios-dir", 1);
> +store_revision($ho, 'ovmf', "$tools/firmware/ovmf-dir", 1);
> +store_revision($ho, 'minios', "$extras/mini-os", 1);
> +}
> +
>  #- submodules -
>  
>  sub submodulefixup () {
> diff --git a/ts-xen-build b/ts-xen-build
> index 8f92729..382fe62 100755
> --- a/ts-xen-build
> +++ b/ts-xen-build
> @@ -151,17 +151,6 @@ END
>  }
>  }
>  
> -sub collectversions () {
> -my $tools="$builddir/xen/tools";
> -my $extras="$builddir/xen/extras";
> -store_revision($ho, 'qemu', "$tools/ioemu-dir", 1);
> -store_revision($ho, 'qemu', "$tools/qemu-xen-traditional-dir", 1);
> -store_revision($ho, 'qemuu', "$tools/qemu-xen-dir", 1);
> -store_revision($ho, 'seabios', "$tools/firmware/seabios-dir", 1);
> -store_revision($ho, 'ovmf', "$tools/firmware/ovmf-dir", 1);
> -store_revision($ho, 'minios', "$extras/mini-os", 1);
> -}
> -
>  sub divide () {
>  # Only move hv to xeninstall, so that we can have
>  # xenpolicy in tools tarball.
> @@ -248,7 +237,7 @@ sub trapping ($) {
>  checkout();
>  
>  trapping(\&build);
> -trapping(\&collectversions);
> +trapping(\&collect_xen_built_versions);
>  
>  die "*** something failed:\n\n".(join "\n\n",@probs)."\n** something
> failed"
>  if @probs;

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

Re: [Xen-devel] [PATCH V8 5/5] x86/hvm: pkeys, add pkeys support for cpuid handling

[Han, Huaitong]

On Wed, 2016-02-03 at 02:50 -0700, Jan Beulich wrote:
> > > > On 02.02.16 at 08:35,  wrote:
> > This patch adds pkeys support for cpuid handing.
> > 
> > Pkeys hardware support is CPUID.7.0.ECX[3]:PKU. software support is
> > CPUID.7.0.ECX[4]:OSPKE and it reflects the support setting of
> > CR4.PKE.
> > 
> > X86_FEATURE_OSXSAVE depends on guest X86_FEATURE_XSAVE, but
> > cpu_has_xsave
> > function reflects hypervisor X86_FEATURE_XSAVE, it is fixed too.
> > 
> > Signed-off-by: Huaitong Han 
> > ---
> > Changes in v7:
> > *Rebase in the latest tree.
> > *Add a comment for cpu_has_xsave adjustment.
> > *Adjust indentation.
> > 
> >  tools/libxc/xc_cpufeature.h |  3 +++
> >  tools/libxc/xc_cpuid_x86.c  |  6 --
> >  xen/arch/x86/hvm/hvm.c  | 18 +-
> >  3 files changed, 20 insertions(+), 7 deletions(-)
> 
> This will need a tools maintainer's ack, so upon re-submission you
> should Cc them.
I will (again) defer this to x86 maintainers. -from wei.l...@citrix.com
> 
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -4572,7 +4572,7 @@ void hvm_cpuid(unsigned int input, unsigned
> > int *eax, unsigned int *ebx,
> >  __clear_bit(X86_FEATURE_APIC & 31, edx);
> >  
> >  /* Fix up OSXSAVE. */
> > -if ( cpu_has_xsave )
> > +if ( *ecx & cpufeat_mask(X86_FEATURE_XSAVE) )
> >  *ecx |= (v->arch.hvm_vcpu.guest_cr[4] &
> > X86_CR4_OSXSAVE) ?
> >   cpufeat_mask(X86_FEATURE_OSXSAVE) : 0;
> 
> First of all I would have wished that since you're already touching
> it,
> you would have brought it into the same shape as you're doing for
> PKU below. And then here as well as ...
It will be updated in V9.
> 
> > @@ -4593,16 +4593,24 @@ void hvm_cpuid(unsigned int input, unsigned
> > int *eax, unsigned int *ebx,
> >  if ( !cpu_has_smap )
> >  *ebx &= ~cpufeat_mask(X86_FEATURE_SMAP);
> >  
> > -/* Don't expose MPX to hvm when VMX support is not
> > available */
> > +/* Don't expose MPX to hvm when VMX support is not
> > available. */
> >  if ( !(vmx_vmexit_control & VM_EXIT_CLEAR_BNDCFGS) ||
> >   !(vmx_vmentry_control & VM_ENTRY_LOAD_BNDCFGS) )
> >  *ebx &= ~cpufeat_mask(X86_FEATURE_MPX);
> >  
> > -/* Don't expose INVPCID to non-hap hvm. */
> >  if ( !hap_enabled(d) )
> > -*ebx &= ~cpufeat_mask(X86_FEATURE_INVPCID);
> > +{
> > + /* Don't expose INVPCID to non-hap hvm. */
> > + *ebx &= ~cpufeat_mask(X86_FEATURE_INVPCID);
> > + /* X86_FEATURE_PKU is not yet implemented for
> > shadow paging. */
> > + *ecx &= ~cpufeat_mask(X86_FEATURE_PKU);
> > +}
> > +
> > +if ( (*ecx & cpufeat_mask(X86_FEATURE_PKU)) &&
> > + (v->arch.hvm_vcpu.guest_cr[4] & X86_CR4_PKE) )
> > +*ecx |= cpufeat_mask(X86_FEATURE_OSPKE);
> 
> ... here - shouldn't we also clear the respective flag in the "else"
> case?
X86_FEATURE_OSPKE is not set by xc_cpuid_hvm_policy(tools), it reflects
the setting of CR4.PKE, so it does not need to be cleared like
X86_CR4_OSXSAVE.
> 
> Jan
> 
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH V8 2/5] x86/hvm: pkeys, add pkeys support for guest_walk_tables

[Han, Huaitong]

On Wed, 2016-02-03 at 02:43 -0700, Jan Beulich wrote:
> > > > On 02.02.16 at 08:35,  wrote:
> > Protection keys define a new 4-bit protection key field(PKEY) in
> > bits 62:59 
> > of
> > leaf entries of the page tables.
> > 
> > PKRU register defines 32 bits, there are 16 domains and 2 attribute
> > bits per
> > domain in pkru, for each i (0 ≤ i ≤ 15), PKRU[2i] is the access
> > -disable bit 
> > for
> > protection key i (ADi); PKRU[2i+1] is the write-disable bit for
> > protection 
> > key
> > i (WDi). PKEY is index to a defined domain.
> > 
> > A fault is considered as a PKU violation if all of the following
> > conditions 
> > are
> > true:
> > 1.CR4_PKE=1.
> > 2.EFER_LMA=1.
> > 3.Page is present with no reserved bit violations.
> > 4.The access is not an instruction fetch.
> > 5.The access is to a user page.
> > 6.PKRU.AD=1
> > or The access is a data write and PKRU.WD=1
> > and either CR0.WP=1 or it is a user access.
> > 
> > Signed-off-by: Huaitong Han 
> 
> Reviewed-by: Jan Beulich 
> albeit ...
> 
> > Changes in v8:
> > *Abstract out _write_cr4.
> 
> ... I'm not happy about the chose name and will try to remember
> to change it to e.g. raw_write_cr4(). Names starting with an
> underscore and a lower case letter are reserved to symbols
> local to a given translation unit, which in my reading doesn't fit
> with them getting placed in header files.
It will be updated in V9.
> 
> Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH] tools: libxencall/foreignmemory: initialise handle->fd

[Ian Campbell]

Otherwise the osdep close on the error path touches an uninitialised
varialble.

CID: 1351231 (foreignmemory) and 1351230 (call)

Signed-off-by: Ian Campbell 
---
 tools/libs/call/core.c  | 2 ++
 tools/libs/foreignmemory/core.c | 1 +
 2 files changed, 3 insertions(+)

diff --git a/tools/libs/call/core.c b/tools/libs/call/core.c
index bbf88de..5ca0372 100644
--- a/tools/libs/call/core.c
+++ b/tools/libs/call/core.c
@@ -24,6 +24,8 @@ xencall_handle *xencall_open(xentoollog_logger *logger, 
unsigned open_flags)
 
 if (!xcall) return NULL;
 
+xcall->fd = -1;
+
 xcall->flags = open_flags;
 xcall->buffer_cache_nr = 0;
 
diff --git a/tools/libs/foreignmemory/core.c b/tools/libs/foreignmemory/core.c
index cfb0a73..6591888 100644
--- a/tools/libs/foreignmemory/core.c
+++ b/tools/libs/foreignmemory/core.c
@@ -27,6 +27,7 @@ xenforeignmemory_handle 
*xenforeignmemory_open(xentoollog_logger *logger,
 
 if (!fmem) return NULL;
 
+fmem->fd = -1;
 fmem->logger = logger;
 fmem->logger_tofree = NULL;
 
-- 
2.1.4


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

[Xen-devel] [PATCH] libxenforeignmemory: handle partial failure correctly

[Ian Campbell]

Coverity rightly points out that checking for ret == NULL and then
calling osdep unmap(ret) is wrong.

The intention on this code path is to turn partial failure into total
failure when the err argument is NULL, so we want to take this patch
whenever ret is _non_ NULL (and err_to_free is set, indicating err was
NULL).

CID: 1351219

Signed-off-by: Ian Campbell 
---
 tools/libs/foreignmemory/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libs/foreignmemory/core.c b/tools/libs/foreignmemory/core.c
index 6591888..a872b95 100644
--- a/tools/libs/foreignmemory/core.c
+++ b/tools/libs/foreignmemory/core.c
@@ -79,7 +79,7 @@ void *xenforeignmemory_map(xenforeignmemory_handle *fmem,
 
 ret = osdep_xenforeignmemory_map(fmem, dom, prot, num, arr, err);
 
-if ( ret == 0 && err_to_free )
+if ( ret && err_to_free )
 {
 int i;
 
-- 
2.1.4


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

Re: [Xen-devel] [PATCH] libxl: Add CPU hotplug support for HVM domains without device model

[Wei Liu]

On Tue, Feb 02, 2016 at 04:02:12PM -0500, Boris Ostrovsky wrote:
> HVMlite domains add/remove VCPUs by toggling "availability" property in
> xenstore.
> 
> Signed-off-by: Boris Ostrovsky 

If this is how it is supposed to work:

Acked-by: Wei Liu 

However, I would like to ask for a follow-up patch to documentation
xen.git hvmlite.markdown (or whichever file you see fit).

> ---
>  tools/libxl/libxl.c |1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 94b5656..ae8bbd8 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -5557,6 +5557,7 @@ int libxl_set_vcpuonline(libxl_ctx *ctx, uint32_t 
> domid, libxl_bitmap *cpumap)
>  case LIBXL_DOMAIN_TYPE_HVM:
>  switch (libxl__device_model_version_running(gc, domid)) {
>  case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
> +case LIBXL_DEVICE_MODEL_VERSION_NONE:
>  rc = libxl__set_vcpuonline_xenstore(gc, domid, cpumap, &info);
>  break;
>  case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
> -- 
> 1.7.1
> 

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

Re: [Xen-devel] [PATCH] tools: libxencall/foreignmemory: initialise handle->fd

[Wei Liu]

On Wed, Feb 03, 2016 at 10:09:42AM +, Ian Campbell wrote:
> Otherwise the osdep close on the error path touches an uninitialised
> varialble.
> 
> CID: 1351231 (foreignmemory) and 1351230 (call)
> 
> Signed-off-by: Ian Campbell 

Acked-by: Wei Liu 

> ---
>  tools/libs/call/core.c  | 2 ++
>  tools/libs/foreignmemory/core.c | 1 +
>  2 files changed, 3 insertions(+)
> 
> diff --git a/tools/libs/call/core.c b/tools/libs/call/core.c
> index bbf88de..5ca0372 100644
> --- a/tools/libs/call/core.c
> +++ b/tools/libs/call/core.c
> @@ -24,6 +24,8 @@ xencall_handle *xencall_open(xentoollog_logger *logger, 
> unsigned open_flags)
>  
>  if (!xcall) return NULL;
>  
> +xcall->fd = -1;
> +
>  xcall->flags = open_flags;
>  xcall->buffer_cache_nr = 0;
>  
> diff --git a/tools/libs/foreignmemory/core.c b/tools/libs/foreignmemory/core.c
> index cfb0a73..6591888 100644
> --- a/tools/libs/foreignmemory/core.c
> +++ b/tools/libs/foreignmemory/core.c
> @@ -27,6 +27,7 @@ xenforeignmemory_handle 
> *xenforeignmemory_open(xentoollog_logger *logger,
>  
>  if (!fmem) return NULL;
>  
> +fmem->fd = -1;
>  fmem->logger = logger;
>  fmem->logger_tofree = NULL;
>  
> -- 
> 2.1.4
> 

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

Re: [Xen-devel] [PATCH] libxenforeignmemory: handle partial failure correctly

[Wei Liu]

On Wed, Feb 03, 2016 at 10:10:01AM +, Ian Campbell wrote:
> Coverity rightly points out that checking for ret == NULL and then
> calling osdep unmap(ret) is wrong.
> 
> The intention on this code path is to turn partial failure into total
> failure when the err argument is NULL, so we want to take this patch
> whenever ret is _non_ NULL (and err_to_free is set, indicating err was
> NULL).
> 
> CID: 1351219
> 
> Signed-off-by: Ian Campbell 

Acked-by: Wei Liu 

> ---
>  tools/libs/foreignmemory/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/libs/foreignmemory/core.c b/tools/libs/foreignmemory/core.c
> index 6591888..a872b95 100644
> --- a/tools/libs/foreignmemory/core.c
> +++ b/tools/libs/foreignmemory/core.c
> @@ -79,7 +79,7 @@ void *xenforeignmemory_map(xenforeignmemory_handle *fmem,
>  
>  ret = osdep_xenforeignmemory_map(fmem, dom, prot, num, arr, err);
>  
> -if ( ret == 0 && err_to_free )
> +if ( ret && err_to_free )
>  {
>  int i;
>  
> -- 
> 2.1.4
> 

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

Re: [Xen-devel] [PATCH 2/3] Add a weekly coverity flight

[Ian Campbell]

On Wed, 2016-02-03 at 09:46 +, Ian Campbell wrote:
> [...]
> +sub build () {
> +my $make = "make $makeflags";
> +
> +# Pre build things we don't want coverity to scan, but which are
> +# normally built by some other command.
> +target_cmd_build($ho, 1000, $builddir, < +cd $builddir/xen
> +./configure
> +$make -C tools/firmware/etherboot all
> +$make mini-os-dir
> +END
> +
> +# Now the stuff we want coverity to look at
> +target_cmd_build($ho, 9000, $builddir, < +cd $builddir/xen
> +export PATH=$builddir/covtools/bin:\$PATH
> +cov-build --dir cov-int $make -C extras/mini-os/
> +cov-build --dir cov-int $make xen tools

This omits building stubdom, which Andy's original script also did.

However stubdom exists as a category in the scan webui and there have
previously been results for stubdoms.

Andy, I presume you deliberately started excluding stubdoms at some point?
I think this is probably the right thing to do, at least for now, since
stubdoms run with guest privileges so aren't hugely interesting, plus they
include an awful lot of third party code which we don't want to be
scanning+triaging (especially given how out of date some of the code is).

Ian.

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

[Xen-devel] [distros-debian-squeeze test] 38724: all pass

[Platform Team regression test user]

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

Perfect :-)
All tests in this flight passed
baseline version:
 flight   38707

jobs:
 build-amd64  pass
 build-armhf  pass
 build-i386   pass
 build-amd64-pvopspass
 build-armhf-pvopspass
 build-i386-pvops pass
 test-amd64-amd64-amd64-squeeze-netboot-pygrubpass
 test-amd64-i386-amd64-squeeze-netboot-pygrub pass
 test-amd64-amd64-i386-squeeze-netboot-pygrub pass
 test-amd64-i386-i386-squeeze-netboot-pygrub  pass



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

[Xen-devel] [libvirt bisection] complete build-i386-libvirt

[osstest service owner]

branch xen-unstable
xenbranch xen-unstable
job build-i386-libvirt
testid libvirt-build

Tree: libvirt git://libvirt.org/libvirt.git
Tree: libvirt_gnulib git://git.sv.gnu.org/gnulib.git
Tree: qemu git://xenbits.xen.org/qemu-xen-traditional.git
Tree: qemuu git://xenbits.xen.org/qemu-xen.git
Tree: xen git://xenbits.xen.org/xen.git

*** Found and reproduced problem changeset ***

  Bug is in tree:  libvirt git://libvirt.org/libvirt.git
  Bug introduced:  a70f3b1c77912012905c6c5be3bf37b05592e80f
  Bug not present: 63e15ad5e093d747db99400c269f71e95bda4bbe
  Last fail repro: http://logs.test-lab.xenproject.org/osstest/logs/80184/


  commit a70f3b1c77912012905c6c5be3bf37b05592e80f
  Author: Michal Privoznik 
  Date:   Sat Jan 30 11:55:45 2016 +0100
  
  includes: Install libvirt-common.h
  
  The libvirt-common.h is build time generated file from .in.
  Obviously, it's generated into builddir and not srcdir. Problem
  is, the list of header files to install, virinc_HEADERS contains
  only $(srcdir)/*.h and this misses libvirt-common.h. This problem
  is pretty obvious when doing a VPATH build.
  
  Signed-off-by: Michal Privoznik 


For bisection revision-tuple graph see:
   
http://logs.test-lab.xenproject.org/osstest/results/bisect/libvirt/build-i386-libvirt.libvirt-build.html
Revision IDs in each graph node refer, respectively, to the Trees above.


Running cs-bisection-step 
--graph-out=/home/logs/results/bisect/libvirt/build-i386-libvirt.libvirt-build 
--summary-out=tmp/80184.bisection-summary --basis-template=79451 
--blessings=real,real-bisect libvirt build-i386-libvirt libvirt-build
Searching for failure / basis pass:
 79891 fail [host=italia0] / 79451 [host=huxelrebe0] 79390 ok.
Failure / basis pass flights: 79891 / 79390
(tree with no url: minios)
(tree with no url: ovmf)
(tree with no url: seabios)
Tree: libvirt git://libvirt.org/libvirt.git
Tree: libvirt_gnulib git://git.sv.gnu.org/gnulib.git
Tree: qemu git://xenbits.xen.org/qemu-xen-traditional.git
Tree: qemuu git://xenbits.xen.org/qemu-xen.git
Tree: xen git://xenbits.xen.org/xen.git
Latest 041f7c9304788ec5c10ee5794612efa614a6eb61 
6cc32c63e80bc1a30c521b2f07f2b54909b59892 
21f6526d1da331611ac5fe12967549d1a04e149b 
2ce1d30ef2858dfed72a281872579e5a26b090dd 
9937763265d9597e5f2439249b16d995842cdf0f
Basis pass 680030c42b7453510f27a41f1778de6d3ade58aa 
6cc32c63e80bc1a30c521b2f07f2b54909b59892 
21f6526d1da331611ac5fe12967549d1a04e149b 
2ce1d30ef2858dfed72a281872579e5a26b090dd 
2e46e3f2539d026594ec1618e7df2c2bc8785b42
Generating revisions with ./adhoc-revtuple-generator  
git://libvirt.org/libvirt.git#680030c42b7453510f27a41f1778de6d3ade58aa-041f7c9304788ec5c10ee5794612efa614a6eb61
 
git://git.sv.gnu.org/gnulib.git#6cc32c63e80bc1a30c521b2f07f2b54909b59892-6cc32c63e80bc1a30c521b2f07f2b54909b59892
 
git://xenbits.xen.org/qemu-xen-traditional.git#21f6526d1da331611ac5fe12967549d1a04e149b-21f6526d1da331611ac5fe12967549d1a04e149b
 
git://xenbits.xen.org/qemu-xen.git#2ce1d30ef2858dfed72a281872579e5a26b090dd-2ce1d30ef2858dfed72a281872579e5a26b090dd
 
git://xenbits.xen.org/xen.git#2e46e3f2539d026594ec1618e7df2c2bc8785b42-9937763265d9597e5f2439249b16d995842cdf0f
>From git://cache:9419/git://libvirt.org/libvirt
   1fe6d8b..1794a01  master -> origin/master
Loaded 2001 nodes in revision graph
Searching for test results:
 79451 [host=huxelrebe0]
 79390 pass 680030c42b7453510f27a41f1778de6d3ade58aa 
6cc32c63e80bc1a30c521b2f07f2b54909b59892 
21f6526d1da331611ac5fe12967549d1a04e149b 
2ce1d30ef2858dfed72a281872579e5a26b090dd 
2e46e3f2539d026594ec1618e7df2c2bc8785b42
 79891 fail 041f7c9304788ec5c10ee5794612efa614a6eb61 
6cc32c63e80bc1a30c521b2f07f2b54909b59892 
21f6526d1da331611ac5fe12967549d1a04e149b 
2ce1d30ef2858dfed72a281872579e5a26b090dd 
9937763265d9597e5f2439249b16d995842cdf0f
 80117 pass 680030c42b7453510f27a41f1778de6d3ade58aa 
6cc32c63e80bc1a30c521b2f07f2b54909b59892 
21f6526d1da331611ac5fe12967549d1a04e149b 
2ce1d30ef2858dfed72a281872579e5a26b090dd 
2e46e3f2539d026594ec1618e7df2c2bc8785b42
 80123 fail 041f7c9304788ec5c10ee5794612efa614a6eb61 
6cc32c63e80bc1a30c521b2f07f2b54909b59892 
21f6526d1da331611ac5fe12967549d1a04e149b 
2ce1d30ef2858dfed72a281872579e5a26b090dd 
9937763265d9597e5f2439249b16d995842cdf0f
 80136 pass f226ecbfbb1231553f2948d6632acdb0b2266300 
6cc32c63e80bc1a30c521b2f07f2b54909b59892 
21f6526d1da331611ac5fe12967549d1a04e149b 
2ce1d30ef2858dfed72a281872579e5a26b090dd 
9937763265d9597e5f2439249b16d995842cdf0f
 80137 fail f73ad5d47e7e9c3d977e72e65b99a3c237a5e1bb 
6cc32c63e80bc1a30c521b2f07f2b54909b59892 
21f6526d1da331611ac5fe12967549d1a04e149b 
2ce1d30ef2858dfed72a281872579e5a26b090dd 
9937763265d9597e5f2439249b16d995842cdf0f
 80169 pass 63e15ad5e093d747db99400c269f71e95bda4bbe 
6cc32c63e80bc1a30c521b2f07f2b54909b59892 
21f6526d1da331611ac5fe12967549d1a04e149b 
2ce1d30ef2858dfed72a281872579e5a26b090dd 
9937763265d9597e5f2439249b16d995842cdf0f
 80173 fail a70f3b1c7791201290

Re: [Xen-devel] [PATCH] libxc: fix uninitialised usage of rc in meminit_hvm

[Ian Campbell]

On Tue, 2016-02-02 at 12:37 +, Wei Liu wrote:
> On Tue, Feb 02, 2016 at 12:33:20PM +0100, Roger Pau Monne wrote:
> > From: Roger Pau Monne 
> > 
> > Due to the HVMlite changes there's a chance that the value in rc is
> > checked
> > without being initialised. Fix this by initialising it to 0.
> > 
> > Signed-off-by: Roger Pau Monné 
> > Reported-by: Olaf Hering 
> 
> Acked-by: Wei Liu 

This is CID 1351229, I think?

** CID 1351229:  Uninitialized variables  (UNINIT)
> /tools/libxc/xc_dom_x86.c: 1443 in meminit_hvm()
> 
> 
> 
> *** CID 1351229:  Uninitialized variables  (UNINIT)
> /tools/libxc/xc_dom_x86.c: 1443 in meminit_hvm()
> 1437 cur_pages = 0xc0;
> 1438 stat_normal_pages += 0xc0;
> 1439 }
> 1440 else
> 1441 cur_pages = vmemranges[vmemid].start >> PAGE_SHIFT;
> 1442 
> >>> CID 1351229:  Uninitialized variables  (UNINIT)
> >>> Using uninitialized value "rc".
> 1443 while ( (rc == 0) && (end_pages > cur_pages) )
> 1444 {
> 1445 /* Clip count to maximum 1GB extent. */
> 1446 unsigned long count = end_pages - cur_pages;
> 1447 unsigned long max_pages = SUPERPAGE_1GB_NR_PFNS;
> 1448    

Note that this while loop ends with:
if ( rc != 0 )
break;
and there are no continue statements.

Therefore I wonder if we would be better off removing the rc == 0 part of
the loop condition?

The issue with this patch is the usual one that it will hide other
unintentional uses of rc before it is set to a good value.

This issue was exposed by a prior "rc = xc_domain_populate_physmap_exact"
becoming conditional on device_model. What is also concerning is the lack
of error checking on that call -- is it really ok to just barrel on under
these circumstance?

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

Re: [Xen-devel] vm_event regression in 4.7

[Andrew Cooper]

On 03/02/16 01:32, Tamas K Lengyel wrote:
>
>
> On Tue, Feb 2, 2016 at 6:00 PM, Andrew Cooper
> mailto:andrew.coop...@citrix.com>> wrote:
>
> On 03/02/2016 00:51, Tamas K Lengyel wrote:
>> Hello all,
>> with the latest master branch of Xen there is a regression
>> enabling vm_event on a domain. If an event listener was
>> previously active on the domain it is now not possible to
>> reenable events as the domctl returns -EINVAL. The problem seems
>> to stem from activating the magic page for vm_event using
>> prepare_ring_for_helper as it returns NULL. Further looking into
>> where things go wrong within that function it seems the page type
>> returned by __get_gfn_type_access is p2m_ram_logdirty with an
>> invalid mfn (0x) and then it hits "Error path:
>> not a suitable GFN at all".
>>
>> Can anyone point me to which change or what may be causing this?
>
> Did the previous event listener replace the page it stole from
> guest physmap for ring purposes when it exited?
>
>
> Ah, here is what seems to be the problem. Previously it was not
> required to do this during teardown. What we had was libxc would check
> if it can map the ring page with xc_map_foreign_pages, and it would
> repopulate the page if it failed before running xc_vm_event_enable.
> However, now it seems xc_map_foreign_pages return non-NULL the second
> time around as well, either though the page is not in the physmap.

This is the bug then.  If there isn't a page in the physmap,
xc_map_foreign_pages() should indicate an error.

> If I enforce libxc to run populate_physmap then I can get vm_event to
> initialize properly again. So the change seems to relate somehow the
> behavior of xc_map_foreign_pages.

This seems likely due to the splitting out of libxenforeignmem from
libxc, which included the the merging of 4? almost identical
map_foreign_$FOO() functions into one.  It is likely that there is a
subtle change in behaviour on an error path.

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

Re: [Xen-devel] [RFC] support more qdisk types

[Wei Liu]

On Tue, Feb 02, 2016 at 03:06:01PM -0700, Jim Fehlig wrote:
> Wei Liu wrote:
> > On Mon, Jan 25, 2016 at 05:25:02PM -0700, Jim Fehlig wrote:
> >> I would like to hear the community's opinion on supporting more qdisk 
> >> types in
> >> xl/libxl, e.g. nbd, rbd, iSCSI, etc. I prefer supporting additional qdisk 
> >> types
> >> in libxl over apps like xl or libvirt doing all the setup, producing a 
> >> block
> >> device, and then passing that to libxl. Each libxl app would have to
> >> re-implement functionality already provided by qdisk. libxl already 
> >> supports
> >> IDE, AHCI, SCSI, and Xen PV qdisks. My suggestion is to extend that to 
> >> initially
> >> include nbd, rbd, and iSCSI. Sheepdog, ssh, etc. could be added in the 
> >> future.
> >>
> > 
> > This is a good idea in general.
> > 
> >> I considered several approaches to supporting additional qdisk types, based
> >> primarily on changes to the disk cfg and interface. At one extreme is to 
> >> change
> >> nothing and use the existing 'target=' to encode all required config for 
> >> the
> >> additional qdisk types. libxl would need to be taught how to turn the blob 
> >> into
> >> an appropriate qdisk. 
> > 
> > I think you mean "libxlu would need to be taught how to turn that blob
> > into an appropriate libxl_device_disk" -- libxl doesn't parse user
> > configuration, it deals with the libxl_device_disk directly to construct
> > arguments for QEMU.
> 
> Right. But if the configuration is all encoded in 'target=', libxlu already
> parses that and puts it in libxl_device_disk.pdev_path.
> 
> > Either it is done by extending target= or adding discrete options, the
> > heavy lifting is going to be in libxlu.  I don't think we want to parse
> > a string inside libxl.
> 
> Ok. So in your opinion, even if any new disk config is encoded in 'target=',
> libxlu should split that up into (new) members of libxl_device_disk, not just
> plop it into libxl_device_disk.pdev_path?
> 

No, not necessarily. I didn't look closely in the code yesterday when
replying, sorry.

If  target= has always been shoveled into pdev_path, using that would be
fine. We already have mechanism to parse target= outside of libxl in
hotplug script.

Are you aware of all those hotplug scripts living under tools/hotplug ?
Does using hotplug script sound plausible to you?

Currently hotplug script for QEMU is broken and needs fixing though, but
I'm sure we can figure it out.

Wei.

> >> At the other extreme is extending xl-disk-configuration
> >> with discrete knobs for each possible config item and making the
> >> libxl_device_disk structure more hierarchical. E.g.
> >>
> > 
> > I don't think the second half (hierarchical structure) is closely tied
> > to whether target= is extended or discrete knobs are used.
> 
> Nod.
> 
> > And extending
> > the structure seems to be the right thing to do.
> 
> So what do you think of the approach in the RFC patch? It adds discrete knobs 
> in
> the disk config and extends the disk structure similarly. Before I can make
> progress on this we need to agree on extending the config and 
> libxl_device_disk
> structure.
> 
> Regards,
> Jim
> 

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

[Xen-devel] Leaks in xc_tbuf_get_size() (Was: Re: New Defects reported by Coverity Scan for XenProject)

[Ian Campbell]

George,

Looks like xentrace is the only maintained component which uses this. so
tag ;-)

On Tue, 2016-02-02 at 20:23 -0800, scan-ad...@coverity.com wrote:
> * CID 1351228:    (RESOURCE_LEAK)
> /tools/libxc/xc_tbuf.c: 73 in xc_tbuf_get_size()
> /tools/libxc/xc_tbuf.c: 77 in xc_tbuf_get_size()

Coverity is reporting these as new in 41b0aa569adb..9937763265d9 although
the file hasn't changed. However it does look correct that t_info is being
leaked by various paths in this function.

> 
> 
> _
> ___
> *** CID 1351228:    (RESOURCE_LEAK)
> /tools/libxc/xc_tbuf.c: 73 in xc_tbuf_get_size()
> 67 
> 68 t_info = xc_map_foreign_range(xch, DOMID_XEN,
> 69 sysctl.u.tbuf_op.size, PROT_READ | PROT_WRITE,
> 70 sysctl.u.tbuf_op.buffer_mfn);
> 71 
> 72 if ( t_info == NULL || t_info->tbuf_size == 0 )
> >>> CID 1351228:    (RESOURCE_LEAK)
> >>> Variable "t_info" going out of scope leaks the storage it points
> to.
> 73 return -1;
> 74 
> 75 *size = t_info->tbuf_size;
> 76 
> 77 return 0;
> 78 }
> /tools/libxc/xc_tbuf.c: 77 in xc_tbuf_get_size()
> 71 
> 72 if ( t_info == NULL || t_info->tbuf_size == 0 )
> 73 return -1;
> 74 
> 75 *size = t_info->tbuf_size;
> 76 
> >>> CID 1351228:    (RESOURCE_LEAK)
> >>> Variable "t_info" going out of scope leaks the storage it points
> to.
> 77 return 0;
> 78 }
> 79 
> 80 int xc_tbuf_enable(xc_interface *xch, unsigned long pages,
> unsigned long *mfn,
> 81    unsigned long *size)
> 82 {
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] Leak in xc_dom_load_hvm_kernel() (Was; Re: New Defects reported by Coverity Scan for XenProject)

[Ian Campbell]

Roger,

On Tue, 2016-02-02 at 20:23 -0800, scan-ad...@coverity.com wrote:
> ** CID 1351227:    (RESOURCE_LEAK)
> /tools/libxc/xc_dom_hvmloader.c: 260 in xc_dom_load_hvm_kernel()
> /tools/libxc/xc_dom_hvmloader.c: 270 in xc_dom_load_hvm_kernel()
> /tools/libxc/xc_dom_hvmloader.c: 277 in xc_dom_load_hvm_kernel()

Looks like this came from ad787bafcd2a3058f0f37f2fe84931bd5136bde9?

> 
> *** CID 1351227:    (RESOURCE_LEAK)
> /tools/libxc/xc_dom_hvmloader.c: 260 in xc_dom_load_hvm_kernel()
> 254 elf->dest_size = pages * XC_DOM_PAGE_SIZE(dom);
> 255 
> 256 rc = elf_load_binary(elf);
> 257 if ( rc < 0 )
> 258 {
> 259 DOMPRINTF("%s: failed to load elf binary", __func__);
> >>> CID 1351227:    (RESOURCE_LEAK)
> >>> Variable "entries" going out of scope leaks the storage it points to.
> 260 return rc;
> 261 }
> 262 
> 263 munmap(elf->dest_base, elf->dest_size);
> 264 
> 265 rc = modules_init(dom, dom->total_pages << PAGE_SHIFT, elf, 
> &m_start,
> /tools/libxc/xc_dom_hvmloader.c: 270 in xc_dom_load_hvm_kernel()
> 264 
> 265 rc = modules_init(dom, dom->total_pages << PAGE_SHIFT, elf, 
> &m_start,
> 266   &m_end);
> 267 if ( rc != 0 )
> 268 {
> 269 DOMPRINTF("%s: insufficient space to load modules.", 
> __func__);
> >>> CID 1351227:    (RESOURCE_LEAK)
> >>> Variable "entries" going out of scope leaks the storage it points to.
> 270 return rc;
> 271 }
> 272 
> 273 rc = loadmodules(dom, m_start, m_end, dom->guest_domid);
> 274 if ( rc != 0 )
> 275 {
> /tools/libxc/xc_dom_hvmloader.c: 277 in xc_dom_load_hvm_kernel()
> 271 }
> 272 
> 273 rc = loadmodules(dom, m_start, m_end, dom->guest_domid);
> 274 if ( rc != 0 )
> 275 {
> 276 DOMPRINTF("%s: unable to load modules.", __func__);
> >>> CID 1351227:    (RESOURCE_LEAK)
> >>> Variable "entries" going out of scope leaks the storage it points to.
> 277 return rc;
> 278 }
> 279 
> 280 dom->parms.phys_entry = elf_uval(elf, elf->ehdr, e_entry);
> 281 
> 282 free(entries);
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] Leaks in xc_tbuf_get_size() (Was: Re: New Defects reported by Coverity Scan for XenProject)

[Andrew Cooper]

On 03/02/16 10:37, Ian Campbell wrote:
> George,
>
> Looks like xentrace is the only maintained component which uses this. so
> tag ;-)
>
> On Tue, 2016-02-02 at 20:23 -0800, scan-ad...@coverity.com wrote:
>> * CID 1351228:(RESOURCE_LEAK)
>> /tools/libxc/xc_tbuf.c: 73 in xc_tbuf_get_size()
>> /tools/libxc/xc_tbuf.c: 77 in xc_tbuf_get_size()
> Coverity is reporting these as new in 41b0aa569adb..9937763265d9 although
> the file hasn't changed. However it does look correct that t_info is being
> leaked by various paths in this function.

It is "new"ly spotted because xc_map_foreign_range() is now a straight
function call rather than a function pointer pointing at functions with
different behaviours.

~Andrew

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

Re: [Xen-devel] [PATCH] libxc: fix uninitialised usage of rc in meminit_hvm

[Wei Liu]

On Wed, Feb 03, 2016 at 10:30:54AM +, Ian Campbell wrote:
> On Tue, 2016-02-02 at 12:37 +, Wei Liu wrote:
> > On Tue, Feb 02, 2016 at 12:33:20PM +0100, Roger Pau Monne wrote:
> > > From: Roger Pau Monne 
> > > 
> > > Due to the HVMlite changes there's a chance that the value in rc is
> > > checked
> > > without being initialised. Fix this by initialising it to 0.
> > > 
> > > Signed-off-by: Roger Pau Monné 
> > > Reported-by: Olaf Hering 
> > 
> > Acked-by: Wei Liu 
> 
> This is CID 1351229, I think?
> 

Yes, I think so.

> ** CID 1351229:  Uninitialized variables  (UNINIT)
> > /tools/libxc/xc_dom_x86.c: 1443 in meminit_hvm()
> > 
> > 
> > 
> > *** CID 1351229:  Uninitialized variables  (UNINIT)
> > /tools/libxc/xc_dom_x86.c: 1443 in meminit_hvm()
> > 1437 cur_pages = 0xc0;
> > 1438 stat_normal_pages += 0xc0;
> > 1439 }
> > 1440 else
> > 1441 cur_pages = vmemranges[vmemid].start >> PAGE_SHIFT;
> > 1442 
> > >>> CID 1351229:  Uninitialized variables  (UNINIT)
> > >>> Using uninitialized value "rc".
> > 1443 while ( (rc == 0) && (end_pages > cur_pages) )
> > 1444 {
> > 1445 /* Clip count to maximum 1GB extent. */
> > 1446 unsigned long count = end_pages - cur_pages;
> > 1447 unsigned long max_pages = SUPERPAGE_1GB_NR_PFNS;
> > 1448    
> 
> Note that this while loop ends with:
> if ( rc != 0 )
> break;
> and there are no continue statements.
> 
> Therefore I wonder if we would be better off removing the rc == 0 part of
> the loop condition?
> 

No, there is no if ( rc != 0 )  inside the said while loop.  That "if"
is for the outer for loop.

We could add a test for the while loop, if that looks clearer to you.

> The issue with this patch is the usual one that it will hide other
> unintentional uses of rc before it is set to a good value.
> 
> This issue was exposed by a prior "rc = xc_domain_populate_physmap_exact"
> becoming conditional on device_model. What is also concerning is the lack
> of error checking on that call -- is it really ok to just barrel on under
> these circumstance?
> 

Yeah, that should ideally be fixed, too.

Wei.

> Ian.

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

[Xen-devel] [PATCH] tools: xenconsole: cleanup when clock_gettime fails.

[Ian Campbell]

All other error paths in the infinite loop in handle_io use break, so
as to free resources.

CID: 1351226

Signed-off-by: Ian Campbell 
---
 tools/console/daemon/io.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
index e2e7a6b..34666c4 100644
--- a/tools/console/daemon/io.c
+++ b/tools/console/daemon/io.c
@@ -1053,7 +1053,7 @@ void handle_io(void)
 POLLIN|POLLPRI);
 
if (clock_gettime(CLOCK_MONOTONIC, &ts) < 0)
-   return;
+   break;
now = ((long long)ts.tv_sec * 1000) + (ts.tv_nsec / 100);
 
/* Re-calculate any event counter allowances & unblock
-- 
2.1.4


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

Re: [Xen-devel] [PATCH] libxc: fix uninitialised usage of rc in meminit_hvm

[Roger Pau Monné]

El 3/2/16 a les 11:30, Ian Campbell ha escrit:
> On Tue, 2016-02-02 at 12:37 +, Wei Liu wrote:
>> On Tue, Feb 02, 2016 at 12:33:20PM +0100, Roger Pau Monne wrote:
>>> From: Roger Pau Monne 
>>>
>>> Due to the HVMlite changes there's a chance that the value in rc is
>>> checked
>>> without being initialised. Fix this by initialising it to 0.
>>>
>>> Signed-off-by: Roger Pau Monné 
>>> Reported-by: Olaf Hering 
>>
>> Acked-by: Wei Liu 
> 
> This is CID 1351229, I think?

Looks like, according the the description below.

> 
> ** CID 1351229:  Uninitialized variables  (UNINIT)
>> /tools/libxc/xc_dom_x86.c: 1443 in meminit_hvm()
>>  
>>  
>> 
>> *** CID 1351229:  Uninitialized variables  (UNINIT)
>> /tools/libxc/xc_dom_x86.c: 1443 in meminit_hvm()
>> 1437 cur_pages = 0xc0;
>> 1438 stat_normal_pages += 0xc0;
>> 1439 }
>> 1440 else
>> 1441 cur_pages = vmemranges[vmemid].start >> PAGE_SHIFT;
>> 1442 
>  CID 1351229:  Uninitialized variables  (UNINIT)
>  Using uninitialized value "rc".
>> 1443 while ( (rc == 0) && (end_pages > cur_pages) )
>> 1444 {
>> 1445 /* Clip count to maximum 1GB extent. */
>> 1446 unsigned long count = end_pages - cur_pages;
>> 1447 unsigned long max_pages = SUPERPAGE_1GB_NR_PFNS;
>> 1448
> 
> Note that this while loop ends with:
> if ( rc != 0 )
> break;
> and there are no continue statements.
> 
> Therefore I wonder if we would be better off removing the rc == 0 part of
> the loop condition?

We could, but I think we would still have the same issue with the "if (
rc != 0 )" at the end of the loop, AFAICT rc is never unconditionally
set inside of the loop itself, so gcc and coverity would still complain
about uninitialized usage.

> The issue with this patch is the usual one that it will hide other
> unintentional uses of rc before it is set to a good value.
> 
> This issue was exposed by a prior "rc = xc_domain_populate_physmap_exact"
> becoming conditional on device_model. What is also concerning is the lack
> of error checking on that call -- is it really ok to just barrel on under
> these circumstance?

Hm, I guess we then rely on the rc == 0 at the start of the while loop
in order to bail out. IMHO the logic in this function is overly complicated.

Roger.

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

[Xen-devel] missing lock in percpu_rwlock? (Was: Re: New Defects reported by Coverity Scan for XenProject)

[Ian Campbell]

On Tue, 2016-02-02 at 20:23 -0800, scan-ad...@coverity.com wrote:
> * CID 1351223:  Concurrent data access violations  (MISSING_LOCK)
> /xen/include/xen/spinlock.h: 362 in _percpu_write_unlock()

Coverity seems to think this is new in 41b0aa569adb..9937763265d,
presumably due to 

commit f9dd43dddc0a31a4343a58072935c1b5c0cbbee
Author: Malcolm Crossley 
Date:   Fri Jan 22 16:04:41 2016 +0100

rwlock: add per-cpu reader-writer lock infrastructure

> _
> ___
> *** CID 1351223:  Concurrent data access violations  (MISSING_LOCK)
> /xen/include/xen/spinlock.h: 362 in _percpu_write_unlock()
> 356 percpu_rwlock_t *percpu_rwlock)
> 357 {
> 358 /* Validate the correct per_cpudata variable has been
> provided. */
> 359 _percpu_rwlock_owner_check(per_cpudata, percpu_rwlock);
> 360 
> 361 ASSERT(percpu_rwlock->writer_activating);
> >>> CID 1351223:  Concurrent data access violations  (MISSING_LOCK)
> >>> Accessing "percpu_rwlock->writer_activating" without holding lock
> "percpu_rwlock.rwlock". Elsewhere, "percpu_rwlock.writer_activating" is
> accessed with "percpu_rwlock.rwlock" held 1 out of 2 times (1 of these
> accesses strongly imply that it is necessary).
> 362 percpu_rwlock->writer_activating = 0;
> 363 write_unlock(&percpu_rwlock->rwlock);
> 364 }
> 365 
> 366 #define percpu_rw_is_write_locked(l)
> _rw_is_write_locked(&((l)->rwlock))
> 367 

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

Re: [Xen-devel] [PATCH 2/3] Add a weekly coverity flight

[Andrew Cooper]

On 03/02/16 10:19, Ian Campbell wrote:
> On Wed, 2016-02-03 at 09:46 +, Ian Campbell wrote:
>>  [...]
>> +sub build () {
>> +my $make = "make $makeflags";
>> +
>> +# Pre build things we don't want coverity to scan, but which are
>> +# normally built by some other command.
>> +target_cmd_build($ho, 1000, $builddir, <> +cd $builddir/xen
>> +./configure
>> +$make -C tools/firmware/etherboot all
>> +$make mini-os-dir
>> +END
>> +
>> +# Now the stuff we want coverity to look at
>> +target_cmd_build($ho, 9000, $builddir, <> +cd $builddir/xen
>> +export PATH=$builddir/covtools/bin:\$PATH
>> +cov-build --dir cov-int $make -C extras/mini-os/
>> +cov-build --dir cov-int $make xen tools
> This omits building stubdom, which Andy's original script also did.
>
> However stubdom exists as a category in the scan webui and there have
> previously been results for stubdoms.
>
> Andy, I presume you deliberately started excluding stubdoms at some point?
> I think this is probably the right thing to do, at least for now, since
> stubdoms run with guest privileges so aren't hugely interesting, plus they
> include an awful lot of third party code which we don't want to be
> scanning+triaging (especially given how out of date some of the code is).

Correct.  That is precisely why I prevented stubdom and etherboot from
being scanned.

~Andrew

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

Re: [Xen-devel] missing lock in percpu_rwlock? (Was: Re: New Defects reported by Coverity Scan for XenProject)

[Ian Campbell]

On Wed, 2016-02-03 at 10:45 +, Ian Campbell wrote:
> On Tue, 2016-02-02 at 20:23 -0800, scan-ad...@coverity.com wrote:
> > * CID 1351223:  Concurrent data access violations  (MISSING_LOCK)
> > /xen/include/xen/spinlock.h: 362 in _percpu_write_unlock()
> 
> Coverity seems to think this is new in 41b0aa569adb..9937763265d,
> presumably due to 
> 
> commit f9dd43dddc0a31a4343a58072935c1b5c0cbbee
> Author: Malcolm Crossley 
> Date:   Fri Jan 22 16:04:41 2016 +0100
> 
> rwlock: add per-cpu reader-writer lock infrastructure

It also reports this one, but I suppose this is a false +ve given the name
of the function.

(Also note "simulatenously" should be "simultaneously")

** CID 1351220:  Program hangs  (LOCK)
/xen/include/xen/spinlock.h: 310 in _percpu_read_lock()




*** CID 1351220:  Program hangs  (LOCK)
/xen/include/xen/spinlock.h: 310 in _percpu_read_lock()
304  * Detect using a second percpu_rwlock_t simulatenously and 
fallback
305  * to standard read_lock.
306  */
307 if ( unlikely(this_cpu_ptr(per_cpudata) != NULL ) )
308 {
309 read_lock(&percpu_rwlock->rwlock);
>>> CID 1351220:  Program hangs  (LOCK)
>>> Returning without unlocking "percpu_rwlock->rwlock".
310 return;
311 }
312 
313 /* Indicate this cpu is reading. */
314 this_cpu_ptr(per_cpudata) = percpu_rwlock;
315 smp_mb();


> 
> > ___
> > __
> > ___
> > *** CID 1351223:  Concurrent data access violations  (MISSING_LOCK)
> > /xen/include/xen/spinlock.h: 362 in _percpu_write_unlock()
> > 356 percpu_rwlock_t *percpu_rwlock)
> > 357 {
> > 358 /* Validate the correct per_cpudata variable has been
> > provided. */
> > 359 _percpu_rwlock_owner_check(per_cpudata, percpu_rwlock);
> > 360 
> > 361 ASSERT(percpu_rwlock->writer_activating);
> > > > >  CID 1351223:  Concurrent data access violations 
> > > > > (MISSING_LOCK)
> > > > >  Accessing "percpu_rwlock->writer_activating" without holding
> > > > > lock
> > "percpu_rwlock.rwlock". Elsewhere, "percpu_rwlock.writer_activating" is
> > accessed with "percpu_rwlock.rwlock" held 1 out of 2 times (1 of these
> > accesses strongly imply that it is necessary).
> > 362 percpu_rwlock->writer_activating = 0;
> > 363 write_unlock(&percpu_rwlock->rwlock);
> > 364 }
> > 365 
> > 366 #define percpu_rw_is_write_locked(l)
> > _rw_is_write_locked(&((l)->rwlock))

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

Re: [Xen-devel] [PATCH] tools: xenconsole: cleanup when clock_gettime fails.

[Wei Liu]

On Wed, Feb 03, 2016 at 10:43:47AM +, Ian Campbell wrote:
> All other error paths in the infinite loop in handle_io use break, so
> as to free resources.
> 
> CID: 1351226
> 
> Signed-off-by: Ian Campbell 

Acked-by: Wei Liu 

> ---
>  tools/console/daemon/io.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
> index e2e7a6b..34666c4 100644
> --- a/tools/console/daemon/io.c
> +++ b/tools/console/daemon/io.c
> @@ -1053,7 +1053,7 @@ void handle_io(void)
>POLLIN|POLLPRI);
>  
>   if (clock_gettime(CLOCK_MONOTONIC, &ts) < 0)
> - return;
> + break;
>   now = ((long long)ts.tv_sec * 1000) + (ts.tv_nsec / 100);
>  
>   /* Re-calculate any event counter allowances & unblock
> -- 
> 2.1.4
> 

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

Re: [Xen-devel] missing lock in percpu_rwlock? (Was: Re: New Defects reported by Coverity Scan for XenProject)

[Andrew Cooper]

On 03/02/16 10:45, Ian Campbell wrote:
> On Tue, 2016-02-02 at 20:23 -0800, scan-ad...@coverity.com wrote:
>> * CID 1351223:  Concurrent data access violations  (MISSING_LOCK)
>> /xen/include/xen/spinlock.h: 362 in _percpu_write_unlock()
> Coverity seems to think this is new in 41b0aa569adb..9937763265d,
> presumably due to 
>
> commit f9dd43dddc0a31a4343a58072935c1b5c0cbbee
> Author: Malcolm Crossley 
> Date:   Fri Jan 22 16:04:41 2016 +0100
>
> rwlock: add per-cpu reader-writer lock infrastructure

Expected behaviour.  writer_activating is expected to only be written
under lock, but read without lock.

~Andrew

>
>> _
>> ___
>> *** CID 1351223:  Concurrent data access violations  (MISSING_LOCK)
>> /xen/include/xen/spinlock.h: 362 in _percpu_write_unlock()
>> 356 percpu_rwlock_t *percpu_rwlock)
>> 357 {
>> 358 /* Validate the correct per_cpudata variable has been
>> provided. */
>> 359 _percpu_rwlock_owner_check(per_cpudata, percpu_rwlock);
>> 360 
>> 361 ASSERT(percpu_rwlock->writer_activating);
>  CID 1351223:  Concurrent data access violations  (MISSING_LOCK)
>  Accessing "percpu_rwlock->writer_activating" without holding lock
>> "percpu_rwlock.rwlock". Elsewhere, "percpu_rwlock.writer_activating" is
>> accessed with "percpu_rwlock.rwlock" held 1 out of 2 times (1 of these
>> accesses strongly imply that it is necessary).
>> 362 percpu_rwlock->writer_activating = 0;
>> 363 write_unlock(&percpu_rwlock->rwlock);
>> 364 }
>> 365 
>> 366 #define percpu_rw_is_write_locked(l)
>> _rw_is_write_locked(&((l)->rwlock))
>> 367 
> ___
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel


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

Re: [Xen-devel] [RFC] support more qdisk types

[Ian Campbell]

On Wed, 2016-02-03 at 10:35 +, Wei Liu wrote:
> > Ok. So in your opinion, even if any new disk config is encoded in 'target=',
> > libxlu should split that up into (new) members of libxl_device_disk, not 
> > just
> > plop it into libxl_device_disk.pdev_path?
> > 
> 
> No, not necessarily. I didn't look closely in the code yesterday when
> replying, sorry.
> 
> If  target= has always been shoveled into pdev_path, using that would be
> fine. We already have mechanism to parse target= outside of libxl in
> hotplug script.
> 
> Are you aware of all those hotplug scripts living under tools/hotplug ?
> Does using hotplug script sound plausible to you?
> 
> Currently hotplug script for QEMU is broken and needs fixing though, but
> I'm sure we can figure it out.

How do hotplug scripts factor into this?

Ian.

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

Re: [Xen-devel] [PATCH] libxc: fix uninitialised usage of rc in meminit_hvm

[Ian Campbell]

On Wed, 2016-02-03 at 11:44 +0100, Roger Pau Monné wrote:
> El 3/2/16 a les 11:30, Ian Campbell ha escrit:
> > On Tue, 2016-02-02 at 12:37 +, Wei Liu wrote:
> > > On Tue, Feb 02, 2016 at 12:33:20PM +0100, Roger Pau Monne wrote:
> > > > From: Roger Pau Monne 
> > > > 
> > > > Due to the HVMlite changes there's a chance that the value in rc is
> > > > checked
> > > > without being initialised. Fix this by initialising it to 0.
> > > > 
> > > > Signed-off-by: Roger Pau Monné 
> > > > Reported-by: Olaf Hering 
> > > 
> > > Acked-by: Wei Liu 
> > 
> > This is CID 1351229, I think?
> 
> Looks like, according the the description below.
> 
> > 
> > ** CID 1351229:  Uninitialized variables  (UNINIT)
> > > /tools/libxc/xc_dom_x86.c: 1443 in meminit_hvm()
> > >  
> > >  
> > > _
> > > ___
> > > *** CID 1351229:  Uninitialized variables  (UNINIT)
> > > /tools/libxc/xc_dom_x86.c: 1443 in meminit_hvm()
> > > 1437 cur_pages = 0xc0;
> > > 1438 stat_normal_pages += 0xc0;
> > > 1439 }
> > > 1440 else
> > > 1441 cur_pages = vmemranges[vmemid].start >>
> > > PAGE_SHIFT;
> > > 1442 
> > > > > >  CID 1351229:  Uninitialized variables  (UNINIT)
> > > > > >  Using uninitialized value "rc".
> > > 1443 while ( (rc == 0) && (end_pages > cur_pages) )
> > > 1444 {
> > > 1445 /* Clip count to maximum 1GB extent. */
> > > 1446 unsigned long count = end_pages - cur_pages;
> > > 1447 unsigned long max_pages = SUPERPAGE_1GB_NR_PFNS;
> > > 1448
> > 
> > Note that this while loop ends with:
> > if ( rc != 0 )
> > break;
> > and there are no continue statements.
> > 
> > Therefore I wonder if we would be better off removing the rc == 0 part
> > of
> > the loop condition?
> 
> We could, but I think we would still have the same issue with the "if (
> rc != 0 )" at the end of the loop, AFAICT rc is never unconditionally
> set inside of the loop itself, so gcc and coverity would still complain
> about uninitialized usage.

Right, I was looking at the wrong loop as Wei pointed out.

I think "rc = 0" before the while might be a reasonable option.

> > The issue with this patch is the usual one that it will hide other
> > unintentional uses of rc before it is set to a good value.
> > 
> > This issue was exposed by a prior "rc =
> > xc_domain_populate_physmap_exact"
> > becoming conditional on device_model. What is also concerning is the
> > lack
> > of error checking on that call -- is it really ok to just barrel on
> > under
> > these circumstance?
> 
> Hm, I guess we then rely on the rc == 0 at the start of the while loop
> in order to bail out. IMHO the logic in this function is overly
> complicated.

Indeed, although we do some other (I suppose pointless) work first in that
case too.

Moving some of it into separate helpers would be a nice further cleanup.

Ian.

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

Re: [Xen-devel] Leaks in xc_tbuf_get_size() (Was: Re: New Defects reported by Coverity Scan for XenProject)

[Ian Campbell]

On Wed, 2016-02-03 at 10:42 +, Andrew Cooper wrote:
> On 03/02/16 10:37, Ian Campbell wrote:
> > George,
> > 
> > Looks like xentrace is the only maintained component which uses this.
> > so
> > tag ;-)
> > 
> > On Tue, 2016-02-02 at 20:23 -0800, scan-ad...@coverity.com wrote:
> > > * CID 1351228:(RESOURCE_LEAK)
> > > /tools/libxc/xc_tbuf.c: 73 in xc_tbuf_get_size()
> > > /tools/libxc/xc_tbuf.c: 77 in xc_tbuf_get_size()
> > Coverity is reporting these as new in 41b0aa569adb..9937763265d9
> > although
> > the file hasn't changed. However it does look correct that t_info is
> > being
> > leaked by various paths in this function.
> 
> It is "new"ly spotted because xc_map_foreign_range() is now a straight
> function call rather than a function pointer pointing at functions with
> different behaviours.

That makes sense, thanks.

Ian.

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

Re: [Xen-devel] [RFC] support more qdisk types

[Wei Liu]

On Wed, Feb 03, 2016 at 10:51:27AM +, Ian Campbell wrote:
> On Wed, 2016-02-03 at 10:35 +, Wei Liu wrote:
> > > Ok. So in your opinion, even if any new disk config is encoded in 
> > > 'target=',
> > > libxlu should split that up into (new) members of libxl_device_disk, not 
> > > just
> > > plop it into libxl_device_disk.pdev_path?
> > > 
> > 
> > No, not necessarily. I didn't look closely in the code yesterday when
> > replying, sorry.
> > 
> > If  target= has always been shoveled into pdev_path, using that would be
> > fine. We already have mechanism to parse target= outside of libxl in
> > hotplug script.
> > 
> > Are you aware of all those hotplug scripts living under tools/hotplug ?
> > Does using hotplug script sound plausible to you?
> > 
> > Currently hotplug script for QEMU is broken and needs fixing though, but
> > I'm sure we can figure it out.
> 
> How do hotplug scripts factor into this?
> 

If supporting all such block devices  requires presenting a block device
to QEMU? If QEMU directly handles them then hotplug script is not in the
picture.

Wei.

> Ian.

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

Re: [Xen-devel] missing lock in percpu_rwlock? (Was: Re: New Defects reported by Coverity Scan for XenProject)

[Ian Campbell]

On Wed, 2016-02-03 at 10:50 +, Andrew Cooper wrote:
> On 03/02/16 10:45, Ian Campbell wrote:
> > On Tue, 2016-02-02 at 20:23 -0800, scan-ad...@coverity.com wrote:
> > > * CID 1351223:  Concurrent data access violations  (MISSING_LOCK)
> > > /xen/include/xen/spinlock.h: 362 in _percpu_write_unlock()
> > Coverity seems to think this is new in 41b0aa569adb..9937763265d,
> > presumably due to 
> > 
> > commit f9dd43dddc0a31a4343a58072935c1b5c0cbbee
> > Author: Malcolm Crossley 
> > Date:   Fri Jan 22 16:04:41 2016 +0100
> > 
> > rwlock: add per-cpu reader-writer lock infrastructure
> 
> Expected behaviour.  writer_activating is expected to only be written
> under lock, but read without lock.

I suppose this is something we should eventually model?

Would you typically mark this as "False positive" or "Intentional"?

I just marked a couple of libxl ones about taking ctx->lock (which is
recursive) twice as "False positive", but perhaps "Intentional" is the
correct designation there?

Ian.


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

[Xen-devel] [PATCH] libxc: fix leak in xc_dom_load_hvm_kernel error path

[Roger Pau Monne]

Error path in xc_dom_load_hvm_kernel needs to use the 'error' label instead
of directly returning. This is needed so the entries local variable is
freed.

Coverity-ID: 1351227
Signed-off-by: Roger Pau Monné 
---
Cc: Ian Jackson 
Cc: Ian Campbell 
Cc: Wei Liu 
---
 tools/libxc/xc_dom_hvmloader.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/libxc/xc_dom_hvmloader.c b/tools/libxc/xc_dom_hvmloader.c
index 79a3b99..330d5e8 100644
--- a/tools/libxc/xc_dom_hvmloader.c
+++ b/tools/libxc/xc_dom_hvmloader.c
@@ -257,7 +257,7 @@ static elf_errorstatus xc_dom_load_hvm_kernel(struct 
xc_dom_image *dom)
 if ( rc < 0 )
 {
 DOMPRINTF("%s: failed to load elf binary", __func__);
-return rc;
+goto error;
 }
 
 munmap(elf->dest_base, elf->dest_size);
@@ -267,14 +267,14 @@ static elf_errorstatus xc_dom_load_hvm_kernel(struct 
xc_dom_image *dom)
 if ( rc != 0 )
 {
 DOMPRINTF("%s: insufficient space to load modules.", __func__);
-return rc;
+goto error;
 }
 
 rc = loadmodules(dom, m_start, m_end, dom->guest_domid);
 if ( rc != 0 )
 {
 DOMPRINTF("%s: unable to load modules.", __func__);
-return rc;
+goto error;
 }
 
 dom->parms.phys_entry = elf_uval(elf, elf->ehdr, e_entry);
-- 
2.5.4 (Apple Git-61)


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

Re: [Xen-devel] [RFC] support more qdisk types

[Ian Campbell]

On Wed, 2016-02-03 at 10:55 +, Wei Liu wrote:
> On Wed, Feb 03, 2016 at 10:51:27AM +, Ian Campbell wrote:
> > On Wed, 2016-02-03 at 10:35 +, Wei Liu wrote:
> > > > Ok. So in your opinion, even if any new disk config is encoded in
> > > > 'target=',
> > > > libxlu should split that up into (new) members of
> > > > libxl_device_disk, not just
> > > > plop it into libxl_device_disk.pdev_path?
> > > > 
> > > 
> > > No, not necessarily. I didn't look closely in the code yesterday when
> > > replying, sorry.
> > > 
> > > If  target= has always been shoveled into pdev_path, using that would
> > > be
> > > fine. We already have mechanism to parse target= outside of libxl in
> > > hotplug script.
> > > 
> > > Are you aware of all those hotplug scripts living under tools/hotplug
> > > ?
> > > Does using hotplug script sound plausible to you?
> > > 
> > > Currently hotplug script for QEMU is broken and needs fixing though,
> > > but
> > > I'm sure we can figure it out.
> > 
> > How do hotplug scripts factor into this?
> > 
> 
> If supporting all such block devices  requires presenting a block device
> to QEMU? If QEMU directly handles them then hotplug script is not in the
> picture.

Perhaps I've misunderstood what this thread is about. I thought it was
about exposing all the various backends which qdisk supports natively, like
CEPH, sheepdog, iscsi, nbd etc.

Ian.

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

Re: [Xen-devel] [PATCH V8 5/5] x86/hvm: pkeys, add pkeys support for cpuid handling

[Jan Beulich]

>>> On 03.02.16 at 11:04,  wrote:
> On Wed, 2016-02-03 at 02:50 -0700, Jan Beulich wrote:
>> > > > On 02.02.16 at 08:35,  wrote:
>> > This patch adds pkeys support for cpuid handing.
>> > 
>> > Pkeys hardware support is CPUID.7.0.ECX[3]:PKU. software support is
>> > CPUID.7.0.ECX[4]:OSPKE and it reflects the support setting of
>> > CR4.PKE.
>> > 
>> > X86_FEATURE_OSXSAVE depends on guest X86_FEATURE_XSAVE, but
>> > cpu_has_xsave
>> > function reflects hypervisor X86_FEATURE_XSAVE, it is fixed too.
>> > 
>> > Signed-off-by: Huaitong Han 
>> > ---
>> > Changes in v7:
>> > *Rebase in the latest tree.
>> > *Add a comment for cpu_has_xsave adjustment.
>> > *Adjust indentation.
>> > 
>> >  tools/libxc/xc_cpufeature.h |  3 +++
>> >  tools/libxc/xc_cpuid_x86.c  |  6 --
>> >  xen/arch/x86/hvm/hvm.c  | 18 +-
>> >  3 files changed, 20 insertions(+), 7 deletions(-)
>> 
>> This will need a tools maintainer's ack, so upon re-submission you
>> should Cc them.
> I will (again) defer this to x86 maintainers. -from wei.l...@citrix.com 

Which means he makes his (future) ack dependent on ours; it
does (to me at least) not mean tools maintainers don't need to
give their ack anymore.

>> > @@ -4593,16 +4593,24 @@ void hvm_cpuid(unsigned int input, unsigned
>> > int *eax, unsigned int *ebx,
>> >  if ( !cpu_has_smap )
>> >  *ebx &= ~cpufeat_mask(X86_FEATURE_SMAP);
>> >  
>> > -/* Don't expose MPX to hvm when VMX support is not
>> > available */
>> > +/* Don't expose MPX to hvm when VMX support is not
>> > available. */
>> >  if ( !(vmx_vmexit_control & VM_EXIT_CLEAR_BNDCFGS) ||
>> >   !(vmx_vmentry_control & VM_ENTRY_LOAD_BNDCFGS) )
>> >  *ebx &= ~cpufeat_mask(X86_FEATURE_MPX);
>> >  
>> > -/* Don't expose INVPCID to non-hap hvm. */
>> >  if ( !hap_enabled(d) )
>> > -*ebx &= ~cpufeat_mask(X86_FEATURE_INVPCID);
>> > +{
>> > + /* Don't expose INVPCID to non-hap hvm. */
>> > + *ebx &= ~cpufeat_mask(X86_FEATURE_INVPCID);
>> > + /* X86_FEATURE_PKU is not yet implemented for
>> > shadow paging. */
>> > + *ecx &= ~cpufeat_mask(X86_FEATURE_PKU);
>> > +}
>> > +
>> > +if ( (*ecx & cpufeat_mask(X86_FEATURE_PKU)) &&
>> > + (v->arch.hvm_vcpu.guest_cr[4] & X86_CR4_PKE) )
>> > +*ecx |= cpufeat_mask(X86_FEATURE_OSPKE);
>> 
>> ... here - shouldn't we also clear the respective flag in the "else"
>> case?
> X86_FEATURE_OSPKE is not set by xc_cpuid_hvm_policy(tools), it reflects
> the setting of CR4.PKE, so it does not need to be cleared like
> X86_CR4_OSXSAVE.

Tools side adjustments currently get done before applying config
file overrides, and hence a config file could also wrongly set that
flag - arguably one might call this an admin error, but why would
we not adjust that if we easily can (the more that we also set
the flag if we think it should be set)?

Jan


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

Re: [Xen-devel] [RFC] support more qdisk types

[Wei Liu]

On Wed, Feb 03, 2016 at 11:05:04AM +, Ian Campbell wrote:
> On Wed, 2016-02-03 at 10:55 +, Wei Liu wrote:
> > On Wed, Feb 03, 2016 at 10:51:27AM +, Ian Campbell wrote:
> > > On Wed, 2016-02-03 at 10:35 +, Wei Liu wrote:
> > > > > Ok. So in your opinion, even if any new disk config is encoded in
> > > > > 'target=',
> > > > > libxlu should split that up into (new) members of
> > > > > libxl_device_disk, not just
> > > > > plop it into libxl_device_disk.pdev_path?
> > > > > 
> > > > 
> > > > No, not necessarily. I didn't look closely in the code yesterday when
> > > > replying, sorry.
> > > > 
> > > > If  target= has always been shoveled into pdev_path, using that would
> > > > be
> > > > fine. We already have mechanism to parse target= outside of libxl in
> > > > hotplug script.
> > > > 
> > > > Are you aware of all those hotplug scripts living under tools/hotplug
> > > > ?
> > > > Does using hotplug script sound plausible to you?
> > > > 
> > > > Currently hotplug script for QEMU is broken and needs fixing though,
> > > > but
> > > > I'm sure we can figure it out.
> > > 
> > > How do hotplug scripts factor into this?
> > > 
> > 
> > If supporting all such block devices  requires presenting a block device
> > to QEMU? If QEMU directly handles them then hotplug script is not in the
> > picture.
> 
> Perhaps I've misunderstood what this thread is about. I thought it was
> about exposing all the various backends which qdisk supports natively, like
> CEPH, sheepdog, iscsi, nbd etc.
> 

Good point. It is me who is confused. Hotplug is not in the picture
then.

Wei.

> Ian.

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

Re: [Xen-devel] [RFC] support more qdisk types

[Roger Pau Monné]

El 3/2/16 a les 12:05, Ian Campbell ha escrit:
> On Wed, 2016-02-03 at 10:55 +, Wei Liu wrote:
>> On Wed, Feb 03, 2016 at 10:51:27AM +, Ian Campbell wrote:
>>> On Wed, 2016-02-03 at 10:35 +, Wei Liu wrote:
> Ok. So in your opinion, even if any new disk config is encoded in
> 'target=',
> libxlu should split that up into (new) members of
> libxl_device_disk, not just
> plop it into libxl_device_disk.pdev_path?
>

 No, not necessarily. I didn't look closely in the code yesterday when
 replying, sorry.

 If  target= has always been shoveled into pdev_path, using that would
 be
 fine. We already have mechanism to parse target= outside of libxl in
 hotplug script.

 Are you aware of all those hotplug scripts living under tools/hotplug
 ?
 Does using hotplug script sound plausible to you?

 Currently hotplug script for QEMU is broken and needs fixing though,
 but
 I'm sure we can figure it out.
>>>
>>> How do hotplug scripts factor into this?
>>>
>>
>> If supporting all such block devices  requires presenting a block device
>> to QEMU? If QEMU directly handles them then hotplug script is not in the
>> picture.
> 
> Perhaps I've misunderstood what this thread is about. I thought it was
> about exposing all the various backends which qdisk supports natively, like
> CEPH, sheepdog, iscsi, nbd etc.

AFAIK QEMU/Qdisk doesn't need a block device or regular file in order to
work, it can use as said above things like iSCSI (which we current
support using a script + blkback [0]), nbd, ceph...

There's been some discussion in the past about whether hotplug scripts
should also be supported by QEMU/Qdisk, or even whether the launch of
the Qdisk backend itself should be done inside of a hotplug script
(abstracting it away from libxl). I think George had some ideas/work on
this (or did look into similar issues in the past).

IMHO, I don't think we need hotplug scripts support for Qemu/Qdisk
handled backends.

Roger.

[0]
http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=bda856e1023868b1e9605e845bedbd8b7ed2944e


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

Re: [Xen-devel] Status of multiboot2 support on EFI?

[Wei Liu]

On Tue, Feb 02, 2016 at 01:02:56PM -0800, PGNet Dev wrote:
> Here http://wiki.xenproject.org/wiki/Xen_EFI#Xen_as_gz_binary refers to
> original discussion in 2013 (work has been deferred to Xen 4.6. )
> 
> and says "work has been deferred to Xen 4.6."
> 
> >I think that this should be Xen 4.8 target.
> 
> That's roughly another year+ from now for EFI support?
> 
> Per, http://wiki.xenproject.org/wiki/Xen_Roadmap, there's no 4.8 info at
> all.
> 
> But 8months plus 4.7 release =~ Feb 2017?
> 

We now have 6 months cycle.

The release after 4.7 will freeze in October and hopefully release in
December.

Wei.

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

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

Re: [Xen-devel] [PATCHv3 1/2] spinlock: move rwlock API and per-cpu rwlocks into their own files

[Jan Beulich]

>>> On 01.02.16 at 12:31,  wrote:
> From: Jennifer Herbert 
> 
> In preparation for a replacement read-write lock implementation, move
> the API and the per-cpu read-write locks into their own files.
> 
> Signed-off-by: Jennifer Herbert 
> Signed-off-by: David Vrabel 

I suppose this was meant to also state "no functional change" or
"no change to generated code" or some such?

> --- /dev/null
> +++ b/xen/include/xen/rwlock.h
> @@ -0,0 +1,150 @@
> +#ifndef __RWLOCK_H__
> +#define __RWLOCK_H__
> +
> +#include 

I suppose you need this because ...

> +#define read_lock(l)  _read_lock(l)
> +#define read_lock_irq(l)  _read_lock_irq(l)
> +#define read_lock_irqsave(l, f) \
> +({  \
> +BUILD_BUG_ON(sizeof(f) != sizeof(unsigned long));   \
> +((f) = _read_lock_irqsave(l));  \
> +})
> +
> +#define read_unlock(l)_read_unlock(l)
> +#define read_unlock_irq(l)_read_unlock_irq(l)
> +#define read_unlock_irqrestore(l, f)  _read_unlock_irqrestore(l, f)
> +#define read_trylock(l)   _read_trylock(l)
> +
> +#define write_lock(l) _write_lock(l)
> +#define write_lock_irq(l) _write_lock_irq(l)
> +#define write_lock_irqsave(l, f)\
> +({  \
> +BUILD_BUG_ON(sizeof(f) != sizeof(unsigned long));   \
> +((f) = _write_lock_irqsave(l)); \
> +})
> +#define write_trylock(l)  _write_trylock(l)
> +
> +#define write_unlock(l)   _write_unlock(l)
> +#define write_unlock_irq(l)   _write_unlock_irq(l)
> +#define write_unlock_irqrestore(l, f) _write_unlock_irqrestore(l, f)
> +
> +#define rw_is_locked(l)   _rw_is_locked(l)
> +#define rw_is_write_locked(l) _rw_is_write_locked(l)

... you move all the macro wrappers, but not the underlying
function declarations? Why is that? Apart from appearing
inconsistent, getting rid of the seeming stray include above
would be nice.

Or, looking at the next patch, was this instead done with the
goal of making that other patch more easily reviewable? In
which case this intention should have been stated at least
after the first --- separator above, and the second patch
should then remove the then unnecessary include again (and
likely put some other includes there which tight now get pulled
in via xen/spinlock.h).

Jan


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

Re: [Xen-devel] [PATCHv3 2/2] spinlock: fair read-write locks

[Jan Beulich]

>>> On 01.02.16 at 12:31,  wrote:
> +void queue_write_lock_slowpath(rwlock_t *lock)
> +{
> +u32 cnts;
> +
> +/* Put the writer into the wait queue. */
> +spin_lock(&lock->lock);
> +
> +/* Try to acquire the lock directly if no reader is present. */
> +if ( !atomic_read(&lock->cnts) &&
> + (atomic_cmpxchg(&lock->cnts, 0, _QW_LOCKED) == 0) )
> +goto unlock;
> +
> +/*
> + * Set the waiting flag to notify readers that a writer is pending,
> + * or wait for a previous writer to go away.
> + */
> +for (;;)

Since everything else here has been nicely converted to Xen style,
strictly speaking these should be

for ( ; ; )

but of course this is no reason to block the patch. Since however,
as said in reply to patch 1, ...

> --- a/xen/include/xen/rwlock.h
> +++ b/xen/include/xen/rwlock.h
> @@ -3,6 +3,188 @@
>  
>  #include 

... this should go away if possible, it would be nice for the cosmetic
thing above to also be fixed up at once.

With at least the latter taken care of, and assuming this won't incur
other changes (apart from adding necessary includes here)
Acked-by: Jan Beulich 

Jan


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

Re: [Xen-devel] Error booting Xen

[Harmandeep Kaur]

On Wed, Feb 3, 2016 at 1:53 PM, Dario Faggioli
 wrote:
> Hey Harmandeep,
>
> On Fri, 2016-01-29 at 03:13 -0700, Jan Beulich wrote:
>> > > > On 28.01.16 at 20:17,  wrote:
>> > Latest set looks good. No boot issues. No resets.
>> > Full log at http://paste2.org/NxHNW4vn
>>
>> > Sorry I don't know much about source of last few
>> > lines. I was just tracing in xen when these came.
>> > I was unable to create them again. I will inform
>> > you if I get these again.
>>
>> The WRMSR ones are of no concern. What I'm curious about are the
>> five instances of
>>
>> (XEN) traps.c:3290: GPF (): 82d08019cb87 -> 82d080242e15
>>
>> Could you check what these addresses correspond to in source?
>>
> I guess you just happened to forgot to follow up on this question from
> Jan?

Sorry for responding so late.

$ addr2line -e xen-syms 82d08019cb87
xen/xen/arch/x86/traps.c:2820

>> So far you've said you don't start any, but
>> the logs clearly show otherwise. Are there perhaps any that
>> get auto-started? In which case I'd be interested in you
>> confirming that these come up fine.
>>

Maybe I failed to shutdown a guest which was showing up
on next boot. But there are no auto-starting guests.
Following link goes to latest serial log
http://paste2.org/5PDz9bP1 and then I created a guest with
config (http://paste2.org/azvj25Hg) and
http://paste2.org/cgKna0j1 log was presented at terminal.

I hope this helps.

Thanks and regards,
Harmandeep Kaur

> And this too?
>
> Can you please do that? :-)
>
> Regards,
> Dario
> --
> <> (Raistlin Majere)
> -
> Dario Faggioli, Ph.D, http://about.me/dario.faggioli
> Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
>

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

[Xen-devel] [qemu-mainline baseline-only test] 38723: tolerable FAIL

[Platform Team regression test user]

This run is configured for baseline tests only.

flight 38723 qemu-mainline real [real]
http://osstest.xs.citrite.net/~osstest/testlogs/logs/38723/

Failures :-/ but no regressions.

Regressions which are regarded as allowable (not blocking):
 test-amd64-amd64-xl  19 guest-start/debian.repeatfail   like 38717
 test-amd64-amd64-qemuu-nested-intel 13 xen-boot/l1 fail like 38717

Tests which did not succeed, but are not blocking:
 test-armhf-armhf-libvirt 14 guest-saverestorefail   never pass
 test-armhf-armhf-libvirt 12 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt-xsm 12 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt-xsm 14 guest-saverestorefail   never pass
 test-amd64-amd64-xl-pvh-intel 11 guest-start  fail  never pass
 test-armhf-armhf-xl-credit2  13 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  12 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt-raw 13 guest-saverestorefail   never pass
 test-armhf-armhf-libvirt-raw 11 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  13 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-xsm  13 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-xsm  12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-multivcpu 13 saverestore-support-checkfail  never pass
 test-armhf-armhf-xl-multivcpu 12 migrate-support-checkfail  never pass
 test-armhf-armhf-xl-midway   13 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-midway   12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 13 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 12 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt  12 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt-xsm  12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  11 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  12 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt-qcow2 11 migrate-support-checkfail never pass
 test-armhf-armhf-libvirt-qcow2 13 guest-saverestorefail never pass
 test-amd64-amd64-libvirt-vhd 11 migrate-support-checkfail   never pass
 test-amd64-i386-xl-qemuu-win7-amd64 16 guest-stop  fail never pass
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check 
fail never pass
 test-amd64-amd64-libvirt-xsm 12 migrate-support-checkfail   never pass
 test-amd64-amd64-xl-pvh-amd  11 guest-start  fail   never pass
 test-amd64-amd64-libvirt 12 migrate-support-checkfail   never pass
 test-amd64-amd64-qemuu-nested-amd 16 debian-hvm-install/l1/l2  fail never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check 
fail never pass
 test-amd64-amd64-xl-qemuu-win7-amd64 16 guest-stop fail never pass

version targeted for testing:
 qemuu10ae9d76388e3f4a31f6a1475b5e2d1f28404a10
baseline version:
 qemuu0430891ce162b986c6e02a7729a942ecd2a32ca4

Last test of basis38717  2016-01-30 23:18:52 Z3 days
Testing same since38723  2016-02-03 05:19:09 Z0 days1 attempts


People who touched revisions under test:
  Alyssa Milburn 
  Anton Blanchard 
  Benjamin Herrenschmidt 
  Bharata B Rao 
  David Gibson 
  Greg Kurz 
  James Clarke 
  John Arbuckle 
  John Snow 
  Mark Cave-Ayland 
  Peter Maydell 
  Programmingkid 

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  pass
 build-i386-libvirt   pass
 build-amd64-pvopspass
 build-armhf-pvopspass
 build-i386-pvops pass
 test-amd64-amd64-xl  fail
 test-armhf-armhf-xl  pass
 test-amd64-i386-xl   pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm   pass
 test-amd64-i386-libvirt-qemu

[Xen-devel] [PATCH v2] arm: p2m.c bug-fix: hypervisor hang on __p2m_get_mem_access

[Corneliu ZUZU]

When __p2m_get_mem_access gets called, the p2m lock is already taken
by either get_page_from_gva or p2m_get_mem_access.
Possible code paths:
1)  -> get_page_from_gva
-> p2m_mem_access_check_and_get_page
-> __p2m_get_mem_access
2)  -> p2m_get_mem_access
-> __p2m_get_mem_access

In both cases if __p2m_get_mem_access subsequently gets to
call p2m_lookup (happens if !radix_tree_lookup(...)), a hypervisor
hang will occur, since p2m_lookup also spin-locks on the p2m lock.

This bug-fix simply replaces the p2m_lookup call from __p2m_get_mem_access
with a call to __p2m_lookup and also adds an ASSERT to ensure that the p2m lock
is already taken upon __p2m_get_mem_access entry.

Signed-off-by: Corneliu ZUZU 

---
Changed since v1:
  * added p2m-lock ASSERT
---
 xen/arch/arm/p2m.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 2190908..e8e6db4 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -468,6 +468,8 @@ static int __p2m_get_mem_access(struct domain *d, gfn_t gfn,
 #undef ACCESS
 };
 
+ASSERT(spin_is_locked(&p2m->lock));
+
 /* If no setting was ever set, just return rwx. */
 if ( !p2m->mem_access_enabled )
 {
@@ -490,7 +492,7 @@ static int __p2m_get_mem_access(struct domain *d, gfn_t gfn,
  * No setting was found in the Radix tree. Check if the
  * entry exists in the page-tables.
  */
-paddr_t maddr = p2m_lookup(d, gfn_x(gfn) << PAGE_SHIFT, NULL);
+paddr_t maddr = __p2m_lookup(d, gfn_x(gfn) << PAGE_SHIFT, NULL);
 if ( INVALID_PADDR == maddr )
 return -ESRCH;
 
-- 
2.5.0


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

Re: [Xen-devel] [PATCH 2/2] x86/xsave: use alternative asm on xsave side.

[Jan Beulich]

>>> On 02.02.16 at 08:11,  wrote:
> --- a/xen/arch/x86/xstate.c
> +++ b/xen/arch/x86/xstate.c
> @@ -248,24 +248,26 @@ void xsave(struct vcpu *v, uint64_t mask)
>  uint32_t hmask = mask >> 32;
>  uint32_t lmask = mask;
>  int word_size = mask & XSTATE_FP ? (cpu_has_fpu_sel ? 8 : 0) : -1;
> +#define XSAVE(pfx) \
> +alternative_io_3(".byte " pfx "0x0f,0xae,0x27\n", \
> + ".byte " pfx "0x0f,0xae,0x37\n", \
> + X86_FEATURE_XSAVEOPT, \
> + ".byte " pfx "0x0f,0xc7,0x27\n", \
> + X86_FEATURE_XSAVEC, \
> + ".byte " pfx "0x0f,0xc7,0x37\n", \
> + X86_FEATURE_XSAVES, \
> + "=m" (*ptr), \
> + "a" (lmask), "d" (hmask), "D" (ptr))
>  
>  if ( word_size <= 0 || !is_pv_32bit_vcpu(v) )
>  {
>  typeof(ptr->fpu_sse.fip.sel) fcs = ptr->fpu_sse.fip.sel;
>  typeof(ptr->fpu_sse.fdp.sel) fds = ptr->fpu_sse.fdp.sel;
>  
> -if ( cpu_has_xsaves )
> -asm volatile ( ".byte 0x48,0x0f,0xc7,0x2f"
> -   : "=m" (*ptr)
> -   : "a" (lmask), "d" (hmask), "D" (ptr) );
> -else if ( cpu_has_xsavec )
> -asm volatile ( ".byte 0x48,0x0f,0xc7,0x27"
> -   : "=m" (*ptr)
> -   : "a" (lmask), "d" (hmask), "D" (ptr) );
> -else if ( cpu_has_xsaveopt )
> +if ( cpu_has_xsaveopt || cpu_has_xsaves )
>  {
>  /*
> - * xsaveopt may not write the FPU portion even when the 
> respective
> + * xsaveopt/xsaves may not write the FPU portion even when the 
> respective

Apart from this line now being too long and hence the entire
comment needing re-formatting
Reviewed-by: Jan Beulich 

Jan


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

Re: [Xen-devel] [PATCH 1/4] libxl: Use libxl_strdup instead of strdup on libxl_version_info

[Ian Campbell]

On Mon, 2016-02-01 at 12:13 +, Wei Liu wrote:
> On Tue, Jan 26, 2016 at 04:30:57PM -0500, Konrad Rzeszutek Wilk wrote:
> > The change is simple replace of raw strdup with a libxl variant.
> > The benefit of that is the libxl variant has the extra
> > behaviour of abort-on-alloc-fail - and will improve error handling.
> > 
> > libxl_version_info is a bit odd - it is a public function and as
> > libxl.h
> > mentions - the callers of libxl_ public function needs to call the
> > appropiate
> > _dispose() function.
> > 
> > "However libxl_get_version_info() is special and returns a cached
> > result from the ctx which cannot and should not be freed (as evidenced
> > by it returning a const struct). This data is freed in libxl_ctx_free()
> > by calling libxl_version_info_dispose(). This is why none of the
> > callers
> > remember to free -- they shouldn't be doing so." (Ian Campbell)
> > 
> > So the patch makes sure to use the NOGC.
> > 
> > Suggested-by: Wei Liu 
> > Signed-off-by: Konrad Rzeszutek Wilk 
> 
> Acked-by: Wei Liu 

Applied.


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

Re: [Xen-devel] xen/arm: drop hip04 support

[Ian Campbell]

On Tue, 2016-02-02 at 13:13 +, Zoltan Kiss wrote:
> This platform is no longer actively used, but it makes GICv2 development
> harder.
> 
> Signed-off-by: Zoltan Kiss 

Thanks, I appreciate the proactive removal of no-longer supported
platforms.

I was going to ask you to confirm that you were still part of Huawei while
on secondment to Linaro, but I see you have CCd that other hat of yours as
well, which is good enough for me. Hence:

Acked-by: Ian Campbell 

and applied.

I don't see anything obvious in the wiki / docs which would need
updating/deleting, if you know of anything please could you either update
or let us know.

Thanks,
Ian.

> ---
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7c1bf82..12f147c 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -177,11 +177,6 @@ S:   Supported
>  F:   xen/arch/x86/debug.c
>  F:   tools/debugger/gdbsx/
>  
> -HISILICON HIP04 SUPPORT
> -M:   Zoltan Kiss 
> -S:   Supported
> -F:   xen/arch/arm/gic-hip04.c
> -
>  INTEL(R) TRUSTED EXECUTION TECHNOLOGY (TXT)
>  M:   Gang Wei 
>  M:   Shane Wang 
> diff --git a/docs/misc/arm/early-printk.txt b/docs/misc/arm/early-
> printk.txt
> index 7e03955..41b528b 100644
> --- a/docs/misc/arm/early-printk.txt
> +++ b/docs/misc/arm/early-printk.txt
> @@ -37,7 +37,6 @@ the name of the machine:
>    - dra7: printk with 8250 on DRA7 platform
>    - exynos5250: printk with the second UART
>    - fastmodel: printk on ARM Fastmodel software emulators
> -  - hip04-d01: printk with 8250 on HiSilicon Hip-04 D01
>    - juno: printk with pl011 on Juno platform
>    - lager: printk with SCIF0 on Renesas R-Car H2 processors
>    - midway: printk with the pl011 on Calxeda Midway processors
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 2f050f5..6a58a41 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -12,7 +12,6 @@ obj-y += domctl.o
>  obj-y += sysctl.o
>  obj-y += domain_build.o
>  obj-y += gic.o gic-v2.o
> -obj-$(CONFIG_ARM_32) += gic-hip04.o
>  obj-$(CONFIG_HAS_GICV3) += gic-v3.o
>  obj-y += io.o
>  obj-y += irq.o
> diff --git a/xen/arch/arm/Rules.mk b/xen/arch/arm/Rules.mk
> index 4947e64..d6bbe7c 100644
> --- a/xen/arch/arm/Rules.mk
> +++ b/xen/arch/arm/Rules.mk
> @@ -42,7 +42,6 @@ EARLY_PRINTK_brcm   := 8250,0xF040AB00,2
>  EARLY_PRINTK_dra7   := 8250,0x4806A000,2
>  EARLY_PRINTK_fastmodel  := pl011,0x1c09,115200
>  EARLY_PRINTK_exynos5250 := exynos4210,0x12c2
> -EARLY_PRINTK_hip04-d01  := 8250,0xE4007000,2
>  EARLY_PRINTK_juno   := pl011,0x7ff8
>  EARLY_PRINTK_lager  := scif,0xe6e6
>  EARLY_PRINTK_midway := pl011,0xfff36000
> diff --git a/xen/arch/arm/gic-hip04.c b/xen/arch/arm/gic-hip04.c
> deleted file mode 100644
> index a42cf24..000
> --- a/xen/arch/arm/gic-hip04.c
> +++ /dev/null
> @@ -1,757 +0,0 @@
> -/*
> - * xen/arch/arm/gic-hip04.c
> - *
> - * Generic Interrupt Controller for HiSilicon Hip04 platform
> - * Based heavily on gic-v2.c (id
> 3bcf563fec26378f7f4cf1e2ad0d4d5b3f341919)
> - *
> - * Tim Deegan 
> - * Copyright (c) 2011 Citrix Systems.
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License as published by
> - * the Free Software Foundation; either version 2 of the License, or
> - * (at your option) any later version.
> - *
> - * This program is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> - * GNU General Public License for more details.
> - */
> -
> -#include 
> -#include 
> -#include 
> -#include 
> -#include 
> -#include 
> -#include 
> -#include 
> -#include 
> -#include 
> -#include 
> -#include 
> -#include 
> -#include 
> -#include 
> -#include 
> -
> -#include 
> -#include 
> -
> -/*
> - * LR register definitions are GIC v2 specific.
> - * Moved these definitions from header file to here
> - */
> -#define GICH_V2_LR_VIRTUAL_MASK0x3ff
> -#define GICH_V2_LR_VIRTUAL_SHIFT   0
> -#define GICH_V2_LR_PHYSICAL_MASK   0x3ff
> -#define GICH_V2_LR_PHYSICAL_SHIFT  10
> -#define GICH_V2_LR_STATE_MASK  0x3
> -#define GICH_V2_LR_STATE_SHIFT 28
> -#define GICH_V2_LR_PRIORITY_SHIFT  23
> -#define GICH_V2_LR_PRIORITY_MASK   0x1f
> -#define GICH_V2_LR_HW_SHIFT31
> -#define GICH_V2_LR_HW_MASK 0x1
> -#define GICH_V2_LR_GRP_SHIFT   30
> -#define GICH_V2_LR_GRP_MASK0x1
> -#define GICH_V2_LR_MAINTENANCE_IRQ (1<<19)
> -#define GICH_V2_LR_GRP1(1<<30)
> -#define GICH_V2_LR_HW  (1<<31)
> -#define GICH_V2_LR_CPUID_SHIFT 9
> -#define GICH_V2_VTR_NRLRGS 0x3f
> -
> -#define GICH_V2_VMCR_PRIORITY_MASK   0x1f
> -#define GICH_V2_VMCR_PRIORITY_SHIFT  27
> -
> -/* Global state */
> -static struct {
> -void __iomem * map_dbase; /* IO mapped Address of distributor
> registers */
> -void __iomem * map_cbase[2]; /* IO map

Re: [Xen-devel] [PATCH 2/4] libxc/xc_domain_resume: Update comment.

[Ian Campbell]

On Mon, 2016-02-01 at 12:14 +, Wei Liu wrote:
> On Tue, Jan 26, 2016 at 04:30:58PM -0500, Konrad Rzeszutek Wilk wrote:
> > To hopefully clarify what it meant. Also point out that mechanism
> > by which the return 1 value is done is via an intimate knowledge of the
> > hypercall ABI (i.e. which register - eax - is the return value).
> > 
> > Signed-off-by: Konrad Rzeszutek Wilk 
> 
> As far as I can tell all concerns in previous versions have been
> addressed.
> 
> Acked-by: Wei Liu 

Applied.


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

Re: [Xen-devel] [PATCH 4/4] tools/libxl: run_helper - add #define for arguments.

[Ian Campbell]

On Tue, 2016-01-26 at 16:31 -0500, Konrad Rzeszutek Wilk wrote:
> Describe what the four (or more in the future) arguments
> are for.
> 
> Acked-by: Ian Jackson 
> Signed-off-by: Konrad Rzeszutek Wilk 


applied.

> ---
>  tools/libxl/libxl_save_callout.c | 11 ++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/libxl/libxl_save_callout.c
> b/tools/libxl/libxl_save_callout.c
> index 3af99af..45b9727 100644
> --- a/tools/libxl/libxl_save_callout.c
> +++ b/tools/libxl/libxl_save_callout.c
> @@ -119,13 +119,22 @@ void
> libxl__save_helper_init(libxl__save_helper_state *shs)
>  
>  /*- helper execution -*/
>  
> +/*
> + * Both save and restore share four parameters:
> + * 1) Path to libxl-save-helper.
> + * 2) --[restore|save]-domain.
> + * 3) stream file descriptor.
> + * n) save/restore specific parameters.
> + * 4) A \0 at the end.
> + */
> +#define HELPER_NR_ARGS 4
>  static void run_helper(libxl__egc *egc, libxl__save_helper_state *shs,
> const char *mode_arg, int stream_fd,
> const int *preserve_fds, int num_preserve_fds,
> const unsigned long *argnums, int num_argnums)
>  {
>  STATE_AO_GC(shs->ao);
> -const char *args[4 + num_argnums];
> +const char *args[HELPER_NR_ARGS + num_argnums];
>  const char **arg = args;
>  int i, rc;
>  

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

Re: [Xen-devel] [PATCH] tools/libxc: Provide evtchn_port_or_error_t for compat xenctrl interface

[Ian Campbell]

On Mon, 2016-02-01 at 11:19 +, Ian Campbell wrote:
> On Mon, 2016-02-01 at 11:08 +, Andrew Cooper wrote:
> > c/s 2d2f789 "tools: rename libxc's evtchn_port_or_error_t with an xc_
> > prefix" doesn't cater for older applications which have requested
> > XC_WANT_COMPAT_EVTCHN_API
> 
> I think we can safely assume that such apps are not also using
> xenevtchn.h
> (otherwise there would be clashes already) so:
> 
> > Signed-off-by: Andrew Cooper 
> 
> Acked-by: Ian Campbell 

applied.


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

Re: [Xen-devel] [PATCH] xl: don't free additional memory on soft reset

[Ian Campbell]

On Mon, 2016-02-01 at 10:39 +, Wei Liu wrote:
> On Thu, Jan 28, 2016 at 11:58:25AM +0100, Vitaly Kuznetsov wrote:
> > We don't need to free anything extra from Dom0 in order to perform soft
> > reset. It can also fail soft reset if it happens that we don't have
> > this
> > memory (which we don't need) available.
> > 
> > Signed-off-by: Vitaly Kuznetsov 
> 
> Acked-by: Wei Liu 

Applied.


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

Re: [Xen-devel] [PATCH V2] arm: p2m.c bug-fix: hypervisor hang on __p2m_get_mem_access

[Ian Campbell]

On Wed, 2016-01-27 at 14:24 +0200, Corneliu ZUZU wrote:
> When __p2m_get_mem_access gets called, the p2m lock is already taken
> by either get_page_from_gva or p2m_get_mem_access.
> 
> Possible code paths:
> 1)-> get_page_from_gva
>   -> p2m_mem_access_check_and_get_page
>   -> __p2m_get_mem_access
> 2)-> p2m_get_mem_access
>   -> __p2m_get_mem_access
> 
> In both cases if __p2m_get_mem_access subsequently gets to
> call p2m_lookup (happens if !radix_tree_lookup(...)), a hypervisor
> hang will occur, since p2m_lookup also spin-locks on the p2m lock.
> 
> This bug-fix simply replaces the p2m_lookup call from
> __p2m_get_mem_access
> with a call to __p2m_lookup.
> 
> Following Ian's suggestion, we also add an ASSERT to ensure that
> the p2m lock is taken upon __p2m_get_mem_access entry.
> 
> Signed-off-by: Corneliu ZUZU 

Acked + applied, thanks.


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

Re: [Xen-devel] [PATCH 3/3] tools/libxl: run_helper - add #define for arguments.

[Ian Campbell]

On Tue, 2016-01-26 at 16:25 +, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH 3/3] tools/libxl: run_helper - add
> #define for arguments."):
> > On Mon, 2016-01-25 at 16:06 -0500, Konrad Rzeszutek Wilk wrote:
> > > Describe what the four (or more in the future) arguments
> > > are for.
> > 
> > I'd say that a code comment on the definition would be sufficient here,
> > but
> > I'll defer to Ian J as author of this code.
> 
> Acked-by: Ian Jackson 

Applied.

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

Re: [Xen-devel] [PATCH] libxl: Add CPU hotplug support for HVM domains without device model

[Ian Campbell]

On Wed, 2016-02-03 at 10:10 +, Wei Liu wrote:
> On Tue, Feb 02, 2016 at 04:02:12PM -0500, Boris Ostrovsky wrote:
> > HVMlite domains add/remove VCPUs by toggling "availability" property in
> > xenstore.
> > 
> > Signed-off-by: Boris Ostrovsky 
> 
> If this is how it is supposed to work:
> 
> Acked-by: Wei Liu 

Applied.

> However, I would like to ask for a follow-up patch to documentation
> xen.git hvmlite.markdown (or whichever file you see fit).

Yes, please.



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

Re: [Xen-devel] [PATCH] libxc: fix leak in xc_dom_load_hvm_kernel error path

[Ian Campbell]

On Wed, 2016-02-03 at 11:59 +0100, Roger Pau Monne wrote:
> Error path in xc_dom_load_hvm_kernel needs to use the 'error' label
> instead
> of directly returning. This is needed so the entries local variable is
> freed.
> 
> Coverity-ID: 1351227
> Signed-off-by: Roger Pau Monné 

Acked + applied, thanks.


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

Re: [Xen-devel] [PATCH] libxenforeignmemory: handle partial failure correctly

[Ian Campbell]

On Wed, 2016-02-03 at 10:17 +, Wei Liu wrote:
> On Wed, Feb 03, 2016 at 10:10:01AM +, Ian Campbell wrote:
> > Coverity rightly points out that checking for ret == NULL and then
> > calling osdep unmap(ret) is wrong.
> > 
> > The intention on this code path is to turn partial failure into total
> > failure when the err argument is NULL, so we want to take this patch
> > whenever ret is _non_ NULL (and err_to_free is set, indicating err was
> > NULL).
> > 
> > CID: 1351219
> > 
> > Signed-off-by: Ian Campbell 
> 
> Acked-by: Wei Liu 

Applied, thanks.

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

Re: [Xen-devel] [PATCH] tools: xenconsole: cleanup when clock_gettime fails.

[Ian Campbell]

On Wed, 2016-02-03 at 10:49 +, Wei Liu wrote:
> On Wed, Feb 03, 2016 at 10:43:47AM +, Ian Campbell wrote:
> > All other error paths in the infinite loop in handle_io use break, so
> > as to free resources.
> > 
> > CID: 1351226
> > 
> > Signed-off-by: Ian Campbell 
> 
> Acked-by: Wei Liu 

applied, thanks.

> 
> > ---
> >  tools/console/daemon/io.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
> > index e2e7a6b..34666c4 100644
> > --- a/tools/console/daemon/io.c
> > +++ b/tools/console/daemon/io.c
> > @@ -1053,7 +1053,7 @@ void handle_io(void)
> >      POLLIN|POLLPRI);
> >  
> >     if (clock_gettime(CLOCK_MONOTONIC, &ts) < 0)
> > -   return;
> > +   break;
> >     now = ((long long)ts.tv_sec * 1000) + (ts.tv_nsec /
> > 100);
> >  
> >     /* Re-calculate any event counter allowances & unblock

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

Re: [Xen-devel] [PATCH] tools: libxencall/foreignmemory: initialise handle->fd

[Ian Campbell]

On Wed, 2016-02-03 at 10:15 +, Wei Liu wrote:
> On Wed, Feb 03, 2016 at 10:09:42AM +, Ian Campbell wrote:
> > Otherwise the osdep close on the error path touches an uninitialised
> > varialble.
> > 
> > CID: 1351231 (foreignmemory) and 1351230 (call)
> > 
> > Signed-off-by: Ian Campbell 
> 
> Acked-by: Wei Liu 

Applied, thanks.

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

Re: [Xen-devel] [PATCH] xenstore: add stddef.h to xenstore_lib.h

[Ian Campbell]

On Thu, 2016-01-28 at 10:37 +, Wei Liu wrote:
> On Wed, Jan 27, 2016 at 05:06:09PM +, Ian Campbell wrote:
> > xs_perm_to_string takes a size_t which isn't defined by anything
> > pulled in directly by this header.
> > 
> > Given the other headers xenstore_lib.h pulls in this looks to be an
> > oversight rather than a deliberate policy.
> > 
> > Signed-off-by: Ian Campbell 
> 
> Acked-by: Wei Liu 

Applied,thanks.



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

Re: [Xen-devel] [PATCH] tools/libxl: improve logging on domain create failure.

[Ian Campbell]

On Thu, 2016-01-28 at 10:52 +, Wei Liu wrote:
> On Tue, Jan 26, 2016 at 02:38:46PM +, Ian Campbell wrote:
> > A user reported[0] that xl create failed with just:
> > libxl: error: libxl_create.c:892:initiate_domain_create: Unable to
> > set domain build info defaults
> > and some resulting fallout, but without indicating why it was unable
> > to set the defaults, even in verbose mode[1].
> > 
> > Go through libxl__domain_{create,build}_info_setdefault and ensure
> > that each error path logs something.
> > 
> > In most cases this involved simply adding a call to LOG.
> > 
> > In two cases this involved switching from strdup to
> > libxl__strdup(NOGC) and removing the existing error handling.
> > 
> > When switching from qemu-xen to qemu-xen-traditional (because the
> > former is not available) log at level INFO rather than VERBOSE, so
> > the message would normally be printed. Also tweak the language here.
> > 
> > I'm not sure all these messages are reachable (some might be shadowed
> > by previous error paths) but it seems better to err on the side of
> > caution.
> > 
> > [0] http://lists.xen.org/archives/html/xen-users/2016-01/msg00125.html
> > [1] http://lists.xen.org/archives/html/xen-users/2016-01/msg00129.html
> > 
> > Signed-off-by: Ian Campbell 
> > Cc: suse@fea.st
> 
> Acked-by: Wei Liu 

Applied.

Ian, can you queue this for backport please?

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

Re: [Xen-devel] [PATCH v2] arm: p2m.c bug-fix: hypervisor hang on __p2m_get_mem_access

[Ian Campbell]

On Wed, 2016-02-03 at 13:37 +0200, Corneliu ZUZU wrote:

I just now applied a previous v2 which was already in my queue. Was this
just an accidental resend of v2 or is there some important change and this
is really a v3?

> When __p2m_get_mem_access gets called, the p2m lock is already taken
> by either get_page_from_gva or p2m_get_mem_access.
> Possible code paths:
> 1)-> get_page_from_gva
>   -> p2m_mem_access_check_and_get_page
>   -> __p2m_get_mem_access
> 2)-> p2m_get_mem_access
>   -> __p2m_get_mem_access
> 
> In both cases if __p2m_get_mem_access subsequently gets to
> call p2m_lookup (happens if !radix_tree_lookup(...)), a hypervisor
> hang will occur, since p2m_lookup also spin-locks on the p2m lock.
> 
> This bug-fix simply replaces the p2m_lookup call from
> __p2m_get_mem_access
> with a call to __p2m_lookup and also adds an ASSERT to ensure that the
> p2m lock
> is already taken upon __p2m_get_mem_access entry.
> 
> Signed-off-by: Corneliu ZUZU 
> 
> ---
> Changed since v1:
>   * added p2m-lock ASSERT
> ---
>  xen/arch/arm/p2m.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 2190908..e8e6db4 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -468,6 +468,8 @@ static int __p2m_get_mem_access(struct domain *d,
> gfn_t gfn,
>  #undef ACCESS
>  };
>  
> +ASSERT(spin_is_locked(&p2m->lock));
> +
>  /* If no setting was ever set, just return rwx. */
>  if ( !p2m->mem_access_enabled )
>  {
> @@ -490,7 +492,7 @@ static int __p2m_get_mem_access(struct domain *d,
> gfn_t gfn,
>   * No setting was found in the Radix tree. Check if the
>   * entry exists in the page-tables.
>   */
> -paddr_t maddr = p2m_lookup(d, gfn_x(gfn) << PAGE_SHIFT, NULL);
> +paddr_t maddr = __p2m_lookup(d, gfn_x(gfn) << PAGE_SHIFT, NULL);
>  if ( INVALID_PADDR == maddr )
>  return -ESRCH;
>  

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

Re: [Xen-devel] [PATCHv3 2/2] spinlock: fair read-write locks

[David Vrabel]

On 03/02/16 11:28, Jan Beulich wrote:
 On 01.02.16 at 12:31,  wrote:
>> +void queue_write_lock_slowpath(rwlock_t *lock)
>> +{
>> +u32 cnts;
>> +
>> +/* Put the writer into the wait queue. */
>> +spin_lock(&lock->lock);
>> +
>> +/* Try to acquire the lock directly if no reader is present. */
>> +if ( !atomic_read(&lock->cnts) &&
>> + (atomic_cmpxchg(&lock->cnts, 0, _QW_LOCKED) == 0) )
>> +goto unlock;
>> +
>> +/*
>> + * Set the waiting flag to notify readers that a writer is pending,
>> + * or wait for a previous writer to go away.
>> + */
>> +for (;;)
> 
> Since everything else here has been nicely converted to Xen style,
> strictly speaking these should be
> 
> for ( ; ; )
> 
> but of course this is no reason to block the patch. Since however,
> as said in reply to patch 1, ...

TBH, I really think you're pointlessly nit-picking here.  This change
would make zero impact on readability.

>> --- a/xen/include/xen/rwlock.h
>> +++ b/xen/include/xen/rwlock.h
>> @@ -3,6 +3,188 @@
>>  
>>  #include 
> 
> ... this should go away if possible, it would be nice for the cosmetic
> thing above to also be fixed up at once.

The rwlock structure now includes a spinlock, so this #include is
required here.

David

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

Re: [Xen-devel] [PATCH OSSTEST 5/5 v2] mg-show-flight-runvars: recurse on buildjobs upon request

[Ian Jackson]

Ian Campbell writes ("[PATCH OSSTEST 5/5 v2] mg-show-flight-runvars: recurse on 
buildjobs upon request"):
> By looping over @rows looking for buildjobs runvars and adding those
> jobs to the output until nothing changes.
> 
> The output is resorted by runvar name which is the desired default
> behaviour. As usual can be piped to sort(1) to sort by flight+job.
...
> +if ($recurse) {
> +foreach my $row (@rows) {
> + next unless $row->[1] =~ m/^(?:.*_)?([^_]*)buildjob$/;
> + next if $jobs{$row->[2]};

This does the deduping based on $row->[2] which is the runvar value.
But the same job might be referred to by different runvar values.  I
think this means that you might report the same job twice.

If
   1234.foo  buildjob   wombat
   1234.bar  buildjob   1234.wombat
then you dump 1234.wombat twice.

> + my ($oflight, $ojob) = flight_otherjob($tflight, $row->[2]);

Also the place you set $jobs{} is somewhat wrong.  If there is a job
with no rows you can process it twice (harmlessly, I think, as it
happens).


So I think you want
next if $jobs{$oflight,$ojob}++;

> + collect($oflight, "job = ?", $ojob);
> +
> + # collect() appends to @rows, so we don't need any special
> + # handling to pickup anything which was newly added.

I don't think this is true.  Or at least, it happens to be true, but
my copy of the FM says:

   If any part of LIST is an array, "foreach" will get very
   confused if you add or remove elements within the loop body,
   for example with "splice".  So don't do that.

AIUI you're avoiding actual recursion, inside collect()'s row loop, to
avoid trying to have one SQL query for each nesting level.  It might
be worth mentioning that somewhere.

Maybe you should have the foreach use an explicit row index.

Ian.

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

Re: [Xen-devel] [PATCH 1/3] Move collectversions from ts-xen-build into Osstest::BuildSupport

[Ian Jackson]

Ian Campbell writes ("[PATCH 1/3] Move collectversions from ts-xen-build into 
Osstest::BuildSupport"):
> I'm going to have a need for it elsewhere.

Acked-by: Ian Jackson 

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

Re: [Xen-devel] [RFC Design Doc] Add vNVDIMM support for Xen

[Stefano Stabellini]

On Wed, 3 Feb 2016, Haozhong Zhang wrote:
> On 02/02/16 17:11, Stefano Stabellini wrote:
> > Haozhong, thanks for your work!
> > 
> > On Mon, 1 Feb 2016, Haozhong Zhang wrote:
> > > 3.2 Address Mapping
> > > 
> > > 3.2.1 My Design
> > > 
> > >  The overview of this design is shown in the following figure.
> > > 
> > >  Dom0 |   DomU
> > >   |
> > >   |
> > >  QEMU |
> > >  +...++...+-+ |
> > >   VA |   | Label Storage Area |   | buf | |
> > >  +...++...+-+ |
> > >  ^^ ^ |
> > >  || | |
> > >  V| | |
> > >  +---+   +---+mmap(2) |
> > >  | vACPI |   | v_DSM || | |+++
> > >  +---+   +---+| | |   SPA  || /dev/pmem0 |
> > >  ^   ^ +--+ | |+++
> > >  |---|-||--   | ^^
> > >  |   | || | ||
> > >  |+--+ +~-~-+|
> > >  |||| |
> > > XEN_DOMCTL_memory_mapping
> > >  |||+-~--+
> > >  |||| |
> > >  ||   +++ |
> > >  Linux   ||   SPA || /dev/pmem0 | | +--+   +--+
> > >  ||   +++ | | ACPI |   | _DSM |
> > >  ||   ^   | +--+   +--+
> > >  ||   |   | |  |
> > >  ||   Dom0 Driver |   hvmloader/xl |
> > >  
> > > ||---|-|--|---
> > >  |+---~-~--+
> > >  Xen || |
> > >  +~-+
> > >  
> > > -|
> > >   ++
> > >|
> > > +-+
> > >  HW |NVDIMM   |
> > > +-+
> > > 
> > > 
> > >  This design treats host NVDIMM devices as ordinary MMIO devices:
> > >  (1) Dom0 Linux NVDIMM driver is responsible to detect (through NFIT)
> > >  and drive host NVDIMM devices (implementing block device
> > >  interface). Namespaces and file systems on host NVDIMM devices
> > >  are handled by Dom0 Linux as well.
> > > 
> > >  (2) QEMU mmap(2) the pmem NVDIMM devices (/dev/pmem0) into its
> > >  virtual address space (buf).
> > > 
> > >  (3) QEMU gets the host physical address of buf, i.e. the host system
> > >  physical address that is occupied by /dev/pmem0, and calls Xen
> > >  hypercall XEN_DOMCTL_memory_mapping to map it to a DomU.
> > 
> > How is this going to work from a security perspective? Is it going to
> > require running QEMU as root in Dom0, which will prevent NVDIMM from
> > working by default on Xen? If so, what's the plan?
> >
> 
> Oh, I forgot to address the non-root qemu issues in this design ...
> 
> The default user:group of /dev/pmem0 is root:disk, and its permission
> is rw-rw. We could lift the others permission to rw, so that
> non-root QEMU can mmap /dev/pmem0. But it looks too risky.

Yep, too risky.


> Or, we can make a file system on /dev/pmem0, create files on it, set
> the owner of those files to xen-qemuuser-domid$domid, and then pass
> those files to QEMU. In this way, non-root QEMU should be able to
> mmap those files.

Maybe that would work. Worth adding it to the design, I would like to
read more details on it.

Also note that QEMU initially runs as root but drops privileges to
xen-qemuuser-domid$domid before the guest is started. Initially QEMU
*could* mmap /dev/pmem0 while is still running as root, but then it
wouldn't work for any devices that need to be mmap'ed at run time
(hotplug scenario).


> > >  (ACPI part is described in Section 3.3 later)
> > > 
> > >  Above (1)(2) have already been done in current QEMU. Only (3) is
> > >  needed to implement in QEMU. No change is needed in Xen for address
> > >  mapping in this design.
> > > 
> > >  Open: It seems no system call/ioctl is provided by Linux kernel to
> > >get the physical address from a virtual address.
> > >

Re: [Xen-devel] [PATCH 2/2] x86/xsave: use alternative asm on xsave side.

[Jan Beulich]

>>> On 03.02.16 at 12:40,  wrote:
 On 02.02.16 at 08:11,  wrote:
>> --- a/xen/arch/x86/xstate.c
>> +++ b/xen/arch/x86/xstate.c
>> @@ -248,24 +248,26 @@ void xsave(struct vcpu *v, uint64_t mask)
>>  uint32_t hmask = mask >> 32;
>>  uint32_t lmask = mask;
>>  int word_size = mask & XSTATE_FP ? (cpu_has_fpu_sel ? 8 : 0) : -1;
>> +#define XSAVE(pfx) \
>> +alternative_io_3(".byte " pfx "0x0f,0xae,0x27\n", \
>> + ".byte " pfx "0x0f,0xae,0x37\n", \
>> + X86_FEATURE_XSAVEOPT, \
>> + ".byte " pfx "0x0f,0xc7,0x27\n", \
>> + X86_FEATURE_XSAVEC, \
>> + ".byte " pfx "0x0f,0xc7,0x37\n", \
>> + X86_FEATURE_XSAVES, \
>> + "=m" (*ptr), \
>> + "a" (lmask), "d" (hmask), "D" (ptr))
>>  
>>  if ( word_size <= 0 || !is_pv_32bit_vcpu(v) )
>>  {
>>  typeof(ptr->fpu_sse.fip.sel) fcs = ptr->fpu_sse.fip.sel;
>>  typeof(ptr->fpu_sse.fdp.sel) fds = ptr->fpu_sse.fdp.sel;
>>  
>> -if ( cpu_has_xsaves )
>> -asm volatile ( ".byte 0x48,0x0f,0xc7,0x2f"
>> -   : "=m" (*ptr)
>> -   : "a" (lmask), "d" (hmask), "D" (ptr) );
>> -else if ( cpu_has_xsavec )
>> -asm volatile ( ".byte 0x48,0x0f,0xc7,0x27"
>> -   : "=m" (*ptr)
>> -   : "a" (lmask), "d" (hmask), "D" (ptr) );
>> -else if ( cpu_has_xsaveopt )
>> +if ( cpu_has_xsaveopt || cpu_has_xsaves )
>>  {
>>  /*
>> - * xsaveopt may not write the FPU portion even when the 
> respective
>> + * xsaveopt/xsaves may not write the FPU portion even when the 
> respective
> 
> Apart from this line now being too long and hence the entire
> comment needing re-formatting
> Reviewed-by: Jan Beulich 

Withdrawn. There's a bug here, and since I'm re-doing patch 1
from scratch I'll send out the result later.

Jan


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

Re: [Xen-devel] [PATCH 2/3] Add a weekly coverity flight

[Ian Jackson]

Ian Campbell writes ("[PATCH 2/3] Add a weekly coverity flight"):
> In my experiments the curl command took ~35 minutes to complete (rate
> in the 100-200k range). Not sure if this is a problem. Note that curl
> is run on the controller (via system_checked) and consequently has no
> timeout etc.

Can you use curl's builtin timeouts, or wrap it in alarm() ?
Otherwise I think it is possible for this to become wedged
indefinitely and need manual un-wedging.

AFAICT during the curl the only lock or resource which is held is the
build host share.  I think we can live with that.

> Note that the token must be supplied with  @/path/to/token. The latter appears to the server as a file upload
> rather than a text field in a form which doesn't work. In early
> attempts I thought that the trailing \n in /path/to/token might be an
> issue and hence wrote a big comment. However having discovered < vs @
> I am no longer 100% sure that is the case, but I left the comment
> anyway since I can observe on the wire that the \n is included in the
> upload (but each test takes ~35 mins and there is a ratelimit on the
> server side too).

Right.

> Deployment notes:
>  - Put cov-analysis-linux64-7.7.0.4.tar.gz in the Images
>directory. DONE in COLO
>  - Populate $HOME/.xen-osstest/coverity-secret with the token
>DONE in COLO
>  - Populate xen.git#coverity-scanned with an initial baseline, update
>ap-fetch-version-old to refer to it instead of master.
...
> +coverity)
> + #XXX doesn't exist yet, use master for now repo_tree_rev_fetch_git xen 
> $TREE_XEN coverity-scanned $LOCALREV_XEN

This XXX is out of date ?  And so the code should be fixed ?


> + case $branch in
> + coverity)
> + if [ "x$TREE_COVERITY" = x ]; then
> + export TREE_COVERITY=$TREE_XEN
> + fi
> + if [ "x$REVISION_COVERITY" = x ]; then
> + determine_version REVISION_COVERITY coverity COVERITY
> + export REVISION_COVERITY
> + fi
> + NEW_REVISION=$REVISION_COVERITY

I'm not sure why these variables are all REVISION_COVERITY rather than
REVISION_XEN.

But in general this looks good.

Ian.

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

Re: [Xen-devel] [PATCH 3/3] make-coverity-flight: set coverity_upload=true

[Ian Jackson]

Ian Campbell writes ("[PATCH 3/3] make-coverity-flight: set 
coverity_upload=true"):
> Signed-off-by: Ian Campbell 
> ---
> Apply once the previous patch + manual upload lead to us being happy.

I'll ack this at that time.  Obviously there's nothing to review :-).

Ian.

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

Re: [Xen-devel] [PATCH 2/3] Add a weekly coverity flight

[Ian Jackson]

Andrew Cooper writes ("Re: [PATCH 2/3] Add a weekly coverity flight"):
> On 03/02/16 10:19, Ian Campbell wrote:
> > Andy, I presume you deliberately started excluding stubdoms at some point?
> > I think this is probably the right thing to do, at least for now, since
> > stubdoms run with guest privileges so aren't hugely interesting, plus they
> > include an awful lot of third party code which we don't want to be
> > scanning+triaging (especially given how out of date some of the code is).
> 
> Correct.  That is precisely why I prevented stubdom and etherboot from
> being scanned.

I agree with this reasoning.

Ian.

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

Re: [Xen-devel] [PATCHv3 2/2] spinlock: fair read-write locks

[Jan Beulich]

>>> On 03.02.16 at 12:57,  wrote:
> On 03/02/16 11:28, Jan Beulich wrote:
> On 01.02.16 at 12:31,  wrote:
>>> +void queue_write_lock_slowpath(rwlock_t *lock)
>>> +{
>>> +u32 cnts;
>>> +
>>> +/* Put the writer into the wait queue. */
>>> +spin_lock(&lock->lock);
>>> +
>>> +/* Try to acquire the lock directly if no reader is present. */
>>> +if ( !atomic_read(&lock->cnts) &&
>>> + (atomic_cmpxchg(&lock->cnts, 0, _QW_LOCKED) == 0) )
>>> +goto unlock;
>>> +
>>> +/*
>>> + * Set the waiting flag to notify readers that a writer is pending,
>>> + * or wait for a previous writer to go away.
>>> + */
>>> +for (;;)
>> 
>> Since everything else here has been nicely converted to Xen style,
>> strictly speaking these should be
>> 
>> for ( ; ; )
>> 
>> but of course this is no reason to block the patch. Since however,
>> as said in reply to patch 1, ...
> 
> TBH, I really think you're pointlessly nit-picking here.  This change
> would make zero impact on readability.

Well, the style I've pointed out is the one used in well formed code
(and in fact I have to remind myself of this each time I omit one or
more of the three expressions). But as said, I'm not demanding a
re-submission just because of this (and if I remember to do so, I
may well fix it up while committing).

>>> --- a/xen/include/xen/rwlock.h
>>> +++ b/xen/include/xen/rwlock.h
>>> @@ -3,6 +3,188 @@
>>>  
>>>  #include 
>> 
>> ... this should go away if possible, it would be nice for the cosmetic
>> thing above to also be fixed up at once.
> 
> The rwlock structure now includes a spinlock, so this #include is
> required here.

Ah, okay, patch 2 indeed makes this necessary.

Both patches
Acked-by: Jan Beulich 
then.

Jan


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

Re: [Xen-devel] [PATCH V2] arm: p2m.c bug-fix: hypervisor hang on __p2m_get_mem_access

[Corneliu ZUZU]

On 2/3/2016 1:48 PM, Ian Campbell wrote:

On Wed, 2016-01-27 at 14:24 +0200, Corneliu ZUZU wrote:

When __p2m_get_mem_access gets called, the p2m lock is already taken
by either get_page_from_gva or p2m_get_mem_access.

Possible code paths:
1)  -> get_page_from_gva
-> p2m_mem_access_check_and_get_page
-> __p2m_get_mem_access
2)  -> p2m_get_mem_access
-> __p2m_get_mem_access

In both cases if __p2m_get_mem_access subsequently gets to
call p2m_lookup (happens if !radix_tree_lookup(...)), a hypervisor
hang will occur, since p2m_lookup also spin-locks on the p2m lock.

This bug-fix simply replaces the p2m_lookup call from
__p2m_get_mem_access
with a call to __p2m_lookup.

Following Ian's suggestion, we also add an ASSERT to ensure that
the p2m lock is taken upon __p2m_get_mem_access entry.

Signed-off-by: Corneliu ZUZU 

Acked + applied, thanks.


I thought this mail was not sent properly (didn't find it any longer on 
the web (?)) and I resent it just earlier.
I figured it must've been the fact that I forgot to put a "Changed since 
v1" section & that I didn't include an

"--in-reply-to" option. Apparently it was actually sent correctly.
Sorry, ignore the last one (which contains a "Changed since v1" section).

Corneliu.

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

Re: [Xen-devel] [PATCH v6 6/8] arm/gic-v3: Refactor gicv3_init into generic and dt specific parts

[Ian Campbell]

On Sat, 2016-01-30 at 17:03 +0800, Shannon Zhao wrote:
> 
> On 2016/1/28 18:27, Stefano Stabellini wrote:
> > On Thu, 28 Jan 2016, Shannon Zhao wrote:
> > > > From: Shannon Zhao 
> > > > 
> > > > Refactor gic-v3 related functions into dt and generic parts. This
> > > > will be
> > > > helpful when adding acpi support for gic-v3.
> > > > 
> > > > Signed-off-by: Shannon Zhao 
> > Reviewed-by: Stefano Stabellini 
> > 
> Thanks a lot!
> 
> Hi Ian, Would you please pick the last five patches(4-8)?

Done. I nearly missed the hidden v6 but Stefano pointed it out to me.

0f5f9d8 pl011: Refactor pl011 driver to dt and common initialization parts
57c5953 arm/uart: Rename dt-uart.c to arm-uart.c
b3aeac4 arm/gic-v3: Refactor gicv3_init into generic and dt specific parts
57ab13c arm/gic-v2: Refactor gicv2_init into generic and dt specific parts
eaf1de3 arm/smpboot: Move dt specific code in smp to seperate functions

There was one minor conflict with a MAINTAINERS change, but I trivially
resolved it.


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

Re: [Xen-devel] missing lock in percpu_rwlock? (Was: Re: New Defects reported by Coverity Scan for XenProject)

[Andrew Cooper]

On 03/02/16 11:00, Ian Campbell wrote:
> On Wed, 2016-02-03 at 10:50 +, Andrew Cooper wrote:
>> On 03/02/16 10:45, Ian Campbell wrote:
>>> On Tue, 2016-02-02 at 20:23 -0800, scan-ad...@coverity.com wrote:
 * CID 1351223:  Concurrent data access violations  (MISSING_LOCK)
 /xen/include/xen/spinlock.h: 362 in _percpu_write_unlock()
>>> Coverity seems to think this is new in 41b0aa569adb..9937763265d,
>>> presumably due to 
>>>
>>> commit f9dd43dddc0a31a4343a58072935c1b5c0cbbee
>>> Author: Malcolm Crossley 
>>> Date:   Fri Jan 22 16:04:41 2016 +0100
>>>
>>> rwlock: add per-cpu reader-writer lock infrastructure
>> Expected behaviour.  writer_activating is expected to only be written
>> under lock, but read without lock.
> I suppose this is something we should eventually model?

Short of annotating the source code with Coverity comments (which has
already been objected to), I don't see a way.

This issue is Coverity (correctly) observing the behaviour of the
function, and coming to the wrong conclusion.  The modelling file is
used to correct the interpretation of the behaviour of the function.

>
> Would you typically mark this as "False positive" or "Intentional"?

I would err on the side of Intentional.

The analysis of the issue was correct; that data was accessed both with
and without the lock, and that this usually means a data race condition.

In this case, it is a deliberate algorithm decision to have the data
access like this.

>
> I just marked a couple of libxl ones about taking ctx->lock (which is
> recursive) twice as "False positive", but perhaps "Intentional" is the
> correct designation there?

There is an attempt to model this in the model file, but it clearly
isn't taking.

~Andrew

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

Re: [Xen-devel] [PATCH v2] arm: p2m.c bug-fix: hypervisor hang on __p2m_get_mem_access

[Corneliu ZUZU]

On 2/3/2016 1:52 PM, Ian Campbell wrote:

On Wed, 2016-02-03 at 13:37 +0200, Corneliu ZUZU wrote:

I just now applied a previous v2 which was already in my queue. Was this
just an accidental resend of v2 or is there some important change and this
is really a v3?


When __p2m_get_mem_access gets called, the p2m lock is already taken
by either get_page_from_gva or p2m_get_mem_access.
Possible code paths:
1)  -> get_page_from_gva
-> p2m_mem_access_check_and_get_page
-> __p2m_get_mem_access
2)  -> p2m_get_mem_access
-> __p2m_get_mem_access

In both cases if __p2m_get_mem_access subsequently gets to
call p2m_lookup (happens if !radix_tree_lookup(...)), a hypervisor
hang will occur, since p2m_lookup also spin-locks on the p2m lock.

This bug-fix simply replaces the p2m_lookup call from
__p2m_get_mem_access
with a call to __p2m_lookup and also adds an ASSERT to ensure that the
p2m lock
is already taken upon __p2m_get_mem_access entry.

Signed-off-by: Corneliu ZUZU 

---
Changed since v1:
   * added p2m-lock ASSERT
---
  xen/arch/arm/p2m.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 2190908..e8e6db4 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -468,6 +468,8 @@ static int __p2m_get_mem_access(struct domain *d,
gfn_t gfn,
  #undef ACCESS
  };
  
+ASSERT(spin_is_locked(&p2m->lock));

+
  /* If no setting was ever set, just return rwx. */
  if ( !p2m->mem_access_enabled )
  {
@@ -490,7 +492,7 @@ static int __p2m_get_mem_access(struct domain *d,
gfn_t gfn,
   * No setting was found in the Radix tree. Check if the
   * entry exists in the page-tables.
   */
-paddr_t maddr = p2m_lookup(d, gfn_x(gfn) << PAGE_SHIFT, NULL);
+paddr_t maddr = __p2m_lookup(d, gfn_x(gfn) << PAGE_SHIFT, NULL);
  if ( INVALID_PADDR == maddr )
  return -ESRCH;
  

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
No, sorry, this is just a duplicate of the 1st v2, I thought the first 
one was not sent properly (after waiting a few days and noticing

I was no longer finding it on the web).
Ignore this one. And thanks.

Corneliu.

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

Re: [Xen-devel] [RFC Design Doc] Add vNVDIMM support for Xen

[Haozhong Zhang]

On 02/03/16 02:18, Jan Beulich wrote:
> >>> On 03.02.16 at 09:28,  wrote:
> > On 02/02/16 14:15, Konrad Rzeszutek Wilk wrote:
> >> > 3.1 Guest clwb/clflushopt/pcommit Enabling
> >> > 
> >> >  The instruction enabling is simple and we do the same work as in 
> >> > KVM/QEMU.
> >> >  - All three instructions are exposed to guest via guest cpuid.
> >> >  - L1 guest pcommit is never intercepted by Xen.
> >> 
> >> I wish there was some watermarks like the PLE has.
> >> 
> >> My fear is that an unfriendly guest can issue sfence all day long
> >> flushing out other guests MMC queue (the writes followed by pcommits).
> >> Which means that an guest may have degraded performance as their
> >> memory writes are being flushed out immediately as if they were
> >> being written to UC instead of WB memory. 
> > 
> > pcommit takes no parameter and it seems hard to solve this problem
> > from hardware for now. And the current VMX does not provide mechanism
> > to limit the commit rate of pcommit like PLE for pause.
> > 
> >> In other words - the NVDIMM resource does not provide any resource
> >> isolation. However this may not be any different than what we had
> >> nowadays with CPU caches.
> >>
> > 
> > Does Xen have any mechanism to isolate multiple guests' operations on
> > CPU caches?
> 
> No. All it does is disallow wbinvd for guests not controlling any
> actual hardware. Perhaps pcommit should at least be limited in
> a similar way?
>

But pcommit is a must that makes writes be persistent on pmem. I'll
look at how guest wbinvd is limited in Xen. Any functions suggested,
vmx_wbinvd_intercept()?

Thanks,
Haozhong

> >> >  - L1 hypervisor is allowed to intercept L2 guest pcommit.
> >> 
> >> clwb?
> > 
> > VMX is not capable to intercept clwb. Any reason to intercept it?
> 
> I don't think so - otherwise normal memory writes might also need
> intercepting. Bus bandwidth simply is shared (and CLWB operates
> on a guest virtual address, so can only cause bus traffic for cache
> lines the guest has managed to dirty).
> 
> Jan
> 

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

Re: [Xen-devel] [PATCH v3 3/3] tools: introduce parameter max_wp_ram_ranges.

[Paul Durrant]

> -Original Message-
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: 03 February 2016 08:33
> To: Zhang Yu
> Cc: Andrew Cooper; Ian Campbell; Paul Durrant; Wei Liu; Ian Jackson; Stefano
> Stabellini; Kevin Tian; zhiyuan...@intel.com; xen-devel@lists.xen.org; Keir
> (Xen.org)
> Subject: Re: [Xen-devel] [PATCH v3 3/3] tools: introduce parameter
> max_wp_ram_ranges.
> 
> >>> On 03.02.16 at 08:10,  wrote:
> > On 2/2/2016 11:21 PM, Jan Beulich wrote:
> > On 02.02.16 at 16:00,  wrote:
> >>> The limit of 4G is to avoid the data missing from uint64 to uint32
> >>> assignment. And I can accept the 8K limit for XenGT in practice.
> >>> After all, it is vGPU page tables we are trying to trap and emulate,
> >>> not normal page frames.
> >>>
> >>> And I guess the reason that one domain exhausting Xen's memory can
> >>> affect another domain is because rangeset uses Xen heap, instead of
> the
> >>> per-domain memory. So what about we use a 8K limit by now for XenGT,
> >>> and in the future, if a per-domain memory allocation solution for
> >>> rangeset is ready, we do need to limit the rangeset size. Does this
> >>> sounds more acceptable?
> >>
> >> The lower the limit the better (but no matter how low the limit
> >> it won't make this a pretty thing). Anyway I'd still like to wait
> >> for what Ian may further say on this.
> >>
> > Hi Jan, I just had a discussion with my colleague. We believe 8K could
> > be the biggest limit for the write-protected ram ranges. If in the
> > future, number of vGPU page tables exceeds this limit, we will modify
> > our back-end device model to find a trade-off method, instead of
> > extending this limit. If you can accept this value as the upper bound
> > of rangeset, maybe we do not need to add any tool stack parameters, but
> > define a MAX_NR_WR_RAM_RANGES for the write-protected ram
> rangesset. As
> > to other rangesets, we keep their limit as 256. Does this sounds OK? :)
> 
> I'm getting the impression that we're moving in circles. A blanket
> limit above the 256 one for all domains is _not_ going to be
> acceptable; going to 8k will still need host admin consent. With
> your rangeset performance improvement patch, each range is
> going to be tracked by a 40 byte structure (up from 32), which
> already means an overhead increase for all the other ranges. 8k
> of wp ranges implies an overhead beyond 448k (including the
> xmalloc() overhead), which is not _that_ much, but also not
> negligible.
> 

... which means we are still going to need a toolstack parameter to set the 
limit. We already have a parameter for VRAM size so is having a parameter for 
max. GTT shadow ranges such a bad thing? Is the fact that the memory comes from 
xenheap rather than domheap the real problem?

  Paul

> Jan


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

Re: [Xen-devel] Xen 4.7 Development Update

[Olaf Hering]

On Mon, Feb 01, Wei Liu wrote:

> == Toolstack == 

> *  Libxl PVSCSI support
>   -  Olaf Hering


I will send a new variant later this week. Getting pvscsi into 4.7 will
most likely require much of your time for review, I hope you guys find
the time and energy to do that.

Olaf

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

Re: [Xen-devel] [PATCH V2] arm: p2m.c bug-fix: hypervisor hang on __p2m_get_mem_access

[Ian Campbell]

On Wed, 2016-02-03 at 13:54 +0200, Corneliu ZUZU wrote:
> On 2/3/2016 1:48 PM, Ian Campbell wrote:
> > On Wed, 2016-01-27 at 14:24 +0200, Corneliu ZUZU wrote:
> > > When __p2m_get_mem_access gets called, the p2m lock is already taken
> > > by either get_page_from_gva or p2m_get_mem_access.
> > > 
> > > Possible code paths:
> > > 1)-> get_page_from_gva
> > >   -> p2m_mem_access_check_and_get_page
> > >   -> __p2m_get_mem_access
> > > 2)-> p2m_get_mem_access
> > >   -> __p2m_get_mem_access
> > > 
> > > In both cases if __p2m_get_mem_access subsequently gets to
> > > call p2m_lookup (happens if !radix_tree_lookup(...)), a hypervisor
> > > hang will occur, since p2m_lookup also spin-locks on the p2m lock.
> > > 
> > > This bug-fix simply replaces the p2m_lookup call from
> > > __p2m_get_mem_access
> > > with a call to __p2m_lookup.
> > > 
> > > Following Ian's suggestion, we also add an ASSERT to ensure that
> > > the p2m lock is taken upon __p2m_get_mem_access entry.
> > > 
> > > Signed-off-by: Corneliu ZUZU 
> > Acked + applied, thanks.
> > 
> > 
> I thought this mail was not sent properly (didn't find it any longer on 
> the web (?)) and I resent it just earlier.
> I figured it must've been the fact that I forgot to put a "Changed since 
> v1" section & that I didn't include an
> "--in-reply-to" option. Apparently it was actually sent correctly.
> Sorry, ignore the last one (which contains a "Changed since v1" section).

OK, please check that what is currently in xen.git#staging is what you
think should be there.

Ian.


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

Re: [Xen-devel] missing lock in percpu_rwlock? (Was: Re: New Defects reported by Coverity Scan for XenProject)

[Andrew Cooper]

On 03/02/16 12:21, Andrew Cooper wrote:
> On 03/02/16 11:00, Ian Campbell wrote:
>> On Wed, 2016-02-03 at 10:50 +, Andrew Cooper wrote:
>>> On 03/02/16 10:45, Ian Campbell wrote:
 On Tue, 2016-02-02 at 20:23 -0800, scan-ad...@coverity.com wrote:
> * CID 1351223:  Concurrent data access violations  (MISSING_LOCK)
> /xen/include/xen/spinlock.h: 362 in _percpu_write_unlock()
 Coverity seems to think this is new in 41b0aa569adb..9937763265d,
 presumably due to 

 commit f9dd43dddc0a31a4343a58072935c1b5c0cbbee
 Author: Malcolm Crossley 
 Date:   Fri Jan 22 16:04:41 2016 +0100

 rwlock: add per-cpu reader-writer lock infrastructure
>>> Expected behaviour.  writer_activating is expected to only be written
>>> under lock, but read without lock.
>> I suppose this is something we should eventually model?
> Short of annotating the source code with Coverity comments (which has
> already been objected to), I don't see a way.
>
> This issue is Coverity (correctly) observing the behaviour of the
> function, and coming to the wrong conclusion.  The modelling file is
> used to correct the interpretation of the behaviour of the function.
>
>> Would you typically mark this as "False positive" or "Intentional"?
> I would err on the side of Intentional.
>
> The analysis of the issue was correct; that data was accessed both with
> and without the lock, and that this usually means a data race condition.
>
> In this case, it is a deliberate algorithm decision to have the data
> access like this.
>
>> I just marked a couple of libxl ones about taking ctx->lock (which is
>> recursive) twice as "False positive", but perhaps "Intentional" is the
>> correct designation there?
> There is an attempt to model this in the model file, but it clearly
> isn't taking.

(I meant to say as well)

This I would err in the side of false positive, with "modelling
required" as a reason.  The lock is a recursive lock and Coverity should
be able to spot this fact, but can't for some reason.

~Andrew

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

Re: [Xen-devel] [PATCH v4] xen: sched: convert RTDS from time to event driven model

[Dario Faggioli]

On Tue, 2016-02-02 at 22:33 -0500, Meng Xu wrote:
> On Tue, Feb 2, 2016 at 10:08 AM, Dario Faggioli
>  wrote:
> > 
> > Is it ok to kill the replenishment in this case?
> > 
> > This is a genuine question. What does the dynamic DS algorithm that
> > you're trying to implement here says about this? Meng, maybe you
> > can
> > help here?
> 
> 
> Based on the DS algorithm (in theory), each VCPU should have its
> budget replenished at the beginning of its next period.
> 
> However, we are thinking that when a VCPU is put to sleep, no one is
> supposed to use it. Do we really need to keep updating the VCPU's
> budget? Can we just update the VCPU's budget when it is waken up
> later? This could potentially save some implementation overhead,
> IMHO.
> That's why we decided not to update the budget of VCPUs that are put
> into sleep.
> 
> > Is it ok to do this _because_ you then handle the situation of a
> > replenishment having to had happened while the vcpu was asleep in
> > rt_vcpu_wake (the '(now>=svc->cur_deadline)' thing)? Yes... it
> > probably
> > is ok for that reason...
> 
> 
> Yes, exactly. We hope this could reduce the frequency of invoking the
> replenishment timer when the system is (kind of) idle.
> 
I see what you mean. I wonder, however, given how big and complicated
this re-structuring we are doing is, whether it would not be easier to
just leave this optimization for the future, and just implement the
algorithm in the new event-driven fashion, as first step.

Early optimization is known to be a bad thing in software.

Note that I'm not saying that the optimization should happen in 6
months, it can be patch number 2 of the same series doing the event-
driven transformation... I'm just saying that we should probably have a
patch 1 which does only that, for ease of both  doing and reviewing.

Regards,
Dario
-- 
<> (Raistlin Majere)
-
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)



signature.asc
Description: This is a digitally signed message part
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH v2 0/2] x86: also use alternative asm on xsave side

[Jan Beulich]

1: support 2- and 3-way alternatives
2: xstate: also use alternative asm on xsave side

Signed-off-by: Jan Beulich 


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

Re: [Xen-devel] missing lock in percpu_rwlock? (Was: Re: New Defects reported by Coverity Scan for XenProject)

[Ian Campbell]

On Wed, 2016-02-03 at 12:24 +, Andrew Cooper wrote:
> On 03/02/16 12:21, Andrew Cooper wrote:
> > On 03/02/16 11:00, Ian Campbell wrote:
> > > On Wed, 2016-02-03 at 10:50 +, Andrew Cooper wrote:
> > > > On 03/02/16 10:45, Ian Campbell wrote:
> > > > > On Tue, 2016-02-02 at 20:23 -0800, scan-ad...@coverity.com wrote:
> > > > > > * CID 1351223:  Concurrent data access
> > > > > > violations  (MISSING_LOCK)
> > > > > > /xen/include/xen/spinlock.h: 362 in _percpu_write_unlock()
> > > > > Coverity seems to think this is new in 41b0aa569adb..9937763265d,
> > > > > presumably due to 
> > > > > 
> > > > > commit f9dd43dddc0a31a4343a58072935c1b5c0cbbee
> > > > > Author: Malcolm Crossley 
> > > > > Date:   Fri Jan 22 16:04:41 2016 +0100
> > > > > 
> > > > > rwlock: add per-cpu reader-writer lock infrastructure
> > > > Expected behaviour.  writer_activating is expected to only be
> > > > written
> > > > under lock, but read without lock.
> > > I suppose this is something we should eventually model?
> > Short of annotating the source code with Coverity comments (which has
> > already been objected to), I don't see a way.
> > 
> > This issue is Coverity (correctly) observing the behaviour of the
> > function, and coming to the wrong conclusion.  The modelling file is
> > used to correct the interpretation of the behaviour of the function.
> > 
> > > Would you typically mark this as "False positive" or "Intentional"?
> > I would err on the side of Intentional.
> > 
> > The analysis of the issue was correct; that data was accessed both with
> > and without the lock, and that this usually means a data race
> > condition.
> > 
> > In this case, it is a deliberate algorithm decision to have the data
> > access like this.

Done. I linked to your explanation in the comments.

> > 
> > > I just marked a couple of libxl ones about taking ctx->lock (which is
> > > recursive) twice as "False positive", but perhaps "Intentional" is
> > > the
> > > correct designation there?
> > There is an attempt to model this in the model file, but it clearly
> > isn't taking.
> 
> (I meant to say as well)
> 
> This I would err in the side of false positive, with "modelling
> required" as a reason.  The lock is a recursive lock and Coverity should
> be able to spot this fact, but can't for some reason.

Good idea, I'll update those.

Ian

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

Re: [Xen-devel] [PATCH v3 3/3] tools: introduce parameter max_wp_ram_ranges.

[Jan Beulich]

>>> On 03.02.16 at 13:20,  wrote:
>>  -Original Message-
>> From: Jan Beulich [mailto:jbeul...@suse.com]
>> Sent: 03 February 2016 08:33
>> To: Zhang Yu
>> Cc: Andrew Cooper; Ian Campbell; Paul Durrant; Wei Liu; Ian Jackson; Stefano
>> Stabellini; Kevin Tian; zhiyuan...@intel.com; xen-devel@lists.xen.org; Keir
>> (Xen.org)
>> Subject: Re: [Xen-devel] [PATCH v3 3/3] tools: introduce parameter
>> max_wp_ram_ranges.
>> 
>> >>> On 03.02.16 at 08:10,  wrote:
>> > On 2/2/2016 11:21 PM, Jan Beulich wrote:
>> > On 02.02.16 at 16:00,  wrote:
>> >>> The limit of 4G is to avoid the data missing from uint64 to uint32
>> >>> assignment. And I can accept the 8K limit for XenGT in practice.
>> >>> After all, it is vGPU page tables we are trying to trap and emulate,
>> >>> not normal page frames.
>> >>>
>> >>> And I guess the reason that one domain exhausting Xen's memory can
>> >>> affect another domain is because rangeset uses Xen heap, instead of
>> the
>> >>> per-domain memory. So what about we use a 8K limit by now for XenGT,
>> >>> and in the future, if a per-domain memory allocation solution for
>> >>> rangeset is ready, we do need to limit the rangeset size. Does this
>> >>> sounds more acceptable?
>> >>
>> >> The lower the limit the better (but no matter how low the limit
>> >> it won't make this a pretty thing). Anyway I'd still like to wait
>> >> for what Ian may further say on this.
>> >>
>> > Hi Jan, I just had a discussion with my colleague. We believe 8K could
>> > be the biggest limit for the write-protected ram ranges. If in the
>> > future, number of vGPU page tables exceeds this limit, we will modify
>> > our back-end device model to find a trade-off method, instead of
>> > extending this limit. If you can accept this value as the upper bound
>> > of rangeset, maybe we do not need to add any tool stack parameters, but
>> > define a MAX_NR_WR_RAM_RANGES for the write-protected ram
>> rangesset. As
>> > to other rangesets, we keep their limit as 256. Does this sounds OK? :)
>> 
>> I'm getting the impression that we're moving in circles. A blanket
>> limit above the 256 one for all domains is _not_ going to be
>> acceptable; going to 8k will still need host admin consent. With
>> your rangeset performance improvement patch, each range is
>> going to be tracked by a 40 byte structure (up from 32), which
>> already means an overhead increase for all the other ranges. 8k
>> of wp ranges implies an overhead beyond 448k (including the
>> xmalloc() overhead), which is not _that_ much, but also not
>> negligible.
>> 
> 
> ... which means we are still going to need a toolstack parameter to set the 
> limit. We already have a parameter for VRAM size so is having a parameter for 
> max. GTT shadow ranges such a bad thing?

It's workable, but not nice (see also Ian's earlier response).

> Is the fact that the memory comes 
> from xenheap rather than domheap the real problem?

Not the primary one, since except on huge memory machines
both heaps are identical. To me the primary one is the quite
more significant resource consumption in the first place (I'm not
going to repeat what I've written in already way too many
replies before).

Jan


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