Re: [Xen-devel] [PATCH 3/4] x86: use optimal NOPs to fill the SMAP/SMEP placeholders
>>> On 07.03.16 at 18:43, wrote: > On 04/03/16 11:28, Jan Beulich wrote: >> Alternatives patching code picks the most suitable NOPs for the >> running system, so simply use it to replace the pre-populated ones. >> >> Use an arbitrary, always available feature to key off from. > > I would be tempted to introduce X86_FEATURE_ALWAYS as an alias of > X86_FEATURE_LM, or even a new synthetic feature. The choice of LM is > explained in the commit message, but will be non-obvious to people > reading the code. Okay, as an alias. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 3/3] xl: new "loglvl" command
>>> On 07.03.16 at 19:07, wrote:
> On Mon, 2016-03-07 at 04:46 -0700, Jan Beulich wrote:
>> > > > On 04.03.16 at 19:45, wrote:
>> > On Fri, 2016-03-04 at 09:48 -0700, Jan Beulich wrote:
>
>> > > --- a/tools/libxl/libxl.c
>> > > +++ b/tools/libxl/libxl.c
>> > > @@ -5958,6 +5958,26 @@ int libxl_send_debug_keys(libxl_ctx *ctx
>> > > return 0;
>> > > }
>> > >
>> > > +int libxl_log_level(libxl_ctx *ctx, bool set, bool guest,
>> > > +int *lower_thresh, int *upper_thresh)
>> > > +{
>> > > +int ret;
>> > >
>> > As per libxl coding style, this wants to be 'r'.
>> This and everything else below look to be valid comments, but
>> it's rather frustrating that simply cloning an existing function (I
>> user the debug key ones as basis) doesn't give me valid code,
>> the more that I did scroll up and down a few pages to see
>> whether I just happened to pick a particularly bad example.
>>
> Hehe, but do you understand that, saying this, you're making it very
> likely that people will ask *you* to fix libxl_send_debug_keys() --and
> perhaps more tool side code? :-P :-P
Except that it's not just that function - as said, I did scroll up and
down, without finding (style wise) better examples. And no, I'm
not going to put together patches to deal with style issues in the
tools.
> No, jokes apart, I agree that inconsistency is a real bad thing... but
> it's an hard fight, and we do have examples spread all around the
> source code (both Xen and tools), AFAICT.
Right, and asking people myself to not follow bad examples when
adding new code, I did take all of your input to adjust the patch.
Just that in this case the set of bad examples is so large that in a
similar case in the hypervisor I probably wouldn't have dared to
ask for a style correction.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[Xen-devel] [distros-debian-snapshot test] 44232: trouble: blocked/broken
flight 44232 distros-debian-snapshot real [real] http://osstest.xs.citrite.net/~osstest/testlogs/logs/44232/ Failures and problems with tests :-( Tests which did not succeed and are blocking, including tests which could not be run: build-i3863 host-install(3) broken REGR. vs. 44202 build-amd64-pvops 3 host-install(3) broken REGR. vs. 44202 build-armhf-pvops 3 host-install(3) broken REGR. vs. 44202 build-armhf 3 host-install(3) broken REGR. vs. 44202 build-amd64 3 host-install(3) broken REGR. vs. 44202 build-i386-pvops 3 host-install(3) broken REGR. vs. 44202 Tests which did not succeed, but are not blocking: test-armhf-armhf-armhf-daily-netboot-pygrub 1 build-check(1) blocked n/a test-amd64-i386-amd64-current-netinst-pygrub 1 build-check(1) blocked n/a test-amd64-i386-i386-current-netinst-pygrub 1 build-check(1) blocked n/a test-amd64-amd64-i386-current-netinst-pygrub 1 build-check(1) blocked n/a test-amd64-amd64-amd64-current-netinst-pygrub 1 build-check(1)blocked n/a test-amd64-i386-amd64-daily-netboot-pygrub 1 build-check(1) blocked n/a test-amd64-amd64-i386-daily-netboot-pygrub 1 build-check(1) blocked n/a test-amd64-i386-i386-weekly-netinst-pygrub 1 build-check(1) blocked n/a test-amd64-amd64-amd64-daily-netboot-pvgrub 1 build-check(1) blocked n/a test-amd64-i386-i386-daily-netboot-pvgrub 1 build-check(1)blocked n/a test-amd64-i386-amd64-weekly-netinst-pygrub 1 build-check(1) blocked n/a test-amd64-amd64-amd64-weekly-netinst-pygrub 1 build-check(1) blocked n/a test-amd64-amd64-i386-weekly-netinst-pygrub 1 build-check(1) blocked n/a baseline version: flight 44202 jobs: build-amd64 broken build-armhf broken build-i386 broken build-amd64-pvopsbroken build-armhf-pvopsbroken build-i386-pvops broken test-amd64-amd64-amd64-daily-netboot-pvgrub blocked test-amd64-i386-i386-daily-netboot-pvgrubblocked test-amd64-i386-amd64-daily-netboot-pygrub blocked test-armhf-armhf-armhf-daily-netboot-pygrub blocked test-amd64-amd64-i386-daily-netboot-pygrub blocked test-amd64-amd64-amd64-current-netinst-pygrubblocked test-amd64-i386-amd64-current-netinst-pygrub blocked test-amd64-amd64-i386-current-netinst-pygrub blocked test-amd64-i386-i386-current-netinst-pygrub blocked test-amd64-amd64-amd64-weekly-netinst-pygrub blocked test-amd64-i386-amd64-weekly-netinst-pygrub blocked test-amd64-amd64-i386-weekly-netinst-pygrub blocked test-amd64-i386-i386-weekly-netinst-pygrub blocked sg-report-flight on osstest.xs.citrite.net logs: /home/osstest/logs images: /home/osstest/images Logs, config files, etc. are available at http://osstest.xs.citrite.net/~osstest/testlogs/logs Test harness code can be found at http://xenbits.xensource.com/gitweb?p=osstest.git;a=summary Push not applicable. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] Running Xen on Nvidia Jetson-TK1
Hi All, I'm working on a research project with IBM, and I want to run Xen on Nvidia Tegra Jetson-tk1 board. I looked at a post on this mailing list (http://lists.xenproject.org/archives/html/xen-devel/2015-03/msg01122.html), and I am using this git tree - git://xenbits.xen.org/people/ianc/xen.git and branch - tegra-tk1-jetson-v1 But when I try to boot Xen on the board I am not able to see any output (even with earlyprintk enabled). After jumping to Xen the board just resets without showing any output. I am using upstream u-boot with non secure mode enabled. I have also tested booting the Linux kernel on the same setup and Linux 4.0 is able to boot with all 4 cores in HYP mode and kvm enabled. Can anyone help me as to what I might have done wrong while using Xen? Thanks, Dushyant ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 2/4] ns16550: enable Pericom controller support
>>> On 07.03.16 at 23:04, wrote:
>> +[param_pericom_4port] = {
>> +.base_baud = 921600,
>> +.uart_offset = 8,
>> +.reg_width = 1,
>> +.fifo_size = 16,
>> +.lsr_mask = UART_LSR_THRE,
>> +.bar0 = 1,
>> +.max_ports = 4,
>> +},
>> +[param_pericom_8port] = {
>> +.base_baud = 921600,
>> +.uart_offset = 8,
>> +.reg_width = 1,
>> +.fifo_size = 16,
>> +.lsr_mask = UART_LSR_THRE,
>> +.bar0 = 1,
>> +.max_ports = 8,
>
> Perhaps document that Xen can only access two of the ports? Unless we
> expand the ns16550_com array of course.
Done.
>> @@ -843,8 +911,10 @@ pci_uart_config(struct ns16550 *uart, bo
>> {
>> for ( f = 0; f < 8; f = nextf )
>> {
>> +unsigned int bar_idx = 0, port_idx = idx;
>
> s/port_idx/port/? or port_nr /?
"port" would be misleading/ambiguous, and I don't see port_nr being
any better than port_idx (or if so, it ought to then also be bar_nr).
In fact, "nr" - other than "idx" - is ambiguous too (commonly
indicating "number of ...").
>> @@ -863,15 +933,38 @@ pci_uart_config(struct ns16550 *uart, bo
>> continue;
>> }
>>
>> +/* Check for params in uart_config lookup table */
>> +for ( i = 0; i < ARRAY_SIZE(uart_config); i++)
>
> I am pretty sure I wrote this piece of code - could you fix the
> Style on it please? The i++) please?
Sure.
>> +if ( port_idx >= param->max_ports )
>> +{
>> +idx -= param->max_ports;
>> +continue;
>
> Could you add a comment about this? I understand it can detect if we are
> using an AMT device with the 'com2=115200,8n1,amt' (which would be
> invalid - AMT devices only have one IO PORT and there is only one of
> them on the machine) we would skip over the found device and continue on..
> Thought I don't understand why we want to decrease the idx value from one to
> zero?
If we're looking for COM2 and have found a 1-port card, we want to
use the 1st (rather than the 2nd) port on the next card we may find
(if any). This seems pretty obvious behavior to me here, so I'm not
really convinced a comment is warranted.
> Hmm, if it was some other PCI based serial card like:
>
> 01:05.0 Serial controller: NetMos Technology PCI 9835 Multi-I/O
> Controller (rev 01) (prog-if 02 [16550])
> Subsystem: LSI Logic / Symbios Logic Device 0001
> Flags: medium devsel, IRQ 20
> I/O ports at e050 [size=8]
> I/O ports at e040 [size=8]
> I/O ports at e030 [size=8]
> I/O ports at e020 [size=8]
> I/O ports at e010 [size=8]
> I/O ports at e000 [size=16]
>
> With 'com1=115200,8n1,pci' and 'com2=115200,8n1,pci' then the first loop
> would find the device. The second loop would decrement idx (1) by 1 and
> continue.. which would make it go search for another device.
>
> I hadn't tested this patch on the above device but I believe it used
> to work with the com1 and com2 going throught it - while with the new code
> it won't?
That's the !bar0 case, and hence the code in the loop over
uart_config[] would set port_idx to zero, so the conditional above
won't evaluate to true anyway. I.e. no change in behavior over
the original code (albeit arguably that behavior is not fully correct,
at least if we consider arbitrary bar_idx values - right now it can
only be 0 or 1 -, since some skipping logic would then be needed
too). The question is whether we shouldn't have all single port
cards have their bar0 flag set to true (or extend the conditional
inside the loop to "!param->bar0 && param->max_ports > 1"), to
enable this skipping in all of those cases.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 for Xen 4.7 1/4] xen: enable per-VCPU parameter settings for RTDS scheduler
>>> On 07.03.16 at 18:53, wrote:
> On Mon, 2016-03-07 at 09:40 -0700, Jan Beulich wrote:
>> > > > On 07.03.16 at 17:28, wrote:
>> > On Mon, Mar 7, 2016 at 6:59 AM, Jan Beulich
>> > wrote:
>> > >
>> > > > @@ -1163,6 +1173,96 @@ rt_dom_cntl(
>> > > >
>> > > > +case XEN_DOMCTL_SCHEDOP_getvcpuinfo:
>> > > > +if ( guest_handle_is_null(op->u.v.vcpus) )
>> > > > +{
>> > > > +rc = -EINVAL;
>> > > Perhaps rather -EFAULT? But then again - what is this check good
>> > > for
>> > > (considering that it doesn't cover other obviously bad handle
>> > > values)?
>> > Dario suggested this in the last post, because vcpus is a handle
>> > and
>> > needs to be validated.
>>
>> Well, as said - the handle being non-null doesn't make it a valid
>> handle. Any validation can be left to copy_{to,from}_guest*()
>> unless you mean to give a null handle some special meaning.
>>
> IIRC, I was looking at how XEN_SYSCTL_pcitopoinfo is handled, for
> reference, and that has some guest_handle_is_null()==>EINVAL sainity
> checking (in xen/common/sysctl.c), which, when I thought about it, made
> sense to me.
>
> My reasoning was, sort of:
> 1. if the handle is NULL, no point getting into the somewhat
> complicated logic of the while,
> 2. more accurate error reporting: as being passed a NULL handler
> looked something we could identify and call invalid, rather than
> waiting for the copy to fault.
I think the XEN_SYSCTL_pcitopoinfo was misguided in this respect,
cloning non applicable logic here which returns the number of needed
(array) elements in such a case for a few other operations.
>> > > > +{
>> > > > +rc = -EINVAL;
>> > > > +break;
>> > > > +}
>> > > > +
>> > > > +spin_lock_irqsave(&prv->lock, flags);
>> > > > +svc = rt_vcpu(d->vcpu[local_sched.vcpuid]);
>> > > > +local_sched.s.rtds.budget = svc->budget /
>> > > > MICROSECS(1);
>> > > > +local_sched.s.rtds.period = svc->period /
>> > > > MICROSECS(1);
>> > > > +spin_unlock_irqrestore(&prv->lock, flags);
>> > > > +
>> > > > +if ( __copy_to_guest_offset(op->u.v.vcpus, index,
>> > > > +&local_sched, 1) )
>> > > > +{
>> > > > +rc = -EFAULT;
>> > > > +break;
>> > > > +}
>> > > > +if ( (++index > 0x3f) && hypercall_preempt_check()
>> > > > )
>> > > > +break;
>> > > So how is the caller going to be able to reliably read all vCPU-
>> > > s'
>> > > information for a guest with more than 64 vCPU-s?
>> > In libxc, we re-issue hypercall if the current one is preempted.
>> And with the current code - how does libxc know? (And anyway,
>> this should only be a last resort, if the hypervisor can't by itself
>> arrange for a continuation. If done this way, having a code
>> comment referring to the required caller behavior would seem to
>> be an absolute must.)
>>
> I definitely agree on commenting.
>
> About the structure of the code, as said above, I do like
> how XEN_SYSCTL_pcitopoinfo ended up being handled, I think it is a
> great fit for this specific case and, comparing at both this and
> previous version, I do think this one is (bugs apart) looking better.
>
> I'm sure I said this --long ago-- when discussing v4 (and maybe even
> previous versions), as well as more recently, when reviewing v5, and
> that's why Chong (finally! :-D) did it.
>
> So, with the comment in place (and with bugs fixed :-)), are you (Jan)
> ok with this being done this way?
Well, this _might_ be acceptable for "get" (since the caller
abandoning the sequence of calls prematurely is no problem),
but for "set" it looks less suitable, as similar abandoning would
leave the guest in some inconsistent / unintended state. The
issue with XEN_SYSCTL_pcitopoinfo was, iirc, the lack of a
good way of encoding the continuation information, and while
that would seem applicable here too I'm not sure now whether
doing it the way it was done was the best choice. Clearly
stating (in the public interface header) that certain normally
input-only fields are volatile would allow the continuation to
be handled without tool stack assistance afaict.
>> > > > +}
>> > > > +
>> > > > +if ( !rc && (op->u.v.nr_vcpus != index) )
>> > > > +op->u.v.nr_vcpus = index;
>> > > I don't think the right side of the && is really necessary /
>> > > useful.
>> > The right side is to check whether the vcpus array is fully
>> > processed.
>> > When it is true and no error occurs (rc == 0), we
>> > update op->u.v.nr_vcpus, which is returned to libxc, and helps xc
>> > function figuring out how many un-processed vcpus should
>> > be taken care of in the next hypercall.
>> Just consider what the contents of op->u.v.nr_vcpus is after
>> this piece of code was executed, once with the full conditional,
>> and another time with the right side of the &&
Re: [Xen-devel] [RFC Design Doc] Add vNVDIMM support for Xen
On 03/04/16 10:20, Haozhong Zhang wrote:
> On 03/02/16 06:03, Jan Beulich wrote:
> > >>> On 02.03.16 at 08:14, wrote:
> > > It means NVDIMM is very possibly mapped in page granularity, and
> > > hypervisor needs per-page data structures like page_info (rather than the
> > > range set style nvdimm_pages) to manage those mappings.
> > >
> > > Then we will face the problem that the potentially huge number of
> > > per-page data structures may not fit in the normal ram. Linux kernel
> > > developers came across the same problem, and their solution is to
> > > reserve an area of NVDIMM and put the page structures in the reserved
> > > area (https://lwn.net/Articles/672457/). I think we may take the similar
> > > solution:
> > > (1) Dom0 Linux kernel reserves an area on each NVDIMM for Xen usage
> > > (besides the one used by Linux kernel itself) and reports the address
> > > and size to Xen hypervisor.
> > >
> > > Reasons to choose Linux kernel to make the reservation include:
> > > (a) only Dom0 Linux kernel has the NVDIMM driver,
> > > (b) make it flexible for Dom0 Linux kernel to handle all
> > > reservations (for itself and Xen).
> > >
> > > (2) Then Xen hypervisor builds the page structures for NVDIMM pages and
> > > stores them in above reserved areas.
> >
[...]
> > Furthermore - why would Dom0 waste space
> > creating per-page control structures for regions which are
> > meant to be handed to guests anyway?
> >
>
> I found my description was not accurate after consulting with our driver
> developers. By default the linux kernel does not create page structures
> for NVDIMM which is called by kernel the "raw mode". We could enforce
> the Dom0 kernel to pin NVDIMM in "raw mode" so as to avoid waste.
>
More thoughts on reserving NVDIMM space for per-page structures
Currently, a per-page struct for managing mapping of NVDIMM pages may
include following fields:
struct nvdimm_page
{
uint64_t mfn;/* MFN of SPA of this NVDIMM page */
uint64_t gfn;/* GFN where this NVDIMM page is mapped */
domid_t domain_id; /* which domain is this NVDIMM page mapped to */
int is_broken; /* Is this NVDIMM page broken? (for MCE) */
}
Its size is 24 bytes (or 22 bytes if packed). For a 2 TB NVDIMM,
nvdimm_page structures would occupy 12 GB space, which is too hard to
fit in the normal ram on a small memory host. However, for smaller
NVDIMMs and/or hosts with large ram, those structures may still be able
to fit in the normal ram. In the latter circumstance, nvdimm_page
structures are stored in the normal ram, so they can be accessed more
quickly.
So we may add a boot parameter for Xen to allow users to configure which
place, the normal ram or nvdimm, are used to store those structures. For
the config of using normal ram, Xen could manage nvdimm_page structures
more quickly (and hence start a domain with NVDIMM more quickly), but
leaves less normal ram for VMs. For the config of using nvdimm, Xen would
take more time to mange nvdimm_page structures, but leaves more normal
ram for VMs.
Haozhong
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC Design Doc] Add vNVDIMM support for Xen
>>> On 08.03.16 at 10:15, wrote:
> More thoughts on reserving NVDIMM space for per-page structures
>
> Currently, a per-page struct for managing mapping of NVDIMM pages may
> include following fields:
>
> struct nvdimm_page
> {
> uint64_t mfn;/* MFN of SPA of this NVDIMM page */
> uint64_t gfn;/* GFN where this NVDIMM page is mapped */
> domid_t domain_id; /* which domain is this NVDIMM page mapped to */
> int is_broken; /* Is this NVDIMM page broken? (for MCE) */
> }
>
> Its size is 24 bytes (or 22 bytes if packed). For a 2 TB NVDIMM,
> nvdimm_page structures would occupy 12 GB space, which is too hard to
> fit in the normal ram on a small memory host. However, for smaller
> NVDIMMs and/or hosts with large ram, those structures may still be able
> to fit in the normal ram. In the latter circumstance, nvdimm_page
> structures are stored in the normal ram, so they can be accessed more
> quickly.
Not sure how you came to the above structure - it's the first time
I see it, yet figuring out what information it needs to hold is what
this design process should be about. For example, I don't see why
it would need to duplicate M2P / P2M information. Nor do I see why
per-page data needs to hold the address of a page (struct
page_info also doesn't). And whether storing a domain ID (rather
than a pointer to struct domain, as in struct page_info) is the
correct think is also to be determined (rather than just stated).
Otoh you make no provisions at all for any kind of ref counting.
What if a guest wants to put page tables into NVDIMM space?
Since all of your calculations are based upon that fixed assumption
on the structure layout, I'm afraid they're not very meaningful
without first settling on what data needs tracking in the first place.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 1/2] xsm: move the XSM_MAGIC value to Kconfig
>>> On 07.03.16 at 19:42, wrote: > Let Kconfig set the XSM_MAGIC value for us. What's the benefit of doing this at the Kconfig layer? > --- a/xen/common/Kconfig > +++ b/xen/common/Kconfig > @@ -113,6 +113,14 @@ config XSM > > If unsure, say N. > > +# XSM magic for policy detection > +config XSM_MAGIC > +hex > +default 0xf97cff8c if FLASK > +default 0 if !FLASK This second "if" is pointless. Also note the broken indentation (using spaces instead of a tab). Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 2/2] xsm: move FLASK_AVC_STATS to Kconfig
>>> On 07.03.16 at 19:42, wrote: > Have Kconfig set CONFIG_FLASK_AVC_STATS and prefix all uses with CONFIG_ > to use the Kconfig variable. Same question here: What's the benefit of doing it this way? > --- a/xen/common/Kconfig > +++ b/xen/common/Kconfig > @@ -23,6 +23,12 @@ config FLASK > > If unsure, say N. > > +config FLASK_AVC_STATS > +def_bool y if FLASK > +depends on FLASK With this "depends" the "if FLASK" is pointless. Also (again) - indentation. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] tools/foreign: Avoid using alignment directives when not appropriate
>>> On 07.03.16 at 19:28, wrote: > --- a/tools/include/xen-foreign/Makefile > +++ b/tools/include/xen-foreign/Makefile > @@ -35,6 +35,8 @@ x86_32.h: mkheader.py structs.py > $(ROOT)/arch-x86/xen-x86_32.h $(ROOT)/arch-x86/ > > x86_64.h: mkheader.py structs.py $(ROOT)/arch-x86/xen-x86_64.h > $(ROOT)/arch-x86/xen.h $(ROOT)/xen.h > $(PYTHON) $< $* $@ $(filter %.h,$^) > + #Avoid mixing an alignment directive with a uint64_t cast or sizeof > expression > + sed 's/(__align8__ uint64_t)/(uint64_t)/g' -i $@ A two step rule like this should make use of a temporary file, to avoid breakage when the build process gets interrupted between the two steps. And then - is it perhaps worth to generalize the pattern in one or more of a couple of possible ways? Considering int64_t uses would perhaps be the most relevant one (even if not needed right away). But of course this could get as generic as s/(__align[0-9]*__ \([a-z0-9_]*\))/(\1)/g without - afaict (based on your commit description) - breaking anything. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3] xen/mm: Fix page_list_* helpers to evaluate all their arguments
>>> On 07.03.16 at 19:12, wrote: > --- a/xen/include/asm-arm/domain.h > +++ b/xen/include/asm-arm/domain.h > @@ -65,6 +65,7 @@ struct arch_domain > RELMEM_mapping, > RELMEM_done, > } relmem; > +struct page_list_head relmem_list; Well, if I was ARM maintainer I would say no to this otherwise pointless addition (even more so that this list doesn't get initialized anywhere). The expectation I had for how the build issue would be fixed was to simply not convert (at least) page_list_del2() to an inline function. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [V3] x86/xsaves: fix overwriting between non-lazy/lazy xsaves
>>> On 08.03.16 at 08:19, wrote:
> After doing some performance test on xsavec and xsaveopt(suggested by jan),
> the result show xsaveopt performs better than xsavec. This patch will clean
> up xsavec suppot code in xen.
>
> Also xsaves will be disabled (xsaves will be used when supervised state is
> introduced). Here in this patch do not change xc_cpuid_config_xsave in
> tools/libxc/xc_cpuid_x86.c but add some check in hvm_cpuid. Next time
> xsaves is needed, only add some code in xstate_init is enough.
I think both of these are too much of a step backwards. E.g. ...
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -922,7 +922,7 @@ long arch_do_domctl(
> ret = -EFAULT;
>
> offset += sizeof(v->arch.xcr0_accum);
> -if ( !ret && (cpu_has_xsaves || cpu_has_xsavec) )
> +if ( !ret && cpu_has_xsaves )
... here (and similarly elsewhere) you shouldn't make the code
continue to depend on the raw CPU feature, but you should have
a variable (or could be a #define for now) indicating whether we're
actively using compressed state areas. For the purpose of
alternative patching, the most suitable thing likely would be a
synthetic CPU feature.
In no case do I see any reason to artificially make cpu_has_* yield
false despite the hardware actually having that feature. Doing so
would only risk making future changes more cumbersome.
> --- a/xen/arch/x86/i387.c
> +++ b/xen/arch/x86/i387.c
> @@ -118,7 +118,19 @@ static inline uint64_t vcpu_xsave_mask(const struct vcpu
> *v)
> if ( v->fpu_dirtied )
> return v->arch.nonlazy_xstate_used ? XSTATE_ALL : XSTATE_LAZY;
>
> -return v->arch.nonlazy_xstate_used ? XSTATE_NONLAZY : 0;
> +/*
> + * The offsets of components in the extended region of xsave area xsaved
> by
> + * xasves are not fixed. This may cause overwriting xsave area when
> + * v->fpu_dirtied set is followed by one with v->fpu_dirtied clear.
> + * The way solve this problem is taking xcro_accum into consideration.
> + * if guest has ever used lazy states (exclude XSTATE_FP_SSE),
> + * vcpu_xsave_mask will return XSTATE_ALL. Otherwise return
> XSTATE_NONLAZY.
> + * The reason XSTATE_FP_SSE should be excluded is that the offsets of
> + * XSTATE_FP_SSE (in the legacy region of xsave area) are fixed, saving
> + * XSTATE_FS_SSE using xsaves will not cause overwriting problem.
> + */
Please carefully go through this comment and fix all typos,
typographical issues, and ideally also grammar. And there also is at
least one apparent factual issue: "The reason XSTATE_FP_SSE
should be excluded ..." seems wrong to me - I think you mean "may"
instead of "should", because this is an optimization aspect, not a
requirement of any sort.
> --- a/xen/arch/x86/xstate.c
> +++ b/xen/arch/x86/xstate.c
> @@ -165,7 +165,7 @@ void expand_xsave_states(struct vcpu *v, void *dest,
> unsigned int size)
> u64 xstate_bv = xsave->xsave_hdr.xstate_bv;
> u64 valid;
>
> -if ( !cpu_has_xsaves && !cpu_has_xsavec )
> +if ( !cpu_has_xsaves )
> {
> memcpy(dest, xsave, size);
> return;
> @@ -206,7 +206,7 @@ void compress_xsave_states(struct vcpu *v, const void
> *src, unsigned int size)
> u64 xstate_bv = ((const struct xsave_struct *)src)->xsave_hdr.xstate_bv;
> u64 valid;
>
> -if ( !cpu_has_xsaves && !cpu_has_xsavec )
> +if ( !cpu_has_xsaves )
> {
> memcpy(xsave, src, size);
> return;
Wouldn't both of these better simply check xcomp_bv[63] instead
of CPU features?
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[Xen-devel] [seabios baseline-only test] 44231: regressions - FAIL
This run is configured for baseline tests only. flight 44231 seabios real [real] http://osstest.xs.citrite.net/~osstest/testlogs/logs/44231/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-amd64-amd64-qemuu-nested-intel 16 debian-hvm-install/l1/l2 fail REGR. vs. 44204 Regressions which are regarded as allowable (not blocking): test-amd64-amd64-xl-qemuu-win7-amd64 16 guest-stopfail REGR. vs. 44204 test-amd64-i386-xl-qemuu-win7-amd64 16 guest-stop fail like 44204 Tests which did not succeed, but are not blocking: 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-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check fail never pass version targeted for testing: seabios dce99e01b6bfc51175bdf32612fd4f2738e5c3c8 baseline version: seabios 3f478b9fcffe1810532192be9d1781f03999776d Last test of basis44204 2016-03-01 18:21:00 Z6 days Testing same since44231 2016-03-08 05:22:26 Z0 days1 attempts People who touched revisions under test: Gal Hammer Marcel Apfelbaum jobs: build-amd64-xsm pass build-i386-xsm pass build-amd64 pass build-i386 pass build-amd64-libvirt pass build-i386-libvirt pass build-amd64-pvopspass build-i386-pvops pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsmpass test-amd64-amd64-xl-qemuu-debianhvm-amd64-xsmpass test-amd64-i386-xl-qemuu-debianhvm-amd64-xsm pass test-amd64-amd64-qemuu-nested-amdfail test-amd64-i386-qemuu-rhel6hvm-amd pass test-amd64-amd64-xl-qemuu-debianhvm-amd64pass test-amd64-i386-xl-qemuu-debianhvm-amd64 pass test-amd64-amd64-xl-qemuu-ovmf-amd64 pass test-amd64-i386-xl-qemuu-ovmf-amd64 pass test-amd64-amd64-xl-qemuu-win7-amd64 fail test-amd64-i386-xl-qemuu-win7-amd64 fail test-amd64-amd64-qemuu-nested-intel fail test-amd64-i386-qemuu-rhel6hvm-intel pass test-amd64-i386-xl-qemuu-winxpsp3-vcpus1 pass test-amd64-amd64-xl-qemuu-winxpsp3 pass test-amd64-i386-xl-qemuu-winxpsp3pass sg-report-flight on osstest.xs.citrite.net logs: /home/osstest/logs images: /home/osstest/images Logs, config files, etc. are available at http://osstest.xs.citrite.net/~osstest/testlogs/logs Test harness code can be found at http://xenbits.xensource.com/gitweb?p=osstest.git;a=summary Push not applicable. commit dce99e01b6bfc51175bdf32612fd4f2738e5c3c8 Author: Marcel Apfelbaum Date: Tue Mar 1 16:06:45 2016 +0200 fw/pci: add Q35 S3 support Following the i440fx example, save the LPC, SMBUS and PCIEXBAR bdfs between OS sleeps and use them to re-configure the corresponding registers. Tested-by: Gal Hammer Reviewed-by: Laszlo Ersek Signed-off-by: Marcel Apfelbaum ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v6 for Xen 4.7 1/4] xen: enable per-VCPU parameter settings for RTDS scheduler
On Tue, 2016-03-08 at 02:10 -0700, Jan Beulich wrote: > > > > On 07.03.16 at 18:53, wrote: > > On Mon, 2016-03-07 at 09:40 -0700, Jan Beulich wrote: > > > > > IIRC, I was looking at how XEN_SYSCTL_pcitopoinfo is handled, for > > reference, and that has some guest_handle_is_null()==>EINVAL > > sainity > > checking (in xen/common/sysctl.c), which, when I thought about it, > > made > > sense to me. > > > > My reasoning was, sort of: > > 1. if the handle is NULL, no point getting into the somewhat > > complicated logic of the while, > > 2. more accurate error reporting: as being passed a NULL handler > > looked something we could identify and call invalid, rather > > than > > waiting for the copy to fault. > I think the XEN_SYSCTL_pcitopoinfo was misguided in this respect, > cloning non applicable logic here which returns the number of needed > (array) elements in such a case for a few other operations. > Sorry, I'm not sure I am getting: are you saying that, for _these_ domctls, we should consider the handle being NULL as a way of the caller to ask for the size of the array? *If* yes, well, that is "just" the number of vcpus of the guest, but, nevertheless, that, FWIW, looks fine to me. > > About the structure of the code, as said above, I do like > > how XEN_SYSCTL_pcitopoinfo ended up being handled, I think it is a > > great fit for this specific case and, comparing at both this and > > previous version, I do think this one is (bugs apart) looking > > better. > > > > I'm sure I said this --long ago-- when discussing v4 (and maybe > > even > > previous versions), as well as more recently, when reviewing v5, > > and > > that's why Chong (finally! :-D) did it. > > > > So, with the comment in place (and with bugs fixed :-)), are you > > (Jan) > > ok with this being done this way? > > Well, this _might_ be acceptable for "get" (since the caller > abandoning the sequence of calls prematurely is no problem), > but for "set" it looks less suitable, as similar abandoning would > leave the guest in some inconsistent / unintended state. > Are you referring to the fact that, with this interface, the caller has the chance to leave intentionally, or that it may happen that not all vcpus are updated because of some bug (still in the caller)? Well, if it's intentional, or even if the caller is buggy in the sense that the code is written in a way that it misses updating some vcpus (and if the interface and the behavior is well documented, as you request), then one gets what he "wants" (and, in the latter case, it wouldn't be too hard to debug and figure out the issue, I think). If it's for bugs (still in the caller) like copy_from_guest_offset() faulting because the array is too small, that can happen if using continuation too, can't it? And it would still leave the guest in similar inconsistent or unintended state, IMO... One last point. Of course, since we are talking about bugs, the final status is not the one desired, but it's not inconsistent in the sense that the guest can't continue running, or crashes, or anything like that. It's something like: - you wants all the 32 vcpus of guest A to have these new parameters - due to a bug, you're (for instance) passing me an array with only 16 vcpus parameters - result: onlyt 16 vcpus will have the new parameters. > The > issue with XEN_SYSCTL_pcitopoinfo was, iirc, the lack of a > good way of encoding the continuation information, and while > that would seem applicable here too I'm not sure now whether > doing it the way it was done was the best choice. > As far as I can remember and see, it was being done by means of an additional dedicated parameter in the handle (called ti->first_dev in that case). Then at some point, you said: http://lists.xenproject.org/archives/html/xen-devel/2015-03/msg02623.html "Considering this is a tools only interface, enforcing a not too high limit on num_devs would seem better than this not really clean continuation mechanism. The (tool stack) caller(s) can be made iterate." With which I did agree (and I still do :-)), as well as I agree on the fact that we basically are in the same situation here. Chong tried doing things with continuations for a few rounds, including v5, which is here: http://lists.xenproject.org/archives/html/xen-devel/2016-02/msg00817.html and he also used an additional field (vcpu_index). So, all this being said, my preference stays for the way the code looks like in this version (with all the due commenting added). Of course, it's your preference that really matters here, me not being the maintainer of this code. :-) So, how do you prefer Chong to continue doing this? > Clearly > stating (in the public interface header) that certain normally > input-only fields are volatile would allow the continuation to > be handled without tool stack assistance afaict. > Which (sorry, I'm not getting again) I guess is something different/more than what was done in v5 (the relevant hunks of
Re: [Xen-devel] [PATCH] libxc: move migration_stream's definition to xenguest.h
On 08/03/16 05:32, Wen Congyang wrote:
> xc_save_domain()'s parameter use this type, so it should
> be public.
xc_domain_save() currently uses an int, which also needs fixing.
>
> Signed-off-by: Wen Congyang
Does this even compile? You have removed a variable without any
replacement.
> ---
> tools/libxc/include/xenguest.h | 10 ++
> tools/libxc/xc_sr_common.h | 10 --
> 2 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/tools/libxc/include/xenguest.h b/tools/libxc/include/xenguest.h
> index affc42b..888536e 100644
> --- a/tools/libxc/include/xenguest.h
> +++ b/tools/libxc/include/xenguest.h
> @@ -238,4 +238,9 @@ xen_pfn_t *xc_map_m2p(xc_interface *xch,
>unsigned long max_mfn,
>int prot,
>unsigned long *mfn0);
> +
> +typedef enum {
> +MIG_STREAM_NONE, /* plain stream */
> +MIG_STREAM_REMUS,
> +} migration_stream;
This typedef should be beside xc_domain_save() as that is where it is
intended to be used. It also needs xc_ prefixes as it is part of the
public interface, and a typedef wants a _t suffix.
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] libxc: move migration_stream's definition to xenguest.h
On 03/08/2016 06:38 PM, Andrew Cooper wrote:
> On 08/03/16 05:32, Wen Congyang wrote:
>> xc_save_domain()'s parameter use this type, so it should
>> be public.
>
> xc_domain_save() currently uses an int, which also needs fixing.
OK. Will fix it in the next version.
>
>>
>> Signed-off-by: Wen Congyang
>
> Does this even compile? You have removed a variable without any
> replacement.
Yes, I compile it. The variable is not used.
>
>> ---
>> tools/libxc/include/xenguest.h | 10 ++
>> tools/libxc/xc_sr_common.h | 10 --
>> 2 files changed, 10 insertions(+), 10 deletions(-)
>>
>> diff --git a/tools/libxc/include/xenguest.h b/tools/libxc/include/xenguest.h
>> index affc42b..888536e 100644
>> --- a/tools/libxc/include/xenguest.h
>> +++ b/tools/libxc/include/xenguest.h
>> @@ -238,4 +238,9 @@ xen_pfn_t *xc_map_m2p(xc_interface *xch,
>>unsigned long max_mfn,
>>int prot,
>>unsigned long *mfn0);
>> +
>> +typedef enum {
>> +MIG_STREAM_NONE, /* plain stream */
>> +MIG_STREAM_REMUS,
>> +} migration_stream;
>
> This typedef should be beside xc_domain_save() as that is where it is
> intended to be used. It also needs xc_ prefixes as it is part of the
> public interface, and a typedef wants a _t suffix.
OK. Will fix it in the next version.
Thanks
Wen Congyang
>
> ~Andrew
>
>
> .
>
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 1/6] xen, cpupool: correct error handling when removing cpu from cpupool
On Thu, 2016-03-03 at 17:48 +0100, Juergen Gross wrote: > When schedule_cpu_switch() called from cpupool_unassign_cpu_helper() > returns an error, the domlist_read_lock isn't released again. > > As cpu_disable_scheduler() might have changed affinity of some > domains domain_update_node_affinity() must be called for all domains > in the cpupool even in error case. > > Even if looking weird it is okay to let the to be removed cpu set in > cpupool_free_cpus in case of an error returned by > cpu_disable_scheduler(). Add a comment explaining the reason for > this. > > Cc: Dario Faggioli > Cc: Jan Beulich > Signed-off-by: Juergen Gross > Acked-by: Dario Faggioli 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 v3] xen/mm: Fix page_list_* helpers to evaluate all their arguments
On 08/03/16 09:57, Jan Beulich wrote: On 07.03.16 at 19:12, wrote: >> --- a/xen/include/asm-arm/domain.h >> +++ b/xen/include/asm-arm/domain.h >> @@ -65,6 +65,7 @@ struct arch_domain >> RELMEM_mapping, >> RELMEM_done, >> } relmem; >> +struct page_list_head relmem_list; > Well, if I was ARM maintainer I would say no to this otherwise pointless > addition (even more so that this list doesn't get initialized anywhere). > The expectation I had for how the build issue would be fixed was to > simply not convert (at least) page_list_del2() to an inline function. No. Discarding parameters is what got us into the first mess. I will not propagate the problem. It is a bug that ARM relied on the discarded parameters to compile. Ultimately, the bug is that common/page_alloc.c references d->arch. More generally, the problem is that common/page_alloc.c has x86 specifics in it. The two options are to make relmem_list common, or to remove x86 specifics from common by introduce arch_free_domheap_page() helpers which maintain relmem_list on x86. On second thoughts, this latter option seems to be better, as it would also allow the removal of page_list_del2, although it is not clear if this is safe to do. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [PATCH 0/2] Make the pcidevs_lock a recursive one
This patch set makes the pcidevs_lock a recursive one. It is a prereq
patch set for Patch:'VT-d Device-TLB flush issue', as the pcidevs_lock
may be recursively held for hiding the ATS device, when IOMMU Device-TLB
flush timed out.
In detail:
1. Fix a bug found in AMD IOMMU initialization.
Doing what we do serves as a fix for a bug found in AMD IOMMU initialization.
The current code is using spin_lock{_irqsave(), _irqrestore()} to
protect pci_get_dev() in the set_iommu_interrupt_handler(). However,
this can only be called during AMD IOMMU initialization, with interrupt
enabled, so at least it is not necessary to disable interrupts, or
save/restore interrupt flag.
In order to fix this, we can use just plain spin{_lock(),_unlock()},
instead of spin_lock{_irqsave(),_irqrestore()}.
2. Make the pcidevs_lock a recursive one.
CC: Keir Fraser
CC: Jan Beulich
CC: Andrew Cooper
CC: Suravee Suthikulpanit
CC: Aravind Gopalakrishnan
CC: Feng Wu
CC: Kevin Tian
CC: Dario Faggioli
Quan Xu (2):
IOMMU/spinlock: Fix a bug found in AMD IOMMU initialization.
IOMMU/spinlock: Make the pcidevs_lock a recursive one
xen/arch/x86/domctl.c | 8 +--
xen/arch/x86/hvm/vmsi.c | 4 +-
xen/arch/x86/irq.c | 8 +--
xen/arch/x86/msi.c | 16 ++---
xen/arch/x86/pci.c | 4 +-
xen/arch/x86/physdev.c | 16 ++---
xen/common/sysctl.c | 4 +-
xen/drivers/passthrough/amd/iommu_init.c| 9 ++-
xen/drivers/passthrough/amd/iommu_map.c | 2 +-
xen/drivers/passthrough/amd/pci_amd_iommu.c | 4 +-
xen/drivers/passthrough/pci.c | 96 +
xen/drivers/passthrough/vtd/iommu.c | 14 ++---
xen/drivers/video/vga.c | 4 +-
xen/include/xen/pci.h | 5 +-
14 files changed, 108 insertions(+), 86 deletions(-)
--
1.9.1
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 13/16] hvmloader: Load ACPI tables from hvm_start_info module
On Fri, Mar 04, 2016 at 01:39:38AM -0700, Jan Beulich wrote:
> >>> On 03.03.16 at 18:59, wrote:
> > On Tue, Mar 01, 2016 at 09:17:25AM -0700, Jan Beulich wrote:
> >> >>> On 25.02.16 at 15:56, wrote:
> >> > --- a/tools/firmware/hvmloader/hvmloader.c
> >> > +++ b/tools/firmware/hvmloader/hvmloader.c
> >> > @@ -365,8 +365,26 @@ int main(void)
> >> >
> >> > if ( bios->acpi_build_tables )
> >> > {
> >> > +const struct hvm_modlist_entry *acpi_module;
> >> > +acpi_module = get_module_entry(hvm_start_info, "acpi");
> >> > printf("Loading ACPI ...\n");
> >> > -bios->acpi_build_tables();
> >> > +if ( acpi_module )
> >> > +{
> >> > +uint32_t paddr = acpi_module->paddr;
> >> > +bios->acpi_build_tables((void*)paddr,
> >> > +acpi_module->size);
> >> > +}
> >>
> >> Hmm, so far it was the build process which ensured the right ACPI
> >> tables would be used with the corresponding BIOS. The disconnect
> >> that gets introduced here worries me a little, since things having
> >> got out of sync may be rather hard to diagnose (as they may
> >> surface only much later).
> >
> > So, my ultimate goal with this series was to be able to create a guest with
> > QEMU's Q35 machine, which would need a different ACPI tables.
> >
> > Also, I would say that the ACPI tables are already disconnected from the
> > thing they describe, the device model QEMU. I don't think there is much
> > information about the BIOS backed into the DSDT table.
>
> But then why would the Q35 model need a different one? Or the
> other way around, why wouldn't that other one be usable with
> with the current machine type (or more generally, why couldn't
> we have one that fits all machine types we mean to support)?
I have not though about using the same one for both. I could try to change
the tables we have to make it work with both. But that for another time.
Thanks,
--
Anthony PERARD
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[Xen-devel] [PATCH 1/2] IOMMU/spinlock: Fix a bug found in AMD IOMMU initialization.
Doing what we do serves as a fix for a bug found in AMD IOMMU initialization.
The current code is using spin_lock{_irqsave(), _irqrestore()} to
protect pci_get_dev() in the set_iommu_interrupt_handler(). However,
this can only be called during AMD IOMMU initialization, with interrupt
enabled, so at least it is not necessary to disable interrupts, or
save/restore interrupt flag.
In order to fix this, we can use just plain spin{_lock(),_unlock()},
instead of spin_lock{_irqsave(),_irqrestore()}.
Signed-off-by: Quan Xu
CC: Suravee Suthikulpanit
CC: Aravind Gopalakrishnan
CC: Dario Faggioli
CC: Jan Beulich
---
xen/drivers/passthrough/amd/iommu_init.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/xen/drivers/passthrough/amd/iommu_init.c
b/xen/drivers/passthrough/amd/iommu_init.c
index d90a2d2..a400497 100644
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -778,7 +778,6 @@ static bool_t __init set_iommu_interrupt_handler(struct
amd_iommu *iommu)
{
int irq, ret;
hw_irq_controller *handler;
-unsigned long flags;
u16 control;
irq = create_irq(NUMA_NO_NODE);
@@ -788,10 +787,10 @@ static bool_t __init set_iommu_interrupt_handler(struct
amd_iommu *iommu)
return 0;
}
-spin_lock_irqsave(&pcidevs_lock, flags);
+spin_lock(&pcidevs_lock);
iommu->msi.dev = pci_get_pdev(iommu->seg, PCI_BUS(iommu->bdf),
PCI_DEVFN2(iommu->bdf));
-spin_unlock_irqrestore(&pcidevs_lock, flags);
+spin_unlock(&pcidevs_lock);
if ( !iommu->msi.dev )
{
AMD_IOMMU_DEBUG("IOMMU: no pdev for %04x:%02x:%02x.%u\n",
--
1.9.1
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[Xen-devel] [PATCH 2/2] IOMMU/spinlock: Make the pcidevs_lock a recursive one
Signed-off-by: Quan Xu
CC: Keir Fraser
CC: Jan Beulich
CC: Andrew Cooper
CC: Suravee Suthikulpanit
CC: Aravind Gopalakrishnan
CC: Feng Wu
CC: Kevin Tian
CC: Dario Faggioli
---
xen/arch/x86/domctl.c | 8 +--
xen/arch/x86/hvm/vmsi.c | 4 +-
xen/arch/x86/irq.c | 8 +--
xen/arch/x86/msi.c | 16 ++---
xen/arch/x86/pci.c | 4 +-
xen/arch/x86/physdev.c | 16 ++---
xen/common/sysctl.c | 4 +-
xen/drivers/passthrough/amd/iommu_init.c| 8 +--
xen/drivers/passthrough/amd/iommu_map.c | 2 +-
xen/drivers/passthrough/amd/pci_amd_iommu.c | 4 +-
xen/drivers/passthrough/pci.c | 96 +
xen/drivers/passthrough/vtd/iommu.c | 14 ++---
xen/drivers/video/vga.c | 4 +-
xen/include/xen/pci.h | 5 +-
14 files changed, 108 insertions(+), 85 deletions(-)
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index bf62a88..21cc161 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -427,9 +427,9 @@ long arch_do_domctl(
ret = -ESRCH;
if ( iommu_enabled )
{
-spin_lock(&pcidevs_lock);
+pcidevs_lock();
ret = pt_irq_create_bind(d, bind);
-spin_unlock(&pcidevs_lock);
+pcidevs_unlock();
}
if ( ret < 0 )
printk(XENLOG_G_ERR "pt_irq_create_bind failed (%ld) for dom%d\n",
@@ -452,9 +452,9 @@ long arch_do_domctl(
if ( iommu_enabled )
{
-spin_lock(&pcidevs_lock);
+pcidevs_lock();
ret = pt_irq_destroy_bind(d, bind);
-spin_unlock(&pcidevs_lock);
+pcidevs_unlock();
}
if ( ret < 0 )
printk(XENLOG_G_ERR "pt_irq_destroy_bind failed (%ld) for dom%d\n",
diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c
index ac838a9..8e0817b 100644
--- a/xen/arch/x86/hvm/vmsi.c
+++ b/xen/arch/x86/hvm/vmsi.c
@@ -388,7 +388,7 @@ int msixtbl_pt_register(struct domain *d, struct pirq
*pirq, uint64_t gtable)
struct msixtbl_entry *entry, *new_entry;
int r = -EINVAL;
-ASSERT(spin_is_locked(&pcidevs_lock));
+ASSERT(pcidevs_is_locked());
ASSERT(spin_is_locked(&d->event_lock));
/*
@@ -443,7 +443,7 @@ void msixtbl_pt_unregister(struct domain *d, struct pirq
*pirq)
struct pci_dev *pdev;
struct msixtbl_entry *entry;
-ASSERT(spin_is_locked(&pcidevs_lock));
+ASSERT(pcidevs_is_locked());
ASSERT(spin_is_locked(&d->event_lock));
irq_desc = pirq_spin_lock_irq_desc(pirq, NULL);
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index bf2e822..68bdf19 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -1955,7 +1955,7 @@ int map_domain_pirq(
struct pci_dev *pdev;
unsigned int nr = 0;
-ASSERT(spin_is_locked(&pcidevs_lock));
+ASSERT(pcidevs_is_locked());
ret = -ENODEV;
if ( !cpu_has_apic )
@@ -2100,7 +2100,7 @@ int unmap_domain_pirq(struct domain *d, int pirq)
if ( (pirq < 0) || (pirq >= d->nr_pirqs) )
return -EINVAL;
-ASSERT(spin_is_locked(&pcidevs_lock));
+ASSERT(pcidevs_is_locked());
ASSERT(spin_is_locked(&d->event_lock));
info = pirq_info(d, pirq);
@@ -2226,7 +2226,7 @@ void free_domain_pirqs(struct domain *d)
{
int i;
-spin_lock(&pcidevs_lock);
+pcidevs_lock();
spin_lock(&d->event_lock);
for ( i = 0; i < d->nr_pirqs; i++ )
@@ -2234,7 +2234,7 @@ void free_domain_pirqs(struct domain *d)
unmap_domain_pirq(d, i);
spin_unlock(&d->event_lock);
-spin_unlock(&pcidevs_lock);
+pcidevs_unlock();
}
static void dump_irqs(unsigned char key)
diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
index 3dbb84d..6e5e33e 100644
--- a/xen/arch/x86/msi.c
+++ b/xen/arch/x86/msi.c
@@ -694,7 +694,7 @@ static int msi_capability_init(struct pci_dev *dev,
u8 slot = PCI_SLOT(dev->devfn);
u8 func = PCI_FUNC(dev->devfn);
-ASSERT(spin_is_locked(&pcidevs_lock));
+ASSERT(pcidevs_is_locked());
pos = pci_find_cap_offset(seg, bus, slot, func, PCI_CAP_ID_MSI);
if ( !pos )
return -ENODEV;
@@ -852,7 +852,7 @@ static int msix_capability_init(struct pci_dev *dev,
u8 func = PCI_FUNC(dev->devfn);
bool_t maskall = msix->host_maskall;
-ASSERT(spin_is_locked(&pcidevs_lock));
+ASSERT(pcidevs_is_locked());
control = pci_conf_read16(seg, bus, slot, func, msix_control_reg(pos));
/*
@@ -1042,7 +1042,7 @@ static int __pci_enable_msi(struct msi_info *msi, struct
msi_desc **desc)
struct pci_dev *pdev;
struct msi_desc *old_desc;
-ASSERT(spin_is_locked(&pcidevs_lock));
+ASSERT(pcidevs_is_locked());
pdev = pci_get_pdev(msi->seg, msi->bus, msi->devfn);
if ( !pdev )
return -ENOD
Re: [Xen-devel] [PATCH] tools/foreign: Avoid using alignment directives when not appropriate
On 08/03/16 09:54, Jan Beulich wrote: On 07.03.16 at 19:28, wrote: >> --- a/tools/include/xen-foreign/Makefile >> +++ b/tools/include/xen-foreign/Makefile >> @@ -35,6 +35,8 @@ x86_32.h: mkheader.py structs.py >> $(ROOT)/arch-x86/xen-x86_32.h $(ROOT)/arch-x86/ >> >> x86_64.h: mkheader.py structs.py $(ROOT)/arch-x86/xen-x86_64.h >> $(ROOT)/arch-x86/xen.h $(ROOT)/xen.h >> $(PYTHON) $< $* $@ $(filter %.h,$^) >> +#Avoid mixing an alignment directive with a uint64_t cast or sizeof >> expression >> +sed 's/(__align8__ uint64_t)/(uint64_t)/g' -i $@ > A two step rule like this should make use of a temporary file, to > avoid breakage when the build process gets interrupted between > the two steps. > > And then - is it perhaps worth to generalize the pattern in one or > more of a couple of possible ways? Considering int64_t uses > would perhaps be the most relevant one (even if not needed > right away). But of course this could get as generic as > > s/(__align[0-9]*__ \([a-z0-9_]*\))/(\1)/g > > without - afaict (based on your commit description) - breaking > anything. Both of these would want to be + rather than * to ensure some content. While generic is usually better, in this case I think it is better to stick with the most specific fix, to reduce the risk of accidentally clobbering a real __align__. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 00/16] Load BIOS via toolstack instead of been embedded in hvmloader.
On Fri, Mar 04, 2016 at 10:57:59AM +, Andrew Cooper wrote: > On 03/03/16 18:03, Anthony PERARD wrote: > > In this series, there are plenty of places where one blob loaded by libxl > > to be consume by hvmloader is called acpi_module or acpi_table... where in > > fact it is only the DSDT table. I think I'm going to do some renaming to > > include "dsdt" into those names. > > The DSDT cannot possibly come from anywhere other than hvmloader (as a > logic extension of Xen). It is very hardware specific, including bits > of ACPI emulated by Xen itself. > > There are plenty of improvements which can be made over the current > status quo by splitting out the optional parts into extra SSDTs, but > having the DSDT itself come from another source will cause all kinds of > problems for the domain. Ok. I think I leave the acpi tables alone in the next revision of the series and only load the BIOS. -- Anthony PERARD ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3] xen/mm: Fix page_list_* helpers to evaluate all their arguments
>>> On 08.03.16 at 12:05, wrote: > The two options are to make relmem_list common, or to remove x86 > specifics from common by introduce arch_free_domheap_page() helpers > which maintain relmem_list on x86. On second thoughts, this latter > option seems to be better, as it would also allow the removal of > page_list_del2, although it is not clear if this is safe to do. Introducing arch_free_domheap_page() would definitely be fine with me. Removing page_list_del2() altogether, otoh, would not be safe. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] tools/foreign: Avoid using alignment directives when not appropriate
>>> On 08.03.16 at 12:19, wrote: > On 08/03/16 09:54, Jan Beulich wrote: > On 07.03.16 at 19:28, wrote: >>> --- a/tools/include/xen-foreign/Makefile >>> +++ b/tools/include/xen-foreign/Makefile >>> @@ -35,6 +35,8 @@ x86_32.h: mkheader.py structs.py >>> $(ROOT)/arch-x86/xen-x86_32.h $(ROOT)/arch-x86/ >>> >>> x86_64.h: mkheader.py structs.py $(ROOT)/arch-x86/xen-x86_64.h >>> $(ROOT)/arch-x86/xen.h $(ROOT)/xen.h >>> $(PYTHON) $< $* $@ $(filter %.h,$^) >>> + #Avoid mixing an alignment directive with a uint64_t cast or sizeof > expression >>> + sed 's/(__align8__ uint64_t)/(uint64_t)/g' -i $@ >> A two step rule like this should make use of a temporary file, to >> avoid breakage when the build process gets interrupted between >> the two steps. >> >> And then - is it perhaps worth to generalize the pattern in one or >> more of a couple of possible ways? Considering int64_t uses >> would perhaps be the most relevant one (even if not needed >> right away). But of course this could get as generic as >> >> s/(__align[0-9]*__ \([a-z0-9_]*\))/(\1)/g >> >> without - afaict (based on your commit description) - breaking >> anything. > > Both of these would want to be + rather than * to ensure some content. True. I had just avoided them because they would also have needed escaping. > While generic is usually better, in this case I think it is better to > stick with the most specific fix, to reduce the risk of accidentally > clobbering a real __align__. Well, as your commit description alludes to, there are no syntactically correct uses of just an attribute and a type in the context of other than sizeof(), typeof(), or a cast. Hence I wouldn't view the generalization as potentially problematic, but otoh I can understand you trying to be conservative. Hence the minimal suggestion of at least also dealing with int64_t. But in the end it's the tools maintainers' call anyway. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [RESEND][PATCH V16 0/6] xen pvusb toolstack work
On 08/03/16 01:37, Chunyan Liu wrote: > This patch series is to add pvusb toolstack work, supporting hot add|remove > USB device to|from guest and specify USB device in domain configuration file. > > RESEND to remove a incorrect rc in 4/6: > +out: > +path = GCSPRINTF(USBBACK_INFO_PATH "/%s", usbdev_encode); > +rc = libxl__xs_rm_checked(gc, XBT_NULL, path); > 'rc' should be removed here. > +return rc; > > Sorry for trouble you. Hey Chunyan, If you make any change at all to the patch series, you should increase the revision number. Otherwise, the person who ultimately commits it is likely to think that the original version and the resend are the same, and accidentally commit the original (incorrect) version. (For instance, they may already like me have downloaded the original patch series and imported it into git.) The best thing to do in this situation would have been to reply to your own patch, saying "And this 'rc' should be removed. I'll spin another version." I took a brief look at the diff between v14 and v15v1 yesterday, and it looks good. IanJ is away this week, and I'm sure he'll want to take a look at it before Ack-ing it next week. So would you mind sending a v16, and I'll review it by the end of the week? Thanks, -George > > Changes to V15: > * address George's comments (patch 4/6) > > V15: > http://lists.xen.org/archives/html/xen-devel/2016-03/msg00040.html > > V14: > http://lists.xenproject.org/archives/html/xen-devel/2016-02/msg02745.html > > V13: > http://lists.xenproject.org/archives/html/xen-devel/2016-01/msg02125.html > > V12: > http://lists.xen.org/archives/html/xen-devel/2015-12/msg02697.html > > V11: > http://lists.xen.org/archives/html/xen-devel/2015-12/msg01626.html > > V10: > http://lists.xen.org/archives/html/xen-devel/2015-12/msg01172.html > > V9: > http://lists.xen.org/archives/html/xen-devel/2015-11/msg02744.html > > V8: > http://lists.xen.org/archives/html/xen-devel/2015-10/msg02178.html > > V7: > http://lists.xen.org/archives/html/xen-devel/2015-09/msg03115.html > > V6: > http://lists.xen.org/archives/html/xen-devel/2015-08/msg00750.html > > V5: > http://lists.xen.org/archives/html/xen-devel/2015-06/msg04052.html > > V4: > http://lists.xenproject.org/archives/html/xen-devel/2015-06/msg01327.html > > Related Discussion Threads: > http://www.redhat.com/archives/libvir-list/2014-June/msg00038.html > http://lists.xen.org/archives/html/xen-devel/2014-06/msg00086.html > > <<< pvusb work introduction >>> > > 1. Overview > > There are two general methods for passing through individual host > devices to a guest. The first is via an emulated USB device > controller; the second is PVUSB. > > Additionally, there are two ways to add USB devices to a guest: via > the config file at domain creation time, and via hot-plug while the VM > is running. > > * Emulated USB > > In emulated USB, the device model (qemu) presents an emulated USB > controller to the guest. The device model process then grabs control > of the device from domain 0 and and passes the USB commands between > the guest OS and the host USB device. > > This method is only available to HVM domains, and is not available for > domains running with device model stubdomains. > > * PVUSB > > PVUSB uses a paravirtialized front-end/back-end interface, similar to > the traditional Xen PV network and disk protocols. In order to use > PVUSB, you need usbfront in your guest OS, and usbback in dom0 (or > your USB driver domain). > > 2. Specifying a host USB device > > QEMU qmp commands allows USB devices to be specified either by their > bus address (in the form bus.device) or their device tag (in the form > vendorid:deviceid). > > Each way of specifying has its advantages: > > Specifying by device tag will always get the same device, > regardless of where the device ends up in the USB bus topology. > However, if there are two identical devices, it will not allow you to > specify which one. > > Specifying by bus address will always allow you to choose a > specific device, even if you have duplicates. However, the bus address > may change depending on which port you plugged the device into, and > possibly also after a reboot. > > To avoid duplication of vendorid:deviceid, we'll use bus address to > specify host USB device in xl toolstack. > > You can use lsusb to list the USB devices on the system: > > Bus 001 Device 003: ID 0424:2514 Standard Microsystems Corp. USB 2.0 > Hub > Bus 003 Device 002: ID f617:0905 > Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub > Bus 001 Device 004: ID 0424:2640 Standard Microsystems Corp. USB 2.0 > Hub > Bus 001 Device 005: ID 0424:4060 Standard Microsystems Corp. Ultra > Fast Media Reader > Bus 001 Device 006: ID 046d:c016 Logitech, Inc. Optical Wheel Mouse > > To pass through the Logitec mouse, for instance, you could specify > 1.6 (remove leading zeroes). > > Note: USB hubs can not be assigned to guest. > > 3. P
Re: [Xen-devel] [PATCH v3 2/6] xen: add hypercall option to override and restore vcpu affinity
On Thu, 2016-03-03 at 17:48 +0100, Juergen Gross wrote:
> Some hardware (e.g. Dell studio 1555 laptops) require SMIs to be
> called on physical cpu 0 only. Linux drivers like dcdbas or i8k try
> to achieve this by pinning the running thread to cpu 0, but in Dom0
> this is not enough: the vcpu must be pinned to physical cpu 0 via
> Xen, too.
>
> Add a stable hypercall option SCHEDOP_pin_override to the sched_op
> hypercall to achieve this. It is taking a physical cpu number as
> parameter. If pinning is possible (the calling domain has the
> privilege to make the call and the cpu is available in the domain's
> cpupool) the calling vcpu is pinned to the specified cpu.
>
I would have added the "and the cpu is available in the domain's
cpupool" part in the comment in public headers too, such as:
> --- a/xen/include/public/sched.h
> +++ b/xen/include/public/sched.h
> @@ -118,6 +118,17 @@
> * With id != 0 and timeout != 0, poke watchdog timer and set new
> timeout.
> */
> #define SCHEDOP_watchdog6
> +
> +/*
> + * Override the current vcpu affinity by pinning it to one physical
> cpu or undo
> + * this override restoring the previous affinity.
> + * @arg == pointer to sched_pin_override_t structure.
> + *
> + * A negative pcpu value will undo a previous pin override and
> restore the
> + * previous cpu affinity.
> + * This call is allowed for the hardware domain only.
", and succeeds only if the specified cpu is available in the domain's
cpupool."
> + */
> +#define SCHEDOP_pin_override 7
> /* ` } */
>
> struct sched_shutdown {
>
In any case, the scheduling part is:
Acked-by: Dario Faggioli
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 v6 for Xen 4.7 1/4] xen: enable per-VCPU parameter settings for RTDS scheduler
>>> On 08.03.16 at 11:34, wrote: > On Tue, 2016-03-08 at 02:10 -0700, Jan Beulich wrote: >> > > > On 07.03.16 at 18:53, wrote: >> > On Mon, 2016-03-07 at 09:40 -0700, Jan Beulich wrote: >> > > >> > IIRC, I was looking at how XEN_SYSCTL_pcitopoinfo is handled, for >> > reference, and that has some guest_handle_is_null()==>EINVAL >> > sainity >> > checking (in xen/common/sysctl.c), which, when I thought about it, >> > made >> > sense to me. >> > >> > My reasoning was, sort of: >> > 1. if the handle is NULL, no point getting into the somewhat >> > complicated logic of the while, >> > 2. more accurate error reporting: as being passed a NULL handler >> > looked something we could identify and call invalid, rather >> > than >> > waiting for the copy to fault. >> I think the XEN_SYSCTL_pcitopoinfo was misguided in this respect, >> cloning non applicable logic here which returns the number of needed >> (array) elements in such a case for a few other operations. >> > Sorry, I'm not sure I am getting: are you saying that, for _these_ > domctls, we should consider the handle being NULL as a way of the > caller to ask for the size of the array? No, I've tried to point out that _when_ such behavior is intended, the special casing of a null handle is warranted. But not (normally) in other cases. >> > About the structure of the code, as said above, I do like >> > how XEN_SYSCTL_pcitopoinfo ended up being handled, I think it is a >> > great fit for this specific case and, comparing at both this and >> > previous version, I do think this one is (bugs apart) looking >> > better. >> > >> > I'm sure I said this --long ago-- when discussing v4 (and maybe >> > even >> > previous versions), as well as more recently, when reviewing v5, >> > and >> > that's why Chong (finally! :-D) did it. >> > >> > So, with the comment in place (and with bugs fixed :-)), are you >> > (Jan) >> > ok with this being done this way? >> >> Well, this _might_ be acceptable for "get" (since the caller >> abandoning the sequence of calls prematurely is no problem), >> but for "set" it looks less suitable, as similar abandoning would >> leave the guest in some inconsistent / unintended state. >> > Are you referring to the fact that, with this interface, the caller has > the chance to leave intentionally, or that it may happen that not all > vcpus are updated because of some bug (still in the caller)? > > Well, if it's intentional, or even if the caller is buggy in the sense > that the code is written in a way that it misses updating some vcpus > (and if the interface and the behavior is well documented, as you > request), then one gets what he "wants" (and, in the latter case, it > wouldn't be too hard to debug and figure out the issue, I think). Intentional or buggy doesn't matter much - if we can avoid making ourselves dependent upon user mode code behaving well, I think we should. > If it's for bugs (still in the caller) like copy_from_guest_offset() > faulting because the array is too small, that can happen if using > continuation too, can't it? And it would still leave the guest in > similar inconsistent or unintended state, IMO... True, albeit that's what error indications are for. > One last point. Of course, since we are talking about bugs, the final > status is not the one desired, but it's not inconsistent in the sense > that the guest can't continue running, or crashes, or anything like > that. It's something like: > - you wants all the 32 vcpus of guest A to have these new parameters > - due to a bug, you're (for instance) passing me an array with only >16 vcpus parameters > - result: onlyt 16 vcpus will have the new parameters. That was my understanding, yes. >> The >> issue with XEN_SYSCTL_pcitopoinfo was, iirc, the lack of a >> good way of encoding the continuation information, and while >> that would seem applicable here too I'm not sure now whether >> doing it the way it was done was the best choice. >> > As far as I can remember and see, it was being done by means of an > additional dedicated parameter in the handle (called ti->first_dev in > that case). Then at some point, you said: > > http://lists.xenproject.org/archives/html/xen-devel/2015-03/msg02623.html > "Considering this is a tools only interface, enforcing a not too high > limit on num_devs would seem better than this not really clean > continuation mechanism. The (tool stack) caller(s) can be made > iterate." > > With which I did agree (and I still do :-)), as well as I agree on the > fact that we basically are in the same situation here. > > Chong tried doing things with continuations for a few rounds, including > v5, which is here: > http://lists.xenproject.org/archives/html/xen-devel/2016-02/msg00817.html > > and he also used an additional field (vcpu_index). > > So, all this being said, my preference stays for the way the code looks > like in this version (with all the due commenting added). Of course, > it's your prefere
Re: [Xen-devel] Ideas Re: [PATCH v14 1/2] vmx: VT-d posted-interrupt core logic handling
On 07/03/16 15:53, Konrad Rzeszutek Wilk wrote:
> On Mon, Mar 07, 2016 at 11:21:33AM +, George Dunlap wrote:
>> On Fri, Mar 4, 2016 at 10:00 PM, Konrad Rzeszutek Wilk
>> wrote:
+/* Handle VT-d posted-interrupt when VCPU is blocked. */
+static void pi_wakeup_interrupt(struct cpu_user_regs *regs)
+{
+struct arch_vmx_struct *vmx, *tmp;
+spinlock_t *lock = &per_cpu(vmx_pi_blocking, smp_processor_id()).lock;
+struct list_head *blocked_vcpus =
+ &per_cpu(vmx_pi_blocking, smp_processor_id()).list;
+
+ack_APIC_irq();
+this_cpu(irq_count)++;
+
+spin_lock(lock);
+
+/*
+ * XXX: The length of the list depends on how many vCPU is current
+ * blocked on this specific pCPU. This may hurt the interrupt latency
+ * if the list grows to too many entries.
+ */
+list_for_each_entry_safe(vmx, tmp, blocked_vcpus, pi_blocking.list)
+{
>>>
>>>
>>> My recollection of the 'most-horrible' case of this being really bad is when
>>> the scheduler puts the vCPU0 and VCPU1 of the guest on the same pCPU (as an
>>> example)
>>> and they round-robin all the time.
>>>
>>>
>>> Would it be perhaps possible to have an anti-affinity flag to deter the
>>> scheduler from this? That is whichever struct vcpu has 'anti-affinity' flag
>>> set - the scheduler will try as much as it can _to not_ schedule the
>>> 'struct vcpu'
>>> if the previous 'struct vcpu' had this flag as well on this pCPU?
>>
>> Well having vcpus from the same guest on the same pcpu is problematic
>> for a number of reasons -- spinlocks first and foremost. So in
>> general trying to avoid that would be useful for most guests.
>
> PV ticketlocks in HVM and PV guests make this "manageable".
>
>>
>> The thing with scheduling is that it's a bit like economics: it seems
>> simple but it's actually not at all obvious what the emergent behavior
>> will be from adding a simple rule. :-)
>
>
>>
>> On the whole it seems unlikely that having two vcpus on a single pcpu
>> is a "stable" situation -- it's likely to be pretty transient, and
>> thus not have a major impact on performance.
>
> Except that we are concerned with it - in fact we are disabling this
> feature because it may happen. How do we make sure it does not happen
> all the time? Or at least do some back-off if things do get
> in this situation.
So it's disabled by default based on a theoretical fear that it *may*
cause performance problems, but without any actual performance problems
having been observed?
It seems like there are a couple of ways we could approach this:
1. Try to optimize the reverse look-up code so that it's not a linear
linked list (getting rid of the theoretical fear)
2. Try to test engineered situations where we expect this to be a
problem, to see how big of a problem it is (proving the theory to be
accurate or inaccurate in this case)
3. Turn the feature on by default as soon as the 4.8 window opens up,
perhaps with some sort of a check that runs when in debug mode that
looks for the condition we're afraid of happening and BUG()s. If we run
a full development cycle without anyone hitting the bug in testing, then
we just leave the feature on.
Then we'll only look at adding complexity to the scheduler if there's
actually a problem to solve.
-George
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 1/2] IOMMU/spinlock: Fix a bug found in AMD IOMMU initialization.
On Tue, 2016-03-08 at 19:09 +0800, Quan Xu wrote: > Doing what we do serves as a fix for a bug found in AMD IOMMU > initialization. > This first line is rather useless, if not worse. :-) I don't know (provided a new version is not necessary, and provided maintainers agree with me :-)) whether you need to repost or it can be removed when code is committed. > Signed-off-by: Quan Xu > CC: Suravee Suthikulpanit > CC: Aravind Gopalakrishnan > get_maintainer.pl gives me only Suravee, as Aravind stepped down a few days ago, so he shouldn't be bothered (and, in fact, I'm moving him from Cc to Bcc). > CC: Dario Faggioli > CC: Jan Beulich > (BTW, it's of course fine to include me and Jan, despite what get_maintainer.pl's output, as we've been involved in previous rounds of review.) All that being said: Reviewed-by: Dario Faggioli 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 2/2] IOMMU/spinlock: Make the pcidevs_lock a recursive one
On Tue, 2016-03-08 at 19:09 +0800, Quan Xu wrote:
> Signed-off-by: Quan Xu
> CC: Keir Fraser
> CC: Jan Beulich
> CC: Andrew Cooper
> CC: Suravee Suthikulpanit
> CC: Aravind Gopalakrishnan
> CC: Feng Wu
> CC: Kevin Tian
> CC: Dario Faggioli
>
I've gone through the code, and it looks fine.
However, when trying to apply the patch, on top of this morning's
staging, I got this:
[dario@Solace xen.git] $ patch -p1 <
\[PATCH_2_2\]_IOMMU_spinlock\:_Make_the_pcidevs_lock_a_recursive_one.mbox
patching file xen/arch/x86/domctl.c
Hunk #1 succeeded at 472 (offset 45 lines).
Hunk #2 succeeded at 497 (offset 45 lines).
patching file xen/arch/x86/hvm/vmsi.c
Hunk #1 succeeded at 388 with fuzz 1.
Hunk #2 succeeded at 446 with fuzz 1 (offset 3 lines).
patching file xen/arch/x86/irq.c
Hunk #1 succeeded at 1960 (offset 5 lines).
Hunk #2 succeeded at 2105 (offset 5 lines).
Hunk #3 succeeded at 2231 (offset 5 lines).
Hunk #4 succeeded at 2239 (offset 5 lines).
patching file xen/arch/x86/msi.c
patching file xen/arch/x86/pci.c
Hunk #1 succeeded at 88 (offset 6 lines).
patching file xen/arch/x86/physdev.c
patching file xen/common/sysctl.c
patching file xen/drivers/passthrough/amd/iommu_init.c
patching file xen/drivers/passthrough/amd/iommu_map.c
patching file xen/drivers/passthrough/amd/pci_amd_iommu.c
patching file xen/drivers/passthrough/pci.c
Hunk #17 succeeded at 1226 with fuzz 1.
Hunk #18 succeeded at 1262 (offset -6 lines).
Hunk #19 succeeded at 1291 (offset -6 lines).
Hunk #20 succeeded at 1340 (offset -6 lines).
Hunk #21 succeeded at 1364 (offset -6 lines).
Hunk #22 succeeded at 1401 (offset -6 lines).
Hunk #23 succeeded at 1416 (offset -6 lines).
Hunk #24 succeeded at 1471 (offset -6 lines).
Hunk #25 succeeded at 1490 (offset -6 lines).
Hunk #26 succeeded at 1625 (offset -6 lines).
patching file xen/drivers/passthrough/vtd/iommu.c
Hunk #1 succeeded at 1282 (offset -4 lines).
Hunk #2 succeeded at 1424 (offset -4 lines).
Hunk #3 succeeded at 1506 (offset -4 lines).
Hunk #4 succeeded at 1816 (offset -4 lines).
Hunk #5 succeeded at 1881 (offset -4 lines).
Hunk #6 succeeded at 2109 (offset -4 lines).
Hunk #7 succeeded at 2123 (offset -4 lines).
patching file xen/drivers/video/vga.c
patching file xen/include/xen/pci.h
And, when building:
gcc -O2 -fomit-frame-pointer -m64 -fno-strict-aliasing -std=gnu99 -Wall
-Wstrict-prototypes -Wdeclaration-after-statement -Wno-unused-but-set-variable
-Wno-unused-local-typedefs -DNDEBUG
-I/home/SOURCES/xen/xen/xen.git/xen/include
-I/home/SOURCES/xen/xen/xen.git/xen/include/asm-x86/mach-generic
-I/home/SOURCES/xen/xen/xen.git/xen/include/asm-x86/mach-default
'-D__OBJECT_LABEL__=drivers$passthrough$vtd$intremap.o' -msoft-float
-fno-stack-protector -fno-exceptions -Wnested-externs -DHAVE_GAS_VMX
-DHAVE_GAS_EPT -DHAVE_GAS_FSGSBASE -U__OBJECT_LABEL__ -DHAVE_GAS_QUOTED_SYM
'-D__OBJECT_LABEL__=drivers/passthrough/vtd/intremap.o' -mno-red-zone -mno-sse
-fpic -fno-asynchronous-unwind-tables -DGCC_HAS_VISIBILITY_ATTRIBUTE -nostdinc
-fno-builtin -fno-common -Werror -Wredundant-decls -Wno-pointer-arith -pipe -g
-D__XEN__ -include /home/SOURCES/xen/xen/xen.git/xen/include/xen/config.h
'-D__OBJECT_FILE__="intremap.o"' -DPERF_COUNTERS -DPERF_ARRAYS -MMD -MF
./.intremap.o.d -c intremap.c -o intremap.o
In file included from
/home/SOURCES/xen/xen/xen.git/xen/include/xen/bitmap.h:6:0,
from
/home/SOURCES/xen/xen/xen.git/xen/include/xen/cpumask.h:78,
from /home/SOURCES/xen/xen/xen.git/xen/include/xen/irq.h:4,
from intremap.c:20:
intremap.c: In function 'pi_update_irte':
intremap.c:987:27: error: passing argument 1 of '_spin_is_locked' from
incompatible pointer type [-Werror]
ASSERT(spin_is_locked(&pcidevs_lock));
^
/home/SOURCES/xen/xen/xen.git/xen/include/xen/lib.h:35:35: note: in definition
of macro 'ASSERT'
#define ASSERT(p) do { if ( 0 && (p) ) {} } while (0)
^
intremap.c:987:12: note: in expansion of macro 'spin_is_locked'
ASSERT(spin_is_locked(&pcidevs_lock));
^
In file included from
/home/SOURCES/xen/xen/xen.git/xen/include/xen/rcupdate.h:35:0,
from /home/SOURCES/xen/xen/xen.git/xen/include/xen/irq.h:5,
from intremap.c:20:
/home/SOURCES/xen/xen/xen.git/xen/include/xen/spinlock.h:163:5: note: expected
'struct spinlock_t *' but argument is of type 'void (*)(void)'
int _spin_is_locked(spinlock_t *lock);
So, I think a refresh is necessary.
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 1/2] IOMMU/spinlock: Fix a bug found in AMD IOMMU initialization.
On March 08, 2016 8:13pm, wrote: > On Tue, 2016-03-08 at 19:09 +0800, Quan Xu wrote: > > Doing what we do serves as a fix for a bug found in AMD IOMMU > > initialization. > > > This first line is rather useless, if not worse. :-) > I will remove it in next v2. :) > I don't know (provided a new version is not necessary, and provided > maintainers > agree with me :-)) whether you need to repost or it can be removed when code > is committed. > I think I'd better send out v2. :) > > Signed-off-by: Quan Xu > > CC: Suravee Suthikulpanit > > CC: Aravind Gopalakrishnan > > > get_maintainer.pl gives me only Suravee, as Aravind stepped down a few days > ago, so he shouldn't be bothered (and, in fact, I'm moving him from Cc to > Bcc). > Got it, thanks for your advice. > > CC: Dario Faggioli > > CC: Jan Beulich > > > (BTW, it's of course fine to include me and Jan, despite what > get_maintainer.pl's > output, as we've been involved in previous rounds of review.) > > All that being said: > > Reviewed-by: Dario Faggioli > Dario, thanks. Quan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 2/2] IOMMU/spinlock: Make the pcidevs_lock a recursive one
On March 08, 2016 8:29pm, wrote:
> On Tue, 2016-03-08 at 19:09 +0800, Quan Xu wrote:
> > Signed-off-by: Quan Xu
> > CC: Keir Fraser
> > CC: Jan Beulich
> > CC: Andrew Cooper
> > CC: Suravee Suthikulpanit
> > CC: Aravind Gopalakrishnan
> > CC: Feng Wu
> > CC: Kevin Tian
> > CC: Dario Faggioli
> >
> I've gone through the code, and it looks fine.
>
> However, when trying to apply the patch, on top of this morning's staging, I
> got
> this:
>
Oh, sorry, it is not against this morning's staging.
I would try to send out patch against this morning's staging soon. Thanks.
-Quan
> [dario@Solace xen.git] $ patch -p1 <
> \[PATCH_2_2\]_IOMMU_spinlock\:_Make_the_pcidevs_lock_a_recursive_one.
> mbox
> patching file xen/arch/x86/domctl.c
> Hunk #1 succeeded at 472 (offset 45 lines).
> Hunk #2 succeeded at 497 (offset 45 lines).
> patching file xen/arch/x86/hvm/vmsi.c
> Hunk #1 succeeded at 388 with fuzz 1.
> Hunk #2 succeeded at 446 with fuzz 1 (offset 3 lines).
> patching file xen/arch/x86/irq.c
> Hunk #1 succeeded at 1960 (offset 5 lines).
> Hunk #2 succeeded at 2105 (offset 5 lines).
> Hunk #3 succeeded at 2231 (offset 5 lines).
> Hunk #4 succeeded at 2239 (offset 5 lines).
> patching file xen/arch/x86/msi.c
> patching file xen/arch/x86/pci.c
> Hunk #1 succeeded at 88 (offset 6 lines).
> patching file xen/arch/x86/physdev.c
> patching file xen/common/sysctl.c
> patching file xen/drivers/passthrough/amd/iommu_init.c
> patching file xen/drivers/passthrough/amd/iommu_map.c
> patching file xen/drivers/passthrough/amd/pci_amd_iommu.c
> patching file xen/drivers/passthrough/pci.c Hunk #17 succeeded at 1226 with
> fuzz 1.
> Hunk #18 succeeded at 1262 (offset -6 lines).
> Hunk #19 succeeded at 1291 (offset -6 lines).
> Hunk #20 succeeded at 1340 (offset -6 lines).
> Hunk #21 succeeded at 1364 (offset -6 lines).
> Hunk #22 succeeded at 1401 (offset -6 lines).
> Hunk #23 succeeded at 1416 (offset -6 lines).
> Hunk #24 succeeded at 1471 (offset -6 lines).
> Hunk #25 succeeded at 1490 (offset -6 lines).
> Hunk #26 succeeded at 1625 (offset -6 lines).
> patching file xen/drivers/passthrough/vtd/iommu.c
> Hunk #1 succeeded at 1282 (offset -4 lines).
> Hunk #2 succeeded at 1424 (offset -4 lines).
> Hunk #3 succeeded at 1506 (offset -4 lines).
> Hunk #4 succeeded at 1816 (offset -4 lines).
> Hunk #5 succeeded at 1881 (offset -4 lines).
> Hunk #6 succeeded at 2109 (offset -4 lines).
> Hunk #7 succeeded at 2123 (offset -4 lines).
> patching file xen/drivers/video/vga.c
> patching file xen/include/xen/pci.h
>
> And, when building:
>
> gcc -O2 -fomit-frame-pointer -m64 -fno-strict-aliasing -std=gnu99 -Wall
> -Wstrict-prototypes -Wdeclaration-after-statement
> -Wno-unused-but-set-variable -Wno-unused-local-typedefs -DNDEBUG
> -I/home/SOURCES/xen/xen/xen.git/xen/include
> -I/home/SOURCES/xen/xen/xen.git/xen/include/asm-x86/mach-generic
> -I/home/SOURCES/xen/xen/xen.git/xen/include/asm-x86/mach-default
> '-D__OBJECT_LABEL__=drivers$passthrough$vtd$intremap.o' -msoft-float
> -fno-stack-protector -fno-exceptions -Wnested-externs -DHAVE_GAS_VMX
> -DHAVE_GAS_EPT -DHAVE_GAS_FSGSBASE -U__OBJECT_LABEL__
> -DHAVE_GAS_QUOTED_SYM
> '-D__OBJECT_LABEL__=drivers/passthrough/vtd/intremap.o' -mno-red-zone
> -mno-sse -fpic -fno-asynchronous-unwind-tables
> -DGCC_HAS_VISIBILITY_ATTRIBUTE -nostdinc -fno-builtin -fno-common
> -Werror -Wredundant-decls -Wno-pointer-arith -pipe -g -D__XEN__ -include
> /home/SOURCES/xen/xen/xen.git/xen/include/xen/config.h
> '-D__OBJECT_FILE__="intremap.o"' -DPERF_COUNTERS -DPERF_ARRAYS -MMD
> -MF ./.intremap.o.d -c intremap.c -o intremap.o In file included from
> /home/SOURCES/xen/xen/xen.git/xen/include/xen/bitmap.h:6:0,
> from
> /home/SOURCES/xen/xen/xen.git/xen/include/xen/cpumask.h:78,
> from
> /home/SOURCES/xen/xen/xen.git/xen/include/xen/irq.h:4,
> from intremap.c:20:
> intremap.c: In function 'pi_update_irte':
> intremap.c:987:27: error: passing argument 1 of '_spin_is_locked' from
> incompatible pointer type [-Werror]
> ASSERT(spin_is_locked(&pcidevs_lock));
> ^
> /home/SOURCES/xen/xen/xen.git/xen/include/xen/lib.h:35:35: note: in
> definition of macro 'ASSERT'
> #define ASSERT(p) do { if ( 0 && (p) ) {} } while (0)
> ^
> intremap.c:987:12: note: in expansion of macro 'spin_is_locked'
> ASSERT(spin_is_locked(&pcidevs_lock));
> ^
> In file included from
> /home/SOURCES/xen/xen/xen.git/xen/include/xen/rcupdate.h:35:0,
> from
> /home/SOURCES/xen/xen/xen.git/xen/include/xen/irq.h:5,
> from intremap.c:20:
> /home/SOURCES/xen/xen/xen.git/xen/include/xen/spinlock.h:163:5: note:
> expected 'struct spinlock_t *' but argument is of type 'void (*)(void)'
> int _spin_is_locked(spinlock_t *lock);
>
> So, I think a refresh is necessary.
>
> Regards,
> Dario
> --
> <> (Raistlin Majere)
> --
[Xen-devel] [linux-linus test] 85667: regressions - FAIL
flight 85667 linux-linus real [real] http://logs.test-lab.xenproject.org/osstest/logs/85667/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-i386-rumpuserxen6 xen-build fail REGR. vs. 59254 build-amd64-rumpuserxen 6 xen-build fail REGR. vs. 59254 test-amd64-amd64-xl 15 guest-localmigratefail REGR. vs. 59254 test-amd64-amd64-xl-credit2 15 guest-localmigratefail REGR. vs. 59254 test-amd64-i386-xl 15 guest-localmigratefail REGR. vs. 59254 test-amd64-i386-xl-xsm 15 guest-localmigratefail REGR. vs. 59254 test-amd64-amd64-pair23 guest-stop/src_host fail REGR. vs. 59254 test-armhf-armhf-xl 15 guest-start/debian.repeat fail REGR. vs. 59254 test-armhf-armhf-xl-xsm 15 guest-start/debian.repeat fail REGR. vs. 59254 test-armhf-armhf-xl-multivcpu 15 guest-start/debian.repeat fail REGR. vs. 59254 test-armhf-armhf-xl-credit2 15 guest-start/debian.repeat fail REGR. vs. 59254 test-amd64-i386-pair 22 guest-migrate/dst_host/src_host fail REGR. vs. 59254 test-amd64-amd64-xl-xsm15 guest-localmigrate fail in 85614 REGR. vs. 59254 test-amd64-amd64-xl-multivcpu 15 guest-localmigrate fail in 85614 REGR. vs. 59254 Tests which are failing intermittently (not blocking): test-amd64-amd64-xl 14 guest-saverestore fail in 85614 pass in 85667 test-armhf-armhf-xl-cubietruck 15 guest-start/debian.repeat fail in 85614 pass in 85667 test-amd64-amd64-xl-xsm 14 guest-saverestore fail pass in 85614 test-amd64-amd64-xl-multivcpu 14 guest-saverestore fail pass in 85614 Regressions which are regarded as allowable (not blocking): test-amd64-amd64-xl-rtds 15 guest-localmigratefail REGR. vs. 59254 test-armhf-armhf-xl-rtds 11 guest-start fail REGR. vs. 59254 test-amd64-i386-libvirt-pair 22 guest-migrate/dst_host/src_host fail baseline untested test-amd64-amd64-libvirt-pair 22 guest-migrate/dst_host/src_host fail baseline untested test-armhf-armhf-xl-vhd 9 debian-di-install fail baseline untested test-amd64-i386-libvirt-xsm 15 guest-saverestore.2 fail blocked in 59254 test-amd64-amd64-libvirt 15 guest-saverestore.2 fail blocked in 59254 test-amd64-amd64-libvirt-xsm 15 guest-saverestore.2 fail blocked in 59254 test-amd64-i386-libvirt 15 guest-saverestore.2 fail blocked in 59254 test-amd64-amd64-xl-qemut-win7-amd64 16 guest-stop fail like 59254 test-amd64-amd64-xl-qemuu-win7-amd64 16 guest-stop fail like 59254 test-amd64-i386-xl-qemuu-win7-amd64 16 guest-stop fail like 59254 test-amd64-i386-xl-qemut-win7-amd64 16 guest-stop fail like 59254 Tests which did not succeed, but are not blocking: test-amd64-i386-rumpuserxen-i386 1 build-check(1) blocked n/a test-amd64-amd64-rumpuserxen-amd64 1 build-check(1) blocked n/a test-amd64-i386-libvirt-xsm 12 migrate-support-checkfail never pass test-amd64-amd64-libvirt 12 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 12 migrate-support-checkfail never pass test-amd64-amd64-xl-pvh-intel 14 guest-saverestorefail never pass test-amd64-amd64-xl-pvh-amd 11 guest-start fail never pass test-amd64-i386-libvirt 12 migrate-support-checkfail never pass test-armhf-armhf-libvirt 14 guest-saverestorefail never pass test-armhf-armhf-libvirt 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-amd64-qemuu-nested-intel 13 xen-boot/l1 fail never pass test-armhf-armhf-xl 12 migrate-support-checkfail never pass test-armhf-armhf-xl 13 saverestore-support-checkfail never pass test-amd64-amd64-qemuu-nested-amd 13 xen-boot/l1 fail 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-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check fail 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-amd64-amd64-libvirt-vhd 11 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-libvirt-xsm 12 migrate-support-checkfail never pass test-armhf-a
Re: [Xen-devel] Xentrace on Xilinx ARM
[Adding (back?) George, which wrote and maintains xenalyze, and tracing in general, and adding ARM people as well, because this is on ARM, isn't it?] On Mon, 2016-03-07 at 19:36 +, Ben Sanda wrote: > it was in a mercurial repo here: > http://xenbits.xensource.com/ext/xenalyze.hg > > but that repo is no longer functional it seems. I searched through > the mailing > lists and it looks like xenalyze was pulled into the mainline and now > resides in > tools/xentrace. > That's correct, it's all in tree now. > I can't determine how to get it to build though. I've tried > calling make in the directory but that fails. I'm using petalinux > which has a > build xen tools make object, which I have also tried, and it > generates an object > file for xenalyze.c, but no executable. > Mmm... In an x86 build, this is what I get: (debian-stable_amd64)dario@Solace:/home/SOURCES/xen/xen/xen.git$ ls tools/xentrace/xenalyze -lah -rwxrwxr-x. 1 dario dario 174K Mar 8 12:36 tools/xentrace/xenalyze (debian-stable_amd64)dario@Solace:/home/SOURCES/xen/xen/xen.git$ ls dist/install/usr/local/bin/xenalyze -lah -rwxr-xr-x. 1 dario dario 174K Mar 8 12:36 dist/install/usr/local/bin/xenalyze I guess we're talking about ARM (cross?) builds, which is something (especially for tools!) that I really have not much experience with. Maybe there's more to modify, in terms of Makefile-s, etc., to make that be build on ARM... > Could you provide any guidance as to how > to actually get xenalyze built? I'm assuming it's still an offline > tool? Or is it now > built into the Xen image? > It's not part of any Xen image. It's a command line tool to be used, usually but not necessarily, in dom0, build and installed together with the other tools... At least in my case, for x86 builds and installs. 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] Ideas Re: [PATCH v14 1/2] vmx: VT-d posted-interrupt core logic handling
> -Original Message-
> From: George Dunlap [mailto:george.dun...@citrix.com]
> Sent: Tuesday, March 8, 2016 8:02 PM
> To: Konrad Rzeszutek Wilk ; George Dunlap
>
> Cc: Wu, Feng ; Tian, Kevin ; Keir
> Fraser ; Andrew Cooper ; Dario
> Faggioli ; xen-devel@lists.xen.org; Jan Beulich
>
> Subject: Re: [Xen-devel] Ideas Re: [PATCH v14 1/2] vmx: VT-d posted-interrupt
> core logic handling
>
> On 07/03/16 15:53, Konrad Rzeszutek Wilk wrote:
> > On Mon, Mar 07, 2016 at 11:21:33AM +, George Dunlap wrote:
> >> On Fri, Mar 4, 2016 at 10:00 PM, Konrad Rzeszutek Wilk
> >> wrote:
> +/* Handle VT-d posted-interrupt when VCPU is blocked. */
> +static void pi_wakeup_interrupt(struct cpu_user_regs *regs)
> +{
> +struct arch_vmx_struct *vmx, *tmp;
> +spinlock_t *lock = &per_cpu(vmx_pi_blocking,
> smp_processor_id()).lock;
> +struct list_head *blocked_vcpus =
> + &per_cpu(vmx_pi_blocking, smp_processor_id()).list;
> +
> +ack_APIC_irq();
> +this_cpu(irq_count)++;
> +
> +spin_lock(lock);
> +
> +/*
> + * XXX: The length of the list depends on how many vCPU is current
> + * blocked on this specific pCPU. This may hurt the interrupt
> latency
> + * if the list grows to too many entries.
> + */
> +list_for_each_entry_safe(vmx, tmp, blocked_vcpus, pi_blocking.list)
> +{
> >>>
> >>>
> >>> My recollection of the 'most-horrible' case of this being really bad is
> >>> when
> >>> the scheduler puts the vCPU0 and VCPU1 of the guest on the same pCPU (as
> an example)
> >>> and they round-robin all the time.
> >>>
> >>>
> >>> Would it be perhaps possible to have an anti-affinity flag to deter the
> >>> scheduler from this? That is whichever struct vcpu has 'anti-affinity'
> >>> flag
> >>> set - the scheduler will try as much as it can _to not_ schedule the
> >>> 'struct
> vcpu'
> >>> if the previous 'struct vcpu' had this flag as well on this pCPU?
> >>
> >> Well having vcpus from the same guest on the same pcpu is problematic
> >> for a number of reasons -- spinlocks first and foremost. So in
> >> general trying to avoid that would be useful for most guests.
> >
> > PV ticketlocks in HVM and PV guests make this "manageable".
> >
> >>
> >> The thing with scheduling is that it's a bit like economics: it seems
> >> simple but it's actually not at all obvious what the emergent behavior
> >> will be from adding a simple rule. :-)
> >
> >
> >>
> >> On the whole it seems unlikely that having two vcpus on a single pcpu
> >> is a "stable" situation -- it's likely to be pretty transient, and
> >> thus not have a major impact on performance.
> >
> > Except that we are concerned with it - in fact we are disabling this
> > feature because it may happen. How do we make sure it does not happen
> > all the time? Or at least do some back-off if things do get
> > in this situation.
>
> So it's disabled by default based on a theoretical fear that it *may*
> cause performance problems, but without any actual performance problems
> having been observed?
Yes, according to Jan's comments in previous thread, theoretically, the list
may become very long, so he tend to make this feature default off now.
>
> It seems like there are a couple of ways we could approach this:
>
> 1. Try to optimize the reverse look-up code so that it's not a linear
> linked list (getting rid of the theoretical fear)
Good point.
>
> 2. Try to test engineered situations where we expect this to be a
> problem, to see how big of a problem it is (proving the theory to be
> accurate or inaccurate in this case)
Maybe we can run a SMP guest with all the vcpus pinned to a dedicated
pCPU, we can run some benchmark in the guest with VT-d PI and without
VT-d PI, then see the performance difference between these two sceanrios.
>
> 3. Turn the feature on by default as soon as the 4.8 window opens up,
> perhaps with some sort of a check that runs when in debug mode that
> looks for the condition we're afraid of happening and BUG()s. If we run
> a full development cycle without anyone hitting the bug in testing, then
> we just leave the feature on.
Maybe we can pre-define a max acceptable length of the list, if it really
reach the number, print out a warning or something like that. However,
how to decide the max length is a problem. May need more thinking.
Thanks,
Feng
>
> Then we'll only look at adding complexity to the scheduler if there's
> actually a problem to solve.
>
> -George
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 4/6] libxc: do some retries in xc_cpupool_removecpu() for EBUSY case
On Thu, 2016-03-03 at 17:48 +0100, Juergen Gross wrote:
> The hypervisor might return EBUSY when trying to remove a cpu from a
> cpupool when a domain running in this cpupool has pinned a vcpu
> temporarily. Do some retries in this case, perhaps the situation
> cleans up.
>
I now I'm at high risk of being called nitpicker (or, more likely, much
worse names), but I think that:
> --- a/tools/libxc/xc_cpupool.c
> +++ b/tools/libxc/xc_cpupool.c
> @@ -20,8 +20,11 @@
> */
>
> #include
> +#include
> #include "xc_private.h"
>
> +#define LIBXC_BUSY_RETRIES 5
> +
This name makes me think about something which wants to be more generic
than it is actually the case... Like some number of retries that libxc
does in general, while it's only applicable to a very specific cpupool
operation.
Just something like CPUPOOL_NUM_REMOVECPU_RETRIES (or, maybe, even
without the CPUPOOL_ prefix, as we're already inside cpupool.c) would
be more appropriate.
I'd also define it closer to xc_cpupool_removecpu() (but that is a lot
about personal taste, I guess) and would add a brief comment
(basically, a summary of what's in the changelog already), if only to
save people having to go through `git blame'.
> @@ -141,13 +144,21 @@ int xc_cpupool_removecpu(xc_interface *xch,
> uint32_t poolid,
> int cpu)
> {
> +unsigned retries;
> +int err;
> DECLARE_SYSCTL;
>
> sysctl.cmd = XEN_SYSCTL_cpupool_op;
> sysctl.u.cpupool_op.op = XEN_SYSCTL_CPUPOOL_OP_RMCPU;
> sysctl.u.cpupool_op.cpupool_id = poolid;
> sysctl.u.cpupool_op.cpu = (cpu < 0) ? XEN_SYSCTL_CPUPOOL_PAR_ANY
> : cpu;
> -return do_sysctl_save(xch, &sysctl);
> +for ( retries = 0; retries < LIBXC_BUSY_RETRIES; retries++ ) {
> +err = do_sysctl_save(xch, &sysctl);
> +if ( err >= 0 || errno != EBUSY )
> +break;
> +sleep(1);
> +}
>
Doing this the other way round (basically, exactly as the same thing is
done in do_sysctl_save() already), reads, IMHO, more natural:
for (...) {
err = do_sysctl_save(..);
if ( err < 0 && errno == EBUSY )
sleep(1);
else
break;
}
But yeah, this really is nitpicking. :-)
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 v3 5/6] libxl: print message how to recover from xl cpupool-cpu-remove errors
On Thu, 2016-03-03 at 17:48 +0100, Juergen Gross wrote: > An error occurring when calling "xl cpupool-cpu-remove" might leave > the system in a state where a cpu is neither completely free nor in > a cpupool. This can easily be repaired by adding the cpu via > "xl cpupool-cpu-add" to the cpupool where it was removed from before. > Print a message telling this the user in case of an error. > > Cc: Ian Jackson > Cc: Stefano Stabellini > Cc: Wei Liu > Signed-off-by: Juergen Gross > Reviewed-by: Dario Faggioli 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 v3 6/6] libxl: add force option for xl vcpu-pin
On Thu, 2016-03-03 at 17:48 +0100, Juergen Gross wrote:
> In order to be able to undo a vcpu pin override in case of a kernel
> driver error add a flag "-f" to the "xl vcpu-pin" command forcing the
> hypervisor to undo the override.
>
> Cc: Ian Jackson
> Cc: Stefano Stabellini
> Cc: Wei Liu
> Signed-off-by: Juergen Gross
>
Reviewed-by: Dario Faggioli
With the only comment that, here:
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -5344,6 +5344,10 @@ int main_vcpulist(int argc, char **argv)
>
> int main_vcpupin(int argc, char **argv)
> {
> +static struct option opts[] = {
> +{"force", 0, 0, 'f'},
> +COMMON_LONG_OPTS
> +};
> libxl_vcpuinfo *vcpuinfo;
> libxl_bitmap cpumap_hard, cpumap_soft;;
> libxl_bitmap *soft = &cpumap_soft, *hard = &cpumap_hard;
> @@ -5355,13 +5359,17 @@ int main_vcpupin(int argc, char **argv)
> long vcpuid;
> const char *vcpu, *hard_str, *soft_str;
> char *endptr;
> -int opt, nb_cpu, nb_vcpu, rc = EXIT_FAILURE;
> +int opt, nb_cpu, nb_vcpu, force = 0, rc = EXIT_FAILURE;
>
force can be bool.
The Reviewed-by stands both with that changed, and as the patch looks
now.
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 2/2] IOMMU/spinlock: Make the pcidevs_lock a recursive one
>>> On 08.03.16 at 13:39, wrote: > On March 08, 2016 8:29pm, wrote: >> On Tue, 2016-03-08 at 19:09 +0800, Quan Xu wrote: >> > Signed-off-by: Quan Xu >> > CC: Keir Fraser >> > CC: Jan Beulich >> > CC: Andrew Cooper >> > CC: Suravee Suthikulpanit >> > CC: Aravind Gopalakrishnan >> > CC: Feng Wu >> > CC: Kevin Tian >> > CC: Dario Faggioli >> > >> I've gone through the code, and it looks fine. >> >> However, when trying to apply the patch, on top of this morning's staging, I >> got >> this: >> > Oh, sorry, it is not against this morning's staging. > I would try to send out patch against this morning's staging soon. Thanks. Well, with e.g. >> [dario@Solace xen.git] $ patch -p1 < >> \[PATCH_2_2\]_IOMMU_spinlock\:_Make_the_pcidevs_lock_a_recursive_one. >> mbox >> patching file xen/arch/x86/domctl.c >> Hunk #1 succeeded at 472 (offset 45 lines). >> Hunk #2 succeeded at 497 (offset 45 lines). >> patching file xen/arch/x86/hvm/vmsi.c >> Hunk #1 succeeded at 388 with fuzz 1. >> Hunk #2 succeeded at 446 with fuzz 1 (offset 3 lines). ... this it must have been quite old a tree - this file didn't change in the last 4 months. I consider it rather unfriendly to post such a patch without RFC tag, and without stating that it's against a stale tree. Was the recent v6 of the 5-patch series this way too? If so, I'm glad I didn't spend time looking at it yet. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 1/2] IOMMU/spinlock: Fix a bug found in AMD IOMMU initialization.
>>> On 08.03.16 at 12:09, wrote:
> Doing what we do serves as a fix for a bug found in AMD IOMMU initialization.
>
> The current code is using spin_lock{_irqsave(), _irqrestore()} to
> protect pci_get_dev() in the set_iommu_interrupt_handler(). However,
> this can only be called during AMD IOMMU initialization, with interrupt
> enabled, so at least it is not necessary to disable interrupts, or
> save/restore interrupt flag.
On top of what Dario said: This description isn't very accurate either:
If the code in question runs with interrupts enabled (which I'm not
sure it does; would take me following back up the call chain to see
whether this happens before or after interrupts get enabled for the
first time), then it may very well be necessary to disable interrupts
for a particular purpose - from an abstract pov. That's not the case
here, but the description of such a change shouldn't make incorrect
claims or statements.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 2/2] IOMMU/spinlock: Make the pcidevs_lock a recursive one
On March 08, 2016 9:49pm, wrote: > >>> On 08.03.16 at 13:39, wrote: > > On March 08, 2016 8:29pm, wrote: > >> On Tue, 2016-03-08 at 19:09 +0800, Quan Xu wrote: > >> > Signed-off-by: Quan Xu > >> > CC: Keir Fraser > >> > CC: Jan Beulich > >> > CC: Andrew Cooper > >> > CC: Suravee Suthikulpanit > >> > CC: Aravind Gopalakrishnan > >> > CC: Feng Wu > >> > CC: Kevin Tian > >> > CC: Dario Faggioli > >> > > >> I've gone through the code, and it looks fine. > >> > >> However, when trying to apply the patch, on top of this morning's > >> staging, I got > >> this: > >> > > Oh, sorry, it is not against this morning's staging. > > I would try to send out patch against this morning's staging soon. Thanks. > > Well, with e.g. > > >> [dario@Solace xen.git] $ patch -p1 < > >> > \[PATCH_2_2\]_IOMMU_spinlock\:_Make_the_pcidevs_lock_a_recursive_one. > >> mbox > >> patching file xen/arch/x86/domctl.c > >> Hunk #1 succeeded at 472 (offset 45 lines). > >> Hunk #2 succeeded at 497 (offset 45 lines). > >> patching file xen/arch/x86/hvm/vmsi.c Hunk #1 succeeded at 388 with > >> fuzz 1. > >> Hunk #2 succeeded at 446 with fuzz 1 (offset 3 lines). > > ... this it must have been quite old a tree - this file didn't change in the > last 4 > months. I consider it rather unfriendly to post such a patch without RFC tag, > and > without stating that it's against a stale tree. Was the recent v6 of the > 5-patch > series this way too? Yes, sorry, I didn't rebase since from v1. :(:( I will try to rebase against current staging and send out new patch sets. Quan > If so, I'm glad I didn't spend time looking at it yet. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 3/3] xl: new "loglvl" command
On Tue, Mar 8, 2016 at 8:08 AM, Jan Beulich wrote:
On 07.03.16 at 19:07, wrote:
>> On Mon, 2016-03-07 at 04:46 -0700, Jan Beulich wrote:
>>> > > > On 04.03.16 at 19:45, wrote:
>>> > On Fri, 2016-03-04 at 09:48 -0700, Jan Beulich wrote:
>>
>>> > > --- a/tools/libxl/libxl.c
>>> > > +++ b/tools/libxl/libxl.c
>>> > > @@ -5958,6 +5958,26 @@ int libxl_send_debug_keys(libxl_ctx *ctx
>>> > > return 0;
>>> > > }
>>> > >
>>> > > +int libxl_log_level(libxl_ctx *ctx, bool set, bool guest,
>>> > > +int *lower_thresh, int *upper_thresh)
>>> > > +{
>>> > > +int ret;
>>> > >
>>> > As per libxl coding style, this wants to be 'r'.
>>> This and everything else below look to be valid comments, but
>>> it's rather frustrating that simply cloning an existing function (I
>>> user the debug key ones as basis) doesn't give me valid code,
>>> the more that I did scroll up and down a few pages to see
>>> whether I just happened to pick a particularly bad example.
>>>
>> Hehe, but do you understand that, saying this, you're making it very
>> likely that people will ask *you* to fix libxl_send_debug_keys() --and
>> perhaps more tool side code? :-P :-P
>
> Except that it's not just that function - as said, I did scroll up and
> down, without finding (style wise) better examples. And no, I'm
> not going to put together patches to deal with style issues in the
> tools.
>
>> No, jokes apart, I agree that inconsistency is a real bad thing... but
>> it's an hard fight, and we do have examples spread all around the
>> source code (both Xen and tools), AFAICT.
>
> Right, and asking people myself to not follow bad examples when
> adding new code, I did take all of your input to adjust the patch.
> Just that in this case the set of bad examples is so large that in a
> similar case in the hypervisor I probably wouldn't have dared to
> ask for a style correction.
Well the approach of the libxl maintainers seems to have be, "Just
make sure the new code adheres to the new style, and eventyally
everything will be up-to-date". A few releases ago I submitted a
patch where I added a new clause in the middle of a series of other
very similar clauses, and I was asked to make the new clause follow
the new style, but to leave the other clauses right next to it in the
old style (to avoid unnecessary code churn, since it was during the
development freeze).
Given that the "new" style has been around for a while now, it
probably would be good to set aside some time at the beginning of the
next development cycle to fix things up -- it is incredibly
frustrating to carefully try to copy the surrounding style, only to be
told to revise it.
-George
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 4/9] xl: Improve return and exit codes of main_console(), main_vncviewer() and main_dump_core().
[Re-adding xen-devel... please, don't drop it. :-)]
On Tue, 2016-03-08 at 13:08 +0530, Harmandeep Kaur wrote:
> On Thu, Feb 25, 2016 at 5:03 PM, Dario Faggioli
> wrote:
> >
>
> > > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > >
> > > @@ -3457,8 +3457,8 @@ int main_vncviewer(int argc, char **argv)
> > > domid = find_domain(argv[optind]);
> > >
> > > if (vncviewer(domid, autopass))
> > > -return 1;
> > > -return 0;
> > > +return EXIT_FAILURE;
> > > +return EXIT_SUCCESS;
> > >
> > Have a look at vncviewer() and autoconnect_vncviewer() too.
> >
> I am not sure about vncviewer(), do we need something like below:
>
> static int vncviewer(uint32_t domid, int autopass)
> {
> if (!libxl_vncviewer_exec(ctx, domid, autopass)) {
> fprintf(stderr, "Unable to execute vncviewer\n");
> return 1;
> }
> }
>
That won't compile, I think.
It's an internal function, and this patch is changing a bunch of
internal functions into returning -1 on failure ad 0 on success, which
is something vncviewer() is not doing.
It's not too big of a deal, honestly, and you can even let it alone
(we're not going to get 100% consistency anyway), but I thought you may
want to at least consider what to do.
OTOH, autoconnect_vncviewer() does have an _exit() that needs to be
dealt with.
Regards,
Dario
--
<> (Raistlin Majere)
-
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
signature.asc
Description: This is a digitally signed message part
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[Xen-devel] [PATCH v4 5/8] igd: use defines for standard pci config space offsets
Signed-off-by: Gerd Hoffmann
Reviewed-by: Stefano Stabellini
---
hw/pci-host/igd.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/hw/pci-host/igd.c b/hw/pci-host/igd.c
index 3654298..8a8b37b 100644
--- a/hw/pci-host/igd.c
+++ b/hw/pci-host/igd.c
@@ -11,9 +11,9 @@ typedef struct {
/* Here we just expose minimal host bridge offset subset. */
static const IGDHostInfo igd_host_bridge_infos[] = {
-{0x08, 2}, /* revision id */
-{0x2c, 2}, /* sybsystem vendor id */
-{0x2e, 2}, /* sybsystem id */
+{PCI_REVISION_ID, 2},
+{PCI_SUBSYSTEM_VENDOR_ID, 2},
+{PCI_SUBSYSTEM_ID,2},
{0x50, 2}, /* SNB: processor graphics control register */
{0x52, 2}, /* processor graphics control register */
{0xa4, 4}, /* SNB: graphics base of stolen memory */
--
1.8.3.1
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[Xen-devel] [PATCH v4 7/8] igd: move igd-passthrough-isa-bridge to igd.c too
Signed-off-by: Gerd Hoffmann
---
hw/i386/pc_piix.c | 113 --
hw/pci-host/igd.c | 108 +++
2 files changed, 108 insertions(+), 113 deletions(-)
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 40c58a5..43964dc 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -910,119 +910,6 @@ static void pc_i440fx_0_10_machine_options(MachineClass
*m)
DEFINE_I440FX_MACHINE(v0_10, "pc-0.10", pc_compat_0_13,
pc_i440fx_0_10_machine_options);
-typedef struct {
-uint16_t gpu_device_id;
-uint16_t pch_device_id;
-uint8_t pch_revision_id;
-} IGDDeviceIDInfo;
-
-/* In real world different GPU should have different PCH. But actually
- * the different PCH DIDs likely map to different PCH SKUs. We do the
- * same thing for the GPU. For PCH, the different SKUs are going to be
- * all the same silicon design and implementation, just different
- * features turn on and off with fuses. The SW interfaces should be
- * consistent across all SKUs in a given family (eg LPT). But just same
- * features may not be supported.
- *
- * Most of these different PCH features probably don't matter to the
- * Gfx driver, but obviously any difference in display port connections
- * will so it should be fine with any PCH in case of passthrough.
- *
- * So currently use one PCH version, 0x8c4e, to cover all HSW(Haswell)
- * scenarios, 0x9cc3 for BDW(Broadwell).
- */
-static const IGDDeviceIDInfo igd_combo_id_infos[] = {
-/* HSW Classic */
-{0x0402, 0x8c4e, 0x04}, /* HSWGT1D, HSWD_w7 */
-{0x0406, 0x8c4e, 0x04}, /* HSWGT1M, HSWM_w7 */
-{0x0412, 0x8c4e, 0x04}, /* HSWGT2D, HSWD_w7 */
-{0x0416, 0x8c4e, 0x04}, /* HSWGT2M, HSWM_w7 */
-{0x041E, 0x8c4e, 0x04}, /* HSWGT15D, HSWD_w7 */
-/* HSW ULT */
-{0x0A06, 0x8c4e, 0x04}, /* HSWGT1UT, HSWM_w7 */
-{0x0A16, 0x8c4e, 0x04}, /* HSWGT2UT, HSWM_w7 */
-{0x0A26, 0x8c4e, 0x06}, /* HSWGT3UT, HSWM_w7 */
-{0x0A2E, 0x8c4e, 0x04}, /* HSWGT3UT28W, HSWM_w7 */
-{0x0A1E, 0x8c4e, 0x04}, /* HSWGT2UX, HSWM_w7 */
-{0x0A0E, 0x8c4e, 0x04}, /* HSWGT1ULX, HSWM_w7 */
-/* HSW CRW */
-{0x0D26, 0x8c4e, 0x04}, /* HSWGT3CW, HSWM_w7 */
-{0x0D22, 0x8c4e, 0x04}, /* HSWGT3CWDT, HSWD_w7 */
-/* HSW Server */
-{0x041A, 0x8c4e, 0x04}, /* HSWSVGT2, HSWD_w7 */
-/* HSW SRVR */
-{0x040A, 0x8c4e, 0x04}, /* HSWSVGT1, HSWD_w7 */
-/* BSW */
-{0x1606, 0x9cc3, 0x03}, /* BDWULTGT1, BDWM_w7 */
-{0x1616, 0x9cc3, 0x03}, /* BDWULTGT2, BDWM_w7 */
-{0x1626, 0x9cc3, 0x03}, /* BDWULTGT3, BDWM_w7 */
-{0x160E, 0x9cc3, 0x03}, /* BDWULXGT1, BDWM_w7 */
-{0x161E, 0x9cc3, 0x03}, /* BDWULXGT2, BDWM_w7 */
-{0x1602, 0x9cc3, 0x03}, /* BDWHALOGT1, BDWM_w7 */
-{0x1612, 0x9cc3, 0x03}, /* BDWHALOGT2, BDWM_w7 */
-{0x1622, 0x9cc3, 0x03}, /* BDWHALOGT3, BDWM_w7 */
-{0x162B, 0x9cc3, 0x03}, /* BDWHALO28W, BDWM_w7 */
-{0x162A, 0x9cc3, 0x03}, /* BDWGT3WRKS, BDWM_w7 */
-{0x162D, 0x9cc3, 0x03}, /* BDWGT3SRVR, BDWM_w7 */
-};
-
-static void isa_bridge_class_init(ObjectClass *klass, void *data)
-{
-DeviceClass *dc = DEVICE_CLASS(klass);
-PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
-
-dc->desc= "ISA bridge faked to support IGD PT";
-k->vendor_id= PCI_VENDOR_ID_INTEL;
-k->class_id = PCI_CLASS_BRIDGE_ISA;
-};
-
-static TypeInfo isa_bridge_info = {
-.name = "igd-passthrough-isa-bridge",
-.parent= TYPE_PCI_DEVICE,
-.instance_size = sizeof(PCIDevice),
-.class_init = isa_bridge_class_init,
-};
-
-static void pt_graphics_register_types(void)
-{
-type_register_static(&isa_bridge_info);
-}
-type_init(pt_graphics_register_types)
-
-void igd_passthrough_isa_bridge_create(PCIBus *bus, uint16_t gpu_dev_id)
-{
-struct PCIDevice *bridge_dev;
-int i, num;
-uint16_t pch_dev_id = 0x;
-uint8_t pch_rev_id;
-
-num = ARRAY_SIZE(igd_combo_id_infos);
-for (i = 0; i < num; i++) {
-if (gpu_dev_id == igd_combo_id_infos[i].gpu_device_id) {
-pch_dev_id = igd_combo_id_infos[i].pch_device_id;
-pch_rev_id = igd_combo_id_infos[i].pch_revision_id;
-}
-}
-
-if (pch_dev_id == 0x) {
-return;
-}
-
-/* Currently IGD drivers always need to access PCH by 1f.0. */
-bridge_dev = pci_create_simple(bus, PCI_DEVFN(0x1f, 0),
- "igd-passthrough-isa-bridge");
-
-/*
- * Note that vendor id is always PCI_VENDOR_ID_INTEL.
- */
-if (!bridge_dev) {
-fprintf(stderr, "set igd-passthrough-isa-bridge failed!\n");
-return;
-}
-pci_config_set_device_id(bridge_dev->config, pch_dev_id);
-pci_config_set_revision(bridge_dev->config, pch_rev_id);
-}
-
static void isapc_machine_options(MachineClass *m)
{
PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
diff --git a/hw/pci-host/igd.c b/hw/pci-host/igd.c
index 5c4a008..e7183a3 100644
--
[Xen-devel] [PATCH v4 1/8] pc: remove has_igd_gfx_passthru global
Signed-off-by: Gerd Hoffmann
Reviewed-by: Stefano Stabellini
Reviewed-by: Eduardo Habkost
---
hw/i386/pc_piix.c | 2 +-
hw/xen/xen_pt.h | 5 +++--
vl.c | 10 --
3 files changed, 4 insertions(+), 13 deletions(-)
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 6f8c2cd..40c58a5 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -364,7 +364,7 @@ static void pc_init_isa(MachineState *machine)
#ifdef CONFIG_XEN
static void pc_xen_hvm_init_pci(MachineState *machine)
{
-const char *pci_type = has_igd_gfx_passthru ?
+const char *pci_type = machine->igd_gfx_passthru ?
TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE :
TYPE_I440FX_PCI_DEVICE;
pc_init1(machine,
diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
index c2f8e1f..4048a5a 100644
--- a/hw/xen/xen_pt.h
+++ b/hw/xen/xen_pt.h
@@ -4,6 +4,7 @@
#include "qemu-common.h"
#include "hw/xen/xen_common.h"
#include "hw/pci/pci.h"
+#include "hw/boards.h"
#include "xen-host-pci-device.h"
void xen_pt_log(const PCIDevice *d, const char *f, ...) GCC_FMT_ATTR(2, 3);
@@ -322,10 +323,10 @@ extern void *pci_assign_dev_load_option_rom(PCIDevice
*dev,
unsigned int domain,
unsigned int bus, unsigned int
slot,
unsigned int function);
-extern bool has_igd_gfx_passthru;
static inline bool is_igd_vga_passthrough(XenHostPCIDevice *dev)
{
-return (has_igd_gfx_passthru
+MachineState *machine = MACHINE(qdev_get_machine());
+return (machine->igd_gfx_passthru
&& ((dev->class_code >> 0x8) == PCI_CLASS_DISPLAY_VGA));
}
int xen_pt_register_vga_regions(XenHostPCIDevice *dev);
diff --git a/vl.c b/vl.c
index adeddd9..bdc2879 100644
--- a/vl.c
+++ b/vl.c
@@ -1374,13 +1374,6 @@ static inline void semihosting_arg_fallback(const char
*file, const char *cmd)
}
}
-/* Now we still need this for compatibility with XEN. */
-bool has_igd_gfx_passthru;
-static void igd_gfx_passthru(void)
-{
-has_igd_gfx_passthru = current_machine->igd_gfx_passthru;
-}
-
/***/
/* USB devices */
@@ -4524,9 +4517,6 @@ int main(int argc, char **argv, char **envp)
exit(1);
}
-/* Check if IGD GFX passthrough. */
-igd_gfx_passthru();
-
/* init generic devices */
if (qemu_opts_foreach(qemu_find_opts("device"),
device_init_func, NULL, NULL)) {
--
1.8.3.1
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[Xen-devel] [PATCH v4 3/8] igd: switch TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE to realize
Signed-off-by: Gerd Hoffmann
Reviewed-by: Stefano Stabellini
---
hw/pci-host/igd.c | 9 -
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/hw/pci-host/igd.c b/hw/pci-host/igd.c
index 331e9e1..93b86ca 100644
--- a/hw/pci-host/igd.c
+++ b/hw/pci-host/igd.c
@@ -56,7 +56,7 @@ out:
return ret;
}
-static int igd_pt_i440fx_initfn(struct PCIDevice *pci_dev)
+static void igd_pt_i440fx_realize(PCIDevice *pci_dev, Error **errp)
{
uint32_t val = 0;
int rc, i, num;
@@ -68,12 +68,11 @@ static int igd_pt_i440fx_initfn(struct PCIDevice *pci_dev)
len = igd_host_bridge_infos[i].len;
rc = host_pci_config_read(pos, len, &val);
if (rc) {
-return -ENODEV;
+error_setg(errp, "failed to read host config");
+return;
}
pci_default_write_config(pci_dev, pos, val, len);
}
-
-return 0;
}
static void igd_passthrough_i440fx_class_init(ObjectClass *klass, void *data)
@@ -81,7 +80,7 @@ static void igd_passthrough_i440fx_class_init(ObjectClass
*klass, void *data)
DeviceClass *dc = DEVICE_CLASS(klass);
PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
-k->init = igd_pt_i440fx_initfn;
+k->realize = igd_pt_i440fx_realize;
dc->desc = "IGD Passthrough Host bridge";
}
--
1.8.3.1
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[Xen-devel] [PATCH v4 4/8] igd: TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE: call parent realize
Signed-off-by: Gerd Hoffmann
---
hw/pci-host/igd.c | 29 ++---
1 file changed, 26 insertions(+), 3 deletions(-)
diff --git a/hw/pci-host/igd.c b/hw/pci-host/igd.c
index 93b86ca..3654298 100644
--- a/hw/pci-host/igd.c
+++ b/hw/pci-host/igd.c
@@ -56,12 +56,32 @@ out:
return ret;
}
+#define IGD_PT_I440FX_CLASS(class) \
+OBJECT_CLASS_CHECK(IGDPtI440fxClass, (class), \
+ TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE)
+#define IGD_PT_I440FX_GET_CLASS(obj)\
+OBJECT_GET_CLASS(IGDPtI440fxClass, (obj), \
+ TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE)
+
+typedef struct IGDPtI440fxClass {
+PCIDeviceClass parent_class;
+void (*parent_realize)(PCIDevice *dev, Error **errp);
+} IGDPtI440fxClass;
+
static void igd_pt_i440fx_realize(PCIDevice *pci_dev, Error **errp)
{
+IGDPtI440fxClass *k = IGD_PT_I440FX_GET_CLASS(pci_dev);
+Error *err = NULL;
uint32_t val = 0;
int rc, i, num;
int pos, len;
+k->parent_realize(pci_dev, &err);
+if (err != NULL) {
+error_propagate(errp, err);
+return;
+}
+
num = ARRAY_SIZE(igd_host_bridge_infos);
for (i = 0; i < num; i++) {
pos = igd_host_bridge_infos[i].offset;
@@ -77,17 +97,20 @@ static void igd_pt_i440fx_realize(PCIDevice *pci_dev, Error
**errp)
static void igd_passthrough_i440fx_class_init(ObjectClass *klass, void *data)
{
+IGDPtI440fxClass *k = IGD_PT_I440FX_CLASS(klass);
DeviceClass *dc = DEVICE_CLASS(klass);
-PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+PCIDeviceClass *pc = PCI_DEVICE_CLASS(klass);
-k->realize = igd_pt_i440fx_realize;
-dc->desc = "IGD Passthrough Host bridge";
+k->parent_realize = pc->realize;
+pc->realize = igd_pt_i440fx_realize;
+dc->desc = "IGD Passthrough Host bridge (i440fx)";
}
static const TypeInfo igd_passthrough_i440fx_info = {
.name = TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE,
.parent= TYPE_I440FX_PCI_DEVICE,
.class_init= igd_passthrough_i440fx_class_init,
+.class_size= sizeof(IGDPtI440fxClass),
};
static void igd_register_types(void)
--
1.8.3.1
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[Xen-devel] [PATCH v4 2/8] pc: move igd support code to igd.c
Pure code motion, except for dropping instance_size for
TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE (no need to set,
we can inherit it from TYPE_I440FX_PCI_DEVICE).
Signed-off-by: Gerd Hoffmann
Acked-by: Stefano Stabellini
---
default-configs/x86_64-softmmu.mak | 1 +
hw/pci-host/Makefile.objs | 3 ++
hw/pci-host/igd.c | 99 ++
hw/pci-host/piix.c | 90 --
4 files changed, 103 insertions(+), 90 deletions(-)
create mode 100644 hw/pci-host/igd.c
diff --git a/default-configs/x86_64-softmmu.mak
b/default-configs/x86_64-softmmu.mak
index 6e3b312..cd3340e 100644
--- a/default-configs/x86_64-softmmu.mak
+++ b/default-configs/x86_64-softmmu.mak
@@ -58,3 +58,4 @@ CONFIG_IOH3420=y
CONFIG_I82801B11=y
CONFIG_SMBIOS=y
CONFIG_HYPERV_TESTDEV=$(CONFIG_KVM)
+CONFIG_PCI_IGD=y
diff --git a/hw/pci-host/Makefile.objs b/hw/pci-host/Makefile.objs
index 45f1f0e..c03210b 100644
--- a/hw/pci-host/Makefile.objs
+++ b/hw/pci-host/Makefile.objs
@@ -16,3 +16,6 @@ common-obj-$(CONFIG_FULONG) += bonito.o
common-obj-$(CONFIG_PCI_PIIX) += piix.o
common-obj-$(CONFIG_PCI_Q35) += q35.o
common-obj-$(CONFIG_PCI_GENERIC) += gpex.o
+
+# igd passthrough support
+common-obj-$(CONFIG_PCI_IGD) += igd.o
diff --git a/hw/pci-host/igd.c b/hw/pci-host/igd.c
new file mode 100644
index 000..331e9e1
--- /dev/null
+++ b/hw/pci-host/igd.c
@@ -0,0 +1,99 @@
+#include "qemu/osdep.h"
+#include "qemu-common.h"
+#include "hw/pci/pci.h"
+#include "hw/i386/pc.h"
+
+/* IGD Passthrough Host Bridge. */
+typedef struct {
+uint8_t offset;
+uint8_t len;
+} IGDHostInfo;
+
+/* Here we just expose minimal host bridge offset subset. */
+static const IGDHostInfo igd_host_bridge_infos[] = {
+{0x08, 2}, /* revision id */
+{0x2c, 2}, /* sybsystem vendor id */
+{0x2e, 2}, /* sybsystem id */
+{0x50, 2}, /* SNB: processor graphics control register */
+{0x52, 2}, /* processor graphics control register */
+{0xa4, 4}, /* SNB: graphics base of stolen memory */
+{0xa8, 4}, /* SNB: base of GTT stolen memory */
+};
+
+static int host_pci_config_read(int pos, int len, uint32_t *val)
+{
+char path[PATH_MAX];
+int config_fd;
+ssize_t size = sizeof(path);
+/* Access real host bridge. */
+int rc = snprintf(path, size, "/sys/bus/pci/devices/%04x:%02x:%02x.%d/%s",
+ 0, 0, 0, 0, "config");
+int ret = 0;
+
+if (rc >= size || rc < 0) {
+return -ENODEV;
+}
+
+config_fd = open(path, O_RDWR);
+if (config_fd < 0) {
+return -ENODEV;
+}
+
+if (lseek(config_fd, pos, SEEK_SET) != pos) {
+ret = -errno;
+goto out;
+}
+
+do {
+rc = read(config_fd, (uint8_t *)val, len);
+} while (rc < 0 && (errno == EINTR || errno == EAGAIN));
+if (rc != len) {
+ret = -errno;
+}
+
+out:
+close(config_fd);
+return ret;
+}
+
+static int igd_pt_i440fx_initfn(struct PCIDevice *pci_dev)
+{
+uint32_t val = 0;
+int rc, i, num;
+int pos, len;
+
+num = ARRAY_SIZE(igd_host_bridge_infos);
+for (i = 0; i < num; i++) {
+pos = igd_host_bridge_infos[i].offset;
+len = igd_host_bridge_infos[i].len;
+rc = host_pci_config_read(pos, len, &val);
+if (rc) {
+return -ENODEV;
+}
+pci_default_write_config(pci_dev, pos, val, len);
+}
+
+return 0;
+}
+
+static void igd_passthrough_i440fx_class_init(ObjectClass *klass, void *data)
+{
+DeviceClass *dc = DEVICE_CLASS(klass);
+PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+
+k->init = igd_pt_i440fx_initfn;
+dc->desc = "IGD Passthrough Host bridge";
+}
+
+static const TypeInfo igd_passthrough_i440fx_info = {
+.name = TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE,
+.parent= TYPE_I440FX_PCI_DEVICE,
+.class_init= igd_passthrough_i440fx_class_init,
+};
+
+static void igd_register_types(void)
+{
+type_register_static(&igd_passthrough_i440fx_info);
+}
+
+type_init(igd_register_types)
diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index 41aa66f..6eb8bff 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -747,95 +747,6 @@ static const TypeInfo i440fx_info = {
.class_init= i440fx_class_init,
};
-/* IGD Passthrough Host Bridge. */
-typedef struct {
-uint8_t offset;
-uint8_t len;
-} IGDHostInfo;
-
-/* Here we just expose minimal host bridge offset subset. */
-static const IGDHostInfo igd_host_bridge_infos[] = {
-{0x08, 2}, /* revision id */
-{0x2c, 2}, /* sybsystem vendor id */
-{0x2e, 2}, /* sybsystem id */
-{0x50, 2}, /* SNB: processor graphics control register */
-{0x52, 2}, /* processor graphics control register */
-{0xa4, 4}, /* SNB: graphics base of stolen memory */
-{0xa8, 4}, /* SNB: base of GTT stolen memory */
-};
-
-static int host_pci_config_read(int pos, int len, uint32_t *val)
-{
-char path[PATH_MAX]
[Xen-devel] [PATCH v4 8/8] igd: handle igd-passthrough-isa-bridge setup in realize()
That way a simple '-device igd-passthrough-isa-bridge,addr=1f' will
do the setup.
Also instead of looking up reasonable PCI IDs based on the graphic
device id simply copy over the ids from the host, thereby reusing the
infrastructure we have in place for the igd host bridges. Less code,
and should be more robust as we don't have to maintain the id table
to keep things going.
Signed-off-by: Gerd Hoffmann
---
hw/pci-host/igd.c| 115 +--
hw/xen/xen_pt.c | 4 +-
include/hw/i386/pc.h | 2 +-
3 files changed, 30 insertions(+), 91 deletions(-)
diff --git a/hw/pci-host/igd.c b/hw/pci-host/igd.c
index e7183a3..0513c55 100644
--- a/hw/pci-host/igd.c
+++ b/hw/pci-host/igd.c
@@ -100,111 +100,52 @@ static const TypeInfo igd_passthrough_i440fx_info = {
.class_size= sizeof(IGDPtI440fxClass),
};
-typedef struct {
-uint16_t gpu_device_id;
-uint16_t pch_device_id;
-uint8_t pch_revision_id;
-} IGDDeviceIDInfo;
-
-/* In real world different GPU should have different PCH. But actually
- * the different PCH DIDs likely map to different PCH SKUs. We do the
- * same thing for the GPU. For PCH, the different SKUs are going to be
- * all the same silicon design and implementation, just different
- * features turn on and off with fuses. The SW interfaces should be
- * consistent across all SKUs in a given family (eg LPT). But just same
- * features may not be supported.
- *
- * Most of these different PCH features probably don't matter to the
- * Gfx driver, but obviously any difference in display port connections
- * will so it should be fine with any PCH in case of passthrough.
- *
- * So currently use one PCH version, 0x8c4e, to cover all HSW(Haswell)
- * scenarios, 0x9cc3 for BDW(Broadwell).
- */
-static const IGDDeviceIDInfo igd_combo_id_infos[] = {
-/* HSW Classic */
-{0x0402, 0x8c4e, 0x04}, /* HSWGT1D, HSWD_w7 */
-{0x0406, 0x8c4e, 0x04}, /* HSWGT1M, HSWM_w7 */
-{0x0412, 0x8c4e, 0x04}, /* HSWGT2D, HSWD_w7 */
-{0x0416, 0x8c4e, 0x04}, /* HSWGT2M, HSWM_w7 */
-{0x041E, 0x8c4e, 0x04}, /* HSWGT15D, HSWD_w7 */
-/* HSW ULT */
-{0x0A06, 0x8c4e, 0x04}, /* HSWGT1UT, HSWM_w7 */
-{0x0A16, 0x8c4e, 0x04}, /* HSWGT2UT, HSWM_w7 */
-{0x0A26, 0x8c4e, 0x06}, /* HSWGT3UT, HSWM_w7 */
-{0x0A2E, 0x8c4e, 0x04}, /* HSWGT3UT28W, HSWM_w7 */
-{0x0A1E, 0x8c4e, 0x04}, /* HSWGT2UX, HSWM_w7 */
-{0x0A0E, 0x8c4e, 0x04}, /* HSWGT1ULX, HSWM_w7 */
-/* HSW CRW */
-{0x0D26, 0x8c4e, 0x04}, /* HSWGT3CW, HSWM_w7 */
-{0x0D22, 0x8c4e, 0x04}, /* HSWGT3CWDT, HSWD_w7 */
-/* HSW Server */
-{0x041A, 0x8c4e, 0x04}, /* HSWSVGT2, HSWD_w7 */
-/* HSW SRVR */
-{0x040A, 0x8c4e, 0x04}, /* HSWSVGT1, HSWD_w7 */
-/* BSW */
-{0x1606, 0x9cc3, 0x03}, /* BDWULTGT1, BDWM_w7 */
-{0x1616, 0x9cc3, 0x03}, /* BDWULTGT2, BDWM_w7 */
-{0x1626, 0x9cc3, 0x03}, /* BDWULTGT3, BDWM_w7 */
-{0x160E, 0x9cc3, 0x03}, /* BDWULXGT1, BDWM_w7 */
-{0x161E, 0x9cc3, 0x03}, /* BDWULXGT2, BDWM_w7 */
-{0x1602, 0x9cc3, 0x03}, /* BDWHALOGT1, BDWM_w7 */
-{0x1612, 0x9cc3, 0x03}, /* BDWHALOGT2, BDWM_w7 */
-{0x1622, 0x9cc3, 0x03}, /* BDWHALOGT3, BDWM_w7 */
-{0x162B, 0x9cc3, 0x03}, /* BDWHALO28W, BDWM_w7 */
-{0x162A, 0x9cc3, 0x03}, /* BDWGT3WRKS, BDWM_w7 */
-{0x162D, 0x9cc3, 0x03}, /* BDWGT3SRVR, BDWM_w7 */
+static const IGDHostInfo igd_isa_bridge_infos[] = {
+{PCI_VENDOR_ID, 2},
+{PCI_DEVICE_ID, 2},
+{PCI_REVISION_ID, 2},
+{PCI_SUBSYSTEM_VENDOR_ID, 2},
+{PCI_SUBSYSTEM_ID,2},
};
+static void igd_pt_isa_bridge_realize(PCIDevice *pci_dev, Error **errp)
+{
+Error *err = NULL;
+
+if (pci_dev->devfn != PCI_DEVFN(0x1f, 0)) {
+error_setg(errp, "igd isa bridge must have address 1f.0");
+return;
+}
+
+host_pci_config_copy(pci_dev, ":00:1f.0",
+ igd_isa_bridge_infos,
+ ARRAY_SIZE(igd_isa_bridge_infos),
+ &err);
+if (err != NULL) {
+error_propagate(errp, err);
+return;
+}
+}
+
static void isa_bridge_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
dc->desc= "ISA bridge faked to support IGD PT";
-k->vendor_id= PCI_VENDOR_ID_INTEL;
+k->realize = igd_pt_isa_bridge_realize;
k->class_id = PCI_CLASS_BRIDGE_ISA;
};
static TypeInfo igd_passthrough_isa_bridge_info = {
.name = "igd-passthrough-isa-bridge",
.parent= TYPE_PCI_DEVICE,
-.instance_size = sizeof(PCIDevice),
.class_init = isa_bridge_class_init,
};
-void igd_passthrough_isa_bridge_create(PCIBus *bus, uint16_t gpu_dev_id)
+void igd_passthrough_isa_bridge_create(PCIBus *bus)
{
-struct PCIDevice *bridge_dev;
-int i, num;
-uint16_t pch_dev_id = 0x;
-uint8_t pch_rev_id;
-
-num = ARRAY_SIZE(igd
[Xen-devel] [xen-unstable test] 85673: tolerable FAIL - PUSHED
flight 85673 xen-unstable real [real] http://logs.test-lab.xenproject.org/osstest/logs/85673/ Failures :-/ but no regressions. Regressions which are regarded as allowable (not blocking): test-armhf-armhf-xl-rtds 11 guest-start fail like 85533 build-i386-rumpuserxen6 xen-buildfail like 85628 build-amd64-rumpuserxen 6 xen-buildfail like 85628 test-amd64-amd64-xl-qemut-win7-amd64 16 guest-stop fail like 85628 test-amd64-i386-xl-qemut-win7-amd64 16 guest-stop fail like 85628 test-amd64-i386-xl-qemuu-win7-amd64 16 guest-stop fail like 85628 Tests which did not succeed, but are not blocking: test-amd64-amd64-rumpuserxen-amd64 1 build-check(1) blocked n/a test-amd64-i386-rumpuserxen-i386 1 build-check(1) blocked n/a 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-qemuu-nested-amd 16 debian-hvm-install/l1/l2 fail never pass test-amd64-i386-libvirt 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-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-qemuu-debianhvm-amd64-xsm 10 migrate-support-check fail 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-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-armhf-armhf-xl-multivcpu 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 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-amd64-libvirt 12 migrate-support-checkfail never pass test-armhf-armhf-libvirt-xsm 12 migrate-support-checkfail never pass test-armhf-armhf-libvirt-xsm 14 guest-saverestorefail never pass test-armhf-armhf-xl 12 migrate-support-checkfail never pass test-armhf-armhf-xl 13 saverestore-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-vhd 11 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 12 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 version targeted for testing: xen 0aa1330aac92fd75f185c9b354396014178fe95d baseline version: xen 1bd52e1fd66c47af690124d74d11ccb271c96f6b Last test of basis85628 2016-03-07 05:37:15 Z1 days Testing same since85673 2016-03-07 22:48:06 Z0 days1 attempts People who touched revisions under test: Andrew Cooper Dario Faggioli Doug Goldstein George Dunlap Jan Beulich Konrad Rzeszutek Wilk 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 pass build-amd64-pvopspass build
[Xen-devel] [PATCH v4 6/8] igd: revamp host config read
Move all work to the host_pci_config_copy helper function,
which we can easily reuse when adding q35 support.
Open sysfs file only once for all values. Use pread.
Proper error handling.
Fix bug: Update config space directly (writing via
pci_default_write_config only works for registers
whitelisted in wmask).
Hmm, this code can hardly ever worked before,
/me wonders what test coverage it had.
With this patch in place igd-passthru=on actually
works, although it still requires root priviledges
because linux refuses to allow non-root users access
pci config space above offset 0x50.
Signed-off-by: Gerd Hoffmann
---
hw/pci-host/igd.c | 65 ++-
1 file changed, 26 insertions(+), 39 deletions(-)
diff --git a/hw/pci-host/igd.c b/hw/pci-host/igd.c
index 8a8b37b..5c4a008 100644
--- a/hw/pci-host/igd.c
+++ b/hw/pci-host/igd.c
@@ -20,40 +20,33 @@ static const IGDHostInfo igd_host_bridge_infos[] = {
{0xa8, 4}, /* SNB: base of GTT stolen memory */
};
-static int host_pci_config_read(int pos, int len, uint32_t *val)
+static void host_pci_config_copy(PCIDevice *guest, const char *host,
+ const IGDHostInfo *list, int len, Error
**errp)
{
-char path[PATH_MAX];
-int config_fd;
-ssize_t size = sizeof(path);
-/* Access real host bridge. */
-int rc = snprintf(path, size, "/sys/bus/pci/devices/%04x:%02x:%02x.%d/%s",
- 0, 0, 0, 0, "config");
-int ret = 0;
+char *path;
+int config_fd, rc, i;
-if (rc >= size || rc < 0) {
-return -ENODEV;
-}
-
-config_fd = open(path, O_RDWR);
+path = g_strdup_printf("/sys/bus/pci/devices/%s/config", host);
+config_fd = open(path, O_RDONLY);
if (config_fd < 0) {
-return -ENODEV;
+error_setg_file_open(errp, errno, path);
+goto out_free;
}
-if (lseek(config_fd, pos, SEEK_SET) != pos) {
-ret = -errno;
-goto out;
+for (i = 0; i < len; i++) {
+rc = pread(config_fd, guest->config + list[i].offset,
+ list[i].len, list[i].offset);
+if (rc != list[i].len) {
+error_setg_errno(errp, errno, "read %s, offset 0x%x",
+ path, list[i].offset);
+goto out_close;
+}
}
-do {
-rc = read(config_fd, (uint8_t *)val, len);
-} while (rc < 0 && (errno == EINTR || errno == EAGAIN));
-if (rc != len) {
-ret = -errno;
-}
-
-out:
+out_close:
close(config_fd);
-return ret;
+out_free:
+g_free(path);
}
#define IGD_PT_I440FX_CLASS(class) \
@@ -72,9 +65,6 @@ static void igd_pt_i440fx_realize(PCIDevice *pci_dev, Error
**errp)
{
IGDPtI440fxClass *k = IGD_PT_I440FX_GET_CLASS(pci_dev);
Error *err = NULL;
-uint32_t val = 0;
-int rc, i, num;
-int pos, len;
k->parent_realize(pci_dev, &err);
if (err != NULL) {
@@ -82,16 +72,13 @@ static void igd_pt_i440fx_realize(PCIDevice *pci_dev, Error
**errp)
return;
}
-num = ARRAY_SIZE(igd_host_bridge_infos);
-for (i = 0; i < num; i++) {
-pos = igd_host_bridge_infos[i].offset;
-len = igd_host_bridge_infos[i].len;
-rc = host_pci_config_read(pos, len, &val);
-if (rc) {
-error_setg(errp, "failed to read host config");
-return;
-}
-pci_default_write_config(pci_dev, pos, val, len);
+host_pci_config_copy(pci_dev, ":00:00.0",
+ igd_host_bridge_infos,
+ ARRAY_SIZE(igd_host_bridge_infos),
+ &err);
+if (err != NULL) {
+error_propagate(errp, err);
+return;
}
}
--
1.8.3.1
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[Xen-devel] [PATCH v4 0/8] igd passthrough chipset tweaks
Hi, Next version of the patches, after a longish break. Meanwhile it is more clear how we are going to handle the igd passthrough quirks with kvm: vfio will get support for device-specific regions, and we will use that for the opregion and also to provide unpriviledged read access to host bridge (00:00.0) and isa bridge (00:1f,0) pci config space. That implies we wouldn't share the code for pci config space access and the existing xen code wouldn't be reused for kvm, except maybe for the struct IGDHostInfo tables. Separating out the igd support code into its own file and the cleanups + bugfixes on top of that still make sense though. So here we go with a stripped down patch series ... cheers, Gerd Gerd Hoffmann (8): pc: remove has_igd_gfx_passthru global pc: move igd support code to igd.c igd: switch TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE to realize igd: TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE: call parent realize igd: use defines for standard pci config space offsets igd: revamp host config read igd: move igd-passthrough-isa-bridge to igd.c too igd: handle igd-passthrough-isa-bridge setup in realize() default-configs/x86_64-softmmu.mak | 1 + hw/i386/pc_piix.c | 115 +-- hw/pci-host/Makefile.objs | 3 + hw/pci-host/igd.c | 157 + hw/pci-host/piix.c | 90 - hw/xen/xen_pt.c| 4 +- hw/xen/xen_pt.h| 5 +- include/hw/i386/pc.h | 2 +- vl.c | 10 --- 9 files changed, 167 insertions(+), 220 deletions(-) create mode 100644 hw/pci-host/igd.c -- 1.8.3.1 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] test
This is just a test of the mailman bounce mechanism: I want to be sure that I receive bounce messages (they have been filtered by SUSE's mail server). Please don't accept, reject or discard this message. I'll do it myself. In case it makes it to the list: please ignore. :-) Juergen ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] Ideas Re: [PATCH v14 1/2] vmx: VT-d posted-interrupt core logic handling
On Tue, Mar 8, 2016 at 1:10 PM, Wu, Feng wrote: >> -Original Message- >> From: George Dunlap [mailto:george.dun...@citrix.com] [snip] >> It seems like there are a couple of ways we could approach this: >> >> 1. Try to optimize the reverse look-up code so that it's not a linear >> linked list (getting rid of the theoretical fear) > > Good point. > >> >> 2. Try to test engineered situations where we expect this to be a >> problem, to see how big of a problem it is (proving the theory to be >> accurate or inaccurate in this case) > > Maybe we can run a SMP guest with all the vcpus pinned to a dedicated > pCPU, we can run some benchmark in the guest with VT-d PI and without > VT-d PI, then see the performance difference between these two sceanrios. This would give us an idea what the worst-case scenario would be. But pinning all vcpus to a single pcpu isn't really a sensible use case we want to support -- if you have to do something stupid to get a performance regression, then I as far as I'm concerned it's not a problem. Or to put it a different way: If we pin 10 vcpus to a single pcpu and then pound them all with posted interrupts, and there is *no* significant performance regression, then that will conclusively prove that the theoretical performance regression is of no concern, and we can enable PI by default. On the other hand, if we pin 10 vcpus to a single pcpu, pound them all with posted interrupts, and then there *is* a significant performance regression, then it would still not convince me there is a real problem to be solved. There is only actually a problem if the "long chain of vcpus" can happen in the course of a semi-realistic use-case. Suppose we had a set of SRIOV NICs with 10-20 virtual functions total, assigned to 10-20 VMs, and those VMs in a cpupool confined to a single socket of about 4 cores; and then we do a really network-intensive benchmark. That's a *bit* far-fetched, but it's something that might conceivably happen in the real world without any deliberate stupidity. If there's no significant performance issues in that case, I would think we can say that posted interrupts are robust enough to be enabled by default. >> 3. Turn the feature on by default as soon as the 4.8 window opens up, >> perhaps with some sort of a check that runs when in debug mode that >> looks for the condition we're afraid of happening and BUG()s. If we run >> a full development cycle without anyone hitting the bug in testing, then >> we just leave the feature on. > > Maybe we can pre-define a max acceptable length of the list, if it really > reach the number, print out a warning or something like that. However, > how to decide the max length is a problem. May need more thinking. I think we want to measure the amount of time spent in the interrupt handler (or with interrupts disabled). It doesn't matter if the list is 100 items long, if it can be handled in 500us. On the other hand, if a list of 4 elements takes 20ms, there's a pretty massive problem. :-) I don't have a good idea what an unreasonably large number would be here -- Jan? -George ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] test
On 3/8/16 8:35 AM, Juergen Gross wrote: > This is just a test of the mailman bounce mechanism: I want to be sure > that I receive bounce messages (they have been filtered by SUSE's mail > server). > > Please don't accept, reject or discard this message. I'll do it myself. > > In case it makes it to the list: please ignore. :-) > > > Juergen FWIW, I've been getting a lot of bounces from SUSE's mail servers in general in the last 2 weeks. -- Doug Goldstein signature.asc Description: OpenPGP digital signature ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] I'm a beginner and want become a Xen tester.
Hello. How can I test Xen and report bug? Can it need programming? Tnx. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [PATCH] xen: Restore p2m_access_t enum order to allow bitmask semantics
Nested hap code assumed implict bitmask semantics of the p2m_access_t
enum prior to C/S 4c63692d7c38c5ac414fe97f8ef37b66e05abe5c
The change to the enum ordering broke this assumption and caused functional
problems for the nested hap code. As it may be error prone to audit and find
all other p2m_access users assuming bitmask semantics, instead restore the
previous enum order and make it explict that bitmask semantics are to be
preserved for the read, write and execute access types.
Signed-off-by: Malcolm Crossley
---
xen/arch/x86/mm/hap/nested_hap.c | 2 +-
xen/include/xen/p2m-common.h | 17 +
2 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/xen/arch/x86/mm/hap/nested_hap.c b/xen/arch/x86/mm/hap/nested_hap.c
index 0dbae13..9cee5a0 100644
--- a/xen/arch/x86/mm/hap/nested_hap.c
+++ b/xen/arch/x86/mm/hap/nested_hap.c
@@ -263,7 +263,7 @@ nestedhvm_hap_nested_page_fault(struct vcpu *v, paddr_t
*L2_gpa,
switch ( p2ma_10 )
{
-case p2m_access_rwx ... p2m_access_n:
+case p2m_access_n ... p2m_access_rwx:
break;
case p2m_access_rx2rw:
p2ma_10 = p2m_access_rx;
diff --git a/xen/include/xen/p2m-common.h b/xen/include/xen/p2m-common.h
index 8b70459..6374a5b 100644
--- a/xen/include/xen/p2m-common.h
+++ b/xen/include/xen/p2m-common.h
@@ -15,14 +15,15 @@
* default.
*/
typedef enum {
-p2m_access_rwx = 0, /* The default access type when not used. */
-p2m_access_wx= 1,
-p2m_access_rx= 2,
-p2m_access_x = 3,
-p2m_access_rw= 4,
-p2m_access_w = 5,
-p2m_access_r = 6,
-p2m_access_n = 7, /* No access allowed. */
+/* Code uses bottom three bits with bitmask semantics */
+p2m_access_n = 0, /* No access allowed. */
+p2m_access_r = 1 << 0,
+p2m_access_w = 1 << 1,
+p2m_access_x = 1 << 2,
+p2m_access_rw= p2m_access_r | p2m_access_w,
+p2m_access_rx= p2m_access_r | p2m_access_x,
+p2m_access_wx= p2m_access_w | p2m_access_x,
+p2m_access_rwx = p2m_access_r | p2m_access_w | p2m_access_x,
p2m_access_rx2rw = 8, /* Special: page goes from RX to RW on write */
p2m_access_n2rwx = 9, /* Special: page goes from N to RWX on access, *
--
1.7.12.4
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] xen: Restore p2m_access_t enum order to allow bitmask semantics
On 08/03/16 15:30, Malcolm Crossley wrote:
> Nested hap code assumed implict bitmask semantics of the p2m_access_t
> enum prior to C/S 4c63692d7c38c5ac414fe97f8ef37b66e05abe5c
>
> The change to the enum ordering broke this assumption and caused functional
> problems for the nested hap code. As it may be error prone to audit and find
> all other p2m_access users assuming bitmask semantics, instead restore the
> previous enum order and make it explict that bitmask semantics are to be
> preserved for the read, write and execute access types.
>
> Signed-off-by: Malcolm Crossley
Reviewed-by: Andrew Cooper
Specifically, the bug causes memory corruption in the L2 guest, because
the code out of context in nestedhvm_hap_nested_page_fault() incorrectly
calculates the permission bits for the nested p2m.
> ---
> xen/arch/x86/mm/hap/nested_hap.c | 2 +-
> xen/include/xen/p2m-common.h | 17 +
> 2 files changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/xen/arch/x86/mm/hap/nested_hap.c
> b/xen/arch/x86/mm/hap/nested_hap.c
> index 0dbae13..9cee5a0 100644
> --- a/xen/arch/x86/mm/hap/nested_hap.c
> +++ b/xen/arch/x86/mm/hap/nested_hap.c
> @@ -263,7 +263,7 @@ nestedhvm_hap_nested_page_fault(struct vcpu *v, paddr_t
> *L2_gpa,
>
> switch ( p2ma_10 )
> {
> -case p2m_access_rwx ... p2m_access_n:
> +case p2m_access_n ... p2m_access_rwx:
> break;
> case p2m_access_rx2rw:
> p2ma_10 = p2m_access_rx;
> diff --git a/xen/include/xen/p2m-common.h b/xen/include/xen/p2m-common.h
> index 8b70459..6374a5b 100644
> --- a/xen/include/xen/p2m-common.h
> +++ b/xen/include/xen/p2m-common.h
> @@ -15,14 +15,15 @@
> * default.
> */
> typedef enum {
> -p2m_access_rwx = 0, /* The default access type when not used. */
> -p2m_access_wx= 1,
> -p2m_access_rx= 2,
> -p2m_access_x = 3,
> -p2m_access_rw= 4,
> -p2m_access_w = 5,
> -p2m_access_r = 6,
> -p2m_access_n = 7, /* No access allowed. */
> +/* Code uses bottom three bits with bitmask semantics */
> +p2m_access_n = 0, /* No access allowed. */
> +p2m_access_r = 1 << 0,
> +p2m_access_w = 1 << 1,
> +p2m_access_x = 1 << 2,
> +p2m_access_rw= p2m_access_r | p2m_access_w,
> +p2m_access_rx= p2m_access_r | p2m_access_x,
> +p2m_access_wx= p2m_access_w | p2m_access_x,
> +p2m_access_rwx = p2m_access_r | p2m_access_w | p2m_access_x,
>
> p2m_access_rx2rw = 8, /* Special: page goes from RX to RW on write */
> p2m_access_n2rwx = 9, /* Special: page goes from N to RWX on access, *
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 1/2] libxl: ensure var is inited in libxl__domain_firmware
On Mon, Mar 07, 2016 at 08:23:39PM -0600, Doug Goldstein wrote:
> Some versions of GCC complain that the 'firmware' variable can be used
> uninitialized. It looks like the switch inside of the else case is just
> confusing GCC.
>
> Signed-off-by: Doug Goldstein
Acked-by: Wei Liu
> ---
> CC: Ian Jackson
> CC: Stefano Stabellini
> CC: Wei Liu
> ---
> tools/libxl/libxl_dom.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index 664adad..b825b98 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -867,7 +867,7 @@ static int libxl__domain_firmware(libxl__gc *gc,
>struct xc_dom_image *dom)
> {
> libxl_ctx *ctx = libxl__gc_owner(gc);
> -const char *firmware;
> +const char *firmware = NULL;
> int e, rc;
> int datalen = 0;
> void *data;
> --
> 2.4.10
>
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 2/2] tools: detect appropriate debug optimization level
On Mon, Mar 07, 2016 at 08:23:40PM -0600, Doug Goldstein wrote: > The build should not use -O0 as that results in miscompilations. There This needs some (concrete) references. Is that a known issue in gcc? If so can you reference the bug number? > have been a few instances on the ML where users were told to switch > from -O0 to -O1 or -O2 or to set debug=n and their issue went away. The > preferred route should be to use -Og if its available, otherwise use > -O1 which is the default. This change undoes the change from -O1 to -O0 gcc manual says -O0 is the default. Not that I disagree with this patch in general, but the commit message seems a bit misleading. > in 1166ecf781b1016eaa61f8d5ba4fb1fde9d599b6. > And I have no idea why -O1 confuses the debugger so I've CC'ed Euan for more input. > Signed-off-by: Doug Goldstein > --- > CC: Ian Jackson > CC: Stefano Stabellini > CC: Wei Liu > --- > tools/Rules.mk | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/tools/Rules.mk b/tools/Rules.mk > index 9ef0b47..ae6b01f 100644 > --- a/tools/Rules.mk > +++ b/tools/Rules.mk > @@ -137,7 +137,8 @@ SHLIB_libxenvchan = $(SHDEPS_libxenvchan) > -Wl,-rpath-link=$(XEN_LIBVCHAN) > > ifeq ($(debug),y) > # Disable optimizations and enable debugging information for macros > -CFLAGS += -O0 -g3 > +$(call cc-option-add,CFLAGS,CC,-Og) > +CFLAGS += -g3 > # But allow an override to -O0 in case Python enforces -D_FORTIFY_SOURCE=. > PY_CFLAGS += $(PY_NOOPT_CFLAGS) > endif > -- > 2.4.10 > ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] Ideas Re: [PATCH v14 1/2] vmx: VT-d posted-interrupt core logic handling
>>> On 08.03.16 at 15:42, wrote: > On Tue, Mar 8, 2016 at 1:10 PM, Wu, Feng wrote: >>> -Original Message- >>> From: George Dunlap [mailto:george.dun...@citrix.com] > [snip] >>> It seems like there are a couple of ways we could approach this: >>> >>> 1. Try to optimize the reverse look-up code so that it's not a linear >>> linked list (getting rid of the theoretical fear) >> >> Good point. >> >>> >>> 2. Try to test engineered situations where we expect this to be a >>> problem, to see how big of a problem it is (proving the theory to be >>> accurate or inaccurate in this case) >> >> Maybe we can run a SMP guest with all the vcpus pinned to a dedicated >> pCPU, we can run some benchmark in the guest with VT-d PI and without >> VT-d PI, then see the performance difference between these two sceanrios. > > This would give us an idea what the worst-case scenario would be. How would a single VM ever give us an idea about the worst case? Something getting close to worst case is a ton of single vCPU guests all temporarily pinned to one and the same pCPU (could be multi-vCPU ones, but the more vCPU-s the more artificial this pinning would become) right before they go into blocked state (i.e. through one of the two callers of arch_vcpu_block()), the pinning removed while blocked, and then all getting woken at once. > But > pinning all vcpus to a single pcpu isn't really a sensible use case we > want to support -- if you have to do something stupid to get a > performance regression, then I as far as I'm concerned it's not a > problem. > > Or to put it a different way: If we pin 10 vcpus to a single pcpu and > then pound them all with posted interrupts, and there is *no* > significant performance regression, then that will conclusively prove > that the theoretical performance regression is of no concern, and we > can enable PI by default. The point isn't the pinning. The point is what pCPU they're on when going to sleep. And that could involve quite a few more than just 10 vCPU-s, provided they all sleep long enough. And the "theoretical performance regression is of no concern" is also not a proper way of looking at it, I would say: Even if such a situation would happen extremely rarely, if it can happen at all, it would still be a security issue. > On the other hand, if we pin 10 vcpus to a single pcpu, pound them all > with posted interrupts, and then there *is* a significant performance > regression, then it would still not convince me there is a real > problem to be solved. There is only actually a problem if the "long > chain of vcpus" can happen in the course of a semi-realistic use-case. > > Suppose we had a set of SRIOV NICs with 10-20 virtual functions total, > assigned to 10-20 VMs, and those VMs in a cpupool confined to a single > socket of about 4 cores; and then we do a really network-intensive > benchmark. That's a *bit* far-fetched, but it's something that might > conceivably happen in the real world without any deliberate stupidity. > If there's no significant performance issues in that case, I would > think we can say that posted interrupts are robust enough to be > enabled by default. > >>> 3. Turn the feature on by default as soon as the 4.8 window opens up, >>> perhaps with some sort of a check that runs when in debug mode that >>> looks for the condition we're afraid of happening and BUG()s. If we run >>> a full development cycle without anyone hitting the bug in testing, then >>> we just leave the feature on. >> >> Maybe we can pre-define a max acceptable length of the list, if it really >> reach the number, print out a warning or something like that. However, >> how to decide the max length is a problem. May need more thinking. > > I think we want to measure the amount of time spent in the interrupt > handler (or with interrupts disabled). It doesn't matter if the list > is 100 items long, if it can be handled in 500us. On the other hand, > if a list of 4 elements takes 20ms, there's a pretty massive problem. > :-) Spending on the order of 500us in an interrupt handler would already seem pretty long to me, especially when the interrupt may get raised at a high frequency. Even more so if, when in that state, _each_ invocation of the interrupt handler would take that long: With an (imo not unrealistic) interrupt rate of 1kHz we would spend half of the available CPU time in that handler. > I don't have a good idea what an unreasonably large number would be here -- > Jan? Neither do I, unfortunately. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] xen: Restore p2m_access_t enum order to allow bitmask semantics
>>> On 08.03.16 at 16:30, wrote:
> Nested hap code assumed implict bitmask semantics of the p2m_access_t
> enum prior to C/S 4c63692d7c38c5ac414fe97f8ef37b66e05abe5c
>
> The change to the enum ordering broke this assumption and caused functional
> problems for the nested hap code. As it may be error prone to audit and find
> all other p2m_access users assuming bitmask semantics, instead restore the
> previous enum order and make it explict that bitmask semantics are to be
> preserved for the read, write and execute access types.
Makes sense, but how certain are you that ...
> --- a/xen/include/xen/p2m-common.h
> +++ b/xen/include/xen/p2m-common.h
> @@ -15,14 +15,15 @@
> * default.
> */
> typedef enum {
> -p2m_access_rwx = 0, /* The default access type when not used. */
... namely this has not meanwhile seen any implicit use somewhere?
Tamas, the original commit talked about this as an optimization only.
Can you confirm that there indeed was no other intention than the
one claimed in that commit's description?
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 06/16] libxl: Load guest ACPI table from file
On Thu, Mar 03, 2016 at 05:12:07PM +, Anthony PERARD wrote: > On Tue, Mar 01, 2016 at 11:51:43AM +, Wei Liu wrote: > > On Thu, Feb 25, 2016 at 02:56:04PM +, Anthony PERARD wrote: > > > A user can provide a different ACPI tables than the default one by using > > > the existing "acpi_firmware" xl's config option or the field > > > u.hvm.acpi_firmware. > > > > > > libxl will check if the provided table is a DSDT or not. > > > > > > > According to xl.cfg manpage, acpi_firmware= cann't be used to override > > DSDT, so you seem to be changing the semantics of existing option. > > Yes, that was an idea from Ian Campbell <1446634655.6461.48.ca...@citrix.com> > I should at least change the manual. > > Would it be OK to reuse this options? Or should I add a new option, maybe > acpi_dsdt_override, or some other name? > If repurposing the old option won't break existing guest then that's fine, otherwise a new option is required. Wei. > -- > Anthony PERARD ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 03/16] configure: #define SEABIOS_PATH and OVMF_PATH
On Thu, Mar 03, 2016 at 05:03:00PM +, Anthony PERARD wrote: > On Tue, Mar 01, 2016 at 11:51:36AM +, Wei Liu wrote: > > On Thu, Feb 25, 2016 at 02:56:01PM +, Anthony PERARD wrote: > > > Those paths are to be used by libxl, in order to load the firmware in > > > memory. If a system path is not define, then this default to the Xen > > > firmware directory. > > > > > > Signed-off-by: Anthony PERARD > > > > > > > There is already --with-system-seabios and --with-system-ovmf that you > > can use. > > The path generated by --with-system-seabios is only for the benefit of the > Makefiles. With this patch, I'm exporting the value to the .c files. And in > the case where --with-system-* is not used, it provide a default path to > where we are going to install the firmware we compiled. > I see. SEABIOS_PATH and OVMF_PATH are already defined in Tools.mk.in. This approach is fine then. Wei. > -- > Anthony PERARD ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 6/6] libxl: add force option for xl vcpu-pin
On Thu, Mar 03, 2016 at 05:48:50PM +0100, Juergen Gross wrote: [...] > int libxl_set_vcpuaffinity_all(libxl_ctx *ctx, uint32_t domid, > unsigned int max_vcpus, > const libxl_bitmap *cpumap_hard, > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h > index f9e3ef5..19ec076 100644 > --- a/tools/libxl/libxl.h > +++ b/tools/libxl/libxl.h > @@ -1715,6 +1715,10 @@ int libxl_get_physinfo(libxl_ctx *ctx, libxl_physinfo > *physinfo); > int libxl_set_vcpuaffinity(libxl_ctx *ctx, uint32_t domid, uint32_t vcpuid, > const libxl_bitmap *cpumap_hard, > const libxl_bitmap *cpumap_soft); > +int libxl_set_vcpuaffinity_force(libxl_ctx *ctx, uint32_t domid, > + uint32_t vcpuid, > + const libxl_bitmap *cpumap_hard, > + const libxl_bitmap *cpumap_soft); With the introduction of this new API you need to have a #define in libxl.h Wei. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 5/6] libxl: print message how to recover from xl cpupool-cpu-remove errors
On Thu, Mar 03, 2016 at 05:48:49PM +0100, Juergen Gross wrote:
> An error occurring when calling "xl cpupool-cpu-remove" might leave
> the system in a state where a cpu is neither completely free nor in
> a cpupool. This can easily be repaired by adding the cpu via
> "xl cpupool-cpu-add" to the cpupool where it was removed from before.
> Print a message telling this the user in case of an error.
>
> Cc: Ian Jackson
> Cc: Stefano Stabellini
> Cc: Wei Liu
> Signed-off-by: Juergen Gross
Acked-by: Wei Liu
> ---
> tools/libxl/xl_cmdimpl.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 990d3c9..411473d 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -7716,8 +7716,10 @@ int main_cpupoolcpuremove(int argc, char **argv)
> goto out;
> }
>
> -if (libxl_cpupool_cpuremove_cpumap(ctx, poolid, &cpumap))
> -fprintf(stderr, "some cpus may not have been removed from %s\n",
> pool);
> +if (libxl_cpupool_cpuremove_cpumap(ctx, poolid, &cpumap)) {
> +fprintf(stderr, "Some cpus may have not or only partially been
> removed from '%s'.\n", pool);
> +fprintf(stderr, "If a cpu can't be added to another cpupool, add it
> to '%s' again and retry.\n", pool);
> +}
>
> rc = EXIT_SUCCESS;
>
> --
> 2.6.2
>
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] xen: Restore p2m_access_t enum order to allow bitmask semantics
On Tue, Mar 8, 2016 at 4:52 PM, Jan Beulich wrote:
> >>> On 08.03.16 at 16:30, wrote:
> > Nested hap code assumed implict bitmask semantics of the p2m_access_t
> > enum prior to C/S 4c63692d7c38c5ac414fe97f8ef37b66e05abe5c
> >
> > The change to the enum ordering broke this assumption and caused
> functional
> > problems for the nested hap code. As it may be error prone to audit and
> find
> > all other p2m_access users assuming bitmask semantics, instead restore
> the
> > previous enum order and make it explict that bitmask semantics are to be
> > preserved for the read, write and execute access types.
>
> Makes sense, but how certain are you that ...
>
> > --- a/xen/include/xen/p2m-common.h
> > +++ b/xen/include/xen/p2m-common.h
> > @@ -15,14 +15,15 @@
> > * default.
> > */
> > typedef enum {
> > -p2m_access_rwx = 0, /* The default access type when not used. */
>
> ... namely this has not meanwhile seen any implicit use somewhere?
>
> Tamas, the original commit talked about this as an optimization only.
> Can you confirm that there indeed was no other intention than the
> one claimed in that commit's description?
>
That's the only reason I recall as well, so from my perspective it's fine
to be reverted.
Tamas
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 3/3] xl: new "loglvl" command
On Tue, Mar 08, 2016 at 02:05:01PM +, George Dunlap wrote:
> On Tue, Mar 8, 2016 at 8:08 AM, Jan Beulich wrote:
> On 07.03.16 at 19:07, wrote:
> >> On Mon, 2016-03-07 at 04:46 -0700, Jan Beulich wrote:
> >>> > > > On 04.03.16 at 19:45, wrote:
> >>> > On Fri, 2016-03-04 at 09:48 -0700, Jan Beulich wrote:
> >>
> >>> > > --- a/tools/libxl/libxl.c
> >>> > > +++ b/tools/libxl/libxl.c
> >>> > > @@ -5958,6 +5958,26 @@ int libxl_send_debug_keys(libxl_ctx *ctx
> >>> > > return 0;
> >>> > > }
> >>> > >
> >>> > > +int libxl_log_level(libxl_ctx *ctx, bool set, bool guest,
> >>> > > +int *lower_thresh, int *upper_thresh)
> >>> > > +{
> >>> > > +int ret;
> >>> > >
> >>> > As per libxl coding style, this wants to be 'r'.
> >>> This and everything else below look to be valid comments, but
> >>> it's rather frustrating that simply cloning an existing function (I
> >>> user the debug key ones as basis) doesn't give me valid code,
> >>> the more that I did scroll up and down a few pages to see
> >>> whether I just happened to pick a particularly bad example.
> >>>
> >> Hehe, but do you understand that, saying this, you're making it very
> >> likely that people will ask *you* to fix libxl_send_debug_keys() --and
> >> perhaps more tool side code? :-P :-P
> >
> > Except that it's not just that function - as said, I did scroll up and
> > down, without finding (style wise) better examples. And no, I'm
> > not going to put together patches to deal with style issues in the
> > tools.
> >
> >> No, jokes apart, I agree that inconsistency is a real bad thing... but
> >> it's an hard fight, and we do have examples spread all around the
> >> source code (both Xen and tools), AFAICT.
> >
> > Right, and asking people myself to not follow bad examples when
> > adding new code, I did take all of your input to adjust the patch.
> > Just that in this case the set of bad examples is so large that in a
> > similar case in the hypervisor I probably wouldn't have dared to
> > ask for a style correction.
>
> Well the approach of the libxl maintainers seems to have be, "Just
> make sure the new code adheres to the new style, and eventyally
> everything will be up-to-date". A few releases ago I submitted a
> patch where I added a new clause in the middle of a series of other
> very similar clauses, and I was asked to make the new clause follow
> the new style, but to leave the other clauses right next to it in the
> old style (to avoid unnecessary code churn, since it was during the
> development freeze).
>
> Given that the "new" style has been around for a while now, it
> probably would be good to set aside some time at the beginning of the
> next development cycle to fix things up -- it is incredibly
> frustrating to carefully try to copy the surrounding style, only to be
> told to revise it.
>
I did fix a few hundred instances of inconsistency at the beginning of
4.7 cycle. Spatch is helpful, but it is far from perfect.
What I'm afraid of is that fixing them would introduce too much noise
that renders file line annotation useless.
Wei.
> -George
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 6/6] libxl: add force option for xl vcpu-pin
On 08/03/16 16:58, Wei Liu wrote: > On Thu, Mar 03, 2016 at 05:48:50PM +0100, Juergen Gross wrote: > [...] >> int libxl_set_vcpuaffinity_all(libxl_ctx *ctx, uint32_t domid, >> unsigned int max_vcpus, >> const libxl_bitmap *cpumap_hard, >> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h >> index f9e3ef5..19ec076 100644 >> --- a/tools/libxl/libxl.h >> +++ b/tools/libxl/libxl.h >> @@ -1715,6 +1715,10 @@ int libxl_get_physinfo(libxl_ctx *ctx, libxl_physinfo >> *physinfo); >> int libxl_set_vcpuaffinity(libxl_ctx *ctx, uint32_t domid, uint32_t vcpuid, >> const libxl_bitmap *cpumap_hard, >> const libxl_bitmap *cpumap_soft); >> +int libxl_set_vcpuaffinity_force(libxl_ctx *ctx, uint32_t domid, >> + uint32_t vcpuid, >> + const libxl_bitmap *cpumap_hard, >> + const libxl_bitmap *cpumap_soft); > > With the introduction of this new API you need to have a #define in > libxl.h Okay. Juergen ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 4/6] libxc: do some retries in xc_cpupool_removecpu() for EBUSY case
On 08/03/16 14:16, Dario Faggioli wrote:
> On Thu, 2016-03-03 at 17:48 +0100, Juergen Gross wrote:
>> The hypervisor might return EBUSY when trying to remove a cpu from a
>> cpupool when a domain running in this cpupool has pinned a vcpu
>> temporarily. Do some retries in this case, perhaps the situation
>> cleans up.
>>
> I now I'm at high risk of being called nitpicker (or, more likely, much
> worse names), but I think that:
>
>> --- a/tools/libxc/xc_cpupool.c
>> +++ b/tools/libxc/xc_cpupool.c
>> @@ -20,8 +20,11 @@
>> */
>>
>> #include
>> +#include
>> #include "xc_private.h"
>>
>> +#define LIBXC_BUSY_RETRIES 5
>> +
> This name makes me think about something which wants to be more generic
> than it is actually the case... Like some number of retries that libxc
> does in general, while it's only applicable to a very specific cpupool
> operation.
>
> Just something like CPUPOOL_NUM_REMOVECPU_RETRIES (or, maybe, even
> without the CPUPOOL_ prefix, as we're already inside cpupool.c) would
> be more appropriate.
>
> I'd also define it closer to xc_cpupool_removecpu() (but that is a lot
> about personal taste, I guess) and would add a brief comment
> (basically, a summary of what's in the changelog already), if only to
> save people having to go through `git blame'.
>
>> @@ -141,13 +144,21 @@ int xc_cpupool_removecpu(xc_interface *xch,
>> uint32_t poolid,
>> int cpu)
>> {
>> +unsigned retries;
>> +int err;
>> DECLARE_SYSCTL;
>>
>> sysctl.cmd = XEN_SYSCTL_cpupool_op;
>> sysctl.u.cpupool_op.op = XEN_SYSCTL_CPUPOOL_OP_RMCPU;
>> sysctl.u.cpupool_op.cpupool_id = poolid;
>> sysctl.u.cpupool_op.cpu = (cpu < 0) ? XEN_SYSCTL_CPUPOOL_PAR_ANY
>> : cpu;
>> -return do_sysctl_save(xch, &sysctl);
>> +for ( retries = 0; retries < LIBXC_BUSY_RETRIES; retries++ ) {
>> +err = do_sysctl_save(xch, &sysctl);
>> +if ( err >= 0 || errno != EBUSY )
>> +break;
>> +sleep(1);
>> +}
>>
> Doing this the other way round (basically, exactly as the same thing is
> done in do_sysctl_save() already), reads, IMHO, more natural:
>
> for (...) {
>err = do_sysctl_save(..);
>if ( err < 0 && errno == EBUSY )
> sleep(1);
>else
> break;
> }
>
> But yeah, this really is nitpicking. :-)
Nevertheless I can do it. Need to respin anyway.
Juergen
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 2/3] libxc: wrapper for log level sysctl
On Fri, Mar 04, 2016 at 09:47:32AM -0700, Jan Beulich wrote: > Signed-off-by: Jan Beulich > Acked-by: Wei Liu ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 2/2] xsm: move FLASK_AVC_STATS to Kconfig
On 03/08/2016 04:46 AM, Jan Beulich wrote: On 07.03.16 at 19:42, wrote: Have Kconfig set CONFIG_FLASK_AVC_STATS and prefix all uses with CONFIG_ to use the Kconfig variable. Same question here: What's the benefit of doing it this way? This removes the stats tracking, which might (I have not tested) speed up the security server by avoiding the __get_cpu_var call and increment. The corresponding SELinux knob is a Kconfig option in Linux. Acked-by: Daniel De Graaf ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 1/2] xsm: move the XSM_MAGIC value to Kconfig
On 03/07/2016 01:42 PM, Doug Goldstein wrote: Let Kconfig set the XSM_MAGIC value for us. Signed-off-by: Doug Goldstein This is not the best place to define this constant: it doesn't make sense for it to be user-configurable. If you want to move it out of config.h, I think the best solution is to define XSM_MAGIC inside xsm/xsm.h depending on the value of CONFIG_FLASK. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 3/3] xl: new "loglvl" command
On Fri, Mar 04, 2016 at 09:48:16AM -0700, Jan Beulich wrote:
> This is pretty simplistic for now, but I'd rather have someone better
> friends with the tools improve it (if desired).
>
> Signed-off-by: Jan Beulich
>
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -5958,6 +5958,26 @@ int libxl_send_debug_keys(libxl_ctx *ctx
> return 0;
> }
>
> +int libxl_log_level(libxl_ctx *ctx, bool set, bool guest,
> +int *lower_thresh, int *upper_thresh)
> +{
> +int ret;
> +GC_INIT(ctx);
> +if (set) {
> +ret = xc_set_log_level(ctx->xch, guest, *lower_thresh,
> *upper_thresh);
> +} else {
> +ret = xc_get_log_level(ctx->xch, guest, lower_thresh, upper_thresh);
> +}
> +if ( ret < 0 ) {
> +LOGE(ERROR, "%s %slog level",
> + set ? "setting" : "getting", guest ? "guest " : "");
> +GC_FREE;
> +return ERROR_FAIL;
> +}
> +GC_FREE;
> +return 0;
> +}
> +
As Dario said, libxl tends to have getter and setter pair.
> libxl_xen_console_reader *
> libxl_xen_console_read_start(libxl_ctx *ctx, int clear)
> {
[...]
>
> /*
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -6469,6 +6469,84 @@ int main_debug_keys(int argc, char **arg
> return 0;
> }
>
> +static const struct {
> +int level;
> +char string[8];
> +} loglvls[] = {
> +{ 0, "none" },
> +{ 1, "error" },
> +{ 2, "warning" },
> +{ 3, "info" },
> +{ 4, "all" },
> +{ 4, "debug" },
The semantics of the numbers should go into libxl_types.idl. Maybe
something like
# Keep in sync with Xen log level.
libxl_xen_log_level = Enumeration (...);
Then in here
static const struct {
int level;
char string[8];
} loglvls[] = {
{ LIBXL_XEN_LOG_LEVEL_NONE, "none" },
{ ..., "error" },
{ ..., "warning" },
{ ..., "info" },
{ ..., "all" },
{ ..., "debug" },
Please also note that after the introduction of this API, Xen log level
will become part of the stable API and subject to some compatibility
constraints. Is this what you wanted?
> +};
> +
> +static int parse_loglvl(char **parg)
> +{
> +unsigned int i;
> +
> +for (i = 0; i < ARRAY_SIZE(loglvls); ++i) {
> +size_t l = strlen(loglvls[i].string);
> +
> +if (!strncmp(*parg, loglvls[i].string, l)) {
> +*parg += l;
> +return loglvls[i].level;
> +}
> +}
> +
> +return -1;
> +}
> +
> +static const char *format_loglvl(int loglvl)
> +{
> +unsigned int i;
> +
> +for (i = 0; i < ARRAY_SIZE(loglvls); ++i) {
> +if (loglvl == loglvls[i].level)
> +return loglvls[i].string;
> +}
> +
> +return "";
> +}
> +
> +int main_loglvl(int argc, char **argv)
> +{
> +static const struct option opts[] = {
> +{"guest", 0, 0, 'g'},
> +{"set", 0, 0, 's'},
> +COMMON_LONG_OPTS
> +};
> +int opt, lower_thresh = -1, upper_thresh = -1;
> +bool guest = false, set = false;
> +
> +SWITCH_FOREACH_OPT(opt, "gs:", opts, "loglvl", 0) {
> +case 'g':
> +guest = true;
> +break;
> +case 's':
> +if (*optarg != '/')
> +lower_thresh = parse_loglvl(&optarg);
> +if (*optarg == '/') {
> +++optarg;
> +upper_thresh = parse_loglvl(&optarg);
> +}
> +set = true;
> +break;
> +}
> +
> +if (libxl_log_level(ctx, set, guest, &lower_thresh, &upper_thresh)) {
> +fprintf(stderr, "cannot %s %s log level\n",
> +set ? "set" : "get", guest ? "guest" : "host");
> +return 1;
> +}
> +
> +if (!set)
> +printf("%s log levels: %s/%s\n", guest ? "guest" : "host",
> + format_loglvl(lower_thresh), format_loglvl(upper_thresh));
> +
> +return 0;
> +}
> +
You also need to patch xl manpage.
Wei.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 2/2] tools: detect appropriate debug optimization level
On 3/8/16 9:38 AM, Wei Liu wrote: > On Mon, Mar 07, 2016 at 08:23:40PM -0600, Doug Goldstein wrote: >> The build should not use -O0 as that results in miscompilations. There > > This needs some (concrete) references. Is that a known issue in gcc? If > so can you reference the bug number? So its not really a bug in GCC but just the complete lack of optimizations in play. inlines aren't inlined. dead code elimination isn't run so things are much bigger. structures aren't padded the same way. This came about from reading reports on the -devel and -user's ML that were solved by building Xen with debug=n. I was also striving to reduce the duplication of CFLAGS that are passed on the command line of builds. > >> have been a few instances on the ML where users were told to switch >> from -O0 to -O1 or -O2 or to set debug=n and their issue went away. The >> preferred route should be to use -Og if its available, otherwise use >> -O1 which is the default. This change undoes the change from -O1 to -O0 > > gcc manual says -O0 is the default. I wasn't clear about where the 'the default' came from. That's the default in the Xen tree (see: config/StdGNU.mk for example but every platform has -O1 set). > > Not that I disagree with this patch in general, but the commit message > seems a bit misleading. I can rewrite it. I'd also be willing to change the patch to prefer -Og if its available and use -O0 if its not. > >> in 1166ecf781b1016eaa61f8d5ba4fb1fde9d599b6. >> > > And I have no idea why -O1 confuses the debugger so I've CC'ed Euan for > more input. -O1 can optimize things out when you look at them with gdb but -Og is suppose to do the right thing. > >> Signed-off-by: Doug Goldstein >> --- >> CC: Ian Jackson >> CC: Stefano Stabellini >> CC: Wei Liu >> --- >> tools/Rules.mk | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/tools/Rules.mk b/tools/Rules.mk >> index 9ef0b47..ae6b01f 100644 >> --- a/tools/Rules.mk >> +++ b/tools/Rules.mk >> @@ -137,7 +137,8 @@ SHLIB_libxenvchan = $(SHDEPS_libxenvchan) >> -Wl,-rpath-link=$(XEN_LIBVCHAN) >> >> ifeq ($(debug),y) >> # Disable optimizations and enable debugging information for macros >> -CFLAGS += -O0 -g3 >> +$(call cc-option-add,CFLAGS,CC,-Og) >> +CFLAGS += -g3 >> # But allow an override to -O0 in case Python enforces >> -D_FORTIFY_SOURCE=. >> PY_CFLAGS += $(PY_NOOPT_CFLAGS) >> endif >> -- >> 2.4.10 >> -- Doug Goldstein signature.asc Description: OpenPGP digital signature ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [libvirt] [PATCH V2 1/4] conf: add 'state' attribute to feature
On 03/01/2016 04:00 AM, Jim Fehlig wrote:
> Most hypervisors use Hardware Assisted Paging by default and don't
> require specifying the feature in domain conf. But some hypervisors
> support disabling HAP on a per-domain basis. To enable HAP by default
> yet provide a knob to disable it, extend the feature with a
> 'state=on|off' attribute, similar to and features.
>
> In the absence of , the hypervisor default (on) is used.
> without the state attribute would be the same as for
> backwards compatibility. And of course disables hap.
>
> Signed-off-by: Jim Fehlig
> ---
> docs/formatdomain.html.in | 6 --
> docs/schemas/domaincommon.rng | 6 +-
> src/conf/domain_conf.c| 4 ++--
> 3 files changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 5016772..c06bcf3 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -1494,8 +1494,10 @@
>Interrupt) for the guest.
>
>hap
> - Enable use of Hardware Assisted Paging if available in
> -the hardware.
> + Depending on the state attribute (values
> on,
> +off) enable or disable use of Hardware Assisted Paging.
> +The default is on if the hypervisor detects availability
> +of Hardware Assisted Paging.
>
>viridian
>Enable Viridian hypervisor extensions for paravirtualizing
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 67af93a..dd6e93a 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -4108,7 +4108,11 @@
>
>
>
> -
> +
> +
> +
> +
> +
Perhaps would be better (see chunk below) ? That one
appears to be a reference of what you are adding above, and it's the same as
pvspinlock. Though some other elements don't appear to use this, not sure why.
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 89d3a6b..141122c 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -4132,9 +4132,7 @@
-
-
-
+
Other that,
Reviewed-by: Joao Martins
>
>
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 79758d4..714bbfc 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -15296,7 +15296,6 @@ virDomainDefParseXML(xmlDocPtr xml,
> /* fallthrough */
> case VIR_DOMAIN_FEATURE_ACPI:
> case VIR_DOMAIN_FEATURE_PAE:
> -case VIR_DOMAIN_FEATURE_HAP:
> case VIR_DOMAIN_FEATURE_VIRIDIAN:
> case VIR_DOMAIN_FEATURE_PRIVNET:
> case VIR_DOMAIN_FEATURE_HYPERV:
> @@ -15321,6 +15320,7 @@ virDomainDefParseXML(xmlDocPtr xml,
> ctxt->node = node;
> break;
>
> +case VIR_DOMAIN_FEATURE_HAP:
> case VIR_DOMAIN_FEATURE_PMU:
> case VIR_DOMAIN_FEATURE_PVSPINLOCK:
> case VIR_DOMAIN_FEATURE_VMPORT:
> @@ -22043,7 +22043,6 @@ virDomainDefFormatInternal(virDomainDefPtr def,
> switch ((virDomainFeature) i) {
> case VIR_DOMAIN_FEATURE_ACPI:
> case VIR_DOMAIN_FEATURE_PAE:
> -case VIR_DOMAIN_FEATURE_HAP:
> case VIR_DOMAIN_FEATURE_VIRIDIAN:
> case VIR_DOMAIN_FEATURE_PRIVNET:
> switch ((virTristateSwitch) def->features[i]) {
> @@ -22065,6 +22064,7 @@ virDomainDefFormatInternal(virDomainDefPtr def,
>
> break;
>
> +case VIR_DOMAIN_FEATURE_HAP:
> case VIR_DOMAIN_FEATURE_PMU:
> case VIR_DOMAIN_FEATURE_PVSPINLOCK:
> case VIR_DOMAIN_FEATURE_VMPORT:
>
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [libvirt] [PATCH V2 2/4] xenconfig: change 'hap' setting to align with Xen behavior
On 03/01/2016 04:00 AM, Jim Fehlig wrote:
> hap is enabled by default in xm and xl config and usually only
> specified when it is desirable to disable hap (hap = 0). Change
> the xm,xl <-> xml converter to behave similarly. I.e. only
> produce 'hap = 0' when and vice versa.
>
> Signed-off-by: Jim Fehlig
> ---
> src/xenconfig/xen_common.c | 14 ++---
> .../test-disk-positional-parms-full.cfg| 1 -
> .../test-disk-positional-parms-partial.cfg | 1 -
> ...est-fullvirt-direct-kernel-boot-bogus-extra.cfg | 1 -
> .../test-fullvirt-direct-kernel-boot-extra.cfg | 1 -
> .../test-fullvirt-direct-kernel-boot.cfg | 1 -
> tests/xlconfigdata/test-fullvirt-multiusb.cfg | 1 -
> tests/xlconfigdata/test-fullvirt-nohap.cfg | 26 ++
> tests/xlconfigdata/test-fullvirt-nohap.xml | 59
> ++
> tests/xlconfigdata/test-new-disk.cfg | 1 -
> tests/xlconfigdata/test-rbd-multihost-noauth.cfg | 1 -
> tests/xlconfigdata/test-spice-features.cfg | 1 -
> tests/xlconfigdata/test-spice.cfg | 1 -
> tests/xlconfigdata/test-vif-rate.cfg | 1 -
> tests/xlconfigtest.c | 1 +
> tests/xmconfigdata/test-escape-paths.cfg | 1 -
> .../xmconfigdata/test-fullvirt-default-feature.cfg | 1 -
> tests/xmconfigdata/test-fullvirt-force-hpet.cfg| 1 -
> tests/xmconfigdata/test-fullvirt-force-nohpet.cfg | 1 -
> tests/xmconfigdata/test-fullvirt-localtime.cfg | 1 -
> tests/xmconfigdata/test-fullvirt-net-netfront.cfg | 1 -
> tests/xmconfigdata/test-fullvirt-new-cdrom.cfg | 1 -
> tests/xmconfigdata/test-fullvirt-nohap.cfg | 28 ++
> tests/xmconfigdata/test-fullvirt-nohap.xml | 51 +++
> tests/xmconfigdata/test-fullvirt-parallel-tcp.cfg | 1 -
> .../test-fullvirt-serial-dev-2-ports.cfg | 1 -
> .../test-fullvirt-serial-dev-2nd-port.cfg | 1 -
> tests/xmconfigdata/test-fullvirt-serial-file.cfg | 1 -
> tests/xmconfigdata/test-fullvirt-serial-null.cfg | 1 -
> tests/xmconfigdata/test-fullvirt-serial-pipe.cfg | 1 -
> tests/xmconfigdata/test-fullvirt-serial-pty.cfg| 1 -
> tests/xmconfigdata/test-fullvirt-serial-stdio.cfg | 1 -
> .../test-fullvirt-serial-tcp-telnet.cfg| 1 -
> tests/xmconfigdata/test-fullvirt-serial-tcp.cfg| 1 -
> tests/xmconfigdata/test-fullvirt-serial-udp.cfg| 1 -
> tests/xmconfigdata/test-fullvirt-serial-unix.cfg | 1 -
> tests/xmconfigdata/test-fullvirt-sound.cfg | 1 -
> tests/xmconfigdata/test-fullvirt-usbmouse.cfg | 1 -
> tests/xmconfigdata/test-fullvirt-usbtablet.cfg | 1 -
> tests/xmconfigdata/test-fullvirt-utc.cfg | 1 -
> tests/xmconfigdata/test-no-source-cdrom.cfg| 1 -
> tests/xmconfigdata/test-pci-devs.cfg | 1 -
> tests/xmconfigtest.c | 1 +
> 43 files changed, 173 insertions(+), 43 deletions(-)
>
> diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c
> index 828c8e9..4dcd484 100644
> --- a/src/xenconfig/xen_common.c
> +++ b/src/xenconfig/xen_common.c
> @@ -528,11 +528,11 @@ xenParseCPUFeatures(virConfPtr conf, virDomainDefPtr
> def)
>
> else if (val)
> def->features[VIR_DOMAIN_FEATURE_APIC] = VIR_TRISTATE_SWITCH_ON;
> -if (xenConfigGetBool(conf, "hap", &val, 0) < 0)
> +if (xenConfigGetBool(conf, "hap", &val, 1) < 0)
> return -1;
>
> -else if (val)
> -def->features[VIR_DOMAIN_FEATURE_HAP] = VIR_TRISTATE_SWITCH_ON;
> +else if (!val)
> +def->features[VIR_DOMAIN_FEATURE_HAP] = VIR_TRISTATE_SWITCH_OFF;
> if (xenConfigGetBool(conf, "viridian", &val, 0) < 0)
> return -1;
>
> @@ -1572,10 +1572,10 @@ xenFormatCPUFeatures(virConfPtr conf, virDomainDefPtr
> def)
> VIR_TRISTATE_SWITCH_ON) ? 1 : 0) < 0)
> return -1;
>
> -if (xenConfigSetInt(conf, "hap",
> -(def->features[VIR_DOMAIN_FEATURE_HAP] ==
> - VIR_TRISTATE_SWITCH_ON) ? 1 : 0) < 0)
> -return -1;
> +if (def->features[VIR_DOMAIN_FEATURE_HAP] ==
> VIR_TRISTATE_SWITCH_OFF) {
> +if (xenConfigSetInt(conf, "hap", 0) < 0)
> +return -1;
> +}
>
> if (xenConfigSetInt(conf, "viridian",
> (def->features[VIR_DOMAIN_FEATURE_VIRIDIAN] ==
> diff --git a/tests/xlconfigdata/test-disk-positional-parms-full.cfg
> b/tests/xlconfigdata/test-disk-positional-parms-full.cfg
> index 026e451..c5bbb03 100644
> --- a/tests/xlconfigdata/test-disk-positional-parms-full.cfg
> +++ b/tests/xlconfigdata/test-disk-positional-parms-full.cfg
> @@ -6,7 +6,6 @@ vcpus = 1
> pae = 1
> acpi = 1
> apic = 1
> -hap = 0
> viridian = 0
> rtc_timeoffset = 0
>
Re: [Xen-devel] [libvirt] [PATCH V2 4/4] libxl: support enabling and disabling feature
On 03/01/2016 04:00 AM, Jim Fehlig wrote: > Until now, the libxl driver ignored any setting in domain XML > and deferred to libxl, which enables hap if not specified. While > this is a good default, it prevents disabling hap if desired. > > This change allows disabling hap with . hap is > explicitly enabled with or
Re: [Xen-devel] [libvirt] [PATCH V2 3/4] Xen drivers: show hap enabled by default in capabilities
On 03/01/2016 04:00 AM, Jim Fehlig wrote: > Hardware Assisted Paging is enabled by default in Xen. Change > the capabilities output to reflect this. > > Signed-off-by: Jim Fehlig > --- > src/libxl/libxl_conf.c | 2 +- > src/xen/xen_hypervisor.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c > index 93c943b..6efd9b5 100644 > --- a/src/libxl/libxl_conf.c > +++ b/src/libxl/libxl_conf.c > @@ -493,7 +493,7 @@ libxlCapsInitGuests(libxl_ctx *ctx, virCapsPtr caps) > > if (virCapabilitiesAddGuestFeature(guest, > "hap", > - 0, > + 1, > 1) == NULL) > return -1; > } > diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c > index c1834cb..fc9e1c6 100644 > --- a/src/xen/xen_hypervisor.c > +++ b/src/xen/xen_hypervisor.c > @@ -2206,7 +2206,7 @@ xenHypervisorBuildCapabilities(virConnectPtr conn, > virArch hostarch, > if ((hv_major == 3 && hv_minor >= 3) || (hv_major > 3)) > if (virCapabilitiesAddGuestFeature(guest, > "hap", > - false, > + true, > true) == NULL) > goto no_memory; > > For the libxl part, Reviewed-by: Joao Martins ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 4/4] arm64: update the introduction of xen boot commands in docs/grub.texi
Hi Andrei,
On 8 March 2016 at 14:54, Andrei Borzenkov wrote:
> 07.03.2016 11:22, Fu Wei пишет:
>> Hi Andrei,
>>
>> On 28 February 2016 at 00:44, Fu Wei wrote:
>>> Hi Andrei
>>>
>>> On 28 February 2016 at 01:26, Andrei Borzenkov wrote:
26.02.2016 14:13, fu@linaro.org пишет:
> From: Fu Wei
>
> delete: xen_linux, xen_initrd, xen_xsm
> add: xen_module
>
> This update bases on
> commit 0edd750e50698854068358ea53528100a9192902
> Author: Vladimir Serbinenko
> Date: Fri Jan 22 10:18:47 2016 +0100
>
> xen_boot: Remove obsolete module type distinctions.
>
> Signed-off-by: Fu Wei
> ---
> docs/grub.texi | 33 ++---
> 1 file changed, 10 insertions(+), 23 deletions(-)
>
> diff --git a/docs/grub.texi b/docs/grub.texi
> index 82f6fa4..3fbdd99 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -3861,9 +3861,7 @@ you forget a command, you can run the command
> @command{help}
> * videoinfo:: List available video modes
> @comment * xen_*:: Xen boot commands
> * xen_hypervisor:: Load xen hypervisor binary
> -* xen_linux:: Load dom0 kernel for xen hypervisor
> -* xen_initrd:: Load dom0 initrd for dom0 kernel
> -* xen_xsm:: Load xen security module for xen
> hypervisor
> +* xen_module:: Load xen modules for xen hypervisor
> @end menu
>
>
> @@ -5141,30 +5139,19 @@ verbatim as the @dfn{kernel command-line}. Any
> other binaries must be
> reloaded after using this command.
> @end deffn
>
> -@node xen_linux
> -@subsection xen_linux
> +@node xen_module
> +@subsection xen_module
>
> -@deffn Command xen_linux file [arguments]
> -Load a dom0 kernel image for xen hypervisor at the booting process of
> xen.
> +@deffn Command xen_module [--nounzip] file [arguments]
> +Load a module for xen hypervisor at the booting process of xen.
> The rest of the line is passed verbatim as the module command line.
==
> +On i386, the modules will be identified by Multiboot(2) protocol.
> +On arm64, each module will be identified by the order in which the
> +modules are added.
I think it is better to skip it entirely. It is not really correct -
neither multiboot protocol provides any module identification (Xen
probes module types), nor is i386 using multiboot2, nor can all modules
be probed, so order still matters. To avoid confusion I'd simply
replaced the above three lines with
Modules should be loaded in the following order:
> +The 1st module: dom0 kernel image
> +The 2nd module: dom0 ramdisk (optional)
This covers both supported platforms without going into too deep
details; if you and Vladimir are OK, I'll commit with this change.
>>>
>>> Thank you very much!
>>> Sorry I am not familiar with xen on i386, so maybe I misunderstand this.
>>> So please commit with your change, Thanks for your correction :-)
>>
>> I just fetched the mainline GRUB, i would like to know why this
>> patchset haven't been applied?
>> Anything I need to do(improve it or post a new patchset according to
>> your suggestion) for this patchset?
>>
>
> Sorry for delay. It is not really about your patchset, but we need some
> decision about loading additional modules/lack of initrd on ARM. Until
> then I'd rather avoid committing to any high-level configuration support
> that will require even more backward compatible hacks later.
>
> As it stands now either Xen needs to support autodetection or we need to
> revert to providing module type explicitly.
So speaking of loading additional modules/lack of initrd on ARM, I thinks that
will (only) affect loading XSM.
For this, I have discussed of that with Julien, I think :
(1) the first module must be kernel
(2) the second module must be initrd, if we have initrd
(3) Start from the 2nd module, XEN will detect that if the module is a XSM by
the XSM binary signature. if we get XSM as the second module, that
means we have not initrd.
please correct me if I misunderstand it
:-)
--
Best regards,
Fu Wei
Software Engineer
Red Hat Software (Beijing) Co.,Ltd.Shanghai Branch
Ph: +86 21 61221326(direct)
Ph: +86 186 2020 4684 (mobile)
Room 1512, Regus One Corporate Avenue,Level 15,
One Corporate Avenue,222 Hubin Road,Huangpu District,
Shanghai,China 200021
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v11 03/27] tools/libxl: Add back channel to allow migration target send data back
On Fri, Mar 04, 2016 at 04:38:23PM +, Ian Jackson wrote:
> Changlong Xie writes ("[PATCH v11 03/27] tools/libxl: Add back channel to
> allow migration target send data back"):
> > From: Wen Congyang
> >
> > In COLO mode, secondary needs to send the following data to primary:
> > 1. In libxl
> >Secondary sends the following CHECKPOINT_CONTEXT to primary:
> >CHECKPOINT_SVM_SUSPENDED, CHECKPOINT_SVM_READY and CHECKPOINT_SVM_RESUMED
> > 2. In libxc
> >Secondary sends the dirty pfn list to primary
>
> The overall API approach here seems good to me.
>
> But, my reading of the code is that this new fd is currently ignored.
> This is, AFAICT, intentional in the non-colo case, and we have no colo
> case yet.
>
> So I think that this new parameter needs to be slightly better
> documented. I suggest:
>
> * In this patch, add a comment next to it saying "always pass -1".
> * In the patch were the fd actually starts to do something, change
> this comment to something more meaningful.
>
> > /*
> > + * LIBXL_HAVE_DOMAIN_CREATE_RESTORE_SEND_BACK_FD 1
> > + *
> > + * If this is defined, libxl_domain_create_restore()'s API has changed to
> > + * include a send_back_fd param which used for libxl migration back channel
> > + * during COLO.
> > + */
> > +#define LIBXL_HAVE_DOMAIN_CREATE_RESTORE_SEND_BACK_FD 1
>
> I have a minor grammar quibble with this. I would write:
>
> "If this is defined, libxl_domain_create_restore()'s API
>includes the send_back_fd param. This is used only with
>COLO, for the libxl migration back channel; other callers
>should pass -1."
>
> And, with this definition of the API, I think that the code should
> actually check that -1 is passed. Personally I would be happy with
> the error case either failing assert() or returning ERROR_INVAL, but
> maybe other maintainers have a specific view.
>
I have no preference on this issue. Either calling assert or
returning ERROR_INVAL is fine by me.
Wei.
> Ian.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v11 07/27] docs/libxl: Introduce CHECKPOINT_CONTEXT to support migration v2 colo streams
On Fri, Mar 04, 2016 at 04:51:20PM +, Ian Jackson wrote:
[...]
>
> > @@ -212,6 +214,11 @@ class VerifyLibxl(VerifyBase):
> > if len(content) != 0:
> > raise RecordError("Checkpoint end record with non-zero length")
> >
> > +def verify_record_checkpoint_state(self, content):
> > +""" Checkpoint state """
> > +if len(content) == 0:
> > +raise RecordError("Checkpoint state record with zero length")
> > +
>
> I'm not verify familiar with this area of the code, but I think that
> this should probably check that the control_id is as expected. Can it
> know what the right sequencing is ?
>
FWIW this script is not used in live system -- so it probably doesn't
have information on the control id and the sequence on a live system.
Wei.
> Ian.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v11 19/27] COLO: introduce new API to prepare/start/do/get_error/stop replication
On Fri, Mar 04, 2016 at 05:26:44PM +, Ian Jackson wrote:
> Changlong Xie writes ("[PATCH v11 19/27] COLO: introduce new API to
> prepare/start/do/get_error/stop replication"):
> > From: Wen Congyang
> >
> > We will use qemu block replication, and qemu provides some qmp commands
> > to prepare replication, start replication, get replication error, and
> > stop replication. Introduce new API to execute these qmp commands.
>
> How will this work if in future we want to support HVM (or
> hvm-lite-ng) guests ?
>
Just to clarify things: all the new functions in this patch are internal
to libxl. So they have no implication on how PVHv2 COLO is implemented
-- it probably won't be using all these functions anyway.
Wei.
> > diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> > index 5160939..8cb9f19 100644
> > --- a/tools/libxl/libxl_internal.h
> > +++ b/tools/libxl/libxl_internal.h
> > @@ -1771,6 +1771,26 @@ _hidden int
> > libxl__qmp_set_global_dirty_log(libxl__gc ...
> > +_hidden int libxl__qmp_nbd_server_add(libxl__gc *gc, int domid, const char
> > *disk);
>
> It's a tiny nit, but can I grumble about the long lines here ? Less
> than ~70-75 characters is best.
>
> Thanks,
> Ian.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 2/2] tools: detect appropriate debug optimization level
On Tue, Mar 08, 2016 at 10:34:42AM -0600, Doug Goldstein wrote: > On 3/8/16 9:38 AM, Wei Liu wrote: > > On Mon, Mar 07, 2016 at 08:23:40PM -0600, Doug Goldstein wrote: > >> The build should not use -O0 as that results in miscompilations. There > > > > This needs some (concrete) references. Is that a known issue in gcc? If > > so can you reference the bug number? > > So its not really a bug in GCC but just the complete lack of > optimizations in play. inlines aren't inlined. dead code elimination > isn't run so things are much bigger. structures aren't padded the same way. > Urgh... > This came about from reading reports on the -devel and -user's ML that > were solved by building Xen with debug=n. I was also striving to reduce > the duplication of CFLAGS that are passed on the command line of builds. > I agree this is a good idea. > > > >> have been a few instances on the ML where users were told to switch > >> from -O0 to -O1 or -O2 or to set debug=n and their issue went away. The > >> preferred route should be to use -Og if its available, otherwise use > >> -O1 which is the default. This change undoes the change from -O1 to -O0 > > > > gcc manual says -O0 is the default. > > I wasn't clear about where the 'the default' came from. That's the > default in the Xen tree (see: config/StdGNU.mk for example but every > platform has -O1 set). > OK. I thought you're talking about something in the manual. > > > > Not that I disagree with this patch in general, but the commit message > > seems a bit misleading. > > I can rewrite it. I'd also be willing to change the patch to prefer -Og > if its available and use -O0 if its not. > No need to do it now because ... > > > >> in 1166ecf781b1016eaa61f8d5ba4fb1fde9d599b6. > >> > > > > And I have no idea why -O1 confuses the debugger so I've CC'ed Euan for > > more input. > > -O1 can optimize things out when you look at them with gdb but -Og is > suppose to do the right thing. > .. I don't know much about gcc so I would like to wait for Ian to give some input. Wei. > > > >> Signed-off-by: Doug Goldstein > >> --- > >> CC: Ian Jackson > >> CC: Stefano Stabellini > >> CC: Wei Liu > >> --- > >> tools/Rules.mk | 3 ++- > >> 1 file changed, 2 insertions(+), 1 deletion(-) > >> > >> diff --git a/tools/Rules.mk b/tools/Rules.mk > >> index 9ef0b47..ae6b01f 100644 > >> --- a/tools/Rules.mk > >> +++ b/tools/Rules.mk > >> @@ -137,7 +137,8 @@ SHLIB_libxenvchan = $(SHDEPS_libxenvchan) > >> -Wl,-rpath-link=$(XEN_LIBVCHAN) > >> > >> ifeq ($(debug),y) > >> # Disable optimizations and enable debugging information for macros > >> -CFLAGS += -O0 -g3 > >> +$(call cc-option-add,CFLAGS,CC,-Og) > >> +CFLAGS += -g3 > >> # But allow an override to -O0 in case Python enforces > >> -D_FORTIFY_SOURCE=. > >> PY_CFLAGS += $(PY_NOOPT_CFLAGS) > >> endif > >> -- > >> 2.4.10 > >> > > > -- > Doug Goldstein > ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 2/2] xsm: move FLASK_AVC_STATS to Kconfig
>>> On 08.03.16 at 17:22, wrote: > On 03/08/2016 04:46 AM, Jan Beulich wrote: > On 07.03.16 at 19:42, wrote: >>> Have Kconfig set CONFIG_FLASK_AVC_STATS and prefix all uses with CONFIG_ >>> to use the Kconfig variable. >> >> Same question here: What's the benefit of doing it this way? > > This removes the stats tracking, which might (I have not tested) speed up > the security server by avoiding the __get_cpu_var call and increment. No, I don not think the patch removes anything. The Kconfig option doesn't have a prompt. But anyway, ... > The > corresponding SELinux knob is a Kconfig option in Linux. > > Acked-by: Daniel De Graaf ... if you're fine with it, we'll put it in (once the mechanical issues got addressed). Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] Ideas Re: [PATCH v14 1/2] vmx: VT-d posted-interrupt core logic handling
On 08/03/16 15:42, Jan Beulich wrote: On 08.03.16 at 15:42, wrote: >> On Tue, Mar 8, 2016 at 1:10 PM, Wu, Feng wrote: -Original Message- From: George Dunlap [mailto:george.dun...@citrix.com] >> [snip] It seems like there are a couple of ways we could approach this: 1. Try to optimize the reverse look-up code so that it's not a linear linked list (getting rid of the theoretical fear) >>> >>> Good point. >>> 2. Try to test engineered situations where we expect this to be a problem, to see how big of a problem it is (proving the theory to be accurate or inaccurate in this case) >>> >>> Maybe we can run a SMP guest with all the vcpus pinned to a dedicated >>> pCPU, we can run some benchmark in the guest with VT-d PI and without >>> VT-d PI, then see the performance difference between these two sceanrios. >> >> This would give us an idea what the worst-case scenario would be. > > How would a single VM ever give us an idea about the worst > case? Something getting close to worst case is a ton of single > vCPU guests all temporarily pinned to one and the same pCPU > (could be multi-vCPU ones, but the more vCPU-s the more > artificial this pinning would become) right before they go into > blocked state (i.e. through one of the two callers of > arch_vcpu_block()), the pinning removed while blocked, and > then all getting woken at once. Why would removing the pinning be important? And I guess it's actually the case that it doesn't need all VMs to actually be *receiving* interrupts; it just requires them to be *capable* of receiving interrupts, for there to be a long chain all blocked on the same physical cpu. > >> But >> pinning all vcpus to a single pcpu isn't really a sensible use case we >> want to support -- if you have to do something stupid to get a >> performance regression, then I as far as I'm concerned it's not a >> problem. >> >> Or to put it a different way: If we pin 10 vcpus to a single pcpu and >> then pound them all with posted interrupts, and there is *no* >> significant performance regression, then that will conclusively prove >> that the theoretical performance regression is of no concern, and we >> can enable PI by default. > > The point isn't the pinning. The point is what pCPU they're on when > going to sleep. And that could involve quite a few more than just > 10 vCPU-s, provided they all sleep long enough. > > And the "theoretical performance regression is of no concern" is > also not a proper way of looking at it, I would say: Even if such > a situation would happen extremely rarely, if it can happen at all, > it would still be a security issue. What I'm trying to get at is -- exactly what situation? What actually constitutes a problematic interrupt latency / interrupt processing workload, how many vcpus must be sleeping on the same pcpu to actually risk triggering that latency / workload, and how feasible is it that such a situation would arise in a reasonable scenario? If 200us is too long, and it only takes 3 sleeping vcpus to get there, then yes, there is a genuine problem we need to try to address before we turn it on by default. If we say that up to 500us is tolerable, and it takes 100 sleeping vcpus to reach that latency, then this is something I don't really think we need to worry about. "I think something bad may happen" is a really difficult to work with. "I want to make sure that even a high number of blocked cpus won't cause the interrupt latency to exceed 500us; and I want it to be basically impossible for the interrupt latency to exceed 5ms under any circumstances" is a concrete target someone can either demonstrate that they meet, or aim for when trying to improve the situation. Feng: It should be pretty easy for you to: * Implement a modified version of Xen where - *All* vcpus get put on the waitqueue - Measure how long it took to run the loop in pi_wakeup_interrupt * Have one VM receiving posted interrupts on a regular basis. * Slowly increase the number of vcpus blocked on a single cpu (e.g., by creating more guests), stopping when you either reach 500us or 500 vcpus. :-) To report the measurements, you could either create a Xen trace record and use xentrace_format or xenalyze to plot the results; or you could create some software performance counters for different "buckets" -- less than 100us, 100-200us, 200-300us, 300-400us, 400-500us, and more than 500us. Or you could printk the min / average / max every 5000 interrupts or so. :-) To test, it seems like using a network benchmark with short packet lengths should be able to trigger large numbers of interrupts; and it also can let you know if / when there's a performance impact of adding more vcpus. Or alternately, you could try to come up with a quicker reverse-lookup algorithm. :-) -George ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 4/4] arm64: update the introduction of xen boot commands in docs/grub.texi
08.03.2016 19:37, Fu Wei пишет:
> Hi Andrei,
>
> On 8 March 2016 at 14:54, Andrei Borzenkov wrote:
>> 07.03.2016 11:22, Fu Wei пишет:
>>> Hi Andrei,
>>>
>>> On 28 February 2016 at 00:44, Fu Wei wrote:
Hi Andrei
On 28 February 2016 at 01:26, Andrei Borzenkov wrote:
> 26.02.2016 14:13, fu@linaro.org пишет:
>> From: Fu Wei
>>
>> delete: xen_linux, xen_initrd, xen_xsm
>> add: xen_module
>>
>> This update bases on
>> commit 0edd750e50698854068358ea53528100a9192902
>> Author: Vladimir Serbinenko
>> Date: Fri Jan 22 10:18:47 2016 +0100
>>
>> xen_boot: Remove obsolete module type distinctions.
>>
>> Signed-off-by: Fu Wei
>> ---
>> docs/grub.texi | 33 ++---
>> 1 file changed, 10 insertions(+), 23 deletions(-)
>>
>> diff --git a/docs/grub.texi b/docs/grub.texi
>> index 82f6fa4..3fbdd99 100644
>> --- a/docs/grub.texi
>> +++ b/docs/grub.texi
>> @@ -3861,9 +3861,7 @@ you forget a command, you can run the command
>> @command{help}
>> * videoinfo:: List available video modes
>> @comment * xen_*:: Xen boot commands
>> * xen_hypervisor:: Load xen hypervisor binary
>> -* xen_linux:: Load dom0 kernel for xen hypervisor
>> -* xen_initrd:: Load dom0 initrd for dom0 kernel
>> -* xen_xsm:: Load xen security module for xen
>> hypervisor
>> +* xen_module:: Load xen modules for xen hypervisor
>> @end menu
>>
>>
>> @@ -5141,30 +5139,19 @@ verbatim as the @dfn{kernel command-line}. Any
>> other binaries must be
>> reloaded after using this command.
>> @end deffn
>>
>> -@node xen_linux
>> -@subsection xen_linux
>> +@node xen_module
>> +@subsection xen_module
>>
>> -@deffn Command xen_linux file [arguments]
>> -Load a dom0 kernel image for xen hypervisor at the booting process of
>> xen.
>> +@deffn Command xen_module [--nounzip] file [arguments]
>> +Load a module for xen hypervisor at the booting process of xen.
>> The rest of the line is passed verbatim as the module command line.
>
> ==
>> +On i386, the modules will be identified by Multiboot(2) protocol.
>> +On arm64, each module will be identified by the order in which the
>> +modules are added.
>
> I think it is better to skip it entirely. It is not really correct -
> neither multiboot protocol provides any module identification (Xen
> probes module types), nor is i386 using multiboot2, nor can all modules
> be probed, so order still matters. To avoid confusion I'd simply
> replaced the above three lines with
>
> Modules should be loaded in the following order:
>
>> +The 1st module: dom0 kernel image
>> +The 2nd module: dom0 ramdisk (optional)
>
> This covers both supported platforms without going into too deep
> details; if you and Vladimir are OK, I'll commit with this change.
Thank you very much!
Sorry I am not familiar with xen on i386, so maybe I misunderstand this.
So please commit with your change, Thanks for your correction :-)
>>>
>>> I just fetched the mainline GRUB, i would like to know why this
>>> patchset haven't been applied?
>>> Anything I need to do(improve it or post a new patchset according to
>>> your suggestion) for this patchset?
>>>
>>
>> Sorry for delay. It is not really about your patchset, but we need some
>> decision about loading additional modules/lack of initrd on ARM. Until
>> then I'd rather avoid committing to any high-level configuration support
>> that will require even more backward compatible hacks later.
>>
>> As it stands now either Xen needs to support autodetection or we need to
>> revert to providing module type explicitly.
>
> So speaking of loading additional modules/lack of initrd on ARM, I thinks that
> will (only) affect loading XSM.
> For this, I have discussed of that with Julien, I think :
> (1) the first module must be kernel
> (2) the second module must be initrd, if we have initrd
> (3) Start from the 2nd module, XEN will detect that if the module is a XSM by
> the XSM binary signature. if we get XSM as the second module, that
> means we have not initrd.
>
If that's the plan, excellent. Vladimir, is it OK to commit then?
> please correct me if I misunderstand it
>
> :-)
>
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [RESEND][PATCH V16 4/6] libxl: add pvusb API
On 08/03/16 01:37, Chunyan Liu wrote:
> Add pvusb APIs, including:
> - attach/detach (create/destroy) virtual usb controller.
> - attach/detach usb device
> - list usb controller and usb devices
> - some other helper functions
>
> Signed-off-by: Simon Cao
> Signed-off-by: George Dunlap
> Signed-off-by: Chunyan Liu
Reviewed-by: George Dunlap
Good work!
-George
> ---
> Changes:
> * Address George's comments
>
> tools/libxl/Makefile |3 +-
> tools/libxl/libxl.c | 18 +
> tools/libxl/libxl.h | 77 ++
> tools/libxl/libxl_device.c |5 +-
> tools/libxl/libxl_internal.h | 18 +
> tools/libxl/libxl_osdeps.h | 13 +
> tools/libxl/libxl_pvusb.c| 1620
> ++
> tools/libxl/libxl_types.idl | 46 +
> tools/libxl/libxl_types_internal.idl |1 +
> tools/libxl/libxl_utils.c| 18 +
> tools/libxl/libxl_utils.h|5 +
> 11 files changed, 1822 insertions(+), 2 deletions(-)
> create mode 100644 tools/libxl/libxl_pvusb.c
>
> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
> index 789a12e..8fa7b87 100644
> --- a/tools/libxl/Makefile
> +++ b/tools/libxl/Makefile
> @@ -105,7 +105,8 @@ LIBXL_OBJS = flexarray.o libxl.o libxl_create.o
> libxl_dm.o libxl_pci.o \
> libxl_stream_read.o libxl_stream_write.o \
> libxl_save_callout.o _libxl_save_msgs_callout.o \
> libxl_qmp.o libxl_event.o libxl_fork.o \
> - libxl_dom_suspend.o libxl_dom_save.o $(LIBXL_OBJS-y)
> + libxl_dom_suspend.o libxl_dom_save.o libxl_pvusb.o \
> +$(LIBXL_OBJS-y)
> LIBXL_OBJS += libxl_genid.o
> LIBXL_OBJS += _libxl_types.o libxl_flask.o _libxl_types_internal.o
>
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 2ab5ad3..1e68688 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -4102,6 +4102,8 @@ out:
> * libxl_device_vkb_destroy
> * libxl_device_vfb_remove
> * libxl_device_vfb_destroy
> + * libxl_device_usbctrl_remove
> + * libxl_device_usbctrl_destroy
> */
> #define DEFINE_DEVICE_REMOVE_EXT(type, remtype, removedestroy, f)\
> int libxl_device_##type##_##removedestroy(libxl_ctx *ctx, \
> @@ -4159,6 +4161,10 @@ DEFINE_DEVICE_REMOVE(vfb, destroy, 1)
> DEFINE_DEVICE_REMOVE(vtpm, remove, 0)
> DEFINE_DEVICE_REMOVE(vtpm, destroy, 1)
>
> +/* usbctrl */
> +DEFINE_DEVICE_REMOVE_CUSTOM(usbctrl, remove, 0)
> +DEFINE_DEVICE_REMOVE_CUSTOM(usbctrl, destroy, 1)
> +
> /* channel/console hotunplug is not implemented. There are 2 possibilities:
> * 1. add support for secondary consoles to xenconsoled
> * 2. dynamically add/remove qemu chardevs via qmp messages. */
> @@ -4174,6 +4180,8 @@ DEFINE_DEVICE_REMOVE(vtpm, destroy, 1)
> * libxl_device_disk_add
> * libxl_device_nic_add
> * libxl_device_vtpm_add
> + * libxl_device_usbctrl_add
> + * libxl_device_usbdev_add
> */
>
> #define DEFINE_DEVICE_ADD(type) \
> @@ -4205,6 +4213,12 @@ DEFINE_DEVICE_ADD(nic)
> /* vtpm */
> DEFINE_DEVICE_ADD(vtpm)
>
> +/* usbctrl */
> +DEFINE_DEVICE_ADD(usbctrl)
> +
> +/* usb */
> +DEFINE_DEVICE_ADD(usbdev)
> +
> #undef DEFINE_DEVICE_ADD
>
>
> /**/
> @@ -6750,6 +6764,10 @@ int libxl_retrieve_domain_configuration(libxl_ctx
> *ctx, uint32_t domid,
>
> MERGE(pci, pcidevs, COMPARE_PCI, {});
>
> +MERGE(usbctrl, usbctrls, COMPARE_USBCTRL, {});
> +
> +MERGE(usbdev, usbdevs, COMPARE_USB, {});
> +
> /* Take care of removable device. We maintain invariant in the
> * insert / remove operation so that:
> * 1. if xenstore is "empty" while JSON is not, the result
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 0859383..5cc3ce3 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -123,6 +123,12 @@
> #define LIBXL_HAVE_DOMAIN_NODEAFFINITY 1
>
> /*
> + * LIBXL_HAVE_PVUSB indicates functions for plugging in USB devices
> + * through pvusb -- both hotplug and at domain creation time..
> + */
> +#define LIBXL_HAVE_PVUSB 1
> +
> +/*
> * LIBXL_HAVE_BUILDINFO_HVM_VENDOR_DEVICE indicates that the
> * libxl_vendor_device field is present in the hvm sections of
> * libxl_domain_build_info. This field tells libxl which
> @@ -1536,6 +1542,77 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid,
> libxl_device_disk *disk,
> const libxl_asyncop_how *ao_how)
> LIBXL_EXTERNAL_CALLERS_ONLY;
>
> +/*
> + * USB
> + *
> + * For each device removed or added, one of these protocols is available:
> + * - PV (i.e., PVUSB)
> + * - DEVICEMODEL (i.e, qemu)
> + *
> + * PV is available for either PV or HVM domains. DEVICEMODEL is only
> + * available for HVM domains. The caller can a
Re: [Xen-devel] [PATCH v3 6/6] libxl: add force option for xl vcpu-pin
On Thu, 2016-03-03 at 17:48 +0100, Juergen Gross wrote: > In order to be able to undo a vcpu pin override in case of a kernel > driver error add a flag "-f" to the "xl vcpu-pin" command forcing the > hypervisor to undo the override. > > Cc: Ian Jackson > Cc: Stefano Stabellini > Cc: Wei Liu > Signed-off-by: Juergen Gross > --- > tools/libxl/libxl.c | 31 +-- > tools/libxl/libxl.h | 4 > tools/libxl/xl_cmdimpl.c | 27 +++ > tools/libxl/xl_cmdtable.c | 3 ++- > Actually, there's something I always forget when reviewing xl stuff, which is that the xl manpage should be modified as well. Sorry for (nearly) missing this! :-/ 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 v3 6/6] libxl: add force option for xl vcpu-pin
On 08/03/16 18:16, Dario Faggioli wrote: > On Thu, 2016-03-03 at 17:48 +0100, Juergen Gross wrote: >> In order to be able to undo a vcpu pin override in case of a kernel >> driver error add a flag "-f" to the "xl vcpu-pin" command forcing the >> hypervisor to undo the override. >> >> Cc: Ian Jackson >> Cc: Stefano Stabellini >> Cc: Wei Liu >> Signed-off-by: Juergen Gross >> --- >> tools/libxl/libxl.c | 31 +-- >> tools/libxl/libxl.h | 4 >> tools/libxl/xl_cmdimpl.c | 27 +++ >> tools/libxl/xl_cmdtable.c | 3 ++- >> > Actually, there's something I always forget when reviewing xl stuff, > which is that the xl manpage should be modified as well. Yeah, I already thought of this, too. Will add it. Juergen > > Sorry for (nearly) missing this! :-/ > > Regards, > Dario > ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v11 20/27] Support colo mode for qemu disk
On Mon, Mar 07, 2016 at 10:10:07AM +0800, Wen Congyang wrote:
> On 03/05/2016 04:30 AM, Konrad Rzeszutek Wilk wrote:
> > On Fri, Mar 04, 2016 at 05:52:09PM +, Ian Jackson wrote:
> >> Changlong Xie writes ("[PATCH v11 20/27] Support colo mode for qemu disk"):
> >>> +Enable COLO HA for disk. For better understanding block replication on
> >>> +QEMU, please refer to:
> >>> +http://wiki.qemu.org/Features/BlockReplication
> >>
> >> Sorry, I missed this link on my first pass. I still think that at the
> >> very least this needs something more user-facing (ie, how should one
> >> set this up).
> >>
> >> But, I'm kind of worried that qemu is the wrong place to be doing
> >> this.
> >>
> >> How can this be made to work with PV guests ?
> >
> > QEMU can also serve PV guests (qdisk).
> >
> > I think your question is more of - what about making this work with
> > PV block backend?
>
> I don't know how to work with PV block backend. It is one reason that
> why we only support pure HVM now.
> For PV block backend, there is also other problem. For exampe resuming
> it in the secondary side is very slow, because we need to disconnect and
> reconnect.
>
Supporting PV guest is certainly going to be non-trivial. And I don't
think we would ever ask you to actually implement that.
The point is to have a story that when other people want to implement
COLO for PV-aware guests (PVHVM, PV and PVH), they are not crippled by
existing interfaces.
Currently the disk spec seems to be designed exclusively for QEMU. This
is not very desirable, but at least it wouldn't stop people from either
reusing them or inventing new parameters.
Furthermore, I think coming up with a story for PV-aware guests (PVHVM,
PV and PVH) is also non-trivial. For one the disk replication logic is
not implemented in PV block backend, we're not sure how feasible to
replicate thing in QEMU into kernel, but we're quite sure it is not
going to be trivial technically and politically. The uncertainty is too
big to come up with a clear idea what it would look like.
Wei.
> Thanks
> Wen Congyang
>
> >>
> >> What if an HVM guest has PV-on-HVM drivers ? In this case there might
> >> be two relevant qemus, one for the qdisk Xen PV block backend, and one
> >> for the emulated IDE.
> >
> > In both cases QEMU would use the same underlaying API to actually write/read
> > out the blocks. That API would then use NBD, etc to replicate writes.
> >
> > Maybe a little ASCII art?
> >
> > qdisk ide
> > \/
> >\ /
> >block API
> > |
> >QCOW2
> > |
> >NBD
> >
> > Or such?
> >
> >>
> >> I don't understand how discrepant writes are detected. Surely they
> >> might occur and should trigger a resynch ?
> >>
> >> Ian.
> >
> >
> > .
> >
>
>
>
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] Ideas Re: [PATCH v14 1/2] vmx: VT-d posted-interrupt core logic handling
>>> On 08.03.16 at 18:05, wrote: > On 08/03/16 15:42, Jan Beulich wrote: > On 08.03.16 at 15:42, wrote: >>> On Tue, Mar 8, 2016 at 1:10 PM, Wu, Feng wrote: > -Original Message- > From: George Dunlap [mailto:george.dun...@citrix.com] > > 2. Try to test engineered situations where we expect this to be a > problem, to see how big of a problem it is (proving the theory to be > accurate or inaccurate in this case) Maybe we can run a SMP guest with all the vcpus pinned to a dedicated pCPU, we can run some benchmark in the guest with VT-d PI and without VT-d PI, then see the performance difference between these two sceanrios. >>> >>> This would give us an idea what the worst-case scenario would be. >> >> How would a single VM ever give us an idea about the worst >> case? Something getting close to worst case is a ton of single >> vCPU guests all temporarily pinned to one and the same pCPU >> (could be multi-vCPU ones, but the more vCPU-s the more >> artificial this pinning would become) right before they go into >> blocked state (i.e. through one of the two callers of >> arch_vcpu_block()), the pinning removed while blocked, and >> then all getting woken at once. > > Why would removing the pinning be important? It's not important by itself, other than to avoid all vCPU-s then waking up on the one pCPU. > And I guess it's actually the case that it doesn't need all VMs to > actually be *receiving* interrupts; it just requires them to be > *capable* of receiving interrupts, for there to be a long chain all > blocked on the same physical cpu. Yes. >>> But >>> pinning all vcpus to a single pcpu isn't really a sensible use case we >>> want to support -- if you have to do something stupid to get a >>> performance regression, then I as far as I'm concerned it's not a >>> problem. >>> >>> Or to put it a different way: If we pin 10 vcpus to a single pcpu and >>> then pound them all with posted interrupts, and there is *no* >>> significant performance regression, then that will conclusively prove >>> that the theoretical performance regression is of no concern, and we >>> can enable PI by default. >> >> The point isn't the pinning. The point is what pCPU they're on when >> going to sleep. And that could involve quite a few more than just >> 10 vCPU-s, provided they all sleep long enough. >> >> And the "theoretical performance regression is of no concern" is >> also not a proper way of looking at it, I would say: Even if such >> a situation would happen extremely rarely, if it can happen at all, >> it would still be a security issue. > > What I'm trying to get at is -- exactly what situation? What actually > constitutes a problematic interrupt latency / interrupt processing > workload, how many vcpus must be sleeping on the same pcpu to actually > risk triggering that latency / workload, and how feasible is it that > such a situation would arise in a reasonable scenario? > > If 200us is too long, and it only takes 3 sleeping vcpus to get there, > then yes, there is a genuine problem we need to try to address before we > turn it on by default. If we say that up to 500us is tolerable, and it > takes 100 sleeping vcpus to reach that latency, then this is something I > don't really think we need to worry about. > > "I think something bad may happen" is a really difficult to work with. I understand that, but coming up with proper numbers here isn't easy. Fact is - it cannot be excluded that on a system with hundreds of pCPU-s and thousands or vCPU-s, that all vCPU-s would at some point pile up on one pCPU's list. How many would be tolerable on a single list depends upon host characteristics, so a fixed number won't do anyway. Hence I think the better approach, instead of improving lookup, is to distribute vCPU-s evenly across lists. Which in turn would likely require those lists to no longer be tied to pCPU-s, an aspect I had already suggested during review. As soon as distribution would be reasonably even, the security concern would vanish: Someone placing more vCPU-s on a host than that host can handle is responsible for the consequences. Quite contrary to someone placing more vCPU-s on a host than a single pCPU can reasonably handle in an interrupt handler. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 1/4] x86/alternatives: correct near branch check
On 07/03/16 16:21, Jan Beulich wrote:
On 07.03.16 at 17:11, wrote:
>> On 07/03/16 15:56, Jan Beulich wrote:
>> On 07.03.16 at 16:43, wrote:
On 04/03/16 11:27, Jan Beulich wrote:
> Make sure the near JMP/CALL check doesn't consume uninitialized
> data, not even in a benign way. And relax the length check at once.
>
> Signed-off-by: Jan Beulich
>
> --- a/xen/arch/x86/alternative.c
> +++ b/xen/arch/x86/alternative.c
> @@ -174,7 +174,7 @@ static void __init apply_alternatives(st
> memcpy(insnbuf, replacement, a->replacementlen);
>
> /* 0xe8/0xe9 are relative branches; fix the offset. */
> -if ( (*insnbuf & 0xfe) == 0xe8 && a->replacementlen == 5 )
> +if ( a->replacementlen >= 5 && (*insnbuf & 0xfe) == 0xe8 )
> *(s32 *)(insnbuf + 1) += replacement - instr;
>
> add_nops(insnbuf + a->replacementlen,
>
>
>
Swapping the order is definitely a good thing.
However, relaxing the length check seems less so. `E8 rel32` or `E9
rel32` encodings are strictly 5 bytes long.
There are complications with the `67 E{8,9} rel16` encodings, but those
are not catered for anyway, and the manual warns about undefined
behaviour if used in long mode.
What is your usecase for relaxing the check? IMO, if it isn't exactly 5
bytes long, there is some corruption somewhere and the relocation
should't happen.
>>> The relaxation is solely because at least CALL could validly
>>> be followed by further instructions.
>> But without scanning the entire replacement buffer, there might be other
>> relocations needing to happen.
>>
>> That would require decoding the instructions, which is an extreme faff.
>> It would be better to leave it currently as-is to effectively disallow
>> mixing a jmp/call replacement with other code, to avoid the subtle
>> failure of a second relocation not taking effect
> Well, such missing further fixup would be noticed immediately by
> someone trying (unless the patch code path never gets executed).
> Whereas a simply adjustment to register state would seem quite
> reasonable to follow a call. While right now the subsequent
> patches don't depend on this being >= or ==, I think it was wrong
> to be == from the beginning.
>
> Plus - there are endless other possibilities of instructions needing
> fixups (most notably such with RIP-relative memory operands),
> none of which are even remotely reasonable to deal with here.
> I.e. namely in the absence of a CALL/JMP the same issue would
> exist anyway, which is why I'm not overly concerned of those.
> All we want is a specific special case to be treated correctly.
Fair enough. Reviewed-by: Andrew Cooper
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 2/2] xsm: move FLASK_AVC_STATS to Kconfig
On 03/08/2016 11:51 AM, Jan Beulich wrote: On 08.03.16 at 17:22, wrote: On 03/08/2016 04:46 AM, Jan Beulich wrote: On 07.03.16 at 19:42, wrote: Have Kconfig set CONFIG_FLASK_AVC_STATS and prefix all uses with CONFIG_ to use the Kconfig variable. Same question here: What's the benefit of doing it this way? This removes the stats tracking, which might (I have not tested) speed up the security server by avoiding the __get_cpu_var call and increment. No, I don not think the patch removes anything. The Kconfig option doesn't have a prompt. But anyway, ... Ah, I missed that: I saw the --help-- line and assumed it was the prompt. Either way, this #define is a configuration-like knob that doesn't need to be hard-coded in a header as it currently is. The corresponding SELinux knob is a Kconfig option in Linux. Acked-by: Daniel De Graaf ... if you're fine with it, we'll put it in (once the mechanical issues got addressed). -- Daniel De Graaf National Security Agency ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 3/3] xl: new "loglvl" command
On Tue, 2016-03-08 at 14:05 +, George Dunlap wrote: > On Tue, Mar 8, 2016 at 8:08 AM, Jan Beulich > wrote: > > > > Right, and asking people myself to not follow bad examples when > > adding new code, I did take all of your input to adjust the patch. > > Just that in this case the set of bad examples is so large that in > > a > > similar case in the hypervisor I probably wouldn't have dared to > > ask for a style correction. > Well the approach of the libxl maintainers seems to have be, "Just > make sure the new code adheres to the new style, and eventyally > everything will be up-to-date". > Funnily enough, basing on my experience, libxl does not look that bad to me, and every time I've been bitten by something like this, it was in Xen rather than in libxl. :-D Of course, although I've been active in both, I don't claim that my experience is statistically significant... I guess it depends on what specific areas of code one gets to work on. Anyway, I personally don't think this affect in any way the point that new code should comply as much as possible with coding style, existing best practises, etc., and that is true for this patch, as well as for all the times everyone of us may have been asked to do the same, either in xen, tools, or anywhere... In fact, especially if we decide to do this (which I'd be in favour of, and up for helping): > Given that the "new" style has been around for a while now, it > probably would be good to set aside some time at the beginning of the > next development cycle to fix things up > being strict about new code actually helps this, as it makes sure there is less --rather than more-- code to fix during such a huge fixup challenge! :-) > -- it is incredibly > frustrating to carefully try to copy the surrounding style, only to > be > told to revise it. > Yep, I fully agree. 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] Xentrace on Xilinx ARM
All, To update to the current situation. I have been able to get xentrace() and xenalyze working completely (at least as far as I can tell) on ARM. For xentrace there were changes to the memory allocation routines to allow mapping of the Xen Heap by dom0, correcting the MFN->PFN translations, adding the trace buffer initialization to setup.c (init_trace_bufs), and correcting the get_cycles() call to provide the system TSC. For the get_cycles() call I gathered that was supposed to return the raw tick count, not a translated ticks->real time timestamp. I then had to call xenalyze with the core frequency defined so the timestamps made sence. Paul: Was there anything else you did I missed? >It's not part of any Xen image. It's a command line tool to be used, usually >but not necessarily, in dom0, build and installed together with the other >tools... At least in my case, for x86 builds and installs. For xenalyze I had to modify the makefile to build xenalyze on the ARM platform (it was specifically removed from the ARM build). Once that was corrected I could find and call it from dom0. It built only locally to Xen though (could only run from dom0), I could not use it from the native Linux development environment (I don't know if you're supposed to be able to? Or since I'm running ARM it built for ARM not x86 and thus could not be used natively). I plan to push they changes in as a patch to the mainline if that seems reasonable to everyone. Thanks, Ben Sanda ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
