Re: [Xen-devel] [Patch v11 3/3] vt-d: fix vt-d Device-TLB flush timeout issue

[Jan Beulich]

>>> On 17.06.16 at 08:08,  wrote:

> On June 16, 2016 5:05 PM, Jan Beulich  wrote:
>> >>> On 16.06.16 at 10:42,  wrote:
>> > On June 02, 2016 7:07 PM, Jan Beulich  wrote:
>> >> >>> On 01.06.16 at 11:05,  wrote:
>> >> > +static void dev_invalidate_iotlb_timeout(struct iommu *iommu, u16 did,
>> >> > + struct pci_ats_dev
>> >> > +*ats_dev) {
>> >> > +struct domain *d = NULL;
>> >> > +struct pci_dev *pdev;
>> >> > +
>> >> > +if ( test_bit(did, iommu->domid_bitmap) )
>> >> > +d = rcu_lock_domain_by_id(iommu->domid_map[did]);
>> >> > +
>> >> > +/*
>> >> > + * In case the domain has been freed or the IOMMU domid bitmap is
>> >> > + * not valid, the device no longer belongs to this domain.
>> >> > + */
>> >> > +if ( d == NULL )
>> >> > +return;
>> >> > +
>> >> > +pcidevs_lock();
>> >> > +
>> >> > +for_each_pdev(d, pdev)
>> >> > +{
>> >> > +if ( (pdev->seg == ats_dev->seg) &&
>> >> > + (pdev->bus == ats_dev->bus) &&
>> >> > + (pdev->devfn == ats_dev->devfn) )
>> >> > +{
>> >> > +ASSERT(pdev->domain);
>> >> > +list_del(&pdev->domain_list);
>> >> > +pdev->domain = NULL;
>> >> > +pci_hide_existing_device(pdev);
>> >> > +break;
>> >> > +}
>> >> > +}
>> >> > +
>> >> > +pcidevs_unlock();
>> >>
>> >> ... this loop (and locking). (Of course such a change may better be
>> >> done in another preparatory patch.)
>> >>
>> >
>> > To eliminate the locking?  I am afraid the locking is still a must
>> > here even without the loop, also referring  to device_assigned()..
>> 
>> If the entire loop gets eliminated, what would be left is
>> 
>> pcidevs_lock();
>> pcidevs_unlock();
>> 
>> which I don't think does any good.
> 
> Why? I can't follow it..

I don't understand your question. Can you explain what use above
code sequence is, in your opinion? Or else - what does the "why"
refer to?

>> >> > +static int __must_check dev_invalidate_sync(struct iommu *iommu,
>> >> > +u16
>> >> did,
>> >> > +struct pci_ats_dev
>> >> > +*ats_dev) {
>> >> > +struct qi_ctrl *qi_ctrl = iommu_qi_ctrl(iommu);
>> >> > +int rc = 0;
>> >> > +
>> >> > +if ( qi_ctrl->qinval_maddr )
>> >> > +{
>> >> > +rc = queue_invalidate_wait(iommu, 0, 1, 1, 1);
>> >> > +
>> >> > +if ( rc == -ETIMEDOUT )
>> >> > +dev_invalidate_iotlb_timeout(iommu, did, ats_dev);
>> >> > +}
>> >> > +
>> >> > +return rc;
>> >> > +}
>> >>
>> >> I've never really understood why invalidate_sync() returns success
>> >> when it didn't do anything. Now that you copy this same behavior
>> >> here, I really need to ask you to explain that.
>> >>
>> >
>> > It is acceptable to me, returning success when it didn't do anything
>> > -- this is worth reflection and criticism:(..
>> > It is better:
>> > +if ( qi_ctrl->qinval_maddr )
>> > +...
>> > +else
>> > +rc = -ENOENT;
>> 
>> Right. And perhaps a separate patch to make invalidate_sync() do the same.
> 
> Agreed.
> 
>> Question is whether this really ought to be a conditional, or whether 
> instead
>> this code is unreachable when qinval is not in use, in which case these
>> conditionals would imo better be converted to ASSERT()s.
>> 
> 
> IMO, this should be a conditional.
> As mentioned below, strictly speaking, this is a bug. We can't  ASSERT() 
> based on a bug..
> A coming patch may fix it..

And again I don't understand: ASSERT()s are to verify assumed
state. If static code analysis resulted in understanding a function
is unreachable when qi_ctrl->qinval_maddr is zero (because
qinval ought to have got disabled if any of the table setup failed),
then adding ASSERT() would (a) document that and (b) allow to
know quickly if something broke that assumption.

But then again I may simply misunderstand your wording: "We
can't ASSERT() based on a bug" is really pretty unclear to me.

Jan

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

Re: [Xen-devel] [PATCH 1/7] minor (formatting) fixes

[Jan Beulich]

>>> On 16.06.16 at 21:19,  wrote:
> On 6/16/2016 5:24 PM, Jan Beulich wrote:
> On 16.06.16 at 16:06,  wrote:
>>> --- a/xen/arch/x86/hvm/event.c
>>> +++ b/xen/arch/x86/hvm/event.c
>>> @@ -23,6 +23,7 @@
>>>   
>>>   #include 
>>>   #include 
>>> +#include 
>>>   #include 
>>>   #include 
>>>   #include 
>>> diff --git a/xen/common/monitor.c b/xen/common/monitor.c
>>> index d950a7c..b30857a 100644
>>> --- a/xen/common/monitor.c
>>> +++ b/xen/common/monitor.c
>>> @@ -22,7 +22,6 @@
>>>   #include 
>>>   #include 
>>>   #include 
>>> -#include 
>>>   #include 
>>>   #include 
>> These two adjustments clearly don't fit title / description. I certainly
>> don't mind unnecessary inclusions to be dropped, but the addition of
>> one clearly needs explanation (after all thing build fine without it).
> 
> Sorry, that was done out of reflex, should have stated the reasoning.
> Generally, if:
> - event.c includes event.h
> - event.c needs paging.h
> - event.h -doesn't need- paging.h
> then I prefer to include paging.h in event.c, not in event.h (include 
> strictly -where- needed).
> Also since xen/paging.h included asm/paging.h and event.c only needs the 
> asm/paging.h part, I also changed xen/paging.h inclusion -> asm/paging.h 
> inclusion (include strictly -what's- needed).
> But I can revert that if you want me to (not that important and these 
> little things are better done with automatic tools anyway), or should I 
> leave the change and update the commit message?

Well, I'd leave the ultimate decision to the maintainers, but I'd say
this doesn't belong in a patch dealing with formatting issues. I.e.
I'm fine with the cleanup, but either under a different title (and
with at least an abbreviated version of what you said above), or
in a separate patch. Considering that you appear to do the same
in later patches, perhaps consolidating all the #include adjustments
into one patch would a good idea?

>>> --- a/xen/include/xen/vm_event.h
>>> +++ b/xen/include/xen/vm_event.h
>>> @@ -85,7 +85,6 @@ void vm_event_monitor_guest_request(void);
>>>   
>>>   #endif /* __VM_EVENT_H__ */
>>>   
>>> -
>>>   /*
>>>* Local variables:
>>>* mode: C
>> Why don't you remove the other stray blank line at once?
> 
> What stray line? Shouldn't there be -one- blank line between the #endif 
> and the local vars block?
> Looking @ other files that rule seems to hold...

Oh, you're right. I thought I had frequently seen it the other way
around (and it would seem more logical to me), but I must have been
misremembering.

Jan


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

Re: [Xen-devel] [PATCH 3/7] vm-event: introduce vm_event_vcpu_enter

[Jan Beulich]

>>> On 16.06.16 at 22:10,  wrote:
> On 6/16/2016 5:51 PM, Jan Beulich wrote:
> On 16.06.16 at 16:08,  wrote:
>>> @@ -509,6 +508,8 @@ void hvm_do_resume(struct vcpu *v)
>>>   }
>>>   }
>>>   
>>> +vm_event_vcpu_enter(v);
>> Why here?
> Why indeed. It made sense because monitor_write_data handling was 
> originally there and then the plan was to move it to vm_event_vcpu_enter 
> (which happens in the following commit).
> The question is though, why was monitor_write_data handled there in the 
> first place? Why was it not put e.g. in vmx_do_resume immediately after 
> the call to hvm_do_resume and just before
> the reset_stack_and_jump...? And what happens with handling 
> monitor_write_data if this:
> 
> if ( !handle_hvm_io_completion(v) )
>  return;
> 
> causes a return?

I see Razvan responded to this. I don't have a strong opinion
either way, my only request if for the call to be in exactly one
place.

>>> --- a/xen/arch/x86/mm/p2m.c
>>> +++ b/xen/arch/x86/mm/p2m.c
>>> @@ -36,7 +36,6 @@
>>>   #include 
>>>   #include 
>>>   #include 
>>> -#include 
>>>   #include 
>> There are way too many of these #include adjustments here. If
>> you really mean to clean these up, please don't randomly throw
>> this into various unrelated patches.
> 
> I haven't thrown anything "randomly into unrelated patches", please 
> first ask for my reasoning and then draw such conclusions.

See patch 1. Plus I don't think I (or in fact any reviewer) should ask
for such reasoning: Instead you should state extra cleanup you do
to unrelated (to the purpose of your patch) files in the description.
Or even better, split it off to a follow-on, purely cleanup patch.
(And to be clear, I much appreciate any form of reduction of the
sometimes extremely long lists of #include-s, just not [apparently
or really] randomly mixed with other, substantial changes. That's
namely because it's not clear whether source files should explicitly
include everything they need, or instead be allowed to rely on
headers they include to include further headers they also
_explicitly_ rely on. IOW there's likely a discussion to be had for
this kind of cleanup, and such a discussion should be a separate
thread from the one on the functional adjustments here.)

Jan

> That was removed because xen/vm_event.h includes asm/vm_event.h with 
> this patch (because it calls arch_vm_event_vcpu_enter) and this file 
> (p2m.c) already included xen/vm_event.h.



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

Re: [Xen-devel] [PATCH 4/7] vm-event/x86: use vm_event_vcpu_enter properly

[Jan Beulich]

>>> On 16.06.16 at 22:20,  wrote:
> On 6/16/2016 6:00 PM, Jan Beulich wrote:
> On 16.06.16 at 16:09,  wrote:
>>> @@ -1432,18 +1430,16 @@ static void vmx_update_guest_cr(struct vcpu *v, 
>>> unsigned int cr)
>>>   if ( paging_mode_hap(v->domain) )
>>>   {
>>>   /* Manage GUEST_CR3 when CR0.PE=0. */
>>> +uint32_t old_ctls = v->arch.hvm_vmx.exec_control;
>>>   uint32_t cr3_ctls = (CPU_BASED_CR3_LOAD_EXITING |
>>>CPU_BASED_CR3_STORE_EXITING);
>>> +
>>>   v->arch.hvm_vmx.exec_control &= ~cr3_ctls;
>>>   if ( !hvm_paging_enabled(v) && !vmx_unrestricted_guest(v) )
>>>   v->arch.hvm_vmx.exec_control |= cr3_ctls;
>>>   
>>> -/* Trap CR3 updates if CR3 memory events are enabled. */
>>> -if ( v->domain->arch.monitor.write_ctrlreg_enabled &
>>> - monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3) )
>>> -v->arch.hvm_vmx.exec_control |= CPU_BASED_CR3_LOAD_EXITING;
>>> -
>>> -vmx_update_cpu_exec_control(v);
>>> +if ( old_ctls != v->arch.hvm_vmx.exec_control )
>>> +vmx_update_cpu_exec_control(v);
>>>   }
>> How does this match up with the rest of this patch?
> 
> And by 'this' you mean slightly optimizing this sequence by adding in 
> old_ctls?
> It seems pretty straight-forward to me, I figured if I am to move the 
> monitor.write_ctrlreg_enabled part from here
> it wouldn't be much of a stretch to also do this little 
> optimization...what would have been appropriate?
> To do this in a separate patch? To mention it in the commit message?

At least the latter, and perhaps better the former. Without even
mentioning it the readers (reviewers) have to guess whether this
is an integral part of the change, or - as you now confirm - just a
minor optimization done along the road.

Jan


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

[Xen-devel] [xen-4.6-testing test] 95825: regressions - trouble: blocked/broken/fail/pass

[osstest service owner]

flight 95825 xen-4.6-testing real [real]
http://logs.test-lab.xenproject.org/osstest/logs/95825/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-amd64-i386-freebsd10-amd64  3 host-install(3)  broken REGR. vs. 95718
 build-armhf   3 host-install(3) broken REGR. vs. 95718
 test-armhf-armhf-libvirt-raw  6 xen-boot fail in 95776 REGR. vs. 95718

Tests which are failing intermittently (not blocking):
 test-amd64-i386-migrupgrade 4 host-install/dst_host(4) broken in 95776 pass in 
95825
 test-amd64-i386-xl-qemut-debianhvm-amd64-xsm 3 host-install(3) broken in 95776 
pass in 95825
 test-amd64-i386-libvirt-xsm   3 host-install(3)   broken pass in 95776
 test-amd64-i386-xl-qemut-debianhvm-amd64 3 host-install(3) broken pass in 95776
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 3 host-install(3) broken 
pass in 95776
 test-amd64-i386-xl-qemuu-debianhvm-amd64-xsm 3 host-install(3) broken pass in 
95776
 test-amd64-amd64-xl-qemut-winxpsp3  3 host-install(3) broken pass in 95776

Regressions which are regarded as allowable (not blocking):
 test-amd64-amd64-xl-qemuu-debianhvm-amd64 3 host-install(3) broken in 95776 
like 95645
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 3 host-install(3) broken in 
95776 like 95718
 test-amd64-i386-xl-qemuu-debianhvm-amd64 3 host-install(3) broken in 95776 
like 95718
 test-armhf-armhf-xl-rtds 11 guest-start   fail in 95776 like 95564
 test-amd64-i386-xl-qemuu-win7-amd64 16 guest-stop  fail like 95718
 test-amd64-amd64-xl-qemuu-win7-amd64 16 guest-stop fail like 95718
 test-amd64-amd64-xl-qemut-win7-amd64 16 guest-stop fail like 95718
 test-amd64-i386-xl-qemut-win7-amd64 16 guest-stop  fail like 95718

Tests which did not succeed, but are not blocking:
 test-armhf-armhf-libvirt-xsm  1 build-check(1)   blocked  n/a
 test-armhf-armhf-libvirt  1 build-check(1)   blocked  n/a
 test-armhf-armhf-xl-arndale   1 build-check(1)   blocked  n/a
 test-armhf-armhf-xl-multivcpu  1 build-check(1)   blocked  n/a
 test-armhf-armhf-xl-cubietruck  1 build-check(1)   blocked  n/a
 test-armhf-armhf-xl   1 build-check(1)   blocked  n/a
 test-armhf-armhf-xl-credit2   1 build-check(1)   blocked  n/a
 build-armhf-libvirt   1 build-check(1)   blocked  n/a
 test-armhf-armhf-xl-vhd   1 build-check(1)   blocked  n/a
 test-armhf-armhf-libvirt-raw  1 build-check(1)   blocked  n/a
 test-armhf-armhf-libvirt-qcow2  1 build-check(1)   blocked  n/a
 test-armhf-armhf-xl-rtds  1 build-check(1)   blocked  n/a
 test-armhf-armhf-libvirt-xsm 12 migrate-support-check fail in 95776 never pass
 test-armhf-armhf-libvirt-xsm 14 guest-saverestore fail in 95776 never pass
 test-armhf-armhf-libvirt 14 guest-saverestore fail in 95776 never pass
 test-armhf-armhf-libvirt 12 migrate-support-check fail in 95776 never pass
 test-armhf-armhf-xl-arndale  12 migrate-support-check fail in 95776 never pass
 test-armhf-armhf-xl-arndale 13 saverestore-support-check fail in 95776 never 
pass
 test-amd64-i386-libvirt-xsm  12 migrate-support-check fail in 95776 never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check 
fail in 95776 never pass
 test-armhf-armhf-xl-multivcpu 13 saverestore-support-check fail in 95776 never 
pass
 test-armhf-armhf-xl-multivcpu 12 migrate-support-check fail in 95776 never pass
 test-armhf-armhf-xl-cubietruck 12 migrate-support-check fail in 95776 never 
pass
 test-armhf-armhf-xl-cubietruck 13 saverestore-support-check fail in 95776 
never pass
 test-armhf-armhf-xl  12 migrate-support-check fail in 95776 never pass
 test-armhf-armhf-xl  13 saverestore-support-check fail in 95776 never pass
 test-armhf-armhf-xl-credit2 13 saverestore-support-check fail in 95776 never 
pass
 test-armhf-armhf-xl-credit2  12 migrate-support-check fail in 95776 never pass
 test-armhf-armhf-xl-vhd  11 migrate-support-check fail in 95776 never pass
 test-armhf-armhf-xl-vhd  12 saverestore-support-check fail in 95776 never pass
 test-armhf-armhf-libvirt-qcow2 11 migrate-support-check fail in 95776 never 
pass
 test-armhf-armhf-libvirt-qcow2 13 guest-saverestore   fail in 95776 never pass
 test-amd64-amd64-xl-pvh-intel 11 guest-start  fail  never pass
 test-amd64-amd64-xl-pvh-amd  11 guest-start  fail   never pass
 test-amd64-amd64-libvirt-xsm 12 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt  12 migrate-support-checkfail   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-armhf-armhf-xl-xsm  13 saverestore-support-checkfail   never pas

Re: [Xen-devel] [PATCH 1/8] x86/time: improve cross-CPU clock monotonicity (and more)

[Jan Beulich]

>>> On 16.06.16 at 22:27,  wrote:
>> I.e. my plan was, once the backwards moves are small enough, to maybe
>> indeed compensate them by maintaining a global variable tracking
>> the most recently returned value. There are issues with such an
>> approach too, though: HT effects can result in one hyperthread
>> making it just past that check of the global, then hardware
>> switching to the other hyperthread, NOW() producing a slightly
>> larger value there, and hardware switching back to the first
>> hyperthread only after the second one consumed the result of
>> NOW(). Dario's use would be unaffected by this aiui, as his NOW()
>> invocations are globally serialized through a spinlock, but arbitrary
>> NOW() invocations on two hyperthreads can't be made such that
>> the invoking party can be guaranteed to see strictly montonic
>> values.
>> 
>> And btw., similar considerations apply for two fully independent
>> CPUs, if one runs at a much higher P-state than the other (i.e.
>> the faster one could overtake the slower one between the
>> montonicity check in NOW() and the callers consuming the returned
>> values). So in the end I'm not sure it's worth the performance hit
>> such a global montonicity check would incur, and therefore I didn't
>> make a respective patch part of this series.
>> 
> 
> Hm, guests pvclock should have faced similar issues too as their
> local stamps for each vcpu diverge. Linux commit 489fb49 ("x86, paravirt: 
> Add a
> global synchronization point for pvclock") depicts a fix to similar 
> situations to the
> scenarios you just described - which lead to have a global variable to keep 
> track of
> most recent timestamp. One important chunk of that commit is pasted below 
> for
> convenience:
> 
> --
> /*
>  * Assumption here is that last_value, a global accumulator, always goes
>  * forward. If we are less than that, we should not be much smaller.
>  * We assume there is an error marging we're inside, and then the correction
>  * does not sacrifice accuracy.
>  *
>  * For reads: global may have changed between test and return,
>  * but this means someone else updated poked the clock at a later time.
>  * We just need to make sure we are not seeing a backwards event.
>  *
>  * For updates: last_value = ret is not enough, since two vcpus could be
>  * updating at the same time, and one of them could be slightly behind,
>  * making the assumption that last_value always go forward fail to hold.
>  */
>  last = atomic64_read(&last_value);
>  do {
>  if (ret < last)
>  return last;
>  last = atomic64_cmpxchg(&last_value, last, ret);
>  } while (unlikely(last != ret));
> --

Meaning they decided it's worth the overhead. But (having read
through the entire description) they don't even discuss whether this
indeed eliminates _all_ apparent backward moves due to effects
like the ones named above.

Plus, the contention they're facing is limited to a single VM, i.e. likely
much more narrow than that on an entire physical system. So for
us to do the same in the hypervisor, quite a bit more of win would
be needed to outweigh the cost.

Jan


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

Re: [Xen-devel] [xen-4.6-testing test] 95825: regressions - trouble: blocked/broken/fail/pass

[Jan Beulich]

>>> On 17.06.16 at 09:27,  wrote:
> flight 95825 xen-4.6-testing real [real]
> http://logs.test-lab.xenproject.org/osstest/logs/95825/ 
> 
> Regressions :-(
> 
> Tests which did not succeed and are blocking,
> including tests which could not be run:
>  test-amd64-i386-freebsd10-amd64  3 host-install(3)  broken REGR. vs. 
> 95718

So yesterday in IRC you said only rimava1 would have an issue, yet
afaict this one failed on rimava0.

Jan


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

Re: [Xen-devel] qemu-traditional build problem for Xen 4.7.0 RC6

[Jan Beulich]

>>> On 17.06.16 at 05:11,  wrote:
> QEMU_TAG update to 698d6d6f8d095edadb0c23612b552a89dd3eee4c of traditional 
> qemu in Config.mk, this commit is a branch stable-4.7 commit for qemu 
> traditional. The master commit is 6e20809727261599e8527c456eb078c0e89139a1.
> It bring up a build error.

It would help quite a bit if you also said what kind of build error you
observe.

Jan

> commit 03c716e32912e288929a6e0d9c8729d208462d66
> Author: Ian Jackson 
> Date:   Fri Jun 10 11:49:24 2016 +0100
> 
> QEMU_TAG update
> 
> diff --git a/Config.mk b/Config.mk
> index bc5c456..1d6522c 100644
> --- a/Config.mk
> +++ b/Config.mk
> @@ -284,9 +284,9 @@ SEABIOS_UPSTREAM_REVISION ?= rel-1.9.2
>  ETHERBOOT_NICS ?= rtl8139 8086100e
> 
> 
> -QEMU_TRADITIONAL_REVISION ?= df553c056104e3dd8a2bd2e72539a57c4c085bae
> -# Thu May 5 11:14:44 2016 +0100
> -# Fix build with newer version of GNUTLS
> +QEMU_TRADITIONAL_REVISION ?= 698d6d6f8d095edadb0c23612b552a89dd3eee4c
> +# Thu May 19 19:38:35 2016 +0100
> +# main loop: Big hammer to fix logfile disk DoS in Xen setups
> 
>  # Specify which qemu-dm to use. This may be `ioemu' to use the old
>  # Mercurial in-tree version, or a local directory, or a git URL.
> 
> Best Regards,
> Xudong
> 
> 
> ___
> 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] [PATCH v3 2/2] xen: add update indicator to vcpu_runstate_info

[Jan Beulich]

>>> On 17.06.16 at 07:26,  wrote:
> In order to support reading another vcpu's mapped vcpu_runstate_info
> an indicator for an occurring update of the runstate information is
> needed.
> 
> Add the possibility to activate setting this indicator in the highest
> bit of state_entry_time via a vm_assist hypercall. When activated the
> update indicator will be set before the runstate information is
> modified in guest memory and it will be reset after modification is
> done. As state_entry_time is guaranteed to be different after each
> update the guest can detect any update (either in progress or while
> reading the runstate data) by comparing state_entry_time before and
> after reading runstate data: in case the values differ or the update
> indicator was set the data might be inconsistent and should be reread.
> 
> Signed-off-by: Juergen Gross 

Reviewed-by: Jan Beulich 

But I'd really like to see some of the explanation why we want this
moved here from the cover letter. This of course can be done while
committing, if no other need for a v4 arises.

Jan


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

Re: [Xen-devel] [Patch v11 3/3] vt-d: fix vt-d Device-TLB flush timeout issue

[Xu, Quan]

