Re: [Xen-devel] [PATCH RFC 28/31] xen/x86: Context switch all levelling state in context_switch()
>>> On 22.01.16 at 15:19, wrote:
> On 22/01/16 09:52, Jan Beulich wrote:
> On 16.12.15 at 22:24, wrote:
>>> @@ -145,6 +145,13 @@ void intel_ctxt_switch_levelling(const struct domain
>>> *nextd)
>>> struct cpumasks *these_masks = &this_cpu(cpumasks);
>>> const struct cpumasks *masks = &cpumask_defaults;
>>>
>>> + if (cpu_has_cpuid_faulting) {
>>> + set_cpuid_faulting(nextd && is_pv_domain(nextd) &&
>>> + !is_control_domain(nextd) &&
>>> + !is_hardware_domain(nextd));
>>> + return;
>>> + }
>> Considering that you don't even probe the masking MSRs, this seems
>> inconsistent with your "always level the entire system" choice.
>
> In the case that faulting is available, we never want to touch masking.
> Faulting is newer and strictly superior to masking.
>
> As documented, there is no hardware which support both. (In reality,
> there is one SKU of IvyBridge CPUs which experimentally have both.)
>
>
> The fact that dom0 and the hardware domain are bypassed is a bug IMO.
And we appear to disagree here. I'd rather see the rest of the
series match this current behavior.
> However, I am preserving the existing behaviour until phase 2 when I fix
> other parts of the cpuid policy. Currently imposing faulting on dom0
> causes carnage because nothing generates it a sane policy.
>
> ~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 1/3] xen-netback: fix license ident used in MODULE_LICENSE
On 22/01/16 14:15, Ian Campbell wrote: > On Fri, 2016-01-22 at 13:49 +, Wei Liu wrote: >> On Fri, Jan 22, 2016 at 01:14:24PM +, David Vrabel wrote: >>> On 22/01/16 12:34, Wei Liu wrote: The comment at the beginning of the file is the canonical source of licenses for this module. Currently it contains GPL and MIT license. Fix the code to reflect the reality. >>> >>> "The MIT license" isn't really a thing. The closest is the X11 >>> license[1], but this not applicable here either since the text in the >>> drivers does not refer to X11 trademarks etc. >>> >> >> That was referring to the license ident string in Linux. If MIT license >> isn't a thing, why would Linux have it at all? > > The fact what include/linux/license.h:license_is_gpl_compatible includes > "Dual MIT/GPL" as an option seems to suggest that it is enough of a thing > to be validly used as the contents of a MODULE_LICENSE() thing. "Dual MIT/GPL" is used exactly once in the source in a file that has no license text and there is no other documentation. David ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH RFC 27/31] xen/x86: Rework Intel masking/faulting setup
>>> On 22.01.16 at 15:09, wrote:
> On 22/01/16 09:40, Jan Beulich wrote:
> On 16.12.15 at 22:24, wrote:
>>> @@ -183,22 +237,13 @@ static void early_init_intel(struct cpuinfo_x86 *c)
>>> (boot_cpu_data.x86_mask == 3 || boot_cpu_data.x86_mask == 4))
>>> paddr_bits = 36;
>>>
>>> - if (c == &boot_cpu_data && c->x86 == 6) {
>>> - if (probe_intel_cpuid_faulting())
>>> - __set_bit(X86_FEATURE_CPUID_FAULTING,
>>> - c->x86_capability);
>>> - } else if (boot_cpu_has(X86_FEATURE_CPUID_FAULTING)) {
>>> - BUG_ON(!probe_intel_cpuid_faulting());
>>> - __set_bit(X86_FEATURE_CPUID_FAULTING, c->x86_capability);
>>> - }
>>> + if (c == &boot_cpu_data)
>>> + intel_init_levelling();
>>> +
>>> + if (test_bit(X86_FEATURE_CPUID_FAULTING, boot_cpu_data.x86_capability))
>>> +__set_bit(X86_FEATURE_CPUID_FAULTING, c->x86_capability);
>> So you intentionally delete the validation of CPUID faulting being
>> available on APs?
>
> Yes. All this does is change where Xen crashes, in the case that AP's
> have different capabilities to the BSP, and allows more startup code to
> move into __init.
So where did that Xen crash point move to (since I didn't spot it)?
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[Xen-devel] [PATCH 0/2] xenalyze: fixes for gcc-6's -Wmisleading-indentation
Debian bug 812166[0] concerned failures due to -Werror=misleading- indentation when building the Xen package. While trying (and failing) to reproduce those failures I came across these two warnings in xenalyze, one relating to misleading indenation and the other for unused code. Cheers, Ian. [0] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=812166 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [PATCH 2/2] xenalyze: remove cr3_compare_total
gcc-6 complains:
xenalyze.c:4132:9: error: 'cr3_compare_total' defined but not used
[-Werror=unused-function]
int cr3_compare_total(const void *_a, const void *_b) {
^
I believe it is correct.
Signed-off-by: Ian Campbell
---
tools/xentrace/xenalyze.c | 17 -
1 file changed, 17 deletions(-)
diff --git a/tools/xentrace/xenalyze.c b/tools/xentrace/xenalyze.c
index 4bcaf83..6520790 100644
--- a/tools/xentrace/xenalyze.c
+++ b/tools/xentrace/xenalyze.c
@@ -4129,23 +4129,6 @@ void cr3_dump_list(struct cr3_value_struct *head){
struct cr3_value_struct **qsort_array;
int i, N=0;
-int cr3_compare_total(const void *_a, const void *_b) {
-struct cr3_value_struct *a=*(typeof(&a))_a;
-struct cr3_value_struct *b=*(typeof(&a))_b;
-
-if(a->total_time.cycles < b->total_time.cycles)
-return 1;
-else if(b->total_time.cycles == a->total_time.cycles) {
-if(a->total_time.count < b->total_time.count)
-return 1;
-else if(a->total_time.count == b->total_time.count)
-return 0;
-else
-return -1;
-} else
-return -1;
-}
-
int cr3_compare_start(const void *_a, const void *_b) {
struct cr3_value_struct *a=*(typeof(&a))_a;
struct cr3_value_struct *b=*(typeof(&a))_b;
--
2.6.1
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[Xen-devel] [PATCH 1/2] xenalyze: fix misleading indentation.
gcc-6 adds -Wmisleading-indentation which found these issues. xenalyze.c: In function 'weighted_percentile': xenalyze.c:2136:18: error: statement is indented as if it were guarded by... [-Werror=misleading-indentation] L=I; L_weight = I_weight; ^~~~ xenalyze.c:2135:9: note: ...this 'if' clause, but it is not if(J_weight --- tools/xentrace/xenalyze.c | 24 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/tools/xentrace/xenalyze.c b/tools/xentrace/xenalyze.c index 5a2735c..4bcaf83 100644 --- a/tools/xentrace/xenalyze.c +++ b/tools/xentrace/xenalyze.c @@ -2132,10 +2132,14 @@ float weighted_percentile(float * A, /* values */ } while (I <= J); /* Keep going until our pointers meet or pass */ /* Re-adjust L and R, based on which element we're looking for */ -if(J_weighthttp://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCHv6 1/3] rwlock: Add per-cpu reader-writer lock infrastructure
On 01/22/2016 02:16 PM, Malcolm Crossley wrote: On 22/01/16 13:54, Ian Campbell wrote: On Fri, 2016-01-22 at 13:41 +, Malcolm Crossley wrote: Changes since v5: - Fix compilation on ARM This was the removal of some spurious "&", I think? Yeah, I forgot to keep the macro's in sync. ARM also needed the xen/percpu.h header file to be explictly included into spinlock.h. Acked-by: Ian Campbell Thanks for catching this! Sorry for not build testing before on ARM. Building the hypervisor itself using a prebuilt linaro toolchain is actually very easy (no chroot required). Do you think it's worth documenting this? So that at least hypervisor build failures are caught. This is already documented (briefly) here: http://wiki.xen.org/wiki/Xen_ARM_with_Virtualization_Extensions#Cross_Compiling -- Ross Lagerwall ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH RFC 29/31] x86/pv: Provide custom cpumasks for PV domains
On 22/01/16 09:56, Jan Beulich wrote:
On 16.12.15 at 22:24, wrote:
>> --- a/xen/arch/x86/cpu/amd.c
>> +++ b/xen/arch/x86/cpu/amd.c
>> @@ -203,7 +203,9 @@ static void __init noinline probe_masking_msrs(void)
>> void amd_ctxt_switch_levelling(const struct domain *nextd)
>> {
>> struct cpumasks *these_masks = &this_cpu(cpumasks);
>> -const struct cpumasks *masks = &cpumask_defaults;
>> +const struct cpumasks *masks =
>> +(nextd && is_pv_domain(nextd) && nextd->arch.pv_domain.masks)
>> +? nextd->arch.pv_domain.masks : &cpumask_defaults;
> Can nextd really ever be NULL here?
Yes, when using this function to set the defaults in the first place
during AP bringup. Care also needs to be taken on the BSP where current
isn't valid either.
>
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -578,6 +578,12 @@ int arch_domain_create(struct domain *d, unsigned int
>> domcr_flags,
>> goto fail;
>> clear_page(d->arch.pv_domain.gdt_ldt_l1tab);
>>
>> +d->arch.pv_domain.masks = xmalloc(struct cpumasks);
>> +if ( !d->arch.pv_domain.masks )
>> +goto fail;
>> +memcpy(d->arch.pv_domain.masks, &cpumask_defaults,
>> + sizeof(*d->arch.pv_domain.masks));
> Structure assignment, to make the thing type safe?
>
> Also there's a change missing to the cleanup code after the "fail"
> label.
What change are you thinking of? I suppose an xfree() wouldn't go amis,
to prevent a problem for whomever introduces a new failure path, but I
don't see a bug in the code as-is.
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCHv6 1/3] rwlock: Add per-cpu reader-writer lock infrastructure
On Fri, 2016-01-22 at 14:16 +, Malcolm Crossley wrote: > On 22/01/16 13:54, Ian Campbell wrote: > > On Fri, 2016-01-22 at 13:41 +, Malcolm Crossley wrote: > > > > > > Changes since v5: > > > - Fix compilation on ARM > > > > This was the removal of some spurious "&", I think? > > Yeah, I forgot to keep the macro's in sync. ARM also needed the > xen/percpu.h header file to be > explictly included into spinlock.h. > > > > > Acked-by: Ian Campbell > > > > Thanks for catching this! > > Sorry for not build testing before on ARM. No problem, some people never remember at all, so v5 isn't too bad ;-) > Building the hypervisor itself using a prebuilt linaro > toolchain is actually very easy (no chroot required). Indeed. > Do you think it's worth documenting this? So that at least hypervisor > build failures are caught. I think it's covered by http://wiki.xen.org/wiki/Xen_ARM_with_Virtualization_Extensions#Cross_Compiling or do you mean something more? Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] xen: Add support for dom0 with Linux kernel 3.19 and newer
On Fri, Jan 22, 2016 at 10:03:34AM +, David Vrabel wrote: > On 21/01/16 20:13, Daniel Kiper wrote: > > Linux kernel commit 054954eb051f35e74b75a566a96fe756015352c8 > > (xen: switch to linear virtual mapped sparse p2m list), which > > appeared in 3.19, introduced linear virtual mapped sparse p2m > > list. If readmem() reads p2m then it access this list using > > physical addresses. Sadly, VMA to physical address translation > > in crash requires access to p2m list. This means that we have > > a chicken and egg problem. In general this issue must be solved > > by introducing some changes in libxl, Linux kernel and crash > > (I have added this task to my long TODO list). However, in dom0 > > case we can use crash_xen_info_t.dom0_pfn_to_mfn_frame_list_list > > which is available out of the box. So, let's use it and make > > at least some users happy. > > I'm confused. How does a virtual address to (pseudo-)physical address > lookup require access to the p2m? Surely this is a walk of the page > tables followed by a M2P lookup on the MFN in the L1 PTE? Correct but crash does it a bit backward and scans p2m. I am not sure way, however, I am going to look at it during work on PV guests support. Daniel ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [xen-unstable test] 78703: regressions - FAIL
flight 78703 xen-unstable real [real] http://logs.test-lab.xenproject.org/osstest/logs/78703/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-armhf-armhf-xl-credit2 15 guest-start/debian.repeat fail REGR. vs. 78610 test-armhf-armhf-xl 15 guest-start/debian.repeat fail REGR. vs. 78610 Regressions which are regarded as allowable (not blocking): test-armhf-armhf-xl-rtds 11 guest-start fail REGR. vs. 78610 test-amd64-i386-rumpuserxen-i386 10 guest-startfail like 78610 test-amd64-i386-xl-qemut-stubdom-debianhvm-amd64-xsm 9 debian-hvm-install fail like 78610 test-amd64-amd64-xl-qemut-win7-amd64 16 guest-stop fail like 78610 test-amd64-i386-xl-qemuu-win7-amd64 16 guest-stop fail like 78610 Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-pvh-amd 11 guest-start fail never pass test-amd64-amd64-xl-pvh-intel 11 guest-start fail never pass test-armhf-armhf-libvirt 14 guest-saverestorefail never pass test-armhf-armhf-libvirt 12 migrate-support-checkfail 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-amd64-qemuu-nested-amd 16 debian-hvm-install/l1/l2 fail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check fail never pass test-amd64-amd64-xl-qemuu-win7-amd64 16 guest-stop fail never pass test-amd64-i386-libvirt 12 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 12 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 12 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 13 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check fail never pass test-armhf-armhf-xl-multivcpu 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 12 migrate-support-checkfail never pass test-armhf-armhf-xl 12 migrate-support-checkfail never pass test-armhf-armhf-xl 13 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-xsm 12 migrate-support-checkfail never pass test-armhf-armhf-libvirt-xsm 14 guest-saverestorefail never pass test-amd64-amd64-libvirt-vhd 11 migrate-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 11 migrate-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 13 guest-saverestorefail never pass test-armhf-armhf-libvirt-raw 13 guest-saverestorefail never pass test-armhf-armhf-libvirt-raw 11 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 12 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-xsm 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-xsm 12 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 11 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 12 saverestore-support-checkfail never pass test-amd64-i386-xl-qemut-win7-amd64 16 guest-stop fail never pass version targeted for testing: xen fa86649c4800bac92c7ba69ec8a16475df3e76f7 baseline version: xen 1949868d640427dc91bfb23741d78eb1d86841c8 Last test of basis78610 2016-01-20 10:56:23 Z2 days Testing same since78703 2016-01-21 17:20:38 Z0 days1 attempts People who touched revisions under test: Ian Campbell Jan Beulich Wen Congyang jobs: build-amd64-xsm pass build-armhf-xsm pass build-i386-xsm pass build-amd64 pass build-armhf pass build-i386 pass build-amd64-libvirt pass build-armhf-libvirt pass build-i386-libvirt pass build-amd64-oldkern pass build-i386-oldkern pass build-amd64-prev pass build-i386-prev
Re: [Xen-devel] [PATCH RFC 28/31] xen/x86: Context switch all levelling state in context_switch()
On 22/01/16 09:52, Jan Beulich wrote:
On 16.12.15 at 22:24, wrote:
>> --- a/xen/arch/x86/cpu/amd.c
>> +++ b/xen/arch/x86/cpu/amd.c
>> @@ -300,6 +300,9 @@ static void __init noinline amd_init_levelling(void)
>> cpumask_defaults._6c &= (~0ULL << 32);
>> cpumask_defaults._6c |= ecx;
>> }
>> +
>> +if (levelling_caps)
>> +ctxt_switch_levelling = amd_ctxt_switch_levelling;
>> }
> Indentation.
>
>> --- a/xen/arch/x86/cpu/common.c
>> +++ b/xen/arch/x86/cpu/common.c
>> @@ -86,6 +86,13 @@ static const struct cpu_dev default_cpu = {
>> };
>> static const struct cpu_dev *this_cpu = &default_cpu;
>>
>> +void default_ctxt_switch_levelling(const struct domain *nextd)
> static
>
>> +{
>> +/* Nop */
>> +}
>> +void (*ctxt_switch_levelling)(const struct domain *nextd) __read_mostly =
>> +default_ctxt_switch_levelling;
> While current and past gcc may accept (and honor) this placement of
> the __read_mostly annotation, I think it is wrong from a strict language
> syntax pov. Imo it instead ought to be
>
> void (*__read_mostly ctxt_switch_levelling)(const struct domain *nextd) =
>
> Also - indentation again.
>
>> @@ -145,6 +145,13 @@ void intel_ctxt_switch_levelling(const struct domain
>> *nextd)
>> struct cpumasks *these_masks = &this_cpu(cpumasks);
>> const struct cpumasks *masks = &cpumask_defaults;
>>
>> +if (cpu_has_cpuid_faulting) {
>> +set_cpuid_faulting(nextd && is_pv_domain(nextd) &&
>> + !is_control_domain(nextd) &&
>> + !is_hardware_domain(nextd));
>> +return;
>> +}
> Considering that you don't even probe the masking MSRs, this seems
> inconsistent with your "always level the entire system" choice.
In the case that faulting is available, we never want to touch masking.
Faulting is newer and strictly superior to masking.
As documented, there is no hardware which support both. (In reality,
there is one SKU of IvyBridge CPUs which experimentally have both.)
The fact that dom0 and the hardware domain are bypassed is a bug IMO.
However, I am preserving the existing behaviour until phase 2 when I fix
other parts of the cpuid policy. Currently imposing faulting on dom0
causes carnage because nothing generates it a sane policy.
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCHv6 1/3] rwlock: Add per-cpu reader-writer lock infrastructure
On 22/01/16 13:54, Ian Campbell wrote: > On Fri, 2016-01-22 at 13:41 +, Malcolm Crossley wrote: >> >> Changes since v5: >> - Fix compilation on ARM > > This was the removal of some spurious "&", I think? Yeah, I forgot to keep the macro's in sync. ARM also needed the xen/percpu.h header file to be explictly included into spinlock.h. > > Acked-by: Ian Campbell > > Thanks for catching this! Sorry for not build testing before on ARM. Building the hypervisor itself using a prebuilt linaro toolchain is actually very easy (no chroot required). Do you think it's worth documenting this? So that at least hypervisor build failures are caught. Malcolm ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 1/3] xen-netback: fix license ident used in MODULE_LICENSE
On Fri, 2016-01-22 at 13:49 +, Wei Liu wrote: > On Fri, Jan 22, 2016 at 01:14:24PM +, David Vrabel wrote: > > On 22/01/16 12:34, Wei Liu wrote: > > > The comment at the beginning of the file is the canonical source of > > > licenses for this module. Currently it contains GPL and MIT license. > > > Fix > > > the code to reflect the reality. > > > > "The MIT license" isn't really a thing. The closest is the X11 > > license[1], but this not applicable here either since the text in the > > drivers does not refer to X11 trademarks etc. > > > > That was referring to the license ident string in Linux. If MIT license > isn't a thing, why would Linux have it at all? The fact what include/linux/license.h:license_is_gpl_compatible includes "Dual MIT/GPL" as an option seems to suggest that it is enough of a thing to be validly used as the contents of a MODULE_LICENSE() thing. It's also in https://opensource.org/licenses/MIT , the fact that it might be confused for other licenses used by MIT notwithstanding. FWIW https://en.wikipedia.org/wiki/MIT_License seems to think that the wording most commonly called the "MIT License" might be the "Expat license", rather than the "X11 License" which is similar but different. Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [Minios-devel] [PATCH v8 0/] Begin to disentangle libxenctrl and provide some stable libraries
On Fri, 2016-01-15 at 13:22 +, Ian Campbell wrote: > In <1431963008.4944.80.ca...@citrix.com> I proposed stabilising some > parts of the libxenctrl API/ABI by disaggregating into separate > libraries. > > This is v8 of that set of series against: > xen > qemu-xen > qemu-xen-traditional > mini-os I have now applied all xen, qemu-xen-traditional and mini-os, with the one patch updated to v9 in the xen series (posted earlier today). I omitted the final two patches from the Xen side: [28/29] tools/libs/*: Introduce APIs to restrict handles to a specific domain. [29/29] HACK: Update Config.mk to pull all the right bits from my xenbits trees #28 isn't quite ready/agreed yet. (I hope #29 is obvious...) Stefano, you can go ahead with the qemu-xen part whenever. Maybe it would be worth waiting for the xen push gate to succeed first though? At least waiting for a xen-unstable-smoke flight would be wise. Ian, you weren't around to discuss the qemu-xen-traditional push with, but I took a gamble that you would be ok with me doing so on this occasion. I hope that's ok. I modified the 4 patches with updated MINIOS_UPSTREAM_REVISION and QEMU_TRADITIONAL_REVISION where called for. Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH RFC 26/31] xen/x86: Rework AMD masking MSR setup
>>> On 22.01.16 at 14:59, wrote: > On 22/01/16 11:13, Jan Beulich wrote: > On 22.01.16 at 12:01, wrote: >>> On 22/01/16 09:27, Jan Beulich wrote: >>> On 16.12.15 at 22:24, wrote: > +expected_levelling_cap, levelling_caps, > +(expected_levelling_cap ^ levelling_caps) & levelling_caps); > + printk(XENLOG_WARNING "Fam %#x, model %#x level %#x\n", > +c->x86, c->x86_model, c->cpuid_level); > + printk(XENLOG_WARNING > +"If not running virtualised, please report a bug\n"); Well - you checked for running virtualized, so making it here when running virtualized is already a bug (albeit not in the code here)? >>> Why would it be a bug? The virtualising environment might provide these >>> MSRs, in which case we should use them. >> Because you won't make it here when cpu_has_hypervisor. > > Not all hypervisors advertise themselves with the hypervisor bit. Some > versions of HyperV and ESXi will panic if they see a hypervisor bit, so > virtualisation software needs to hide the hypervisor bit to successfully > visualise some software. Well, but that's the bug I talk of: Not setting that CPUID bit is not following the spec, and forcing it off to make some code work in a virtualized environment isn't much better. > The user reading Xen's error message is in a better position to judge > when Xen is actually virtualised, because Xen is not able to say with > certainty. Okay, okay, bit that way then. And then, how is this supposed to work? You only restore defaults, but never write non-default values. Namely, nextd is an unused function parameter ... Also I guess my comment about adding unused code needs repeating here. >>> Future patches build on this, both using the parameter, and not using >>> the defaults. >>> >>> I am also certain that if I did two patches, the first putting in a void >>> function, and the second changing it to a pointer, your review would ask >>> me to turn it into this. >> Well, I realize things will all make sense by the end of the series, but >> honestly in some of the cases I'm not sure the split between patches >> was well done. > > If you can suggest a better ordering then I am all ears. For example, move all the context switch logic into the patch actually invoking the new hook. That still leaves more than enough in the AMD and Intel rework patches. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH RFC 27/31] xen/x86: Rework Intel masking/faulting setup
On 22/01/16 09:40, Jan Beulich wrote:
On 16.12.15 at 22:24, wrote:
>> +if (msr_basic)
>> +__probe_mask_msr(&msr_basic, LCAP_1cd, &cpumask_defaults._1cd);
>> +
>> +if (msr_ext)
>> +__probe_mask_msr(&msr_ext, LCAP_e1cd, &cpumask_defaults.e1cd);
>> +
>> +if (msr_xsave)
>> +__probe_mask_msr(&msr_xsave, LCAP_Da1, &cpumask_defaults.Da1);
>> +
>> +/*
>> + * Don't bother warning about a mismatch if virtualised. These MSRs
>> + * are not architectural and almost never virtualised.
>> + */
>> +if ((expected_levelling_cap == levelling_caps) ||
>> +cpu_has_hypervisor)
>> +return;
>> +
>> +printk(XENLOG_WARNING "Mismatch between expected (%#x"
>> + ") and real (%#x) levelling caps: missing %#x\n",
>> + expected_levelling_cap, levelling_caps,
>> + (expected_levelling_cap ^ levelling_caps) & levelling_caps);
>> +printk(XENLOG_WARNING "Fam %#x, model %#x expected (%#x/%#x/%#x), "
>> + "got (%#x/%#x/%#x)\n", c->x86, c->x86_model,
>> + exp_msr_basic, exp_msr_ext, exp_msr_xsave,
>> + msr_basic, msr_ext, msr_xsave);
> I may not have noticed the same on the AMD patch, but printing
> zeros as "missing" MSR indexes seems strange to me. Why not
> print the missing MSRs with their textual names, easing cross
> referencing with the FlexMigration document?
AMD and Intel are different in this regard. AMD's masking MSRs are at
fixed addresses, while Intel's vary MSR address by generation, which is
why I stored the probed address in a variable.
There are not consistent names available. In this case, I think the
numbers are actually clearer than the names.
>
>> +/*
>> + * opt_cpuid_mask_ecx/edx: cpuid.1[ecx, edx] feature mask.
>> + * For example, E8400[Intel Core 2 Duo Processor series] ecx = 0x0008E3FD,
>> + * edx = 0xBFEBFBFF when executing CPUID.EAX = 1 normally. If you want to
>> + * 'rev down' to E8400, you can set these values in these Xen boot
>> parameters.
>> + */
>> +static void __init noinline intel_init_levelling(void)
>> +{
>> +if ( !probe_intel_cpuid_faulting() )
>> +probe_masking_msrs();
>> +
>> +if ( msr_basic )
>> +{
>> +cpumask_defaults._1cd =
>> +((u64)opt_cpuid_mask_edx << 32) | opt_cpuid_mask_ecx;
>> +
>> +if ( !~(opt_cpuid_mask_ecx & opt_cpuid_mask_edx) )
>> printk("Writing CPUID feature mask ecx:edx ->
>> %08x:%08x\n",
>> opt_cpuid_mask_ecx, opt_cpuid_mask_edx);
> Are these messages, without adjustment to their wording, not
> going to be confusing? After all the intention is to not just write
> a single, never modified value. E.g. better "Defaulting ..."?
I will change the wording. It would be better than confusing a new
different meaning with the old written message.
>
>> @@ -183,22 +237,13 @@ static void early_init_intel(struct cpuinfo_x86 *c)
>> (boot_cpu_data.x86_mask == 3 || boot_cpu_data.x86_mask == 4))
>> paddr_bits = 36;
>>
>> -if (c == &boot_cpu_data && c->x86 == 6) {
>> -if (probe_intel_cpuid_faulting())
>> -__set_bit(X86_FEATURE_CPUID_FAULTING,
>> - c->x86_capability);
>> -} else if (boot_cpu_has(X86_FEATURE_CPUID_FAULTING)) {
>> -BUG_ON(!probe_intel_cpuid_faulting());
>> -__set_bit(X86_FEATURE_CPUID_FAULTING, c->x86_capability);
>> -}
>> +if (c == &boot_cpu_data)
>> +intel_init_levelling();
>> +
>> +if (test_bit(X86_FEATURE_CPUID_FAULTING, boot_cpu_data.x86_capability))
>> +__set_bit(X86_FEATURE_CPUID_FAULTING, c->x86_capability);
> So you intentionally delete the validation of CPUID faulting being
> available on APs?
Yes. All this does is change where Xen crashes, in the case that AP's
have different capabilities to the BSP, and allows more startup code to
move into __init.
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[Xen-devel] [PATCH v2 3/3] xen-scsiback: fix license ident used in MODULE_LICENSE
The comment at the beginning of the file is the canonical source of
licenses for this module. The license used to distribute outside of
Linux kernel is not BSD license but X11 license. Instead of trying to
determine whether X11 license can be mapped into Linux's "MIT" license
ident, simply make the code to use "GPL" only.
Signed-off-by: Wei Liu
---
drivers/xen/xen-scsiback.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
index ad4eb10..250d70f 100644
--- a/drivers/xen/xen-scsiback.c
+++ b/drivers/xen/xen-scsiback.c
@@ -1915,6 +1915,6 @@ module_init(scsiback_init);
module_exit(scsiback_exit);
MODULE_DESCRIPTION("Xen SCSI backend driver");
-MODULE_LICENSE("Dual BSD/GPL");
+MODULE_LICENSE("GPL");
MODULE_ALIAS("xen-backend:vscsi");
MODULE_AUTHOR("Juergen Gross ");
--
2.1.4
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[Xen-devel] [PATCH v2 2/3] xen-blkback: fix license ident used in MODULE_LICENSE
The comment at the beginning of the file is the canonical source of
licenses for this module. The license used to distribute outside of
Linux kernel is not BSD license but X11 license. Instead of trying to
determine whether X11 license can be mapped into Linux's "MIT" license
ident, simply make the code to use "GPL" only.
Signed-off-by: Wei Liu
---
drivers/block/xen-blkback/blkback.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/block/xen-blkback/blkback.c
b/drivers/block/xen-blkback/blkback.c
index 4809c15..1cb29a2 100644
--- a/drivers/block/xen-blkback/blkback.c
+++ b/drivers/block/xen-blkback/blkback.c
@@ -1496,5 +1496,5 @@ static int __init xen_blkif_init(void)
module_init(xen_blkif_init);
-MODULE_LICENSE("Dual BSD/GPL");
+MODULE_LICENSE("GPL");
MODULE_ALIAS("xen-backend:vbd");
--
2.1.4
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[Xen-devel] [PATCH v2 0/3] xen: fix wrong idents in MODULE_LICENSE in some drivers
Wei Liu (3): xen-netback: fix license ident used in MODULE_LICENSE xen-blkback: fix license ident used in MODULE_LICENSE xen-scsiback: fix license ident used in MODULE_LICENSE drivers/block/xen-blkback/blkback.c | 2 +- drivers/net/xen-netback/netback.c | 2 +- drivers/xen/xen-scsiback.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) -- 2.1.4 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [PATCH v2 1/3] xen-netback: fix license ident used in MODULE_LICENSE
The comment at the beginning of the file is the canonical source of
licenses for this module. The license used to distribute outside of
Linux kernel is not BSD license but X11 license. Instead of trying to
determine whether X11 license can be mapped into Linux's "MIT" license
ident, simply make the code to use "GPL" only.
Signed-off-by: Wei Liu
---
drivers/net/xen-netback/netback.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/xen-netback/netback.c
b/drivers/net/xen-netback/netback.c
index 61b97c3..e9601d1 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -2192,5 +2192,5 @@ static void __exit netback_fini(void)
}
module_exit(netback_fini);
-MODULE_LICENSE("Dual BSD/GPL");
+MODULE_LICENSE("GPL");
MODULE_ALIAS("xen-backend:vif");
--
2.1.4
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH RFC 26/31] xen/x86: Rework AMD masking MSR setup
On 22/01/16 11:13, Jan Beulich wrote:
On 22.01.16 at 12:01, wrote:
>> On 22/01/16 09:27, Jan Beulich wrote:
>> On 16.12.15 at 22:24, wrote:
+ expected_levelling_cap, levelling_caps,
+ (expected_levelling_cap ^ levelling_caps) & levelling_caps);
+ printk(XENLOG_WARNING "Fam %#x, model %#x level %#x\n",
+ c->x86, c->x86_model, c->cpuid_level);
+ printk(XENLOG_WARNING
+ "If not running virtualised, please report a bug\n");
>>> Well - you checked for running virtualized, so making it here when
>>> running virtualized is already a bug (albeit not in the code here)?
>> Why would it be a bug? The virtualising environment might provide these
>> MSRs, in which case we should use them.
> Because you won't make it here when cpu_has_hypervisor.
Not all hypervisors advertise themselves with the hypervisor bit. Some
versions of HyperV and ESXi will panic if they see a hypervisor bit, so
virtualisation software needs to hide the hypervisor bit to successfully
visualise some software.
The user reading Xen's error message is in a better position to judge
when Xen is actually virtualised, because Xen is not able to say with
certainty.
>
+void amd_ctxt_switch_levelling(const struct domain *nextd)
+{
+ struct cpumasks *these_masks = &this_cpu(cpumasks);
+ const struct cpumasks *masks = &cpumask_defaults;
+
+#define LAZY(cap, msr, field)
\
+ ({ \
+ if ( ((levelling_caps & cap) == cap) && \
+ (these_masks->field != masks->field) ) \
+ { \
+ wrmsr_amd(msr, masks->field); \
+ these_masks->field = masks->field; \
+ } \
+ })
+
+ LAZY(LCAP_1cd, MSR_K8_FEATURE_MASK, _1cd);
+ LAZY(LCAP_e1cd, MSR_K8_EXT_FEATURE_MASK, e1cd);
+ LAZY(LCAP_7ab0, MSR_AMD_L7S0_FEATURE_MASK, _7ab0);
+ LAZY(LCAP_6c, MSR_AMD_THRM_FEATURE_MASK, _6c);
>>> So here we already have the first example where fully consistent
>>> naming would allow elimination of a macro parameter.
>> Token concatenation deliberately obscures code from tool like grep and
>> cscope. There is already too much of the Xen source code obscured like
>> this; I'd prefer not to add to it.
> I'm not sure grep-abilty is more important than code readability.
> My personal opinion surely is that the latter is more important.
It doesn't matter how readable the code is if you can't find it again later.
And in this case, the difference between 2 and 3 macro parameters
doesn't affect the readability of the code. All context is available in
the preceding 10 lines.
>
>>> And then, how is this supposed to work? You only restore defaults,
>>> but never write non-default values. Namely, nextd is an unused
>>> function parameter ...
>>>
>>> Also I guess my comment about adding unused code needs
>>> repeating here.
>> Future patches build on this, both using the parameter, and not using
>> the defaults.
>>
>> I am also certain that if I did two patches, the first putting in a void
>> function, and the second changing it to a pointer, your review would ask
>> me to turn it into this.
> Well, I realize things will all make sense by the end of the series, but
> honestly in some of the cases I'm not sure the split between patches
> was well done.
If you can suggest a better ordering then I am all ears.
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC V2] xen: interface: introduce pvclk interface
On Fri, 22 Jan 2016, Jan Beulich wrote: > >>> On 22.01.16 at 13:12, wrote: > > On Fri, Jan 22, 2016 at 03:25:40AM -0700, Jan Beulich wrote: > >>In particular, with the user space exposure of clock control > >>discussed in another sub-thread, the next best option would > >>seem to be to handle this via emulation in a device model. Yes, > >>ARM guests currently have no qemu attached to them, but I > >>guess sooner or later this will need to change anyway. > > > > I have not look into qemu for xen. > > If using qemu, then we still need to expose the clk interface to userspace? > > I think so, yes. In fact - see above - the discussed userspace > exposure made me consider this option. I would think twice before introducing QEMU on Xen on ARM for many reasons. I don't think that a simple clock is worth the downsides. If we really have to emulate it, we could do that in Xen. We could have an extremely simple clock that only takes one or two commands. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCHv6 1/3] rwlock: Add per-cpu reader-writer lock infrastructure
On Fri, 2016-01-22 at 13:41 +, Malcolm Crossley wrote: > > Changes since v5: > - Fix compilation on ARM This was the removal of some spurious "&", I think? Acked-by: Ian Campbell Thanks for catching this! ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 1/3] xen-netback: fix license ident used in MODULE_LICENSE
On Fri, Jan 22, 2016 at 01:14:24PM +, David Vrabel wrote:
> On 22/01/16 12:34, Wei Liu wrote:
> > The comment at the beginning of the file is the canonical source of
> > licenses for this module. Currently it contains GPL and MIT license. Fix
> > the code to reflect the reality.
>
> "The MIT license" isn't really a thing. The closest is the X11
> license[1], but this not applicable here either since the text in the
> drivers does not refer to X11 trademarks etc.
>
That was referring to the license ident string in Linux. If MIT license
isn't a thing, why would Linux have it at all?
> You can either use "GPL" which would be correct for a Linux kernel
> module since the alternate only applies when distributed separately from
> Linux ("or, when distributed separately from the Linux kernel or
> incorporated into other software packages, subject to the following
> license:"); or you can use "GPL and additional rights".
>
> (Or you could just leave it as-is since "Dual BSD/GPL" is close enough.)
>
No, I don't want to leave it as-is. That's not BSD license.
I can change that to "GPL". That is acceptable to me.
Wei.
> David
>
> [1] http://www.gnu.org/licenses/license-list.html#X11License
>
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC v1 0/8] x86/init: Linux linker tables
Hi,
On Thu, 21 Jan 2016, H. Peter Anvin wrote:
> Something that confuses me is that gcc seems to give these sections the
> "aw" attributes which makes as complain. This might be a gcc bug.
Workaround: use an (possibly empty) intializer:
struct foo {int i;};
const struct foo
__attribute__((used,section(".rodata.tbl.tablename.0"))) tablename[0] = {};
Ciao,
Michael.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [BUG] XEN domU crash when PV grub chainloads 32-bit domU grub
On 22/01/16 13:08, Vladimir 'φ-coder/phcoder' Serbinenko wrote: > On 22.01.2016 14:01, Andrew Cooper wrote: >> On 22/01/16 12:56, Vladimir 'φ-coder/phcoder' Serbinenko wrote: >>> On 22.09.2015 10:53, Ian Campbell wrote: Hi Vladimir & grub-devel, Do you have any thoughts on this issue with i386 pv-grub2? >>> Is it still an issue? If so I'll try to replicate it. From stack dump I >>> see that it has jumped to NULL. GRUB has no threads so it's not a race >>> condition with itself but may be one with some Xen part. An altrnative >>> possibility is that grub forgets to flush cache at some point in boot >>> process. >> Looks like GRUB doesn't have a traptable registered with Xen (the PV >> equivalent of the IDT). >> >> First, Xen tried to inject a #GP fault and found that the entry EIP was >> at 0 (which is sadly the default if nothing is specified). It then took >> a pagefault while attempting to inject the #GP, and crashed the domain. >> > Do you have a link how to add one? We can put a catch-stacktrace-abort > on it. This is from my microkernel framework, and is probably the most succinct code implementation: http://xenbits.xen.org/gitweb/?p=people/andrewcoop/xen-test-framework.git;a=blob;f=arch/x86/pv/traps.c;h=7f9a1908d260659c10f5cbb1d2d234c9fea1edb5;hb=HEAD#l31 The hypercall ABI documentation is: http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/include/public/arch-x86/xen.h;h=cdd93c1c6446a92e89188c6a5132538188825d27;hb=refs/heads/staging#l126 ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [PATCHv6 0/3] Implement per-cpu reader-writer locks
This patch series adds per-cpu reader-writer locks as a generic lock implementation and then converts the grant table and p2m rwlocks to use the percpu rwlocks, in order to improve multi-socket host performance. CPU profiling has revealed the rwlocks themselves suffer from severe cache line bouncing due to the cmpxchg operation used even when taking a read lock. Multiqueue paravirtualised I/O results in heavy contention of the grant table and p2m read locks of a specific domain and so I/O throughput is bottlenecked by the overhead of the cache line bouncing itself. Per-cpu read locks avoid lock cache line bouncing by using a per-cpu data area to record a CPU has taken the read lock. Correctness is enforced for the write lock by using a per lock barrier which forces the per-cpu read lock to revert to using a standard read lock. The write lock then polls all the percpu data area until active readers for the lock have exited. Removing the cache line bouncing on a multi-socket Haswell-EP system dramatically improves performance, with 16 vCPU network IO performance going from 15 gb/s to 64 gb/s! The host under test was fully utilising all 40 logical CPU's at 64 gb/s, so a bigger logical CPU host may see an even better IO improvement. Note: Benchmarking of the these performance improvements should be done with the non debug version of the hypervisor otherwise the map_domain_page spinlock is the main bottleneck. Changes in V6: - Fix compilation on ARM Changes in V5: - Move percpu_owner ASSERTS to be inline function - Rename grant table rwlock wrappers Changes in V4: - Fix the ASSERTS for the percpu_owner check Changes in V3: - Add percpu rwlock owner for debug Xen builds - Validate percpu rwlock owner at runtime for debug Xen builds - Fix hard tab issues - Use percpu rwlock wrappers for grant table rwlock users - Add comments why rw_is_locked ASSERTS have been removed in grant table code Changes in V2: - Add Cover letter - Convert p2m rwlock to percpu rwlock - Improve percpu rwlock to safely handle simultaneously holding 2 or more locks - Move percpu rwlock barrier from global to per lock - Move write lock cpumask variable to a percpu variable - Add macros to help initialise and use percpu rwlocks - Updated IO benchmark results to cover revised locking implementation Malcolm Crossley (3): rwlock: Add per-cpu reader-writer lock infrastructure grant_table: convert grant table rwlock to percpu rwlock p2m: convert p2m rwlock to percpu rwlock xen/arch/arm/mm.c | 4 +- xen/arch/x86/mm.c | 4 +- xen/arch/x86/mm/mm-locks.h| 12 ++-- xen/arch/x86/mm/p2m.c | 1 + xen/common/grant_table.c | 124 +++--- xen/common/spinlock.c | 45 +++ xen/include/asm-arm/percpu.h | 5 ++ xen/include/asm-x86/mm.h | 2 +- xen/include/asm-x86/percpu.h | 6 ++ xen/include/xen/grant_table.h | 24 +++- xen/include/xen/percpu.h | 4 ++ xen/include/xen/spinlock.h| 116 +++ 12 files changed, 281 insertions(+), 66 deletions(-) -- 1.7.12.4 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [PATCHv6 2/3] grant_table: convert grant table rwlock to percpu rwlock
The per domain grant table read lock suffers from significant contention when
performance multi-queue block or network IO due to the parallel
grant map/unmaps/copies occurring on the DomU's grant table.
On multi-socket systems, the contention results in the locked compare swap
operation failing frequently which results in a tight loop of retries of the
compare swap operation. As the coherency fabric can only support a specific
rate of compare swap operations for a particular data location then taking
the read lock itself becomes a bottleneck for grant operations.
Standard rwlock performance of a single VIF VM-VM transfer with 16 queues
configured was limited to approximately 15 gbit/s on a 2 socket Haswell-EP
host.
Percpu rwlock performance with the same configuration is approximately
48 gbit/s.
Oprofile was used to determine the initial overhead of the read-write locks
and to confirm the overhead was dramatically reduced by the percpu rwlocks.
Signed-off-by: Malcolm Crossley
Acked-by: Ian Campbell
--
Changes since v5:
- None
Changes since v4:
- Rename grant table rwlock wrappers and use grant table pointer as argument
Changes since v3:
- None
Changes since v2:
- Switched to using wrappers for taking percpu rwlock
- Added percpu structure pointer to percpu rwlock initialisation
- Added comment on removal of ASSERTS for grant table rw_is_locked()
Changes since v1:
- Used new macros provided in updated percpu rwlock v2 patch
- Converted grant table rwlock_t to percpu_rwlock_t
- Patched a missed grant table rwlock_t usage site
---
xen/arch/arm/mm.c | 4 +-
xen/arch/x86/mm.c | 4 +-
xen/common/grant_table.c | 124 +++---
xen/include/xen/grant_table.h | 24 +++-
4 files changed, 96 insertions(+), 60 deletions(-)
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 47bfb27..81f9e2e 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1055,7 +1055,7 @@ int xenmem_add_to_physmap_one(
switch ( space )
{
case XENMAPSPACE_grant_table:
-write_lock(&d->grant_table->lock);
+grant_write_lock(d->grant_table);
if ( d->grant_table->gt_version == 0 )
d->grant_table->gt_version = 1;
@@ -1085,7 +1085,7 @@ int xenmem_add_to_physmap_one(
t = p2m_ram_rw;
-write_unlock(&d->grant_table->lock);
+grant_write_unlock(d->grant_table);
break;
case XENMAPSPACE_shared_info:
if ( idx != 0 )
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index b81d1fd..a5a9b6f 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4671,7 +4671,7 @@ int xenmem_add_to_physmap_one(
mfn = virt_to_mfn(d->shared_info);
break;
case XENMAPSPACE_grant_table:
-write_lock(&d->grant_table->lock);
+grant_write_lock(d->grant_table);
if ( d->grant_table->gt_version == 0 )
d->grant_table->gt_version = 1;
@@ -4693,7 +4693,7 @@ int xenmem_add_to_physmap_one(
mfn = virt_to_mfn(d->grant_table->shared_raw[idx]);
}
-write_unlock(&d->grant_table->lock);
+grant_write_unlock(d->grant_table);
break;
case XENMAPSPACE_gmfn_range:
case XENMAPSPACE_gmfn:
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 5d52d1e..6a536d2 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -178,6 +178,8 @@ struct active_grant_entry {
#define _active_entry(t, e) \
((t)->active[(e)/ACGNT_PER_PAGE][(e)%ACGNT_PER_PAGE])
+DEFINE_PERCPU_RWLOCK_GLOBAL(grant_rwlock);
+
static inline void gnttab_flush_tlb(const struct domain *d)
{
if ( !paging_mode_external(d) )
@@ -208,7 +210,13 @@ active_entry_acquire(struct grant_table *t, grant_ref_t e)
{
struct active_grant_entry *act;
-ASSERT(rw_is_locked(&t->lock));
+/*
+ * The grant table for the active entry should be locked but the
+ * percpu rwlock cannot be checked for read lock without race conditions
+ * or high overhead so we cannot use an ASSERT
+ *
+ * ASSERT(rw_is_locked(&t->lock));
+ */
act = &_active_entry(t, e);
spin_lock(&act->lock);
@@ -270,23 +278,23 @@ double_gt_lock(struct grant_table *lgt, struct
grant_table *rgt)
*/
if ( lgt < rgt )
{
-write_lock(&lgt->lock);
-write_lock(&rgt->lock);
+grant_write_lock(lgt);
+grant_write_lock(rgt);
}
else
{
if ( lgt != rgt )
-write_lock(&rgt->lock);
-write_lock(&lgt->lock);
+grant_write_lock(rgt);
+grant_write_lock(lgt);
}
}
static inline void
double_gt_unlock(struct grant_table *lgt, struct grant_table *rgt)
{
-write_unlock(&lgt->lock);
+grant_write_unlock(lgt);
if ( lgt != rgt )
-write_unlock(&rgt->lock);
+grant_write_unlock(rgt);
}
static inline int
@@ -660,7
[Xen-devel] [PATCHv6 1/3] rwlock: Add per-cpu reader-writer lock infrastructure
Per-cpu read-write locks allow for the fast path read case to have
low overhead by only setting/clearing a per-cpu variable for using
the read lock. The per-cpu read fast path also avoids locked
compare swap operations which can be particularly slow on coherent
multi-socket systems, particularly if there is heavy usage of the
read lock itself.
The per-cpu reader-writer lock uses a local variable to control
the read lock fast path. This allows a writer to disable the fast
path and ensures the readers switch to using the underlying
read-write lock implementation instead of the per-cpu variable.
Once the writer has taken the write lock and disabled the fast path,
it must poll the per-cpu variable for all CPU's which have entered
the critical section for the specific read-write lock the writer is
attempting to take. This design allows for a single per-cpu variable
to be used for read/write locks belonging to seperate data structures.
If a two or more different per-cpu read lock(s) are taken
simultaneously then the per-cpu data structure is not used and the
implementation takes the read lock of the underlying read-write lock,
this behaviour is equivalent to the slow path in terms of performance.
The per-cpu rwlock is not recursion safe for taking the per-cpu
read lock because there is no recursion count variable, this is
functionally equivalent to standard spin locks.
Slow path readers which are unblocked, set the per-cpu variable and
drop the read lock. This simplifies the implementation and allows
for fairness in the underlying read-write lock to be taken
advantage of.
There is more overhead on the per-cpu write lock path due to checking
each CPUs fast path per-cpu variable but this overhead is likely be
hidden by the required delay of waiting for readers to exit the
critical section. The loop is optimised to only iterate over
the per-cpu data of active readers of the rwlock. The cpumask_t for
tracking the active readers is stored in a single per-cpu data
location and thus the write lock is not pre-emption safe. Therefore
the per-cpu write lock can only be used with interrupts disabled.
Signed-off-by: Malcolm Crossley
--
Changes since v5:
- Fix compilation on ARM
Changes since v4:
- Use static inlines for the percpu_owner check
Changes since v3:
- Fix the ASSERTS for the percpu_owner check
Changes since v2:
- Remove stray hard tabs
- Added owner field to percpu_rwlock_t for debug builds. This allows
for ASSERTs to be added which verify the correct global percpu
structure is used for the local initialised percpu_rwlock lock
Changes since v1:
- Removed restriction on taking two or more different percpu rw
locks simultaneously
- Moved fast-path/slow-path barrier to be per lock instead of global
- Created seperate percpu_rwlock_t type and added macros to
initialise new type
- Added helper macros for using the percpu rwlock itself
- Moved active_readers cpumask off the stack and into a percpu
variable
---
xen/common/spinlock.c| 45 +
xen/include/asm-arm/percpu.h | 5 ++
xen/include/asm-x86/percpu.h | 6 +++
xen/include/xen/percpu.h | 4 ++
xen/include/xen/spinlock.h | 116 +++
5 files changed, 176 insertions(+)
diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c
index 7b0cf6c..bab1f95 100644
--- a/xen/common/spinlock.c
+++ b/xen/common/spinlock.c
@@ -10,6 +10,8 @@
#include
#include
+static DEFINE_PER_CPU(cpumask_t, percpu_rwlock_readers);
+
#ifndef NDEBUG
static atomic_t spin_debug __read_mostly = ATOMIC_INIT(0);
@@ -492,6 +494,49 @@ int _rw_is_write_locked(rwlock_t *lock)
return (lock->lock == RW_WRITE_FLAG); /* writer in critical section? */
}
+void _percpu_write_lock(percpu_rwlock_t **per_cpudata,
+percpu_rwlock_t *percpu_rwlock)
+{
+unsigned int cpu;
+cpumask_t *rwlock_readers = &this_cpu(percpu_rwlock_readers);
+
+/* Validate the correct per_cpudata variable has been provided. */
+_percpu_rwlock_owner_check(per_cpudata, percpu_rwlock);
+
+/*
+ * First take the write lock to protect against other writers or slow
+ * path readers.
+ */
+write_lock(&percpu_rwlock->rwlock);
+
+/* Now set the global variable so that readers start using read_lock. */
+percpu_rwlock->writer_activating = 1;
+smp_mb();
+
+/* Using a per cpu cpumask is only safe if there is no nesting. */
+ASSERT(!in_irq());
+cpumask_copy(rwlock_readers, &cpu_online_map);
+
+/* Check if there are any percpu readers in progress on this rwlock. */
+for ( ; ; )
+{
+for_each_cpu(cpu, rwlock_readers)
+{
+/*
+ * Remove any percpu readers not contending on this rwlock
+ * from our check mask.
+ */
+if ( per_cpu_ptr(per_cpudata, cpu) != percpu_rwlock )
+__cpumask_clear_cpu(cpu, rwlock_readers);
+}
+/* Check if we've cleared all percpu readers from che
[Xen-devel] [PATCHv6 3/3] p2m: convert p2m rwlock to percpu rwlock
The per domain p2m read lock suffers from significant contention when
performance multi-queue block or network IO due to the parallel
grant map/unmaps/copies occuring on the DomU's p2m.
On multi-socket systems, the contention results in the locked compare swap
operation failing frequently which results in a tight loop of retries of the
compare swap operation. As the coherency fabric can only support a specific
rate of compare swap operations for a particular data location then taking
the read lock itself becomes a bottleneck for p2m operations.
Percpu rwlock p2m performance with the same configuration is approximately
64 gbit/s vs the 48 gbit/s with grant table percpu rwlocks only.
Oprofile was used to determine the initial overhead of the read-write locks
and to confirm the overhead was dramatically reduced by the percpu rwlocks.
Note: altp2m users will not achieve a gain if they take an altp2m read lock
simultaneously with the main p2m lock.
Signed-off-by: Malcolm Crossley
Reviewed-by: George Dunlap
--
Changes since v5:
- None
Changes since v4:
- None
Changes since v3:
- None
Changes since v2
- Updated local percpu rwlock initialisation
---
xen/arch/x86/mm/mm-locks.h | 12 +++-
xen/arch/x86/mm/p2m.c | 1 +
xen/include/asm-x86/mm.h | 2 +-
3 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/xen/arch/x86/mm/mm-locks.h b/xen/arch/x86/mm/mm-locks.h
index 76c7217..8a40986 100644
--- a/xen/arch/x86/mm/mm-locks.h
+++ b/xen/arch/x86/mm/mm-locks.h
@@ -31,6 +31,8 @@
DECLARE_PER_CPU(int, mm_lock_level);
#define __get_lock_level() (this_cpu(mm_lock_level))
+DECLARE_PERCPU_RWLOCK_GLOBAL(p2m_percpu_rwlock);
+
static inline void mm_lock_init(mm_lock_t *l)
{
spin_lock_init(&l->lock);
@@ -99,7 +101,7 @@ static inline void _mm_enforce_order_lock_post(int level,
int *unlock_level,
static inline void mm_rwlock_init(mm_rwlock_t *l)
{
-rwlock_init(&l->lock);
+percpu_rwlock_resource_init(&l->lock, p2m_percpu_rwlock);
l->locker = -1;
l->locker_function = "nobody";
l->unlock_level = 0;
@@ -115,7 +117,7 @@ static inline void _mm_write_lock(mm_rwlock_t *l, const
char *func, int level)
if ( !mm_write_locked_by_me(l) )
{
__check_lock_level(level);
-write_lock(&l->lock);
+percpu_write_lock(p2m_percpu_rwlock, &l->lock);
l->locker = get_processor_id();
l->locker_function = func;
l->unlock_level = __get_lock_level();
@@ -131,20 +133,20 @@ static inline void mm_write_unlock(mm_rwlock_t *l)
l->locker = -1;
l->locker_function = "nobody";
__set_lock_level(l->unlock_level);
-write_unlock(&l->lock);
+percpu_write_unlock(p2m_percpu_rwlock, &l->lock);
}
static inline void _mm_read_lock(mm_rwlock_t *l, int level)
{
__check_lock_level(level);
-read_lock(&l->lock);
+percpu_read_lock(p2m_percpu_rwlock, &l->lock);
/* There's nowhere to store the per-CPU unlock level so we can't
* set the lock level. */
}
static inline void mm_read_unlock(mm_rwlock_t *l)
{
-read_unlock(&l->lock);
+percpu_read_unlock(p2m_percpu_rwlock, &l->lock);
}
/* This wrapper uses the line number to express the locking order below */
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index ed0bbd7..a45ee35 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -54,6 +54,7 @@ boolean_param("hap_2mb", opt_hap_2mb);
#undef page_to_mfn
#define page_to_mfn(_pg) _mfn(__page_to_mfn(_pg))
+DEFINE_PERCPU_RWLOCK_GLOBAL(p2m_percpu_rwlock);
/* Init the datastructures for later use by the p2m code */
static int p2m_initialise(struct domain *d, struct p2m_domain *p2m)
diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
index de3f973..7598414 100644
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -584,7 +584,7 @@ typedef struct mm_lock {
} mm_lock_t;
typedef struct mm_rwlock {
-rwlock_t lock;
+percpu_rwlock_tlock;
intunlock_level;
intrecurse_count;
intlocker; /* CPU that holds the write lock */
--
1.7.12.4
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 6/6] x86/HVM: report the set of enabled emulated devices through CPUID
On 22/01/16 12:43, Roger Pau Monné wrote: > El 22/01/16 a les 11.57, Jan Beulich ha escrit: > On 21.01.16 at 17:51, wrote: >>> Add a new HVM-specific feature flag that signals the presence of a bitmap >>> that contains the current set of enabled emulated devices. The bitmap is >>> placed in the ecx register. The bit fields used in the bitmap are the same >>> as the ones used in the xen_arch_domainconfig emulation_flags field, and >>> their meaning can be found at arch-x86/xen.h. >>> >>> This will allow Xen to enable emulated devices for HVMlite guests in the >>> future, by having a proper ABI for reporting which devices are enabled. >> The idea is certainly nice and appreciated, but ... >> >>> --- a/xen/include/public/arch-x86/cpuid.h >>> +++ b/xen/include/public/arch-x86/cpuid.h >>> @@ -78,12 +78,17 @@ >>> * HVM-specific features >>> * EAX: Features >>> * EBX: vcpu id (iff EAX has XEN_HVM_CPUID_VCPU_ID_PRESENT flag) >>> + * ECX: bitmap of enabled devices, according to the bit fields defined in >>> + * arch-x86/xen.h. >> ... this set of definitions is not currently a stable ABI (limited to >> hypervisor and tool stack), and if we wanted to make it stable >> we'd first need to think a little about the complications that may >> arise if the granularity chosen (think about the PM bit and the >> discussion around it before your changes went in) turns out to >> be a problem later on. > Yes, in fact I'm having second thoughts on the PM flag, and I think I > should have split it into ACPI_PM and ACPI_TIMER instead. > >> Also at least some of the features can be determined by other >> means (CPUID, ACPI tables), so I'm not even sure we need all >> of this, and I'd really prefer to avoid multiple distinct ways to >> learn of a certain feature, as it's too easy for the two (or more) >> mechanisms to get out of sync. > So let's look at the flags and whether there's an existing way to signal > it's presence: > > LAPIC: CPUID.01h:EDX[bit 9] > IOAPIC: tied to LAPIC (so either both enabled or none). An IOAPIC is by no means required - they are only for turning legacy interrupts into MSIs. It would be perfectly fine for a PVH domain to have an LAPIC and an SRIOV virtual function, without an IOAPIC at all. The presence of LAPICs and IOAPICs reside in the MADT ACPI table. Note also that the cpuid bit is a fastforward of the hardware enable bit in the APIC_BASE MSR. The cpuid bit will disappear from view if you hardware-disable the LAPIC. > > HPET: can only be enabled from/with ACPI, since it's base memory address > is not fixed, and we would need to find a way to pass it's address to > the OS in the absence of ACPI. In reality, there are heuristics to guess if an HPET is present. The legacy HPET traditionally always resides at pfn fed000. Linux even has heuristics to find the legacy HPET based on the IOH, for when the BIOS doesn't present the HPET properly in ACPI. This leads to an awkward bug where Linux is able to turn off legacy timer interrupts behinds Xen's back, and cause carnage for kdump environment, as Xen didn't know to re-enable legacy interrupts on the crash path. > > RTC: I don't know of any way to signal the RTC presence, AFAICT it's > always assumed to be there in the PC architecture. Could maybe return ~0 > when reading from IO port 0x71, but that's meh..., not the best way IMHO. > > PIC: same as RTC, I don't know of any way to signal it's presence since > it's assumed to be there. > > VGA: again I don't think there's an easy way to signal it's presence, > apart from returning ~0 from the multiple IO ports it uses. The fact > that the 0xA-0xB memory range is also marked as RAM in the e820 > map in HVMlite DomUs should also trigger OSes into disabling VGA due to > the lack of proper MMIO range, but sadly I think most OSes just assume > it's there. VGA can be found by following the VGA routing bit in PCI config space. This is how real hardware makes the legacy IO ranges reach the graphics card configured as the primary vga device. > > PIT: assumed to be always present in the PC architecture. PIT, RTC and PIC have their presence always assumed, but returning ~0 on reads is completely fine. A DMLite OS knows it is booting in a virtualised environment. > > PM: I'm leaning to split this into ACPI_PM and ACPI_TIMER as said > before. ACPI_TIMER presence it's contained inside of ACPI tables, and > the availability of ACPI_PM (power management) can be inferred from the > presence of ACPI itself. > > AMD guest IOMMU: AFAICT this seems to be currently disabled, since the > MMIO range it checks is [~0ULL, ~0ULL + 0x8000]. There is a function to > change the base address ~0ULL to something else, but it doesn't seem to > be reachable from any path. In any case, I guess the presence of this > device will be reported from ACPI. It is indeed currently disabled (See https://bugs.xenserver.org/browse/XSO-132 if you want to see why. It manifested as a very curious bug). It will be ava
Re: [Xen-devel] [PATCH v3 1/1] xen: sched: convert RTDS from time to event driven model
Hi guys, On Thu, 2016-01-21 at 23:06 -0500, Tianyang Chen wrote: > Budget replenishment and enforcement are separated by adding > a replenishment timer, which fires at the next most imminent > release time of all runnable vcpus. > > [...] > I started to look at this patch, and for doing that, I tried to apply it to current staging branch, and I got this: Hunk #4 FAILED at 169. Hunk #5 succeeded at 222 (offset -2 lines). Hunk #6 succeeded at 242 (offset -2 lines). Hunk #7 FAILED at 316. Hunk #8 succeeded at 327 (offset -2 lines). Hunk #9 succeeded at 343 with fuzz 2 (offset -2 lines). Hunk #14 FAILED at 655. Hunk #15 succeeded at 853 (offset -7 lines). Hunk #16 succeeded at 871 (offset -7 lines). Hunk #17 succeeded at 896 (offset -7 lines). Hunk #18 succeeded at 911 (offset -7 lines). Hunk #19 succeeded at 1058 (offset -7 lines). Hunk #20 FAILED at 1102. Hunk #21 succeeded at 1197 with fuzz 1 (offset 1 line). 4 out of 21 hunks FAILED Looks like the patch is based on a pretty old source base (it's missing one cleanup I've made to all schedulers' files', including sched_rt.c, a few months ago). When you send a patch, you usually do that on the most updated source base or, if something prevents doing that, you should clearly specify in the cover letter on top of what the patch should be applied. So, please, refresh the patch. > A new runningq has been added to keep track of all vcpus that > are on pcpus. > I've been looking at the v2, and at the discussion between you and Meng. I'm not convinced that we really need to add this runningq thing. So, if you don't mind, I''l go back to v2, and comment on it. If, during the review, it turns out that we need something like this, I'll come here again. In the meantime, do the refresh, but do not resend. You can do that after having seen my comments. Thanks and Regards, Dario -- <> (Raistlin Majere) - Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) signature.asc Description: This is a digitally signed message part ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 6/6] x86/HVM: report the set of enabled emulated devices through CPUID
>>> On 22.01.16 at 13:43, wrote: > RTC: I don't know of any way to signal the RTC presence, AFAICT it's > always assumed to be there in the PC architecture. Could maybe return ~0 > when reading from IO port 0x71, but that's meh..., not the best way IMHO. There actually is an RTC-absent flag in the FADT, which the hypervisor itself actually looks at (ACPI_FADT_NO_CMOS_RTC). > PIC: same as RTC, I don't know of any way to signal it's presence since > it's assumed to be there. I think PIC absence can also be gathered from ACPI (ACPI_FADT_HW_REDUCED). > VGA: again I don't think there's an easy way to signal it's presence, > apart from returning ~0 from the multiple IO ports it uses. The fact > that the 0xA-0xB memory range is also marked as RAM in the e820 > map in HVMlite DomUs should also trigger OSes into disabling VGA due to > the lack of proper MMIO range, but sadly I think most OSes just assume > it's there. Yes, VGA is kind of more difficult. Looking at all PCI devices' command words may provide a hint, as may looking at all PCI bridges' bridge control words. > PIT: assumed to be always present in the PC architecture. See PIC above. > PM: I'm leaning to split this into ACPI_PM and ACPI_TIMER as said > before. ACPI_TIMER presence it's contained inside of ACPI tables, and > the availability of ACPI_PM (power management) can be inferred from the > presence of ACPI itself. As indicated in the original discussion, I don't think these should have been separate flags anyway - either you have ACPI (then you have indication of both in FADT), or there is no ACPI. > AMD guest IOMMU: AFAICT this seems to be currently disabled, since the > MMIO range it checks is [~0ULL, ~0ULL + 0x8000]. There is a function to > change the base address ~0ULL to something else, but it doesn't seem to > be reachable from any path. In any case, I guess the presence of this > device will be reported from ACPI. Yes, the implementation is incomplete (abandoned?), but IOMMU presence can always be determined by the guest through inspecting its ACPI tables. > So, we have the following devices that are assumed to be there: RTC, > PIC, PIT. Everything else I think can be signalled by other means > already available. > > IMHO, I think we could say that the PIC is never going to be available > to HVMlite guests (in any case we would enable the lapic/ioapic), and > maybe enable the RTC and PIT by default? That may be a sane initial setup, but with the ACPI flags named above we may be able to expressed even their absence. > Then I think we could get away without any Xen-specific way of reporting > enabled devices. Indeed - that should be the preferred goal. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 1/3] xen-netback: fix license ident used in MODULE_LICENSE
On 22/01/16 12:34, Wei Liu wrote:
> The comment at the beginning of the file is the canonical source of
> licenses for this module. Currently it contains GPL and MIT license. Fix
> the code to reflect the reality.
"The MIT license" isn't really a thing. The closest is the X11
license[1], but this not applicable here either since the text in the
drivers does not refer to X11 trademarks etc.
You can either use "GPL" which would be correct for a Linux kernel
module since the alternate only applies when distributed separately from
Linux ("or, when distributed separately from the Linux kernel or
incorporated into other software packages, subject to the following
license:"); or you can use "GPL and additional rights".
(Or you could just leave it as-is since "Dual BSD/GPL" is close enough.)
David
[1] http://www.gnu.org/licenses/license-list.html#X11License
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [BUG] XEN domU crash when PV grub chainloads 32-bit domU grub
On 22.01.2016 14:01, Andrew Cooper wrote: > On 22/01/16 12:56, Vladimir 'φ-coder/phcoder' Serbinenko wrote: >> On 22.09.2015 10:53, Ian Campbell wrote: >>> Hi Vladimir & grub-devel, >>> >>> Do you have any thoughts on this issue with i386 pv-grub2? >>> >> Is it still an issue? If so I'll try to replicate it. From stack dump I >> see that it has jumped to NULL. GRUB has no threads so it's not a race >> condition with itself but may be one with some Xen part. An altrnative >> possibility is that grub forgets to flush cache at some point in boot >> process. > > Looks like GRUB doesn't have a traptable registered with Xen (the PV > equivalent of the IDT). > > First, Xen tried to inject a #GP fault and found that the entry EIP was > at 0 (which is sadly the default if nothing is specified). It then took > a pagefault while attempting to inject the #GP, and crashed the domain. > Do you have a link how to add one? We can put a catch-stacktrace-abort on it. > ~Andrew > >>> Thanks, Ian. >>> >>> On Mon, 2015-09-21 at 22:03 +0200, Andreas Sundstrom wrote: This is using Debian Jessie and grub 2.02~beta2-22 (with Debian patches applied) and Xen 4.4.1 I originally posted a bug report with Debian but got the suggestion to file bugs with upstream as well. Debian bug report: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=799480 Note that my original thought was that this bug probably is within GRUB. But Ian asked me to file a bug with Xen as well, you have to live with the fact that it is centered around GRUB though. Here's the information from my original bug report: Using 64-bit dom0 and 32-bit domU PV (para-virtualized) grub sometimes fail when chainloading the domU's grub. 64-bit domU seem to work 100% of the time. My understanding of the process: * dom0 launches domU with grub that is loaded from dom0's disk. * Grub reads config file from memdisk, and then looks for grub binary in domU filesystem. * If grub is found in domU it then chainloads (multiboot) that grub binary and the domU grub reads grub.cfg and continue booting. * If grub is not found in domU it reads grub.cfg and continues with boot. It fails at step 3 in my list of the boot process, but sometimes it does work so it may be something like a race condition that causes the problem? A workaround is to not install or rename /boot/xen in domU so that the first grub that is loaded from dom0's disk will not find the grub binary in the domU filesystem and hence continues to read grub.cfg and boot. The drawback of this is of course that the two versions can't differ too much as there are different setups creating grub.cfg and then reading/parsing it at boot time. I am not sure at this point whether this is a problem in XEN or a problem in grub but I compiled the legacy pvgrub that uses some minios from XEN (don't really know much more about it) and when that legacy pvgrub chainloads the domU grub it seems to work 100% of the time. Now the legace pvgrub is not a real alternative as it's not packaged for Debian though. When it fails "xl create vm -c" outputs this: Parsing config from /etc/xen/vm libxl: error: libxl_dom.c:35:libxl__domain_type: unable to get domain type for domid=16 Unable to attach console libxl: error: libxl_exec.c:118:libxl_report_child_exitstatus: console child [0] exited with error status 1 And "xl dmesg" shows errors like this: (XEN) traps.c:2514:d15 Domain attempted WRMSR c0010201 from 0x to 0x. (XEN) d16:v0: unhandled page fault (ec=0010) (XEN) Pagetable walk from : (XEN) L4[0x000] = 000200256027 049c (XEN) L3[0x000] = 000200255027 049d (XEN) L2[0x000] = 000200251023 04a1 (XEN) L1[0x000] = (XEN) domain_crash_sync called from entry.S: fault at 82d08021feb0 compat_create_bounce_frame+0xc6/0xde (XEN) Domain 16 (vcpu#0) crashed on cpu#0: (XEN) [ Xen-4.4.1 x86_64 debug=n Not tainted ] (XEN) CPU: 0 (XEN) RIP: e019:[<>] (XEN) RFLAGS: 0246 EM: 1 CONTEXT: pv guest (XEN) rax: rbx: rcx: (XEN) rdx: rsi: 00499000 rdi: 0080 (XEN) rbp: 000a rsp: 005a5ff0 r8: (XEN) r9: r10: 83023e9b9000 r11: 83023e9b9000 (XEN) r12: 033f3d335bfb r13: 82d080300800 r14: 82d0802ea940 (XEN) r15: 83005e819000 cr0: 8005003b cr4: 000506f0 (XEN) cr3: 000200b7a000 cr2: (XEN) ds: e021 es: e021 fs: e0
Re: [Xen-devel] [BUG] XEN domU crash when PV grub chainloads 32-bit domU grub
On 22/01/16 12:56, Vladimir 'φ-coder/phcoder' Serbinenko wrote: > On 22.09.2015 10:53, Ian Campbell wrote: >> Hi Vladimir & grub-devel, >> >> Do you have any thoughts on this issue with i386 pv-grub2? >> > Is it still an issue? If so I'll try to replicate it. From stack dump I > see that it has jumped to NULL. GRUB has no threads so it's not a race > condition with itself but may be one with some Xen part. An altrnative > possibility is that grub forgets to flush cache at some point in boot > process. Looks like GRUB doesn't have a traptable registered with Xen (the PV equivalent of the IDT). First, Xen tried to inject a #GP fault and found that the entry EIP was at 0 (which is sadly the default if nothing is specified). It then took a pagefault while attempting to inject the #GP, and crashed the domain. ~Andrew >> Thanks, Ian. >> >> On Mon, 2015-09-21 at 22:03 +0200, Andreas Sundstrom wrote: >>> This is using Debian Jessie and grub 2.02~beta2-22 (with Debian patches >>> applied) and Xen 4.4.1 >>> >>> I originally posted a bug report with Debian but got the suggestion to >>> file bugs with upstream as well. >>> Debian bug report: >>> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=799480 >>> >>> Note that my original thought was that this bug probably is within GRUB. >>> But Ian asked me to file a bug with Xen as well, you have to live with >>> the >>> fact that it is centered around GRUB though. >>> >>> Here's the information from my original bug report: >>> >>> Using 64-bit dom0 and 32-bit domU PV (para-virtualized) grub sometimes >>> fail when chainloading the domU's grub. 64-bit domU seem to work 100% >>> of the time. >>> >>> My understanding of the process: >>> >>> * dom0 launches domU with grub that is loaded from dom0's disk. >>> * Grub reads config file from memdisk, and then looks for grub binary in >>> domU filesystem. >>> * If grub is found in domU it then chainloads (multiboot) that grub >>> binary >>> and the domU grub reads grub.cfg and continue booting. >>> * If grub is not found in domU it reads grub.cfg and continues with >>> boot. >>> >>> It fails at step 3 in my list of the boot process, but sometimes it >>> does work so it may be something like a race condition that causes the >>> problem? >>> >>> A workaround is to not install or rename /boot/xen in domU so that the >>> first grub that is loaded from dom0's disk will not find the grub >>> binary in the domU filesystem and hence continues to read grub.cfg and >>> boot. The drawback of this is of course that the two versions can't >>> differ too much as there are different setups creating grub.cfg and >>> then reading/parsing it at boot time. >>> >>> I am not sure at this point whether this is a problem in XEN or a >>> problem in grub but I compiled the legacy pvgrub that uses some minios >>> from XEN (don't really know much more about it) and when that legacy >>> pvgrub chainloads the domU grub it seems to work 100% of the time. Now >>> the legace pvgrub is not a real alternative as it's not packaged for >>> Debian though. >>> >>> When it fails "xl create vm -c" outputs this: >>> Parsing config from /etc/xen/vm >>> libxl: error: libxl_dom.c:35:libxl__domain_type: unable to get domain >>> type for domid=16 >>> Unable to attach console >>> libxl: error: libxl_exec.c:118:libxl_report_child_exitstatus: console >>> child [0] exited with error status 1 >>> >>> And "xl dmesg" shows errors like this: >>> (XEN) traps.c:2514:d15 Domain attempted WRMSR c0010201 from >>> 0x to 0x. >>> (XEN) d16:v0: unhandled page fault (ec=0010) >>> (XEN) Pagetable walk from : >>> (XEN) L4[0x000] = 000200256027 049c >>> (XEN) L3[0x000] = 000200255027 049d >>> (XEN) L2[0x000] = 000200251023 04a1 >>> (XEN) L1[0x000] = >>> (XEN) domain_crash_sync called from entry.S: fault at 82d08021feb0 >>> compat_create_bounce_frame+0xc6/0xde >>> (XEN) Domain 16 (vcpu#0) crashed on cpu#0: >>> (XEN) [ Xen-4.4.1 x86_64 debug=n Not tainted ] >>> (XEN) CPU: 0 >>> (XEN) RIP: e019:[<>] >>> (XEN) RFLAGS: 0246 EM: 1 CONTEXT: pv guest >>> (XEN) rax: rbx: rcx: >>> (XEN) rdx: rsi: 00499000 rdi: 0080 >>> (XEN) rbp: 000a rsp: 005a5ff0 r8: >>> (XEN) r9: r10: 83023e9b9000 r11: 83023e9b9000 >>> (XEN) r12: 033f3d335bfb r13: 82d080300800 r14: 82d0802ea940 >>> (XEN) r15: 83005e819000 cr0: 8005003b cr4: 000506f0 >>> (XEN) cr3: 000200b7a000 cr2: >>> (XEN) ds: e021 es: e021 fs: e021 gs: e021 ss: e021 cs: e019 >>> (XEN) Guest stack trace from esp=005a5ff0: >>> (XEN) 0010 0001e019 00010046 0016b38b 0016b38a 0016b389 >>> 0016b388 >>> (XEN) 0016b387 0016b386 0016b385 0016b384 0016b383 0016b382 0016b381
Re: [Xen-devel] [BUG] XEN domU crash when PV grub chainloads 32-bit domU grub
On 22.09.2015 10:53, Ian Campbell wrote: > Hi Vladimir & grub-devel, > > Do you have any thoughts on this issue with i386 pv-grub2? > Is it still an issue? If so I'll try to replicate it. From stack dump I see that it has jumped to NULL. GRUB has no threads so it's not a race condition with itself but may be one with some Xen part. An altrnative possibility is that grub forgets to flush cache at some point in boot process. > Thanks, Ian. > > On Mon, 2015-09-21 at 22:03 +0200, Andreas Sundstrom wrote: >> This is using Debian Jessie and grub 2.02~beta2-22 (with Debian patches >> applied) and Xen 4.4.1 >> >> I originally posted a bug report with Debian but got the suggestion to >> file bugs with upstream as well. >> Debian bug report: >> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=799480 >> >> Note that my original thought was that this bug probably is within GRUB. >> But Ian asked me to file a bug with Xen as well, you have to live with >> the >> fact that it is centered around GRUB though. >> >> Here's the information from my original bug report: >> >> Using 64-bit dom0 and 32-bit domU PV (para-virtualized) grub sometimes >> fail when chainloading the domU's grub. 64-bit domU seem to work 100% >> of the time. >> >> My understanding of the process: >> >> * dom0 launches domU with grub that is loaded from dom0's disk. >> * Grub reads config file from memdisk, and then looks for grub binary in >> domU filesystem. >> * If grub is found in domU it then chainloads (multiboot) that grub >> binary >> and the domU grub reads grub.cfg and continue booting. >> * If grub is not found in domU it reads grub.cfg and continues with >> boot. >> >> It fails at step 3 in my list of the boot process, but sometimes it >> does work so it may be something like a race condition that causes the >> problem? >> >> A workaround is to not install or rename /boot/xen in domU so that the >> first grub that is loaded from dom0's disk will not find the grub >> binary in the domU filesystem and hence continues to read grub.cfg and >> boot. The drawback of this is of course that the two versions can't >> differ too much as there are different setups creating grub.cfg and >> then reading/parsing it at boot time. >> >> I am not sure at this point whether this is a problem in XEN or a >> problem in grub but I compiled the legacy pvgrub that uses some minios >> from XEN (don't really know much more about it) and when that legacy >> pvgrub chainloads the domU grub it seems to work 100% of the time. Now >> the legace pvgrub is not a real alternative as it's not packaged for >> Debian though. >> >> When it fails "xl create vm -c" outputs this: >> Parsing config from /etc/xen/vm >> libxl: error: libxl_dom.c:35:libxl__domain_type: unable to get domain >> type for domid=16 >> Unable to attach console >> libxl: error: libxl_exec.c:118:libxl_report_child_exitstatus: console >> child [0] exited with error status 1 >> >> And "xl dmesg" shows errors like this: >> (XEN) traps.c:2514:d15 Domain attempted WRMSR c0010201 from >> 0x to 0x. >> (XEN) d16:v0: unhandled page fault (ec=0010) >> (XEN) Pagetable walk from : >> (XEN) L4[0x000] = 000200256027 049c >> (XEN) L3[0x000] = 000200255027 049d >> (XEN) L2[0x000] = 000200251023 04a1 >> (XEN) L1[0x000] = >> (XEN) domain_crash_sync called from entry.S: fault at 82d08021feb0 >> compat_create_bounce_frame+0xc6/0xde >> (XEN) Domain 16 (vcpu#0) crashed on cpu#0: >> (XEN) [ Xen-4.4.1 x86_64 debug=n Not tainted ] >> (XEN) CPU: 0 >> (XEN) RIP: e019:[<>] >> (XEN) RFLAGS: 0246 EM: 1 CONTEXT: pv guest >> (XEN) rax: rbx: rcx: >> (XEN) rdx: rsi: 00499000 rdi: 0080 >> (XEN) rbp: 000a rsp: 005a5ff0 r8: >> (XEN) r9: r10: 83023e9b9000 r11: 83023e9b9000 >> (XEN) r12: 033f3d335bfb r13: 82d080300800 r14: 82d0802ea940 >> (XEN) r15: 83005e819000 cr0: 8005003b cr4: 000506f0 >> (XEN) cr3: 000200b7a000 cr2: >> (XEN) ds: e021 es: e021 fs: e021 gs: e021 ss: e021 cs: e019 >> (XEN) Guest stack trace from esp=005a5ff0: >> (XEN) 0010 0001e019 00010046 0016b38b 0016b38a 0016b389 >> 0016b388 >> (XEN) 0016b387 0016b386 0016b385 0016b384 0016b383 0016b382 0016b381 >> 0016b380 >> (XEN) 0016b37f 0016b37e 0016b37d 0016b37c 0016b37b 0016b37a 0016b379 >> 0016b378 >> (XEN) 0016b377 0016b376 0016b375 0016b374 0016b373 0016b372 0016b371 >> 0016b370 >> (XEN) 0016b36f 0016b36e 0016b36d 0016b36c 0016b36b 0016b36a 0016b369 >> 0016b368 >> (XEN) 0016b367 0016b366 0016b365 0016b364 0016b363 0016b362 0016b361 >> 0016b360 >> (XEN) 0016b35f 0016b35e 0016b35d 0016b35c 0016b35b 0016b35a 0016b359 >> 0016b358 >> (XEN) 0016b357 0016b356 0016b355 0016b354 0016b353
Re: [Xen-devel] [PATCH 3/3] xen-scsiback: fix license ident used in MODULE_LICENSE
On 22/01/16 13:34, Wei Liu wrote:
> The comment at the beginning of the file is the canonical source of
> licenses for this module. Currently it contains GPL and MIT license.
> Fix the code to reflect the reality.
>
> Signed-off-by: Wei Liu
Acked-by: Juergen Gross
> ---
> drivers/xen/xen-scsiback.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
> index ad4eb10..0695cf8 100644
> --- a/drivers/xen/xen-scsiback.c
> +++ b/drivers/xen/xen-scsiback.c
> @@ -1915,6 +1915,6 @@ module_init(scsiback_init);
> module_exit(scsiback_exit);
>
> MODULE_DESCRIPTION("Xen SCSI backend driver");
> -MODULE_LICENSE("Dual BSD/GPL");
> +MODULE_LICENSE("Dual MIT/GPL");
> MODULE_ALIAS("xen-backend:vscsi");
> MODULE_AUTHOR("Juergen Gross ");
>
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 2/3] xen-blkback: fix license ident used in MODULE_LICENSE
El 22/01/16 a les 13.34, Wei Liu ha escrit: > The comment at the beginning of the file is the canonical source of > licenses for this module. Currently it contains GPL and MIT license. > Fix the code to reflect the reality. > > Signed-off-by: Wei Liu Acked-by: Roger Pau Monné Thanks. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 6/6] x86/HVM: report the set of enabled emulated devices through CPUID
El 22/01/16 a les 11.57, Jan Beulich ha escrit: On 21.01.16 at 17:51, wrote: >> Add a new HVM-specific feature flag that signals the presence of a bitmap >> that contains the current set of enabled emulated devices. The bitmap is >> placed in the ecx register. The bit fields used in the bitmap are the same >> as the ones used in the xen_arch_domainconfig emulation_flags field, and >> their meaning can be found at arch-x86/xen.h. >> >> This will allow Xen to enable emulated devices for HVMlite guests in the >> future, by having a proper ABI for reporting which devices are enabled. > > The idea is certainly nice and appreciated, but ... > >> --- a/xen/include/public/arch-x86/cpuid.h >> +++ b/xen/include/public/arch-x86/cpuid.h >> @@ -78,12 +78,17 @@ >> * HVM-specific features >> * EAX: Features >> * EBX: vcpu id (iff EAX has XEN_HVM_CPUID_VCPU_ID_PRESENT flag) >> + * ECX: bitmap of enabled devices, according to the bit fields defined in >> + * arch-x86/xen.h. > > ... this set of definitions is not currently a stable ABI (limited to > hypervisor and tool stack), and if we wanted to make it stable > we'd first need to think a little about the complications that may > arise if the granularity chosen (think about the PM bit and the > discussion around it before your changes went in) turns out to > be a problem later on. Yes, in fact I'm having second thoughts on the PM flag, and I think I should have split it into ACPI_PM and ACPI_TIMER instead. > Also at least some of the features can be determined by other > means (CPUID, ACPI tables), so I'm not even sure we need all > of this, and I'd really prefer to avoid multiple distinct ways to > learn of a certain feature, as it's too easy for the two (or more) > mechanisms to get out of sync. So let's look at the flags and whether there's an existing way to signal it's presence: LAPIC: CPUID.01h:EDX[bit 9] IOAPIC: tied to LAPIC (so either both enabled or none). HPET: can only be enabled from/with ACPI, since it's base memory address is not fixed, and we would need to find a way to pass it's address to the OS in the absence of ACPI. RTC: I don't know of any way to signal the RTC presence, AFAICT it's always assumed to be there in the PC architecture. Could maybe return ~0 when reading from IO port 0x71, but that's meh..., not the best way IMHO. PIC: same as RTC, I don't know of any way to signal it's presence since it's assumed to be there. VGA: again I don't think there's an easy way to signal it's presence, apart from returning ~0 from the multiple IO ports it uses. The fact that the 0xA-0xB memory range is also marked as RAM in the e820 map in HVMlite DomUs should also trigger OSes into disabling VGA due to the lack of proper MMIO range, but sadly I think most OSes just assume it's there. PIT: assumed to be always present in the PC architecture. PM: I'm leaning to split this into ACPI_PM and ACPI_TIMER as said before. ACPI_TIMER presence it's contained inside of ACPI tables, and the availability of ACPI_PM (power management) can be inferred from the presence of ACPI itself. AMD guest IOMMU: AFAICT this seems to be currently disabled, since the MMIO range it checks is [~0ULL, ~0ULL + 0x8000]. There is a function to change the base address ~0ULL to something else, but it doesn't seem to be reachable from any path. In any case, I guess the presence of this device will be reported from ACPI. So, we have the following devices that are assumed to be there: RTC, PIC, PIT. Everything else I think can be signalled by other means already available. IMHO, I think we could say that the PIC is never going to be available to HVMlite guests (in any case we would enable the lapic/ioapic), and maybe enable the RTC and PIT by default? Then I think we could get away without any Xen-specific way of reporting enabled devices. >> All unused bits have undefined values. > > Nor is this an option, but maybe this is just a wording issue: > Perhaps you mean to say that they're reserved for future use? > Since truly unused bits have are guaranteed to have the value > zero, just that the set of bits varies. Yes, that's exactly what I meant to say, thanks. Roger. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [PATCH 3/3] xen-scsiback: fix license ident used in MODULE_LICENSE
The comment at the beginning of the file is the canonical source of
licenses for this module. Currently it contains GPL and MIT license.
Fix the code to reflect the reality.
Signed-off-by: Wei Liu
---
drivers/xen/xen-scsiback.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
index ad4eb10..0695cf8 100644
--- a/drivers/xen/xen-scsiback.c
+++ b/drivers/xen/xen-scsiback.c
@@ -1915,6 +1915,6 @@ module_init(scsiback_init);
module_exit(scsiback_exit);
MODULE_DESCRIPTION("Xen SCSI backend driver");
-MODULE_LICENSE("Dual BSD/GPL");
+MODULE_LICENSE("Dual MIT/GPL");
MODULE_ALIAS("xen-backend:vscsi");
MODULE_AUTHOR("Juergen Gross ");
--
2.1.4
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[Xen-devel] [PATCH 1/3] xen-netback: fix license ident used in MODULE_LICENSE
The comment at the beginning of the file is the canonical source of
licenses for this module. Currently it contains GPL and MIT license. Fix
the code to reflect the reality.
Signed-off-by: Wei Liu
---
drivers/net/xen-netback/netback.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/xen-netback/netback.c
b/drivers/net/xen-netback/netback.c
index 61b97c3..2427242 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -2192,5 +2192,5 @@ static void __exit netback_fini(void)
}
module_exit(netback_fini);
-MODULE_LICENSE("Dual BSD/GPL");
+MODULE_LICENSE("Dual MIT/GPL");
MODULE_ALIAS("xen-backend:vif");
--
2.1.4
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[Xen-devel] [PATCH 2/3] xen-blkback: fix license ident used in MODULE_LICENSE
The comment at the beginning of the file is the canonical source of
licenses for this module. Currently it contains GPL and MIT license.
Fix the code to reflect the reality.
Signed-off-by: Wei Liu
---
drivers/block/xen-blkback/blkback.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/block/xen-blkback/blkback.c
b/drivers/block/xen-blkback/blkback.c
index 4809c15..01cc572 100644
--- a/drivers/block/xen-blkback/blkback.c
+++ b/drivers/block/xen-blkback/blkback.c
@@ -1496,5 +1496,5 @@ static int __init xen_blkif_init(void)
module_init(xen_blkif_init);
-MODULE_LICENSE("Dual BSD/GPL");
+MODULE_LICENSE("Dual MIT/GPL");
MODULE_ALIAS("xen-backend:vbd");
--
2.1.4
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[Xen-devel] [PATCH 0/3] xen: fix wrong idents in MODULE_LICENSE in some drivers
Wei Liu (3): xen-netback: fix license ident used in MODULE_LICENSE xen-blkback: fix license ident used in MODULE_LICENSE xen-scsiback: fix license ident used in MODULE_LICENSE drivers/block/xen-blkback/blkback.c | 2 +- drivers/net/xen-netback/netback.c | 2 +- drivers/xen/xen-scsiback.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) -- 2.1.4 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC V2] xen: interface: introduce pvclk interface
>>> On 22.01.16 at 13:12, wrote: > On Fri, Jan 22, 2016 at 03:25:40AM -0700, Jan Beulich wrote: >>In particular, with the user space exposure of clock control >>discussed in another sub-thread, the next best option would >>seem to be to handle this via emulation in a device model. Yes, >>ARM guests currently have no qemu attached to them, but I >>guess sooner or later this will need to change anyway. > > I have not look into qemu for xen. > If using qemu, then we still need to expose the clk interface to userspace? I think so, yes. In fact - see above - the discussed userspace exposure made me consider this option. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 05/10] acpi: Refactor acpi_os_map_memory to be architecturally independent
>>> On 22.01.16 at 12:55, wrote:
>
> On 2016/1/22 18:15, Jan Beulich wrote:
> On 22.01.16 at 10:37, wrote:
>>> >
>>> >On 2016/1/22 16:47, Jan Beulich wrote:
>>> >On 22.01.16 at 09:38, wrote:
>> >>> >
>> >>> >On 2016/1/18 21:33, Jan Beulich wrote:
>> >>> >On 16.01.16 at 06:01,
>> >>> >wrote:
>> >>> >--- a/xen/drivers/acpi/osl.c
>> >>> >+++ b/xen/drivers/acpi/osl.c
>> >>> >@@ -86,17 +86,7 @@ acpi_physical_address __init
>> >>> >acpi_os_get_root_pointer(void)
>> >>> > void __iomem *
>> >>> > acpi_os_map_memory(acpi_physical_address phys,
>> >>> > acpi_size size)
>> >>> > {
>> >>> >- if (system_state >= SYS_STATE_active) {
>> >>> >- mfn_t mfn = _mfn(PFN_DOWN(phys));
>> >>> >- unsigned int offs = phys & (PAGE_SIZE -
>> >>> >1);
>> >>> >-
>> >>> >- /* The low first Mb is always mapped. */
>> >>> >- if ( !((phys + size - 1) >> 20) )
>> >>> >- return __va(phys);
>> >>> >- return __vmap(&mfn, PFN_UP(offs +
>> >>> >size), 1, 1,
>> >>> >-PAGE_HYPERVISOR_NOCACHE)
>> >>> >+ offs;
>> >>> >- }
>> >>> >- return __acpi_map_table(phys, size);
>> >>> >+ return arch_acpi_os_map_memory(phys, size);
>> >>> > }
>>I'm quite sure I've said before that this goes too far: The
>>__vmap()
>>part and likely also the early-boot __acpi_map_table() one
>>already
>>are architecture independent and hence should stay. The
>>factoring
>>hence should only concern the first Mb handling and maybe the
>>the mapping attributes passed to __vmap().
>> >>> >
>> >>> >Yes, the first MB handling and __vmap() should refactor. So if it
>> >>> >only
>> >>> >moves them to an architecture function, how about below patch?
>> >>> >
>> >>> >diff --git a/xen/arch/x86/acpi/lib.c b/xen/arch/x86/acpi/lib.c
>> >>> >index cc15ea3..5885a3a 100644
>> >>> >--- a/xen/arch/x86/acpi/lib.c
>> >>> >+++ b/xen/arch/x86/acpi/lib.c
>> >>> >@@ -33,6 +33,19 @@ u8 __read_mostly acpi_disable_value;
>> >>> > u32 __read_mostly x86_acpiid_to_apicid[MAX_MADT_ENTRIES] =
>> >>> > {[0 ... MAX_MADT_ENTRIES - 1] = BAD_APICID };
>> >>> >
>> >>> >+void __iomem *
>> >>> >+arch_acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
>> >>> >+{
>> >>> >+ mfn_t mfn = _mfn(PFN_DOWN(phys));
>> >>> >+ unsigned int offs = phys & (PAGE_SIZE - 1);
>> >>> >+
>> >>> >+ /* The low first Mb is always mapped. */
>> >>> >+ if ( !((phys + size - 1) >> 20) )
>> >>> >+ return __va(phys);
>> >>> >+ return __vmap(&mfn, PFN_UP(offs + size), 1, 1,
>> >>> >+ PAGE_HYPERVISOR_NOCACHE) + offs;
>> >>> >+}
>>Well, I had clearly said the vmap() part is generic; if there's
>>anything architecture dependent here, then the mapping
>>attributes (and hence only_those_ should be factored out,
>>not the entire function invocation).
>>> >I know what you said. But how can we change the attribute for ARM in
>>> >acpi_os_map_memory() without moving these codes out?
>> By having each arch #define their value, and use that constant here?
> You really want that?
Yes.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC V2] xen: interface: introduce pvclk interface
Hi Jan, On Fri, Jan 22, 2016 at 03:25:40AM -0700, Jan Beulich wrote: On 22.01.16 at 10:27, wrote: >> Hi Jan, >> >> On Fri, Jan 22, 2016 at 12:36:31AM -0700, Jan Beulich wrote: >> On 22.01.16 at 02:56, wrote: On Thu, Jan 21, 2016 at 05:52:12AM -0700, Jan Beulich wrote: >At the very least it would need to be avoided by denying the request. >Upon shared use, either all parties agree, or only one may use the >clock. And passing through a (platform) device would therefore imply >validating that the needed clock(s) are available to the target domain. >Doing this in a consistent way with all control in one component's >hands seems doable only if hypervisor and/or tool stack are the >controlling (and arbitrating) entity. In the end this is one of the >reasons why to me a simple PV I/O interface doesn't seem suitable >here. How about let userspace libxl pvclk code to denying the request? >>> >>>Userspace would be fine, but >> >> Then you are ok with the pvclk way to handle clock for platform device >> passthrough? > >No, not really. While I accept that doing clock management in the >hypervisor is undesirable, we're still not at the point where such >a frontend/backend pair would look like the only possible route >out of a dilemma, and I continue to think that this proposed model >should indeed only be the last resort. Thanks for following the thread and giving comments. Alougth this frustrate me, we still need to find a better option for this. > >In particular, with the user space exposure of clock control >discussed in another sub-thread, the next best option would >seem to be to handle this via emulation in a device model. Yes, >ARM guests currently have no qemu attached to them, but I >guess sooner or later this will need to change anyway. I have not look into qemu for xen. If using qemu, then we still need to expose the clk interface to userspace? > >>>- How would this fit in your frontend/backend model, where >>> userspace shouldn't be involved at all? >> >> rethought about this. clk is binded to device. we can not passthrough >> one device to two guest, so this means we can not let two different >> guest access one clk input. Since this is mainly for embedded products, >> just as Ian said "experts option", the developer should be aware of >> the clk sharing between two device. >> >> If we truly need to let userspace deny the request. If one clk >> already assigned to Dom1, then the toolstack need to fail >> the creation of Dom2, if Dom2 want to use the same clock. > >I.e. you're now proposing actual assignment of clocks to guests? >That's at least one step in the (from my pov) right direction... Based on the pvclk, I am coding the userspace tool part. Alought we have not find a good solution for this, I first need it work on my platform. Later I'll also try the fixed clock way. Thanks, Peng. > >Jan > ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 05/10] acpi: Refactor acpi_os_map_memory to be architecturally independent
On 2016/1/22 18:15, Jan Beulich wrote:
On 22.01.16 at 10:37, wrote:
>
>On 2016/1/22 16:47, Jan Beulich wrote:
>On 22.01.16 at 09:38, wrote:
>>> >
>>> >On 2016/1/18 21:33, Jan Beulich wrote:
>>> >On 16.01.16 at 06:01, wrote:
>> >>> >--- a/xen/drivers/acpi/osl.c
>> >>> >+++ b/xen/drivers/acpi/osl.c
>> >>> >@@ -86,17 +86,7 @@ acpi_physical_address __init
>>> >acpi_os_get_root_pointer(void)
>> >>> > void __iomem *
>> >>> > acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
>> >>> > {
>> >>> >- if (system_state >= SYS_STATE_active) {
>> >>> >- mfn_t mfn = _mfn(PFN_DOWN(phys));
>> >>> >- unsigned int offs = phys & (PAGE_SIZE - 1);
>> >>> >-
>> >>> >- /* The low first Mb is always mapped. */
>> >>> >- if ( !((phys + size - 1) >> 20) )
>> >>> >- return __va(phys);
>> >>> >- return __vmap(&mfn, PFN_UP(offs + size), 1, 1,
>> >>> >- PAGE_HYPERVISOR_NOCACHE) + offs;
>> >>> >- }
>> >>> >- return __acpi_map_table(phys, size);
>> >>> >+ return arch_acpi_os_map_memory(phys, size);
>> >>> > }
>>I'm quite sure I've said before that this goes too far: The __vmap()
>>part and likely also the early-boot __acpi_map_table() one already
>>are architecture independent and hence should stay. The factoring
>>hence should only concern the first Mb handling and maybe the
>>the mapping attributes passed to __vmap().
>>> >
>>> >Yes, the first MB handling and __vmap() should refactor. So if it only
>>> >moves them to an architecture function, how about below patch?
>>> >
>>> >diff --git a/xen/arch/x86/acpi/lib.c b/xen/arch/x86/acpi/lib.c
>>> >index cc15ea3..5885a3a 100644
>>> >--- a/xen/arch/x86/acpi/lib.c
>>> >+++ b/xen/arch/x86/acpi/lib.c
>>> >@@ -33,6 +33,19 @@ u8 __read_mostly acpi_disable_value;
>>> > u32 __read_mostly x86_acpiid_to_apicid[MAX_MADT_ENTRIES] =
>>> > {[0 ... MAX_MADT_ENTRIES - 1] = BAD_APICID };
>>> >
>>> >+void __iomem *
>>> >+arch_acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
>>> >+{
>>> >+ mfn_t mfn = _mfn(PFN_DOWN(phys));
>>> >+ unsigned int offs = phys & (PAGE_SIZE - 1);
>>> >+
>>> >+ /* The low first Mb is always mapped. */
>>> >+ if ( !((phys + size - 1) >> 20) )
>>> >+ return __va(phys);
>>> >+ return __vmap(&mfn, PFN_UP(offs + size), 1, 1,
>>> >+ PAGE_HYPERVISOR_NOCACHE) + offs;
>>> >+}
>>Well, I had clearly said the vmap() part is generic; if there's
>>anything architecture dependent here, then the mapping
>>attributes (and hence only_those_ should be factored out,
>>not the entire function invocation).
>I know what you said. But how can we change the attribute for ARM in
>acpi_os_map_memory() without moving these codes out?
By having each arch #define their value, and use that constant here?
You really want that? Even though the way here I use is not too many
dunplicated codes (and I think it looks clearer).
--
Shannon
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[Xen-devel] [PATCH 4/4] xl: rework vtpm config parsing code
Follow the same pattern as vif config parse to introduce
parse_vtpm_config_token, parse_vtpm_config_one,
parse_vtpm_config_multistring and parse_vtpm_config. Then replace
open-coded parsing code with appropriate functions.
Signed-off-by: Wei Liu
---
tools/libxl/xl_cmdimpl.c | 119 ++-
1 file changed, 76 insertions(+), 43 deletions(-)
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index f736f95..1444b0b 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1110,6 +1110,76 @@ static void parse_nic_config(XLU_Config **config, const
char *buf,
parse_nic_config_multistring(config, 1, &buf, nic);
}
+static int parse_vtpm_config_token(XLU_Config **config, char *token,
+ libxl_device_vtpm *vtpm)
+{
+char *oparg;
+
+if (MATCH_OPTION("uuid", token, oparg)) {
+if(libxl_uuid_from_string(&vtpm->uuid, oparg)) {
+fprintf(stderr, "Invalid uuid specified (%s)\n", oparg);
+return 1;
+}
+} else if (MATCH_OPTION("backend", token, oparg)) {
+replace_string(&vtpm->backend_domname, oparg);
+} else {
+fprintf(stderr, "unrecognized argument `%s'\n", token);
+return 1;
+}
+
+return 0;
+}
+
+static int parse_vtpm_config_one(XLU_Config **config, const char *config_str,
+ libxl_device_vtpm *vtpm)
+{
+char *buf = xstrdup(config_str);
+char *p;
+int ret;
+
+p = strtok(buf, ",");
+if (!p) {
+ret = 0;
+goto out;
+}
+
+do {
+while (*p == ' ')
+p++;
+ret = parse_vtpm_config_token(config, p, vtpm);
+} while ((p = strtok(NULL, ",")) != NULL && ret == 0);
+
+out:
+free(buf);
+
+return ret;
+}
+
+static void parse_vtpm_config_multistring(XLU_Config **config,
+ int nspecs, const char *const *specs,
+ libxl_device_vtpm *vtpm)
+{
+int i;
+
+libxl_device_vtpm_init(vtpm);
+
+if (!*config) {
+*config = xlu_cfg_init(stderr, "command line");
+if (!*config) { perror("xlu_cfg_init"); exit(-1); }
+}
+
+for (i = 0; i < nspecs; i++) {
+if (parse_vtpm_config_one(config, specs[i], vtpm))
+exit(EXIT_FAILURE);
+}
+}
+
+static void parse_vtpm_config(XLU_Config **config, const char *buf,
+ libxl_device_vtpm *vtpm)
+{
+parse_vtpm_config_multistring(config, 1, &buf, vtpm);
+}
+
static unsigned long parse_ulong(const char *str)
{
char *endptr;
@@ -1858,42 +1928,17 @@ static void parse_config_data(const char *config_source,
while ((buf = xlu_cfg_get_listitem (vtpms, d_config->num_vtpms))
!= NULL) {
libxl_device_vtpm *vtpm;
-char * buf2 = strdup(buf);
-char *p, *p2;
-bool got_backend = false;
vtpm = ARRAY_EXTEND_INIT(d_config->vtpms,
d_config->num_vtpms,
libxl_device_vtpm_init);
-p = strtok(buf2, ",");
-if(p) {
- do {
- while(*p == ' ')
- ++p;
- if ((p2 = strchr(p, '=')) == NULL)
- break;
- *p2 = '\0';
- if (!strcmp(p, "backend")) {
- vtpm->backend_domname = strdup(p2 + 1);
- got_backend = true;
- } else if(!strcmp(p, "uuid")) {
- if( libxl_uuid_from_string(&vtpm->uuid, p2 + 1) ) {
-fprintf(stderr,
- "Failed to parse vtpm UUID: %s\n", p2 + 1);
-exit(1);
-}
- } else {
- fprintf(stderr, "Unknown string `%s' in vtpm spec\n", p);
- exit(1);
- }
- } while ((p = strtok(NULL, ",")) != NULL);
-}
-if(!got_backend) {
+parse_vtpm_config(&config, buf, vtpm);
+
+if(!vtpm->backend_domname) {
fprintf(stderr, "vtpm spec missing required backend field!\n");
exit(1);
}
-free(buf2);
}
}
@@ -6812,8 +6857,8 @@ int main_vtpmattach(int argc, char **argv)
{
int opt;
libxl_device_vtpm vtpm;
-char *oparg;
uint32_t domid;
+XLU_Config *config = 0;
SWITCH_FOREACH_OPT(opt, "", NULL, "vtpm-attach", 1) {
/* No options */
@@ -6825,20 +6870,8 @@ int main_vtpmattach(int argc, char **argv)
}
++optind;
-libxl_device_vtpm_init(&vtpm);
-for (argv += optind, argc -= optind; argc > 0; ++argv, --argc) {
-if (MATCH_OPTION("uuid", *argv, oparg)) {
-if(libxl_uuid_from_string(&(vtpm.uuid), oparg)) {
-fprintf(stderr, "Invalid uuid sp
[Xen-devel] [PATCH 2/4] xl: wrap long lines where possible
No functional changes introduced.
Signed-off-by: Wei Liu
---
tools/libxl/xl_cmdimpl.c | 107 ---
1 file changed, 73 insertions(+), 34 deletions(-)
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index f380799..ff561c3 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -237,12 +237,14 @@ static int acquire_lock(void)
fl.l_len = 0;
fd_lock = open(lockfile, O_WRONLY|O_CREAT, S_IWUSR);
if (fd_lock < 0) {
-fprintf(stderr, "cannot open the lockfile %s errno=%d\n", lockfile,
errno);
+fprintf(stderr, "cannot open the lockfile %s errno=%d\n",
+lockfile, errno);
return ERROR_FAIL;
}
if (fcntl(fd_lock, F_SETFD, FD_CLOEXEC) < 0) {
close(fd_lock);
-fprintf(stderr, "cannot set cloexec to lockfile %s errno=%d\n",
lockfile, errno);
+fprintf(stderr, "cannot set cloexec to lockfile %s errno=%d\n",
+lockfile, errno);
return ERROR_FAIL;
}
get_lock:
@@ -538,12 +540,16 @@ out:
return ret;
}
-static int parse_action_on_shutdown(const char *buf, libxl_action_on_shutdown
*a)
+static int parse_action_on_shutdown(const char *buf,
+libxl_action_on_shutdown *a)
{
-int i;
+int i, size;
const char *n;
-for (i = 0; i < sizeof(action_on_shutdown_names) /
sizeof(action_on_shutdown_names[0]); i++) {
+size = sizeof(action_on_shutdown_names) /
+sizeof(action_on_shutdown_names[0]);
+
+for (i = 0; i < size; i++) {
n = action_on_shutdown_names[i];
if (!n) continue;
@@ -994,7 +1000,8 @@ static int match_option_size(const char *prefix, size_t
len,
/* Parses network data and adds info into nic
* Returns 1 if the input token does not match one of the keys
* or parsed values are not correct. Successful parse returns 0 */
-static int parse_nic_config(libxl_device_nic *nic, XLU_Config **config, char
*token)
+static int parse_nic_config(libxl_device_nic *nic, XLU_Config **config,
+char *token)
{
char *endptr, *oparg;
int i;
@@ -1040,7 +1047,8 @@ static int parse_nic_config(libxl_device_nic *nic,
XLU_Config **config, char *to
} else if (MATCH_OPTION("rate", token, oparg)) {
parse_vif_rate(config, oparg, nic);
} else if (MATCH_OPTION("accel", token, oparg)) {
-fprintf(stderr, "the accel parameter for vifs is currently not
supported\n");
+fprintf(stderr,
+"the accel parameter for vifs is currently not supported\n");
} else {
fprintf(stderr, "unrecognized argument `%s'\n", token);
return 1;
@@ -1580,13 +1588,15 @@ static void parse_config_data(const char *config_source,
if (l < LIBXL_TIMER_MODE_DELAY_FOR_MISSED_TICKS ||
l > LIBXL_TIMER_MODE_ONE_MISSED_TICK_PENDING) {
-fprintf(stderr, "ERROR: invalid value %ld for
\"timer_mode\"\n", l);
+fprintf(stderr,
+"ERROR: invalid value %ld for \"timer_mode\"\n", l);
exit (1);
}
b_info->u.hvm.timer_mode = l;
} else if (!xlu_cfg_get_string(config, "timer_mode", &buf, 0)) {
if (libxl_timer_mode_from_string(buf, &b_info->u.hvm.timer_mode)) {
-fprintf(stderr, "ERROR: invalid value \"%s\" for
\"timer_mode\"\n",
+fprintf(stderr,
+"ERROR: invalid value \"%s\" for \"timer_mode\"\n",
buf);
exit (1);
}
@@ -1605,7 +1615,8 @@ static void parse_config_data(const char *config_source,
if (!strcmp(buf, "generate")) {
e = libxl_ms_vm_genid_generate(ctx,
&b_info->u.hvm.ms_vm_genid);
if (e) {
-fprintf(stderr, "ERROR: failed to generate a VM Generation
ID\n");
+fprintf(stderr,
+"ERROR: failed to generate a VM Generation ID\n");
exit(1);
}
} else if (!strcmp(buf, "none")) {
@@ -1621,7 +1632,8 @@ static void parse_config_data(const char *config_source,
break;
case LIBXL_DOMAIN_TYPE_PV:
{
-xlu_cfg_replace_string (config, "bootloader",
&b_info->u.pv.bootloader, 0);
+xlu_cfg_replace_string (config, "bootloader",
+&b_info->u.pv.bootloader, 0);
switch (xlu_cfg_get_list_as_string_list(config, "bootloader_args",
&b_info->u.pv.bootloader_args, 1))
{
@@ -1772,7 +1784,8 @@ static void parse_config_data(const char *config_source,
if (!xlu_cfg_get_list (config, "disk", &vbds, 0, 0)) {
d_config->num_disks = 0;
d_config->disks = NULL;
-while ((buf = xlu_cfg_get_listitem (vbds, d_config->num_disks)) !=
NULL) {
+while ((buf = xlu_cfg_get_listi
[Xen-devel] [PATCH 3/4] xl: rework vif config parsing code
The original parse_nic_config was in fact only parsing tokens.
main_networkattach erroneously used it to parse raw config string, which
led to xl network-attach not respecting network configuration syntax.
Rework vif config parser:
1. Extract the snippet in parse_config_data to parse_nic_config_one.
2. Rename original parse_nic_config to parse_nic_config_token.
3. Provide _multistring variant.
4. Implement parse_nic_config with _multistring variant.
5. Use _multistring variant in main_networkattach.
Finally the call chain becomes: parse_nic_config ->
parse_nic_config_multistring -> parse_nic_config_one ->
parse_nic_config_token.
Some other less important changes:
1. Change prototypes of parse_nic_config functions to match
block counterpart.
2. parse_nic_config will now exit if parsing failed.
3. main_networkattach doesn't call set_default_nic_values anymore.
Signed-off-by: Wei Liu
---
tools/libxl/xl_cmdimpl.c | 82 +++-
1 file changed, 60 insertions(+), 22 deletions(-)
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index ff561c3..f736f95 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1000,8 +1000,9 @@ static int match_option_size(const char *prefix, size_t
len,
/* Parses network data and adds info into nic
* Returns 1 if the input token does not match one of the keys
* or parsed values are not correct. Successful parse returns 0 */
-static int parse_nic_config(libxl_device_nic *nic, XLU_Config **config,
-char *token)
+static int parse_nic_config_token(XLU_Config **config,
+ char *token,
+ libxl_device_nic *nic)
{
char *endptr, *oparg;
int i;
@@ -1056,6 +1057,59 @@ static int parse_nic_config(libxl_device_nic *nic,
XLU_Config **config,
return 0;
}
+static int parse_nic_config_one(XLU_Config **config,
+const char *config_str,
+libxl_device_nic *nic)
+{
+char *buf = xstrdup(config_str);
+char *p;
+int ret;
+
+set_default_nic_values(nic);
+
+p = strtok(buf, ",");
+if (!p) {
+ret = 0;
+goto out;
+}
+
+do {
+while (*p == ' ')
+p++;
+ret = parse_nic_config_token(config, p, nic);
+} while ((p = strtok(NULL, ",")) != NULL && ret == 0);
+
+out:
+free(buf);
+
+return ret;
+}
+
+static void parse_nic_config_multistring(XLU_Config **config,
+ int nspecs, const char *const *specs,
+ libxl_device_nic *nic)
+{
+int i;
+
+libxl_device_nic_init(nic);
+
+if (!*config) {
+*config = xlu_cfg_init(stderr, "command line");
+if (!*config) { perror("xlu_cfg_init"); exit(-1); }
+}
+
+for (i = 0; i < nspecs; i++) {
+if (parse_nic_config_one(config, specs[i], nic))
+exit(EXIT_FAILURE);
+}
+}
+
+static void parse_nic_config(XLU_Config **config, const char *buf,
+ libxl_device_nic *nic)
+{
+parse_nic_config_multistring(config, 1, &buf, nic);
+}
+
static unsigned long parse_ulong(const char *str)
{
char *endptr;
@@ -1928,24 +1982,10 @@ static void parse_config_data(const char *config_source,
while ((buf = xlu_cfg_get_listitem (nics, d_config->num_nics))
!= NULL) {
libxl_device_nic *nic;
-char *buf2 = strdup(buf);
-char *p;
-
nic = ARRAY_EXTEND_INIT(d_config->nics,
d_config->num_nics,
libxl_device_nic_init);
-set_default_nic_values(nic);
-
-p = strtok(buf2, ",");
-if (!p)
-goto skip_nic;
-do {
-while (*p == ' ')
-p++;
-parse_nic_config(nic, &config, p);
-} while ((p = strtok(NULL, ",")) != NULL);
-skip_nic:
-free(buf2);
+parse_nic_config(&config, buf, nic);
}
}
@@ -6532,12 +6572,10 @@ int main_networkattach(int argc, char **argv)
}
libxl_device_nic_init(&nic);
-set_default_nic_values(&nic);
-for (argv += optind+1, argc -= optind+1; argc > 0; ++argv, --argc) {
-if (parse_nic_config(&nic, &config, *argv))
-return 1;
-}
+optind++;
+parse_nic_config_multistring(&config, argc-optind,
+ (const char* const*) argv + optind, &nic);
if (dryrun_only) {
char *json = libxl_device_nic_to_json(ctx, &nic);
--
2.1.4
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[Xen-devel] [PATCH 1/4] xl: remove unused macros
They were added back in 2011 to be used in an adhoc parser which has now been removed. Signed-off-by: Wei Liu --- tools/libxl/xl_cmdimpl.c | 8 1 file changed, 8 deletions(-) diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c index f9933cb..f380799 100644 --- a/tools/libxl/xl_cmdimpl.c +++ b/tools/libxl/xl_cmdimpl.c @@ -556,14 +556,6 @@ static int parse_action_on_shutdown(const char *buf, libxl_action_on_shutdown *a return 0; } -#define DSTATE_INITIAL 0 -#define DSTATE_TAP 1 -#define DSTATE_PHYSPATH 2 -#define DSTATE_VIRTPATH 3 -#define DSTATE_VIRTTYPE 4 -#define DSTATE_RW5 -#define DSTATE_TERMINAL 6 - static void parse_disk_config_multistring(XLU_Config **config, int nspecs, const char *const *specs, libxl_device_disk *disk) -- 2.1.4 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [PATCH 0/4] xl: consolidate adhoc parsers
This patch series consolidates adhoc parsers in xl. There are currently 4 types of devices: 1. block 2. netowrk 3. vtpm 4. pci that support hotplug as well as being specified in config file. Block and pci devices are fine because they use libxlu to parse configuration strings. Network and vtpms config parsers are not in a very good state, which need to be consolidated. Note that there is code repetition in the newly introduced parser code. We either need to have very long macro definition or code repetition. I chose the latter. Let me know your opinion. Wei. Wei Liu (4): xl: remove unused macros xl: wrap long lines where possible xl: rework vif config parsing code xl: rework vtpm config parsing code tools/libxl/xl_cmdimpl.c | 312 +++ 1 file changed, 207 insertions(+), 105 deletions(-) -- 2.1.4 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v11 2/3] Differentiate IO/mem resources tracked by ioreq server
>>> On 22.01.16 at 04:20, wrote:
> @@ -2601,6 +2605,16 @@ struct hvm_ioreq_server
> *hvm_select_ioreq_server(struct domain *d,
> type = (p->type == IOREQ_TYPE_PIO) ?
> HVMOP_IO_RANGE_PORT : HVMOP_IO_RANGE_MEMORY;
> addr = p->addr;
> +if ( type == HVMOP_IO_RANGE_MEMORY )
> +{
> + ram_page = get_page_from_gfn(d, p->addr >> PAGE_SHIFT,
> + &p2mt, P2M_UNSHARE);
It seems to me like I had asked before: Why P2M_UNSHARE instead
of just P2M_QUERY? (This could surely be fixed up while committing,
the more that I've already done some cleanup here, but I'd like to
understand this before it goes in.)
> + if ( p2mt == p2m_mmio_write_dm )
> + type = HVMOP_IO_RANGE_WP_MEM;
> +
> + if ( ram_page )
> + put_page(ram_page);
> +}
> }
>
> list_for_each_entry ( s,
> @@ -2642,6 +2656,11 @@ struct hvm_ioreq_server
> *hvm_select_ioreq_server(struct domain *d,
> }
>
> break;
> +case HVMOP_IO_RANGE_WP_MEM:
> +if ( rangeset_contains_singleton(r, PFN_DOWN(addr)) )
> +return s;
Considering you've got p2m_mmio_write_dm above - can this
validly return false here?
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [BUG] EDAC infomation partially missing
Am 22.01.16 um 11:40 schrieb Jan Beulich: On 22.01.16 at 10:09, wrote: >> When booting with Xen 4.4.1: >> >> AMD64 EDAC driver v3.4.0 >> EDAC amd64: DRAM ECC enabled. >> EDAC amd64: NB MCE bank disabled, set MSR 0x017b[4] on node 0 to enable. > I wonder how valid his message is. We actually write this MSR with > all ones during boot. > > However, considering involved functions like > nb_mce_bank_enabled_on_node() or node_to_amd_nb() taking > node IDs as inputs, and considering that PV guests (including > Dom0) don't have a topology matching that of the host, I doubt > very much that this driver is even remotely prepared to run > under Xen. It working on Xen 4.1.x would then be by pure > accident. The dmesg is identical with or without Xen4.1, so I'd guess it does work if flags are detected correctly. Regards Andreas ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC] VirtFS support on Xen
On Fri, Jan 22, 2016 at 11:12:01AM +, Paul Durrant wrote: > > -Original Message- > > From: xen-devel-boun...@lists.xen.org [mailto:xen-devel- > > boun...@lists.xen.org] On Behalf Of Wei Liu > > Sent: 22 January 2016 10:51 > > To: Bob Liu > > Cc: Xen-devel; Wei Liu; David Vrabel > > Subject: Re: [Xen-devel] [RFC] VirtFS support on Xen > > > > On Fri, Jan 22, 2016 at 06:45:30PM +0800, Bob Liu wrote: > > > Hi Wei, > > > > > > On 01/21/2016 06:59 PM, Wei Liu wrote: > > > > On Thu, Jan 21, 2016 at 10:50:08AM +, David Vrabel wrote: > > > >> On 21/01/16 10:28, Wei Liu wrote: > > > >>> [RFC] VirtFS support on Xen > > > >>> > > > >>> # Introduction > > > >>> > > > >>> QEMU/KVM supports file system passthrough via an interface called > > > >>> VirtFS [0]. VirtFS is in turn implemented with 9pfs protocol [1] and > > > >>> VirtIO transport. > > > >>> > > > >>> Xen used to have its own implementation of file system passthrough > > > >>> called XenFS, but that has been inactive for a few years. The latest > > > >>> update was in 2009 [2]. > > > >>> > > > >>> This project aims to add VirtFS support on Xen. This is more > > > >>> sustainable than inventing our own wheel.# > > > >> > > > >> What's the use case for this? Who wants this feature? > > > >> > > > > > > > > Anyone who wants file system passthrough. More specifically, VM- > > based > > > > container solutions can share files from host file system. > > > > > > > > > > I'm a bit confused, can't we just use the VirtFS of Qemu? > > > E.g > > > ./configure --with-extra-qemuu-configure-args="--enable-virtfs" > > > > > > > Yes, in theory you can -- with VirtIO transport. But I'm not sure if > > Virtio has been fixed to work with Xen. That also means you need QEMU > > emulation, which we don't really need (or want) when running in PV or > > PVH mode. > > > > Is there not scope for: > > a) Fixing VirtIO so it does work with Xen? Presumably you mean fixing VirtIO to work with Xen HVM guest. Yes. Stefano sent patches for fixing that. That involves changing some assumptions in Virtio design. Not sure how it goes at the moment. As far as I can tell those patches are not yet upstreamed or have been superseded by some other patches promised to be written by someone else. > b) Resurrecting the idea of emulator disaggregation so that we could > spawn a QEMU that only serves as a VirtIO backend so that this can be > used for PV/PHV guests? > I think disaggregation of emulators is a different and orthogonal issue. (There is on-going work to spawn two QEMU for HVM guests, one for PV backends, one for emulation.) PV can't use existing Virtio transports because there is no emulation for PV guest, unless we write Xen transport for Virtio. After investigation I found it very hard to finish in a finite time frame and unsustainable in the long run. PVH can possibly use VirtIO, but how it is going to be implemented is unclear. There are a few things to get hashed out before we can think of VirtIO with PVH guests. There is nothing that prevents we make VirtIO work with PVH in the future though, once everything is in place. Wei. > Seems like a better solution that implementing our own 9p transport and > backend. > > Paul > > > > > Wei. > > > > > > > Thanks, > > > Bob > > > > > > > http://xendevsummit2015.sched.org/event/3WDg/xen-containers- > > better-way-to-run-docker-containers-sainath-grandhi- > > intel?iframe=no&w=&sidebar=yes&bg=no > > > > http://xendevsummit2015.sched.org/event/3X1L/hyper-make-vm-run- > > like-containers-xu-wang-hyper?iframe=no&w=&sidebar=yes&bg=no > > > > > > > > Wei. > > > > > > > > ___ > > Xen-devel mailing list > > Xen-devel@lists.xen.org > > http://lists.xen.org/xen-devel ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PULL 10/11] Add Error **errp for xen_pt_config_init()
On 21/01/2016 18:01, Stefano Stabellini wrote:
> -XEN_PT_LOG(&s->dev, "Failed to initialize %d/%ld reg
> 0x%x in grp_type=0x%x (%d/%ld), rc=%d\n",
> - j,
> ARRAY_SIZE(xen_pt_emu_reg_grps[i].emu_regs),
> - regs->offset,
> xen_pt_emu_reg_grps[i].grp_type,
> - i, ARRAY_SIZE(xen_pt_emu_reg_grps), rc);
> +xen_pt_config_reg_init(s, reg_grp_entry, regs, &err);
> +if (err) {
> +error_append_hint(&err, "Failed to initialize %d/%zu"
> +" reg 0x%x in grp_type = 0x%x (%d/%zu)",
> +j,
> ARRAY_SIZE(xen_pt_emu_reg_grps[i].emu_regs),
Coverity noticed a preexisting problem here. emu_regs is a pointer,
thus ARRAY_SIZE doesn't return what you expect.
Paolo
> +regs->offset,
> xen_pt_emu_reg_grps[i].grp_type,
> +i, ARRAY_SIZE(xen_pt_emu_reg_grps));
> +error_propagate(errp, err);
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH RFC 26/31] xen/x86: Rework AMD masking MSR setup
>>> On 22.01.16 at 12:01, wrote:
> On 22/01/16 09:27, Jan Beulich wrote:
> On 16.12.15 at 22:24, wrote:
>>> + expected_levelling_cap, levelling_caps,
>>> + (expected_levelling_cap ^ levelling_caps) & levelling_caps);
>>> + printk(XENLOG_WARNING "Fam %#x, model %#x level %#x\n",
>>> + c->x86, c->x86_model, c->cpuid_level);
>>> + printk(XENLOG_WARNING
>>> + "If not running virtualised, please report a bug\n");
>> Well - you checked for running virtualized, so making it here when
>> running virtualized is already a bug (albeit not in the code here)?
>
> Why would it be a bug? The virtualising environment might provide these
> MSRs, in which case we should use them.
Because you won't make it here when cpu_has_hypervisor.
>>> +void amd_ctxt_switch_levelling(const struct domain *nextd)
>>> +{
>>> + struct cpumasks *these_masks = &this_cpu(cpumasks);
>>> + const struct cpumasks *masks = &cpumask_defaults;
>>> +
>>> +#define LAZY(cap, msr, field)
>>> \
>>> + ({ \
>>> + if ( ((levelling_caps & cap) == cap) && \
>>> +(these_masks->field != masks->field) ) \
>>> + { \
>>> + wrmsr_amd(msr, masks->field); \
>>> + these_masks->field = masks->field; \
>>> + } \
>>> + })
>>> +
>>> + LAZY(LCAP_1cd, MSR_K8_FEATURE_MASK, _1cd);
>>> + LAZY(LCAP_e1cd, MSR_K8_EXT_FEATURE_MASK, e1cd);
>>> + LAZY(LCAP_7ab0, MSR_AMD_L7S0_FEATURE_MASK, _7ab0);
>>> + LAZY(LCAP_6c, MSR_AMD_THRM_FEATURE_MASK, _6c);
>> So here we already have the first example where fully consistent
>> naming would allow elimination of a macro parameter.
>
> Token concatenation deliberately obscures code from tool like grep and
> cscope. There is already too much of the Xen source code obscured like
> this; I'd prefer not to add to it.
I'm not sure grep-abilty is more important than code readability.
My personal opinion surely is that the latter is more important.
>> And then, how is this supposed to work? You only restore defaults,
>> but never write non-default values. Namely, nextd is an unused
>> function parameter ...
>>
>> Also I guess my comment about adding unused code needs
>> repeating here.
>
> Future patches build on this, both using the parameter, and not using
> the defaults.
>
> I am also certain that if I did two patches, the first putting in a void
> function, and the second changing it to a pointer, your review would ask
> me to turn it into this.
Well, I realize things will all make sense by the end of the series, but
honestly in some of the cases I'm not sure the split between patches
was well done. But just to be clear, none of the related comments
mean I would outright reject any of the patches just for that reason.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC] VirtFS support on Xen
> -Original Message- > From: xen-devel-boun...@lists.xen.org [mailto:xen-devel- > boun...@lists.xen.org] On Behalf Of Wei Liu > Sent: 22 January 2016 10:51 > To: Bob Liu > Cc: Xen-devel; Wei Liu; David Vrabel > Subject: Re: [Xen-devel] [RFC] VirtFS support on Xen > > On Fri, Jan 22, 2016 at 06:45:30PM +0800, Bob Liu wrote: > > Hi Wei, > > > > On 01/21/2016 06:59 PM, Wei Liu wrote: > > > On Thu, Jan 21, 2016 at 10:50:08AM +, David Vrabel wrote: > > >> On 21/01/16 10:28, Wei Liu wrote: > > >>> [RFC] VirtFS support on Xen > > >>> > > >>> # Introduction > > >>> > > >>> QEMU/KVM supports file system passthrough via an interface called > > >>> VirtFS [0]. VirtFS is in turn implemented with 9pfs protocol [1] and > > >>> VirtIO transport. > > >>> > > >>> Xen used to have its own implementation of file system passthrough > > >>> called XenFS, but that has been inactive for a few years. The latest > > >>> update was in 2009 [2]. > > >>> > > >>> This project aims to add VirtFS support on Xen. This is more > > >>> sustainable than inventing our own wheel.# > > >> > > >> What's the use case for this? Who wants this feature? > > >> > > > > > > Anyone who wants file system passthrough. More specifically, VM- > based > > > container solutions can share files from host file system. > > > > > > > I'm a bit confused, can't we just use the VirtFS of Qemu? > > E.g > > ./configure --with-extra-qemuu-configure-args="--enable-virtfs" > > > > Yes, in theory you can -- with VirtIO transport. But I'm not sure if > Virtio has been fixed to work with Xen. That also means you need QEMU > emulation, which we don't really need (or want) when running in PV or > PVH mode. > Is there not scope for: a) Fixing VirtIO so it does work with Xen? b) Resurrecting the idea of emulator disaggregation so that we could spawn a QEMU that only serves as a VirtIO backend so that this can be used for PV/PHV guests? Seems like a better solution that implementing our own 9p transport and backend. Paul > > Wei. > > > > Thanks, > > Bob > > > > > http://xendevsummit2015.sched.org/event/3WDg/xen-containers- > better-way-to-run-docker-containers-sainath-grandhi- > intel?iframe=no&w=&sidebar=yes&bg=no > > > http://xendevsummit2015.sched.org/event/3X1L/hyper-make-vm-run- > like-containers-xu-wang-hyper?iframe=no&w=&sidebar=yes&bg=no > > > > > > Wei. > > > > > ___ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [xen-4.6-testing test] 78701: tolerable FAIL - PUSHED
flight 78701 xen-4.6-testing real [real] http://logs.test-lab.xenproject.org/osstest/logs/78701/ Failures :-/ but no regressions. Regressions which are regarded as allowable (not blocking): test-amd64-amd64-rumpuserxen-amd64 15 rumpuserxen-demo-xenstorels/xenstorels.repeat fail REGR. vs. 78618 test-amd64-i386-xl-qemut-win7-amd64 16 guest-stop fail REGR. vs. 78618 test-armhf-armhf-xl-rtds 11 guest-start fail REGR. vs. 78618 test-amd64-amd64-xl-qemut-win7-amd64 16 guest-stop fail like 78618 test-amd64-amd64-xl-qemuu-win7-amd64 16 guest-stop fail like 78618 test-amd64-i386-xl-qemuu-win7-amd64 16 guest-stop fail like 78618 Tests which did not succeed, but are not blocking: 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-armhf-armhf-libvirt 14 guest-saverestorefail never pass test-armhf-armhf-libvirt 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-qemuu-nested-amd 16 debian-hvm-install/l1/l2 fail never pass test-amd64-amd64-libvirt-xsm 12 migrate-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 11 migrate-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 13 guest-saverestorefail never pass test-amd64-i386-libvirt-xsm 12 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 12 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 12 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 13 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 11 migrate-support-checkfail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check fail never pass test-armhf-armhf-libvirt-xsm 12 migrate-support-checkfail never pass test-armhf-armhf-libvirt-xsm 14 guest-saverestorefail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check fail never pass test-armhf-armhf-xl-credit2 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 12 migrate-support-checkfail never pass test-armhf-armhf-xl 12 migrate-support-checkfail never pass test-armhf-armhf-xl 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-cubietruck 12 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-xsm 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-xsm 12 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 13 guest-saverestorefail never pass test-armhf-armhf-libvirt-raw 11 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 11 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 12 saverestore-support-checkfail never pass version targeted for testing: xen 19fc53a92312876761659e82a1ae5d69b603a4ef baseline version: xen 1fd615aa0108490ffc558d27627f509183cbfdaf Last test of basis78618 2016-01-20 13:17:45 Z1 days Testing same since78701 2016-01-21 17:20:37 Z0 days1 attempts People who touched revisions under test: Andrew Cooper Boris Ostrovsky Edgar E. Iglesias Ian Campbell Julien Grall Kevin Tian Paul Durrant Tim Deegan jobs: build-amd64-xsm pass build-armhf-xsm pass build-i386-xsm pass build-amd64 pass build-armhf pass build-i386 pass build-amd64-libvirt pass build-armhf-libvirt pass build-i386-libvirt pass build-amd64-prev pass build-i386-prev pass build-amd64-pvopspass build-armhf-pvopspass build-i386-pvops pass build-amd64-rumpuserxen pass build-i386-rumpuserxen
Re: [Xen-devel] [PATCH v4 4/6] x86/PV: allow PV guests to have an emulated PIT
On Fri, 2016-01-22 at 03:48 -0700, Jan Beulich wrote:
> > > > On 21.01.16 at 17:51, wrote:
> > This fixes the fallout from the HVMlite series, that removed the
> > emulated
> > PIT from PV(H) guests. Also, this patch forces the hardware domain to
> > always have an emulated PIT, regardless of whether the toolstack
> > specified
> > one or not.
> >
> > Signed-off-by: Roger Pau Monné
>
> Reviewed-by: Jan Beulich
> albeit ...
>
> > --- a/xen/arch/x86/domain.c
> > +++ b/xen/arch/x86/domain.c
> > @@ -542,8 +542,11 @@ int arch_domain_create(struct domain *d, unsigned
> > int
> > domcr_flags,
> > d->domain_id, config->emulation_flags);
> > return -EINVAL;
> > }
> > +if ( is_hardware_domain(d) )
> > +config->emulation_flags |= XEN_X86_EMU_PIT;
> > if ( config->emulation_flags != 0 &&
> > - (!is_hvm_domain(d) || config->emulation_flags !=
> > XEN_X86_EMU_ALL) )
> > + (is_hvm_domain(d) ? (config->emulation_flags !=
> > XEN_X86_EMU_ALL) :
> > + (config->emulation_flags !=
> > XEN_X86_EMU_PIT)) )
> > {
>
> ... you having chosen the != route instead of the suggested &
> one, I guess I'll take the liberty to further simplify this while
> committing (as the ?: is now only needed on the right side of the
> != and I'm generally of the opinion that redundancy like this is
> hampering readability).
Readability would be even greater IMHO with the use of a
"required_emulation_flags" variable suitably initialised and then checked,
compared with the use of bitops etc.
>
> 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 v4 3/6] libxl: initialise the build info before calling prepare_config
On Thu, 2016-01-21 at 17:51 +0100, Roger Pau Monne wrote: > libxl__arch_domain_prepare_config has access to the > libxl_domain_build_info > struct, so make sure it's properly initialised. Note that prepare_config > is > called from within libxl__domain_make. > > This is not a bug at the moment, because prepare_config doesn't access > libxl_domain_build_info yet, but this is likely going to change. > > Signed-off-by: Roger Pau Monné Acked-by: Ian Campbell ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] netfront/netback multiqueue exhausting grants
>>> On 22.01.16 at 11:40, wrote: > On 01/22/2016 03:53 PM, Jan Beulich wrote: > On 22.01.16 at 04:36, wrote: >>> By the way, do you think it's possible to make grant table support bigger >>> page e.g 64K? >>> One grant-ref per 64KB instead of 4KB, this should able to reduce the grant >>> entry consumption significantly. >> >> How would that work with an underlying page size of 4k, and pages >> potentially being non-contiguous in machine address space? Besides >> that the grant table hypercall interface isn't prepared to support >> 64k page size, due to its use of uint16_t for the length of copy ops. > > Right, and I mean whether we should consider address all the place as your > mentioned. Just from an abstract perspective: How would you envision to avoid machine address discontiguity? Or would you want to limit such an improvement to only HVM/PVH/HVMlite guests? Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH RFC 26/31] xen/x86: Rework AMD masking MSR setup
On 22/01/16 09:27, Jan Beulich wrote:
On 16.12.15 at 22:24, wrote:
>> This patch is best reviewed as its end result rather than as a diff, as it
>> rewrites almost all of the setup.
> This, I think, doesn't belong in the commit message itself.
Why not? It applies equally to anyone reading the commit in tree.
>
>> @@ -126,126 +133,172 @@ static const struct cpuidmask *__init noinline
>> get_cpuidmask(const char *opt)
>> }
>>
>> /*
>> - * Mask the features and extended features returned by CPUID. Parameters
>> are
>> - * set from the boot line via two methods:
>> - *
>> - * 1) Specific processor revision string
>> - * 2) User-defined masks
>> - *
>> - * The processor revision string parameter has precedene.
>> + * Sets caps in expected_levelling_cap, probes for the specified mask MSR,
>> and
>> + * set caps in levelling_caps if it is found. Processors prior to Fam 10h
>> + * required a 32-bit password for masking MSRs. Reads the default value
>> into
>> + * msr_val.
>> */
>> -static void __devinit set_cpuidmask(const struct cpuinfo_x86 *c)
>> +static void __init __probe_mask_msr(unsigned int msr, uint64_t caps,
>> +uint64_t *msr_val)
>> {
>> -static unsigned int feat_ecx, feat_edx;
>> -static unsigned int extfeat_ecx, extfeat_edx;
>> -static unsigned int l7s0_eax, l7s0_ebx;
>> -static unsigned int thermal_ecx;
>> -static bool_t skip_feat, skip_extfeat;
>> -static bool_t skip_l7s0_eax_ebx, skip_thermal_ecx;
>> -static enum { not_parsed, no_mask, set_mask } status;
>> -unsigned int eax, ebx, ecx, edx;
>> -
>> -if (status == no_mask)
>> -return;
>> +unsigned int hi, lo;
>> +
>> +expected_levelling_cap |= caps;
> Mixing indentation styles.
Yes - sadly some crept in, despite my best efforts. I have already
fixed all I can find in the series.
>
>> +
>> +if ((rdmsr_amd_safe(msr, &lo, &hi) == 0) &&
>> +(wrmsr_amd_safe(msr, lo, hi) == 0))
>> +levelling_caps |= caps;
> This logic assumes that faults are possible only because the MSR is
> not there at all, not because of the actual value used. Is this a safe
> assumption, the more that you don't use the "safe" variants
> anymore at runtime?
Yes - it is a safe assumption.
The MSRs on AMD are at fixed indices and all specified to cover the full
32bit of a cpuid feature leaf. All we really care about is whether the
MSR is present (and to read its defaults, if it is).
If the MSR isn't present, levelling_caps won't be updated, and Xen will
never touch the MSR again.
>
>> +/*
>> + * Probe for the existance of the expected masking MSRs. They might easily
>> + * not be available if Xen is running virtualised.
>> + */
>> +static void __init noinline probe_masking_msrs(void)
>> +{
>> +const struct cpuinfo_x86 *c = &boot_cpu_data;
>>
>> -ASSERT((status == not_parsed) && (c == &boot_cpu_data));
>> -status = no_mask;
>> +/*
>> + * First, work out which masking MSRs we should have, based on
>> + * revision and cpuid.
>> + */
>>
>> /* Fam11 doesn't support masking at all. */
>> if (c->x86 == 0x11)
>> return;
>>
>> -if (~(opt_cpuid_mask_ecx & opt_cpuid_mask_edx &
>> - opt_cpuid_mask_ext_ecx & opt_cpuid_mask_ext_edx &
>> - opt_cpuid_mask_l7s0_eax & opt_cpuid_mask_l7s0_ebx &
>> - opt_cpuid_mask_thermal_ecx)) {
>> -feat_ecx = opt_cpuid_mask_ecx;
>> -feat_edx = opt_cpuid_mask_edx;
>> -extfeat_ecx = opt_cpuid_mask_ext_ecx;
>> -extfeat_edx = opt_cpuid_mask_ext_edx;
>> -l7s0_eax = opt_cpuid_mask_l7s0_eax;
>> -l7s0_ebx = opt_cpuid_mask_l7s0_ebx;
>> -thermal_ecx = opt_cpuid_mask_thermal_ecx;
>> -} else if (*opt_famrev == '\0') {
>> +__probe_mask_msr(MSR_K8_FEATURE_MASK, LCAP_1cd,
>> + &cpumask_defaults._1cd);
>> +__probe_mask_msr(MSR_K8_EXT_FEATURE_MASK, LCAP_e1cd,
>> + &cpumask_defaults.e1cd);
>> +
>> +if (c->cpuid_level >= 7)
>> +__probe_mask_msr(MSR_AMD_L7S0_FEATURE_MASK, LCAP_7ab0,
>> + &cpumask_defaults._7ab0);
>> +
>> +if (c->x86 == 0x15 && c->cpuid_level >= 6 && cpuid_ecx(6))
>> +__probe_mask_msr(MSR_AMD_THRM_FEATURE_MASK, LCAP_6c,
>> + &cpumask_defaults._6c);
>> +
>> +/*
>> + * Don't bother warning about a mismatch if virtualised. These MSRs
>> + * are not architectural and almost never virtualised.
>> + */
>> +if ((expected_levelling_cap == levelling_caps) ||
>> +cpu_has_hypervisor)
>> return;
>> -} else {
>> -const struct cpuidmask *m = get_cpuidmask(opt_famrev);
>> +
>> +printk(XENLOG_WARNING "Mismatch between expected (%#x"
>> + ") and real (%#x) levelling caps: missing %#x\n",
> Breaking a format string inside parentheses is certainly a little odd.
> A
Re: [Xen-devel] [PATCH v2 16/16] ARM64: XEN: Initialize Xen specific UEFI runtime services
On Fri, 22 Jan 2016, Shannon Zhao wrote:
> On 2016/1/19 1:03, Stefano Stabellini wrote:
> > On Fri, 15 Jan 2016, Shannon Zhao wrote:
> >> > From: Shannon Zhao
> >> >
> >> > When running on Xen hypervisor, runtime services are supported through
> >> > hypercall. So call Xen specific function to initialize runtime services.
> >> >
> >> > Signed-off-by: Shannon Zhao
> > Thanks Shannon, much much better! Just a couple of questions.
> >
> >
> >> > arch/arm/xen/enlighten.c | 5 +
> >> > arch/arm64/xen/Makefile | 1 +
> >> > arch/arm64/xen/efi.c | 36
> >> > drivers/xen/Kconfig | 2 +-
> >> > include/xen/xen-ops.h| 1 +
> >> > 5 files changed, 44 insertions(+), 1 deletion(-)
> >> > create mode 100644 arch/arm64/xen/efi.c
> >> >
> >> > diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> >> > index 485e117..84f27ec 100644
> >> > --- a/arch/arm/xen/enlighten.c
> >> > +++ b/arch/arm/xen/enlighten.c
> >> > @@ -414,6 +414,11 @@ static int __init xen_guest_init(void)
> >> > if (xen_initial_domain())
> >> >
> >> > pvclock_gtod_register_notifier(&xen_pvclock_gtod_notifier);
> >> >
> >> > +if (IS_ENABLED(CONFIG_XEN_EFI)) {
> >> > +if (efi_enabled(EFI_PARAVIRT))
> >> > +xen_efi_runtime_setup();
> >> > +}
> >> > +
> >> > return 0;
> >> > }
> >> > early_initcall(xen_guest_init);
> >> > diff --git a/arch/arm64/xen/Makefile b/arch/arm64/xen/Makefile
> >> > index 74a8d87..62e6fe2 100644
> >> > --- a/arch/arm64/xen/Makefile
> >> > +++ b/arch/arm64/xen/Makefile
> >> > @@ -1,2 +1,3 @@
> >> > xen-arm-y += $(addprefix ../../arm/xen/, enlighten.o
> >> > grant-table.o p2m.o mm.o)
> >> > obj-y := xen-arm.o hypercall.o
> >> > +obj-$(CONFIG_XEN_EFI) += efi.o
> >> > diff --git a/arch/arm64/xen/efi.c b/arch/arm64/xen/efi.c
> >> > new file mode 100644
> >> > index 000..33046b0
> >> > --- /dev/null
> >> > +++ b/arch/arm64/xen/efi.c
> >> > @@ -0,0 +1,36 @@
> >> > +/*
> >> > + * Copyright (c) 2015, Linaro Limited, Shannon Zhao
> >> > + *
> >> > + * This program is free software; you can redistribute it and/or modify
> >> > + * it under the terms of the GNU General Public License as published by
> >> > + * the Free Software Foundation; either version 2 of the License, or
> >> > + * (at your option) any later version.
> >> > + *
> >> > + * This program is distributed in the hope that it will be useful,
> >> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> >> > + * GNU General Public License for more details.
> >> > + *
> >> > + * You should have received a copy of the GNU General Public License
> >> > along
> >> > + * with this program. If not, see
Re: [Xen-devel] [PATCH v4 2/6] libxl: introduce LIBXL_VGA_INTERFACE_TYPE_UNKNOWN
On Thu, 2016-01-21 at 17:51 +0100, Roger Pau Monne wrote: > And use it as the default value for the VGA kind. This allows libxl to > set > it to the default value later on when the domain type is known. For HVM > guests the default value is LIBXL_VGA_INTERFACE_TYPE_CIRRUS while for > HVMlite the default value is LIBXL_VGA_INTERFACE_TYPE_NONE. > > Signed-off-by: Roger Pau Monné Acked-by: Ian Campbell There a behavioural change from an xl users PoV, WRT the dmlite/qemu=none, which warrants a documentation change I think. That might need to be part of a bigger overhaul of the xl manpages to incorporate discussion of pvh/dmlite though? and in any case can come in a followup. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 6/6] x86/HVM: report the set of enabled emulated devices through CPUID
>>> On 21.01.16 at 17:51, wrote: > Add a new HVM-specific feature flag that signals the presence of a bitmap > that contains the current set of enabled emulated devices. The bitmap is > placed in the ecx register. The bit fields used in the bitmap are the same > as the ones used in the xen_arch_domainconfig emulation_flags field, and > their meaning can be found at arch-x86/xen.h. > > This will allow Xen to enable emulated devices for HVMlite guests in the > future, by having a proper ABI for reporting which devices are enabled. The idea is certainly nice and appreciated, but ... > --- a/xen/include/public/arch-x86/cpuid.h > +++ b/xen/include/public/arch-x86/cpuid.h > @@ -78,12 +78,17 @@ > * HVM-specific features > * EAX: Features > * EBX: vcpu id (iff EAX has XEN_HVM_CPUID_VCPU_ID_PRESENT flag) > + * ECX: bitmap of enabled devices, according to the bit fields defined in > + * arch-x86/xen.h. ... this set of definitions is not currently a stable ABI (limited to hypervisor and tool stack), and if we wanted to make it stable we'd first need to think a little about the complications that may arise if the granularity chosen (think about the PM bit and the discussion around it before your changes went in) turns out to be a problem later on. Also at least some of the features can be determined by other means (CPUID, ACPI tables), so I'm not even sure we need all of this, and I'd really prefer to avoid multiple distinct ways to learn of a certain feature, as it's too easy for the two (or more) mechanisms to get out of sync. > All unused bits have undefined values. Nor is this an option, but maybe this is just a wording issue: Perhaps you mean to say that they're reserved for future use? Since truly unused bits have are guaranteed to have the value zero, just that the set of bits varies. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC] VirtFS support on Xen
On Fri, Jan 22, 2016 at 06:45:30PM +0800, Bob Liu wrote: > Hi Wei, > > On 01/21/2016 06:59 PM, Wei Liu wrote: > > On Thu, Jan 21, 2016 at 10:50:08AM +, David Vrabel wrote: > >> On 21/01/16 10:28, Wei Liu wrote: > >>> [RFC] VirtFS support on Xen > >>> > >>> # Introduction > >>> > >>> QEMU/KVM supports file system passthrough via an interface called > >>> VirtFS [0]. VirtFS is in turn implemented with 9pfs protocol [1] and > >>> VirtIO transport. > >>> > >>> Xen used to have its own implementation of file system passthrough > >>> called XenFS, but that has been inactive for a few years. The latest > >>> update was in 2009 [2]. > >>> > >>> This project aims to add VirtFS support on Xen. This is more > >>> sustainable than inventing our own wheel.# > >> > >> What's the use case for this? Who wants this feature? > >> > > > > Anyone who wants file system passthrough. More specifically, VM-based > > container solutions can share files from host file system. > > > > I'm a bit confused, can't we just use the VirtFS of Qemu? > E.g > ./configure --with-extra-qemuu-configure-args="--enable-virtfs" > Yes, in theory you can -- with VirtIO transport. But I'm not sure if Virtio has been fixed to work with Xen. That also means you need QEMU emulation, which we don't really need (or want) when running in PV or PVH mode. Wei. > Thanks, > Bob > > > http://xendevsummit2015.sched.org/event/3WDg/xen-containers-better-way-to-run-docker-containers-sainath-grandhi-intel?iframe=no&w=&sidebar=yes&bg=no > > http://xendevsummit2015.sched.org/event/3X1L/hyper-make-vm-run-like-containers-xu-wang-hyper?iframe=no&w=&sidebar=yes&bg=no > > > > Wei. > > ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 4/6] x86/PV: allow PV guests to have an emulated PIT
>>> On 21.01.16 at 17:51, wrote:
> This fixes the fallout from the HVMlite series, that removed the emulated
> PIT from PV(H) guests. Also, this patch forces the hardware domain to
> always have an emulated PIT, regardless of whether the toolstack specified
> one or not.
>
> Signed-off-by: Roger Pau Monné
Reviewed-by: Jan Beulich
albeit ...
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -542,8 +542,11 @@ int arch_domain_create(struct domain *d, unsigned int
> domcr_flags,
> d->domain_id, config->emulation_flags);
> return -EINVAL;
> }
> +if ( is_hardware_domain(d) )
> +config->emulation_flags |= XEN_X86_EMU_PIT;
> if ( config->emulation_flags != 0 &&
> - (!is_hvm_domain(d) || config->emulation_flags !=
> XEN_X86_EMU_ALL) )
> + (is_hvm_domain(d) ? (config->emulation_flags !=
> XEN_X86_EMU_ALL) :
> + (config->emulation_flags !=
> XEN_X86_EMU_PIT)) )
> {
... you having chosen the != route instead of the suggested &
one, I guess I'll take the liberty to further simplify this while
committing (as the ?: is now only needed on the right side of the
!= and I'm generally of the opinion that redundancy like this is
hampering readability).
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC] VirtFS support on Xen
Hi Wei, On 01/21/2016 06:59 PM, Wei Liu wrote: > On Thu, Jan 21, 2016 at 10:50:08AM +, David Vrabel wrote: >> On 21/01/16 10:28, Wei Liu wrote: >>> [RFC] VirtFS support on Xen >>> >>> # Introduction >>> >>> QEMU/KVM supports file system passthrough via an interface called >>> VirtFS [0]. VirtFS is in turn implemented with 9pfs protocol [1] and >>> VirtIO transport. >>> >>> Xen used to have its own implementation of file system passthrough >>> called XenFS, but that has been inactive for a few years. The latest >>> update was in 2009 [2]. >>> >>> This project aims to add VirtFS support on Xen. This is more >>> sustainable than inventing our own wheel.# >> >> What's the use case for this? Who wants this feature? >> > > Anyone who wants file system passthrough. More specifically, VM-based > container solutions can share files from host file system. > I'm a bit confused, can't we just use the VirtFS of Qemu? E.g ./configure --with-extra-qemuu-configure-args="--enable-virtfs" Thanks, Bob > http://xendevsummit2015.sched.org/event/3WDg/xen-containers-better-way-to-run-docker-containers-sainath-grandhi-intel?iframe=no&w=&sidebar=yes&bg=no > http://xendevsummit2015.sched.org/event/3X1L/hyper-make-vm-run-like-containers-xu-wang-hyper?iframe=no&w=&sidebar=yes&bg=no > > Wei. > ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [Minios-devel] [PATCH v8 0/] Begin to disentangle libxenctrl and provide some stable libraries
On Tue, 2016-01-19 at 15:44 +, Ian Campbell wrote:
> On Fri, 2016-01-15 at 13:22 +, Ian Campbell wrote:
> >
> > Therefore needing attention from Ian and/or Wei are:
> >
> > tools/libs/foreignmemory: Mention restrictions on fork in docs.
> > N tools/libs/evtchn: Use uint32_t for domid arguments
> > D tools/libs/gnttab: Extensive updates to API documentation.
> > tools/libs/call: linux: touch newly allocated pages after madvise
> > l
> > tools/libs/{call,evtchn}: Document requirements around forking.
> > Rtools/libs/*: Use O_CLOEXEC on Linux and FreeBSD
>
> Thanks to Wei for acking all of these. This set of series is now ready to
> go in, but we've not had a push for a little while and this is
> potentially
> disruptive so I'm going to hold off for now until we get a push.
We've now had a push in 78610 so I'm going to go ahead with applying this
mass of patches today.
> There are one or two patches which will require rebasing over Jeurgens
> introduction of tools/helpers, I'll resend just those ones though (or at
> least only the Xen part of this series).
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] netfront/netback multiqueue exhausting grants
On 01/22/2016 03:53 PM, Jan Beulich wrote: On 22.01.16 at 04:36, wrote: >> By the way, do you think it's possible to make grant table support bigger >> page e.g 64K? >> One grant-ref per 64KB instead of 4KB, this should able to reduce the grant >> entry consumption significantly. > > How would that work with an underlying page size of 4k, and pages > potentially being non-contiguous in machine address space? Besides > that the grant table hypercall interface isn't prepared to support > 64k page size, due to its use of uint16_t for the length of copy ops. > Right, and I mean whether we should consider address all the place as your mentioned. With multi-queue xen-block and xen-network, we got more reports that the grants were exhausted. -- Regards, -Bob ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [BUG] EDAC infomation partially missing
>>> On 22.01.16 at 10:09, wrote: > When booting with Xen 4.4.1: > > AMD64 EDAC driver v3.4.0 > EDAC amd64: DRAM ECC enabled. > EDAC amd64: NB MCE bank disabled, set MSR 0x017b[4] on node 0 to enable. I wonder how valid his message is. We actually write this MSR with all ones during boot. However, considering involved functions like nb_mce_bank_enabled_on_node() or node_to_amd_nb() taking node IDs as inputs, and considering that PV guests (including Dom0) don't have a topology matching that of the host, I doubt very much that this driver is even remotely prepared to run under Xen. It working on Xen 4.1.x would then be by pure accident. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC V2] xen: interface: introduce pvclk interface
>>> On 22.01.16 at 10:27, wrote: > Hi Jan, > > On Fri, Jan 22, 2016 at 12:36:31AM -0700, Jan Beulich wrote: > On 22.01.16 at 02:56, wrote: >>> On Thu, Jan 21, 2016 at 05:52:12AM -0700, Jan Beulich wrote: At the very least it would need to be avoided by denying the request. Upon shared use, either all parties agree, or only one may use the clock. And passing through a (platform) device would therefore imply validating that the needed clock(s) are available to the target domain. Doing this in a consistent way with all control in one component's hands seems doable only if hypervisor and/or tool stack are the controlling (and arbitrating) entity. In the end this is one of the reasons why to me a simple PV I/O interface doesn't seem suitable here. >>> >>> How about let userspace libxl pvclk code to denying the request? >> >>Userspace would be fine, but > > Then you are ok with the pvclk way to handle clock for platform device > passthrough? No, not really. While I accept that doing clock management in the hypervisor is undesirable, we're still not at the point where such a frontend/backend pair would look like the only possible route out of a dilemma, and I continue to think that this proposed model should indeed only be the last resort. In particular, with the user space exposure of clock control discussed in another sub-thread, the next best option would seem to be to handle this via emulation in a device model. Yes, ARM guests currently have no qemu attached to them, but I guess sooner or later this will need to change anyway. >>- How would this fit in your frontend/backend model, where >> userspace shouldn't be involved at all? > > rethought about this. clk is binded to device. we can not passthrough > one device to two guest, so this means we can not let two different > guest access one clk input. Since this is mainly for embedded products, > just as Ian said "experts option", the developer should be aware of > the clk sharing between two device. > > If we truly need to let userspace deny the request. If one clk > already assigned to Dom1, then the toolstack need to fail > the creation of Dom2, if Dom2 want to use the same clock. I.e. you're now proposing actual assignment of clocks to guests? That's at least one step in the (from my pov) right direction... Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 05/10] acpi: Refactor acpi_os_map_memory to be architecturally independent
>>> On 22.01.16 at 10:37, wrote:
>
> On 2016/1/22 16:47, Jan Beulich wrote:
> On 22.01.16 at 09:38, wrote:
>>> >
>>> > On 2016/1/18 21:33, Jan Beulich wrote:
>>> > On 16.01.16 at 06:01, wrote:
>> >>> > --- a/xen/drivers/acpi/osl.c
>> >>> > +++ b/xen/drivers/acpi/osl.c
>> >>> > @@ -86,17 +86,7 @@ acpi_physical_address __init
>>> > acpi_os_get_root_pointer(void)
>> >>> > void __iomem *
>> >>> > acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
>> >>> > {
>> >>> > - if (system_state >= SYS_STATE_active) {
>> >>> > - mfn_t mfn = _mfn(PFN_DOWN(phys));
>> >>> > - unsigned int offs = phys & (PAGE_SIZE - 1);
>> >>> > -
>> >>> > - /* The low first Mb is always mapped. */
>> >>> > - if ( !((phys + size - 1) >> 20) )
>> >>> > - return __va(phys);
>> >>> > - return __vmap(&mfn, PFN_UP(offs + size), 1, 1,
>> >>> > -PAGE_HYPERVISOR_NOCACHE) + offs;
>> >>> > - }
>> >>> > - return __acpi_map_table(phys, size);
>> >>> > + return arch_acpi_os_map_memory(phys, size);
>> >>> > }
>> I'm quite sure I've said before that this goes too far: The __vmap()
>> part and likely also the early-boot __acpi_map_table() one already
>> are architecture independent and hence should stay. The factoring
>> hence should only concern the first Mb handling and maybe the
>> the mapping attributes passed to __vmap().
>>> >
>>> > Yes, the first MB handling and __vmap() should refactor. So if it only
>>> > moves them to an architecture function, how about below patch?
>>> >
>>> > diff --git a/xen/arch/x86/acpi/lib.c b/xen/arch/x86/acpi/lib.c
>>> > index cc15ea3..5885a3a 100644
>>> > --- a/xen/arch/x86/acpi/lib.c
>>> > +++ b/xen/arch/x86/acpi/lib.c
>>> > @@ -33,6 +33,19 @@ u8 __read_mostly acpi_disable_value;
>>> > u32 __read_mostly x86_acpiid_to_apicid[MAX_MADT_ENTRIES] =
>>> > {[0 ... MAX_MADT_ENTRIES - 1] = BAD_APICID };
>>> >
>>> > +void __iomem *
>>> > +arch_acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
>>> > +{
>>> > + mfn_t mfn = _mfn(PFN_DOWN(phys));
>>> > + unsigned int offs = phys & (PAGE_SIZE - 1);
>>> > +
>>> > + /* The low first Mb is always mapped. */
>>> > + if ( !((phys + size - 1) >> 20) )
>>> > + return __va(phys);
>>> > + return __vmap(&mfn, PFN_UP(offs + size), 1, 1,
>>> > + PAGE_HYPERVISOR_NOCACHE) + offs;
>>> > +}
>> Well, I had clearly said the vmap() part is generic; if there's
>> anything architecture dependent here, then the mapping
>> attributes (and hence only _those_ should be factored out,
>> not the entire function invocation).
> I know what you said. But how can we change the attribute for ARM in
> acpi_os_map_memory() without moving these codes out?
By having each arch #define their value, and use that constant here?
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [BUG] Assertion '(sp == 0) || (peoi[sp-1].vector < vector)' failed at irq.c:1163
>>> On 22.01.16 at 10:20, wrote:
> ** sp 1, irq 107, vec 0x3b
> **peoi[0] = {107, 0x3b, 0}
> Assertion '(sp == 0) || (peoi[sp-1].vector < vector)' failed at irq.c:1172
> [ Xen-4.6.0 x86_64 debug=y Tainted:C ]
>
> Xen call trace:
>[] do_IRQ+0x451/0x6ea
>[] common_interrupt+0x62/0x70
>[] mwait_idle+0x2cb/0x315
>[] idle_loop+0x51/0x6b
>
> So we have been interrupted with an interrupt we already believe to be
> pending. I wonder if there is an erratum to do with going to sleep with
> a pending interrupt.
An immediate way to check whether that's (part of) the problem
would be to run with "cpuidle=0" for a while.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH RFC 25/31] xen/x86: Common infrastructure for levelling context switching
On 22/01/16 08:56, Jan Beulich wrote:
On 16.12.15 at 22:24, wrote:
>> --- a/xen/arch/x86/cpu/common.c
>> +++ b/xen/arch/x86/cpu/common.c
>> @@ -35,6 +35,9 @@ integer_param("cpuid_mask_ext_edx",
>> opt_cpuid_mask_ext_edx);
>> unsigned int __initdata expected_levelling_cap;
>> unsigned int __read_mostly levelling_caps;
>>
>> +DEFINE_PER_CPU(struct cpumasks, cpumasks);
>> +struct cpumasks __read_mostly cpumask_defaults;
> Any reason these can't be introduced when first used? (The question
> really applies to the rest of this patch too, I guess.)
The subsequent two patches are sufficiently complicated in their own
right that I deliberately pulled these bits out.
>
>> --- a/xen/include/asm-x86/processor.h
>> +++ b/xen/include/asm-x86/processor.h
>> @@ -585,6 +585,21 @@ int microcode_resume_cpu(unsigned int cpu);
>> */
>> extern unsigned int expected_levelling_cap, levelling_caps;
>>
>> +struct cpumasks
>> +{
>> +uint64_t _1cd;
>> +uint64_t e1cd;
>> +uint64_t Da1;
>> +uint64_t _6c;
>> +uint64_t _7ab0;
>> +};
> While I see the need for these underscore prefixes with the
> current naming scheme, perhaps it would be better to make
> this fully consistent with the acronym #define-s, by e.g.
> prefixing the base ones with 'b'? Such full naming consistency
> would allow for string concatenation in macros, should such a
> possibility ever arise, no matter whether manifest constants
> or structure members here are needed to be accessed.
>
> As to the name of the structure itself - perhaps better
> cpuidmasks?
Ok.
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] xen: Add support for dom0 with Linux kernel 3.19 and newer
On 21/01/16 20:13, Daniel Kiper wrote: > Linux kernel commit 054954eb051f35e74b75a566a96fe756015352c8 > (xen: switch to linear virtual mapped sparse p2m list), which > appeared in 3.19, introduced linear virtual mapped sparse p2m > list. If readmem() reads p2m then it access this list using > physical addresses. Sadly, VMA to physical address translation > in crash requires access to p2m list. This means that we have > a chicken and egg problem. In general this issue must be solved > by introducing some changes in libxl, Linux kernel and crash > (I have added this task to my long TODO list). However, in dom0 > case we can use crash_xen_info_t.dom0_pfn_to_mfn_frame_list_list > which is available out of the box. So, let's use it and make > at least some users happy. I'm confused. How does a virtual address to (pseudo-)physical address lookup require access to the p2m? Surely this is a walk of the page tables followed by a M2P lookup on the MFN in the L1 PTE? David ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH RFC 30/31] x86/domctl: Update PV domain cpumasks when setting cpuid policy
>>> On 16.12.15 at 22:24, wrote:
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -77,6 +77,74 @@ static void update_domain_cpuid_info(struct domain *d,
> d->arch.x86_model = (ctl->eax >> 4) & 0xf;
> if ( d->arch.x86 >= 0x6 )
> d->arch.x86_model |= (ctl->eax >> 12) & 0xf0;
> +
> +if ( is_pv_domain(d) )
> +{
> +uint64_t mask = cpumask_defaults._1cd;
> +
> +if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL )
> +mask &= ((uint64_t)ctl->edx << 32) | ctl->ecx;
> +else if ( boot_cpu_data.x86_vendor == X86_VENDOR_AMD )
> +mask &= ((uint64_t)ctl->ecx << 32) | ctl->edx;
I'd prefer switch() to be used in cases like this, but anyway
Reviewed-by: Jan Beulich
notwithstanding possible mechanical adjustments to the patch due
to changes to earlier ones.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 1/6] libelf: rewrite symtab/strtab loading for Dom0
El 22/01/16 a les 9.11, Jan Beulich ha escrit:
On 21.01.16 at 18:55, wrote:
>> El 21/01/16 a les 18.29, Ian Jackson ha escrit:
>>> Roger Pau Monne writes ("[PATCH v4 1/6] libelf: rewrite symtab/strtab
>> loading for Dom0"):
Current implementation of elf_load_bsdsyms is broken when loading inside of
a HVM guest, because it assumes elf_memcpy_safe is able to write into guest
memory space, which it is not.
Take the oportunity to do some cleanup and properly document how
elf_{parse/load}_bsdsyms works. The new implementation uses elf_load_image
when dealing with data that needs to be copied to the guest memory space.
Also reduce the number of section headers copied to the minimum necessary.
>>> ...
#define elf_hdr_elm(_elf, _hdr, _elm, _val) \
do {\
if ( elf_64bit(_elf) ) \
-elf_store_field(_elf, _hdr, e64._elm, _val); \
+(_hdr).e64._elm = _val; \
>>>
>>> This seems to bypass the safe store mechanism which was introduced to
>>> fix XSA-55.
>>
>> This macro is only used to store fields inside of a structure that's
>> allocated on the stack, and it doesn't involve any kind of pointer
>> magic/arithmetic. The way it was used previously in this function indeed
>> required the use of the _safe mechanism in order to prevent writing into
>> arbitrary memory places, because we were actually modifying guest memory
>> IIRC.
>>
>> I could restore the previous behaviour, but that would mean adding some
>> handlers in order to access the structure. Since this is only used for
>> Dom0, which already makes use of the elf_memcpy_unchecked function as
>> called by elf_store_val which is in turn called from elf_store_field, so
>> it's not like we were protected previously anyway.
>
> If this is indeed Dom0-only code, could (and perhaps should) it be
> guarded suitably by an #ifdef to make obvious it's not used for
> DomU loading, and hence not security sensitive? From looking at
> the call sites of elf_{parse,load}_bsdsyms() I can't, btw., tell that
> this is Dom0-only ...
You are completely right, TBH I'm not sure what's going on with this.
xc_dom_elfloader.c has it's own functions to load the strtab/symtab, but
it's also calling elf_load_binary which, AFAICT, will call
elf_load_bsdsyms. Am I missing something, or are we loading the
symtab/strtab twice from libxc?
Roger.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH RFC 29/31] x86/pv: Provide custom cpumasks for PV domains
>>> On 16.12.15 at 22:24, wrote:
> --- a/xen/arch/x86/cpu/amd.c
> +++ b/xen/arch/x86/cpu/amd.c
> @@ -203,7 +203,9 @@ static void __init noinline probe_masking_msrs(void)
> void amd_ctxt_switch_levelling(const struct domain *nextd)
> {
> struct cpumasks *these_masks = &this_cpu(cpumasks);
> - const struct cpumasks *masks = &cpumask_defaults;
> + const struct cpumasks *masks =
> +(nextd && is_pv_domain(nextd) && nextd->arch.pv_domain.masks)
> +? nextd->arch.pv_domain.masks : &cpumask_defaults;
Can nextd really ever be NULL here?
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -578,6 +578,12 @@ int arch_domain_create(struct domain *d, unsigned int
> domcr_flags,
> goto fail;
> clear_page(d->arch.pv_domain.gdt_ldt_l1tab);
>
> +d->arch.pv_domain.masks = xmalloc(struct cpumasks);
> +if ( !d->arch.pv_domain.masks )
> +goto fail;
> +memcpy(d->arch.pv_domain.masks, &cpumask_defaults,
> + sizeof(*d->arch.pv_domain.masks));
Structure assignment, to make the thing type safe?
Also there's a change missing to the cleanup code after the "fail"
label.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH RFC 28/31] xen/x86: Context switch all levelling state in context_switch()
>>> On 16.12.15 at 22:24, wrote:
> --- a/xen/arch/x86/cpu/amd.c
> +++ b/xen/arch/x86/cpu/amd.c
> @@ -300,6 +300,9 @@ static void __init noinline amd_init_levelling(void)
> cpumask_defaults._6c &= (~0ULL << 32);
> cpumask_defaults._6c |= ecx;
> }
> +
> +if (levelling_caps)
> +ctxt_switch_levelling = amd_ctxt_switch_levelling;
> }
Indentation.
> --- a/xen/arch/x86/cpu/common.c
> +++ b/xen/arch/x86/cpu/common.c
> @@ -86,6 +86,13 @@ static const struct cpu_dev default_cpu = {
> };
> static const struct cpu_dev *this_cpu = &default_cpu;
>
> +void default_ctxt_switch_levelling(const struct domain *nextd)
static
> +{
> +/* Nop */
> +}
> +void (*ctxt_switch_levelling)(const struct domain *nextd) __read_mostly =
> +default_ctxt_switch_levelling;
While current and past gcc may accept (and honor) this placement of
the __read_mostly annotation, I think it is wrong from a strict language
syntax pov. Imo it instead ought to be
void (*__read_mostly ctxt_switch_levelling)(const struct domain *nextd) =
Also - indentation again.
> @@ -145,6 +145,13 @@ void intel_ctxt_switch_levelling(const struct domain
> *nextd)
> struct cpumasks *these_masks = &this_cpu(cpumasks);
> const struct cpumasks *masks = &cpumask_defaults;
>
> + if (cpu_has_cpuid_faulting) {
> + set_cpuid_faulting(nextd && is_pv_domain(nextd) &&
> +!is_control_domain(nextd) &&
> +!is_hardware_domain(nextd));
> + return;
> + }
Considering that you don't even probe the masking MSRs, this seems
inconsistent with your "always level the entire system" choice. As said
I'm opposed to that (i.e. the code here is fine), but at the very least
things ought to be consistent.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC V2] xen: interface: introduce pvclk interface
Hi Jan, On Fri, Jan 22, 2016 at 12:36:31AM -0700, Jan Beulich wrote: On 22.01.16 at 02:56, wrote: >> On Thu, Jan 21, 2016 at 05:52:12AM -0700, Jan Beulich wrote: >>>At the very least it would need to be avoided by denying the request. >>>Upon shared use, either all parties agree, or only one may use the >>>clock. And passing through a (platform) device would therefore imply >>>validating that the needed clock(s) are available to the target domain. >>>Doing this in a consistent way with all control in one component's >>>hands seems doable only if hypervisor and/or tool stack are the >>>controlling (and arbitrating) entity. In the end this is one of the >>>reasons why to me a simple PV I/O interface doesn't seem suitable >>>here. >> >> How about let userspace libxl pvclk code to denying the request? > >Userspace would be fine, but Then you are ok with the pvclk way to handle clock for platform device passthrough? >- How would this fit in your frontend/backend model, where > userspace shouldn't be involved at all? rethought about this. clk is binded to device. we can not passthrough one device to two guest, so this means we can not let two different guest access one clk input. Since this is mainly for embedded products, just as Ian said "experts option", the developer should be aware of the clk sharing between two device. If we truly need to let userspace deny the request. If one clk already assigned to Dom1, then the toolstack need to fail the creation of Dom2, if Dom2 want to use the same clock. In xl configuraiton file for Dom1, introduce such an entry, vclks=["gpu_root_clk,uart2_root_clk,usdhc2_root_clk"] and toolstack will create the xenstore nodes. /local/domain/0/backend/vclk/1/0/nrclks-->3 /local/domain/0/backend/vclk/1/0/clk-0/name-->gpu_root_clk /local/domain/0/backend/vclk/1/0/clk-1/name-->uart2_root_clk /local/domain/0/backend/vclk/1/0/clk-2/name-->usdhc2_root_clk /local/domain/1/device/vclk/0/clk-ring-ref /local/domain/1/device/vclk/0/event-channel The DomU dts also contains the clk names. Maybe the dts for clk node can be created by the toolstack. If Dom2 also want to use gpu_root_clk, it should be blocked by toolstack. Thanks, Peng. >- Libxl may be a little too high up the stack, libxc would seem a > more appropriate place to me (but that's subject to tools > maintainers disagreeing with me). > >Jan > ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH RFC 27/31] xen/x86: Rework Intel masking/faulting setup
>>> On 16.12.15 at 22:24, wrote:
> + if (msr_basic)
> + __probe_mask_msr(&msr_basic, LCAP_1cd, &cpumask_defaults._1cd);
> +
> + if (msr_ext)
> + __probe_mask_msr(&msr_ext, LCAP_e1cd, &cpumask_defaults.e1cd);
> +
> + if (msr_xsave)
> + __probe_mask_msr(&msr_xsave, LCAP_Da1, &cpumask_defaults.Da1);
> +
> + /*
> + * Don't bother warning about a mismatch if virtualised. These MSRs
> + * are not architectural and almost never virtualised.
> + */
> + if ((expected_levelling_cap == levelling_caps) ||
> + cpu_has_hypervisor)
> + return;
> +
> + printk(XENLOG_WARNING "Mismatch between expected (%#x"
> +") and real (%#x) levelling caps: missing %#x\n",
> +expected_levelling_cap, levelling_caps,
> +(expected_levelling_cap ^ levelling_caps) & levelling_caps);
> + printk(XENLOG_WARNING "Fam %#x, model %#x expected (%#x/%#x/%#x), "
> +"got (%#x/%#x/%#x)\n", c->x86, c->x86_model,
> +exp_msr_basic, exp_msr_ext, exp_msr_xsave,
> +msr_basic, msr_ext, msr_xsave);
I may not have noticed the same on the AMD patch, but printing
zeros as "missing" MSR indexes seems strange to me. Why not
print the missing MSRs with their textual names, easing cross
referencing with the FlexMigration document?
> +/*
> + * opt_cpuid_mask_ecx/edx: cpuid.1[ecx, edx] feature mask.
> + * For example, E8400[Intel Core 2 Duo Processor series] ecx = 0x0008E3FD,
> + * edx = 0xBFEBFBFF when executing CPUID.EAX = 1 normally. If you want to
> + * 'rev down' to E8400, you can set these values in these Xen boot
> parameters.
> + */
> +static void __init noinline intel_init_levelling(void)
> +{
> + if ( !probe_intel_cpuid_faulting() )
> + probe_masking_msrs();
> +
> + if ( msr_basic )
> + {
> + cpumask_defaults._1cd =
> + ((u64)opt_cpuid_mask_edx << 32) | opt_cpuid_mask_ecx;
> +
> + if ( !~(opt_cpuid_mask_ecx & opt_cpuid_mask_edx) )
> printk("Writing CPUID feature mask ecx:edx ->
> %08x:%08x\n",
> opt_cpuid_mask_ecx, opt_cpuid_mask_edx);
Are these messages, without adjustment to their wording, not
going to be confusing? After all the intention is to not just write
a single, never modified value. E.g. better "Defaulting ..."?
> @@ -183,22 +237,13 @@ static void early_init_intel(struct cpuinfo_x86 *c)
> (boot_cpu_data.x86_mask == 3 || boot_cpu_data.x86_mask == 4))
> paddr_bits = 36;
>
> - if (c == &boot_cpu_data && c->x86 == 6) {
> - if (probe_intel_cpuid_faulting())
> - __set_bit(X86_FEATURE_CPUID_FAULTING,
> - c->x86_capability);
> - } else if (boot_cpu_has(X86_FEATURE_CPUID_FAULTING)) {
> - BUG_ON(!probe_intel_cpuid_faulting());
> - __set_bit(X86_FEATURE_CPUID_FAULTING, c->x86_capability);
> - }
> + if (c == &boot_cpu_data)
> + intel_init_levelling();
> +
> + if (test_bit(X86_FEATURE_CPUID_FAULTING, boot_cpu_data.x86_capability))
> +__set_bit(X86_FEATURE_CPUID_FAULTING, c->x86_capability);
So you intentionally delete the validation of CPUID faulting being
available on APs? Also - indentation.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 05/10] acpi: Refactor acpi_os_map_memory to be architecturally independent
On 2016/1/22 16:47, Jan Beulich wrote:
On 22.01.16 at 09:38, wrote:
>> >
>> > On 2016/1/18 21:33, Jan Beulich wrote:
>> > On 16.01.16 at 06:01, wrote:
> >>> > --- a/xen/drivers/acpi/osl.c
> >>> > +++ b/xen/drivers/acpi/osl.c
> >>> > @@ -86,17 +86,7 @@ acpi_physical_address __init
>> > acpi_os_get_root_pointer(void)
> >>> > void __iomem *
> >>> > acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
> >>> > {
> >>> > - if (system_state >= SYS_STATE_active) {
> >>> > - mfn_t mfn = _mfn(PFN_DOWN(phys));
> >>> > - unsigned int offs = phys & (PAGE_SIZE - 1);
> >>> > -
> >>> > - /* The low first Mb is always mapped. */
> >>> > - if ( !((phys + size - 1) >> 20) )
> >>> > - return __va(phys);
> >>> > - return __vmap(&mfn, PFN_UP(offs + size), 1, 1,
> >>> > - PAGE_HYPERVISOR_NOCACHE) + offs;
> >>> > - }
> >>> > - return __acpi_map_table(phys, size);
> >>> > + return arch_acpi_os_map_memory(phys, size);
> >>> > }
>>> >> I'm quite sure I've said before that this goes too far: The __vmap()
>>> >> part and likely also the early-boot __acpi_map_table() one already
>>> >> are architecture independent and hence should stay. The factoring
>>> >> hence should only concern the first Mb handling and maybe the
>>> >> the mapping attributes passed to __vmap().
>> >
>> > Yes, the first MB handling and __vmap() should refactor. So if it only
>> > moves them to an architecture function, how about below patch?
>> >
>> > diff --git a/xen/arch/x86/acpi/lib.c b/xen/arch/x86/acpi/lib.c
>> > index cc15ea3..5885a3a 100644
>> > --- a/xen/arch/x86/acpi/lib.c
>> > +++ b/xen/arch/x86/acpi/lib.c
>> > @@ -33,6 +33,19 @@ u8 __read_mostly acpi_disable_value;
>> > u32 __read_mostly x86_acpiid_to_apicid[MAX_MADT_ENTRIES] =
>> > {[0 ... MAX_MADT_ENTRIES - 1] = BAD_APICID };
>> >
>> > +void __iomem *
>> > +arch_acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
>> > +{
>> > + mfn_t mfn = _mfn(PFN_DOWN(phys));
>> > + unsigned int offs = phys & (PAGE_SIZE - 1);
>> > +
>> > + /* The low first Mb is always mapped. */
>> > + if ( !((phys + size - 1) >> 20) )
>> > + return __va(phys);
>> > + return __vmap(&mfn, PFN_UP(offs + size), 1, 1,
>> > + PAGE_HYPERVISOR_NOCACHE) + offs;
>> > +}
> Well, I had clearly said the vmap() part is generic; if there's
> anything architecture dependent here, then the mapping
> attributes (and hence only _those_ should be factored out,
> not the entire function invocation).
I know what you said. But how can we change the attribute for ARM in
acpi_os_map_memory() without moving these codes out?
Thanks,
--
Shannon
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [XenGT][IGVT-g] DomU pgt_device structure initialization
Hello Greg,
Thanks for answer.
I am trying that you advice me.
Seems that there is same results.
Currently I am stopped this task, but if someone have more ideas why it can
be write please, Ill check.
Btw, I tried to look at /sys/kernel/debug/vgt/irqinfo and here seems all is
fine:
> --
>
> Interrupt control
> status:
> vGT: VLVDEISR is 10, VLVDEIIR is 0, VLVDEIMR is fffdff7f, VLVDEIER is
> 200f0
> vGT: DEISR is 0, DEIIR is 0, DEIMR is 0, DEIER is
> 8000
> vGT: SDEISR is 0, SDEIIR is 0, SDEIMR is 0, SDEIER is
> 0
> vGT: GTISR is 0, GTIIR is 0, GTIMR is 41, GTIER is
> 401001
> vGT: PMISR is 0, PMIIR is 0, PMIMR is 0, PMIER is
> 70
> vGT: RCS_IMR is , VCS_IMR is ffe00fff, BCS_IMR is
>
> Total 207574 interrupts
> logged:
> # WARNING: precisely this is the number of
> vGT
> # physical interrupt handler be
> called,
> # each calling several events can
> be
> # been handled, so usually this
> number
> # is less than the total events
> number.
> 4042: Render Command Streamer MI USER
> INTERRUPT
>1: Render MMIO sync flush
> status
>1: Video MMIO sync flush
> status
> 426: Blitter Command Streamer MI USER
> INTERRUPT
>1: Billter MMIO sync flush
> status
> 202447: Pipe A
> vblank
>38975: Render geyserville UP evaluation interval
> interrupt
> 1987: RP UP threshold
> interrupt
> 21: Render Frequency Downward Timeout During RC6
> interrupt
> 11740980876912: Last
> pirq
> 11740981018263: Last
> virq
>78066: Average pirq
> cycles
>15262: Average virq
> cycles
> 228105: Average delay between pirq/virq
> handling
>
>
> -->vgt-0:
>
> vreg (gtlc_mir: 8000, vlvier: 200f0, vlviir: 0, vlvimr: fffdff2f,
> vlvis)
> vreg (gtier: 401001, gtiir: 0, gtimr: 41, gtisr:
> 0)
> vreg (sdeier: 0, sdeiir: 0, sdeimr: 0, sdeisr:
> 0)
> vreg (pmier: 70, pmiir: 0, pmimr: 0, pmisr:
> 0)
> vreg (rcs_imr: , vcs_imr: 0, bcs_imr:
>
> 11740981028847: Last
> injection
> Total 208373 virtual irq
> injection:
> 3399: Render Command Streamer MI USER
> INTERRUPT
>1: Render MMIO sync flush
> status
>1: Video MMIO sync flush
> status
> 405: Blitter Command Streamer MI USER
> INTERRUPT
>1: Billter MMIO sync flush
> status
> 202205: Pipe A
> vblank
> 2304: Primary Plane A flip
> done
>38642: Render geyserville UP evaluation interval
> interrupt
> 1737: RP UP threshold
> interrupt
> 21: Render Frequency Downward Timeout During RC6
> interrupt
>
>
> -->vgt-1:
>
> vreg (gtlc_mir: 8000, vlvier: 0, vlviir: 0, vlvimr: ,
> vlvisr: 0)
> vreg (gtier: 401001, gtiir: 0, gtimr: 41, gtisr:
> 0)
> vreg (sdeier: 0, sdeiir: 0, sdeimr: 0, sdeisr:
> 0)
> vreg (pmier: 0, pmiir: 0, pmimr: 0, pmisr:
> 0)
> vreg (rcs_imr: , vcs_imr: 0, bcs_imr:
>
>9054347637006: Last
> injection
> Total 259251 virtual irq
> injection:
> 3359: Render Command Streamer MI USER
> INTERRUPT
>1: Render MMIO sync flush
> status
>1: Video MMIO sync flush
> status
> 174: Blitter Command Streamer MI USER
> INTERRUPT
>1: Billter MMIO sync flush
> status
> 391176: Pipe A
> vblank
> 3604: Primary Plane A flip
> done
>
With best regards,
Oleksii
On Thu, Jan 21, 2016 at 8:56 PM, Dr. Greg Wettstein
wrote:
> On Jan 5, 10:04am, Oleksii Kurochko wrote:
> } Subject: Re: [Xen-devel] [XenGT][IGVT-g] DomU pgt_device structure
> initial
>
> > Hey.
>
> Hi Oleksii, I hope this note finds your day going well.
>
> > Strange for me was that I got vmid=0 and gen_type=0, so I decided go
> > to i915_gem_vgtbuffer_ioctl and write next at the start: if
> > (!xen_initial_domain()) { return -EPERM; } Also same code is in 3.17
> > kernel from XenGT-kernel repo.
> >
> > it seems that there is no more this error now( from vgt_fb_decoder ), BUT
> > there is often freeze or very laggy UI in guest.
> >
> > What it can be?
>
> It wasn't in this e-mail but I went through the console logs which
> were in one of your postings on the IGVT-g list. I believe you have
> your hypervisor configured for synchronous serial console output
> (sync_console command-line parameter) and you are directing your dom0
> kernel console logging through the Xen provided serial interface.
>
> Setting this option is documented to cause significant latencies. In
> fact there is a warning about this in the Xen console logs when the
> hypervisor boots. Here is the code snippet from
> xen/
Re: [Xen-devel] [PATCH RFC 26/31] xen/x86: Rework AMD masking MSR setup
>>> On 16.12.15 at 22:24, wrote:
> This patch is best reviewed as its end result rather than as a diff, as it
> rewrites almost all of the setup.
This, I think, doesn't belong in the commit message itself.
> @@ -126,126 +133,172 @@ static const struct cpuidmask *__init noinline
> get_cpuidmask(const char *opt)
> }
>
> /*
> - * Mask the features and extended features returned by CPUID. Parameters are
> - * set from the boot line via two methods:
> - *
> - * 1) Specific processor revision string
> - * 2) User-defined masks
> - *
> - * The processor revision string parameter has precedene.
> + * Sets caps in expected_levelling_cap, probes for the specified mask MSR,
> and
> + * set caps in levelling_caps if it is found. Processors prior to Fam 10h
> + * required a 32-bit password for masking MSRs. Reads the default value into
> + * msr_val.
> */
> -static void __devinit set_cpuidmask(const struct cpuinfo_x86 *c)
> +static void __init __probe_mask_msr(unsigned int msr, uint64_t caps,
> +uint64_t *msr_val)
> {
> - static unsigned int feat_ecx, feat_edx;
> - static unsigned int extfeat_ecx, extfeat_edx;
> - static unsigned int l7s0_eax, l7s0_ebx;
> - static unsigned int thermal_ecx;
> - static bool_t skip_feat, skip_extfeat;
> - static bool_t skip_l7s0_eax_ebx, skip_thermal_ecx;
> - static enum { not_parsed, no_mask, set_mask } status;
> - unsigned int eax, ebx, ecx, edx;
> -
> - if (status == no_mask)
> - return;
> + unsigned int hi, lo;
> +
> +expected_levelling_cap |= caps;
Mixing indentation styles.
> +
> + if ((rdmsr_amd_safe(msr, &lo, &hi) == 0) &&
> + (wrmsr_amd_safe(msr, lo, hi) == 0))
> + levelling_caps |= caps;
This logic assumes that faults are possible only because the MSR is
not there at all, not because of the actual value used. Is this a safe
assumption, the more that you don't use the "safe" variants
anymore at runtime?
> +/*
> + * Probe for the existance of the expected masking MSRs. They might easily
> + * not be available if Xen is running virtualised.
> + */
> +static void __init noinline probe_masking_msrs(void)
> +{
> + const struct cpuinfo_x86 *c = &boot_cpu_data;
>
> - ASSERT((status == not_parsed) && (c == &boot_cpu_data));
> - status = no_mask;
> + /*
> + * First, work out which masking MSRs we should have, based on
> + * revision and cpuid.
> + */
>
> /* Fam11 doesn't support masking at all. */
> if (c->x86 == 0x11)
> return;
>
> - if (~(opt_cpuid_mask_ecx & opt_cpuid_mask_edx &
> - opt_cpuid_mask_ext_ecx & opt_cpuid_mask_ext_edx &
> - opt_cpuid_mask_l7s0_eax & opt_cpuid_mask_l7s0_ebx &
> - opt_cpuid_mask_thermal_ecx)) {
> - feat_ecx = opt_cpuid_mask_ecx;
> - feat_edx = opt_cpuid_mask_edx;
> - extfeat_ecx = opt_cpuid_mask_ext_ecx;
> - extfeat_edx = opt_cpuid_mask_ext_edx;
> - l7s0_eax = opt_cpuid_mask_l7s0_eax;
> - l7s0_ebx = opt_cpuid_mask_l7s0_ebx;
> - thermal_ecx = opt_cpuid_mask_thermal_ecx;
> - } else if (*opt_famrev == '\0') {
> + __probe_mask_msr(MSR_K8_FEATURE_MASK, LCAP_1cd,
> + &cpumask_defaults._1cd);
> + __probe_mask_msr(MSR_K8_EXT_FEATURE_MASK, LCAP_e1cd,
> + &cpumask_defaults.e1cd);
> +
> + if (c->cpuid_level >= 7)
> + __probe_mask_msr(MSR_AMD_L7S0_FEATURE_MASK, LCAP_7ab0,
> + &cpumask_defaults._7ab0);
> +
> + if (c->x86 == 0x15 && c->cpuid_level >= 6 && cpuid_ecx(6))
> + __probe_mask_msr(MSR_AMD_THRM_FEATURE_MASK, LCAP_6c,
> + &cpumask_defaults._6c);
> +
> + /*
> + * Don't bother warning about a mismatch if virtualised. These MSRs
> + * are not architectural and almost never virtualised.
> + */
> + if ((expected_levelling_cap == levelling_caps) ||
> + cpu_has_hypervisor)
> return;
> - } else {
> - const struct cpuidmask *m = get_cpuidmask(opt_famrev);
> +
> + printk(XENLOG_WARNING "Mismatch between expected (%#x"
> +") and real (%#x) levelling caps: missing %#x\n",
Breaking a format string inside parentheses is certainly a little odd.
Also I don't think the "missing" part is really useful, considering that
you already print both values used in its calculation.
> +expected_levelling_cap, levelling_caps,
> +(expected_levelling_cap ^ levelling_caps) & levelling_caps);
> + printk(XENLOG_WARNING "Fam %#x, model %#x level %#x\n",
> +c->x86, c->x86_model, c->cpuid_level);
> + printk(XENLOG_WARNING
> +"If not running virtualised, please report a bug\n");
Well - you checked for running virtualized, so making it here when
running virtualized is already a bug (albeit not in the code here
Re: [Xen-devel] Which trees are supported (Was: Re: [qemu-upstream-4.2-testing test] 77180: regressions - FAIL)
On Thu, 2016-01-07 at 11:22 +, Ian Campbell wrote: > So this arose because Stefano was unaware that 4.2 was no longer > supported. > Neither am I ever confident about where the cut-off lie, e.g. I > always have > to ask if I am doing backports for a security issue. > > We should add rows to > http://wiki.xen.org/wiki/Xen_Release_Features right > under Initial Release giving first the date until which that tree is > supported with backports and second the date until which security > support > will exist. We might also want to add a third "status" row. e.g. > "Supported", "Security Support only", "EOL" (we'll deal with extended > support by a third party when that next arises). > > I'm happy to make the edits, however I don't know what dates I would > write > here. Taking it to be 18 months of Support and a further 18 months of > security support I would get: > Xen 4.0 Xen 4.1 Xen 4.2 Xen > 4.3 Xen 4.4 Xen 4.5 Xen 4.6 > Initial Release 7 April 201025 March 2011 17 Sept 20129 > July 2013 10 March 2014 15 Jan 2015 13 Oct 2015 > Supported until EOL - ??? EOL - ??? EOL - ??? EOL - > Jan 2015 EOL - Sept 2015 July 2016 April 2017 > Security support tilEOL - ??? EOL - ??? EOL - ??? July > 2016 March 2016 Jan 2017Oct 2018 George pointed out that 4.4 only has 6 months security support here, which is just me counting wrong I think. It should be March 2017. Likewise 4.5 followed suite. Both of them should have been 1 year later. I have updated the wiki. Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [BUG] Assertion '(sp == 0) || (peoi[sp-1].vector < vector)' failed at irq.c:1163
On 22/01/2016 08:57, Håkon Alstadheim wrote:
> Den 17. jan. 2016 16:25, skrev Andrew Cooper:
>> On 17/01/16 15:16, Andrew Cooper wrote:
> This isn't the first time we have seen this on Haswell processors. Do
> you have microcode loading set up?
>
> ~Andrew
>
Still happening with kernel-genkernel-x86_64-4.1.15-gentoo and updated
cpu microcode, using microcode from 20151106.
> ...
Actually, this will be more useful:
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index 1228568..4e75b03 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -1165,6 +1165,15 @@ static void __do_IRQ_guest(int irq)
if ( action->ack_type == ACKTYPE_EOI )
{
sp = pending_eoi_sp(peoi);
+if ( unlikely(!((sp == 0) || (peoi[sp-1].vector < vector))) )
+{
+int p;
+
+printk("** sp %d, irq %d, vec %#x\n", sp, irq, vector);
+for ( p = sp; p > 0; --p )
+printk("**peoi[%d] = {%d, %#x, %d}\n",
+ p-1, peoi[p-1].irq, peoi[p-1].vector,
peoi[p-1].ready);
+}
ASSERT((sp == 0) || (peoi[sp-1].vector < vector));
ASSERT(sp < (NR_DYNAMIC_VECTORS-1));
peoi[sp].irq = irq;
> Got one again. dom5 is my desktop, dom1 is my
> mail-server/router/firewall. (planning to split that up ... ) . Is there
> any additional info that would be useful?
>
> Running now with gentoo xen 4.6.0-r8 and xen-tools 4.6.0-r7. dom0 kernel
> is gentoo-sources-4.1.15-r1 , and the above patch.
>
> I tried running with maxcpus=6 for a while, but I had to disable some
> services to get that running. So, when nothing happened for a while I
> re-enabled all my cores (two cpus, 12 cores, 24 threads). I was running
> with two cpu-pools, one for each cpu. I have not re-enabled that.
grant_table.c:1491:d1v3 Expanding dom (1) grant table from (12) to (13)
frames.
** sp 1, irq 107, vec 0x3b
**peoi[0] = {107, 0x3b, 0}
Assertion '(sp == 0) || (peoi[sp-1].vector < vector)' failed at irq.c:1172
[ Xen-4.6.0 x86_64 debug=y Tainted:C ]
Xen call trace:
[] do_IRQ+0x451/0x6ea
[] common_interrupt+0x62/0x70
[] mwait_idle+0x2cb/0x315
[] idle_loop+0x51/0x6b
So we have been interrupted with an interrupt we already believe to be
pending. I wonder if there is an erratum to do with going to sleep with
a pending interrupt.
I will see about extending the debugging patch to stash the IIR/ISR
before going to sleep.
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] Uniform commands for booting xen
On 18.01.2016 11:28, Ian Campbell wrote:
> On Mon, 2016-01-11 at 15:06 +0100, Vladimir 'φ-coder/phcoder' Serbinenko
> wrote:
>> On 13.11.2015 10:50, Ian Campbell wrote:
>>> On Fri, 2015-11-13 at 12:04 +0300, Andrei Borzenkov wrote:
> How do you express modules other than kernel+initrd in that
> scheme, without grub needing to be aware of any new addition we
> may find necessary going forward?
>
Are modules used by Xen self-identifying? Is it enough to simply pass
Xen kernel list of binary blobs or Xen kernel must be told what these
binary blobs are? If they are self identifying, why arm needs to be
passed module type in the first place?
>>>
>>> At first Xen/ARM required the bootloader to identify, but that was
>>> since
>>> identified as causing madness and fixed by having Xen/ARM do as Xen/x86
>>> does and figure things out for itself, but I failed to communicate this
>>> clearly and things got implemented on the grub side under the old
>>> assumptions.
>>>
>> This changes a lot. This removes most of hurdles towards uniformity. Are
>> you ok with replacing xen_kernel/xen_xsm/... with just xen_module and
>> dropping type altogether?
>
> So ending up with xen_hypervisor followed by one or more xen_module lines?
> That's fine with me. This bit:
>
> @@ -203,15 +155,11 @@ prepare_xen_module_params (struct xen_boot_binary
> *module, void *xen_boot_fdt)
>grub_fdt_add_subnode (xen_boot_fdt, chosen_node, module_name);
>
>retval = grub_fdt_set_prop (xen_boot_fdt, module_node, "compatible",
> - module->node_info.compat_string,
> - (grub_uint32_t) module->
> - node_info.compat_string_size);
> + "deprecated", sizeof("deprecated") - 1);
>
> Seems to be changing the compatibility string of hte node to "deprecated",
> which isn't right (or at least won't work). The nodes still need to be
> identified as being modules per http://xenbits.xen.org/docs/unstable/misc/a
> rm/device-tree/booting.txt that means "multiboot,module" (or if you insist
> "xen,multiboot-module").
>
Changed to "multiboot,module" and committed
>> Do you think that it makes sense to have xen_initrd in order to have
>> in-memory initrd concatenation like baremetal counterpart? In either
>> case we can add it later. I'd rather not have a command than to change
>> its meaning later.
>
> If it is useful on baremetal (and I can see that it would be) then I think
> it would be useful on Xen too.
>
> Ian.
>
>
signature.asc
Description: OpenPGP digital signature
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [BUG] EDAC infomation partially missing
Am 21.01.16 um 17:41 schrieb Jan Beulich: On 20.01.16 at 16:01, wrote: >> Initially reported to debian >> (http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=810964), redirected here: >> >> With AMD Opteron 6xxx processors, half of the memory controllers are >> missing from /sys/devices/system/edac/mc >> Checked with single 6120 (dual memory controller) and twin 6344 (2x dual >> MC), other dual-module CPUs might be affected too. >> >> Booting plain Linux (3.2, 3.16, 4.1, 4.3), all memory controllers are >> listed under /sys/devices/system/edac/mc as expected. Same happens, when >> Xen 4.1 is used: all MCs present. >> >> Starting with Xen 4.4 (Debian Jessie), only mc1 (on the single CPU >> machine) or mc2/mc3 (dual CPU machine) are present, although the full >> system memory is accessible. Checked versions were 4.1.4 (Debian >> Wheezy), 4.4.1 (Jessie) and 4.6.0 (Sid) > As already indicated by Ian in that bug, you should supply us with > full kernel and hypervisor logs for both the good and bad cases > (ideally with the same kernel version use in both runs, so that we > can exclude kernel behavior differences). Here are some dmesg excerpts, all performed with Linux 4.1.3. When booting with Xen 4.1.4: AMD64 EDAC driver v3.4.0 EDAC amd64: DRAM ECC enabled. EDAC amd64: F10h detected (node 0). EDAC MC: DCT0 chip selects: EDAC amd64: MC: 0: 0MB 1: 0MB EDAC amd64: MC: 2: 2048MB 3: 2048MB EDAC amd64: MC: 4: 0MB 5: 0MB EDAC amd64: MC: 6: 0MB 7: 0MB EDAC MC: DCT1 chip selects: EDAC amd64: MC: 0: 0MB 1: 0MB EDAC amd64: MC: 2: 2048MB 3: 2048MB EDAC amd64: MC: 4: 0MB 5: 0MB EDAC amd64: MC: 6: 0MB 7: 0MB EDAC amd64: using x8 syndromes. EDAC amd64: MCT channel count: 2 EDAC MC0: Giving out device to module amd64_edac controller F10h: DEV :00:18.2 (INTERRUPT) EDAC amd64: DRAM ECC enabled. EDAC amd64: F10h detected (node 1). EDAC MC: DCT0 chip selects: EDAC amd64: MC: 0: 0MB 1: 0MB EDAC amd64: MC: 2: 2048MB 3: 2048MB EDAC amd64: MC: 4: 0MB 5: 0MB EDAC amd64: MC: 6: 0MB 7: 0MB EDAC MC: DCT1 chip selects: EDAC amd64: MC: 0: 0MB 1: 0MB EDAC amd64: MC: 2: 2048MB 3: 2048MB EDAC amd64: MC: 4: 0MB 5: 0MB EDAC amd64: MC: 6: 0MB 7: 0MB EDAC amd64: using x8 syndromes. EDAC amd64: MCT channel count: 2 EDAC MC1: Giving out device to module amd64_edac controller F10h: DEV :00:19.2 (INTERRUPT) When booting with Xen 4.4.1: AMD64 EDAC driver v3.4.0 EDAC amd64: DRAM ECC enabled. EDAC amd64: NB MCE bank disabled, set MSR 0x017b[4] on node 0 to enable. EDAC amd64: ECC disabled in the BIOS or no ECC capability, module will not load. Either enable ECC checking or force module loading by setting 'ecc_enable_override'. (Note that use of the override may cause unknown side effects.) EDAC amd64: DRAM ECC enabled. EDAC amd64: F10h detected (node 1). EDAC MC: DCT0 chip selects: EDAC amd64: MC: 0: 0MB 1: 0MB EDAC amd64: MC: 2: 2048MB 3: 2048MB EDAC amd64: MC: 4: 0MB 5: 0MB EDAC amd64: MC: 6: 0MB 7: 0MB EDAC MC: DCT1 chip selects: EDAC amd64: MC: 0: 0MB 1: 0MB EDAC amd64: MC: 2: 2048MB 3: 2048MB EDAC amd64: MC: 4: 0MB 5: 0MB EDAC amd64: MC: 6: 0MB 7: 0MB EDAC amd64: using x8 syndromes. EDAC amd64: MCT channel count: 2 EDAC MC1: Giving out device to module amd64_edac controller F10h: DEV :00:19.2 (INTERRUPT) Apparently Xen4.4 doesn't report the BIOS flag correctly. I added ecc_enable_override=1 to amd64_edac_mod, and then I get EDAC MC: Ver: 3.0.0 AMD64 EDAC driver v3.4.0 EDAC amd64: DRAM ECC enabled. EDAC amd64: NB MCE bank disabled, set MSR 0x017b[4] on node 0 to enable. EDAC amd64: ECC disabled in the BIOS or no ECC capability, module will not load. EDAC amd64: Forcing ECC on! EDAC amd64: F10h detected (node 0). EDAC MC: DCT0 chip selects: EDAC amd64: MC: 0: 0MB 1: 0MB EDAC amd64: MC: 2: 2048MB 3: 2048MB EDAC amd64: MC: 4: 0MB 5: 0MB EDAC amd64: MC: 6: 0MB 7: 0MB EDAC MC: DCT1 chip selects: EDAC amd64: MC: 0: 0MB 1: 0MB EDAC amd64: MC: 2: 2048MB 3: 2048MB EDAC amd64: MC: 4: 0MB 5: 0MB EDAC amd64: MC: 6: 0MB 7: 0MB EDAC amd64: using x8 syndromes. EDAC amd64: MCT channel count: 2 EDAC MC0: Giving out device to module amd64_edac controller F10h: DEV :00:18.2 (INTERRUPT) EDAC amd64: DRAM ECC enabled. EDAC amd64: F10h detected (node 1). EDAC MC: DCT0 chip selects: EDAC amd64: MC: 0: 0MB 1: 0MB EDAC amd64: MC: 2: 2048MB 3: 2048MB EDAC amd64: MC: 4: 0MB 5: 0MB EDAC amd64: MC: 6: 0MB 7: 0MB EDAC MC: DCT1 chip selects: EDAC amd64: MC: 0: 0MB 1: 0MB EDAC amd64: MC: 2: 2048MB 3: 2048MB EDAC amd64: MC: 4: 0MB 5: 0MB EDAC amd64: MC: 6: 0MB 7: 0MB EDAC amd64: using x8 syndromes. EDAC amd64: MCT channel count: 2 EDAC MC1: Giving out device to module amd64_edac controller F10h: DEV :00:19.2 (INTERRUPT) This restored both MCs, so the BIOS flag seems to be the culprit. Re
Re: [Xen-devel] [PATCH RFC 25/31] xen/x86: Common infrastructure for levelling context switching
>>> On 16.12.15 at 22:24, wrote:
> --- a/xen/arch/x86/cpu/common.c
> +++ b/xen/arch/x86/cpu/common.c
> @@ -35,6 +35,9 @@ integer_param("cpuid_mask_ext_edx", opt_cpuid_mask_ext_edx);
> unsigned int __initdata expected_levelling_cap;
> unsigned int __read_mostly levelling_caps;
>
> +DEFINE_PER_CPU(struct cpumasks, cpumasks);
> +struct cpumasks __read_mostly cpumask_defaults;
Any reason these can't be introduced when first used? (The question
really applies to the rest of this patch too, I guess.)
> --- a/xen/include/asm-x86/processor.h
> +++ b/xen/include/asm-x86/processor.h
> @@ -585,6 +585,21 @@ int microcode_resume_cpu(unsigned int cpu);
> */
> extern unsigned int expected_levelling_cap, levelling_caps;
>
> +struct cpumasks
> +{
> +uint64_t _1cd;
> +uint64_t e1cd;
> +uint64_t Da1;
> +uint64_t _6c;
> +uint64_t _7ab0;
> +};
While I see the need for these underscore prefixes with the
current naming scheme, perhaps it would be better to make
this fully consistent with the acronym #define-s, by e.g.
prefixing the base ones with 'b'? Such full naming consistency
would allow for string concatenation in macros, should such a
possibility ever arise, no matter whether manifest constants
or structure members here are needed to be accessed.
As to the name of the structure itself - perhaps better
cpuidmasks?
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 05/10] acpi: Refactor acpi_os_map_memory to be architecturally independent
>>> On 22.01.16 at 09:38, wrote:
>
> On 2016/1/18 21:33, Jan Beulich wrote:
> On 16.01.16 at 06:01, wrote:
>>> > --- a/xen/drivers/acpi/osl.c
>>> > +++ b/xen/drivers/acpi/osl.c
>>> > @@ -86,17 +86,7 @@ acpi_physical_address __init
> acpi_os_get_root_pointer(void)
>>> > void __iomem *
>>> > acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
>>> > {
>>> > - if (system_state >= SYS_STATE_active) {
>>> > - mfn_t mfn = _mfn(PFN_DOWN(phys));
>>> > - unsigned int offs = phys & (PAGE_SIZE - 1);
>>> > -
>>> > - /* The low first Mb is always mapped. */
>>> > - if ( !((phys + size - 1) >> 20) )
>>> > - return __va(phys);
>>> > - return __vmap(&mfn, PFN_UP(offs + size), 1, 1,
>>> > - PAGE_HYPERVISOR_NOCACHE) + offs;
>>> > - }
>>> > - return __acpi_map_table(phys, size);
>>> > + return arch_acpi_os_map_memory(phys, size);
>>> > }
>> I'm quite sure I've said before that this goes too far: The __vmap()
>> part and likely also the early-boot __acpi_map_table() one already
>> are architecture independent and hence should stay. The factoring
>> hence should only concern the first Mb handling and maybe the
>> the mapping attributes passed to __vmap().
>
> Yes, the first MB handling and __vmap() should refactor. So if it only
> moves them to an architecture function, how about below patch?
>
> diff --git a/xen/arch/x86/acpi/lib.c b/xen/arch/x86/acpi/lib.c
> index cc15ea3..5885a3a 100644
> --- a/xen/arch/x86/acpi/lib.c
> +++ b/xen/arch/x86/acpi/lib.c
> @@ -33,6 +33,19 @@ u8 __read_mostly acpi_disable_value;
> u32 __read_mostly x86_acpiid_to_apicid[MAX_MADT_ENTRIES] =
> {[0 ... MAX_MADT_ENTRIES - 1] = BAD_APICID };
>
> +void __iomem *
> +arch_acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
> +{
> + mfn_t mfn = _mfn(PFN_DOWN(phys));
> + unsigned int offs = phys & (PAGE_SIZE - 1);
> +
> + /* The low first Mb is always mapped. */
> + if ( !((phys + size - 1) >> 20) )
> + return __va(phys);
> + return __vmap(&mfn, PFN_UP(offs + size), 1, 1,
> + PAGE_HYPERVISOR_NOCACHE) + offs;
> +}
Well, I had clearly said the vmap() part is generic; if there's
anything architecture dependent here, then the mapping
attributes (and hence only _those_ should be factored out,
not the entire function invocation).
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 05/10] acpi: Refactor acpi_os_map_memory to be architecturally independent
On 2016/1/18 21:33, Jan Beulich wrote:
On 16.01.16 at 06:01, wrote:
>> > --- a/xen/drivers/acpi/osl.c
>> > +++ b/xen/drivers/acpi/osl.c
>> > @@ -86,17 +86,7 @@ acpi_physical_address __init
>> > acpi_os_get_root_pointer(void)
>> > void __iomem *
>> > acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
>> > {
>> > - if (system_state >= SYS_STATE_active) {
>> > - mfn_t mfn = _mfn(PFN_DOWN(phys));
>> > - unsigned int offs = phys & (PAGE_SIZE - 1);
>> > -
>> > - /* The low first Mb is always mapped. */
>> > - if ( !((phys + size - 1) >> 20) )
>> > - return __va(phys);
>> > - return __vmap(&mfn, PFN_UP(offs + size), 1, 1,
>> > -PAGE_HYPERVISOR_NOCACHE) + offs;
>> > - }
>> > - return __acpi_map_table(phys, size);
>> > + return arch_acpi_os_map_memory(phys, size);
>> > }
> I'm quite sure I've said before that this goes too far: The __vmap()
> part and likely also the early-boot __acpi_map_table() one already
> are architecture independent and hence should stay. The factoring
> hence should only concern the first Mb handling and maybe the
> the mapping attributes passed to __vmap().
Yes, the first MB handling and __vmap() should refactor. So if it only
moves them to an architecture function, how about below patch?
diff --git a/xen/arch/x86/acpi/lib.c b/xen/arch/x86/acpi/lib.c
index cc15ea3..5885a3a 100644
--- a/xen/arch/x86/acpi/lib.c
+++ b/xen/arch/x86/acpi/lib.c
@@ -33,6 +33,19 @@ u8 __read_mostly acpi_disable_value;
u32 __read_mostly x86_acpiid_to_apicid[MAX_MADT_ENTRIES] =
{[0 ... MAX_MADT_ENTRIES - 1] = BAD_APICID };
+void __iomem *
+arch_acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
+{
+ mfn_t mfn = _mfn(PFN_DOWN(phys));
+ unsigned int offs = phys & (PAGE_SIZE - 1);
+
+ /* The low first Mb is always mapped. */
+ if ( !((phys + size - 1) >> 20) )
+ return __va(phys);
+ return __vmap(&mfn, PFN_UP(offs + size), 1, 1,
+ PAGE_HYPERVISOR_NOCACHE) + offs;
+}
+
/*
* Important Safety Note: The fixed ACPI page numbers are *subtracted*
* from the fixed base. That's why we start at FIX_ACPI_END and
diff --git a/xen/drivers/acpi/osl.c b/xen/drivers/acpi/osl.c
index a2fc8c4..e2dda2e 100644
--- a/xen/drivers/acpi/osl.c
+++ b/xen/drivers/acpi/osl.c
@@ -88,16 +88,8 @@ acpi_physical_address __init
acpi_os_get_root_pointer(void)
void __iomem *
acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
{
- if (system_state >= SYS_STATE_active) {
- mfn_t mfn = _mfn(PFN_DOWN(phys));
- unsigned int offs = phys & (PAGE_SIZE - 1);
-
- /* The low first Mb is always mapped. */
- if ( !((phys + size - 1) >> 20) )
- return __va(phys);
- return __vmap(&mfn, PFN_UP(offs + size), 1, 1,
- PAGE_HYPERVISOR_NOCACHE) + offs;
- }
+ if (system_state >= SYS_STATE_active)
+ return arch_acpi_os_map_memory(phys, size);
return __acpi_map_table(phys, size);
}
--
Shannon
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC v1 0/8] x86/init: Linux linker tables
On 21/01/16 21:37, Konrad Rzeszutek Wilk wrote: On Thu, Jan 21, 2016 at 12:33:43PM -0800, Luis R. Rodriguez wrote: Sure, do we know if that ICC compatible? Do we care? There are a series of ICC hacks put in place on ipxe's original solution which I've folded in, it seems that works but if we care about ICC those folks should perhaps help review as well. I didn't know the kernel could even be compiled with ICC? Thought only GCC worked? Anyhow - it may be that those fixes were for quite old ICC versions. Does the latest one manifest these oddities? I haven't tested building iPXE with icc for some time. (The support for icc was originally added with the plan to be able to compile to EFI Byte Code; this plan was swiftly abandoned.) The most recent version of icc that I have personally used with that code was 10.1.018. Michael ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 1/6] libelf: rewrite symtab/strtab loading for Dom0
>>> On 21.01.16 at 18:55, wrote:
> El 21/01/16 a les 18.29, Ian Jackson ha escrit:
>> Roger Pau Monne writes ("[PATCH v4 1/6] libelf: rewrite symtab/strtab
> loading for Dom0"):
>>> Current implementation of elf_load_bsdsyms is broken when loading inside of
>>> a HVM guest, because it assumes elf_memcpy_safe is able to write into guest
>>> memory space, which it is not.
>>>
>>> Take the oportunity to do some cleanup and properly document how
>>> elf_{parse/load}_bsdsyms works. The new implementation uses elf_load_image
>>> when dealing with data that needs to be copied to the guest memory space.
>>> Also reduce the number of section headers copied to the minimum necessary.
>> ...
>>> #define elf_hdr_elm(_elf, _hdr, _elm, _val) \
>>> do {\
>>> if ( elf_64bit(_elf) ) \
>>> -elf_store_field(_elf, _hdr, e64._elm, _val); \
>>> +(_hdr).e64._elm = _val; \
>>
>> This seems to bypass the safe store mechanism which was introduced to
>> fix XSA-55.
>
> This macro is only used to store fields inside of a structure that's
> allocated on the stack, and it doesn't involve any kind of pointer
> magic/arithmetic. The way it was used previously in this function indeed
> required the use of the _safe mechanism in order to prevent writing into
> arbitrary memory places, because we were actually modifying guest memory
> IIRC.
>
> I could restore the previous behaviour, but that would mean adding some
> handlers in order to access the structure. Since this is only used for
> Dom0, which already makes use of the elf_memcpy_unchecked function as
> called by elf_store_val which is in turn called from elf_store_field, so
> it's not like we were protected previously anyway.
If this is indeed Dom0-only code, could (and perhaps should) it be
guarded suitably by an #ifdef to make obvious it's not used for
DomU loading, and hence not security sensitive? From looking at
the call sites of elf_{parse,load}_bsdsyms() I can't, btw., tell that
this is Dom0-only ...
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 3/3] tools: introduce parameter max_wp_ram_ranges.
>>> On 22.01.16 at 04:20, wrote:
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -940,6 +940,10 @@ static int hvm_ioreq_server_alloc_rangesets(struct
> hvm_ioreq_server *s,
> {
> unsigned int i;
> int rc;
> +unsigned int max_wp_ram_ranges =
> +( s->domain->arch.hvm_domain.params[HVM_PARAM_MAX_WP_RAM_RANGES] > 0
> ) ?
> +s->domain->arch.hvm_domain.params[HVM_PARAM_MAX_WP_RAM_RANGES] :
> +MAX_NR_IO_RANGES;
Besides this having stray blanks inside the parentheses it truncates
the value from 64 to 32 bits and would benefit from using the gcc
extension of omitting the middle operand of ?:. But even better
would imo be if you avoided the local variable and ...
> @@ -962,7 +966,10 @@ static int hvm_ioreq_server_alloc_rangesets(struct
> hvm_ioreq_server *s,
> if ( !s->range[i] )
> goto fail;
>
> -rangeset_limit(s->range[i], MAX_NR_IO_RANGES);
> +if ( i == HVMOP_IO_RANGE_WP_MEM )
> +rangeset_limit(s->range[i], max_wp_ram_ranges);
> +else
> +rangeset_limit(s->range[i], MAX_NR_IO_RANGES);
... did the entire computation here, using ?: for the second argument
of the function invocation.
> @@ -6009,6 +6016,7 @@ static int hvm_allow_set_param(struct domain *d,
> case HVM_PARAM_IOREQ_SERVER_PFN:
> case HVM_PARAM_NR_IOREQ_SERVER_PAGES:
> case HVM_PARAM_ALTP2M:
> +case HVM_PARAM_MAX_WP_RAM_RANGES:
> if ( value != 0 && a->value != value )
> rc = -EEXIST;
> break;
Is there a particular reason you want this limit to be unchangeable
after having got set once?
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