On June 17, 2016 3:01 PM, Jan Beulich  wrote:
> >>> On 17.06.16 at 08:08,  wrote:
> 
> > On June 16, 2016 5:05 PM, Jan Beulich  wrote:
> >> >>> On 16.06.16 at 10:42,  wrote:
> >> > On June 02, 2016 7:07 PM, Jan Beulich  wrote:
> >> >> >>> On 01.06.16 at 11:05,  wrote:
> >> >> > +static void dev_invalidate_iotlb_timeout(struct iommu *iommu, u16
> did,
> >> >> > + struct pci_ats_dev
> >> >> > +*ats_dev) {
> >> >> > +struct domain *d = NULL;
> >> >> > +struct pci_dev *pdev;
> >> >> > +
> >> >> > +if ( test_bit(did, iommu->domid_bitmap) )
> >> >> > +d = rcu_lock_domain_by_id(iommu->domid_map[did]);
> >> >> > +
> >> >> > +/*
> >> >> > + * In case the domain has been freed or the IOMMU domid bitmap
> is
> >> >> > + * not valid, the device no longer belongs to this domain.
> >> >> > + */
> >> >> > +if ( d == NULL )
> >> >> > +return;
> >> >> > +
> >> >> > +pcidevs_lock();
> >> >> > +
> >> >> > +for_each_pdev(d, pdev)
> >> >> > +{
> >> >> > +if ( (pdev->seg == ats_dev->seg) &&
> >> >> > + (pdev->bus == ats_dev->bus) &&
> >> >> > + (pdev->devfn == ats_dev->devfn) )
> >> >> > +{
> >> >> > +ASSERT(pdev->domain);
> >> >> > +list_del(&pdev->domain_list);
> >> >> > +pdev->domain = NULL;
> >> >> > +pci_hide_existing_device(pdev);
> >> >> > +break;
> >> >> > +}
> >> >> > +}
> >> >> > +
> >> >> > +pcidevs_unlock();
> >> >>
> >> >> ... this loop (and locking). (Of course such a change may better
> >> >> be done in another preparatory patch.)
> >> >>
> >> >
> >> > To eliminate the locking?  I am afraid the locking is still a must
> >> > here even without the loop, also referring  to device_assigned()..
> >>
> >> If the entire loop gets eliminated, what would be left is
> >>
> >> pcidevs_lock();
> >> pcidevs_unlock();
> >>
> >> which I don't think does any good.
> >
> > Why? I can't follow it..
> 
> I don't understand your question. Can you explain what use above code
> sequence is, in your opinion? Or else - what does the "why"
> refer to?
> 

Ah, there may be a gap between us. without this loop,  these pdev operation 
should be still there, such as,


+ASSERT(pdev->domain);
+list_del(&pdev->domain_list);
+pdev->domain = NULL;
+pci_hide_existing_device(pdev);

So, the left is not only:
   pcidevs_lock();
   pcidevs_unlock();


> >> >> > +static int __must_check dev_invalidate_sync(struct iommu
> >> >> > +*iommu,
> >> >> > +u16
> >> >> did,
> >> >> > +struct pci_ats_dev
> >> >> > +*ats_dev) {
> >> >> > +struct qi_ctrl *qi_ctrl = iommu_qi_ctrl(iommu);
> >> >> > +int rc = 0;
> >> >> > +
> >> >> > +if ( qi_ctrl->qinval_maddr )
> >> >> > +{
> >> >> > +rc = queue_invalidate_wait(iommu, 0, 1, 1, 1);
> >> >> > +
> >> >> > +if ( rc == -ETIMEDOUT )
> >> >> > +dev_invalidate_iotlb_timeout(iommu, did, ats_dev);
> >> >> > +}
> >> >> > +
> >> >> > +return rc;
> >> >> > +}
> >> >>
> >> >> I've never really understood why invalidate_sync() returns success
> >> >> when it didn't do anything. Now that you copy this same behavior
> >> >> here, I really need to ask you to explain that.
> >> >>
> >> >
> >> > It is acceptable to me, returning success when it didn't do
> >> > anything
> >> > -- this is worth reflection and criticism:(..
> >> > It is better:
> >> > +if ( qi_ctrl->qinval_maddr )
> >> > +...
> >> > +else
> >> > +rc = -ENOENT;
> >>
> >> Right. And perhaps a separate patch to make invalidate_sync() do the
> same.
> >
> > Agreed.
> >
> >> Question is whether this really ought to be a conditional, or whether
> > instead
> >> this code is unreachable when qinval is not in use, in which case
> >> these conditionals would imo better be converted to ASSERT()s.
> >>
> >
> > IMO, this should be a conditional.
> > As mentioned below, strictly speaking, this is a bug. We can't
> > ASSERT() based on a bug..
> > A coming patch may fix it..
> 
> And again I don't understand: ASSERT()s are to verify assumed state. If static
> code analysis resulted in understanding a function is unreachable when
> qi_ctrl->qinval_maddr is zero (because qinval ought to have got disabled if 
> any
> of the table setup failed), then adding ASSERT() would (a) document that and
> (b) allow to know quickly if something broke that assumption.
> 

You are right.  A separate patch does this.

> But then again I may simply misunderstand your wording: "We can't ASSERT()
> based on a bug" is really pretty unclear to me.
> 

I supposed the variable in ASSERT() is always true, but disable_qinval() needs 
to make
qi_ctrl->qinval_maddr  zero, but today it doesn't do this -- a bug.
With your explanation,  I got it now. Thanks.

Quan








___
Xen-devel mailing list
Xen-devel@lists.xen

Re: [Xen-devel] [PATCH RESEND 05/14] libxl/arm: Construct ACPI GTDT table

[Julien Grall]

Hello Shannon,

On 17/06/16 04:29, Shannon Zhao wrote:

On 2016/6/6 19:40, Stefano Stabellini wrote:

ACPI tables for ARM guests should be user configurable: acpi=1 in the VM
config file enables them, with default off.

While the configuration "acpi" already exists for HVM guest
configuration, do we plan to reuse it or use a new name, e.g. arm_acpi?


We always try to re-use options unless their meaning are completely 
different.


Regards,

--
Julien Grall

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

Re: [Xen-devel] [PATCH RESEND 00/14] Xen ARM DomU ACPI support

[Julien Grall]

Hi Wei,

On 16/06/16 12:20, Wei Liu wrote:

On Tue, Jun 07, 2016 at 12:00:26PM +0100, Julien Grall wrote:
I've read this sub-thread and the other thread in which Boris replied, I
think due to the fact that libxl has more information at hand and libxc
is intended to be simple, I think having the building code in libxl
makes sense.

Shannon, Boris and Julien, what do you guys think? Can we agree on
something so that Shannon can move on with next iteration?


It sounds good to me.

Regards,

--
Julien Grall

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

[Xen-devel] Ping: Re: [PATCH 2/2] x86/HVM: re-order operations in hvm_ud_intercept()

[Jan Beulich]

>>> On 09.06.16 at 17:05,  wrote:
 On 09.06.16 at 16:27,  wrote:
>> On 09/06/16 15:13, Jan Beulich wrote:
>> On 09.06.16 at 16:06,  wrote:
 On 09/06/16 13:31, Jan Beulich wrote:
 On 09.06.16 at 13:34,  wrote:
>> On 08/06/16 14:43, Jan Beulich wrote:
>>> Don't fetch CS explicitly, leverage the fact that hvm_emulate_prepare()
>>> already does (and that hvm_virtual_to_linear_addr() doesn't alter it).
>>>
>>> At once increase the length passed to hvm_virtual_to_linear_addr() by
>>> one: There definitely needs to be at least one more opcode byte, and we
>>> can avoid missing a wraparound case this way.
>>>
>>> Signed-off-by: Jan Beulich 
>> I looked into this when you suggested it, but it latches the wrong eip
>> in the emulation state, and you will end up re-emulating the ud2a
>> instruction, rather than the following instruction.
> Where is there any latching of eip? All hvm_emulate_prepare() does
> is storing the regs pointer.
 Oh - so it does.  I clearly looked over it too quickly.

 What wraparound issue are you referring to?  Adding 1 will cause
 incorrect behaviour when the emulation prefix ends at the segment limit.
>>> I don't think so: The prefix together with the actual instruction
>>> encoding should be viewed as a single instruction, and it crossing
>>> the segment limit should #GP. It wrapping at the prefix/encoding
>>> boundary is the case that I'm specifically referring to (this case
>>> should also #GP, but wouldn't without this adjustment).
>> 
>> But the force emulation prefix specifically doesn't behave like other
>> prefixes.
>> 
>> It doesn't count towards the 15 byte instruction limit, and if the
>> emulated instruction does fault, we want the fault pointing at the
>> emulated instruction, not the force prefix.  We should avoid making any
>> link.
> 
> Well, are you saying placing such a prefix right below the boundary
> of a flat segment is _expected_ to get the instruction at address 0
> emulated? I don't think I could buy that. The patch makes no other
> connection between prefix and actual insn. And #GP because of
> such a boundary condition should imo point at the prefix; only all
> faults associated with the actual insn should point there.

Ping?

Jan


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

[Xen-devel] Ping: [PATCH 0/4] x86/vMSI-X: misc improvements

[Jan Beulich]

>>> On 08.06.16 at 14:48,  wrote:
> 1: defer intercept handler registration
> 2: drop list lock
> 3: drop pci_msix_get_table_len()
> 4: use generic intercept handler in place of MMIO one
> 
> Signed-off-by: Jan Beulich 



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

Re: [Xen-devel] [PATCH 6/7] vm-event/arm: move hvm_event_cr->common vm_event_monitor_cr

[Corneliu ZUZU]

On 6/16/2016 6:16 PM, Jan Beulich wrote:

On 16.06.16 at 16:12,  wrote:

Prepare for ARM implementation of control-register write vm-events by moving
X86-specific hvm_event_cr to the common-side.

Signed-off-by: Corneliu ZUZU 
---
  xen/arch/x86/hvm/event.c| 30 --
  xen/arch/x86/hvm/hvm.c  |  2 +-
  xen/arch/x86/monitor.c  | 37 -
  xen/arch/x86/vm_event.c |  2 +-
  xen/common/monitor.c| 40 
  xen/common/vm_event.c   | 31 +++
  xen/include/asm-x86/hvm/event.h | 13 -
  xen/include/asm-x86/monitor.h   |  2 --
  xen/include/xen/monitor.h   |  4 
  xen/include/xen/vm_event.h  | 10 ++
  10 files changed, 91 insertions(+), 80 deletions(-)

Considering that there's no ARM file getting altered here at all,
mentioning ARM in the subject is a little odd.


This patch and the following one should be meld together.
I only split them to ease reviewing, sorry I forgot to mention that in 
the cover letter.





--- a/xen/common/monitor.c
+++ b/xen/common/monitor.c
@@ -62,6 +62,46 @@ int monitor_domctl(struct domain *d, struct 
xen_domctl_monitor_op *mop)
  
  switch ( mop->event )

  {
+#if CONFIG_X86

#ifdef please.

Ack.

+case XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG:
+{
+struct arch_domain *ad = &d->arch;

Peeking into the next patch I see that this stays there. Common code,
however, shouldn't access ->arch sub-structures - respective fields
should be moved out.


Then we need to find a resolution that avoids code duplication.
The code is the same, but those bits that are currently on the arch side 
(arch.monitor.write_ctrlreg_*) cannot be moved to common as they are, 
since their -number- might differ from arch-to-arch.

But we could:
- in public/vm_event.h, besides the VM_EVENT_X86_* and VM_EVENT_ARM_* 
defines (wcr index), also have

#define VM_EVENT_X86_CR_COUNT4
#define VM_EVENT_ARM_CR_COUNT  4
- move the 3 write_ctrlreg_{enabled,sync,onchangeonly} fields from 
arch_domain to domain (common) and make them 8-bits wide each for now 
(widened more in the future if the need arises)
- let monitor_ctrlreg_bitmask() macro to be architecture-dependent and 
use the introduced VM_EVENT__CR_COUNT


Tamas, we also talked on this matter @ some point (when I sent the 
patches that moved vm-event parts to common). What do you think of the 
above?




And looking at all the uses of this variable I get the impression that
you really want a shorthand for &d->arch.monitor (if any such
helper variable is worthwhile to have here in the first place).


Well, this was a simple cut-paste operation, not very old content aware :)
Personally I prefer the current shorthand (ad) (seems more intuitive and 
is consistent with the other XEN_DOMCTL_MONITOR_EVENT_* cases), but if 
you prefer I'll change that shorthand to am = &d->arch.monitor?



--- a/xen/include/asm-x86/monitor.h
+++ b/xen/include/asm-x86/monitor.h
@@ -24,8 +24,6 @@
  
  #include 
  
-#define monitor_ctrlreg_bitmask(ctrlreg_index) (1U << (ctrlreg_index))

-
  static inline
  int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op 
*mop)
  {
--- a/xen/include/xen/monitor.h
+++ b/xen/include/xen/monitor.h
@@ -25,6 +25,10 @@
  struct domain;
  struct xen_domctl_monitor_op;
  
+#if CONFIG_X86

+#define monitor_ctrlreg_bitmask(ctrlreg_index) (1U << (ctrlreg_index))
+#endif

What's the point in removing this from the x86 header if then it
needs to be put in such a conditional? If the conditional gets
dropped in the next patch, then I think you have two options:
Leave it where it is here, and move it there. Or move it here,
but omit the conditional right away - I can't see this definition
being present to harm the ARM build in any way.


Meld comment above.




--- a/xen/include/xen/vm_event.h
+++ b/xen/include/xen/vm_event.h
@@ -96,6 +96,16 @@ void vm_event_vcpu_unpause(struct vcpu *v);
  int vm_event_monitor_traps(struct vcpu *v, uint8_t sync,
 vm_event_request_t *req);
  
+#if CONFIG_X86

+/*
+ * Called for the current vCPU on control-register changes by guest.
+ * The event might not fire if the client has subscribed to it in onchangeonly
+ * mode, hence the bool_t return type for control register write events.
+ */
+bool_t vm_event_monitor_cr(unsigned int index, unsigned long value,
+   unsigned long old);
+#endif

Same goes for the declaration here.

Jan




Meld comment above.

Corneliu.

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

Re: [Xen-devel] [PATCH 1/7] minor (formatting) fixes

[Corneliu ZUZU]

On 6/16/2016 7:02 PM, Tamas K Lengyel wrote:

diff --git a/xen/include/asm-arm/vm_event.h b/xen/include/asm-arm/vm_event.h
index 014d9ba..05c3027 100644
--- a/xen/include/asm-arm/vm_event.h
+++ b/xen/include/asm-arm/vm_event.h
@@ -23,21 +23,18 @@
  #include 
  #include 

-static inline
-int vm_event_init_domain(struct domain *d)
+static inline int vm_event_init_domain(struct domain *d)
  {
  /* Nothing to do. */
  return 0;
  }

-static inline
-void vm_event_cleanup_domain(struct domain *d)
+static inline void vm_event_cleanup_domain(struct domain *d)
  {
  memset(&d->monitor, 0, sizeof(d->monitor));
  }

-static inline
-void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v)
+static inline void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v)
  {
  /* Not supported on ARM. */
  }
@@ -59,6 +56,9 @@ static inline void vm_event_fill_regs(vm_event_request_t *req)
  /* Not supported on ARM. */
  }

+/*
+ * Monitor vm-events.
+ */

I already have an acked patch that relocates monitor-related functions
from here and the x86 header to the monitor subsystem
(https://patchwork.kernel.org/patch/913/). Generally, I'm trying
to keep monitor-related stuff in the appropriately named files, so if
you encounter things like this in the future the best course of action
is to relocate them. vm_event should be use-case neutral by not having
specific things for the monitor subsystem and just be the i/o system
used for passing messages.


  static inline uint32_t vm_event_monitor_get_capabilities(struct domain *d)
  {
  uint32_t capabilities = 0;


Ah, ok. Didn't that patch make it to staging yet? I pulled the latest.
Since you already have a patch for that I guess it's ok to remove those 
comments and leave the rest as it is and merge later when one of these 
patches makes it to staging?


Corneliu.

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

Re: [Xen-devel] [PATCH 1/7] minor (formatting) fixes

[Razvan Cojocaru]

On 06/17/2016 11:33 AM, Corneliu ZUZU wrote:
> On 6/16/2016 7:02 PM, Tamas K Lengyel wrote:
>>> diff --git a/xen/include/asm-arm/vm_event.h
>>> b/xen/include/asm-arm/vm_event.h
>>> index 014d9ba..05c3027 100644
>>> --- a/xen/include/asm-arm/vm_event.h
>>> +++ b/xen/include/asm-arm/vm_event.h
>>> @@ -23,21 +23,18 @@
>>>   #include 
>>>   #include 
>>>
>>> -static inline
>>> -int vm_event_init_domain(struct domain *d)
>>> +static inline int vm_event_init_domain(struct domain *d)
>>>   {
>>>   /* Nothing to do. */
>>>   return 0;
>>>   }
>>>
>>> -static inline
>>> -void vm_event_cleanup_domain(struct domain *d)
>>> +static inline void vm_event_cleanup_domain(struct domain *d)
>>>   {
>>>   memset(&d->monitor, 0, sizeof(d->monitor));
>>>   }
>>>
>>> -static inline
>>> -void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v)
>>> +static inline void vm_event_toggle_singlestep(struct domain *d,
>>> struct vcpu *v)
>>>   {
>>>   /* Not supported on ARM. */
>>>   }
>>> @@ -59,6 +56,9 @@ static inline void
>>> vm_event_fill_regs(vm_event_request_t *req)
>>>   /* Not supported on ARM. */
>>>   }
>>>
>>> +/*
>>> + * Monitor vm-events.
>>> + */
>> I already have an acked patch that relocates monitor-related functions
>> from here and the x86 header to the monitor subsystem
>> (https://patchwork.kernel.org/patch/913/). Generally, I'm trying
>> to keep monitor-related stuff in the appropriately named files, so if
>> you encounter things like this in the future the best course of action
>> is to relocate them. vm_event should be use-case neutral by not having
>> specific things for the monitor subsystem and just be the i/o system
>> used for passing messages.
>>
>>>   static inline uint32_t vm_event_monitor_get_capabilities(struct
>>> domain *d)
>>>   {
>>>   uint32_t capabilities = 0;
> 
> Ah, ok. Didn't that patch make it to staging yet? I pulled the latest.
> Since you already have a patch for that I guess it's ok to remove those
> comments and leave the rest as it is and merge later when one of these
> patches makes it to staging?

I think there's a delay caused by the 4.7 release. I also have an acked
patch waiting to go in that might cause you to have to rebase (part of)
the series: https://patchwork.kernel.org/patch/9033771/


Thanks,
Razvan

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

Re: [Xen-devel] [PATCH 6/7] vm-event/arm: move hvm_event_cr->common vm_event_monitor_cr

[Jan Beulich]

>>> On 17.06.16 at 10:25,  wrote:
> On 6/16/2016 6:16 PM, Jan Beulich wrote:
>> And looking at all the uses of this variable I get the impression that
>> you really want a shorthand for &d->arch.monitor (if any such
>> helper variable is worthwhile to have here in the first place).
> 
> Well, this was a simple cut-paste operation, not very old content aware :)
> Personally I prefer the current shorthand (ad) (seems more intuitive and 
> is consistent with the other XEN_DOMCTL_MONITOR_EVENT_* cases), but if 
> you prefer I'll change that shorthand to am = &d->arch.monitor?

I'd prefer either no shorthand, or one eliminating the longest common
prefix across all uses.

>>> --- a/xen/include/asm-x86/monitor.h
>>> +++ b/xen/include/asm-x86/monitor.h
>>> @@ -24,8 +24,6 @@
>>>   
>>>   #include 
>>>   
>>> -#define monitor_ctrlreg_bitmask(ctrlreg_index) (1U << (ctrlreg_index))
>>> -
>>>   static inline
>>>   int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op 
>>> *mop)
>>>   {
>>> --- a/xen/include/xen/monitor.h
>>> +++ b/xen/include/xen/monitor.h
>>> @@ -25,6 +25,10 @@
>>>   struct domain;
>>>   struct xen_domctl_monitor_op;
>>>   
>>> +#if CONFIG_X86
>>> +#define monitor_ctrlreg_bitmask(ctrlreg_index) (1U << (ctrlreg_index))
>>> +#endif
>> What's the point in removing this from the x86 header if then it
>> needs to be put in such a conditional? If the conditional gets
>> dropped in the next patch, then I think you have two options:
>> Leave it where it is here, and move it there. Or move it here,
>> but omit the conditional right away - I can't see this definition
>> being present to harm the ARM build in any way.
> 
> Meld comment above.

I don't follow - having split the patches for reviewability was
a good idea. I merely commented on some oddities resulting
from that split, which - afaict - could be easily corrected without
folding the patches together.

Jan


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

Re: [Xen-devel] [Patch v11 3/3] vt-d: fix vt-d Device-TLB flush timeout issue

[Jan Beulich]

>>> On 17.06.16 at 10:15,  wrote:
> On June 17, 2016 3:01 PM, Jan Beulich  wrote:
>> >>> On 17.06.16 at 08:08,  wrote:
>> 
>> > On June 16, 2016 5:05 PM, Jan Beulich  wrote:
>> >> >>> On 16.06.16 at 10:42,  wrote:
>> >> > On June 02, 2016 7:07 PM, Jan Beulich  wrote:
>> >> >> >>> On 01.06.16 at 11:05,  wrote:
>> >> >> > +static void dev_invalidate_iotlb_timeout(struct iommu *iommu, u16
>> did,
>> >> >> > + struct pci_ats_dev
>> >> >> > +*ats_dev) {
>> >> >> > +struct domain *d = NULL;
>> >> >> > +struct pci_dev *pdev;
>> >> >> > +
>> >> >> > +if ( test_bit(did, iommu->domid_bitmap) )
>> >> >> > +d = rcu_lock_domain_by_id(iommu->domid_map[did]);
>> >> >> > +
>> >> >> > +/*
>> >> >> > + * In case the domain has been freed or the IOMMU domid bitmap
>> is
>> >> >> > + * not valid, the device no longer belongs to this domain.
>> >> >> > + */
>> >> >> > +if ( d == NULL )
>> >> >> > +return;
>> >> >> > +
>> >> >> > +pcidevs_lock();
>> >> >> > +
>> >> >> > +for_each_pdev(d, pdev)
>> >> >> > +{
>> >> >> > +if ( (pdev->seg == ats_dev->seg) &&
>> >> >> > + (pdev->bus == ats_dev->bus) &&
>> >> >> > + (pdev->devfn == ats_dev->devfn) )
>> >> >> > +{
>> >> >> > +ASSERT(pdev->domain);
>> >> >> > +list_del(&pdev->domain_list);
>> >> >> > +pdev->domain = NULL;
>> >> >> > +pci_hide_existing_device(pdev);
>> >> >> > +break;
>> >> >> > +}
>> >> >> > +}
>> >> >> > +
>> >> >> > +pcidevs_unlock();
>> >> >>
>> >> >> ... this loop (and locking). (Of course such a change may better
>> >> >> be done in another preparatory patch.)
>> >> >>
>> >> >
>> >> > To eliminate the locking?  I am afraid the locking is still a must
>> >> > here even without the loop, also referring  to device_assigned()..
>> >>
>> >> If the entire loop gets eliminated, what would be left is
>> >>
>> >> pcidevs_lock();
>> >> pcidevs_unlock();
>> >>
>> >> which I don't think does any good.
>> >
>> > Why? I can't follow it..
>> 
>> I don't understand your question. Can you explain what use above code
>> sequence is, in your opinion? Or else - what does the "why"
>> refer to?
>> 
> 
> Ah, there may be a gap between us. without this loop,  these pdev operation 
> should be still there, such as,
> 
> 
> +ASSERT(pdev->domain);
> +list_del(&pdev->domain_list);
> +pdev->domain = NULL;
> +pci_hide_existing_device(pdev);
> 
> So, the left is not only:
>pcidevs_lock();
>pcidevs_unlock();

Oh, indeed. My bad.

Jan


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

Re: [Xen-devel] [PATCH v3 03/16] x86/boot: call reloc() using cdecl calling convention

[Daniel Kiper]

On Fri, Apr 15, 2016 at 04:56:26PM +0100, Andrew Cooper wrote:
> On 15/04/16 13:33, Daniel Kiper wrote:
> > reloc() is not called according to cdecl calling convention.
> > This makes confusion and does not scale well for more arguments.
> > And patch adding multiboot2 protocol support have to pass 3
> > arguments instead of 2. Hence, move reloc() call to cdecl
> > calling convention.
> >
> > I add push %ebp/mov %esp,%ebp/leave instructions here. Though they
> > are not strictly needed in this patch. However, then assembly code
> > in patch adding multiboot2 protocol support is easier to read.
> >
> > Suggested-by: Jan Beulich 
> > Signed-off-by: Daniel Kiper 
> > ---
> > v3 - suggestions/fixes:
> >- simplify assembly in xen/arch/x86/boot/reloc.c file
> >  (suggested by Jan Beulich),
> >- reorder arguments for reloc() call from xen/arch/x86/boot/head.S
> >  (suggested by Jan Beulich),
> >- improve commit message
> >  (suggested by Jan Beulich).
> > ---
> >  xen/arch/x86/boot/head.S  |4 +++-
> >  xen/arch/x86/boot/reloc.c |   18 ++
> >  2 files changed, 17 insertions(+), 5 deletions(-)
> >
> > diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
> > index 32a54a0..28ac721 100644
> > --- a/xen/arch/x86/boot/head.S
> > +++ b/xen/arch/x86/boot/head.S
> > @@ -119,8 +119,10 @@ __start:
> >
> >  /* Save the Multiboot info struct (after relocation) for later 
> > use. */
> >  mov $sym_phys(cpu0_stack)+1024,%esp
> > -push%ebx
> > +push%eax/* Boot trampoline address. */
> > +push%ebx/* Multiboot information address. */
> >  callreloc
> > +add $8,%esp /* Remove reloc() args from stack. */
> >  mov %eax,sym_phys(multiboot_ptr)
> >
> >  /* Initialize BSS (no nasty surprises!). */
> > diff --git a/xen/arch/x86/boot/reloc.c b/xen/arch/x86/boot/reloc.c
> > index 63045c0..006f41d 100644
> > --- a/xen/arch/x86/boot/reloc.c
> > +++ b/xen/arch/x86/boot/reloc.c
> > @@ -10,15 +10,25 @@
> >   *Keir Fraser 
> >   */
> >
> > -/* entered with %eax = BOOT_TRAMPOLINE */
> > +/*
> > + * This entry point is entered from xen/arch/x86/boot/head.S with:
> > + *   - 0x4(%esp) = MULTIBOOT_INFORMATION_ADDRESS,
> > + *   - 0x8(%esp) = BOOT_TRAMPOLINE_ADDRESS.
> > + */
> >  asm (
> >  ".text \n"
> >  ".globl _start \n"
> >  "_start:   \n"
> > +"push %ebp \n"
> > +"mov  %esp,%ebp\n"
> >  "call 1f   \n"
> > -"1:  pop  %ebx \n"
> > -"mov  %eax,alloc-1b(%ebx)  \n"
> > -"jmp  reloc\n"
> > +"1:  pop  %ecx \n"
> > +"mov  0xc(%ebp),%eax   \n"
> > +"mov  %eax,alloc-1b(%ecx)  \n"
> > +"push 0x8(%ebp)\n"
> > +"call reloc\n"
> > +"leave \n"
> > +"ret   \n"
> >  );
> >
> >  /*
>
> Come to think of this, why are we playing asm games like this at all?
>
> This object file gets linked with head.o anyway, and the reloc()
> function is safe to live anywhere in .init.text.  It might be worth

It does not. reloc.c is converted to asm and then included in head.S. However,
as we discussed during hackhaton I tried to link reloc.o directly with other
objects. As we expected it is easy to convert 32-bit ELF to 64-bit ELF file.
Though ld fails. As I saw the main problem is that virtual addresses start at
0x82d08020. This value simply overflows 32-bit relocations, e.g.:

prelink.o: In function `reloc':
(.text+0x359): relocation truncated to fit: R_X86_64_32 against `.text'

There is a chance that we can fix it by changing virtual addresses to
physical addresses. At first sight it should work because final 32-bit
ELF image contains only physical addresses in ELF headers. However, I am
not sure that this way we will not break something. Hmmm... I have just
realized that at least debugging and crash analysis of hypervisor memory
could be more difficult. Am I missing anything else?

Additionally, there is a lack of _GLOBAL_OFFSET_TABLE_ symbol and ld
complains in that way:

prelink.o: In function `reloc':
(.text+0x36c): undefined reference to `_GLOBAL_OFFSET_TABLE_'

Probably we can fix this issue by putting above mentioned symbol somewhere
into xen.lds or something like that.

Andrew, IIRC, you told us that you linked 32-bit code wrapped into 64-bit
ELF file successfully. How did you do that?

Daniel

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

Re: [Xen-devel] [PATCH 2/7] vm-event: VM_EVENT_FLAG_DENY requires VM_EVENT_FLAG_VCPU_PAUSED

[Corneliu ZUZU]

On 6/16/2016 7:11 PM, Tamas K Lengyel wrote:

On Thu, Jun 16, 2016 at 8:07 AM, Corneliu ZUZU  wrote:

For VM_EVENT_FLAG_DENY to work, the vcpu must be paused (sync = 1) until the
vm-event is handled. A vm-event response having VM_EVENT_FLAG_DENY flag set
should also set the VM_EVENT_FLAG_VCPU_PAUSED flag. Enforce that in
vm_event_register_write_resume().

Well, the problem with this is that the user can set the VCPU_PAUSED
flag any time it wants. It can happen that Xen hasn't paused the vCPU
but the user still sends that flag, in which case the unpause the flag
induces will not actually do anything. You should also check if the
vCPU is in fact paused rather then just relying on this flag.

Tamas



Right, I actually meant to take a second look over that.
I see a situation like that was taken into account in 
vm_event_vcpu_unpause (comment "prevent underflow of the vcpu pause count.")

How can I check for vcpu pause here?

Thanks,
Corneliu.

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

Re: [Xen-devel] qemu-traditional build problem for Xen 4.7.0 RC6

[Hao, Xudong]

> -Original Message-
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: Friday, June 17, 2016 3:59 PM
> To: Hao, Xudong 
> Cc: Wei Liu ; ian.jack...@eu.citrix.com; xen-
> de...@lists.xenproject.org
> Subject: Re: [Xen-devel] qemu-traditional build problem for Xen 4.7.0 RC6
> 
> >>> On 17.06.16 at 05:11,  wrote:
> > QEMU_TAG update to 698d6d6f8d095edadb0c23612b552a89dd3eee4c of
> > traditional qemu in Config.mk, this commit is a branch stable-4.7
> > commit for qemu traditional. The master commit is
> 6e20809727261599e8527c456eb078c0e89139a1.
> > It bring up a build error.
> 
> It would help quite a bit if you also said what kind of build error you 
> observe.
> 

Actually it's not compile error, it's a repo clone issue. Error log is:

...
Initialized empty Git repository in 
/home/www/builds_xen_unstable/xen-src-xen-4.7.0-rc6-20160616/tools/qemu-xen-traditional-dir-remote.tmp/.git/
fatal: reference is not a tree: 698d6d6f8d095edadb0c23612b552a89dd3eee4c
make[2]: *** [qemu-xen-traditional-dir-find] Error 128
...

The issue get root caused and resolved now. 
The failure is that we set up a local repo mirror for qemu-xen-traditional, but 
the mirror only sync "master" branch, not include "stable-4.7" branch, so 
commit 698d6d6* didn't be identified when doing checkout. 

Thanks,
-Xudong

> Jan
> 
> > commit 03c716e32912e288929a6e0d9c8729d208462d66
> > Author: Ian Jackson 
> > Date:   Fri Jun 10 11:49:24 2016 +0100
> >
> > QEMU_TAG update
> >
> > diff --git a/Config.mk b/Config.mk
> > index bc5c456..1d6522c 100644
> > --- a/Config.mk
> > +++ b/Config.mk
> > @@ -284,9 +284,9 @@ SEABIOS_UPSTREAM_REVISION ?= rel-1.9.2
> > ETHERBOOT_NICS ?= rtl8139 8086100e
> >
> >
> > -QEMU_TRADITIONAL_REVISION ?=
> df553c056104e3dd8a2bd2e72539a57c4c085bae
> > -# Thu May 5 11:14:44 2016 +0100
> > -# Fix build with newer version of GNUTLS
> > +QEMU_TRADITIONAL_REVISION ?=
> 698d6d6f8d095edadb0c23612b552a89dd3eee4c
> > +# Thu May 19 19:38:35 2016 +0100
> > +# main loop: Big hammer to fix logfile disk DoS in Xen setups
> >
> >  # Specify which qemu-dm to use. This may be `ioemu' to use the old  #
> > Mercurial in-tree version, or a local directory, or a git URL.
> >
> > Best Regards,
> > Xudong
> >
> >
> > ___
> > 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] [Patch v11 3/3] vt-d: fix vt-d Device-TLB flush timeout issue (+ crash logic )

[Xu, Quan]

+ arm/amd maintainers..

On June 01, 2016 5:05 PM, Xu, Quan  wrote:
> If Device-TLB flush timed out, we hide the target ATS device immediately and
> crash the domain owning this ATS device. If impacted domain is hardware
> domain, just throw out a warning.
> 
> By hiding the device, we make sure it can't be assigned to any domain any
> longer (see device_assigned).

DomU (other than Dom0) gets crashed when a device IOTLB flush times out. I 
suppose that's what you will want on ARM/AMD then too.
We need to move up the crash logic , as similar as iommu_map_page() / 
iommu_unmap_page().

- add the crash logic to iommu_iotlb_flush() / iommu_iotlb_flush_all().

- when IOMMU/MMU share page tables, we need to fix it one by one.
-- on amd, we need to add the crash logic to amd_iommu_flush_pages().
-- on intel, we need to add the crash logic to iommu_pte_flush().
-- on arm, we benefit that we add the crash logic to 
iommu_iotlb_flush().


Taken together, we need to add crash logic to
 iommu_iotlb_flush() / iommu_iotlb_flush_all() / 
iommu_pte_flush() / amd_iommu_flush_pages().

Thoughts?

Quan

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

Re: [Xen-devel] [PATCH 3/7] vm-event: introduce vm_event_vcpu_enter

[Julien Grall]

Hello,

On 16/06/16 15:08, Corneliu ZUZU wrote:

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index d31f821..ba248c8 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -19,6 +19,7 @@
  #include 
  #include 
  #include 
+#include 

  #include 
  #include 
@@ -251,6 +252,8 @@ static void schedule_tail(struct vcpu *prev)

  ctxt_switch_to(current);

+vm_event_vcpu_enter(current);
+
  local_irq_enable();

  context_saved(prev);
@@ -296,7 +299,7 @@ void context_switch(struct vcpu *prev, struct vcpu *next)

  void continue_running(struct vcpu *same)
  {
-/* Nothing to do */
+vm_event_vcpu_enter(same);
  }

  void sync_local_execstate(void)


From my understanding of the commit message, vm_event_vcpu_enter should 
be called before returning to the guest. The scheduling functions are 
not called every-time Xen is returning to the guest. So if you want to 
do every time Xen is re-entering to the guest, then this should be done 
in leave_hypervisor_tail.


Regards,

--
Julien Grall

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

[Xen-devel] [qemu-upstream-4.3-testing test] 95831: regressions - trouble: blocked/broken/fail/pass

[osstest service owner]

flight 95831 qemu-upstream-4.3-testing real [real]
http://logs.test-lab.xenproject.org/osstest/logs/95831/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 build-amd64-libvirt   5 libvirt-build fail REGR. vs. 80927
 build-i386-libvirt5 libvirt-build fail REGR. vs. 80927

Tests which are failing intermittently (not blocking):
 test-amd64-i386-xl-raw3 host-install(3)  broken in 95788 pass in 95831
 test-amd64-i386-freebsd10-i386 3 host-install(3) broken in 95788 pass in 95831
 test-amd64-i386-xl3 host-install(3)   broken pass in 95788
 test-amd64-amd64-pygrub   3 host-install(3)   broken pass in 95788
 test-amd64-amd64-amd64-pvgrub  3 host-install(3)  broken pass in 95788

Regressions which are regarded as allowable (not blocking):
 test-amd64-i386-xl-qemuu-win7-amd64 16 guest-stop  fail like 80927
 test-amd64-amd64-xl-qemuu-win7-amd64 16 guest-stop fail like 80927

Tests which did not succeed, but are not blocking:
 test-amd64-i386-libvirt   1 build-check(1)   blocked  n/a
 test-amd64-amd64-libvirt-vhd  1 build-check(1)   blocked  n/a
 test-amd64-amd64-libvirt  1 build-check(1)   blocked  n/a
 test-amd64-amd64-xl-qemuu-ovmf-amd64  9 debian-hvm-install fail never pass
 test-amd64-i386-xl-qemuu-ovmf-amd64  9 debian-hvm-install  fail never pass

version targeted for testing:
 qemuu12e8fccf5b5460be7aecddc71d27eceaba6e1f15
baseline version:
 qemuu10c1b763c26feb645627a1639e722515f3e1e876

Last test of basis80927  2016-02-06 13:30:02 Z  131 days
Failing since 93977  2016-05-10 11:09:16 Z   37 days  126 attempts
Testing same since95534  2016-06-11 00:59:46 Z6 days6 attempts


People who touched revisions under test:
  Anthony PERARD 
  Gerd Hoffmann 
  Ian Jackson 
  Stefano Stabellini 
  Wei Liu 

jobs:
 build-amd64  pass
 build-i386   pass
 build-amd64-libvirt  fail
 build-i386-libvirt   fail
 build-amd64-pvopspass
 build-i386-pvops pass
 test-amd64-amd64-xl  pass
 test-amd64-i386-xl   broken  
 test-amd64-i386-qemuu-rhel6hvm-amd   pass
 test-amd64-amd64-xl-qemuu-debianhvm-amd64pass
 test-amd64-i386-xl-qemuu-debianhvm-amd64 pass
 test-amd64-i386-freebsd10-amd64  pass
 test-amd64-amd64-xl-qemuu-ovmf-amd64 fail
 test-amd64-i386-xl-qemuu-ovmf-amd64  fail
 test-amd64-amd64-xl-qemuu-win7-amd64 fail
 test-amd64-i386-xl-qemuu-win7-amd64  fail
 test-amd64-amd64-xl-credit2  pass
 test-amd64-i386-freebsd10-i386   pass
 test-amd64-i386-qemuu-rhel6hvm-intel pass
 test-amd64-amd64-libvirt blocked 
 test-amd64-i386-libvirt  blocked 
 test-amd64-amd64-xl-multivcpupass
 test-amd64-amd64-pairpass
 test-amd64-i386-pair pass
 test-amd64-amd64-pv  pass
 test-amd64-i386-pv   pass
 test-amd64-amd64-amd64-pvgrubbroken  
 test-amd64-amd64-i386-pvgrub pass
 test-amd64-amd64-pygrub  broken  
 test-amd64-amd64-xl-qcow2pass
 test-amd64-i386-xl-raw   pass
 test-amd64-i386-xl-qemuu-winxpsp3-vcpus1 pass
 test-amd64-amd64-libvirt-vhd blocked 
 test-amd64-amd64-xl-qemuu-winxpsp3   pass



sg-report-flight on osstest.test-lab.xenproject.org
logs: /home/logs/logs
images: /home/logs/images

Logs, config files, etc. are available at
http://logs.test-lab.xenproject.org/osstest/logs

Explanation of these reports, and of osstest in general, is at
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master

Test harness code can be found at
ht

Re: [Xen-devel] [PATCH v3 07/16] x86/boot: create *.lnk files with linker script

[Daniel Kiper]

On Tue, May 24, 2016 at 06:52:39AM -0600, Jan Beulich wrote:
> >>> On 24.05.16 at 14:28,  wrote:
> > On Tue, May 24, 2016 at 03:05:06AM -0600, Jan Beulich wrote:
> >> >>> On 15.04.16 at 14:33,  wrote:
> >> > --- /dev/null
> >> > +++ b/xen/arch/x86/boot/build32.lds
> >> > @@ -0,0 +1,49 @@
> >> > +/*
> >> > + * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
> >> > + *  Daniel Kiper 
> >> > + *
> >> > + * This program is free software; you can redistribute it and/or modify
> >> > + * it under the terms of the GNU General Public License as published by
> >> > + * the Free Software Foundation; either version 2 of the License, or
> >> > + * (at your option) any later version.
> >> > + *
> >> > + * This program is distributed in the hope that it will be useful,
> >> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >> > + * GNU General Public License for more details.
> >> > + *
> >> > + * You should have received a copy of the GNU General Public License 
> >> > along
> >> > + * with this program.  If not, see .
> >> > + */
> >> > +
> >> > +ENTRY(_start)
> >> > +
> >> > +SECTIONS
> >> > +{
> >> > +  /* Merge code and data into one section. */
> >> > +  .text : {
> >> > +*(.text)
> >> > +*(.text.*)
> >> > +*(.rodata)
> >>
> >> This (and the respective %.lnk rule change below) is not in line with
> >> the patch description. It's further suspicious that you only handle
> >
> > I am not sure what exactly do you mean by that.
>
> Quoting your commit message: "...  merge all text and data sections
> into one .text section." Contrast this to the limited set of sections
> above.
>
> >> .rodata but not also .rodata.* here.
> >
> > I did this deliberately. I just want to take only these sections which I
> > know that
> > contain required code and data. Nothing more. If in the future we find out
> > that
> > .rodata.* (or anything else) is needed then we can add it later.
> >
> >> > +  }
> >> > +
> >> > +  /DISCARD/ : {
> >> > +/*
> >> > + * .got.plt section is used only by dynamic linker
> >> > + * and our output is not supposed to be loaded by
> >> > + * dynamic linker. Additionally, it just contains
> >> > + * .PLT0 which is referenced from nowhere. So, we
> >> > + * can safely drop .got.plt here.
> >> > + *
> >> > + * Ha! This should be really discarded here. However,
> >> > + * .got.plt section contains _GLOBAL_OFFSET_TABLE_
> >> > + * symbol too and it is used as a reference for relative
> >> > + * addressing (and only for that thing). Hence, ld
> >> > + * complains if we remove that section because it
> >> > + * cannot find _GLOBAL_OFFSET_TABLE_. So, drop .got.plt
> >> > + * section during conversion to plain binary format.
> >> > + * Please check build32.mk for more details.
> >> > + */
> >> > +/* *(.got.plt) */
> >> > +  }
> >>
> >> I'm afraid this needs more investigation: Afaik there should be no
> >
> > I am not sure what else we should look for.
>
> The reason why such an empty .got.plt gets created in the first place.
> If e.g. that turns out to be a bug in (some versions of) binutils, then
> that bug should be named here as the reason.

If PIC/PIE code is build then .got.plt exists in executable even if it
is not linked with dynamic libraries. Then it is just placeholder for
_GLOBAL_OFFSET_TABLE_ symbol and .PLT0. .PLT0 is filled by dynamic
linker and our code is not supposed to be loaded by dynamic linker.
So, from our point of view .PLT0 is unused.

> >> reason for the linker to create an otherwise empty .got.plt in the
> >
> > As I wrote above. It contains _GLOBAL_OFFSET_TABLE_ which is used
> > as a reference for relative addressing.
>
> But we don't use any such, so without being needed I don't think
> the symbol needs to be created.

R_386_GOTPC and R_386_GOTOFF relocations use address of _GLOBAL_OFFSET_TABLE_
as a reference. So, it is needed during linking phase. However, later it is
not needed.  Hence, .got.plt with _GLOBAL_OFFSET_TABLE_ and .PLT0 can safely
be dropped.

> >> first place. And discarding it without being sure it is empty is not
> >> that good an idea anyway.
> >
> > Good point! Potentially we can check is it empty, excluding
> > _GLOBAL_OFFSET_TABLE_ symbol, in build32.mk.
>
> Well, your comment above says it have .PLT0, which means it's not
> exactly empty.

Ditto. And right know I do not think that we need any safety checks here.

Daniel

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

[Xen-devel] [PATCH v2] xen: fix qdisk BLKIF_OP_DISCARD for 32/64 word size mix

[Juergen Gross]

In case the word size of the domU and qemu running the qdisk backend
differ BLKIF_OP_DISCARD will not work reliably, as the request
structure in the ring have different layouts for different word size.

Correct this by copying the request structure in case of different
word size element by element in the BLKIF_OP_DISCARD case, too.

The easiest way to achieve this is to resync hw/block/xen_blkif.h with
its original source from the Linux kernel.

Signed-off-by: Juergen Gross 
---
V2: resync with Linux kernel version of hw/block/xen_blkif.h as
suggested by Paul Durrant
---
 hw/block/xen_blkif.h | 379 ---
 hw/block/xen_disk.c  |   1 +
 2 files changed, 304 insertions(+), 76 deletions(-)

diff --git a/hw/block/xen_blkif.h b/hw/block/xen_blkif.h
index e3b133b..7b31d87 100644
--- a/hw/block/xen_blkif.h
+++ b/hw/block/xen_blkif.h
@@ -1,3 +1,29 @@
+/*
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License version 2
+ * as published by the Free Software Foundation; or, when distributed
+ * separately from the Linux kernel or incorporated into other
+ * software packages, subject to the following license:
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this source file (the "Software"), to deal in the Software without
+ * restriction, including without limitation the rights to use, copy, modify,
+ * merge, publish, distribute, sublicense, and/or sell copies of the Software,
+ * and to permit persons to whom the Software is furnished to do so, subject to
+ * the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
 #ifndef __XEN_BLKIF_H__
 #define __XEN_BLKIF_H__
 
@@ -5,115 +31,316 @@
 #include 
 #include 
 
+/*
+ * This is the maximum number of segments that would be allowed in indirect
+ * requests. This value will also be passed to the frontend.
+ */
+#define MAX_INDIRECT_SEGMENTS 256
+
+/*
+ * Xen use 4K pages. The guest may use different page size (4K or 64K)
+ * Number of Xen pages per segment
+ */
+#define XEN_PAGES_PER_SEGMENT   (PAGE_SIZE / XC_PAGE_SIZE)
+
+#define XEN_PAGES_PER_INDIRECT_FRAME \
+(XC_PAGE_SIZE / sizeof(struct blkif_request_segment))
+#define SEGS_PER_INDIRECT_FRAME  \
+(XEN_PAGES_PER_INDIRECT_FRAME / XEN_PAGES_PER_SEGMENT)
+
+#define MAX_INDIRECT_PAGES   \
+((MAX_INDIRECT_SEGMENTS + SEGS_PER_INDIRECT_FRAME - 1) /\
+ SEGS_PER_INDIRECT_FRAME)
+#define INDIRECT_PAGES(_segs) DIV_ROUND_UP(_segs, XEN_PAGES_PER_INDIRECT_FRAME)
+
 /* Not a real protocol.  Used to generate ring structs which contain
  * the elements common to all protocols only.  This way we get a
  * compiler-checkable way to use common struct elements, so we can
  * avoid using switch(protocol) in a number of places.  */
 struct blkif_common_request {
-   char dummy;
+char dummy;
 };
 struct blkif_common_response {
-   char dummy;
+char dummy;
 };
 
+struct blkif_x86_32_request_rw {
+uint8_tnr_segments;  /* number of segments   */
+blkif_vdev_t   handle;   /* only for read/write requests */
+uint64_t   id;   /* private guest value, echoed in resp  */
+blkif_sector_t sector_number;/* start sector idx on disk (r/w only)  */
+struct blkif_request_segment seg[BLKIF_MAX_SEGMENTS_PER_REQUEST];
+} __attribute__((__packed__));
+
+struct blkif_x86_32_request_discard {
+uint8_tflag; /* BLKIF_DISCARD_SECURE or zero */
+blkif_vdev_t   _pad1;/* was "handle" for read/write requests */
+uint64_t   id;   /* private guest value, echoed in resp  */
+blkif_sector_t sector_number;/* start sector idx on disk (r/w only)  */
+uint64_t   nr_sectors;
+} __attribute__((__packed__));
+
+struct blkif_x86_32_request_other {
+uint8_t_pad1;
+blkif_vdev_t   _pad2;
+uint64_t   id;   /* private guest value, echoed in resp  */
+} __attribute__((__packed__));
+
+struct blkif_x86_32_request_indirect {
+uint8_tindirect_op;
+uint16_t   nr_segments;
+uint64_t   id;
+blkif_sector_t sector_number;
+blkif_vdev_t   handle;
+uint16_t   _pad1;
+grant_ref_tindirect_grefs[BLKIF_MAX_INDIRECT_PAGES_PER_REQUEST];
+/*
+ * The maximum number of ind

Re: [Xen-devel] [PATCH RESEND 05/14] libxl/arm: Construct ACPI GTDT table

[Stefano Stabellini]

On Fri, 17 Jun 2016, Julien Grall wrote:
> Hello Shannon,
> 
> On 17/06/16 04:29, Shannon Zhao wrote:
> > On 2016/6/6 19:40, Stefano Stabellini wrote:
> > > ACPI tables for ARM guests should be user configurable: acpi=1 in the VM
> > > config file enables them, with default off.
> > While the configuration "acpi" already exists for HVM guest
> > configuration, do we plan to reuse it or use a new name, e.g. arm_acpi?
> 
> We always try to re-use options unless their meaning are completely different.
 
I agree. "acpi" is a good name for it. I would avoid introducing
something like "arm_acpi".

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

Re: [Xen-devel] [PATCH 3/7] vm-event: introduce vm_event_vcpu_enter

[Corneliu ZUZU]

On 6/16/2016 7:17 PM, Tamas K Lengyel wrote:

On Thu, Jun 16, 2016 at 8:08 AM, Corneliu ZUZU  wrote:

In an effort to improve on the vm-event interface, we introduce a new function
called vm_event_vcpu_enter. Its significance is that of a "final touch" vCPU
function - i.e. it should be called by implementing architectures just before
re-entering vCPUs.
On X86 for example, it is called on the scheduling tail (hvm_do_resume) and just
before reentering the guest world after a hypervisor trap (vmx_vmenter_helper).


I don't think this belongs to the vm_event system. It should probably
be in the monitor subsystem as it has nothing to do with passing
messages, which is essentially what vm_event is for.

Tamas



I figured it's usage is Xen-internal and it's meant to be of eventual 
use - whenever the need arises to do something just before a vCPU is 
reentered from the vm-event subsystem, you can simply adjust the 
implementation of that function.
I thought non-monitor vm-events could also be 'eventual users'. But then 
again by this reasoning I could broaden its significance even more, 
outside the vm-event world and make it 'usable' by any parts of code in Xen.
Shall I then rename it to vm_event_monitor_vcpu_enter and put it 
alongside the other vm_event_monitor_* functions?


Corneliu.

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

Re: [Xen-devel] [PATCH 4/7] vm-event/x86: use vm_event_vcpu_enter properly

[Corneliu ZUZU]

On 6/16/2016 7:27 PM, Tamas K Lengyel wrote:

diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
index 1fec412..1e5445f 100644
--- a/xen/arch/x86/monitor.c
+++ b/xen/arch/x86/monitor.c
@@ -20,7 +20,6 @@
   */

  #include 
-#include 

  int arch_monitor_domctl_event(struct domain *d,
struct xen_domctl_monitor_op *mop)
@@ -62,14 +61,6 @@ int arch_monitor_domctl_event(struct domain *d,
  else
  ad->monitor.write_ctrlreg_enabled &= ~ctrlreg_bitmask;

-if ( VM_EVENT_X86_CR3 == mop->u.mov_to_cr.index )
-{
-struct vcpu *v;
-/* Latches new CR3 mask through CR0 code. */
-for_each_vcpu ( d, v )
-hvm_update_guest_cr(v, 0);
-}
-

So this block is not really getting relocated as the commit message
suggests as much as being completely reworked at a different location?
It would be better for it to be it's own separate patch as the changes
are not trivial.


That's actually not reworked, it's completely removed since there's no 
need for it anymore.
That is: "latching of CR3 mask" is not done "through CR0" anymore but 
rather through the vm_event_vcpu_enter function instead and you don't 
have to do anything more here in arch_monitor_domctl_event for that to 
happen.



  domain_unpause(d);

  break;

Thanks,
Tamas





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

Re: [Xen-devel] [PATCH v2] xen: fix qdisk BLKIF_OP_DISCARD for 32/64 word size mix

[Jan Beulich]

>>> On 17.06.16 at 11:14,  wrote:
> In case the word size of the domU and qemu running the qdisk backend
> differ BLKIF_OP_DISCARD will not work reliably, as the request
> structure in the ring have different layouts for different word size.
> 
> Correct this by copying the request structure in case of different
> word size element by element in the BLKIF_OP_DISCARD case, too.
> 
> The easiest way to achieve this is to resync hw/block/xen_blkif.h with
> its original source from the Linux kernel.
> 
> Signed-off-by: Juergen Gross 
> ---
> V2: resync with Linux kernel version of hw/block/xen_blkif.h as
> suggested by Paul Durrant

Oh, I didn't realize he suggested syncing with the Linux variant.
Why not with the canonical one? I have to admit that I particularly
dislike Linux'es strange union-izng, mainly because of it requiring
this myriad of __attribute__((__packed__)).

Jan


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

Re: [Xen-devel] [PATCH 1/7] minor (formatting) fixes

[Andrew Cooper]

On 17/06/16 09:36, Razvan Cojocaru wrote:
> On 06/17/2016 11:33 AM, Corneliu ZUZU wrote:
>> On 6/16/2016 7:02 PM, Tamas K Lengyel wrote:
 diff --git a/xen/include/asm-arm/vm_event.h
 b/xen/include/asm-arm/vm_event.h
 index 014d9ba..05c3027 100644
 --- a/xen/include/asm-arm/vm_event.h
 +++ b/xen/include/asm-arm/vm_event.h
 @@ -23,21 +23,18 @@
   #include 
   #include 

 -static inline
 -int vm_event_init_domain(struct domain *d)
 +static inline int vm_event_init_domain(struct domain *d)
   {
   /* Nothing to do. */
   return 0;
   }

 -static inline
 -void vm_event_cleanup_domain(struct domain *d)
 +static inline void vm_event_cleanup_domain(struct domain *d)
   {
   memset(&d->monitor, 0, sizeof(d->monitor));
   }

 -static inline
 -void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v)
 +static inline void vm_event_toggle_singlestep(struct domain *d,
 struct vcpu *v)
   {
   /* Not supported on ARM. */
   }
 @@ -59,6 +56,9 @@ static inline void
 vm_event_fill_regs(vm_event_request_t *req)
   /* Not supported on ARM. */
   }

 +/*
 + * Monitor vm-events.
 + */
>>> I already have an acked patch that relocates monitor-related functions
>>> from here and the x86 header to the monitor subsystem
>>> (https://patchwork.kernel.org/patch/913/). Generally, I'm trying
>>> to keep monitor-related stuff in the appropriately named files, so if
>>> you encounter things like this in the future the best course of action
>>> is to relocate them. vm_event should be use-case neutral by not having
>>> specific things for the monitor subsystem and just be the i/o system
>>> used for passing messages.
>>>
   static inline uint32_t vm_event_monitor_get_capabilities(struct
 domain *d)
   {
   uint32_t capabilities = 0;
>> Ah, ok. Didn't that patch make it to staging yet? I pulled the latest.
>> Since you already have a patch for that I guess it's ok to remove those
>> comments and leave the rest as it is and merge later when one of these
>> patches makes it to staging?
> I think there's a delay caused by the 4.7 release. I also have an acked
> patch waiting to go in that might cause you to have to rebase (part of)
> the series: https://patchwork.kernel.org/patch/9033771/

Staging is now open for Xen 4.8 submissions.

Would it be possible for previous submissions to be resent, or a summary
email pointing at the intended versions of the patches?  I will see
about getting stuff in.

~Andrew

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

Re: [Xen-devel] [PATCH v3 03/16] x86/boot: call reloc() using cdecl calling convention

[Jan Beulich]

>>> On 17.06.16 at 10:41,  wrote:
> On Fri, Apr 15, 2016 at 04:56:26PM +0100, Andrew Cooper wrote:
>> On 15/04/16 13:33, Daniel Kiper wrote:
>> > reloc() is not called according to cdecl calling convention.
>> > This makes confusion and does not scale well for more arguments.
>> > And patch adding multiboot2 protocol support have to pass 3
>> > arguments instead of 2. Hence, move reloc() call to cdecl
>> > calling convention.
>> >
>> > I add push %ebp/mov %esp,%ebp/leave instructions here. Though they
>> > are not strictly needed in this patch. However, then assembly code
>> > in patch adding multiboot2 protocol support is easier to read.
>> >
>> > Suggested-by: Jan Beulich 
>> > Signed-off-by: Daniel Kiper 
>> > ---
>> > v3 - suggestions/fixes:
>> >- simplify assembly in xen/arch/x86/boot/reloc.c file
>> >  (suggested by Jan Beulich),
>> >- reorder arguments for reloc() call from xen/arch/x86/boot/head.S
>> >  (suggested by Jan Beulich),
>> >- improve commit message
>> >  (suggested by Jan Beulich).
>> > ---
>> >  xen/arch/x86/boot/head.S  |4 +++-
>> >  xen/arch/x86/boot/reloc.c |   18 ++
>> >  2 files changed, 17 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
>> > index 32a54a0..28ac721 100644
>> > --- a/xen/arch/x86/boot/head.S
>> > +++ b/xen/arch/x86/boot/head.S
>> > @@ -119,8 +119,10 @@ __start:
>> >
>> >  /* Save the Multiboot info struct (after relocation) for later 
> use. */
>> >  mov $sym_phys(cpu0_stack)+1024,%esp
>> > -push%ebx
>> > +push%eax/* Boot trampoline address. */
>> > +push%ebx/* Multiboot information address. */
>> >  callreloc
>> > +add $8,%esp /* Remove reloc() args from stack. */
>> >  mov %eax,sym_phys(multiboot_ptr)
>> >
>> >  /* Initialize BSS (no nasty surprises!). */
>> > diff --git a/xen/arch/x86/boot/reloc.c b/xen/arch/x86/boot/reloc.c
>> > index 63045c0..006f41d 100644
>> > --- a/xen/arch/x86/boot/reloc.c
>> > +++ b/xen/arch/x86/boot/reloc.c
>> > @@ -10,15 +10,25 @@
>> >   *Keir Fraser 
>> >   */
>> >
>> > -/* entered with %eax = BOOT_TRAMPOLINE */
>> > +/*
>> > + * This entry point is entered from xen/arch/x86/boot/head.S with:
>> > + *   - 0x4(%esp) = MULTIBOOT_INFORMATION_ADDRESS,
>> > + *   - 0x8(%esp) = BOOT_TRAMPOLINE_ADDRESS.
>> > + */
>> >  asm (
>> >  ".text \n"
>> >  ".globl _start \n"
>> >  "_start:   \n"
>> > +"push %ebp \n"
>> > +"mov  %esp,%ebp\n"
>> >  "call 1f   \n"
>> > -"1:  pop  %ebx \n"
>> > -"mov  %eax,alloc-1b(%ebx)  \n"
>> > -"jmp  reloc\n"
>> > +"1:  pop  %ecx \n"
>> > +"mov  0xc(%ebp),%eax   \n"
>> > +"mov  %eax,alloc-1b(%ecx)  \n"
>> > +"push 0x8(%ebp)\n"
>> > +"call reloc\n"
>> > +"leave \n"
>> > +"ret   \n"
>> >  );
>> >
>> >  /*
>>
>> Come to think of this, why are we playing asm games like this at all?
>>
>> This object file gets linked with head.o anyway, and the reloc()
>> function is safe to live anywhere in .init.text.  It might be worth
> 
> It does not. reloc.c is converted to asm and then included in head.S. 
> However,
> as we discussed during hackhaton I tried to link reloc.o directly with other
> objects. As we expected it is easy to convert 32-bit ELF to 64-bit ELF file.
> Though ld fails. As I saw the main problem is that virtual addresses start at
> 0x82d08020. This value simply overflows 32-bit relocations, e.g.:
> 
> prelink.o: In function `reloc':
> (.text+0x359): relocation truncated to fit: R_X86_64_32 against `.text'
> 
> There is a chance that we can fix it by changing virtual addresses to
> physical addresses. At first sight it should work because final 32-bit
> ELF image contains only physical addresses in ELF headers. However, I am
> not sure that this way we will not break something. Hmmm... I have just
> realized that at least debugging and crash analysis of hypervisor memory
> could be more difficult. Am I missing anything else?

I don't think you can switch the linking process to use physical
addresses: The final image has to have proper virtual addresses
used for any involved relocations. This would only be benign if
_all_ relocations were relative ones.

Jan

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

Re: [Xen-devel] [PATCH v2] xen: fix qdisk BLKIF_OP_DISCARD for 32/64 word size mix

[Juergen Gross]

On 17/06/16 11:26, Jan Beulich wrote:
 On 17.06.16 at 11:14,  wrote:
>> In case the word size of the domU and qemu running the qdisk backend
>> differ BLKIF_OP_DISCARD will not work reliably, as the request
>> structure in the ring have different layouts for different word size.
>>
>> Correct this by copying the request structure in case of different
>> word size element by element in the BLKIF_OP_DISCARD case, too.
>>
>> The easiest way to achieve this is to resync hw/block/xen_blkif.h with
>> its original source from the Linux kernel.
>>
>> Signed-off-by: Juergen Gross 
>> ---
>> V2: resync with Linux kernel version of hw/block/xen_blkif.h as
>> suggested by Paul Durrant
> 
> Oh, I didn't realize he suggested syncing with the Linux variant.
> Why not with the canonical one? I have to admit that I particularly
> dislike Linux'es strange union-izng, mainly because of it requiring
> this myriad of __attribute__((__packed__)).

What would be gained by syncing with the canonical one? The part to be
modified is available in the Linux variant only.


Juergen


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

[Xen-devel] Xen 4.1 maintenance? (Was Re: [xen-4.1.6.1] SIGSEGV libxc/xc_save_domain.c: p2m_size >> configured_ram_size)

[George Dunlap]

On 13/06/16 12:14, Philipp Hahn wrote:
> Am 13.06.2016 um 12:15 schrieb George Dunlap:
>> On Fri, Jun 10, 2016 at 4:22 PM, Philipp Hahn  wrote:
>>> while trying to live migrate some VMs from an xen-4.1.6.1 host "xc_save"
>>> crashes with a segmentation fault in tools/libxc/xc_domain_save.c:1141
 /*
  * Quick belt and braces sanity check.
  */
 for ( i = 0; i < dinfo->p2m_size; i++ )
 {
 mfn = pfn_to_mfn(i);
 if( (mfn != INVALID_P2M_ENTRY) && (mfn_to_pfn(mfn) != i) )
>>>  ^^^
>>> due to a de-reference through
 #define pfn_to_mfn(_pfn)\
   ((xen_pfn_t) ((dinfo->guest_width==8)   \
 ? (((uint64_t *)ctx->live_p2m)[(_pfn)])  \
 : uint32_t *)ctx->live_p2m)[(_pfn)]) == 0xU  \
? (-1UL) : (((uint32_t *)ctx->live_p2m)[(_pfn)]
> ...
>> Given that 4.1 is long out of support, we won't be making a proper fix
>> in-tree (since it will never be released).
> 
> I know that 4.1 is EOL.
> I'm aware of Ubuntu still having xen-4.1 in one of their LTS versions
> (Precise) and its also in Debian-oldstable, which a lot people (us
> included) still use. I would prefer to update, but I can for reasons
> outside my direct control.
> 
> I'm already working with Stefan Bader from Canonical to backport most of
> the XSAs to 4.1, so there already exists a "better" version outside of
> the official Xen repositories.

Philipp / Stefan -- if there really is a large following of people still
using 4.1, would it make sense to have one or both of you step up and
maintain an official branch on xenbits?

 -George

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

Re: [Xen-devel] [PATCH 1/7] minor (formatting) fixes

[Jan Beulich]

>>> On 17.06.16 at 10:36,  wrote:
> On 06/17/2016 11:33 AM, Corneliu ZUZU wrote:
>> Ah, ok. Didn't that patch make it to staging yet? I pulled the latest.
>> Since you already have a patch for that I guess it's ok to remove those
>> comments and leave the rest as it is and merge later when one of these
>> patches makes it to staging?
> 
> I think there's a delay caused by the 4.7 release. I also have an acked
> patch waiting to go in that might cause you to have to rebase (part of)
> the series: https://patchwork.kernel.org/patch/9033771/ 

Well, there's a slight difference here: Yours is indeed delayed until
4.7 is set in stone, for being more intrusive. Tamas'es, otoh, I would
probably have put in already if it had all necessary acks (or they
had at least been pinged for, at which point we could waive the need
for some trivial adjustments).

Jan


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

Re: [Xen-devel] [PATCH 1/7] minor (formatting) fixes

[Jan Beulich]

>>> On 17.06.16 at 11:29,  wrote:
> Staging is now open for Xen 4.8 submissions.

Just to clarify - not fully. Intrusive changes (with a higher risk of
causing backporting conflicts for eventual changes still wanting to
go into 4.7) were agreed to not get put in yet.

Jan


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

Re: [Xen-devel] [PATCH 1/7] minor (formatting) fixes

[Razvan Cojocaru]

On 06/17/2016 12:33 PM, Jan Beulich wrote:
 On 17.06.16 at 10:36,  wrote:
>> On 06/17/2016 11:33 AM, Corneliu ZUZU wrote:
>>> Ah, ok. Didn't that patch make it to staging yet? I pulled the latest.
>>> Since you already have a patch for that I guess it's ok to remove those
>>> comments and leave the rest as it is and merge later when one of these
>>> patches makes it to staging?
>>
>> I think there's a delay caused by the 4.7 release. I also have an acked
>> patch waiting to go in that might cause you to have to rebase (part of)
>> the series: https://patchwork.kernel.org/patch/9033771/ 
> 
> Well, there's a slight difference here: Yours is indeed delayed until
> 4.7 is set in stone, for being more intrusive. Tamas'es, otoh, I would
> probably have put in already if it had all necessary acks (or they
> had at least been pinged for, at which point we could waive the need
> for some trivial adjustments).

Right, so should I resend as Andrew has requested, or just wait?


Thanks,
Razvan

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

Re: [Xen-devel] [PATCH 2/2] x86/HVM: re-order operations in hvm_ud_intercept()

[Andrew Cooper]

On 09/06/16 16:05, Jan Beulich wrote:
 On 09.06.16 at 16:27,  wrote:
>> On 09/06/16 15:13, Jan Beulich wrote:
>> On 09.06.16 at 16:06,  wrote:
 On 09/06/16 13:31, Jan Beulich wrote:
 On 09.06.16 at 13:34,  wrote:
>> On 08/06/16 14:43, Jan Beulich wrote:
>>> Don't fetch CS explicitly, leverage the fact that hvm_emulate_prepare()
>>> already does (and that hvm_virtual_to_linear_addr() doesn't alter it).
>>>
>>> At once increase the length passed to hvm_virtual_to_linear_addr() by
>>> one: There definitely needs to be at least one more opcode byte, and we
>>> can avoid missing a wraparound case this way.
>>>
>>> Signed-off-by: Jan Beulich 
>> I looked into this when you suggested it, but it latches the wrong eip
>> in the emulation state, and you will end up re-emulating the ud2a
>> instruction, rather than the following instruction.
> Where is there any latching of eip? All hvm_emulate_prepare() does
> is storing the regs pointer.
 Oh - so it does.  I clearly looked over it too quickly.

 What wraparound issue are you referring to?  Adding 1 will cause
 incorrect behaviour when the emulation prefix ends at the segment limit.
>>> I don't think so: The prefix together with the actual instruction
>>> encoding should be viewed as a single instruction, and it crossing
>>> the segment limit should #GP. It wrapping at the prefix/encoding
>>> boundary is the case that I'm specifically referring to (this case
>>> should also #GP, but wouldn't without this adjustment).
>> But the force emulation prefix specifically doesn't behave like other
>> prefixes.
>>
>> It doesn't count towards the 15 byte instruction limit, and if the
>> emulated instruction does fault, we want the fault pointing at the
>> emulated instruction, not the force prefix.  We should avoid making any
>> link.
> Well, are you saying placing such a prefix right below the boundary
> of a flat segment is _expected_ to get the instruction at address 0
> emulated? I don't think I could buy that. The patch makes no other
> connection between prefix and actual insn. And #GP because of
> such a boundary condition should imo point at the prefix; only all
> faults associated with the actual insn should point there.

Ok.  That sounds reasonable.  Would it be possible to add a small
comment to the code? Even with the commit message, I was confused as to
the nature of the +1.

Reviewed-by: Andrew Cooper 

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

Re: [Xen-devel] [PATCH v2] xen: fix qdisk BLKIF_OP_DISCARD for 32/64 word size mix

[Paul Durrant]

> -Original Message-
> From: Xen-devel [mailto:xen-devel-boun...@lists.xen.org] On Behalf Of Jan
> Beulich
> Sent: 17 June 2016 10:26
> To: Juergen Gross
> Cc: Anthony Perard; xen-devel; sstabell...@kernel.org; qemu-
> de...@nongnu.org; kra...@redhat.com
> Subject: Re: [Xen-devel] [PATCH v2] xen: fix qdisk BLKIF_OP_DISCARD for
> 32/64 word size mix
> 
> >>> On 17.06.16 at 11:14,  wrote:
> > In case the word size of the domU and qemu running the qdisk backend
> > differ BLKIF_OP_DISCARD will not work reliably, as the request
> > structure in the ring have different layouts for different word size.
> >
> > Correct this by copying the request structure in case of different
> > word size element by element in the BLKIF_OP_DISCARD case, too.
> >
> > The easiest way to achieve this is to resync hw/block/xen_blkif.h with
> > its original source from the Linux kernel.
> >
> > Signed-off-by: Juergen Gross 
> > ---
> > V2: resync with Linux kernel version of hw/block/xen_blkif.h as
> > suggested by Paul Durrant
> 
> Oh, I didn't realize he suggested syncing with the Linux variant.
> Why not with the canonical one? I have to admit that I particularly
> dislike Linux'es strange union-izng, mainly because of it requiring
> this myriad of __attribute__((__packed__)).
>

Yes, it's truly grotesque and such things should be blown away with extreme 
prejudice.

  Paul
 
> Jan
> 
> 
> ___
> 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] [PATCH 1/2] hvmloader: limit CPUs exposed to guests

[Andrew Cooper]

On 16/06/16 10:40, Jan Beulich wrote:
> Various Linux versions allocate (partial) per-CPU data for all of them,
> as there is no indication in MADT whether they're hotpluggable. That's
> a little wasteful in terms of resource consumption especially for
> - guests with not overly much memory assigned,
> - 32-bit guests not having overly much address space available.
> Therefore limit what we put into MADT to the "maxvcpus" value, and make
> sure AML doesn't touch memory addresses corresponding to CPUs beyond
> that value (we can't reasonably make the respective processor objects
> disappear).
>
> Signed-off-by: Jan Beulich 

I don't feel that I know AML well enough to review, but the C looks ok
and this seems like a sensible change.

Acked-by: Andrew Cooper 

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

Re: [Xen-devel] [PATCH 1/7] minor (formatting) fixes

[Jan Beulich]

>>> On 17.06.16 at 11:36,  wrote:
> On 06/17/2016 12:33 PM, Jan Beulich wrote:
> On 17.06.16 at 10:36,  wrote:
>>> On 06/17/2016 11:33 AM, Corneliu ZUZU wrote:
 Ah, ok. Didn't that patch make it to staging yet? I pulled the latest.
 Since you already have a patch for that I guess it's ok to remove those
 comments and leave the rest as it is and merge later when one of these
 patches makes it to staging?
>>>
>>> I think there's a delay caused by the 4.7 release. I also have an acked
>>> patch waiting to go in that might cause you to have to rebase (part of)
>>> the series: https://patchwork.kernel.org/patch/9033771/ 
>> 
>> Well, there's a slight difference here: Yours is indeed delayed until
>> 4.7 is set in stone, for being more intrusive. Tamas'es, otoh, I would
>> probably have put in already if it had all necessary acks (or they
>> had at least been pinged for, at which point we could waive the need
>> for some trivial adjustments).
> 
> Right, so should I resend as Andrew has requested, or just wait?

I have the patch queued, so unless it needs rebasing I don't see a
strict need for resending.

Jan


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

Re: [Xen-devel] [PATCH v2] xen: fix qdisk BLKIF_OP_DISCARD for 32/64 word size mix

[Jan Beulich]

>>> On 17.06.16 at 11:31,  wrote:
> On 17/06/16 11:26, Jan Beulich wrote:
> On 17.06.16 at 11:14,  wrote:
>>> In case the word size of the domU and qemu running the qdisk backend
>>> differ BLKIF_OP_DISCARD will not work reliably, as the request
>>> structure in the ring have different layouts for different word size.
>>>
>>> Correct this by copying the request structure in case of different
>>> word size element by element in the BLKIF_OP_DISCARD case, too.
>>>
>>> The easiest way to achieve this is to resync hw/block/xen_blkif.h with
>>> its original source from the Linux kernel.
>>>
>>> Signed-off-by: Juergen Gross 
>>> ---
>>> V2: resync with Linux kernel version of hw/block/xen_blkif.h as
>>> suggested by Paul Durrant
>> 
>> Oh, I didn't realize he suggested syncing with the Linux variant.
>> Why not with the canonical one? I have to admit that I particularly
>> dislike Linux'es strange union-izng, mainly because of it requiring
>> this myriad of __attribute__((__packed__)).
> 
> What would be gained by syncing with the canonical one? The part to be
> modified is available in the Linux variant only.

In, as said, a rather disgusting (in my opinion) way. I'm no the
maintainer anyway, so you don't have to convince me.

Jan


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

Re: [Xen-devel] [PATCH 1/7] minor (formatting) fixes

[Razvan Cojocaru]

On 06/17/2016 12:40 PM, Jan Beulich wrote:
 On 17.06.16 at 11:36,  wrote:
>> On 06/17/2016 12:33 PM, Jan Beulich wrote:
>> On 17.06.16 at 10:36,  wrote:
 On 06/17/2016 11:33 AM, Corneliu ZUZU wrote:
> Ah, ok. Didn't that patch make it to staging yet? I pulled the latest.
> Since you already have a patch for that I guess it's ok to remove those
> comments and leave the rest as it is and merge later when one of these
> patches makes it to staging?

 I think there's a delay caused by the 4.7 release. I also have an acked
 patch waiting to go in that might cause you to have to rebase (part of)
 the series: https://patchwork.kernel.org/patch/9033771/ 
>>>
>>> Well, there's a slight difference here: Yours is indeed delayed until
>>> 4.7 is set in stone, for being more intrusive. Tamas'es, otoh, I would
>>> probably have put in already if it had all necessary acks (or they
>>> had at least been pinged for, at which point we could waive the need
>>> for some trivial adjustments).
>>
>> Right, so should I resend as Andrew has requested, or just wait?
> 
> I have the patch queued, so unless it needs rebasing I don't see a
> strict need for resending.

Checking just now, it doesn't seem to need rebasing.


Thanks,
Razvan

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

Re: [Xen-devel] [PATCH v2] xen: fix qdisk BLKIF_OP_DISCARD for 32/64 word size mix

[Paul Durrant]

> -Original Message-
> From: Xen-devel [mailto:xen-devel-boun...@lists.xen.org] On Behalf Of
> Juergen Gross
> Sent: 17 June 2016 10:31
> To: Jan Beulich
> Cc: Anthony Perard; xen-devel; sstabell...@kernel.org; qemu-
> de...@nongnu.org; kra...@redhat.com
> Subject: Re: [Xen-devel] [PATCH v2] xen: fix qdisk BLKIF_OP_DISCARD for
> 32/64 word size mix
> 
> On 17/06/16 11:26, Jan Beulich wrote:
>  On 17.06.16 at 11:14,  wrote:
> >> In case the word size of the domU and qemu running the qdisk backend
> >> differ BLKIF_OP_DISCARD will not work reliably, as the request
> >> structure in the ring have different layouts for different word size.
> >>
> >> Correct this by copying the request structure in case of different
> >> word size element by element in the BLKIF_OP_DISCARD case, too.
> >>
> >> The easiest way to achieve this is to resync hw/block/xen_blkif.h with
> >> its original source from the Linux kernel.
> >>
> >> Signed-off-by: Juergen Gross 
> >> ---
> >> V2: resync with Linux kernel version of hw/block/xen_blkif.h as
> >> suggested by Paul Durrant
> >
> > Oh, I didn't realize he suggested syncing with the Linux variant.
> > Why not with the canonical one? I have to admit that I particularly
> > dislike Linux'es strange union-izng, mainly because of it requiring
> > this myriad of __attribute__((__packed__)).
> 
> What would be gained by syncing with the canonical one? The part to be
> modified is available in the Linux variant only.
>

So there's something in the Linux variant that's not in the canonical header?! 
Well that needs to be fixed first and then, yes, I did mean re-sync with the 
canonical header.

  Paul
 
> 
> Juergen
> 
> 
> ___
> 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] [PATCH v2] xen: fix qdisk BLKIF_OP_DISCARD for 32/64 word size mix

[Juergen Gross]

On 17/06/16 11:37, Paul Durrant wrote:
>> -Original Message-
>> From: Xen-devel [mailto:xen-devel-boun...@lists.xen.org] On Behalf Of Jan
>> Beulich
>> Sent: 17 June 2016 10:26
>> To: Juergen Gross
>> Cc: Anthony Perard; xen-devel; sstabell...@kernel.org; qemu-
>> de...@nongnu.org; kra...@redhat.com
>> Subject: Re: [Xen-devel] [PATCH v2] xen: fix qdisk BLKIF_OP_DISCARD for
>> 32/64 word size mix
>>
> On 17.06.16 at 11:14,  wrote:
>>> In case the word size of the domU and qemu running the qdisk backend
>>> differ BLKIF_OP_DISCARD will not work reliably, as the request
>>> structure in the ring have different layouts for different word size.
>>>
>>> Correct this by copying the request structure in case of different
>>> word size element by element in the BLKIF_OP_DISCARD case, too.
>>>
>>> The easiest way to achieve this is to resync hw/block/xen_blkif.h with
>>> its original source from the Linux kernel.
>>>
>>> Signed-off-by: Juergen Gross 
>>> ---
>>> V2: resync with Linux kernel version of hw/block/xen_blkif.h as
>>> suggested by Paul Durrant
>>
>> Oh, I didn't realize he suggested syncing with the Linux variant.
>> Why not with the canonical one? I have to admit that I particularly
>> dislike Linux'es strange union-izng, mainly because of it requiring
>> this myriad of __attribute__((__packed__)).
>>
> 
> Yes, it's truly grotesque and such things should be blown away with extreme 
> prejudice.

Sorry, I'm confused now.

Do you still mandate for the resync or not?

Resyncing with elimination of all the __packed__ stuff seems not to be
a proper alternative as this would require a major rework. So either I
do a resync and keep this stuff or I don't resync at all.


Juergen


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

Re: [Xen-devel] [PATCH v2] xen: fix qdisk BLKIF_OP_DISCARD for 32/64 word size mix

[Paul Durrant]

> -Original Message-
> From: Juergen Gross [mailto:jgr...@suse.com]
> Sent: 17 June 2016 10:46
> To: Paul Durrant; Jan Beulich
> Cc: Anthony Perard; xen-devel; sstabell...@kernel.org; qemu-
> de...@nongnu.org; kra...@redhat.com
> Subject: Re: [Xen-devel] [PATCH v2] xen: fix qdisk BLKIF_OP_DISCARD for
> 32/64 word size mix
> 
> On 17/06/16 11:37, Paul Durrant wrote:
> >> -Original Message-
> >> From: Xen-devel [mailto:xen-devel-boun...@lists.xen.org] On Behalf Of
> Jan
> >> Beulich
> >> Sent: 17 June 2016 10:26
> >> To: Juergen Gross
> >> Cc: Anthony Perard; xen-devel; sstabell...@kernel.org; qemu-
> >> de...@nongnu.org; kra...@redhat.com
> >> Subject: Re: [Xen-devel] [PATCH v2] xen: fix qdisk BLKIF_OP_DISCARD for
> >> 32/64 word size mix
> >>
> > On 17.06.16 at 11:14,  wrote:
> >>> In case the word size of the domU and qemu running the qdisk backend
> >>> differ BLKIF_OP_DISCARD will not work reliably, as the request
> >>> structure in the ring have different layouts for different word size.
> >>>
> >>> Correct this by copying the request structure in case of different
> >>> word size element by element in the BLKIF_OP_DISCARD case, too.
> >>>
> >>> The easiest way to achieve this is to resync hw/block/xen_blkif.h with
> >>> its original source from the Linux kernel.
> >>>
> >>> Signed-off-by: Juergen Gross 
> >>> ---
> >>> V2: resync with Linux kernel version of hw/block/xen_blkif.h as
> >>> suggested by Paul Durrant
> >>
> >> Oh, I didn't realize he suggested syncing with the Linux variant.
> >> Why not with the canonical one? I have to admit that I particularly
> >> dislike Linux'es strange union-izng, mainly because of it requiring
> >> this myriad of __attribute__((__packed__)).
> >>
> >
> > Yes, it's truly grotesque and such things should be blown away with
> extreme prejudice.
> 
> Sorry, I'm confused now.
> 
> Do you still mandate for the resync or not?
> 
> Resyncing with elimination of all the __packed__ stuff seems not to be
> a proper alternative as this would require a major rework.

Why? Replacing the existing horribleness with the canonical header (fixed for 
style) might mean a large diff but it should be functionally the same or 
something has gone very seriously wrong. If the extra part you need is not in 
the canonical header then adding this as a second patch seems like a reasonable 
plan. 

> So either I
> do a resync and keep this stuff or I don't resync at all.
>

Proliferating brokenness should avoided IMO but if the maintainers are happy 
with it then go ahead.

  Paul

> 
> Juergen

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

Re: [Xen-devel] [PATCH 2/2] x86/HVM: re-order operations in hvm_ud_intercept()

[Jan Beulich]

>>> On 17.06.16 at 11:37,  wrote:
> On 09/06/16 16:05, Jan Beulich wrote:
> On 09.06.16 at 16:27,  wrote:
>>> On 09/06/16 15:13, Jan Beulich wrote:
>>> On 09.06.16 at 16:06,  wrote:
> On 09/06/16 13:31, Jan Beulich wrote:
> On 09.06.16 at 13:34,  wrote:
>>> On 08/06/16 14:43, Jan Beulich wrote:
 Don't fetch CS explicitly, leverage the fact that hvm_emulate_prepare()
 already does (and that hvm_virtual_to_linear_addr() doesn't alter it).

 At once increase the length passed to hvm_virtual_to_linear_addr() by
 one: There definitely needs to be at least one more opcode byte, and we
 can avoid missing a wraparound case this way.

 Signed-off-by: Jan Beulich 
>>> I looked into this when you suggested it, but it latches the wrong eip
>>> in the emulation state, and you will end up re-emulating the ud2a
>>> instruction, rather than the following instruction.
>> Where is there any latching of eip? All hvm_emulate_prepare() does
>> is storing the regs pointer.
> Oh - so it does.  I clearly looked over it too quickly.
>
> What wraparound issue are you referring to?  Adding 1 will cause
> incorrect behaviour when the emulation prefix ends at the segment limit.
 I don't think so: The prefix together with the actual instruction
 encoding should be viewed as a single instruction, and it crossing
 the segment limit should #GP. It wrapping at the prefix/encoding
 boundary is the case that I'm specifically referring to (this case
 should also #GP, but wouldn't without this adjustment).
>>> But the force emulation prefix specifically doesn't behave like other
>>> prefixes.
>>>
>>> It doesn't count towards the 15 byte instruction limit, and if the
>>> emulated instruction does fault, we want the fault pointing at the
>>> emulated instruction, not the force prefix.  We should avoid making any
>>> link.
>> Well, are you saying placing such a prefix right below the boundary
>> of a flat segment is _expected_ to get the instruction at address 0
>> emulated? I don't think I could buy that. The patch makes no other
>> connection between prefix and actual insn. And #GP because of
>> such a boundary condition should imo point at the prefix; only all
>> faults associated with the actual insn should point there.
> 
> Ok.  That sounds reasonable.  Would it be possible to add a small
> comment to the code? Even with the commit message, I was confused as to
> the nature of the +1.

+/*
+ * Note that in the call below we pass 1 more than the signature
+ * size, to guard against the overall code sequence wrapping between
+ * "prefix" and actual instruction. There's necessarily at least one
+ * actual instruction byte required, so this won't cause failure on
+ * legitimate uses.
+ */

> Reviewed-by: Andrew Cooper 

Thanks, Jan




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

Re: [Xen-devel] [PATCH 1/1] tools/livepatch: initialise j to 0 to make some versions of gcc happy

[Wei Liu]

On Thu, Jun 16, 2016 at 02:47:51PM -0700, Dongli Zhang wrote:
> 
> > I suggest pasting in Olaf's exact error message here.
> > 
> > To avoid extra round trip, I propose updating the commit message as
> > followed:
> > 
> > Initialise j to 0 to make some versions of gcc (e.g., gcc4.5/4.3)
> > happy to
> > avoid compilation error by commit
> > beba3693f7243e68bbe31fe3794da91068eeea5b.
> > 
> > Failure manifests with gcc 4.5 as:
> > 
> > [  153s] cc1: warnings being treated as errors
> > [  153s] xen-livepatch.c: In function 'main':
> > [  153s] xen-livepatch.c:415:12: error: 'j' may be used
> > uninitialized in this function
> > [  153s] make[3]: *** [xen-livepatch.o] Error 1
> > 
> > If you agree with this I will handle the updating while doing my next
> > sweep.
> 
> 
> Sure. Feel free to update the commit message.

Thanks for confirming. This patch is now acked and queued.

Wei.

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

Re: [Xen-devel] [PATCH v3 07/16] x86/boot: create *.lnk files with linker script

[Jan Beulich]

>>> On 17.06.16 at 11:06,  wrote:
> On Tue, May 24, 2016 at 06:52:39AM -0600, Jan Beulich wrote:
>> >>> On 24.05.16 at 14:28,  wrote:
>> > On Tue, May 24, 2016 at 03:05:06AM -0600, Jan Beulich wrote:
>> >> >>> On 15.04.16 at 14:33,  wrote:
>> >> > +  /DISCARD/ : {
>> >> > +/*
>> >> > + * .got.plt section is used only by dynamic linker
>> >> > + * and our output is not supposed to be loaded by
>> >> > + * dynamic linker. Additionally, it just contains
>> >> > + * .PLT0 which is referenced from nowhere. So, we
>> >> > + * can safely drop .got.plt here.
>> >> > + *
>> >> > + * Ha! This should be really discarded here. However,
>> >> > + * .got.plt section contains _GLOBAL_OFFSET_TABLE_
>> >> > + * symbol too and it is used as a reference for relative
>> >> > + * addressing (and only for that thing). Hence, ld
>> >> > + * complains if we remove that section because it
>> >> > + * cannot find _GLOBAL_OFFSET_TABLE_. So, drop .got.plt
>> >> > + * section during conversion to plain binary format.
>> >> > + * Please check build32.mk for more details.
>> >> > + */
>> >> > +/* *(.got.plt) */
>> >> > +  }
>> >>
>> >> I'm afraid this needs more investigation: Afaik there should be no
>> >
>> > I am not sure what else we should look for.
>>
>> The reason why such an empty .got.plt gets created in the first place.
>> If e.g. that turns out to be a bug in (some versions of) binutils, then
>> that bug should be named here as the reason.
> 
> If PIC/PIE code is build then .got.plt exists in executable even if it
> is not linked with dynamic libraries.

Well - then just don't force -fPIC or -fPIE for the compilation of this
code?

>> >> reason for the linker to create an otherwise empty .got.plt in the
>> >
>> > As I wrote above. It contains _GLOBAL_OFFSET_TABLE_ which is used
>> > as a reference for relative addressing.
>>
>> But we don't use any such, so without being needed I don't think
>> the symbol needs to be created.
> 
> R_386_GOTPC and R_386_GOTOFF relocations use address of 
> _GLOBAL_OFFSET_TABLE_
> as a reference. So, it is needed during linking phase. However, later it is
> not needed.  Hence, .got.plt with _GLOBAL_OFFSET_TABLE_ and .PLT0 can safely
> be dropped.

These two relocation types should not appear for non-PIC/PIE code.

Jan


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

Re: [Xen-devel] [PATCH 2/2] hvmloader: don't hard-code IO-APIC parameters

[Andrew Cooper]

On 16/06/16 10:40, Jan Beulich wrote:
> The IO-APIC address has variable bits determined by the PCI-to-ISA
> bridge, and the IO-APIC version should be read from the IO-APIC. (Note
> that there's still implicit rather than explicit agreement on the
> IO-APIC base address between qemu and the hypervisor.)
>
> Signed-off-by: Jan Beulich 

The status quo is not great, and I can see why you want to improve it.

However, I think that this is not the way to do that.  It ties HVMLoader
to the PIIX4 board in Qemu, and will break attempts to use Q35 or
something else.  (In Q35, the IO-APIC decode address comes from Chipset
Configuration Register, rather than ISA device config space).

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

Re: [Xen-devel] [PATCH v2] xen: fix qdisk BLKIF_OP_DISCARD for 32/64 word size mix

[Juergen Gross]

On 17/06/16 11:50, Paul Durrant wrote:
>> -Original Message-
>> From: Juergen Gross [mailto:jgr...@suse.com]
>> Sent: 17 June 2016 10:46
>> To: Paul Durrant; Jan Beulich
>> Cc: Anthony Perard; xen-devel; sstabell...@kernel.org; qemu-
>> de...@nongnu.org; kra...@redhat.com
>> Subject: Re: [Xen-devel] [PATCH v2] xen: fix qdisk BLKIF_OP_DISCARD for
>> 32/64 word size mix
>>
>> On 17/06/16 11:37, Paul Durrant wrote:
 -Original Message-
 From: Xen-devel [mailto:xen-devel-boun...@lists.xen.org] On Behalf Of
>> Jan
 Beulich
 Sent: 17 June 2016 10:26
 To: Juergen Gross
 Cc: Anthony Perard; xen-devel; sstabell...@kernel.org; qemu-
 de...@nongnu.org; kra...@redhat.com
 Subject: Re: [Xen-devel] [PATCH v2] xen: fix qdisk BLKIF_OP_DISCARD for
 32/64 word size mix

>>> On 17.06.16 at 11:14,  wrote:
> In case the word size of the domU and qemu running the qdisk backend
> differ BLKIF_OP_DISCARD will not work reliably, as the request
> structure in the ring have different layouts for different word size.
>
> Correct this by copying the request structure in case of different
> word size element by element in the BLKIF_OP_DISCARD case, too.
>
> The easiest way to achieve this is to resync hw/block/xen_blkif.h with
> its original source from the Linux kernel.
>
> Signed-off-by: Juergen Gross 
> ---
> V2: resync with Linux kernel version of hw/block/xen_blkif.h as
> suggested by Paul Durrant

 Oh, I didn't realize he suggested syncing with the Linux variant.
 Why not with the canonical one? I have to admit that I particularly
 dislike Linux'es strange union-izng, mainly because of it requiring
 this myriad of __attribute__((__packed__)).

>>>
>>> Yes, it's truly grotesque and such things should be blown away with
>> extreme prejudice.
>>
>> Sorry, I'm confused now.
>>
>> Do you still mandate for the resync or not?
>>
>> Resyncing with elimination of all the __packed__ stuff seems not to be
>> a proper alternative as this would require a major rework.
> 
> Why? Replacing the existing horribleness with the canonical header (fixed for 
> style) might mean a large diff but it should be functionally the same or 
> something has gone very seriously wrong. If the extra part you need is not in 
> the canonical header then adding this as a second patch seems like a 
> reasonable plan. 

I think you don't realize that qemu is built using the public headers
from the Xen build environment. So there is no way to resync with the
canonical header as this isn't part of the qemu tree.

The header in question is originating from the Linux one which is an
add-on of the canonical header containing the explicit 32- and 64-bit
variants of the xenbus protocol and the conversion routines between
those.

It would be possible to add these parts to the canonical header, but
do we really want that?


Juergen

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

Re: [Xen-devel] [PATCH RESEND 00/14] Xen ARM DomU ACPI support

[Wei Liu]

On Thu, Jun 16, 2016 at 09:25:20AM -0400, Boris Ostrovsky wrote:
> On 06/16/2016 07:20 AM, Wei Liu wrote:
> > On Tue, Jun 07, 2016 at 12:00:26PM +0100, Julien Grall wrote:
> >> Hello Shannon,
> >>
> >> On 07/06/16 02:07, Shannon Zhao wrote:
> >>> On 2016/6/6 23:48, Julien Grall wrote:
>  On 06/06/16 13:26, Wei Liu wrote:
> > On Tue, May 31, 2016 at 01:02:52PM +0800, Shannon Zhao wrote:
> >> From: Shannon Zhao 
> >>
> >> The design of this feature is described as below.
> >> Firstly, the toolstack (libxl) generates the ACPI tables according the
> >> number of vcpus and gic controller.
> >>
> >> Then, it copies these ACPI tables to DomU memory space and passes
> >> them to UEFI firmware through the "ARM multiboot" protocol.
> >>
> >> At last, UEFI gets the ACPI tables through the "ARM multiboot" protocol
> >> and installs these tables like the usual way and passes both ACPI and 
> >> DT
> >> information to the Xen DomU.
> >>
> >> Currently libxl only generates RSDP, XSDT, GTDT, MADT, FADT, DSDT 
> >> tables
> >> since it's enough now.
> >>
> >> This has been tested using guest kernel with the Dom0 ACPI support
> >> patches which could be fetched from:
> >> https://git.kernel.org/cgit/linux/kernel/git/mfleming/efi.git/log/?h=efi/arm-xen
> >>
> >>
> >> Shannon Zhao (14):
> >>libxl/arm: Fix the function name in error log
> >>libxl/arm: Factor out codes for generating DTB
> >>libxc: Add placeholders for ACPI tables blob and size
> >>tools: add ACPI tables relevant definitions
> >>libxl/arm: Construct ACPI GTDT table
> >>libxl/arm: Construct ACPI FADT table
> >>libxl/arm: Construct ACPI DSDT table
> >>libxl/arm: Construct ACPI MADT table
> >>libxl/arm: Construct ACPI XSDT table
> >>libxl/arm: Construct ACPI RSDP table
> >>libxl/arm: Initialize domain param HVM_PARAM_CALLBACK_IRQ
> >>libxl/arm: Add ACPI module
> >>libxl/arm: initialize memory information of ACPI blob
> >>libxc/xc_dom_core: Copy ACPI tables to guest memory space
> >>
> > After going through this series, I think the code mostly looks good.
> >
> > There is one higher level question: you seem to have put a lot of the
> > table construction code in libxl, while I failed to see why specific
> > libxl information is needed. Have you considered moving the code to
> > libxc?
>  I would rather avoid to have the device tree built by libxl and ACPI
>  tables built by libxc.
> 
>  I don't remember why we have decided to implement DT building in libxl,
>  I guess it is because we need to know which GIC version is used (GICv2
>  vs GICv3).
> 
>  In the long run, we might also need to have more part of the firmware
>  configurable (performance monitor support, memory layout, UART...).
> 
>  Most of those information are available easily in libxl, we would to
>  export them of libxc.
> >>> Yes, and one more reason is that to support ACPI, it also needs to add
> >>> the ACPI multiboot module in DT.
> >> I don't consider this as a reason for building the ACPI tables in libxl. I
> >> am more concerned about building the firmwares in different place.
> >>
> >> If we decide to build the ACPI tables in libxc, then the code to build the
> >> device tree should move in libxc too.
> >>
> > I've read this sub-thread and the other thread in which Boris replied, I
> > think due to the fact that libxl has more information at hand and libxc
> > is intended to be simple, I think having the building code in libxl
> > makes sense.
> >
> > Shannon, Boris and Julien, what do you guys think? Can we agree on
> > something so that Shannon can move on with next iteration?
> 
> I kind of agree that libxl is perhaps a better place but it would be
> good to have something more than "libxc is intended to be simple" that
> tells us what goes where. In other words -- what libxl is for and what
> libxc is for.
> 
> For example libxl_x86.c and xc_dom86.c. The latter has a comment at the
> top that says this is for arch-specific code. But there is plenty of
> arch-specific stuff (e820) in libxl file.
> 

I myself have long been following the convention that any complex
decision should be made inside libxl because it just has more
information than libxc and a plethora of machinery at its disposal.
Libxc should merely be a wrapper for the underlying hypervisor
interface. There are, of course, exceptions that the libxc functions
look a bit more complex because we want to provide some compatibility
where we can, but keep in mind that libxc is meant to be unstable so
that semantics is subject to change at any time.

Overtime the evolution of libraries will certainly leave some areas
unclear.  The E820 code is an example that libxc is not able to make
decision on its own and we move that to libxl. I wouldn't want 

Re: [Xen-devel] [PATCH v2] xen: fix qdisk BLKIF_OP_DISCARD for 32/64 word size mix

[Paul Durrant]

> -Original Message-
> From: Xen-devel [mailto:xen-devel-boun...@lists.xen.org] On Behalf Of
> Juergen Gross
> Sent: 17 June 2016 11:08
> To: Paul Durrant; Jan Beulich
> Cc: Anthony Perard; xen-devel; sstabell...@kernel.org; qemu-
> de...@nongnu.org; kra...@redhat.com
> Subject: Re: [Xen-devel] [PATCH v2] xen: fix qdisk BLKIF_OP_DISCARD for
> 32/64 word size mix
> 
> On 17/06/16 11:50, Paul Durrant wrote:
> >> -Original Message-
> >> From: Juergen Gross [mailto:jgr...@suse.com]
> >> Sent: 17 June 2016 10:46
> >> To: Paul Durrant; Jan Beulich
> >> Cc: Anthony Perard; xen-devel; sstabell...@kernel.org; qemu-
> >> de...@nongnu.org; kra...@redhat.com
> >> Subject: Re: [Xen-devel] [PATCH v2] xen: fix qdisk BLKIF_OP_DISCARD for
> >> 32/64 word size mix
> >>
> >> On 17/06/16 11:37, Paul Durrant wrote:
>  -Original Message-
>  From: Xen-devel [mailto:xen-devel-boun...@lists.xen.org] On Behalf
> Of
> >> Jan
>  Beulich
>  Sent: 17 June 2016 10:26
>  To: Juergen Gross
>  Cc: Anthony Perard; xen-devel; sstabell...@kernel.org; qemu-
>  de...@nongnu.org; kra...@redhat.com
>  Subject: Re: [Xen-devel] [PATCH v2] xen: fix qdisk BLKIF_OP_DISCARD
> for
>  32/64 word size mix
> 
> >>> On 17.06.16 at 11:14,  wrote:
> > In case the word size of the domU and qemu running the qdisk
> backend
> > differ BLKIF_OP_DISCARD will not work reliably, as the request
> > structure in the ring have different layouts for different word size.
> >
> > Correct this by copying the request structure in case of different
> > word size element by element in the BLKIF_OP_DISCARD case, too.
> >
> > The easiest way to achieve this is to resync hw/block/xen_blkif.h with
> > its original source from the Linux kernel.
> >
> > Signed-off-by: Juergen Gross 
> > ---
> > V2: resync with Linux kernel version of hw/block/xen_blkif.h as
> > suggested by Paul Durrant
> 
>  Oh, I didn't realize he suggested syncing with the Linux variant.
>  Why not with the canonical one? I have to admit that I particularly
>  dislike Linux'es strange union-izng, mainly because of it requiring
>  this myriad of __attribute__((__packed__)).
> 
> >>>
> >>> Yes, it's truly grotesque and such things should be blown away with
> >> extreme prejudice.
> >>
> >> Sorry, I'm confused now.
> >>
> >> Do you still mandate for the resync or not?
> >>
> >> Resyncing with elimination of all the __packed__ stuff seems not to be
> >> a proper alternative as this would require a major rework.
> >
> > Why? Replacing the existing horribleness with the canonical header (fixed
> for style) might mean a large diff but it should be functionally the same or
> something has gone very seriously wrong. If the extra part you need is not in
> the canonical header then adding this as a second patch seems like a
> reasonable plan.
> 
> I think you don't realize that qemu is built using the public headers
> from the Xen build environment. So there is no way to resync with the
> canonical header as this isn't part of the qemu tree.
> 

Now I'm confused... you're posting a patch to hw/block/xen_blkif.h. That's in 
the QEMU source, right? That's not a Xen public header but is a Linux mangled 
variant of a Xen public header. So, actually, I guess the question is why can't 
this header just go away and QEMU use the canonical header directly from Xen?

> The header in question is originating from the Linux one which is an
> add-on of the canonical header containing the explicit 32- and 64-bit
> variants of the xenbus protocol and the conversion routines between
> those.
> 
> It would be possible to add these parts to the canonical header, but
> do we really want that?
> 

No, we shouldn't be taking Linux brokenness into the canonical header.

  Paul

> 
> Juergen
> 
> ___
> 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] [PATCH v4 3/3] x86/ioreq server: Add HVMOP to map guest ram with p2m_ioreq_server to an ioreq server.

[George Dunlap]

On 16/06/16 10:55, Jan Beulich wrote:
>> Previously in the 2nd version, I used p2m_change_entry_type_global() to 
>> reset the
>> outstanding p2m_ioreq_server entries back to p2m_ram_rw asynchronously after
>> the de-registration. But we realized later that this approach means we 
>> can not support
>> live migration. And to recalculate the whole p2m table forcefully when 
>> de-registration
>> happens means too much cost.
>>
>> And further discussion with Paul was that we can leave the 
>> responsibility to reset p2m type
>> to the device model side, and even a device model fails to do so, the 
>> affected one will only
>> be the current VM, neither other VM nor hypervisor will get hurt.
>>
>> I thought we have reached agreement in the review process of version 2, 
>> so I removed
>> this part from version 3.
> 
> In which case I would appreciate the commit message to explain
> this (in particular I admit I don't recall why live migration would
> be affected by the p2m_change_entry_type_global() approach,
> but the request is also so that later readers have at least some
> source of information other than searching the mailing list).

Yes, I don't see why either.  You wouldn't de-register the ioreq server
until after the final sweep after the VM has been paused, right?  At
which point the lazy p2m re-calculation shouldn't really matter much I
don't think.

 -George


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

Re: [Xen-devel] [PATCH v2] xen: fix qdisk BLKIF_OP_DISCARD for 32/64 word size mix

[Jan Beulich]

>>> On 17.06.16 at 12:08,  wrote:
> On 17/06/16 11:50, Paul Durrant wrote:
>>> -Original Message-
>>> From: Juergen Gross [mailto:jgr...@suse.com]
>>> Sent: 17 June 2016 10:46
>>> To: Paul Durrant; Jan Beulich
>>> Cc: Anthony Perard; xen-devel; sstabell...@kernel.org; qemu-
>>> de...@nongnu.org; kra...@redhat.com 
>>> Subject: Re: [Xen-devel] [PATCH v2] xen: fix qdisk BLKIF_OP_DISCARD for
>>> 32/64 word size mix
>>>
>>> On 17/06/16 11:37, Paul Durrant wrote:
> -Original Message-
> From: Xen-devel [mailto:xen-devel-boun...@lists.xen.org] On Behalf Of
>>> Jan
> Beulich
> Sent: 17 June 2016 10:26
> To: Juergen Gross
> Cc: Anthony Perard; xen-devel; sstabell...@kernel.org; qemu-
> de...@nongnu.org; kra...@redhat.com 
> Subject: Re: [Xen-devel] [PATCH v2] xen: fix qdisk BLKIF_OP_DISCARD for
> 32/64 word size mix
>
 On 17.06.16 at 11:14,  wrote:
>> In case the word size of the domU and qemu running the qdisk backend
>> differ BLKIF_OP_DISCARD will not work reliably, as the request
>> structure in the ring have different layouts for different word size.
>>
>> Correct this by copying the request structure in case of different
>> word size element by element in the BLKIF_OP_DISCARD case, too.
>>
>> The easiest way to achieve this is to resync hw/block/xen_blkif.h with
>> its original source from the Linux kernel.
>>
>> Signed-off-by: Juergen Gross 
>> ---
>> V2: resync with Linux kernel version of hw/block/xen_blkif.h as
>> suggested by Paul Durrant
>
> Oh, I didn't realize he suggested syncing with the Linux variant.
> Why not with the canonical one? I have to admit that I particularly
> dislike Linux'es strange union-izng, mainly because of it requiring
> this myriad of __attribute__((__packed__)).
>

 Yes, it's truly grotesque and such things should be blown away with
>>> extreme prejudice.
>>>
>>> Sorry, I'm confused now.
>>>
>>> Do you still mandate for the resync or not?
>>>
>>> Resyncing with elimination of all the __packed__ stuff seems not to be
>>> a proper alternative as this would require a major rework.
>> 
>> Why? Replacing the existing horribleness with the canonical header (fixed 
> for style) might mean a large diff but it should be functionally the same or 
> something has gone very seriously wrong. If the extra part you need is not in 
> the canonical header then adding this as a second patch seems like a 
> reasonable plan. 
> 
> I think you don't realize that qemu is built using the public headers
> from the Xen build environment. So there is no way to resync with the
> canonical header as this isn't part of the qemu tree.

Oh, right, these are just the 32- and 64-bit variants which get
declared here.

> The header in question is originating from the Linux one which is an
> add-on of the canonical header containing the explicit 32- and 64-bit
> variants of the xenbus protocol and the conversion routines between
> those.
> 
> It would be possible to add these parts to the canonical header, but
> do we really want that?

Definitely not, I would say. But syncing with Linux'es strange
code then isn't a good idea either. (Still waiting for a qemu
maintainer opinion, though.)

Jan


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

Re: [Xen-devel] [PATCH 2/2] hvmloader: don't hard-code IO-APIC parameters

[Jan Beulich]

>>> On 17.06.16 at 12:05,  wrote:
> On 16/06/16 10:40, Jan Beulich wrote:
>> The IO-APIC address has variable bits determined by the PCI-to-ISA
>> bridge, and the IO-APIC version should be read from the IO-APIC. (Note
>> that there's still implicit rather than explicit agreement on the
>> IO-APIC base address between qemu and the hypervisor.)
>>
>> Signed-off-by: Jan Beulich 
> 
> The status quo is not great, and I can see why you want to improve it.
> 
> However, I think that this is not the way to do that.  It ties HVMLoader
> to the PIIX4 board in Qemu, and will break attempts to use Q35 or
> something else.  (In Q35, the IO-APIC decode address comes from Chipset
> Configuration Register, rather than ISA device config space).

hvmloader is already tied to PIIX4, and enabling Q35 will, among
other things, require hvmloader to become aware of that chipset.

Jan


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

[Xen-devel] [PATCH] xen: sched: use default scheduler upon an invalid "sched="

[Dario Faggioli]

instead of just the first scheduler we find in the array.

In fact, right now, if someone makes a typo when passing
the "sched=" command line option to Xen, we (with all
schedulers configured in) pick ARINC653, which is most
likely not what one would expect.

Go for the default scheduler instead.

Signed-off-by: Dario Faggioli 
---
Cc: George Dunlap 
Cc: Jan Beulich 
Cc: Doug Goldstein 
Cc: Jonathan Creekmore 
---
I don't think I'm going to ask for this to be put in 4.7,
as I don't want to give Wei an heart attack... :-)

Considering it for backporting should be enough, IMO.
---
 xen/common/schedule.c |3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index 5e35310..7ac12d3 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -1625,7 +1625,8 @@ void __init scheduler_init(void)
 {
 printk("Could not find scheduler: %s\n", opt_sched);
 for ( i = 0; i < NUM_SCHEDULERS; i++ )
-if ( schedulers[i] )
+if ( schedulers[i] &&
+ !strcmp(schedulers[i]->opt_name, CONFIG_SCHED_DEFAULT) )
 {
 ops = *schedulers[i];
 break;


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

Re: [Xen-devel] [PATCH v3 07/16] x86/boot: create *.lnk files with linker script

[Daniel Kiper]

On Fri, Jun 17, 2016 at 04:04:20AM -0600, Jan Beulich wrote:
> >>> On 17.06.16 at 11:06,  wrote:
> > On Tue, May 24, 2016 at 06:52:39AM -0600, Jan Beulich wrote:
> >> >>> On 24.05.16 at 14:28,  wrote:
> >> > On Tue, May 24, 2016 at 03:05:06AM -0600, Jan Beulich wrote:
> >> >> >>> On 15.04.16 at 14:33,  wrote:
> >> >> > +  /DISCARD/ : {
> >> >> > +/*
> >> >> > + * .got.plt section is used only by dynamic linker
> >> >> > + * and our output is not supposed to be loaded by
> >> >> > + * dynamic linker. Additionally, it just contains
> >> >> > + * .PLT0 which is referenced from nowhere. So, we
> >> >> > + * can safely drop .got.plt here.
> >> >> > + *
> >> >> > + * Ha! This should be really discarded here. However,
> >> >> > + * .got.plt section contains _GLOBAL_OFFSET_TABLE_
> >> >> > + * symbol too and it is used as a reference for relative
> >> >> > + * addressing (and only for that thing). Hence, ld
> >> >> > + * complains if we remove that section because it
> >> >> > + * cannot find _GLOBAL_OFFSET_TABLE_. So, drop .got.plt
> >> >> > + * section during conversion to plain binary format.
> >> >> > + * Please check build32.mk for more details.
> >> >> > + */
> >> >> > +/* *(.got.plt) */
> >> >> > +  }
> >> >>
> >> >> I'm afraid this needs more investigation: Afaik there should be no
> >> >
> >> > I am not sure what else we should look for.
> >>
> >> The reason why such an empty .got.plt gets created in the first place.
> >> If e.g. that turns out to be a bug in (some versions of) binutils, then
> >> that bug should be named here as the reason.
> >
> > If PIC/PIE code is build then .got.plt exists in executable even if it
> > is not linked with dynamic libraries.
>
> Well - then just don't force -fPIC or -fPIE for the compilation of this
> code?

No way. Then final code is not relocatable. And this code must be relocatable.

> >> >> reason for the linker to create an otherwise empty .got.plt in the
> >> >
> >> > As I wrote above. It contains _GLOBAL_OFFSET_TABLE_ which is used
> >> > as a reference for relative addressing.
> >>
> >> But we don't use any such, so without being needed I don't think
> >> the symbol needs to be created.
> >
> > R_386_GOTPC and R_386_GOTOFF relocations use address of
> > _GLOBAL_OFFSET_TABLE_
> > as a reference. So, it is needed during linking phase. However, later it is
> > not needed.  Hence, .got.plt with _GLOBAL_OFFSET_TABLE_ and .PLT0 can safely
> > be dropped.
>
> These two relocation types should not appear for non-PIC/PIE code.

Sure but as above.

Daniel

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

Re: [Xen-devel] [PATCH 7/7] vm-event/arm: implement support for control-register write vm-events

[Corneliu ZUZU]

On 6/16/2016 7:49 PM, Julien Grall wrote:

Hello Corneliu,

On 16/06/16 15:13, Corneliu ZUZU wrote:

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 8c50685..af61ac3 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -43,6 +43,7 @@
  #include 
  #include 
  #include 
+#include 

  #include "decode.h"
  #include "vtimer.h"
@@ -124,7 +125,12 @@ void init_traps(void)
  WRITE_SYSREG((HCPTR_CP_MASK & ~(HCPTR_CP(10) | HCPTR_CP(11))) | 
HCPTR_TTA,

   CPTR_EL2);

-/* Setup hypervisor traps */
+/* Setup hypervisor traps
+ *
+ * Note: HCR_TVM bit is also set for system-register write 
monitoring
+ * purposes (see vm_event_monitor_cr), but (for performance 
reasons) that's

+ * done selectively (see vcpu_enter_adjust_traps).
+ */
WRITE_SYSREG(HCR_PTW|HCR_BSU_INNER|HCR_AMO|HCR_IMO|HCR_FMO|HCR_VM|
HCR_TWE|HCR_TWI|HCR_TSC|HCR_TAC|HCR_SWIO|HCR_TIDCP|HCR_FB,
   HCR_EL2);
@@ -1720,6 +1726,7 @@ static void do_cp15_32(struct cpu_user_regs *regs,
  const struct hsr_cp32 cp32 = hsr.cp32;
  int regidx = cp32.reg;
  struct vcpu *v = current;
+register_t r = get_user_reg(regs, regidx);

  if ( !check_conditional_instr(regs, hsr) )
  {
@@ -1730,6 +1737,61 @@ static void do_cp15_32(struct cpu_user_regs 
*regs,

  switch ( hsr.bits & HSR_CP32_REGS_MASK )
  {
  /*
+ * HCR_EL2.TVM / HCR.TVM
+ *
+ * ARMv7 (DDI 0406C.b): B1.14.13


Please try to mention the most recent spec. This was published in 
2012, the latest release (C.c) was published in 2014.


Ack.




+ * ARMv8 (DDI 0487A.e): D1-1569 Table D1-34


Ditto. This was published in 2014, whilst the most recent was 
published in 2016.



+ */
+case HSR_CPREG32(SCTLR):
+TVM_EMUL_VMEVT(v, regs, hsr, r, SCTLR);
+break;
+case HSR_CPREG32(TTBR0_32):
+TVM_EMUL_VMEVT(v, regs, hsr, r, TTBR0_32);
+break;
+case HSR_CPREG32(TTBR1_32):
+TVM_EMUL_VMEVT(v, regs, hsr, r, TTBR1_32);
+break;
+case HSR_CPREG32(TTBCR):
+TVM_EMUL_VMEVT(v, regs, hsr, r, TTBCR);
+break;
+case HSR_CPREG32(DACR):
+TVM_EMUL(regs, hsr, r, DACR);
+break;
+case HSR_CPREG32(DFSR):
+TVM_EMUL(regs, hsr, r, DFSR);
+break;
+case HSR_CPREG32(IFSR):
+TVM_EMUL(regs, hsr, r, IFSR);
+break;
+case HSR_CPREG32(DFAR):
+TVM_EMUL(regs, hsr, r, DFAR);
+break;
+case HSR_CPREG32(IFAR):
+TVM_EMUL(regs, hsr, r, IFAR);
+break;

[...]

+case HSR_CPREG32(ADFSR):
+TVM_EMUL(regs, hsr, r, ADFSR);
+break;
+case HSR_CPREG32(AIFSR):
+TVM_EMUL(regs, hsr, r, AIFSR);
+break;
+case HSR_CPREG32(MAIR0):
+TVM_EMUL(regs, hsr, r, MAIR0);
+break;
+case HSR_CPREG32(MAIR1):
+TVM_EMUL(regs, hsr, r, MAIR1);
+break;


Please mention that PRRR and NMRR are aliased to respectively MAIR0 
and MAIR1. This will avoid to spend time trying to understanding why 
the spec says they are trapped but you don't "handle" them.


I mentioned that in traps.h (see "AKA" comments). Will put the same 
comment here then.





+case HSR_CPREG32(AMAIR0):
+TVM_EMUL(regs, hsr, r, AMAIR0);
+break;
+case HSR_CPREG32(AMAIR1):
+TVM_EMUL(regs, hsr, r, AMAIR1);
+break;
+case HSR_CPREG32(CONTEXTIDR):
+TVM_EMUL(regs, hsr, r, CONTEXTIDR);
+break;
+
+/*
   * !CNTHCTL_EL2.EL1PCEN / !CNTHCTL.PL1PCEN
   *
   * ARMv7 (DDI 0406C.b): B4.1.22
@@ -1853,6 +1915,13 @@ static void do_cp15_32(struct cpu_user_regs 
*regs,

  static void do_cp15_64(struct cpu_user_regs *regs,
 const union hsr hsr)
  {
+struct vcpu *v = current;
+const struct hsr_cp64 cp64 = hsr.cp64;
+sysreg64_t r = {
+.low32 = (uint32_t) get_user_reg(regs, cp64.reg1),


The cast is not necessary.


Ack.




+.high32 = (uint32_t) get_user_reg(regs, cp64.reg2)


Ditto.


+};
+
  if ( !check_conditional_instr(regs, hsr) )
  {
  advance_pc(regs, hsr);
@@ -1862,6 +1931,19 @@ static void do_cp15_64(struct cpu_user_regs 
*regs,

  switch ( hsr.bits & HSR_CP64_REGS_MASK )
  {
  /*
+ * HCR_EL2.TVM / HCR.TVM
+ *
+ * ARMv7 (DDI 0406C.b): B1.14.13
+ * ARMv8 (DDI 0487A.e): D1-1569 Table D1-34


Same remarks as above for the spec version.


+ */
+case HSR_CPREG64(TTBR0):
+TVM_EMUL_VMEVT(v, regs, hsr, r.val64, TTBR0_64);
+break;
+case HSR_CPREG64(TTBR1):
+TVM_EMUL_VMEVT(v, regs, hsr, r.val64, TTBR1_64);
+break;
+
+/*
   * !CNTHCTL_EL2.EL1PCEN / !CNTHCTL.PL1PCEN
   *
   * ARMv7 (DDI 0406C.b): B4.1.22
@@ -1891,8 +1973,6 @@ static void do_cp15_64(struct cpu_user_regs *regs,
   */
  default:
  {
-const struct hsr_cp64 cp64 = hsr.cp64;
-
  gdprintk(XENLOG_ERR,
   "%s p15, %d, r

Re: [Xen-devel] [PATCH 6/7] vm-event/arm: move hvm_event_cr->common vm_event_monitor_cr

[Corneliu ZUZU]

On 6/16/2016 7:55 PM, Tamas K Lengyel wrote:

On Thu, Jun 16, 2016 at 8:12 AM, Corneliu ZUZU  wrote:

Prepare for ARM implementation of control-register write vm-events by moving
X86-specific hvm_event_cr to the common-side.

Signed-off-by: Corneliu ZUZU 
---
  xen/arch/x86/hvm/event.c| 30 --
  xen/arch/x86/hvm/hvm.c  |  2 +-
  xen/arch/x86/monitor.c  | 37 -
  xen/arch/x86/vm_event.c |  2 +-
  xen/common/monitor.c| 40 
  xen/common/vm_event.c   | 31 +++
  xen/include/asm-x86/hvm/event.h | 13 -
  xen/include/asm-x86/monitor.h   |  2 --
  xen/include/xen/monitor.h   |  4 
  xen/include/xen/vm_event.h  | 10 ++
  10 files changed, 91 insertions(+), 80 deletions(-)

Some of the code-movement here touches code I cleaned up in
https://patchwork.kernel.org/patch/9151229 to keep vm_event and
monitor code separate as much as possible. The same separation should
be followed when we move code to common.

Tamas



Again, that didn't make it to staging. We'll see which gets in first and 
merge afterwards?


Corneliu.

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

Re: [Xen-devel] Question of xl dump-core

[Daniel Kiper]

On Wed, Jun 15, 2016 at 09:46:45AM +0800, wj zhou wrote:
> Hi,
>
> Thanks a lot for your reply!
>
> On Tue, Jun 14, 2016 at 11:02 PM, Konrad Rzeszutek Wilk
>  wrote:
> > On Tue, Jun 14, 2016 at 08:21:16AM +0800, wj zhou wrote:
> >> Hello all,
> >
> > Hey,
> >
> > CC-ing Daniel, and Dave.
> >>
> >> Sorry to disturb you, but I really want to figure it out.
> >> The xen core of redhat 6 with pod is unable to be used with crash.
> >>
> >> I installed a hvm of redhat 6 by xen 4.7.0-rc2.
> >> And the memory is set as below:
> >> memory=1024
> >> maxmem=4096
> >>
> >> "xl dump-core" is executed, and the core is produced successfully.
> >> I got the following message:
> >> xc: info: exceeded nr_pages (26) losing pages
> >>
> >> Unfortunately, I got some errors when executing crash with it.
> >> The below is the log of crash.
> >>
> >> 
> >> crash 7.0.9-4.el7
> >
> > http://people.redhat.com/anderson says that the latest is 7.1.5.
> > Can you try that version?
> >
>
> I have just tried the latest crash version, and got the same error message.
> Since the following info exists, I think there is something wrong in xen.
> xc: info: exceeded nr_pages (26) losing pages
>
> >> ...
> >>
> >> please wait... (gathering kmem slab cache data)
> >> crash: read error: kernel virtual address: 88010b532e00  type:
> >> "kmem_cache buffer"
> >>
> >> crash: unable to initialize kmem slab cache subsystem
> >>
> >> please wait... (gathering module symbol data)
> >> crash: read error: physical address: 1058a1000  type: "page table"
> >> 
> >>
> >> I knew balloon is not supported by redhat 6, so pod is also not supported.
> >
> > ?
> >
>
> In redhat 6 hvm, there is no balloon module.
>
> >> But I wonder the reason why the above error happens.
> >> Balloon and pod are also not supported by redhat 7, but it won't
> >> happen in redhat 7 hvm.
> >> I'm very appreciated if someone can help me.
> >
> > Well, does it work if you maxmem != memory ?
> >
>
> Yes, it works in redhat 7 hvm when maxmem>memory.
>
> But I found something strange.
> "free -m" performs quite different between redhat6 and redhat7 hvm.
> In redhat 6 the value by "free -m" equals maxmem(4096).
> However, in redhat 7 it equals memory(1024),

Sadly I am not able to play with this issue right know.
However, I could review patches. So, If you wish to do
that I think that you should first of all investigate
xl issue described earlier and then look at crash code.

Daniel

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

Re: [Xen-devel] [PATCH v2] xen: fix qdisk BLKIF_OP_DISCARD for 32/64 word size mix

[Juergen Gross]

On 17/06/16 12:15, Paul Durrant wrote:
>> -Original Message-
>> From: Xen-devel [mailto:xen-devel-boun...@lists.xen.org] On Behalf Of
>> Juergen Gross
>> Sent: 17 June 2016 11:08
>> To: Paul Durrant; Jan Beulich
>> Cc: Anthony Perard; xen-devel; sstabell...@kernel.org; qemu-
>> de...@nongnu.org; kra...@redhat.com
>> Subject: Re: [Xen-devel] [PATCH v2] xen: fix qdisk BLKIF_OP_DISCARD for
>> 32/64 word size mix
>>
>> On 17/06/16 11:50, Paul Durrant wrote:
 -Original Message-
 From: Juergen Gross [mailto:jgr...@suse.com]
 Sent: 17 June 2016 10:46
 To: Paul Durrant; Jan Beulich
 Cc: Anthony Perard; xen-devel; sstabell...@kernel.org; qemu-
 de...@nongnu.org; kra...@redhat.com
 Subject: Re: [Xen-devel] [PATCH v2] xen: fix qdisk BLKIF_OP_DISCARD for
 32/64 word size mix

 On 17/06/16 11:37, Paul Durrant wrote:
>> -Original Message-
>> From: Xen-devel [mailto:xen-devel-boun...@lists.xen.org] On Behalf
>> Of
 Jan
>> Beulich
>> Sent: 17 June 2016 10:26
>> To: Juergen Gross
>> Cc: Anthony Perard; xen-devel; sstabell...@kernel.org; qemu-
>> de...@nongnu.org; kra...@redhat.com
>> Subject: Re: [Xen-devel] [PATCH v2] xen: fix qdisk BLKIF_OP_DISCARD
>> for
>> 32/64 word size mix
>>
> On 17.06.16 at 11:14,  wrote:
>>> In case the word size of the domU and qemu running the qdisk
>> backend
>>> differ BLKIF_OP_DISCARD will not work reliably, as the request
>>> structure in the ring have different layouts for different word size.
>>>
>>> Correct this by copying the request structure in case of different
>>> word size element by element in the BLKIF_OP_DISCARD case, too.
>>>
>>> The easiest way to achieve this is to resync hw/block/xen_blkif.h with
>>> its original source from the Linux kernel.
>>>
>>> Signed-off-by: Juergen Gross 
>>> ---
>>> V2: resync with Linux kernel version of hw/block/xen_blkif.h as
>>> suggested by Paul Durrant
>>
>> Oh, I didn't realize he suggested syncing with the Linux variant.
>> Why not with the canonical one? I have to admit that I particularly
>> dislike Linux'es strange union-izng, mainly because of it requiring
>> this myriad of __attribute__((__packed__)).
>>
>
> Yes, it's truly grotesque and such things should be blown away with
 extreme prejudice.

 Sorry, I'm confused now.

 Do you still mandate for the resync or not?

 Resyncing with elimination of all the __packed__ stuff seems not to be
 a proper alternative as this would require a major rework.
>>>
>>> Why? Replacing the existing horribleness with the canonical header (fixed
>> for style) might mean a large diff but it should be functionally the same or
>> something has gone very seriously wrong. If the extra part you need is not in
>> the canonical header then adding this as a second patch seems like a
>> reasonable plan.
>>
>> I think you don't realize that qemu is built using the public headers
>> from the Xen build environment. So there is no way to resync with the
>> canonical header as this isn't part of the qemu tree.
>>
> 
> Now I'm confused... you're posting a patch to hw/block/xen_blkif.h. That's in 
> the QEMU source, right? That's not a Xen public header but is a Linux mangled 
> variant of a Xen public header. So, actually, I guess the question is why 
> can't this header just go away and QEMU use the canonical header directly 
> from Xen?

No, hw/block/xen_blkif.h is based on the Linux header
drivers/block/xen-blkback/common.h which is an add-on header to the
canonical-based Linux header include/xen/interface/io/blkif.h

>> The header in question is originating from the Linux one which is an
>> add-on of the canonical header containing the explicit 32- and 64-bit
>> variants of the xenbus protocol and the conversion routines between
>> those.
>>
>> It would be possible to add these parts to the canonical header, but
>> do we really want that?
>>
> 
> No, we shouldn't be taking Linux brokenness into the canonical header.

Okay, so then back to the first approach using hw/block/xen_blkif.h as
today and adapting the style first and then doing the necessary code
correction?


Juergen


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

Re: [Xen-devel] [PATCH 3/7] vm-event: introduce vm_event_vcpu_enter

[Corneliu ZUZU]

On 6/16/2016 11:33 PM, Razvan Cojocaru wrote:

On 06/16/16 23:10, Corneliu ZUZU wrote:

On 6/16/2016 5:51 PM, Jan Beulich wrote:

On 16.06.16 at 16:08,  wrote:

@@ -509,6 +508,8 @@ void hvm_do_resume(struct vcpu *v)
   }
   }
   +vm_event_vcpu_enter(v);

Why here?

Why indeed. It made sense because monitor_write_data handling was
originally there and then the plan was to move it to vm_event_vcpu_enter
(which happens in the following commit).
The question is though, why was monitor_write_data handled there in the
first place? Why was it not put e.g. in vmx_do_resume immediately after
the call to hvm_do_resume and just before
the reset_stack_and_jump...? And what happens with handling
monitor_write_data if this:

if ( !handle_hvm_io_completion(v) )
 return;

causes a return?

It's in hvm_do_resume() because, for one, that's the place that was
suggested (or at least confirmed when I've proposed it for such things)
on this list back when I wrote the code. And then it's here because
vmx_do_things()-type functions are, well, VMX, and I had hoped that by
choosing hvm-prefixed functions I'd get SVM support for free.

As for the handle_hvm_io_completion(v) return, my understanding was that
that would eventually cause another exit, and eventually we'd get to the
code below once the IO part is done.


Thanks,
Razvan


Thanks, so then indeed the vm_event_vcpu_enter call should be there to 
avoid wrongfully calling it many times before actually entering the vCPU 
(due to IO).
I wonder though if anything wrong would happen if I put the call after 
the "inject pending hw/sw trap" part.


Corneliu.

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

Re: [Xen-devel] [PATCH 00/17] Honour more configure variables in various places (iteration 2)

[Wei Liu]

On Tue, Jun 14, 2016 at 02:17:38PM +0100, Wei Liu wrote:
> 
> >   docs: honour XEN_DUMP_DIR
> 
> This is acked but we would like to wait a bit.
> 
> >   build: introduce XEN_RUN_STORED
> 
> This is not yet acked.
> 
> >   hotplug/Linux: honour XEN_RUN_STORED
> >   libxenstore: honour XEN_RUN_STORED
> >   hotplug/FreeBSD: honour XEN_RUN_STORED
> 
> These are acked but depend on the introduction of XEN_RUN_STORED.
> 
> >   ocaml/libxs: generate a paths.ml
> >   ocaml/libxs: honour XEN_RUN_STORED
> >   oxenstored: honour XEN_RUN_STORED and XEN_CONFIG_DIR
> >   oxenstored: honour XEN_RUN_STORED in systemd C stub
> 

I've pushed the remaining patches to staging.

Wei.

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

Re: [Xen-devel] [PATCH] libxl: correct xl cpupool-numa-split with vcpu limited dom0

[Wei Liu]

On Tue, Jun 14, 2016 at 12:05:35PM +0200, Juergen Gross wrote:
> On 14/06/16 11:58, Wei Liu wrote:
> > On Tue, Jun 14, 2016 at 10:56:52AM +0100, Wei Liu wrote:
> >> On Tue, Jun 14, 2016 at 11:01:50AM +0200, Dario Faggioli wrote:
> >>> On Tue, 2016-06-14 at 06:30 +0200, Juergen Gross wrote:
>  When trying to use xl cpupool-numa-split and dom0 is limited to less
>  vcpus than one numa node the operation will fail.
> 
>  Correct this by allowing this configuration.
> 
>  Reported-by: Glenn Enright 
>  Signed-off-by: Juergen Gross 
> 
> >>> Reviewed-by: Dario Faggioli 
> >>>
> >>
> >> Acked-by: Wei Liu 
> >>
> > 
> > BTW I will change the "libxl" to "xl" in patch title while committing if
> > you don't object.
> 
> NP.

Pushed to staging with updated subject line.

Wei.

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

Re: [Xen-devel] [PATCH 1/7] minor (formatting) fixes

[Corneliu ZUZU]

On 6/17/2016 10:06 AM, Jan Beulich wrote:

On 16.06.16 at 21:19,  wrote:

On 6/16/2016 5:24 PM, Jan Beulich wrote:

On 16.06.16 at 16:06,  wrote:

--- a/xen/arch/x86/hvm/event.c
+++ b/xen/arch/x86/hvm/event.c
@@ -23,6 +23,7 @@
   
   #include 

   #include 
+#include 
   #include 
   #include 
   #include 
diff --git a/xen/common/monitor.c b/xen/common/monitor.c
index d950a7c..b30857a 100644
--- a/xen/common/monitor.c
+++ b/xen/common/monitor.c
@@ -22,7 +22,6 @@
   #include 
   #include 
   #include 
-#include 
   #include 
   #include 

These two adjustments clearly don't fit title / description. I certainly
don't mind unnecessary inclusions to be dropped, but the addition of
one clearly needs explanation (after all thing build fine without it).

Sorry, that was done out of reflex, should have stated the reasoning.
Generally, if:
- event.c includes event.h
- event.c needs paging.h
- event.h -doesn't need- paging.h
then I prefer to include paging.h in event.c, not in event.h (include
strictly -where- needed).
Also since xen/paging.h included asm/paging.h and event.c only needs the
asm/paging.h part, I also changed xen/paging.h inclusion -> asm/paging.h
inclusion (include strictly -what's- needed).
But I can revert that if you want me to (not that important and these
little things are better done with automatic tools anyway), or should I
leave the change and update the commit message?

Well, I'd leave the ultimate decision to the maintainers, but I'd say
this doesn't belong in a patch dealing with formatting issues. I.e.
I'm fine with the cleanup, but either under a different title (and
with at least an abbreviated version of what you said above), or
in a separate patch. Considering that you appear to do the same
in later patches, perhaps consolidating all the #include adjustments
into one patch would a good idea?


I tend to prefer organizing what's easy to understand/minor first (to 
let reviewers get rid of those in a flash), so I'd prefer to leave it in 
this patch with the other minor fixes and update the commit message instead.

But either way it's fine by me.


--- a/xen/include/xen/vm_event.h
+++ b/xen/include/xen/vm_event.h
@@ -85,7 +85,6 @@ void vm_event_monitor_guest_request(void);
   
   #endif /* __VM_EVENT_H__ */
   
-

   /*
* Local variables:
* mode: C

Why don't you remove the other stray blank line at once?

What stray line? Shouldn't there be -one- blank line between the #endif
and the local vars block?
Looking @ other files that rule seems to hold...

Oh, you're right. I thought I had frequently seen it the other way
around (and it would seem more logical to me), but I must have been
misremembering.

Jan




Thanks,
Corneliu.

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

Re: [Xen-devel] [PATCH RESEND 01/14] libxl/arm: Fix the function name in error log

[Wei Liu]

On Fri, Jun 03, 2016 at 08:25:05PM +0100, Wei Liu wrote:
> On Tue, May 31, 2016 at 01:02:53PM +0800, Shannon Zhao wrote:
> > From: Shannon Zhao 
> > 
> > It should be xc_dom_devicetree_mem instead of xc_dom_devicetree_file.
> > 
> > Signed-off-by: Shannon Zhao 
> 
> Acked-by: Wei Liu 
> 

I've pushed this to staging to reduce the length of your patch queue.

Wei.

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

Re: [Xen-devel] [PATCH v2] xen: fix qdisk BLKIF_OP_DISCARD for 32/64 word size mix

[Paul Durrant]

> -Original Message-
> From: Juergen Gross [mailto:jgr...@suse.com]
> Sent: 17 June 2016 11:40
> To: Paul Durrant; Jan Beulich
> Cc: Anthony Perard; xen-devel; sstabell...@kernel.org; qemu-
> de...@nongnu.org; kra...@redhat.com
> Subject: Re: [Xen-devel] [PATCH v2] xen: fix qdisk BLKIF_OP_DISCARD for
> 32/64 word size mix
> 
> On 17/06/16 12:15, Paul Durrant wrote:
> >> -Original Message-
> >> From: Xen-devel [mailto:xen-devel-boun...@lists.xen.org] On Behalf Of
> >> Juergen Gross
> >> Sent: 17 June 2016 11:08
> >> To: Paul Durrant; Jan Beulich
> >> Cc: Anthony Perard; xen-devel; sstabell...@kernel.org; qemu-
> >> de...@nongnu.org; kra...@redhat.com
> >> Subject: Re: [Xen-devel] [PATCH v2] xen: fix qdisk BLKIF_OP_DISCARD for
> >> 32/64 word size mix
> >>
> >> On 17/06/16 11:50, Paul Durrant wrote:
>  -Original Message-
>  From: Juergen Gross [mailto:jgr...@suse.com]
>  Sent: 17 June 2016 10:46
>  To: Paul Durrant; Jan Beulich
>  Cc: Anthony Perard; xen-devel; sstabell...@kernel.org; qemu-
>  de...@nongnu.org; kra...@redhat.com
>  Subject: Re: [Xen-devel] [PATCH v2] xen: fix qdisk BLKIF_OP_DISCARD
> for
>  32/64 word size mix
> 
>  On 17/06/16 11:37, Paul Durrant wrote:
> >> -Original Message-
> >> From: Xen-devel [mailto:xen-devel-boun...@lists.xen.org] On
> Behalf
> >> Of
>  Jan
> >> Beulich
> >> Sent: 17 June 2016 10:26
> >> To: Juergen Gross
> >> Cc: Anthony Perard; xen-devel; sstabell...@kernel.org; qemu-
> >> de...@nongnu.org; kra...@redhat.com
> >> Subject: Re: [Xen-devel] [PATCH v2] xen: fix qdisk
> BLKIF_OP_DISCARD
> >> for
> >> 32/64 word size mix
> >>
> > On 17.06.16 at 11:14,  wrote:
> >>> In case the word size of the domU and qemu running the qdisk
> >> backend
> >>> differ BLKIF_OP_DISCARD will not work reliably, as the request
> >>> structure in the ring have different layouts for different word size.
> >>>
> >>> Correct this by copying the request structure in case of different
> >>> word size element by element in the BLKIF_OP_DISCARD case, too.
> >>>
> >>> The easiest way to achieve this is to resync hw/block/xen_blkif.h
> with
> >>> its original source from the Linux kernel.
> >>>
> >>> Signed-off-by: Juergen Gross 
> >>> ---
> >>> V2: resync with Linux kernel version of hw/block/xen_blkif.h as
> >>> suggested by Paul Durrant
> >>
> >> Oh, I didn't realize he suggested syncing with the Linux variant.
> >> Why not with the canonical one? I have to admit that I particularly
> >> dislike Linux'es strange union-izng, mainly because of it requiring
> >> this myriad of __attribute__((__packed__)).
> >>
> >
> > Yes, it's truly grotesque and such things should be blown away with
>  extreme prejudice.
> 
>  Sorry, I'm confused now.
> 
>  Do you still mandate for the resync or not?
> 
>  Resyncing with elimination of all the __packed__ stuff seems not to be
>  a proper alternative as this would require a major rework.
> >>>
> >>> Why? Replacing the existing horribleness with the canonical header
> (fixed
> >> for style) might mean a large diff but it should be functionally the same 
> >> or
> >> something has gone very seriously wrong. If the extra part you need is
> not in
> >> the canonical header then adding this as a second patch seems like a
> >> reasonable plan.
> >>
> >> I think you don't realize that qemu is built using the public headers
> >> from the Xen build environment. So there is no way to resync with the
> >> canonical header as this isn't part of the qemu tree.
> >>
> >
> > Now I'm confused... you're posting a patch to hw/block/xen_blkif.h. That's
> in the QEMU source, right? That's not a Xen public header but is a Linux
> mangled variant of a Xen public header. So, actually, I guess the question is
> why can't this header just go away and QEMU use the canonical header
> directly from Xen?
> 
> No, hw/block/xen_blkif.h is based on the Linux header
> drivers/block/xen-blkback/common.h which is an add-on header to the
> canonical-based Linux header include/xen/interface/io/blkif.h
>
> >> The header in question is originating from the Linux one which is an
> >> add-on of the canonical header containing the explicit 32- and 64-bit
> >> variants of the xenbus protocol and the conversion routines between
> >> those.
> >>
> >> It would be possible to add these parts to the canonical header, but
> >> do we really want that?
> >>
> >
> > No, we shouldn't be taking Linux brokenness into the canonical header.
> 
> Okay, so then back to the first approach using hw/block/xen_blkif.h as
> today and adapting the style first and then doing the necessary code
> correction?
> 

I guess re-syncing with the Linux header as in your v2 patch is the least worst 
option then.

  Paul

> 
> Juergen

___
Xen-devel mailin

Re: [Xen-devel] [PATCH] xen: sched: use default scheduler upon an invalid "sched="

[Andrew Cooper]

On 17/06/16 11:31, Dario Faggioli wrote:
> instead of just the first scheduler we find in the array.
>
> In fact, right now, if someone makes a typo when passing
> the "sched=" command line option to Xen, we (with all
> schedulers configured in) pick ARINC653, which is most
> likely not what one would expect.
>
> Go for the default scheduler instead.
>
> Signed-off-by: Dario Faggioli 
> ---
> Cc: George Dunlap 
> Cc: Jan Beulich 
> Cc: Doug Goldstein 
> Cc: Jonathan Creekmore 

Reviewed-by: Andrew Cooper 

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

[Xen-devel] [PATCH v2 0/2] Make hvm_fep available to non-debug build

[Wei Liu]

Wei Liu (2):
  xen/kernel: document 'C' in print_tainted
  xen: make available hvm_fep to non-debug build as well

 docs/misc/xen-command-line.markdown |  8 ++--
 xen/arch/x86/Kconfig| 14 ++
 xen/arch/x86/hvm/hvm.c  | 28 ++--
 xen/common/kernel.c |  7 +--
 xen/include/asm-x86/hvm/hvm.h   |  2 +-
 xen/include/xen/lib.h   |  1 +
 6 files changed, 53 insertions(+), 7 deletions(-)

-- 
2.1.4


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

[Xen-devel] [PATCH v2 2/2] xen: make available hvm_fep to non-debug build as well

[Wei Liu]

Originally hvm_fep was guarded by NDEBUG, which means it was only
available to debug builds.

However there is value to have it for non-debug builds as well. User can
use that to run tests in setup that replicates production setup.

Make it clear with a sync_console style warning that this option can't
be used in production setup. Update command line documentation
accordingly. Finally mark Xen as tainted when this option is enabled.

Add a kconfig option under x86 to configure hvm_fep.

Signed-off-by: Wei Liu 
---
Cc: Andrew Cooper 
Cc: Jan Beulich 
Cc: Doug Goldstein 

v2:
1. unsigned -> unsigned int
2. %d -> %u
3. Add spaces around "-"
4. Update warning message
5. Only taint hv when fep is used
6. Add kconfig option
---
 docs/misc/xen-command-line.markdown |  8 ++--
 xen/arch/x86/Kconfig| 14 ++
 xen/arch/x86/hvm/hvm.c  | 28 ++--
 xen/common/kernel.c |  6 --
 xen/include/asm-x86/hvm/hvm.h   |  2 +-
 xen/include/xen/lib.h   |  1 +
 6 files changed, 52 insertions(+), 7 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown 
b/docs/misc/xen-command-line.markdown
index fed732c..dc53e24 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -878,8 +878,12 @@ Recognized in debug builds of the hypervisor only.
 Allow use of the Forced Emulation Prefix in HVM guests, to allow emulation of
 arbitrary instructions.
 
-This option is intended for development purposes, and is only available in
-debug builds of the hypervisor.
+This option is intended for development and testing purposes.
+
+*Warning*
+As this feature opens up the instruction emulator to HVM guest, don't
+use this in production system. No security support is provided when
+this flag is set.
 
 ### hvm\_port80
 > `= `
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 73f79cc..5e3b04a 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -59,6 +59,20 @@ config BIGMEM
 
  If unsure, say N.
 
+config HVM_FEP
+   bool "HVM Forced Emulation Prefix support"
+   default y
+   ---help---
+
+ Compiles in a feature that allows HVM guest to enter
+ instruction emulator with forced emulation prefix.
+
+ This feature can only be enabled during boot time with
+ appropriate hypervisor command line option. Please read
+ hypervisor command line documentation before trying to use
+ this feature.
+
+ If unsure, say Y.
 endmenu
 
 source "common/Kconfig"
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 78db903..373b78e 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -37,6 +37,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -95,9 +96,9 @@ unsigned long __section(".bss.page_aligned")
 static bool_t __initdata opt_hap_enabled = 1;
 boolean_param("hap", opt_hap_enabled);
 
-#ifndef opt_hvm_fep
+#if CONFIG_HVM_FEP
 /* Permit use of the Forced Emulation Prefix in HVM guests */
-bool_t opt_hvm_fep;
+bool_t __read_mostly opt_hvm_fep;
 boolean_param("hvm_fep", opt_hvm_fep);
 #endif
 
@@ -182,6 +183,28 @@ static int __init hvm_enable(void)
 if ( !opt_altp2m_enabled )
 hvm_funcs.altp2m_supported = 0;
 
+if ( opt_hvm_fep )
+{
+unsigned int i, j;
+
+printk("**\n");
+printk("*** WARNING: HVM FORCED EMULATION PREFIX IS AVAILABLE\n");
+printk("*** This option is *ONLY* intended to aid testing of 
Xen.\n");
+printk("*** It has implications on the security of the system.\n");
+printk("*** Please *DO NOT* use this in production.\n");
+printk("**\n");
+for ( i = 0; i < 3; i++ )
+{
+printk("%u... ", 3 - i);
+for ( j = 0; j < 100; j++ )
+{
+process_pending_softirqs();
+mdelay(10);
+}
+}
+printk("\n");
+}
+
 /*
  * Allow direct access to the PC debug ports 0x80 and 0xed (they are
  * often used for I/O delays, but the vmexits simply slow things down).
@@ -3905,6 +3928,7 @@ void hvm_ud_intercept(struct cpu_user_regs *regs)
 {
 regs->eip += sizeof(sig);
 regs->eflags &= ~X86_EFLAGS_RF;
+add_taint(TAINT_HVM_FEP);
 }
 }
 
diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index dae7e35..5bf77aa 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -175,6 +175,7 @@ int __init parse_bool(const char *s)
  *  'M' - Machine had a machine check experience.
  *  'B' - System has hit bad_page.
  *  'C' - Console output is synchronous.
+ *  'H' - HVM forced emulation prefix is permitted.
  *
  *  The string is overwritten by the next call to print_taint().
  */
@@ -182,11 +183,12 @@ char *print_tainted(char *str)
 {

[Xen-devel] [PATCH v2 1/2] xen/kernel: document 'C' in print_tainted

[Wei Liu]

Signed-off-by: Wei Liu 
Acked-by: Jan Beulich 
---
Cc: Andrew Cooper 
Cc: George Dunlap 
Cc: Ian Jackson 
Cc: Jan Beulich 
Cc: Konrad Rzeszutek Wilk 
Cc: Stefano Stabellini 
Cc: Tim Deegan 
Cc: Wei Liu 
---
 xen/common/kernel.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index 1a6823a..dae7e35 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -174,6 +174,7 @@ int __init parse_bool(const char *s)
  *  'S' - SMP with CPUs not designed for SMP.
  *  'M' - Machine had a machine check experience.
  *  'B' - System has hit bad_page.
+ *  'C' - Console output is synchronous.
  *
  *  The string is overwritten by the next call to print_taint().
  */
-- 
2.1.4


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

Re: [Xen-devel] [PATCH 3/7] vm-event: introduce vm_event_vcpu_enter

[Corneliu ZUZU]

On 6/17/2016 10:17 AM, Jan Beulich wrote:

On 16.06.16 at 22:10,  wrote:

On 6/16/2016 5:51 PM, Jan Beulich wrote:

On 16.06.16 at 16:08,  wrote:

@@ -509,6 +508,8 @@ void hvm_do_resume(struct vcpu *v)
   }
   }
   
+vm_event_vcpu_enter(v);

Why here?

Why indeed. It made sense because monitor_write_data handling was
originally there and then the plan was to move it to vm_event_vcpu_enter
(which happens in the following commit).
The question is though, why was monitor_write_data handled there in the
first place? Why was it not put e.g. in vmx_do_resume immediately after
the call to hvm_do_resume and just before
the reset_stack_and_jump...? And what happens with handling
monitor_write_data if this:

if ( !handle_hvm_io_completion(v) )
  return;

causes a return?

I see Razvan responded to this. I don't have a strong opinion
either way, my only request if for the call to be in exactly one
place.


--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -36,7 +36,6 @@
   #include 
   #include 
   #include 
-#include 
   #include 

There are way too many of these #include adjustments here. If
you really mean to clean these up, please don't randomly throw
this into various unrelated patches.

I haven't thrown anything "randomly into unrelated patches", please
first ask for my reasoning and then draw such conclusions.

See patch 1.


"Sorry, that was done out of reflex, should have stated the reasoning."


Plus I don't think I (or in fact any reviewer) should ask
for such reasoning: Instead you should state extra cleanup you do
to unrelated (to the purpose of your patch) files in the description.


Is that still the case when that reasoning is obvious? (at least it 
seemed to me)


but anyway..


Or even better, split it off to a follow-on, purely cleanup patch.


I agree with this. Will keep in mind with v2.


(And to be clear, I much appreciate any form of reduction of the
sometimes extremely long lists of #include-s, just not [apparently
or really] randomly mixed with other, substantial changes. That's
namely because it's not clear whether source files should explicitly
include everything they need, or instead be allowed to rely on
headers they include to include further headers they also
_explicitly_ rely on.


Personally I prefer the former since I think it also cuts down 
compilation time.
Having header H include every header Ni needed by source S makes H 
unnecessarily bulky at compilation time for other sources <> S that 
don't need headers Ni but which depend on H nonetheless.



IOW there's likely a discussion to be had for
this kind of cleanup, and such a discussion should be a separate
thread from the one on the functional adjustments here.)


Corneliu.

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

[Xen-devel] [PATCH] xen: use native disk xenbus protocol if possible

[Juergen Gross]

The qdisk implementation is using the native xenbus protocol only in
case of no protocol specified at all. As using the explicit 32- or
64-bit protocol is slower than the native one due to copying requests
not by memcpy but element for element, this is not optimal.

Correct this by using the native protocol in case word sizes of
frontend and backend match.

Signed-off-by: Juergen Gross 
---
 hw/block/xen_disk.c | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
index 2862b59..0961fcb 100644
--- a/hw/block/xen_disk.c
+++ b/hw/block/xen_disk.c
@@ -976,14 +976,14 @@ static int blk_connect(struct XenDevice *xendev)
 blkdev->feature_persistent = !!pers;
 }
 
-blkdev->protocol = BLKIF_PROTOCOL_NATIVE;
-if (blkdev->xendev.protocol) {
-if (strcmp(blkdev->xendev.protocol, XEN_IO_PROTO_ABI_X86_32) == 0) {
-blkdev->protocol = BLKIF_PROTOCOL_X86_32;
-}
-if (strcmp(blkdev->xendev.protocol, XEN_IO_PROTO_ABI_X86_64) == 0) {
-blkdev->protocol = BLKIF_PROTOCOL_X86_64;
-}
+if (!blkdev->xendev.protocol) {
+blkdev->protocol = BLKIF_PROTOCOL_NATIVE;
+} else if (strcmp(blkdev->xendev.protocol, XEN_IO_PROTO_ABI_NATIVE) == 0) {
+blkdev->protocol = BLKIF_PROTOCOL_NATIVE;
+} else if (strcmp(blkdev->xendev.protocol, XEN_IO_PROTO_ABI_X86_32) == 0) {
+blkdev->protocol = BLKIF_PROTOCOL_X86_32;
+} else if (strcmp(blkdev->xendev.protocol, XEN_IO_PROTO_ABI_X86_64) == 0) {
+blkdev->protocol = BLKIF_PROTOCOL_X86_64;
 }
 
 blkdev->sring = xengnttab_map_grant_ref(blkdev->xendev.gnttabdev,
-- 
2.6.6


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

Re: [Xen-devel] [PATCH 4/7] vm-event/x86: use vm_event_vcpu_enter properly

[Corneliu ZUZU]

On 6/17/2016 10:20 AM, Jan Beulich wrote:

On 16.06.16 at 22:20,  wrote:

On 6/16/2016 6:00 PM, Jan Beulich wrote:

On 16.06.16 at 16:09,  wrote:

@@ -1432,18 +1430,16 @@ static void vmx_update_guest_cr(struct vcpu *v, 
unsigned int cr)
   if ( paging_mode_hap(v->domain) )
   {
   /* Manage GUEST_CR3 when CR0.PE=0. */
+uint32_t old_ctls = v->arch.hvm_vmx.exec_control;
   uint32_t cr3_ctls = (CPU_BASED_CR3_LOAD_EXITING |
CPU_BASED_CR3_STORE_EXITING);
+
   v->arch.hvm_vmx.exec_control &= ~cr3_ctls;
   if ( !hvm_paging_enabled(v) && !vmx_unrestricted_guest(v) )
   v->arch.hvm_vmx.exec_control |= cr3_ctls;
   
-/* Trap CR3 updates if CR3 memory events are enabled. */

-if ( v->domain->arch.monitor.write_ctrlreg_enabled &
- monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3) )
-v->arch.hvm_vmx.exec_control |= CPU_BASED_CR3_LOAD_EXITING;
-
-vmx_update_cpu_exec_control(v);
+if ( old_ctls != v->arch.hvm_vmx.exec_control )
+vmx_update_cpu_exec_control(v);
   }

How does this match up with the rest of this patch?

And by 'this' you mean slightly optimizing this sequence by adding in
old_ctls?
It seems pretty straight-forward to me, I figured if I am to move the
monitor.write_ctrlreg_enabled part from here
it wouldn't be much of a stretch to also do this little
optimization...what would have been appropriate?
To do this in a separate patch? To mention it in the commit message?

At least the latter, and perhaps better the former. Without even
mentioning it the readers (reviewers) have to guess whether this
is an integral part of the change, or - as you now confirm - just a
minor optimization done along the road.

Jan


Ack, will split in separate patch in v2.
You're right, I've got to be more attentive to always separate unrelated 
code changes, however minor they are :)


Thanks,
Corneliu.

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

[Xen-devel] [xen-unstable-smoke test] 95853: tolerable all pass - PUSHED

[osstest service owner]

flight 95853 xen-unstable-smoke real [real]
http://logs.test-lab.xenproject.org/osstest/logs/95853/

Failures :-/ but no regressions.

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-libvirt 12 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

version targeted for testing:
 xen  c1cee3ff93c1be525443472dd4eebe230b57ece9
baseline version:
 xen  759b9618b8a22ddd87d01c0bff5366814b17eea7

Last test of basis95786  2016-06-15 16:07:33 Z1 days
Testing same since95853  2016-06-17 09:14:43 Z0 days1 attempts


People who touched revisions under test:
  George Dunlap 
  Jan Beulich 
  Julien Grall 
  Kevin Tian 
  Quan Xu 
  Suravee Suthikulpanit 

jobs:
 build-amd64  pass
 build-armhf  pass
 build-amd64-libvirt  pass
 test-armhf-armhf-xl  pass
 test-amd64-amd64-xl-qemuu-debianhvm-i386 pass
 test-amd64-amd64-libvirt pass



sg-report-flight on osstest.test-lab.xenproject.org
logs: /home/logs/logs
images: /home/logs/images

Logs, config files, etc. are available at
http://logs.test-lab.xenproject.org/osstest/logs

Explanation of these reports, and of osstest in general, is at
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master

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


Pushing revision :

+ branch=xen-unstable-smoke
+ revision=c1cee3ff93c1be525443472dd4eebe230b57ece9
+ . ./cri-lock-repos
++ . ./cri-common
+++ . ./cri-getconfig
+++ umask 002
+++ getrepos
 getconfig Repos
 perl -e '
use Osstest;
readglobalconfig();
print $c{"Repos"} or die $!;
'
+++ local repos=/home/osstest/repos
+++ '[' -z /home/osstest/repos ']'
+++ '[' '!' -d /home/osstest/repos ']'
+++ echo /home/osstest/repos
++ repos=/home/osstest/repos
++ repos_lock=/home/osstest/repos/lock
++ '[' x '!=' x/home/osstest/repos/lock ']'
++ OSSTEST_REPOS_LOCK_LOCKED=/home/osstest/repos/lock
++ exec with-lock-ex -w /home/osstest/repos/lock ./ap-push xen-unstable-smoke 
c1cee3ff93c1be525443472dd4eebe230b57ece9
+ branch=xen-unstable-smoke
+ revision=c1cee3ff93c1be525443472dd4eebe230b57ece9
+ . ./cri-lock-repos
++ . ./cri-common
+++ . ./cri-getconfig
+++ umask 002
+++ getrepos
 getconfig Repos
 perl -e '
use Osstest;
readglobalconfig();
print $c{"Repos"} or die $!;
'
+++ local repos=/home/osstest/repos
+++ '[' -z /home/osstest/repos ']'
+++ '[' '!' -d /home/osstest/repos ']'
+++ echo /home/osstest/repos
++ repos=/home/osstest/repos
++ repos_lock=/home/osstest/repos/lock
++ '[' x/home/osstest/repos/lock '!=' x/home/osstest/repos/lock ']'
+ . ./cri-common
++ . ./cri-getconfig
++ umask 002
+ select_xenbranch
+ case "$branch" in
+ tree=xen
+ xenbranch=xen-unstable-smoke
+ qemuubranch=qemu-upstream-unstable
+ '[' xxen = xlinux ']'
+ linuxbranch=
+ '[' xqemu-upstream-unstable = x ']'
+ select_prevxenbranch
++ ./cri-getprevxenbranch xen-unstable-smoke
+ prevxenbranch=xen-4.7-testing
+ '[' xc1cee3ff93c1be525443472dd4eebe230b57ece9 = x ']'
+ : tested/2.6.39.x
+ . ./ap-common
++ : osst...@xenbits.xen.org
+++ getconfig OsstestUpstream
+++ perl -e '
use Osstest;
readglobalconfig();
print $c{"OsstestUpstream"} or die $!;
'
++ :
++ : git://xenbits.xen.org/xen.git
++ : osst...@xenbits.xen.org:/home/xen/git/xen.git
++ : git://xenbits.xen.org/qemu-xen-traditional.git
++ : git://git.kernel.org
++ : git://git.kernel.org/pub/scm/linux/kernel/git
++ : git
++ : git://xenbits.xen.org/libvirt.git
++ : osst...@xenbits.xen.org:/home/xen/git/libvirt.git
++ : git://xenbits.xen.org/libvirt.git
++ : git://xenbits.xen.org/rumpuser-xen.git
++ : git
++ : git://xenbits.xen.org/rumpuser-xen.git
++ : osst...@xenbits.xen.org:/home/xen/git/rumpuser-xen.git
+++ besteffort_repo https://github.com/rumpkernel/rumpkernel-netbsd-src
+++ local repo=https://github.com/rumpkernel/rumpkernel-netbsd-src
+++ cached_repo https://github.com/rumpkernel/rumpkernel-netbsd-src 
'[fetch=try]'
+++ local repo=https://github.com/rumpkernel/rumpkernel-netbsd-src
+++ local 'options=[fetch=try]'
 getconfig GitCacheProxy
 perl -e '
use Osstest;
readglobalconfig();
print $c{"GitCacheProxy"} or die $!;
'
+++ local cache=git://cache:9419

Re: [Xen-devel] [PATCH 3/7] vm-event: introduce vm_event_vcpu_enter

[Jan Beulich]

>>> On 17.06.16 at 13:13,  wrote:
> On 6/17/2016 10:17 AM, Jan Beulich wrote:
>> (And to be clear, I much appreciate any form of reduction of the
>> sometimes extremely long lists of #include-s, just not [apparently
>> or really] randomly mixed with other, substantial changes. That's
>> namely because it's not clear whether source files should explicitly
>> include everything they need, or instead be allowed to rely on
>> headers they include to include further headers they also
>> _explicitly_ rely on.
> 
> Personally I prefer the former since I think it also cuts down 
> compilation time.
> Having header H include every header Ni needed by source S makes H 
> unnecessarily bulky at compilation time for other sources <> S that 
> don't need headers Ni but which depend on H nonetheless.

I nowhere said every _header_ should include everything any of
its _consumers_ would require. My point was solely about source
files. For example, if S depends on both H1 and H2, and H2
already includes H1, whether S then needs to just include H2, or
should also explicitly include H1 (such that S doesn't need changing
when the inclusion of H1 by H2 goes away).

Jan


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

Re: [Xen-devel] [PATCH 6/7] vm-event/arm: move hvm_event_cr->common vm_event_monitor_cr

[Corneliu ZUZU]

On 6/17/2016 11:38 AM, Jan Beulich wrote:

On 17.06.16 at 10:25,  wrote:

On 6/16/2016 6:16 PM, Jan Beulich wrote:

And looking at all the uses of this variable I get the impression that
you really want a shorthand for &d->arch.monitor (if any such
helper variable is worthwhile to have here in the first place).

Well, this was a simple cut-paste operation, not very old content aware :)
Personally I prefer the current shorthand (ad) (seems more intuitive and
is consistent with the other XEN_DOMCTL_MONITOR_EVENT_* cases), but if
you prefer I'll change that shorthand to am = &d->arch.monitor?

I'd prefer either no shorthand, or one eliminating the longest common
prefix across all uses.


am = &d->arch.monitor it is then.


--- a/xen/include/asm-x86/monitor.h
+++ b/xen/include/asm-x86/monitor.h
@@ -24,8 +24,6 @@
   
   #include 
   
-#define monitor_ctrlreg_bitmask(ctrlreg_index) (1U << (ctrlreg_index))

-
   static inline
   int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op 
*mop)
   {
--- a/xen/include/xen/monitor.h
+++ b/xen/include/xen/monitor.h
@@ -25,6 +25,10 @@
   struct domain;
   struct xen_domctl_monitor_op;
   
+#if CONFIG_X86

+#define monitor_ctrlreg_bitmask(ctrlreg_index) (1U << (ctrlreg_index))
+#endif

What's the point in removing this from the x86 header if then it
needs to be put in such a conditional? If the conditional gets
dropped in the next patch, then I think you have two options:
Leave it where it is here, and move it there. Or move it here,
but omit the conditional right away - I can't see this definition
being present to harm the ARM build in any way.

Meld comment above.

I don't follow - having split the patches for reviewability was
a good idea. I merely commented on some oddities resulting
from that split, which - afaict - could be easily corrected without
folding the patches together.

Jan


Ooh, haha, sorry I've re-read your comment now. You're right, there's no 
point in that change, I'll leave it on the X86 side until the ARM part 
is actually implemented (last patch).


Corneliu.

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

Re: [Xen-devel] [PATCH v2 2/2] xen: make available hvm_fep to non-debug build as well

[Jan Beulich]

>>> On 17.06.16 at 13:05,  wrote:
> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -59,6 +59,20 @@ config BIGMEM
>  
> If unsure, say N.
>  
> +config HVM_FEP
> + bool "HVM Forced Emulation Prefix support"

And no "if EXPERT"?

> @@ -95,9 +96,9 @@ unsigned long __section(".bss.page_aligned")
>  static bool_t __initdata opt_hap_enabled = 1;
>  boolean_param("hap", opt_hap_enabled);
>  
> -#ifndef opt_hvm_fep
> +#if CONFIG_HVM_FEP

#ifdef please. And I think this would better be left alone anyway
(and then the comment only applies to the other instance).

Jan


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

Re: [Xen-devel] [PATCH 3/7] vm-event: introduce vm_event_vcpu_enter

[Corneliu ZUZU]

On 6/17/2016 11:55 AM, Julien Grall wrote:

Hello,

On 16/06/16 15:08, Corneliu ZUZU wrote:

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index d31f821..ba248c8 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -19,6 +19,7 @@
  #include 
  #include 
  #include 
+#include 

  #include 
  #include 
@@ -251,6 +252,8 @@ static void schedule_tail(struct vcpu *prev)

  ctxt_switch_to(current);

+vm_event_vcpu_enter(current);
+
  local_irq_enable();

  context_saved(prev);
@@ -296,7 +299,7 @@ void context_switch(struct vcpu *prev, struct 
vcpu *next)


  void continue_running(struct vcpu *same)
  {
-/* Nothing to do */
+vm_event_vcpu_enter(same);
  }

  void sync_local_execstate(void)


From my understanding of the commit message, vm_event_vcpu_enter 
should be called before returning to the guest. The scheduling 
functions are not called every-time Xen is returning to the guest. So 
if you want to do every time Xen is re-entering to the guest, then 
this should be done in leave_hypervisor_tail.


Regards,



Which is also done, vm_event_vcpu_enter is called from 
leave_hypervisor_tail as well.
Are you saying that calling from leave_hypervisor_tail is enough and 
that the schedule_tail call is not necessary?


Thanks,
Corneliu.

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

Re: [Xen-devel] [PATCH 7/7] vm-event/arm: implement support for control-register write vm-events

[Corneliu ZUZU]

On 6/17/2016 12:28 AM, Julien Grall wrote:



On 16/06/2016 20:24, Corneliu ZUZU wrote:

On 6/16/2016 5:26 PM, Julien Grall wrote:
Hi Julien. Yes, I agree that it's complex, I would have preferred to
split it up too and I actually tried, but the changes are tightly
coupled and they don't seem to be 'split-able'.


After I reviewed this patch I think it could be simplified a lot.

Most of the registers not trapped by vm-event could be emulated with 
one-line WRITE_SYSREGxx and does not require specific case depending 
on the architecture.


You can give a look how we handle the domain context switch in C 
(arch/arm/domain.c).


Only few of the registers will require more than one-line (such as 
MAIR*, *FAR) which could be over a macro.


Regards,



Ok, I'll take a look again but I'm not that optimistic, I remember 
trying pretty hard to simplify this (and kind of failing), I got the 
best I could.
I think it had to do with the fact the SCTLR macros & such got expanded 
too early (where I wanted to use them as prefixes to construct other 
macros).
Having said this, I'll look into your suggestions and reinspect the code 
to recall why those macros got to be the way the are and get back with 
conclusions.


Regards,
Corneliu.

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

Re: [Xen-devel] [PATCH 2/2] hvmloader: don't hard-code IO-APIC parameters

[Boris Ostrovsky]

On 06/17/2016 02:00 AM, Jan Beulich wrote:
 On 16.06.16 at 18:49,  wrote:
>> On 06/16/2016 05:40 AM, Jan Beulich wrote:
>>> The IO-APIC address has variable bits determined by the PCI-to-ISA
>>> bridge, and the IO-APIC version should be read from the IO-APIC. (Note
>>> that there's still implicit rather than explicit agreement on the
>>> IO-APIC base address between qemu and the hypervisor.)
>> It probably doesn't matter now for PVHv2 since we are not going to
>> initially emulate IOAPIC but when/if we do, how do we query for base
>> address and version? Should we just rely on the same implicit agreement?
> If this refers to the sentence in parentheses, then this was really
> meant to indicate a work item. I.e. qemu should negotiate with
> Xen. For PVHv2 this would then probably mean for hvmloader to
> query Xen (via new hypercall).

We are not using hvmloader for PVH so I am not sure how that would work.
You mean the toolstack?

-boris

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

Re: [Xen-devel] [PATCH 3/7] vm-event: introduce vm_event_vcpu_enter

[Corneliu ZUZU]

On 6/17/2016 2:27 PM, Jan Beulich wrote:

On 17.06.16 at 13:13,  wrote:

On 6/17/2016 10:17 AM, Jan Beulich wrote:

(And to be clear, I much appreciate any form of reduction of the
sometimes extremely long lists of #include-s, just not [apparently
or really] randomly mixed with other, substantial changes. That's
namely because it's not clear whether source files should explicitly
include everything they need, or instead be allowed to rely on
headers they include to include further headers they also
_explicitly_ rely on.

Personally I prefer the former since I think it also cuts down
compilation time.
Having header H include every header Ni needed by source S makes H
unnecessarily bulky at compilation time for other sources <> S that
don't need headers Ni but which depend on H nonetheless.

I nowhere said every _header_ should include everything any of
its _consumers_ would require. My point was solely about source
files. For example, if S depends on both H1 and H2, and H2
already includes H1, whether S then needs to just include H2, or
should also explicitly include H1 (such that S doesn't need changing
when the inclusion of H1 by H2 goes away).

Jan




Ah, ok got it.
I restate my view of treating these "issues" with automation tools 
rather than leaving the programmer to do primitive work that he 
shouldn't be required to do with a nowadays programming language.
Clang for example offers a powerful parsing library (can parse GCC too) 
with python bindings, it would be nice to take more advantage of that.


Corneliu.

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

[Xen-devel] [linux-3.14 test] 95836: regressions - trouble: blocked/broken/fail/pass

[osstest service owner]

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

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-amd64-i386-freebsd10-amd64 10 guest-startfail REGR. vs. 95164
 test-amd64-amd64-qemuu-nested-intel 13 xen-boot/l1fail REGR. vs. 95164
 test-amd64-amd64-qemuu-nested-amd 13 xen-boot/l1  fail REGR. vs. 95164

Tests which are failing intermittently (not blocking):
 test-amd64-amd64-xl-qemut-debianhvm-amd64-xsm 3 host-install(3) broken pass in 
95800
 test-amd64-i386-xl-qemut-stubdom-debianhvm-amd64-xsm 3 host-install(3) broken 
pass in 95800
 test-amd64-amd64-xl-qemuu-debianhvm-amd64-xsm 9 debian-hvm-install fail in 
95800 pass in 95836
 test-amd64-amd64-xl-qemuu-ovmf-amd64  6 xen-bootfail pass in 95800

Regressions which are regarded as allowable (not blocking):
 build-i386-rumpuserxen6 xen-buildfail   like 95164
 build-amd64-rumpuserxen   6 xen-buildfail   like 95164
 test-amd64-amd64-xl-qemut-win7-amd64 16 guest-stop fail like 95164
 test-amd64-i386-xl-qemut-win7-amd64 16 guest-stop  fail like 95164
 test-amd64-amd64-xl-qemuu-win7-amd64 16 guest-stop fail like 95164
 test-amd64-i386-xl-qemuu-win7-amd64 16 guest-stop  fail like 95164

Tests which did not succeed, but are not blocking:
 test-amd64-i386-rumpuserxen-i386  1 build-check(1)   blocked  n/a
 test-amd64-amd64-rumpuserxen-amd64  1 build-check(1)   blocked n/a
 test-amd64-amd64-xl-pvh-intel 11 guest-start  fail  never pass
 test-amd64-amd64-libvirt-xsm 12 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt-xsm  12 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt 12 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt  12 migrate-support-checkfail   never pass
 test-amd64-amd64-xl-pvh-amd  11 guest-start  fail   never pass
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check 
fail never pass
 test-amd64-amd64-libvirt-vhd 11 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check 
fail never pass

version targeted for testing:
 linux022aafd04696a3c6b7ad47ab83a9650646287bf8
baseline version:
 linuxf06cb456a442c7df95a4ba6e2f3a341cf925d7cf

Last test of basis95164  2016-06-01 19:46:34 Z   15 days
Testing same since95407  2016-06-08 00:46:14 Z9 days8 attempts


People who touched revisions under test:
  Andrew Morton 
  Ben Hutchings 
  Bjorn Helgaas 
  Daniel Lezcano 
  Daniel Vetter 
  Dave Chinner 
  Dave Chinner 
  Dave Gerlach 
  David Vrabel 
  Dmitry Torokhov 
  Greg Kroah-Hartman 
  Hari Bathini 
  Itai Handler 
  J. Bruce Fields 
  James Hogan 
  Jeff Layton 
  Joseph Salisbury 
  Kalle Valo 
  Kalle Valo 
  Larry Finger 
  Linus Torvalds 
  Lyude 
  Mahesh Salgaonkar 
  Martin K. Petersen 
  Matthias Schiffer 
  Michael Ellerman 
  Nicolai Stange 
  Patrik Jakobsson 
  Paul Burton 
  Prarit Bhargava 
  Rafael J. Wysocki 
  Raghava Aditya Renukunta 
  Ralf Baechle 
  Ricky Liang 
  Ross Lagerwall 
  Shyam Kaushik 
  Theodore Ts'o 
  Tomáš Trnka 
  Ville Syrjälä 
  Wang YanQing 

jobs:
 build-amd64-xsm  pass
 build-i386-xsm   pass
 build-amd64  pass
 build-i386   pass
 build-amd64-libvirt  pass
 build-i386-libvirt   pass
 build-amd64-pvopspass
 build-i386-pvops pass
 build-amd64-rumpuserxen  fail
 build-i386-rumpuserxen   fail
 test-amd64-amd64-xl  pass
 test-amd64-i386-xl   pass
 test-amd64-amd64-xl-qemut-debianhvm-amd64-xsmbroken  
 test-amd64-i386-xl-qemut-debianhvm-amd64-xsm pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm   pass
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsmpass
 test-amd64-amd64-xl-qemuu-debianhvm-amd64-xsmpass
 test-amd64-i386-xl-qemuu-debianhvm-amd64-xsm pass
 test-amd64-amd64-xl-qemut-stubdom-debianhvm-amd64-xsmpass
 test-amd64-i386-xl-qemut-stubdom-debianhvm-amd64-xsm broken  
 test-amd64-amd64-libvirt-xsm pass
 test-amd64-i386-libvirt-xsm  

Re: [Xen-devel] [PATCH 7/7] vm-event/arm: implement support for control-register write vm-events

[Julien Grall]

Hello Corneliu,

On 17/06/16 11:36, Corneliu ZUZU wrote:

On 6/16/2016 7:49 PM, Julien Grall wrote:

On 16/06/16 15:13, Corneliu ZUZU wrote:


[...]


Please mention that PRRR and NMRR are aliased to respectively MAIR0
and MAIR1. This will avoid to spend time trying to understanding why
the spec says they are trapped but you don't "handle" them.


I mentioned that in traps.h (see "AKA" comments). Will put the same
comment here then.


I noticed it later. But it was not obvious to find.

[...]


diff --git a/xen/arch/arm/vm_event.c b/xen/arch/arm/vm_event.c
new file mode 100644
index 000..3f23fec
--- /dev/null
+++ b/xen/arch/arm/vm_event.c
@@ -0,0 +1,112 @@


[...]


+#include 
+#include 
+
+#if CONFIG_ARM_64
+
+#define MWSINF_SCTLR32,SCTLR_EL1
+#define MWSINF_TTBR064,TTBR0_EL1
+#define MWSINF_TTBR164,TTBR1_EL1
+#define MWSINF_TTBCR64,TCR_EL1
+
+#elif CONFIG_ARM_32
+
+#define MWSINF_SCTLR32,SCTLR
+#define MWSINF_TTBR064,TTBR0
+#define MWSINF_TTBR164,TTBR1


The values are the same as for arm64 (*_EL1 is aliased to * for
arm32). Please avoid duplication.


(see comment below about later reply)


Your later reply explain why you did not expose TTBR*_32 to ARM64, but 
does not explain why the 3 define above is the same as the ARM64.







+#define MWSINF_TTBR0_32 32,TTBR0_32
+#define MWSINF_TTBR1_32 32,TTBR1_32
+#define MWSINF_TTBCR32,TTBCR
+
+#endif
+
+#define MWS_EMUL_(val, sz, r...) WRITE_SYSREG##sz((uint##sz##_t)
(val), r)


The cast is not necessary.


+#define MWS_EMUL(r) CALL_MACRO(MWS_EMUL_, w->value, MWSINF_##r)
+
+static inline void vcpu_enter_write_data(struct vcpu *v)
+{
+struct monitor_write_data *w = &v->arch.vm_event.write_data;
+
+if ( likely(MWS_NOWRITE == w->status) )
+return;
+
+switch ( w->status )
+{
+case MWS_SCTLR:
+MWS_EMUL(SCTLR);
+break;
+case MWS_TTBR0:
+MWS_EMUL(TTBR0);
+break;
+case MWS_TTBR1:
+MWS_EMUL(TTBR1);
+break;
+#if CONFIG_ARM_32
+case MWS_TTBR0_32:
+MWS_EMUL(TTBR0_32);
+break;
+case MWS_TTBR1_32:
+MWS_EMUL(TTBR1_32);
+break;
+#endif


Aarch32 kernel can return on an AArch64 Xen. This means that
TTBR{0,1}_32 could be trapped and the write therefore be properly
emulated.


MCR TTBRx writes from AArch32 guests appear as writes of the low 32-bits
of AArch64 TTBRx_EL1 (architecturally mapped).
MCRR TTBRx writes from AArch32 guests appear as writes to AArch64
TTBRx_EL1.
Therefore, in the end the register we need to write is still TTBRx_EL1.


Why would you want to notify write to TTBR*_32 when Xen is running in 
AArch32 mode and none in Aaarch64 mode?


The vm-events for an AArch32 guests should be exactly the same
regardless of Xen mode (i.e AArch32 vs AArch64).

[...]




@@ -0,0 +1,253 @@


[...]


+/*
+ * Emulation of system-register trapped writes that do not cause
+ * VM_EVENT_REASON_WRITE_CTRLREG monitor vm-events.
+ * Such writes are collaterally trapped due to setting the
HCR_EL2.TVM bit.
+ *
+ * Regarding aarch32 domains, note that from Xen's perspective
system-registers
+ * of such domains are architecturally-mapped to aarch64 registers
in one of
+ * three ways:
+ *  - low 32-bits mapping   (e.g. aarch32 DFAR -> aarch64
FAR_EL1[31:0])
+ *  - high 32-bits mapping  (e.g. aarch32 IFAR -> aarch64
FAR_EL1[63:32])
+ *  - full mapping  (e.g. aarch32 SCTLR -> aarch64 SCTLR_EL1)
+ *
+ * Hence we define 2 macro variants:
+ *  - TVM_EMUL_SZ variant, for full mappings
+ *  - TVM_EMUL_LH variant, for low/high 32-bits mappings
+ */
+#define TVM_EMUL_SZ(regs, hsr, val, sz,
r...)   \
+{ \
+if ( psr_mode_is_user(regs)
)   \


Those registers are not accessible at EL0.


Hmm, I have this question noted.
I remember finding from the manuals that a write from user-mode of those
regs would still trap to EL2, but I didn't test that yet.
Will put that to test and come back with results for v2.


Testing will not tell you if a trap will occur or not. The ARM ARM may 
define it as IMPLEMENTATION DEFINED.


From my understanding of the ARMv7 spec (B1.14.1 and B1.14.13 in ARM 
DDI 0406C.c), the instruction at PL0 (user mode) will not trap to the 
hypervisor:


"Setting HCR.TVM to 1 means that any attempt, to write to a Non-secure 
memory control register from a Non-secure
PL1 or PL0 mode, that this reference manual does not describe as 
UNDEFINED , generates a Hyp Trap exception."


For ARMv8 (See description of HCR_EL2.TVM D7-1971 in ARM DDI 0487A.j), 
only NS EL1 write to the registers will be trapped.





+return inject_undef_exception(regs,
hsr);   \
+WRITE_SYSREG##sz((uint##sz##_t) (val), r);


[...]


diff --git a/xen/include/asm-arm/vm_event.h
b/xen/include/asm-arm/vm_event.h
index 4e5a272..edf9654 100644
--- a/xen/include/asm-arm/vm_event.h
+++ b/xen/include/asm-arm/vm_event.h
@@ -30,6 +30,12 @@ static inline int vm_event_ini

Re: [Xen-devel] [PATCH 3/7] vm-event: introduce vm_event_vcpu_enter

[Julien Grall]

Hello,

On 17/06/16 12:40, Corneliu ZUZU wrote:

On 6/17/2016 11:55 AM, Julien Grall wrote:

On 16/06/16 15:08, Corneliu ZUZU wrote:

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index d31f821..ba248c8 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -19,6 +19,7 @@
  #include 
  #include 
  #include 
+#include 

  #include 
  #include 
@@ -251,6 +252,8 @@ static void schedule_tail(struct vcpu *prev)

  ctxt_switch_to(current);

+vm_event_vcpu_enter(current);
+
  local_irq_enable();

  context_saved(prev);
@@ -296,7 +299,7 @@ void context_switch(struct vcpu *prev, struct
vcpu *next)

  void continue_running(struct vcpu *same)
  {
-/* Nothing to do */
+vm_event_vcpu_enter(same);
  }

  void sync_local_execstate(void)


From my understanding of the commit message, vm_event_vcpu_enter
should be called before returning to the guest. The scheduling
functions are not called every-time Xen is returning to the guest. So
if you want to do every time Xen is re-entering to the guest, then
this should be done in leave_hypervisor_tail.

Regards,



Which is also done, vm_event_vcpu_enter is called from
leave_hypervisor_tail as well.


Sorry I did not spot this one.


Are you saying that calling from leave_hypervisor_tail is enough and
that the schedule_tail call is not necessary?


leave_hypervisor_tail will always be called before returning to the 
guest. So a call to vm_event_vcpu_enter in this function will be sufficient.


Regards,

--
Julien Grall

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

Re: [Xen-devel] [PATCH RESEND 05/14] libxl/arm: Construct ACPI GTDT table

[Julien Grall]

On 17/06/16 10:15, Stefano Stabellini wrote:

On Fri, 17 Jun 2016, Julien Grall wrote:

Hello Shannon,

On 17/06/16 04:29, Shannon Zhao wrote:

On 2016/6/6 19:40, Stefano Stabellini wrote:

ACPI tables for ARM guests should be user configurable: acpi=1 in the VM
config file enables them, with default off.

While the configuration "acpi" already exists for HVM guest
configuration, do we plan to reuse it or use a new name, e.g. arm_acpi?


We always try to re-use options unless their meaning are completely different.


I agree. "acpi" is a good name for it. I would avoid introducing
something like "arm_acpi".


BTW, this value should be default false for at least 32-bit domain.

I am not sure if we agreed what should be the default for 64-bit domain.

Cheers,

--
Julien Grall

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

Re: [Xen-devel] [Patch v11 3/3] vt-d: fix vt-d Device-TLB flush timeout issue (+ crash logic )

[Julien Grall]

Hi Quan,

On 17/06/16 09:51, Xu, Quan wrote:

+ arm/amd maintainers..

On June 01, 2016 5:05 PM, Xu, Quan  wrote:

If Device-TLB flush timed out, we hide the target ATS device immediately and
crash the domain owning this ATS device. If impacted domain is hardware
domain, just throw out a warning.

By hiding the device, we make sure it can't be assigned to any domain any
longer (see device_assigned).


DomU (other than Dom0) gets crashed when a device IOTLB flush times out. I 
suppose that's what you will want on ARM/AMD then too.


Correct it is what we want for ARM :).


We need to move up the crash logic , as similar as iommu_map_page() / 
iommu_unmap_page().

 - add the crash logic to iommu_iotlb_flush() / iommu_iotlb_flush_all().

 - when IOMMU/MMU share page tables, we need to fix it one by one.
 -- on amd, we need to add the crash logic to amd_iommu_flush_pages().
 -- on intel, we need to add the crash logic to iommu_pte_flush().
 -- on arm, we benefit that we add the crash logic to 
iommu_iotlb_flush().


Right.




Taken together, we need to add crash logic to
  iommu_iotlb_flush() / iommu_iotlb_flush_all() / 
iommu_pte_flush() / amd_iommu_flush_pages().


For iommu_iotlb_* yes as it is common code. I don't know for the others.

Regards,

--
Julien Grall

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

Re: [Xen-devel] [PATCH v2 2/2] xen: make available hvm_fep to non-debug build as well

[Wei Liu]

On Fri, Jun 17, 2016 at 05:34:07AM -0600, Jan Beulich wrote:
> >>> On 17.06.16 at 13:05,  wrote:
> > --- a/xen/arch/x86/Kconfig
> > +++ b/xen/arch/x86/Kconfig
> > @@ -59,6 +59,20 @@ config BIGMEM
> >  
> >   If unsure, say N.
> >  
> > +config HVM_FEP
> > +   bool "HVM Forced Emulation Prefix support"
> 
> And no "if EXPERT"?
> 

Done.

> > @@ -95,9 +96,9 @@ unsigned long __section(".bss.page_aligned")
> >  static bool_t __initdata opt_hap_enabled = 1;
> >  boolean_param("hap", opt_hap_enabled);
> >  
> > -#ifndef opt_hvm_fep
> > +#if CONFIG_HVM_FEP
> 
> #ifdef please. And I think this would better be left alone anyway
> (and then the comment only applies to the other instance).
> 

Change this instance back to what is was and fix the other.

Wei.

> Jan
> 

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

[Xen-devel] [ovmf test] 95833: regressions - FAIL

[osstest service owner]

flight 95833 ovmf real [real]
http://logs.test-lab.xenproject.org/osstest/logs/95833/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-amd64-i386-xl-qemuu-ovmf-amd64 17 guest-start/debianhvm.repeat fail REGR. 
vs. 94748
 test-amd64-amd64-xl-qemuu-ovmf-amd64 17 guest-start/debianhvm.repeat fail 
REGR. vs. 94748

version targeted for testing:
 ovmf d8d217c57632549d99256f134a4cee6be8dc67bb
baseline version:
 ovmf dc99315b8732b6e3032d01319d3f534d440b43d0

Last test of basis94748  2016-05-24 22:43:25 Z   23 days
Failing since 94750  2016-05-25 03:43:08 Z   23 days   40 attempts
Testing same since95833  2016-06-16 12:53:07 Z1 days1 attempts


People who touched revisions under test:
  Ard Biesheuvel 
  Chao Zhang 
  Cinnamon Shia 
  Cohen, Eugene 
  Dandan Bi 
  Darbin Reyes 
  Eric Dong 
  Eugene Cohen 
  Evan Lloyd 
  Fu Siyuan 
  Fu, Siyuan 
  Gary Li 
  Gary Lin 
  Giri P Mudusuru 
  Hao Wu 
  Hegde Nagaraj P 
  hegdenag 
  Heyi Guo 
  Jeff Fan 
  Jiaxin Wu 
  Jiewen Yao 
  Katie Dellaquila 
  Laszlo Ersek 
  Liming Gao 
  lushifex 
  Marvin H?user 
  Marvin Haeuser 
  Maurice Ma 
  Michael Zimmermann 
  Ruiyu Ni 
  Ryan Harkin 
  Sami Mujawar 
  Satya Yarlagadda 
  Sriram Subramanian 
  Star Zeng 
  Tapan Shah 
  Thomas Palmer 
  Yonghong Zhu 
  Zhang, Chao B 

jobs:
 build-amd64-xsm  pass
 build-i386-xsm   pass
 build-amd64  pass
 build-i386   pass
 build-amd64-libvirt  pass
 build-i386-libvirt   pass
 build-amd64-pvopspass
 build-i386-pvops pass
 test-amd64-amd64-xl-qemuu-ovmf-amd64 fail
 test-amd64-i386-xl-qemuu-ovmf-amd64  fail



sg-report-flight on osstest.test-lab.xenproject.org
logs: /home/logs/logs
images: /home/logs/images

Logs, config files, etc. are available at
http://logs.test-lab.xenproject.org/osstest/logs

Explanation of these reports, and of osstest in general, is at
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master

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


Not pushing.

(No revision log; it would be 2140 lines long.)

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

Re: [Xen-devel] [PATCH] xen: sched: use default scheduler upon an invalid "sched="

[Jonathan Creekmore]

> On Jun 17, 2016, at 5:31 AM, Dario Faggioli  wrote:
> 
> instead of just the first scheduler we find in the array.
> 
> In fact, right now, if someone makes a typo when passing
> the "sched=" command line option to Xen, we (with all
> schedulers configured in) pick ARINC653, which is most
> likely not what one would expect.
> 
> Go for the default scheduler instead.
> 
> Signed-off-by: Dario Faggioli 
> ---
> Cc: George Dunlap 
> Cc: Jan Beulich 
> Cc: Doug Goldstein 
> Cc: Jonathan Creekmore 

Reviewed-By: Jonathan Creekmore 


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

Re: [Xen-devel] [PATCH v2 2/2] xen: make available hvm_fep to non-debug build as well

[Andrew Cooper]

On 17/06/16 12:05, Wei Liu wrote:
> Originally hvm_fep was guarded by NDEBUG, which means it was only
> available to debug builds.
>
> However there is value to have it for non-debug builds as well. User can
> use that to run tests in setup that replicates production setup.
>
> Make it clear with a sync_console style warning that this option can't
> be used in production setup. Update command line documentation
> accordingly. Finally mark Xen as tainted when this option is enabled.
>
> Add a kconfig option under x86 to configure hvm_fep.
>
> Signed-off-by: Wei Liu 
> ---
> Cc: Andrew Cooper 
> Cc: Jan Beulich 
> Cc: Doug Goldstein 
>
> v2:
> 1. unsigned -> unsigned int
> 2. %d -> %u
> 3. Add spaces around "-"
> 4. Update warning message
> 5. Only taint hv when fep is used
> 6. Add kconfig option
> ---
>  docs/misc/xen-command-line.markdown |  8 ++--
>  xen/arch/x86/Kconfig| 14 ++
>  xen/arch/x86/hvm/hvm.c  | 28 ++--
>  xen/common/kernel.c |  6 --
>  xen/include/asm-x86/hvm/hvm.h   |  2 +-
>  xen/include/xen/lib.h   |  1 +
>  6 files changed, 52 insertions(+), 7 deletions(-)
>
> diff --git a/docs/misc/xen-command-line.markdown 
> b/docs/misc/xen-command-line.markdown
> index fed732c..dc53e24 100644
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -878,8 +878,12 @@ Recognized in debug builds of the hypervisor only.
>  Allow use of the Forced Emulation Prefix in HVM guests, to allow emulation of
>  arbitrary instructions.
>  
> -This option is intended for development purposes, and is only available in
> -debug builds of the hypervisor.
> +This option is intended for development and testing purposes.
> +
> +*Warning*
> +As this feature opens up the instruction emulator to HVM guest, don't

"to arbitrary instruction from an HVM guest,".  It is an important
distinction, because all guest can enter the emulator in other ways as
part of normal operation.

> +use this in production system. No security support is provided when
> +this flag is set.
>  
>  ### hvm\_port80
>  > `= `
> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
> index 73f79cc..5e3b04a 100644
> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -59,6 +59,20 @@ config BIGMEM
>  
> If unsure, say N.
>  
> +config HVM_FEP
> + bool "HVM Forced Emulation Prefix support"
> + default y
> + ---help---
> +
> +   Compiles in a feature that allows HVM guest to enter
> +   instruction emulator with forced emulation prefix.

"allows HVM guests to arbitrarily exercise the instruction emulator." 
There are other ways in which guests can enter the instruction emulator,
but they are specifically limited in nature.

I would also have a separate paragraph stating:

"This is strictly for testing purposes, and not appropriate for use in
production."

> +
> +   This feature can only be enabled during boot time with
> +   appropriate hypervisor command line option. Please read
> +   hypervisor command line documentation before trying to use
> +   this feature.
> +
> +   If unsure, say Y.
>  endmenu
>  
>  source "common/Kconfig"
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 78db903..373b78e 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -37,6 +37,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -95,9 +96,9 @@ unsigned long __section(".bss.page_aligned")
>  static bool_t __initdata opt_hap_enabled = 1;
>  boolean_param("hap", opt_hap_enabled);
>  
> -#ifndef opt_hvm_fep
> +#if CONFIG_HVM_FEP
>  /* Permit use of the Forced Emulation Prefix in HVM guests */
> -bool_t opt_hvm_fep;
> +bool_t __read_mostly opt_hvm_fep;
>  boolean_param("hvm_fep", opt_hvm_fep);
>  #endif
>  
> @@ -182,6 +183,28 @@ static int __init hvm_enable(void)
>  if ( !opt_altp2m_enabled )
>  hvm_funcs.altp2m_supported = 0;
>  
> +if ( opt_hvm_fep )
> +{
> +unsigned int i, j;
> +
> +printk("**\n");
> +printk("*** WARNING: HVM FORCED EMULATION PREFIX IS 
> AVAILABLE\n");
> +printk("*** This option is *ONLY* intended to aid testing of 
> Xen.\n");
> +printk("*** It has implications on the security of the 
> system.\n");
> +printk("*** Please *DO NOT* use this in production.\n");
> +printk("**\n");
> +for ( i = 0; i < 3; i++ )
> +{
> +printk("%u... ", 3 - i);
> +for ( j = 0; j < 100; j++ )
> +{
> +process_pending_softirqs();
> +mdelay(10);
> +}
> +}

I would drop this 3s wait, and I plan to drop it from sync_console as
well.  It isn't of any practical use, even if you are watching the
serial console on boot, and just

[Xen-devel] [xen-unstable-smoke test] 95858: tolerable all pass - PUSHED

[osstest service owner]

flight 95858 xen-unstable-smoke real [real]
http://logs.test-lab.xenproject.org/osstest/logs/95858/

Failures :-/ but no regressions.

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-libvirt 12 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

version targeted for testing:
 xen  2f7b56c1a7b1ee92ff5f92888723e380412dd3ab
baseline version:
 xen  c1cee3ff93c1be525443472dd4eebe230b57ece9

Last test of basis95853  2016-06-17 09:14:43 Z0 days
Testing same since95858  2016-06-17 12:02:01 Z0 days1 attempts


People who touched revisions under test:
  David Scott 
  Dongli Zhang 
  Ian Jackson 
  Juergen Gross 
  Julien Grall 
  Peng Fan 
  Roger Pau Monné 
  Shanker Donthineni 
  Shannon Zhao 
  Stefano Stabellini 
  Wei Chen 
  Wei Liu 

jobs:
 build-amd64  pass
 build-armhf  pass
 build-amd64-libvirt  pass
 test-armhf-armhf-xl  pass
 test-amd64-amd64-xl-qemuu-debianhvm-i386 pass
 test-amd64-amd64-libvirt pass



sg-report-flight on osstest.test-lab.xenproject.org
logs: /home/logs/logs
images: /home/logs/images

Logs, config files, etc. are available at
http://logs.test-lab.xenproject.org/osstest/logs

Explanation of these reports, and of osstest in general, is at
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master

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


Pushing revision :

+ branch=xen-unstable-smoke
+ revision=2f7b56c1a7b1ee92ff5f92888723e380412dd3ab
+ . ./cri-lock-repos
++ . ./cri-common
+++ . ./cri-getconfig
+++ umask 002
+++ getrepos
 getconfig Repos
 perl -e '
use Osstest;
readglobalconfig();
print $c{"Repos"} or die $!;
'
+++ local repos=/home/osstest/repos
+++ '[' -z /home/osstest/repos ']'
+++ '[' '!' -d /home/osstest/repos ']'
+++ echo /home/osstest/repos
++ repos=/home/osstest/repos
++ repos_lock=/home/osstest/repos/lock
++ '[' x '!=' x/home/osstest/repos/lock ']'
++ OSSTEST_REPOS_LOCK_LOCKED=/home/osstest/repos/lock
++ exec with-lock-ex -w /home/osstest/repos/lock ./ap-push xen-unstable-smoke 
2f7b56c1a7b1ee92ff5f92888723e380412dd3ab
+ branch=xen-unstable-smoke
+ revision=2f7b56c1a7b1ee92ff5f92888723e380412dd3ab
+ . ./cri-lock-repos
++ . ./cri-common
+++ . ./cri-getconfig
+++ umask 002
+++ getrepos
 getconfig Repos
 perl -e '
use Osstest;
readglobalconfig();
print $c{"Repos"} or die $!;
'
+++ local repos=/home/osstest/repos
+++ '[' -z /home/osstest/repos ']'
+++ '[' '!' -d /home/osstest/repos ']'
+++ echo /home/osstest/repos
++ repos=/home/osstest/repos
++ repos_lock=/home/osstest/repos/lock
++ '[' x/home/osstest/repos/lock '!=' x/home/osstest/repos/lock ']'
+ . ./cri-common
++ . ./cri-getconfig
++ umask 002
+ select_xenbranch
+ case "$branch" in
+ tree=xen
+ xenbranch=xen-unstable-smoke
+ qemuubranch=qemu-upstream-unstable
+ '[' xxen = xlinux ']'
+ linuxbranch=
+ '[' xqemu-upstream-unstable = x ']'
+ select_prevxenbranch
++ ./cri-getprevxenbranch xen-unstable-smoke
+ prevxenbranch=xen-4.7-testing
+ '[' x2f7b56c1a7b1ee92ff5f92888723e380412dd3ab = x ']'
+ : tested/2.6.39.x
+ . ./ap-common
++ : osst...@xenbits.xen.org
+++ getconfig OsstestUpstream
+++ perl -e '
use Osstest;
readglobalconfig();
print $c{"OsstestUpstream"} or die $!;
'
++ :
++ : git://xenbits.xen.org/xen.git
++ : osst...@xenbits.xen.org:/home/xen/git/xen.git
++ : git://xenbits.xen.org/qemu-xen-traditional.git
++ : git://git.kernel.org
++ : git://git.kernel.org/pub/scm/linux/kernel/git
++ : git
++ : git://xenbits.xen.org/libvirt.git
++ : osst...@xenbits.xen.org:/home/xen/git/libvirt.git
++ : git://xenbits.xen.org/libvirt.git
++ : git://xenbits.xen.org/rumpuser-xen.git
++ : git
++ : git://xenbits.xen.org/rumpuser-xen.git
++ : osst...@xenbits.xen.org:/home/xen/git/rumpuser-xen.git
+++ besteffort_repo https://github.com/rumpkernel/rumpkernel-netbsd-src
+++ local repo=https://github.com/rumpkernel/rumpkernel-netbsd-src
+++ cached_repo https://github.com/rumpkernel/rumpkernel-netbsd-src 
'[fetch=try]'
+++ local repo=https://github.com/rumpkernel/rumpkernel-netbsd-src
+++ local 'options=[fetch=try]'
 getconfig GitCacheProxy
 perl -e '
use Osstest;
readglobalconfig();

Re: [Xen-devel] [PATCH v2 2/2] xen: make available hvm_fep to non-debug build as well

[Andrew Cooper]

On 17/06/16 12:34, Jan Beulich wrote:
 On 17.06.16 at 13:05,  wrote:
>> --- a/xen/arch/x86/Kconfig
>> +++ b/xen/arch/x86/Kconfig
>> @@ -59,6 +59,20 @@ config BIGMEM
>>  
>>If unsure, say N.
>>  
>> +config HVM_FEP
>> +bool "HVM Forced Emulation Prefix support"
> And no "if EXPERT"?

Is that wise? Does that mean we get a default on option which can't be
selected in menuconfig?

~Andrew

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

Re: [Xen-devel] [PATCH] xen: sched: use default scheduler upon an invalid "sched="

[Doug Goldstein]

On 6/17/16 5:31 AM, Dario Faggioli wrote:
> instead of just the first scheduler we find in the array.
> 
> In fact, right now, if someone makes a typo when passing
> the "sched=" command line option to Xen, we (with all
> schedulers configured in) pick ARINC653, which is most
> likely not what one would expect.
> 
> Go for the default scheduler instead.
> 
> Signed-off-by: Dario Faggioli 
> ---
> Cc: George Dunlap 
> Cc: Jan Beulich 
> Cc: Doug Goldstein 
> Cc: Jonathan Creekmore 

Reviewed-by: Doug Goldstein 

-- 
Doug Goldstein



signature.asc
Description: OpenPGP digital signature
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] xen: sched: use default scheduler upon an invalid "sched="

[George Dunlap]

On 17/06/16 11:31, Dario Faggioli wrote:
> instead of just the first scheduler we find in the array.
> 
> In fact, right now, if someone makes a typo when passing
> the "sched=" command line option to Xen, we (with all
> schedulers configured in) pick ARINC653, which is most
> likely not what one would expect.
> 
> Go for the default scheduler instead.
> 
> Signed-off-by: Dario Faggioli 

Acked-by: George Dunlap 

I'll check it in here shortly.


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

[Xen-devel] [PATCH XTF] tests: add fep test

[Wei Liu]

Signed-off-by: Wei Liu 
---
 tests/fep/Makefile | 12 
 tests/fep/main.c   | 31 +++
 2 files changed, 43 insertions(+)
 create mode 100644 tests/fep/Makefile
 create mode 100644 tests/fep/main.c

diff --git a/tests/fep/Makefile b/tests/fep/Makefile
new file mode 100644
index 000..8702123
--- /dev/null
+++ b/tests/fep/Makefile
@@ -0,0 +1,12 @@
+MAKEFLAGS += -r
+ROOT := $(abspath $(CURDIR)/../..)
+
+include $(ROOT)/build/common.mk
+
+NAME  := fep
+CATEGORY  := utility
+TEST-ENVS := $(HVM_ENVIRONMENTS)
+
+obj-perenv += main.o
+
+include $(ROOT)/build/gen.mk
diff --git a/tests/fep/main.c b/tests/fep/main.c
new file mode 100644
index 000..34a93c0
--- /dev/null
+++ b/tests/fep/main.c
@@ -0,0 +1,31 @@
+/**
+ * @file tests/fep/main.c
+ * @ref test-fep
+ *
+ * @page test-fep FEP
+ *
+ * Returns SUCCESS if FEP is available, FAILURE if not.
+ *
+ * @sa tests/fep/main.c
+ */
+#include 
+
+void test_main(void)
+{
+printk("Test availability of HVM forced emulation prefix\n");
+
+if ( xtf_has_fep )
+xtf_success(NULL);
+else
+xtf_failure(NULL);
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.1.4


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

Re: [Xen-devel] [PATCH 2/2] hvmloader: don't hard-code IO-APIC parameters

[Jan Beulich]

>>> On 17.06.16 at 13:51,  wrote:
> On 06/17/2016 02:00 AM, Jan Beulich wrote:
> On 16.06.16 at 18:49,  wrote:
>>> On 06/16/2016 05:40 AM, Jan Beulich wrote:
 The IO-APIC address has variable bits determined by the PCI-to-ISA
 bridge, and the IO-APIC version should be read from the IO-APIC. (Note
 that there's still implicit rather than explicit agreement on the
 IO-APIC base address between qemu and the hypervisor.)
>>> It probably doesn't matter now for PVHv2 since we are not going to
>>> initially emulate IOAPIC but when/if we do, how do we query for base
>>> address and version? Should we just rely on the same implicit agreement?
>> If this refers to the sentence in parentheses, then this was really
>> meant to indicate a work item. I.e. qemu should negotiate with
>> Xen. For PVHv2 this would then probably mean for hvmloader to
>> query Xen (via new hypercall).
> 
> We are not using hvmloader for PVH so I am not sure how that would work.
> You mean the toolstack?

Oh, right. Whoever puts the ACPI tables in place.

Jan


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

Re: [Xen-devel] [PATCH] xen: sched: use default scheduler upon an invalid "sched="

[Wei Liu]

On Fri, Jun 17, 2016 at 12:31:00PM +0200, Dario Faggioli wrote:
> instead of just the first scheduler we find in the array.
> 
> In fact, right now, if someone makes a typo when passing
> the "sched=" command line option to Xen, we (with all
> schedulers configured in) pick ARINC653, which is most
> likely not what one would expect.
> 
> Go for the default scheduler instead.
> 
> Signed-off-by: Dario Faggioli 
> ---
> Cc: George Dunlap 
> Cc: Jan Beulich 
> Cc: Doug Goldstein 
> Cc: Jonathan Creekmore 
> ---
> I don't think I'm going to ask for this to be put in 4.7,
> as I don't want to give Wei an heart attack... :-)
> 

I will just say "Meh, let's backport this." :-P

> Considering it for backporting should be enough, IMO.
> ---
>  xen/common/schedule.c |3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/common/schedule.c b/xen/common/schedule.c
> index 5e35310..7ac12d3 100644
> --- a/xen/common/schedule.c
> +++ b/xen/common/schedule.c
> @@ -1625,7 +1625,8 @@ void __init scheduler_init(void)
>  {
>  printk("Could not find scheduler: %s\n", opt_sched);
>  for ( i = 0; i < NUM_SCHEDULERS; i++ )
> -if ( schedulers[i] )
> +if ( schedulers[i] &&
> + !strcmp(schedulers[i]->opt_name, CONFIG_SCHED_DEFAULT) )
>  {
>  ops = *schedulers[i];
>  break;
> 
> 
> ___
> 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