Re: [Xen-devel] [PATCH v6 5/5] x86/vm_event: Add HVM debug exception vm_events
>>> On 24.06.16 at 18:29, wrote: > On Fri, Jun 24, 2016 at 5:24 AM, Jan Beulich wrote: > On 24.06.16 at 13:20, wrote: From: Tamas K Lengyel [mailto:ta...@tklengyel.com] + * rc < error, fall-through to exit_and_crash + */ +if ( !rc ) +{ +vmx_propagate_intr(intr_info); +break; +} +if ( rc > 0 ) +break; +} else +{ domain_pause_for_debugger(); -break; +break; +} + +goto exit_and_crash; >>> >>> Putting goto as the last line within a 'case' looks a bit strange. What >>> about putting goto directly under a "if ( rc < 0 )" check earlier? >>> >>> if ( !rc ) >>> ... >>> if ( rc < 0 ) >>> goto exit_and_crash; >>> } >>> else >>> domain_pause_for_debugger(); >>> break; >> >> Thanks, Kevin - indeed that's exactly what I had asked for already >> on the previous iteration. >> > > I'm OK with adding the rc < 0 case, it's just that the fall-through > style is already used for handling the int3 events for example. Should > I fix that too while I'm at it so the code is consistent? Especially if you did it in a separate patch, I for one would appreciate that. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v5 08/14] hvmloader: Locate the BIOS blob
>>> On 24.06.16 at 19:02, wrote: > On Fri, Jun 24, 2016 at 01:33:45AM -0600, Jan Beulich wrote: >> >>> On 22.06.16 at 19:15, wrote: >> > --- a/tools/firmware/hvmloader/hvmloader.c >> > +++ b/tools/firmware/hvmloader/hvmloader.c >> > @@ -253,10 +253,51 @@ static void acpi_enable_sci(void) >> > BUG_ON(!(pm1a_cnt_val & ACPI_PM1C_SCI_EN)); >> > } >> > >> > +const struct hvm_modlist_entry *get_module_entry( >> > +const struct hvm_start_info *info, >> > +const char *name) >> > +{ >> > +const struct hvm_modlist_entry *modlist = >> > +(struct hvm_modlist_entry *)(uint32_t)info->modlist_paddr; >> > +unsigned int i; >> > + >> > +if ( !modlist || info->modlist_paddr > UINT_MAX) >> > +return NULL; >> >> How about info->modlist_paddr + info->nr_modules * sizeof()? >> You check for overflow below, but not here. I think you should >> either consistently rely on there being something right below 4Gb >> which makes this impossible (and then say so in a comment), or >> do full checks everywhere. > > I'll do the full checks. > >> > +for ( i = 0; i < info->nr_modules; i++ ) >> > +{ >> > +uint32_t module_name = modlist[i].cmdline_paddr; >> > + >> > +/* Skip if the module or its cmdline is missing. */ >> > +if ( !module_name || !modlist[i].paddr ) >> > +continue; >> > + >> > +/* Skip if the cmdline can not be read. */ >> > +if ( modlist[i].cmdline_paddr > UINT_MAX ) >> > +continue; >> >> Similarly here. > > Here, I don't know the size of the cmdline and I don't think calling an > extra strlen() would be usefull. I think that the strcmp() below is going to > be enough for the top bondary check. No - once you reach the 4Gb boundary, the compare would continue at address zero. That's not what you want. > Or I could use the size of name. Size of name? >> > +{ >> > +if ( modlist[i].paddr > UINT_MAX || modlist[i].size > >> > UINT_MAX || >> > + (modlist[i].paddr + modlist[i].size) > UINT_MAX ) >> >> I think the last one could be >=. > > I think it's valid if addr+size == UINT_MAX. That would means the last > byte of the module would be at 0xFFFE. It's even valid when addr+size == UINT_MAX+1 (without value wrapping of course), as the last valid byte (i.e. the last one hvmloader can address easily) is at 0x. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v5 09/14] hvmloader: Check modules whereabouts in perform_tests
>>> On 24.06.16 at 19:14, wrote: > On Fri, Jun 24, 2016 at 01:44:30AM -0600, Jan Beulich wrote: >> >>> On 22.06.16 at 19:15, wrote: >> > +uint64_t cmdline_paddr = modlist[i].cmdline_paddr; >> > + >> > +if ( check_test_overlap(modlist[i].paddr, modlist[i].size) ) >> > +{ >> > +printf("Skipping tests due to memory used by module[%d]\n", > i); >> > +return; >> > +} >> > +if ( cmdline_paddr && cmdline_paddr < UINT_MAX && >> > + check_test_overlap(cmdline_paddr, >> > +strlen((char*)(uint32_t)cmdline_paddr)) ) >> >> As said on the previous patch - cmdline_paddr being below 4Gb >> doesn't necessarily mean the string not crossing that boundary. >> Depending on the resolution for that other patch this may need >> adjustment. > > I'm not sure how I could handle that, here. Either determine the length of the string first, or have a special variant of strcmp() which returns false when the address would wrap without having reached the end of the string. But again - maybe all this goes too far, and we should instead opt for the simpler route (and easier to grok code) assuming that no item would ever cross the 4Gb boundary. I'm sure making this a requirement even for PVH (which might not actually have anything right below 4Gb, especially when there is no ACPI enabled, and hence no tables to be put anywhere) wouldn't pose a severe limitation to the party setting up the domain: After all such code has to be prepared for stuff to need placement below 4Gb (apart from ACPI tables this could also be PCI device MMIO BAR assignment ranges) anyway. >> Also, thinking about it again, the use of UINT_MAX for bounding >> pointers is unfortunate: I think this would better be UINTPTR_MAX. > > Well, I do cast every addr to uint32_t, and I had to define UINT_MAX in > the previous patch (hvmloader: Locate the BIOS blob)(I should probably > add a comment about it in the patch description). > > I could try to add UINTPTR_MAX instead. That would seem more natural, thanks. And in fact I question the use of uint32_t (instead of unsigned long or even better uintptr_t) for such casts and variable types. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 2/2] xen-pciback: clean up {bar, rom}_init()
>>> On 24.06.16 at 17:01, wrote: > On 07/06/16 07:31, Jan Beulich wrote: >> - drop unused function parameter of read_dev_bar() >> - drop rom_init() (now identical to bar_init()) >> - fold read_dev_bar() into its now single caller >> - simplify determination of 64-bit memory resource >> - use const and unsigned > > Please split this in 5 separate patches for easier review. > > Especially as often anyone writing "simplify" means "accidentally break". So this is directly opposite of what Boris had asked for - originally there were two patches, which I folded upon his request (and which he gave his R-b for already). May I ask the two of you to first settle on a consistent set of expectations to patches like this? Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [PATCH v4] xen: arm: Update arm64 image header
With the Linux kernel commits https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/Documentation/arm64/booting.txt?id=4370eec05a887b0cd4392cd5dc5b2713174745c0 https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/Documentation/arm64/booting.txt?id=a2c1d73b94ed49f5fac12e95052d7b140783f800 the arm64 image header changed. While the size of the header isn't changed, some members have changed their usage. Update Xen to this updated image header. The main changes are that the first magic is gone and that there is an image size, now. In case we read a size != 0, let's use this image size, now. This does allow us to check if the kernel Image is larger than the size given in the device tree, too. Additionally, add an error message if the magic is not found. This might be the case with kernel's < 3.12 prior to https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=4370eec05a887b0cd4392cd5dc5b2713174745c0 which introduced the second magic. This is acceptable as the support of Xen for ARM64 in Linux has been added in Linux 3.11 and the number of boards supported by Linux 3.11 on ARM64 is very limited: ARM models and X-gene. And for the latter it was an early support with only the serial and timer upstreamed. Signed-off-by: Dirk Behme --- Changes in v4: Print header error only based on ZIMAGE64_MAGIC_V0. Drop comment and unnecessary if statement. Changes in v3: Just update of the commit message regarding the support for kernels < 3.12. No change to the patch itself. xen/arch/arm/kernel.c | 43 +++ 1 file changed, 27 insertions(+), 16 deletions(-) diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c index 9871bd9..1797097 100644 --- a/xen/arch/arm/kernel.c +++ b/xen/arch/arm/kernel.c @@ -29,7 +29,7 @@ #define ZIMAGE32_MAGIC 0x016f2818 #define ZIMAGE64_MAGIC_V0 0x1408 -#define ZIMAGE64_MAGIC_V1 0x644d5241 /* "ARM\x64" */ +#define ZIMAGE64_MAGIC 0x644d5241 /* "ARM\x64" */ struct minimal_dtb_header { uint32_t magic; @@ -335,17 +335,19 @@ static int kernel_zimage64_probe(struct kernel_info *info, { /* linux/Documentation/arm64/booting.txt */ struct { -uint32_t magic0; -uint32_t res0; -uint64_t text_offset; /* Image load offset */ -uint64_t res1; +union { +uint32_t code0; +uint32_t magic0; /* Old header magic */ +}; +uint32_t code1; +uint64_t text_offset; /* Image load offset, little endian */ +uint64_t image_size; /* Effective Image size, little endian */ +uint64_t flags; uint64_t res2; -/* zImage V1 only from here */ uint64_t res3; uint64_t res4; -uint64_t res5; -uint32_t magic1; -uint32_t res6; +uint32_t magic;/* Magic number, little endian, "ARM\x64" */ +uint32_t res5; } zimage; uint64_t start, end; @@ -354,20 +356,29 @@ static int kernel_zimage64_probe(struct kernel_info *info, copy_from_paddr(&zimage, addr, sizeof(zimage)); -if ( zimage.magic0 != ZIMAGE64_MAGIC_V0 && - zimage.magic1 != ZIMAGE64_MAGIC_V1 ) +if ( zimage.magic != ZIMAGE64_MAGIC ) { +if ( zimage.magic0 == ZIMAGE64_MAGIC_V0 ) + printk(XENLOG_ERR "No valid magic found in header! Kernel too old\n"); return -EINVAL; +} -/* Currently there is no length in the header, so just use the size */ start = 0; -end = size; /* - * Given the above this check is a bit pointless, but leave it - * here in case someone adds a length field in the future. + * Where image_size is non-zero image_size is little-endian + * and must be respected. */ -if ( (end - start) > size ) +if ( zimage.image_size ) +end = zimage.image_size; +else +end = size; + +if ( (end - start) > size ) { +printk(XENLOG_ERR "Error: Kernel Image size: %lu bytes > bootmodule size: %lu bytes\n", + zimage.image_size, (uint64_t)size); +printk(XENLOG_ERR "The field 'size' does not match the size of blob!\n"); return -EINVAL; +} info->zimage.kernel_addr = addr; info->zimage.len = end - start; -- 2.8.0 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v12 6/6] vt-d: fix vt-d Device-TLB flush timeout issue
>>> On 26.06.16 at 11:18, wrote: > On June 24, 2016 7:55 PM, Tian, Kevin wrote: >> > From: Xu, Quan >> > Sent: Friday, June 24, 2016 1:52 PM >> > diff --git a/xen/drivers/passthrough/vtd/extern.h >> > b/xen/drivers/passthrough/vtd/extern.h >> > index 45357f2..efaff28 100644 >> > --- a/xen/drivers/passthrough/vtd/extern.h >> > +++ b/xen/drivers/passthrough/vtd/extern.h >> > @@ -25,6 +25,7 @@ >> > >> > #define VTDPREFIX "[VT-D]" >> > >> > +struct pci_ats_dev; >> > extern bool_t rwbf_quirk; >> > >> > void print_iommu_regs(struct acpi_drhd_unit *drhd); @@ -60,8 +61,8 @@ >> > int dev_invalidate_iotlb(struct iommu *iommu, u16 did, >> > u64 addr, unsigned int size_order, u64 >> > type); >> > >> > int __must_check qinval_device_iotlb_sync(struct iommu *iommu, >> > - u32 max_invs_pend, >> > - u16 sid, u16 size, u64 addr); >> > + struct pci_ats_dev *ats_dev, >> > + u16 did, u16 size, u64 >> > + addr); >> > >> > unsigned int get_cache_line_size(void); void cacheline_flush(char >> > *); diff --git a/xen/drivers/passthrough/vtd/qinval.c >> > b/xen/drivers/passthrough/vtd/qinval.c >> > index 4492b29..e4e2771 100644 >> > --- a/xen/drivers/passthrough/vtd/qinval.c >> > +++ b/xen/drivers/passthrough/vtd/qinval.c >> > @@ -27,11 +27,11 @@ >> > #include "dmar.h" >> > #include "vtd.h" >> > #include "extern.h" >> > +#include "../ats.h" >> >> Earlier you said: >> >1. a forward declaration struct pci_ats_dev*, instead of >> > including ats.h. >> > > This context is 'in extern.h', but.. > >> But above you still have ats.h included. >> > > .. I really need to include 'ats.h' here, as the 'struct pci_ats_dev*' is > used in this file. Used as in "a variable of this type, or a pointer to this type de-referenced", or only as in "a variable/parameter of pointer to this type declared"? In the latter case you don't need the include. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v12 1/6] IOMMU: add a timeout parameter for device IOTLB invalidation
>>> On 24.06.16 at 07:51, wrote: > From: Quan Xu > > The parameter 'iommu_dev_iotlb_timeout' specifies the timeout > of device IOTLB invalidation in milliseconds. By default, the > timeout is 1000 milliseconds, which can be boot-time changed. > > We also confirmed with VT-d hardware team that 1 milliseconds > is large enough for VT-d IOMMU internal invalidation. > > the existing panic() is eliminated and we bubble up the timeout > of device IOTLB invalidation for further processing, as the > PCI-e Address Translation Services (ATS) mandates a timeout of > 60 seconds for device IOTLB invalidation. Obviously we can't > spin for 60 seconds or otherwise Xen hypervisor hangs. > > Add a __must_check annotation. The followup patch titled > 'VT-d IOTLB/Context/IEC flush issue' addresses the __mustcheck. > That is the other callers of this routine (two or three levels up) > ignore the return code. This patch does not address this but the > other does. The patch itself looks okay, but I'm confused by this paragraph: There's no patch with the named title later in this series. And having gone through this patch I also don't see what remains to be addressed wrt the __must_check-s getting added here. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v12 4/6] IOMMU/x86: using a struct pci_dev* instead of SBDF
>>> On 24.06.16 at 07:51, wrote: > --- a/xen/drivers/passthrough/x86/ats.c > +++ b/xen/drivers/passthrough/x86/ats.c > @@ -22,26 +22,34 @@ LIST_HEAD(ats_devices); > bool_t __read_mostly ats_enabled = 0; > boolean_param("ats", ats_enabled); > > -int enable_ats_device(int seg, int bus, int devfn, const void *iommu) > +int enable_ats_device(const void *iommu, struct pci_dev *pci_dev) Is there anything preventing the second parameter to become a pointer to const too? Afaict that would in turn eliminate the need for some of the changes further up. > { > struct pci_ats_dev *pdev = NULL; > u32 value; > int pos; > > -pos = pci_find_ext_capability(seg, bus, devfn, PCI_EXT_CAP_ID_ATS); > +pos = pci_find_ext_capability(pci_dev->seg, pci_dev->bus, pci_dev->devfn, > + PCI_EXT_CAP_ID_ATS); Please add local variables seg, bus, and devfn, which will greatly reduce the number of changes you need to do to this function (and which likely will also produce better code). > @@ -98,7 +104,13 @@ void disable_ats_device(int seg, int bus, int devfn) For symmetry reasons this function would also get converted to taking const struct pci_dev *. > @@ -120,7 +132,13 @@ struct pci_ats_dev *get_ats_device(int seg, int bus, int > devfn) And this one then probably too. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v12 5/6] IOMMU: move the domain crash logic up to the generic IOMMU layer
>>> On 26.06.16 at 10:58, wrote: > On June 24, 2016 7:48 PM, Tian, Kevin wrote: >> > From: Xu, Quan >> > Sent: Friday, June 24, 2016 1:52 PM >> > >> > From: Quan Xu >> > >> > Signed-off-by: Quan Xu >> > >> > CC: Julien Grall >> > CC: Kevin Tian >> > CC: Feng Wu >> > CC: Jan Beulich >> > CC: Suravee Suthikulpanit >> > --- >> > xen/drivers/passthrough/iommu.c | 30 >> > -- >> > xen/drivers/passthrough/vtd/iommu.c | 11 +++ >> > 2 files changed, 39 insertions(+), 2 deletions(-) >> >> when you say "moving the logic up", I don't see any lines being deleted. >> Looks >> you are just "adding the domain crash logic"? > > Yes, it is 'adding'.. But why don't you do here what the title says? Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v12 1/6] IOMMU: add a timeout parameter for device IOTLB invalidation
On June 27, 2016 4:03 PM, Jan Beulich wrote: > >>> On 24.06.16 at 07:51, wrote: > > From: Quan Xu > > > > The parameter 'iommu_dev_iotlb_timeout' specifies the timeout of > > device IOTLB invalidation in milliseconds. By default, the timeout is > > 1000 milliseconds, which can be boot-time changed. > > > > We also confirmed with VT-d hardware team that 1 milliseconds is large > > enough for VT-d IOMMU internal invalidation. > > > > the existing panic() is eliminated and we bubble up the timeout of > > device IOTLB invalidation for further processing, as the PCI-e Address > > Translation Services (ATS) mandates a timeout of > > 60 seconds for device IOTLB invalidation. Obviously we can't spin for > > 60 seconds or otherwise Xen hypervisor hangs. > > > > Add a __must_check annotation. The followup patch titled 'VT-d > > IOTLB/Context/IEC flush issue' addresses the __mustcheck. > > That is the other callers of this routine (two or three levels up) > > ignore the return code. This patch does not address this but the other > > does. > > The patch itself looks okay, Jan, thanks for your review. > but I'm confused by this paragraph: > There's no patch with the named title later in this series. And having gone > through this patch I also don't see what remains to be addressed wrt the > __must_check-s getting added here. > This paragraph was added from a few rounds ago. I will drop it in next version. Quan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [xen-4.3-testing test] 96295: regressions - FAIL
flight 96295 xen-4.3-testing real [real] http://logs.test-lab.xenproject.org/osstest/logs/96295/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-i386-libvirt5 libvirt-build fail REGR. vs. 87893 build-amd64-libvirt 5 libvirt-build fail REGR. vs. 87893 build-armhf 5 xen-build fail REGR. vs. 87893 Tests which are failing intermittently (not blocking): test-amd64-i386-xend-qemut-winxpsp3 9 windows-install fail pass in 96279 Regressions which are regarded as allowable (not blocking): test-amd64-amd64-xl-qemuu-win7-amd64 16 guest-stop fail like 87893 test-amd64-i386-xl-qemuu-win7-amd64 16 guest-stop fail like 87893 test-amd64-i386-xl-qemut-win7-amd64 16 guest-stop fail like 87893 Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt-vhd 1 build-check(1) blocked n/a test-armhf-armhf-libvirt-raw 1 build-check(1) blocked n/a test-amd64-amd64-libvirt 1 build-check(1) blocked n/a test-armhf-armhf-xl-credit2 1 build-check(1) blocked n/a test-armhf-armhf-xl-arndale 1 build-check(1) blocked n/a test-armhf-armhf-xl-multivcpu 1 build-check(1) blocked n/a test-armhf-armhf-xl-cubietruck 1 build-check(1) blocked n/a test-armhf-armhf-libvirt-qcow2 1 build-check(1) blocked n/a test-armhf-armhf-xl-vhd 1 build-check(1) blocked n/a test-armhf-armhf-libvirt 1 build-check(1) blocked n/a test-armhf-armhf-xl 1 build-check(1) blocked n/a test-amd64-i386-libvirt 1 build-check(1) blocked n/a test-amd64-i386-rumpuserxen-i386 1 build-check(1) blocked n/a test-amd64-amd64-rumpuserxen-amd64 1 build-check(1) blocked n/a build-armhf-libvirt 1 build-check(1) blocked n/a test-amd64-i386-xend-qemut-winxpsp3 20 leak-check/check fail in 96279 never pass test-amd64-i386-xl-qemuu-ovmf-amd64 9 debian-hvm-install fail never pass test-amd64-amd64-xl-qemuu-ovmf-amd64 9 debian-hvm-install fail never pass build-amd64-rumpuserxen 6 xen-buildfail never pass build-i386-rumpuserxen6 xen-buildfail never pass test-amd64-amd64-xl-qemut-win7-amd64 16 guest-stop fail never pass version targeted for testing: xen 0a8c94fae993dd8f2b27fd4cc694f61c21de84bf baseline version: xen 8fa31952e2d08ef63897c43b5e8b33475ebf5d93 Last test of basis87893 2016-03-29 13:49:52 Z 89 days Failing since 92180 2016-04-20 17:49:21 Z 67 days 34 attempts Testing same since96017 2016-06-20 17:22:27 Z6 days 13 attempts People who touched revisions under test: Andrew Cooper Anthony Liguori Anthony PERARD Gerd Hoffmann Ian Jackson Jan Beulich Jim Paris Stefan Hajnoczi Tim Deegan Wei Liu jobs: build-amd64 pass build-armhf fail build-i386 pass build-amd64-libvirt fail build-armhf-libvirt blocked build-i386-libvirt fail build-amd64-pvopspass build-armhf-pvopspass build-i386-pvops pass build-amd64-rumpuserxen fail build-i386-rumpuserxen fail test-amd64-amd64-xl pass test-armhf-armhf-xl blocked test-amd64-i386-xl pass test-amd64-i386-qemut-rhel6hvm-amd pass test-amd64-i386-qemuu-rhel6hvm-amd pass test-amd64-amd64-xl-qemut-debianhvm-amd64pass test-amd64-i386-xl-qemut-debianhvm-amd64 pass test-amd64-amd64-xl-qemuu-debianhvm-amd64pass test-amd64-i386-xl-qemuu-debianhvm-amd64 pass test-amd64-i386-freebsd10-amd64 pass test-amd64-amd64-xl-qemuu-ovmf-amd64 fail test-amd64-i386-xl-qemuu-ovmf-amd64 fail test-amd64-amd64-rumpuserxen-amd64 blocked test-amd64-amd64-xl-qemut-win7-amd64 fail test-amd64-i386-
Re: [Xen-devel] [PATCH v12 6/6] vt-d: fix vt-d Device-TLB flush timeout issue
>>> On 24.06.16 at 07:51, wrote: > @@ -199,24 +199,73 @@ static int __must_check queue_invalidate_wait(struct > iommu *iommu, > return -EOPNOTSUPP; > } > > -static int __must_check invalidate_sync(struct iommu *iommu, > -bool_t flush_dev_iotlb) > +static int __must_check invalidate_sync(struct iommu *iommu) > { > struct qi_ctrl *qi_ctrl = iommu_qi_ctrl(iommu); > > ASSERT(qi_ctrl->qinval_maddr); > > -return queue_invalidate_wait(iommu, 0, 1, 1, flush_dev_iotlb); > +return queue_invalidate_wait(iommu, 0, 1, 1, 0); > +} > + > +static void dev_invalidate_iotlb_timeout(struct iommu *iommu, u16 did, > + struct pci_dev *pdev) > +{ > +struct domain *d = NULL; > + > +if ( test_bit(did, iommu->domid_bitmap) ) > +d = rcu_lock_domain_by_id(iommu->domid_map[did]); > + > +/* > + * In case the domain has been freed or the IOMMU domid bitmap is > + * not valid, the device no longer belongs to this domain. > + */ > +if ( d == NULL ) > +return; > + > +pcidevs_lock(); > +ASSERT(pdev->domain); > +list_del(&pdev->domain_list); > +pdev->domain = NULL; > +pci_hide_existing_device(pdev); > +pcidevs_unlock(); > + > +if ( !d->is_shutting_down && printk_ratelimit() ) > +printk(XENLOG_WARNING VTDPREFIX > + " dom%d: ATS device %04x:%02x:%02x.%u flush failed\n", > + d->domain_id, pdev->seg, pdev->bus, > + PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn)); > + > +if ( !is_hardware_domain(d) ) > +domain_crash(d); > + > +rcu_unlock_domain(d); > +} So in an earlier patch in this series you (supposedly) moved similar logic up to the vendor independent layer. I think this then would better get moved up too, if at all possible. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v12 4/6] IOMMU/x86: using a struct pci_dev* instead of SBDF
>>> On 27.06.16 at 10:17, wrote: On 24.06.16 at 07:51, wrote: >> --- a/xen/drivers/passthrough/x86/ats.c >> +++ b/xen/drivers/passthrough/x86/ats.c >> @@ -22,26 +22,34 @@ LIST_HEAD(ats_devices); >> bool_t __read_mostly ats_enabled = 0; >> boolean_param("ats", ats_enabled); >> >> -int enable_ats_device(int seg, int bus, int devfn, const void *iommu) >> +int enable_ats_device(const void *iommu, struct pci_dev *pci_dev) > > Is there anything preventing the second parameter to become > a pointer to const too? Afaict that would in turn eliminate the > need for some of the changes further up. I just figured that patch 6 depends on this being non-const. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v12 1/6] IOMMU: add a timeout parameter for device IOTLB invalidation
>>> On 27.06.16 at 10:19, wrote: > On June 27, 2016 4:03 PM, Jan Beulich wrote: >> >>> On 24.06.16 at 07:51, wrote: >> > From: Quan Xu >> > >> > The parameter 'iommu_dev_iotlb_timeout' specifies the timeout of >> > device IOTLB invalidation in milliseconds. By default, the timeout is >> > 1000 milliseconds, which can be boot-time changed. >> > >> > We also confirmed with VT-d hardware team that 1 milliseconds is large >> > enough for VT-d IOMMU internal invalidation. >> > >> > the existing panic() is eliminated and we bubble up the timeout of >> > device IOTLB invalidation for further processing, as the PCI-e Address >> > Translation Services (ATS) mandates a timeout of >> > 60 seconds for device IOTLB invalidation. Obviously we can't spin for >> > 60 seconds or otherwise Xen hypervisor hangs. >> > >> > Add a __must_check annotation. The followup patch titled 'VT-d >> > IOTLB/Context/IEC flush issue' addresses the __mustcheck. >> > That is the other callers of this routine (two or three levels up) >> > ignore the return code. This patch does not address this but the other >> > does. >> >> The patch itself looks okay, > > Jan, thanks for your review. > >> but I'm confused by this paragraph: >> There's no patch with the named title later in this series. And having gone >> through this patch I also don't see what remains to be addressed wrt the >> __must_check-s getting added here. > > This paragraph was added from a few rounds ago. I will drop it in next > version. Well, if dropping this paragraph is all that's needed, I can do this while committing: Patches 1-3 appear to be ready to go in. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [PATCH] xen-blkfront: save uncompleted reqs in blkfront_resume()
Uncompleted reqs used to be 'saved and resubmitted' in blkfront_recover() during migration, but that's too later after multi-queue introduced. After a migrate to another host (which may not have multiqueue support), the number of rings (block hardware queues) may be changed and the ring and shadow structure will also be reallocated. So that blkfront_recover() can't 'save and resubmit' the real uncompleted reqs because shadow structure has been reallocated. This patch fixes this issue by moving the 'save and resubmit' logic out of blkfront_recover() to earlier place:blkfront_resume(). Signed-off-by: Bob Liu --- drivers/block/xen-blkfront.c | 91 +++- 1 file changed, 40 insertions(+), 51 deletions(-) diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c index 2e6d1e9..fcc5b4e 100644 --- a/drivers/block/xen-blkfront.c +++ b/drivers/block/xen-blkfront.c @@ -207,6 +207,9 @@ struct blkfront_info struct blk_mq_tag_set tag_set; struct blkfront_ring_info *rinfo; unsigned int nr_rings; + /* Save uncomplete reqs and bios for migration. */ + struct list_head requests; + struct bio_list bio_list; }; static unsigned int nr_minors; @@ -2002,69 +2005,22 @@ static int blkif_recover(struct blkfront_info *info) { unsigned int i, r_index; struct request *req, *n; - struct blk_shadow *copy; int rc; struct bio *bio, *cloned_bio; - struct bio_list bio_list, merge_bio; unsigned int segs, offset; int pending, size; struct split_bio *split_bio; - struct list_head requests; blkfront_gather_backend_features(info); segs = info->max_indirect_segments ? : BLKIF_MAX_SEGMENTS_PER_REQUEST; blk_queue_max_segments(info->rq, segs); - bio_list_init(&bio_list); - INIT_LIST_HEAD(&requests); for (r_index = 0; r_index < info->nr_rings; r_index++) { - struct blkfront_ring_info *rinfo; - - rinfo = &info->rinfo[r_index]; - /* Stage 1: Make a safe copy of the shadow state. */ - copy = kmemdup(rinfo->shadow, sizeof(rinfo->shadow), - GFP_NOIO | __GFP_REPEAT | __GFP_HIGH); - if (!copy) - return -ENOMEM; - - /* Stage 2: Set up free list. */ - memset(&rinfo->shadow, 0, sizeof(rinfo->shadow)); - for (i = 0; i < BLK_RING_SIZE(info); i++) - rinfo->shadow[i].req.u.rw.id = i+1; - rinfo->shadow_free = rinfo->ring.req_prod_pvt; - rinfo->shadow[BLK_RING_SIZE(info)-1].req.u.rw.id = 0x0fff; + struct blkfront_ring_info *rinfo = &info->rinfo[r_index]; rc = blkfront_setup_indirect(rinfo); - if (rc) { - kfree(copy); + if (rc) return rc; - } - - for (i = 0; i < BLK_RING_SIZE(info); i++) { - /* Not in use? */ - if (!copy[i].request) - continue; - - /* -* Get the bios in the request so we can re-queue them. -*/ - if (copy[i].request->cmd_flags & - (REQ_FLUSH | REQ_FUA | REQ_DISCARD | REQ_SECURE)) { - /* -* Flush operations don't contain bios, so -* we need to requeue the whole request -*/ - list_add(©[i].request->queuelist, &requests); - continue; - } - merge_bio.head = copy[i].request->bio; - merge_bio.tail = copy[i].request->biotail; - bio_list_merge(&bio_list, &merge_bio); - copy[i].request->bio = NULL; - blk_end_request_all(copy[i].request, 0); - } - - kfree(copy); } xenbus_switch_state(info->xbdev, XenbusStateConnected); @@ -2079,7 +2035,7 @@ static int blkif_recover(struct blkfront_info *info) kick_pending_request_queues(rinfo); } - list_for_each_entry_safe(req, n, &requests, queuelist) { + list_for_each_entry_safe(req, n, &info->requests, queuelist) { /* Requeue pending requests (flush or discard) */ list_del_init(&req->queuelist); BUG_ON(req->nr_phys_segments > segs); @@ -2087,7 +2043,7 @@ static int blkif_recover(struct blkfront_info *info) } blk_mq_kick_requeue_list(info->rq); - while ((bio = bio_list_pop(&bio_list)) != NULL) { + while ((bio = bio_list_pop(&info->bio_list)) != NULL) { /* Traverse the list of pendi
Re: [Xen-devel] [PATCH v12 1/6] IOMMU: add a timeout parameter for device IOTLB invalidation
On June 27, 2016 4:29 PM, Jan Beulich wrote: > >>> On 27.06.16 at 10:19, wrote: > > On June 27, 2016 4:03 PM, Jan Beulich wrote: > >> >>> On 24.06.16 at 07:51, wrote: > >> > From: Quan Xu > >> > > >> > The parameter 'iommu_dev_iotlb_timeout' specifies the timeout of > >> > device IOTLB invalidation in milliseconds. By default, the timeout > >> > is > >> > 1000 milliseconds, which can be boot-time changed. > >> > > >> > We also confirmed with VT-d hardware team that 1 milliseconds is > >> > large enough for VT-d IOMMU internal invalidation. > >> > > >> > the existing panic() is eliminated and we bubble up the timeout of > >> > device IOTLB invalidation for further processing, as the PCI-e > >> > Address Translation Services (ATS) mandates a timeout of > >> > 60 seconds for device IOTLB invalidation. Obviously we can't spin > >> > for > >> > 60 seconds or otherwise Xen hypervisor hangs. > >> > > >> > Add a __must_check annotation. The followup patch titled 'VT-d > >> > IOTLB/Context/IEC flush issue' addresses the __mustcheck. > >> > That is the other callers of this routine (two or three levels up) > >> > ignore the return code. This patch does not address this but the > >> > other does. > >> > >> The patch itself looks okay, > > > > Jan, thanks for your review. > > > >> but I'm confused by this paragraph: > >> There's no patch with the named title later in this series. And > >> having gone through this patch I also don't see what remains to be > >> addressed wrt the __must_check-s getting added here. > > > > This paragraph was added from a few rounds ago. I will drop it in next > > version. > > Well, if dropping this paragraph is all that's needed, Yes, > I can do this while > committing: Patches 1-3 appear to be ready to go in. > Ah, that's great. Thanks. Quan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [qemu-mainline test] 96288: regressions - FAIL
flight 96288 qemu-mainline real [real] http://logs.test-lab.xenproject.org/osstest/logs/96288/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-amd64-i386-xl-qemuu-debianhvm-amd64-xsm 9 debian-hvm-install fail REGR. vs. 94856 test-amd64-amd64-xl-qemuu-debianhvm-amd64 9 debian-hvm-install fail REGR. vs. 94856 test-amd64-i386-xl-qemuu-debianhvm-amd64 9 debian-hvm-install fail REGR. vs. 94856 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 9 debian-hvm-install fail REGR. vs. 94856 test-amd64-amd64-xl-qemuu-debianhvm-amd64-xsm 9 debian-hvm-install fail REGR. vs. 94856 test-amd64-i386-xl-qemuu-ovmf-amd64 9 debian-hvm-install fail REGR. vs. 94856 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 9 debian-hvm-install fail REGR. vs. 94856 test-amd64-amd64-xl-qemuu-ovmf-amd64 9 debian-hvm-install fail REGR. vs. 94856 Regressions which are regarded as allowable (not blocking): test-amd64-i386-xl-qemuu-win7-amd64 16 guest-stop fail like 94856 test-amd64-amd64-xl-qemuu-win7-amd64 16 guest-stop fail like 94856 test-amd64-amd64-xl-rtds 9 debian-install fail like 94856 Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-pvh-amd 11 guest-start fail never pass test-armhf-armhf-libvirt 14 guest-saverestorefail never pass test-armhf-armhf-libvirt 12 migrate-support-checkfail never pass test-armhf-armhf-libvirt-xsm 12 migrate-support-checkfail never pass test-armhf-armhf-libvirt-xsm 14 guest-saverestorefail never pass test-amd64-amd64-libvirt-xsm 12 migrate-support-checkfail never pass test-amd64-amd64-qemuu-nested-amd 16 debian-hvm-install/l1/l2 fail never pass test-armhf-armhf-xl-arndale 12 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 13 saverestore-support-checkfail never pass test-amd64-i386-libvirt 12 migrate-support-checkfail never pass test-amd64-amd64-xl-pvh-intel 11 guest-start fail never pass test-amd64-amd64-libvirt-vhd 11 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-credit2 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 12 migrate-support-checkfail never pass test-amd64-amd64-libvirt 12 migrate-support-checkfail never pass test-armhf-armhf-xl-xsm 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-xsm 12 migrate-support-checkfail never pass test-armhf-armhf-xl 12 migrate-support-checkfail never pass test-armhf-armhf-xl 13 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 11 migrate-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 13 guest-saverestorefail never pass test-armhf-armhf-libvirt-raw 13 guest-saverestorefail never pass test-armhf-armhf-libvirt-raw 11 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 12 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 11 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 12 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 12 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-rtds 12 migrate-support-checkfail never pass version targeted for testing: qemuua01aef5d2f96c334d048f43f0d3573a1152b37ca baseline version: qemuud6550e9ed2e1a60d889dfb721de00d9a4e3bafbe Last test of basis94856 2016-05-27 20:14:49 Z 30 days Failing since 94983 2016-05-31 09:40:12 Z 26 days 37 attempts Testing same since96228 2016-06-24 19:43:51 Z2 days4 attempts People who touched revisions under test: Alberto Garcia Alex Bennée Alex Bligh Alex Williamson Alexander Graf Alexey Kardashevskiy Alistair Francis Amit Shah Andrea Arcangeli Andrew Jeffery Andrew Jones Aneesh Kumar K.V Anthony PERARD Anton Blanchard Ard Biesheuvel Artyom Tarasenko Benjamin Herrenschmidt Bharata B Rao Cao jin Changlong Xie Chao Peng Chen Fan Christian Borntraeger Christophe Lyon Cole Robinson Colin Lord Corey Minyard Cornelia Huck Cédric Le Goater Daniel P. Berrange David Gibson David Hildenbrand Denis V. Lunev Dmitry Fleytman Dmitry Fleytman Dmitry Osipenko Dr. David Alan Gilbert Drew Jones Edgar
[Xen-devel] FIXME question
Hi, When I read IOMMU code, In xen/drivers/passthrough/vtd/intremap.c : pi_update_irte().. There are a FIXME -- '' * FIXME: For performance reasons we should store the 'iommu' pointer in * 'struct msi_desc' in some other place, so we don't need to waste * time searching it here. " IMO, we are better to store the 'iommu' pointer in pci_dev, then could I fix it as: 1. save a void *iommu in pci_dev structure: --- a/xen/include/xen/pci.h +++ b/xen/include/xen/pci.h @@ -83,6 +83,8 @@ struct pci_dev { #define PT_FAULT_THRESHOLD 10 } fault; u64 vf_rlen[6]; + + void *iommu; /* No common IOMMU struct so use void pointer */ }; 2. Save iommu pointer in 'struct pci_dev' when to add device: --- a/xen/drivers/passthrough/vtd/iommu.c +++ b/xen/drivers/passthrough/vtd/iommu.c @@ -1994,6 +1994,7 @@ static int intel_iommu_enable_device(struct pci_dev *pdev) if ( ret <= 0 ) return ret; +pdev->iommu = drhd->iommu; ret = enable_ats_device(pdev->seg, pdev->bus, pdev->devfn, drhd->iommu); 3. use iommu pointer from pci_dev instead of calling acpi_find_matched_drhd_unit each time (also fix the related code). -Quan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [libvirt test] 96299: tolerable FAIL - PUSHED
flight 96299 libvirt real [real] http://logs.test-lab.xenproject.org/osstest/logs/96299/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-armhf-armhf-libvirt-xsm 12 migrate-support-checkfail never pass test-armhf-armhf-libvirt-xsm 14 guest-saverestorefail 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-libvirt 12 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 12 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 13 guest-saverestorefail never pass test-armhf-armhf-libvirt-raw 11 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 12 migrate-support-checkfail never pass test-amd64-i386-libvirt 12 migrate-support-checkfail 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 test-amd64-amd64-libvirt-vhd 11 migrate-support-checkfail never pass version targeted for testing: libvirt 0b4645a7e061abc8a4be71fe89865cf248ce6e56 baseline version: libvirt d0a9dbc323118cfd78f078e1fff48207e82d4b8e Last test of basis96270 2016-06-26 04:19:46 Z1 days Testing same since96299 2016-06-27 04:21:09 Z0 days1 attempts People who touched revisions under test: Laine Stump Vasiliy Tolstov jobs: build-amd64-xsm pass build-armhf-xsm pass build-i386-xsm pass build-amd64 pass build-armhf pass build-i386 pass build-amd64-libvirt pass build-armhf-libvirt pass build-i386-libvirt pass build-amd64-pvopspass build-armhf-pvopspass build-i386-pvops pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsmpass test-amd64-amd64-libvirt-xsm pass test-armhf-armhf-libvirt-xsm fail test-amd64-i386-libvirt-xsm pass test-amd64-amd64-libvirt pass test-armhf-armhf-libvirt fail test-amd64-i386-libvirt pass test-amd64-amd64-libvirt-pairpass test-amd64-i386-libvirt-pair pass test-armhf-armhf-libvirt-qcow2 fail test-armhf-armhf-libvirt-raw fail test-amd64-amd64-libvirt-vhd pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Pushing revision : + branch=libvirt + revision=0b4645a7e061abc8a4be71fe89865cf248ce6e56 + . ./cri-lock-repos ++ . ./cri-common +++ . ./cri-getconfig +++ umask 002 +++ getrepos getconfig Repos perl -e ' use Osstest; readglobalconfig(); print $c{"Repos"} or die $!; ' +++ local repos=/home/osstest/repos +++ '[' -z /home/osstest/repos ']' +++ '[' '!' -d /home/osstest/repos ']' +++ echo /home/osstest/repos ++ repos=/home/osstest/repos ++ repos_lock=/home/osstest/repos/lock ++ '[' x '!=' x/home/osstest/repos/lock ']' ++ OSSTEST_REPOS_LOCK_LOCKED=/home/osstest/repos/lock ++ exec with-lock-ex -w /home/osstest/repos/lock ./ap-push libvirt 0b4645a7e061abc8a4be71fe89865cf248ce6e56 + branch=libvirt + revision=0b4645a7e061abc8a
Re: [Xen-devel] FIXME question
> -Original Message- > From: Xu, Quan > Sent: Monday, June 27, 2016 5:24 PM > To: xen-devel@lists.xen.org > Cc: Jan Beulich ; Tian, Kevin ; Wu, > Feng > Subject: FIXME question > > Hi, > > When I read IOMMU code, > In xen/drivers/passthrough/vtd/intremap.c : pi_update_irte().. > There are a FIXME -- > '' > * FIXME: For performance reasons we should store the 'iommu' pointer in > * 'struct msi_desc' in some other place, so we don't need to waste > * time searching it here. > " > > IMO, we are better to store the 'iommu' pointer in pci_dev, then > could I fix it as: Yes, I think saving the 'iommu' pointer in pci_dev is the right direction. Thanks, Feng > > 1. save a void *iommu in pci_dev structure: > > --- a/xen/include/xen/pci.h > +++ b/xen/include/xen/pci.h > @@ -83,6 +83,8 @@ struct pci_dev { > #define PT_FAULT_THRESHOLD 10 > } fault; > u64 vf_rlen[6]; > + > + void *iommu; /* No common IOMMU struct so use void pointer */ > }; > > > > 2. Save iommu pointer in 'struct pci_dev' when to add device: > > --- a/xen/drivers/passthrough/vtd/iommu.c > +++ b/xen/drivers/passthrough/vtd/iommu.c > @@ -1994,6 +1994,7 @@ static int intel_iommu_enable_device(struct pci_dev > *pdev) > if ( ret <= 0 ) > return ret; > > +pdev->iommu = drhd->iommu; > ret = enable_ats_device(pdev->seg, pdev->bus, pdev->devfn, drhd->iommu); > > 3. use iommu pointer from pci_dev instead of calling > acpi_find_matched_drhd_unit each time (also fix the related code). > > > -Quan > ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] FIXME question
>>> On 27.06.16 at 11:23, wrote: > Hi, > > When I read IOMMU code, > In xen/drivers/passthrough/vtd/intremap.c : pi_update_irte().. > There are a FIXME -- > '' > * FIXME: For performance reasons we should store the 'iommu' pointer in > * 'struct msi_desc' in some other place, so we don't need to waste > * time searching it here. > " > > IMO, we are better to store the 'iommu' pointer in pci_dev, then > could I fix it as: Sounds reasonable. Jan > 1. save a void *iommu in pci_dev structure: > > --- a/xen/include/xen/pci.h > +++ b/xen/include/xen/pci.h > @@ -83,6 +83,8 @@ struct pci_dev { > #define PT_FAULT_THRESHOLD 10 > } fault; > u64 vf_rlen[6]; > + > + void *iommu; /* No common IOMMU struct so use void pointer */ > }; > > > > 2. Save iommu pointer in 'struct pci_dev' when to add device: > > --- a/xen/drivers/passthrough/vtd/iommu.c > +++ b/xen/drivers/passthrough/vtd/iommu.c > @@ -1994,6 +1994,7 @@ static int intel_iommu_enable_device(struct pci_dev > *pdev) > if ( ret <= 0 ) > return ret; > > +pdev->iommu = drhd->iommu; > ret = enable_ats_device(pdev->seg, pdev->bus, pdev->devfn, drhd->iommu); > > 3. use iommu pointer from pci_dev instead of calling > acpi_find_matched_drhd_unit each time (also fix the related code). > > > -Quan > ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 00/17] XSM/FLASK updates for 4.8
On 21/06/16 16:24, Andrew Cooper wrote: > On 20/06/16 15:04, Daniel De Graaf wrote: >> Changes from v1: >> - Change c->context and c->sid from arrays to fields when shrinking >> - Keep struct xen_flask_userlist in headers, but guard it with #ifs >> - Split off Kconfig changes into their own patches >> - Add patch 16 (AVC_STATS in Kconfig) >> - Prevent free() of static data in xsm_dt_init >> >> FLASK policy updates: >> [PATCH 01/17] flask/policy: split into modules >> [PATCH 02/17] flask/policy: split out rules for system_r >> [PATCH 03/17] flask/policy: move user definitions and constraints >> [PATCH 04/17] flask/policy: remove unused support for binary modules >> [PATCH 05/17] flask/policy: xenstore stubdom policy >> [PATCH 06/17] flask/policy: remove unused example >> >> Hypervisor updates to the FLASK security server: >> [PATCH 07/17] flask: unify {get,set}vcpucontext permissions >> [PATCH 08/17] flask: remove unused secondary context in ocontext >> [PATCH 09/17] flask: remove unused AVC callback functions >> [PATCH 10/17] flask: remove xen_flask_userlist operation >> [PATCH 11/17] flask: improve unknown permission handling >> >> Hypervisor updates to the XSM framework (and its config): >> [PATCH 12/17] xen/xsm: remove .xsm_initcall.init section >> [PATCH 13/17] xen: fix FLASK dependency in Kconfig >> [PATCH 14/17] xsm: annotate setup functions with __init >> [PATCH 15/17] xsm: clean up unregistration >> [PATCH 16/17] xen: Make FLASK_AVC_STATS kconfig option visible >> [PATCH 17/17] xsm: add a default policy to .init.data > I have committed the first two sections. Patch 12 requires an ARM ack, > and patch 13 has some outstanding discussion. In the interest of getting this work more widely tested, I have pushed the remaining patches, now that acks have appeared. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 15/17] libxl/arm: Add ACPI module
Hi Shannon, On 25/06/16 04:22, Shannon Zhao wrote: On 2016/6/24 2:35, Julien Grall wrote: Hi Shannon, On 23/06/2016 04:17, Shannon Zhao wrote: From: Shannon Zhao Add the ARM Multiboot module for ACPI, so UEFI or DomU can get the base address of ACPI tables from it. Signed-off-by: Shannon Zhao --- docs/misc/arm/device-tree/acpi.txt | 23 +++ tools/libxl/libxl_arm.c| 24 2 files changed, 47 insertions(+) create mode 100644 docs/misc/arm/device-tree/acpi.txt diff --git a/docs/misc/arm/device-tree/acpi.txt b/docs/misc/arm/device-tree/acpi.txt new file mode 100644 index 000..c39c4d0 --- /dev/null +++ b/docs/misc/arm/device-tree/acpi.txt @@ -0,0 +1,23 @@ +DomU ACPI module + + +Xen toolstack passes the domU ACPI tables via a reference in the /chosen node of +the device tree. + +Each node contains the following properties: + +- compatible + +"xen,guest-acpi", "multiboot,module" + +- reg + +Specifies the physical address and the length of the module. We need to clarify how the firmware can find the RSDP. I.e will it always be at the beginning of the region? Also, do we really need the size of the region? Would not be simpler to give the base address of RSDP? Maybe it needs for some other UEFI implementations which will directly pass the ACPI tables information with a UEFI memory map to guest like you said. So maybe it needs the size to construct the UEFI memory map. Good point. Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] FIXME question
On 27/06/16 10:32, Jan Beulich wrote: On 27.06.16 at 11:23, wrote: >> Hi, >> >> When I read IOMMU code, >> In xen/drivers/passthrough/vtd/intremap.c : pi_update_irte().. >> There are a FIXME -- >> '' >> * FIXME: For performance reasons we should store the 'iommu' pointer in >> * 'struct msi_desc' in some other place, so we don't need to waste >> * time searching it here. >> " >> >> IMO, we are better to store the 'iommu' pointer in pci_dev, then >> could I fix it as: > Sounds reasonable. I agree. > > Jan > >> 1. save a void *iommu in pci_dev structure: >> >> --- a/xen/include/xen/pci.h >> +++ b/xen/include/xen/pci.h >> @@ -83,6 +83,8 @@ struct pci_dev { >> #define PT_FAULT_THRESHOLD 10 >> } fault; >> u64 vf_rlen[6]; >> + >> + void *iommu; /* No common IOMMU struct so use void pointer */ Longer term, it would be nice to have concrete IOMMU type with implementation specific fields hidden in a union, but void * will work for now. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] FIXME question
> -Original Message- > From: Andrew Cooper [mailto:andrew.coop...@citrix.com] > Sent: Monday, June 27, 2016 5:51 PM > To: Jan Beulich ; Xu, Quan > Cc: Tian, Kevin ; Wu, Feng ; xen- > de...@lists.xen.org > Subject: Re: [Xen-devel] FIXME question > > On 27/06/16 10:32, Jan Beulich wrote: > On 27.06.16 at 11:23, wrote: > >> Hi, > >> > >> When I read IOMMU code, > >> In xen/drivers/passthrough/vtd/intremap.c : pi_update_irte().. > >> There are a FIXME -- > >> '' > >> * FIXME: For performance reasons we should store the 'iommu' pointer in > >> * 'struct msi_desc' in some other place, so we don't need to waste > >> * time searching it here. > >> " > >> > >> IMO, we are better to store the 'iommu' pointer in pci_dev, then > >> could I fix it as: > > Sounds reasonable. > > I agree. > > > > > Jan > > > >> 1. save a void *iommu in pci_dev structure: > >> > >> --- a/xen/include/xen/pci.h > >> +++ b/xen/include/xen/pci.h > >> @@ -83,6 +83,8 @@ struct pci_dev { > >> #define PT_FAULT_THRESHOLD 10 > >> } fault; > >> u64 vf_rlen[6]; > >> + > >> + void *iommu; /* No common IOMMU struct so use void pointer */ > > Longer term, it would be nice to have concrete IOMMU type with > implementation specific fields hidden in a union, but void * will work > for now. Agree, I think something like below should be the case: struct arch_pci_dev { vmask_t used_vectors; +union { +struct acpi_drhd_unit *drhd; +/* AMD can add its counterpart here if needed */ +}; }; Thanks, Feng > > ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 07/17] libxl/arm: Construct ACPI GTDT table
Hi Shannon, On 27/06/16 02:44, Shannon Zhao wrote: On 2016/6/24 0:26, Julien Grall wrote: On 23/06/16 04:16, Shannon Zhao wrote: From: Shannon Zhao Construct GTDT table with the interrupt information of timers. Signed-off-by: Shannon Zhao --- tools/libxl/libxl_arm_acpi.c | 28 1 file changed, 28 insertions(+) diff --git a/tools/libxl/libxl_arm_acpi.c b/tools/libxl/libxl_arm_acpi.c index d5ffedf..de863f4 100644 --- a/tools/libxl/libxl_arm_acpi.c +++ b/tools/libxl/libxl_arm_acpi.c @@ -39,6 +39,9 @@ typedef uint64_t u64; #define ACPI_BUILD_APPNAME6 "XenARM" #define ACPI_BUILD_APPNAME4 "Xen " +#define ACPI_LEVEL_SENSITIVE(u8) 0x00 +#define ACPI_ACTIVE_LOW (u8) 0x01 + Why did not you include actypes.h rather than define these two defines? If we include actypes.h, there will be some compiling errors. ../../xen/include/acpi/actypes.h:55:2: error: #error ACPI_MACHINE_WIDTH not defined #error ACPI_MACHINE_WIDTH not defined ^ ../../xen/include/acpi/actypes.h:130:9: error: unknown type name 'COMPILER_DEPENDENT_UINT64' typedef COMPILER_DEPENDENT_UINT64 UINT64; ^ ../../xen/include/acpi/actypes.h:131:9: error: unknown type name 'COMPILER_DEPENDENT_INT64' typedef COMPILER_DEPENDENT_INT64 INT64; ^ ../../xen/include/acpi/actypes.h:202:2: error: #error unknown ACPI_MACHINE_WIDTH #error unknown ACPI_MACHINE_WIDTH ^ ../../xen/include/acpi/actypes.h:207:9: error: unknown type name 'acpi_native_uint' typedef acpi_native_uint acpi_size; ^ ../../xen/include/acpi/actypes.h:617:3: error: unknown type name 'acpi_io_address' acpi_io_address pblk_address; Yeah, it maybe can be solved by defining ACPI_MACHINE_WIDTH and COMPILER_DEPENDENT_INT64 here, but since we only needs ACPI_LEVEL_SENSITIVE and ACPI_ACTIVE_LOW, I think it's ok to define them here. We should avoid to redefine value as much as possible. The 2 missing values are easy to define (see below) so there is no point to redefine in a less obvious way: no comment to explain what the values are for, and only a part of the set defined. #define ACPI_MACHINE_WIDTH BITS_PER_LONG #define COMPILER_DEPENDENT_INT64 int64_t Wei, Ian, Stefano, do you have any opinions? Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 11/17] libxl/arm: Construct ACPI DSDT table
(CC Boris and Doug) Hi Shannon, On 27/06/16 07:01, Shannon Zhao wrote: On 2016/6/24 1:03, Julien Grall wrote: On 23/06/16 04:16, Shannon Zhao wrote: [...] diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile index 264b6ef..5347480 100644 --- a/tools/libxl/Makefile +++ b/tools/libxl/Makefile @@ -77,7 +77,29 @@ endif LIBXL_OBJS-$(CONFIG_X86) += libxl_cpuid.o libxl_x86.o libxl_psr.o LIBXL_OBJS-$(CONFIG_ARM) += libxl_nocpuid.o libxl_arm.o libxl_libfdt_compat.o -LIBXL_OBJS-$(CONFIG_ARM) += libxl_arm_acpi.o +LIBXL_OBJS-$(CONFIG_ARM) += libxl_arm_acpi.o libxl_dsdt_anycpu_arm.o + +vpath iasl $(PATH) +libxl_mk_dsdt_arm: libxl_mk_dsdt_arm.c +$(CC) $(CFLAGS) -o $@ libxl_mk_dsdt_arm.c + +libxl_dsdt_anycpu_arm.asl: libxl_empty_dsdt_arm.asl libxl_mk_dsdt_arm +awk 'NR > 1 {print s} {s=$$0}' $< > $@ +./libxl_mk_dsdt_arm >> $@ + +libxl_dsdt_anycpu_arm.c: %.c: iasl %.asl +iasl -vs -p $* -tc $*.asl +sed -e 's/AmlCode/$*/g' $*.hex >$@ +echo "int $*_len=sizeof($*);" >>$@ +rm -f $*.aml $*.hex + I don't like the idea to add iasl as a dependency for all ARM platforms. For instance ARMv7 platform will not use ACPI, but we still ask users to install iasl. So I think we should allow the user to opt-in/opt-out for ACPI. Any opinions? I agree. But how to exclude for ARMv7. I notice it only has the option CONFIG_ARM which doesn't distinguish ARM32 and ARM64. I am not sure if we plan to introduce Kconfig for tools. If not, you can add an option to the configure to enable/disable ACPI for guest. This would be gated by the presence of "iasl". [...] diff --git a/tools/libxl/libxl_mk_dsdt_arm.c b/tools/libxl/libxl_mk_dsdt_arm.c new file mode 100644 index 000..96fadbd --- /dev/null +++ b/tools/libxl/libxl_mk_dsdt_arm.c Can we share the code from tools/firmware/acpi/mk_dsdt.c? Yeah, we can share push_block(), pop_block() stmt() and indent() but the main() function is totally different since there are only the processor device objects for ARM DSDT but there are many other things in x86. I think that since Boris will move the codes under tools/firmware/hvmloader/acpi to other place, after that we could see how to share codes then. I would prefer if we discuss about it now in order to avoid code duplication (I have CCed Boris). For instance we can create a new directory under tools for mk_dsdt.c. The main could be different, although it might be possible to gate ARM options, and the rest of the code would be shared. Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v5 0/6] Support calling functions on dedicated physical cpu
On 06/04/16 16:17, Juergen Gross wrote: > Some hardware (e.g. Dell Studio laptops) require special functions to > be called on physical cpu 0 in order to avoid occasional hangs. When > running as dom0 under Xen this could be achieved only via special boot > parameters (vcpu pinning) limiting the hypervisor in it's scheduling > decisions. > > This patch series is adding a generic function to be able to temporarily > pin a (virtual) cpu to a dedicated physical cpu for executing above > mentioned functions on that specific cpu. The drivers (dcdbas and i8k) > requiring this functionality are modified accordingly. David, as I've got no reactions when asking for the last outstanding Acks (patches 2 and 5) and neither Ingo nor Peter seem to be willing to take the patches, I'd like to suggest we carry the series via the xen tree. Are you okay with this suggestion? Juergen > > Changes in V5: > - patch 3: rename and reshuffle parameters of smp_call_on_cpu() as requested > by Peter Zijlstra > - patch 3: test target cpu to be online as requested by Peter Zijlstra > - patch 4: less wordy messages as requested by David Vrabel > > Changes in V4: > - move patches 5 and 6 further up in the series > - patch 2 (was 5): WARN_ONCE in case platform doesn't support pinning > as requested by Peter Zijlstra > - patch 3 (was 2): change return value in case of illegal cpu as > requested by Peter Zijlstra > - patch 3 (was 2): make pinning of vcpu an option as suggested by > Peter Zijlstra > - patches 5 and 6 (were 3 and 4): add call to get_online_cpus() > > Changes in V3: > - use get_cpu()/put_cpu() as suggested by David Vrabel > > Changes in V2: > - instead of manipulating the allowed set of cpus use cpu specific > workqueue as requested by Peter Zijlstra > - add include/linux/hypervisor.h to hide architecture specific stuff > from generic kernel code > > Juergen Gross (6): > xen: sync xen header > virt, sched: add generic vcpu pinning support > smp: add function to execute a function synchronously on a cpu > xen: add xen_pin_vcpu() to support calling functions on a dedicated > pcpu > dcdbas: make use of smp_call_on_cpu() > hwmon: use smp_call_on_cpu() for dell-smm i8k > > MAINTAINERS | 1 + > arch/x86/include/asm/hypervisor.h | 4 ++ > arch/x86/kernel/cpu/hypervisor.c | 11 + > arch/x86/xen/enlighten.c | 40 +++ > drivers/firmware/dcdbas.c | 51 +-- > drivers/hwmon/dell-smm-hwmon.c| 35 +++-- > include/linux/hypervisor.h| 17 +++ > include/linux/smp.h | 3 ++ > include/xen/interface/sched.h | 100 > +++--- > kernel/smp.c | 51 +++ > kernel/up.c | 18 +++ > 11 files changed, 273 insertions(+), 58 deletions(-) > create mode 100644 include/linux/hypervisor.h > ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 03/17] libxl/arm: Add a configuration option for ARM DomU ACPI
Hi Shannon, On 23/06/16 15:34, Shannon Zhao wrote: On 2016年06月23日 21:39, Stefano Stabellini wrote: On Thu, 23 Jun 2016, Shannon Zhao wrote: From: Shannon Zhao Add a configuration option for ARM DomU so that user can deicde to use ACPI or not. This option is defaultly false. Signed-off-by: Shannon Zhao --- tools/libxl/libxl_arm.c | 3 +++ tools/libxl/libxl_types.idl | 1 + tools/libxl/xl_cmdimpl.c | 4 xen/include/public/arch-arm.h | 1 + 4 files changed, 9 insertions(+) diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c index 8f15d9b..cc5a717 100644 --- a/tools/libxl/libxl_arm.c +++ b/tools/libxl/libxl_arm.c @@ -77,6 +77,9 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc, return ERROR_FAIL; } +xc_config->acpi = libxl_defbool_val(d_config->b_info.arch_arm.acpi) + ? true : false; + return 0; } diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl index ef614be..426b868 100644 --- a/tools/libxl/libxl_types.idl +++ b/tools/libxl/libxl_types.idl @@ -560,6 +560,7 @@ libxl_domain_build_info = Struct("domain_build_info",[ ("arch_arm", Struct(None, [("gic_version", libxl_gic_version), + ("acpi", libxl_defbool), ])), ], dir=DIR_IN diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c index 6459eec..0634ffa 100644 --- a/tools/libxl/xl_cmdimpl.c +++ b/tools/libxl/xl_cmdimpl.c @@ -2506,6 +2506,10 @@ skip_usbdev: } } +if (xlu_cfg_get_defbool(config, "acpi", &b_info->arch_arm.acpi, 0)) { +libxl_defbool_set(&b_info->arch_arm.acpi, 0); +} We cannot share the existing code to parse the acpi paramter because that is saved in b_info->u.hvm.acpi, right? Yes. It's a pity. I wonder if we could refactor the existing code so that we can actually share the acpi parameter between x86 and arm. I have no idea about this since I'm not familiar with this. But is there any downsides of current way? Because for x86, it will use b_info->u.hvm.acpi and for ARM it will use b_info->arch_arm.acpi. I think they don't conflict even though we store it at two places. Yes, there is a downside. Toolstack, such as libvirt, would need to have separate code for x86 and ARM in order to enable/disable ACPI. I would introduce a new generic acpi parameters, deprecate b_info->u.hvm.acpi. Ian, Stefano, Wei, any opinions? Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 16/17] libxc/xc_dom_arm: Copy ACPI tables to guest space
(CC Boris) On 27/06/16 07:25, Shannon Zhao wrote: On 2016/6/24 2:46, Julien Grall wrote: + static int alloc_magic_pages(struct xc_dom_image *dom) { > int rc, i; @@ -100,6 +141,16 @@ static int alloc_magic_pages(struct xc_dom_image *dom) xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_STORE_EVTCHN, dom->xenstore_evtchn); +if ( dom->acpitable_blob && dom->acpitable_size > 0 ) +{ +rc = xc_dom_copy_acpi(dom); +if ( rc != 0 ) +{ +DOMPRINTF("Unable to copy ACPI tables"); +return rc; +} +} alloc_magic_pages looks the wrong place with this function. Any reason to not have a generic ACPI blob loading in xc_dom_core.c as we do for devicetree? Looks like xc_dom_build_image is used for allocating pages in guest RAM while ACPI blob is not put in guest RAM. The function xc_dom_build_image is used to load blob into the guest memory and allocate others pages (such as magic pages) which may not be part of the actual guest RAM. The callback alloc_magic_pages is used to allocate per-architecture specific pages. However, the ACPI blob is pretty much generic. So I am not sure why we would want to do it for ARM only. Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] xen: use native disk xenbus protocol if possible
On 17/06/16 13:14, Juergen Gross wrote: > The qdisk implementation is using the native xenbus protocol only in > case of no protocol specified at all. As using the explicit 32- or > 64-bit protocol is slower than the native one due to copying requests > not by memcpy but element for element, this is not optimal. > > Correct this by using the native protocol in case word sizes of > frontend and backend match. Ping? > > Signed-off-by: Juergen Gross > --- > hw/block/xen_disk.c | 16 > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c > index 2862b59..0961fcb 100644 > --- a/hw/block/xen_disk.c > +++ b/hw/block/xen_disk.c > @@ -976,14 +976,14 @@ static int blk_connect(struct XenDevice *xendev) > blkdev->feature_persistent = !!pers; > } > > -blkdev->protocol = BLKIF_PROTOCOL_NATIVE; > -if (blkdev->xendev.protocol) { > -if (strcmp(blkdev->xendev.protocol, XEN_IO_PROTO_ABI_X86_32) == 0) { > -blkdev->protocol = BLKIF_PROTOCOL_X86_32; > -} > -if (strcmp(blkdev->xendev.protocol, XEN_IO_PROTO_ABI_X86_64) == 0) { > -blkdev->protocol = BLKIF_PROTOCOL_X86_64; > -} > +if (!blkdev->xendev.protocol) { > +blkdev->protocol = BLKIF_PROTOCOL_NATIVE; > +} else if (strcmp(blkdev->xendev.protocol, XEN_IO_PROTO_ABI_NATIVE) == > 0) { > +blkdev->protocol = BLKIF_PROTOCOL_NATIVE; > +} else if (strcmp(blkdev->xendev.protocol, XEN_IO_PROTO_ABI_X86_32) == > 0) { > +blkdev->protocol = BLKIF_PROTOCOL_X86_32; > +} else if (strcmp(blkdev->xendev.protocol, XEN_IO_PROTO_ABI_X86_64) == > 0) { > +blkdev->protocol = BLKIF_PROTOCOL_X86_64; > } > > blkdev->sring = xengnttab_map_grant_ref(blkdev->xendev.gnttabdev, > ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH V2 01/10] arm/gic-v3: Fix bug in function cmp_rdist()
Hi Shanker, On 26/06/16 18:48, Shanker Donthineni wrote: The cmp_rdist() is always returning value zero irrespective of the input Redistributor base addresses. Both the local variables 'l' and 'r' are pointing to the first argument 'a' causing the logical expression 'l->base < r->base' always evaluated as false which is wrong. Signed-off-by: Shanker Donthineni --- xen/arch/arm/gic-v3.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c index 8d3f149..b89c608 100644 --- a/xen/arch/arm/gic-v3.c +++ b/xen/arch/arm/gic-v3.c @@ -1133,7 +1133,7 @@ static const hw_irq_controller gicv3_guest_irq_type = { static int __init cmp_rdist(const void *a, const void *b) { -const struct rdist_region *l = a, *r = a; +const struct rdist_region *l = a, *r = b; Thank you for spotting the error. The sorting was required because of the way the vGIC emulated the re-distributors. However, this code has been reworked and sorted array is not necessary anymore. So I would directly drop the sorting here. /* We assume that re-distributor regions can never overlap */ return ( l->base < r->base) ? -1 : 0; Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH V2 02/10] arm/gic-v3: Do early GICD ioremap and clean up
Hi Shanker, On 26/06/16 18:48, Shanker Donthineni wrote: For ACPI based XEN boot, the GICD region needs to be accessed inside the function gicv3_acpi_init() in later pacth. There is a duplicate s/pachth/patch/ panic() message, one in the DTS probe and second one in the ACPI probe path. For these two reasons, move the code that validates the GICD base address and does the region ioremap to a separate function. The following pacth accesses the GICD region inside gicv3_acpi_init() for s/pacth/patch/ finding per CPU Redistributor size. Signed-off-by: Shanker Donthineni Acked-by: Julien Grall Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v12 4/6] IOMMU/x86: using a struct pci_dev* instead of SBDF
On June 27, 2016 4:17 PM, Jan Beulich wrote: > >>> On 24.06.16 at 07:51, wrote: > > --- a/xen/drivers/passthrough/x86/ats.c > > +++ b/xen/drivers/passthrough/x86/ats.c > > @@ -22,26 +22,34 @@ LIST_HEAD(ats_devices); bool_t __read_mostly > > ats_enabled = 0; boolean_param("ats", ats_enabled); > > > > -int enable_ats_device(int seg, int bus, int devfn, const void *iommu) > > +int enable_ats_device(const void *iommu, struct pci_dev *pci_dev) > > Is there anything preventing the second parameter to become a pointer to > const too? Afaict that would in turn eliminate the need for some of the > changes further up. > If I make the second parameter to const, then compilation fails: """ ats.c: In function 'enable_ats_device': ats.c:71:23: error: assignment discards 'const' qualifier from pointer target type [-Werror] ats_dev->pdev = pdev; ^ """ Also I will hide pci_dev as device IOTLB flush error, with changing of pci_dev. A const 'struct pci_dev *' in 'struct pci_ats_dev' is not working.. I have not verified this, correct me if I am wrong. > > { > > struct pci_ats_dev *pdev = NULL; > > u32 value; > > int pos; > > > > -pos = pci_find_ext_capability(seg, bus, devfn, PCI_EXT_CAP_ID_ATS); > > +pos = pci_find_ext_capability(pci_dev->seg, pci_dev->bus, pci_dev- > >devfn, > > + PCI_EXT_CAP_ID_ATS); > > Please add local variables seg, bus, and devfn, which will greatly reduce the > number of changes you need to do to this function (and which likely will also > produce better code). Agreed. Good idea. > > > @@ -98,7 +104,13 @@ void disable_ats_device(int seg, int bus, int > > devfn) > > For symmetry reasons this function would also get converted to taking const > struct pci_dev *. > What about ' struct pci_dev *', without const? > > @@ -120,7 +132,13 @@ struct pci_ats_dev *get_ats_device(int seg, int > > bus, int devfn) > > And this one then probably too. > Ditto. Quan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH V2 03/10] arm/gic-v3: Fold GICR subtable parsing into a new function
Hi Shanker, Title: I think you want to say "Move GICR..." rather than "Fold GICR...". On 26/06/16 18:48, Shanker Donthineni wrote: Add a new function for parsing GICR subtable and move the code Add a new function to parse GICR... that is specific to GICR table to new function without changing to a new function the function gicv3_acpi_init() behavior. Signed-off-by: Shanker Donthineni --- Changes since v1: Removed the unnecessary GICR ioremap operation inside GICR table parse code. xen/arch/arm/gic-v3.c | 61 --- 1 file changed, 39 insertions(+), 22 deletions(-) diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c index 542c4f3..0471fea 100644 --- a/xen/arch/arm/gic-v3.c +++ b/xen/arch/arm/gic-v3.c @@ -1282,6 +1282,14 @@ static int gicv3_iomem_deny_access(const struct domain *d) } #ifdef CONFIG_ACPI +static void __init gic_acpi_add_rdist_region(u64 base_addr, u32 size) Please use paddr_t for both parameter. Also the suffix _addr is pointless. +{ +unsigned int idx = gicv3.rdist_count++; + +gicv3.rdist_regions[idx].base = base_addr; +gicv3.rdist_regions[idx].size = size; +} + static int gicv3_make_hwdom_madt(const struct domain *d, u32 offset) { struct acpi_subtable_header *header; @@ -1387,6 +1395,25 @@ gic_acpi_parse_madt_distributor(struct acpi_subtable_header *header, return 0; } + +static int __init +gic_acpi_parse_madt_redistributor(struct acpi_subtable_header *header, + const unsigned long end) +{ +struct acpi_madt_generic_redistributor *rdist; + +rdist = (struct acpi_madt_generic_redistributor *)header; +if ( BAD_MADT_ENTRY(rdist, end) ) +return -EINVAL; + +if ( !rdist->base_address || !rdist->length ) +return -EINVAL; In the commit message you said that the behavior is unchanged, however this check is not part of the previous code. Anyway, I don't think this check is necessary. + +gic_acpi_add_rdist_region(rdist->base_address, rdist->length); + +return 0; +} + static int __init gic_acpi_get_madt_redistributor_num(struct acpi_subtable_header *header, const unsigned long end) @@ -1402,7 +1429,7 @@ static void __init gicv3_acpi_init(void) struct acpi_table_header *table; struct rdist_region *rdist_regs; acpi_status status; -int count, i; +int count; status = acpi_get_table(ACPI_SIG_MADT, 0, &table); @@ -1433,37 +1460,27 @@ static void __init gicv3_acpi_init(void) if ( count <= 0 ) panic("GICv3: No valid GICR entries exists"); -gicv3.rdist_count = count; - -if ( gicv3.rdist_count > MAX_RDIST_COUNT ) +if ( count > MAX_RDIST_COUNT ) panic("GICv3: Number of redistributor regions is more than" "%d (Increase MAX_RDIST_COUNT!!)\n", MAX_RDIST_COUNT); -rdist_regs = xzalloc_array(struct rdist_region, gicv3.rdist_count); +rdist_regs = xzalloc_array(struct rdist_region, count); if ( !rdist_regs ) panic("GICv3: Failed to allocate memory for rdist regions\n"); -for ( i = 0; i < gicv3.rdist_count; i++ ) -{ -struct acpi_subtable_header *header; -struct acpi_madt_generic_redistributor *gic_rdist; - -header = acpi_table_get_entry_madt(ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR, - i); -if ( !header ) -panic("GICv3: Can't get GICR entry"); - -gic_rdist = - container_of(header, struct acpi_madt_generic_redistributor, header); -rdist_regs[i].base = gic_rdist->base_address; -rdist_regs[i].size = gic_rdist->length; -} +gicv3.rdist_regions = rdist_regs; + +/* Parse always-on power domain Re-distributor entries */ +count = acpi_parse_entries(ACPI_SIG_MADT, + sizeof(struct acpi_table_madt), + gic_acpi_parse_madt_redistributor, table, + ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR, count); Please use acpi_table_parse_madt here. +if ( count <= 0 ) +panic("GICv3: Can't get Redistributor entry"); /* The vGIC code requires the region to be sorted */ sort(rdist_regs, gicv3.rdist_count, sizeof(*rdist_regs), cmp_rdist, NULL); -gicv3.rdist_regions= rdist_regs; - /* Collect CPU base addresses */ count = acpi_parse_entries(ACPI_SIG_MADT, sizeof(struct acpi_table_madt), gic_acpi_parse_madt_cpu, table, Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] Xen 4.3, armhf, ocamlopt, wheezy
Since around January we have been testing our Xen 4.3 stable branches (which still receive security support until very soon) with a version of Debian which was current when 4.3 was also current - ie, wheezy. Debian wheezy has an `ocaml-nox' package, but it does not contain `ocamlopt' on armhf. There is code in `configure' in xen.git#staging-4.3 to attempt to discover whether ocamlopt exists. That code appears to work, correctly detecting that ocamlopt does not exist - but then the build tries to use it anyway: http://logs.test-lab.xenproject.org/osstest/logs/96291/build-armhf/5.ts-xen-build.log I would like to get one final push of Xen 4.3 before it goes out of security support. I don't think this configuration bug in Xen 4.3 is recently introduced. I think that previous tests we were probably using squeeze, which probably did not have ocaml at all, and that we were using an osstest which didn't try to use it. I propose to drop the ocaml package installation from osstest runs using Debian squeeze or Debian wheezy. Feel free to object, if you have a better plan. Also, you might like to consider whether the logic in xen-unstable is correct, wrt the use of ocamlopt. I can't seem even there to find the plumbing which makes use of the result of the configure check. Thanks, Ian. From 6fb8fdd67de8a4ca2b3b26a93f1a6664e3e06a01 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Mon, 27 Jun 2016 12:25:14 +0100 Subject: [OSSTEST PATCH] ts-xen-build-prep: Do not install Ocaml on squeeze or wheezy squeeze doesn't (didn't) have it at all. wheezy doesn't have ocamlopt on armhf, and the Xen build system (in the old branches where this is relevant) seems not to be able to test this. In any case we use these old Debian suites when testing old Xen branches, which were (when they were current) built without ocaml. This partially reverts "ts-xen-build-prep: Install Ocaml" bbe1a9b2a6c0. Signed-off-by: Ian Jackson CC: Andrew Cooper CC: Wei Liu CC: David Scott CC: Jan Beulich --- ts-xen-build-prep | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/ts-xen-build-prep b/ts-xen-build-prep index c8cebf4..0450811 100755 --- a/ts-xen-build-prep +++ b/ts-xen-build-prep @@ -206,9 +206,11 @@ sub prep () { autoconf automake libtool xsltproc libxml2-utils libxml2-dev libdevmapper-dev w3c-dtd-xhtml libxml-xpath-perl - ocaml-nox ocaml-findlib ccache nasm checkpolicy ebtables); +if ($ho->{Suite} !~ m/squeeze|wheezy/) { + push(@packages, qw(ocaml-nox ocaml-findlib)); +} if ($ho->{Suite} =~ m/wheezy|squeeze|lenny/) { push(@packages, "libnl-dev"); } else { -- 2.1.4 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [xen-unstable-smoke test] 96307: trouble: blocked/broken/pass
flight 96307 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/96307/ Failures and problems with tests :-( Tests which did not succeed and are blocking, including tests which could not be run: build-armhf 3 host-install(3) broken REGR. vs. 96214 Tests which did not succeed, but are not blocking: test-armhf-armhf-xl 1 build-check(1) blocked n/a test-amd64-amd64-libvirt 12 migrate-support-checkfail never pass version targeted for testing: xen 08cffe6696c047123bd552e095163924c8ef4353 baseline version: xen 8384dc2d95538c5910d98db3df3ff5448bf0af48 Last test of basis96214 2016-06-24 11:02:52 Z3 days Testing same since96307 2016-06-27 10:03:47 Z0 days1 attempts People who touched revisions under test: Daniel De Graaf Julien Grall jobs: build-amd64 pass build-armhf broken build-amd64-libvirt pass test-armhf-armhf-xl blocked test-amd64-amd64-xl-qemuu-debianhvm-i386 pass test-amd64-amd64-libvirt pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary broken-step build-armhf host-install(3) Not pushing. commit 08cffe6696c047123bd552e095163924c8ef4353 Author: Daniel De Graaf Date: Mon Jun 20 10:04:26 2016 -0400 xsm: add a default policy to .init.data This adds a Kconfig option and support for including the XSM policy from tools/flask/policy in the hypervisor so that the bootloader does not need to provide a policy to get sane behavior from an XSM-enabled hypervisor. The policy provided by the bootloader, if present, will override the built-in policy. Enabling this option only builds the policy if checkpolicy is available during compilation of the hypervisor; otherwise, it does nothing. The XSM policy is not moved out of tools because that remains the primary location for installing and configuring the policy. Signed-off-by: Daniel De Graaf Reviewed-by: Konrad Rzeszutek Wilk Reviewed-by: Doug Goldstein Reviewed-by: Andrew Cooper Acked-by: Julien Grall commit 668ba1f85bf2e4086cf18c35abc880b9eee4e8f2 Author: Daniel De Graaf Date: Mon Jun 20 10:04:25 2016 -0400 xen: Make FLASK_AVC_STATS kconfig option visible Signed-off-by: Daniel De Graaf Reviewed-by: Doug Goldstein commit 5efcebc66de0c34b071fea5c84e7d379541929af Author: Daniel De Graaf Date: Mon Jun 20 10:04:24 2016 -0400 xsm: clean up unregistration The only possible value of original_ops was &dummy_xsm_ops, and unregister_xsm was never used. Signed-off-by: Daniel De Graaf Reviewed-by: Andrew Cooper Reviewed-by: Konrad Rzeszutek Wilk Reviewed-by: Doug Goldstein commit 70dda5f4e9c92b35c88b8f52f0fddd52f8778e51 Author: Daniel De Graaf Date: Mon Jun 20 10:04:23 2016 -0400 xsm: annotate setup functions with __init These functions were only called from __init functions. Signed-off-by: Daniel De Graaf Reviewed-by: Andrew Cooper Reviewed-by: Doug Goldstein commit 6a962ebddce8f287588dfb6599d37ff7b6f583b7 Author: Daniel De Graaf Date: Tue Jun 21 13:09:23 2016 -0400 xen: move FLASK entry under XSM in Kconfig Since enabling XSM is required to enable FLASK, place the option for FLASK below the one for XSM. In addition, since it does not make sense to enable XSM without any XSM providers, and FLASK is the only XSM provider, hide the option to disable FLASK under EXPERT. Signed-off-by: Daniel De Graaf Reviewed-by: Doug Goldstein (qemu changes not included) ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH V2 04/10] arm/gic-v3: Parse per-cpu redistributor entry in GICC subtable
Hi Shanker, On 26/06/16 18:48, Shanker Donthineni wrote: The redistributor address can be specified either as part of GICC or GICR subtable depending on the power domain. The current driver doesn't support parsing redistributor entry that is defined in GICC subtable. The GIC CPU subtable entry holds the associated Redistributor base address if it is not on always-on power domain. The per CPU Redistributor size is not defined in ACPI specification. Set it's size to SZ_256K if the GIC hardware is capable of Direct s/it's/its/, although I would use "the". Virtual LPI Injection feature otherwise SZ_128K. "Set the size to SZ_256K if the GIC hardware is supporting Direct Virtual LPI injection, SZ_128K otherwise". This patch adds necessary code to handle both types of Redistributors base addresses. Signed-off-by: Shanker Donthineni --- Changes since v1: Edited commit text and fixed white spaces. Added a new function for parsing per CPU Redistributor entry. xen/arch/arm/gic-v3.c | 84 ++- xen/include/asm-arm/gic.h | 1 + xen/include/asm-arm/gic_v3_defs.h | 1 + 3 files changed, 77 insertions(+), 9 deletions(-) diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c index 0471fea..3977244 100644 --- a/xen/arch/arm/gic-v3.c +++ b/xen/arch/arm/gic-v3.c @@ -659,6 +659,10 @@ static int __init gicv3_populate_rdist(void) smp_processor_id(), i, ptr); return 0; } + +if ( gicv3.rdist_regions[i].single_rdist ) +break; + if ( gicv3.rdist_stride ) ptr += gicv3.rdist_stride; else @@ -1282,14 +1286,21 @@ static int gicv3_iomem_deny_access(const struct domain *d) } #ifdef CONFIG_ACPI -static void __init gic_acpi_add_rdist_region(u64 base_addr, u32 size) +static void __init +gic_acpi_add_rdist_region(u64 base_addr, u32 size, bool single_rdist) { unsigned int idx = gicv3.rdist_count++; +gicv3.rdist_regions[idx].single_rdist = single_rdist; gicv3.rdist_regions[idx].base = base_addr; gicv3.rdist_regions[idx].size = size; } +static inline bool gic_dist_supports_dvis(void) +{ +return !!(readl_relaxed(GICD + GICD_TYPER) & GICD_TYPER_DVIS); +} + static int gicv3_make_hwdom_madt(const struct domain *d, u32 offset) { struct acpi_subtable_header *header; @@ -1397,6 +1408,42 @@ gic_acpi_parse_madt_distributor(struct acpi_subtable_header *header, } static int __init +gic_acpi_parse_cpu_redistributor(struct acpi_subtable_header *header, + const unsigned long end) +{ +struct acpi_madt_generic_interrupt *processor; +u32 size; + +processor = (struct acpi_madt_generic_interrupt *)header; +if ( BAD_MADT_ENTRY(processor, end) ) +return -EINVAL; + +if ( !processor->gicr_base_address ) +return -EINVAL; You already check it in gic_acpi_get_madt_cpu_num, so there is no reason to do it again. + +if ( processor->flags & ACPI_MADT_ENABLED ) +{ +size = gic_dist_supports_dvis() ? 4 * SZ_64K : 2 * SZ_64K; +gic_acpi_add_rdist_region(processor->gicr_base_address, size, true); +} I would revert the condition to avoid one level of indentation. I.e if ( !(processor->flags & ACPI_MADT_ENABLED) ) return 0; size = gic_acpi_add... return 0; However, it looks like that the other function that parses GICC within gic-v3.c (see gic_acpi_parse_madt_cpu) does not check if the CPU is usable. I think we need to have the same parsing behavior on every function. + +return 0; +} + +static int __init +gic_acpi_get_madt_cpu_num(struct acpi_subtable_header *header, +const unsigned long end) +{ +struct acpi_madt_generic_interrupt *cpuif; + +cpuif = (struct acpi_madt_generic_interrupt *)header; +if ( BAD_MADT_ENTRY(cpuif, end) || !cpuif->gicr_base_address ) +return -EINVAL; + +return 0; +} + +static int __init gic_acpi_parse_madt_redistributor(struct acpi_subtable_header *header, const unsigned long end) { @@ -1409,7 +1456,7 @@ gic_acpi_parse_madt_redistributor(struct acpi_subtable_header *header, if ( !rdist->base_address || !rdist->length ) return -EINVAL; -gic_acpi_add_rdist_region(rdist->base_address, rdist->length); +gic_acpi_add_rdist_region(rdist->base_address, rdist->length, false); return 0; } @@ -1428,6 +1475,7 @@ static void __init gicv3_acpi_init(void) { struct acpi_table_header *table; struct rdist_region *rdist_regs; +bool gicr_table = true; acpi_status status; int count; @@ -1457,8 +1505,18 @@ static void __init gicv3_acpi_init(void) count = acpi_parse_entries(ACPI_SIG_MADT, sizeof(struct acpi_table_madt), gic_acpi_get_madt_redistributor_num, table,
Re: [Xen-devel] [PATCH v2 11/17] libxl/arm: Construct ACPI DSDT table
On 06/27/2016 06:29 AM, Julien Grall wrote: (CC Boris and Doug) Hi Shannon, On 27/06/16 07:01, Shannon Zhao wrote: On 2016/6/24 1:03, Julien Grall wrote: On 23/06/16 04:16, Shannon Zhao wrote: [...] diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile index 264b6ef..5347480 100644 --- a/tools/libxl/Makefile +++ b/tools/libxl/Makefile @@ -77,7 +77,29 @@ endif LIBXL_OBJS-$(CONFIG_X86) += libxl_cpuid.o libxl_x86.o libxl_psr.o LIBXL_OBJS-$(CONFIG_ARM) += libxl_nocpuid.o libxl_arm.o libxl_libfdt_compat.o -LIBXL_OBJS-$(CONFIG_ARM) += libxl_arm_acpi.o +LIBXL_OBJS-$(CONFIG_ARM) += libxl_arm_acpi.o libxl_dsdt_anycpu_arm.o + +vpath iasl $(PATH) +libxl_mk_dsdt_arm: libxl_mk_dsdt_arm.c +$(CC) $(CFLAGS) -o $@ libxl_mk_dsdt_arm.c + +libxl_dsdt_anycpu_arm.asl: libxl_empty_dsdt_arm.asl libxl_mk_dsdt_arm +awk 'NR > 1 {print s} {s=$$0}' $< > $@ +./libxl_mk_dsdt_arm >> $@ + +libxl_dsdt_anycpu_arm.c: %.c: iasl %.asl +iasl -vs -p $* -tc $*.asl +sed -e 's/AmlCode/$*/g' $*.hex >$@ +echo "int $*_len=sizeof($*);" >>$@ +rm -f $*.aml $*.hex + I don't like the idea to add iasl as a dependency for all ARM platforms. For instance ARMv7 platform will not use ACPI, but we still ask users to install iasl. So I think we should allow the user to opt-in/opt-out for ACPI. Any opinions? I agree. But how to exclude for ARMv7. I notice it only has the option CONFIG_ARM which doesn't distinguish ARM32 and ARM64. I am not sure if we plan to introduce Kconfig for tools. If not, you can add an option to the configure to enable/disable ACPI for guest. This would be gated by the presence of "iasl". [...] diff --git a/tools/libxl/libxl_mk_dsdt_arm.c b/tools/libxl/libxl_mk_dsdt_arm.c new file mode 100644 index 000..96fadbd --- /dev/null +++ b/tools/libxl/libxl_mk_dsdt_arm.c Can we share the code from tools/firmware/acpi/mk_dsdt.c? Yeah, we can share push_block(), pop_block() stmt() and indent() but the main() function is totally different since there are only the processor device objects for ARM DSDT but there are many other things in x86. I think that since Boris will move the codes under tools/firmware/hvmloader/acpi to other place, after that we could see how to share codes then. I would prefer if we discuss about it now in order to avoid code duplication (I have CCed Boris). For instance we can create a new directory under tools for mk_dsdt.c. The main could be different, although it might be possible to gate ARM options, and the rest of the code would be shared. So I think we decided earlier to keep ARM and x86 ACPI builders separate, at least for now. However, looking at the Makefile and mk_dsdt I wonder whether it would make sense to put the builders in the same directory (I am currently using tools/libacpi) so that those two files can be kept common as much as possible, with the sources being different. E.g. something like tools/libacpi: Makefile mk_dsdt.c acpi_x86.[ch] acpi_arm.[ch] *asl etc. The objects will be built in tools/libxl (there will be no libacpi.so) but the infrastructure and sources will live together. -boris ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 16/17] libxc/xc_dom_arm: Copy ACPI tables to guest space
On 06/27/2016 06:49 AM, Julien Grall wrote: (CC Boris) On 27/06/16 07:25, Shannon Zhao wrote: On 2016/6/24 2:46, Julien Grall wrote: + static int alloc_magic_pages(struct xc_dom_image *dom) { > int rc, i; @@ -100,6 +141,16 @@ static int alloc_magic_pages(struct xc_dom_image *dom) xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_STORE_EVTCHN, dom->xenstore_evtchn); +if ( dom->acpitable_blob && dom->acpitable_size > 0 ) +{ +rc = xc_dom_copy_acpi(dom); +if ( rc != 0 ) +{ +DOMPRINTF("Unable to copy ACPI tables"); +return rc; +} +} alloc_magic_pages looks the wrong place with this function. Any reason to not have a generic ACPI blob loading in xc_dom_core.c as we do for devicetree? Looks like xc_dom_build_image is used for allocating pages in guest RAM while ACPI blob is not put in guest RAM. The function xc_dom_build_image is used to load blob into the guest memory and allocate others pages (such as magic pages) which may not be part of the actual guest RAM. The callback alloc_magic_pages is used to allocate per-architecture specific pages. However, the ACPI blob is pretty much generic. So I am not sure why we would want to do it for ARM only. FWIW, for PVH I don't plan, at least for now, to keep a pointer to ACPI stuff in xc_dom_image. I am building the tables and loading them into the guest right away. (As a separate point, I noticed that this series adds 4th type of blob (in addition to kernel, ramdisk and devicetree) so I wonder whether introducing a struct blob might be useful.) -boris ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [qemu-upstream-4.3-testing test] 96301: regressions - FAIL
flight 96301 qemu-upstream-4.3-testing real [real] http://logs.test-lab.xenproject.org/osstest/logs/96301/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-amd64-libvirt 5 libvirt-build fail REGR. vs. 80927 build-i386-libvirt5 libvirt-build fail REGR. vs. 80927 Regressions which are regarded as allowable (not blocking): test-amd64-i386-xl-qemuu-win7-amd64 16 guest-stop fail like 80927 test-amd64-amd64-xl-qemuu-win7-amd64 16 guest-stop fail like 80927 Tests which did not succeed, but are not blocking: test-amd64-i386-libvirt 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-vhd 1 build-check(1) blocked n/a test-amd64-amd64-libvirt 1 build-check(1) blocked n/a test-amd64-amd64-xl-qemuu-ovmf-amd64 9 debian-hvm-install fail never pass test-amd64-i386-xl-qemuu-ovmf-amd64 9 debian-hvm-install fail never pass version targeted for testing: qemuu12e8fccf5b5460be7aecddc71d27eceaba6e1f15 baseline version: qemuu10c1b763c26feb645627a1639e722515f3e1e876 Last test of basis80927 2016-02-06 13:30:02 Z 141 days Failing since 93977 2016-05-10 11:09:16 Z 48 days 151 attempts Testing same since95534 2016-06-11 00:59:46 Z 16 days 31 attempts People who touched revisions under test: Anthony PERARD Gerd Hoffmann Ian Jackson Stefano Stabellini Wei Liu jobs: build-amd64 pass build-i386 pass build-amd64-libvirt fail build-i386-libvirt fail build-amd64-pvopspass build-i386-pvops pass test-amd64-amd64-xl pass test-amd64-i386-xl pass test-amd64-i386-qemuu-rhel6hvm-amd pass test-amd64-amd64-xl-qemuu-debianhvm-amd64pass test-amd64-i386-xl-qemuu-debianhvm-amd64 pass test-amd64-i386-freebsd10-amd64 pass test-amd64-amd64-xl-qemuu-ovmf-amd64 fail test-amd64-i386-xl-qemuu-ovmf-amd64 fail test-amd64-amd64-xl-qemuu-win7-amd64 fail test-amd64-i386-xl-qemuu-win7-amd64 fail test-amd64-amd64-xl-credit2 pass test-amd64-i386-freebsd10-i386 pass test-amd64-i386-qemuu-rhel6hvm-intel pass test-amd64-amd64-libvirt blocked test-amd64-i386-libvirt blocked test-amd64-amd64-xl-multivcpupass test-amd64-amd64-pairpass test-amd64-i386-pair pass test-amd64-amd64-pv pass test-amd64-i386-pv pass test-amd64-amd64-amd64-pvgrubpass test-amd64-amd64-i386-pvgrub pass test-amd64-amd64-pygrub pass test-amd64-amd64-xl-qcow2pass test-amd64-i386-xl-raw pass test-amd64-i386-xl-qemuu-winxpsp3-vcpus1 pass test-amd64-amd64-libvirt-vhd blocked test-amd64-amd64-xl-qemuu-winxpsp3 pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Not pushing. commit 12e8fccf5b5460be7aecddc71d27eceaba6e1f15 Author: Ian Jackson Date: Thu May 26 16:21:56 2016 +0100 main loop: Big hammer to fix logfile disk DoS in Xen setups Each time round the main loop, we now fstat stderr. If it is too big, we dup2 /dev/null onto it. This is not a very pretty patch but it is
Re: [Xen-devel] [PATCH V2 05/10] xen/arm: vgic: Use dynamic memory allocation for vgic_rdist_region
Hi Shanker, On 26/06/16 18:48, Shanker Donthineni wrote: The number of Re-distributor regions allowed for dom0 is hardcoded s/Re-distributor/Redistributor/ to a compile time macro MAX_RDIST_COUNT which is 4. On some systems, s/a compile time macro/a define/ s/On some/Some/ especially latest server chips might have more than 4 redistributors. I would add a comma after 'chips'. NIT: s/redistributors/Redistributors/ Either we have to increase MAX_RDIST_COUNT to a bigger number or allocate memory based on number of redistributors that are found in s/based on number/based on the number/ MADT table. In the worst case scenario, the macro MAX_RDIST_COUNT should be equal to CONFIG_NR_CPUS in order to support per CPU Redistributors. Increasing MAX_RDIST_COUNT has side effect, it blows 'struct domain' s/has side/has a/ size and hits BUILD_BUG_ON() in domain build code path. struct domain *alloc_domain_struct(void) { struct domain *d; BUILD_BUG_ON(sizeof(*d) > PAGE_SIZE); d = alloc_xenheap_pages(0, 0); if ( d == NULL ) return NULL; ... This patch uses the second approach to fix the BUILD_BUG(). Signed-off-by: Shanker Donthineni --- Changes since v1: Keep 'struct vgic_rdist_region' definition inside 'struct arch_domain'. xen/arch/arm/vgic-v2.c | 6 ++ xen/arch/arm/vgic-v3.c | 22 +++--- xen/arch/arm/vgic.c | 1 + xen/include/asm-arm/domain.h | 2 +- xen/include/asm-arm/vgic.h | 2 ++ 5 files changed, 29 insertions(+), 4 deletions(-) diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c index 9adb4a9..f5778e6 100644 --- a/xen/arch/arm/vgic-v2.c +++ b/xen/arch/arm/vgic-v2.c @@ -699,9 +699,15 @@ static int vgic_v2_domain_init(struct domain *d) return 0; } +static void vgic_v2_domain_free(struct domain *d) +{ +/* Nothing to be cleanup for this driver */ +} + static const struct vgic_ops vgic_v2_ops = { .vcpu_init = vgic_v2_vcpu_init, .domain_init = vgic_v2_domain_init, +.domain_free = vgic_v2_domain_free, .max_vcpus = 8, }; diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c index b37a7c0..e877e9e 100644 --- a/xen/arch/arm/vgic-v3.c +++ b/xen/arch/arm/vgic-v3.c @@ -1393,7 +1393,19 @@ static int vgic_v3_vcpu_init(struct vcpu *v) static int vgic_v3_domain_init(struct domain *d) { -int i; +struct vgic_rdist_region *rdist_regions; +int rdist_count, i; + +/* Allocate memory for Re-distributor regions */ +rdist_count = is_hardware_domain(d) ? vgic_v3_hw.nr_rdist_regions : + GUEST_GICV3_RDIST_REGIONS; I would directly introduce the inline helper in this patch, rather than in patch #10. + +rdist_regions = xzalloc_array(struct vgic_rdist_region, rdist_count); +if ( !rdist_regions ) +return -ENOMEM; + +d->arch.vgic.nr_regions = rdist_count; +d->arch.vgic.rdist_regions = rdist_regions; /* * Domain 0 gets the hardware address. [...] diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h index a2fccc0..fbb763a 100644 --- a/xen/include/asm-arm/vgic.h +++ b/xen/include/asm-arm/vgic.h @@ -128,6 +128,8 @@ struct vgic_ops { int (*vcpu_init)(struct vcpu *v); /* Domain specific initialization of vGIC */ int (*domain_init)(struct domain *d); +/* Release resources that are allocated by domain_init */ s/are/were/ +void (*domain_free)(struct domain *d); /* vGIC sysreg emulation */ int (*emulate_sysreg)(struct cpu_user_regs *regs, union hsr hsr); /* Maximum number of vCPU supported */ Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] making xenstore domain easy configurable
I'm just writing some patches to make it easy to switch between xenstore daemon and xenstore domain. My plan is to achieve this by a global configuration file containing configuration options for the host (e.g. /etc/xen/xen.conf). With the current systemd support this is not easy. There are systemd socket definitions to let systemd create the sockets for xenstored. As the sockets are not to be created in case xenstore is running in a xenstore domain things are becoming complicated. Today we have the following xenstore related systemd items: - xenstored_ro.socket and xenstored.socket - xenstored.service depending on the sockets - other services depending on xenstored.service A xenstore domain would need: - xenstore-domain.service - other services depending on xenstore-domain.service Being able to switch between both schemes just via a config file seems to be not easy, at least I don't know of any way to do the socket creation only in case they are required without breaking the dependency chain. So I'd suggest to remove xenstored_ro.socket and xenstored.socket and let xenstored create the sockets (as it is doing without systemd). I'm not aware of any disadvantage, as xenstored isn't restartable and thus can't take advantage of the permanent sockets offered by systemd. This would mean I could rip out the systemd specific stuff from xenstored and oxenstored. I could create a single xenstore.service script evaluating the config file and starting the correct xenstore (xenstored or xenstore domain). The other services would then depend on xenstore.service. This would remove the need to specify the type of xenstore daemon/domain (ocaml based or C based) in the systemd file, too. Is there a better way to achieve what I want? Any other opinions? Juergen ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH V2 05/10] xen/arm: vgic: Use dynamic memory allocation for vgic_rdist_region
>> to a compile time macro MAX_RDIST_COUNT which is 4. On some systems, > > s/a compile time macro/a define/ > s/On some/Some/ > >> especially latest server chips might have more than 4 redistributors. > > I would add a comma after 'chips'. In English, rules for dual commas forming a subclause like this are that the sentence still makes grammatical sense if the subclause is omitted. Therefore "On some systems, especially latest server chips, might have more than 4 redistributors." is expected be equivalent to "On some systems might have more than 4 redistributors." The latter however doesn't make sense. May I recommend instead: "Some systems, especially latest server chips, may have more than 4 redistributors." ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH V2 07/10] arm: vgic: Split vgic_domain_init() functionality into two functions
Hi Shanker, On 26/06/16 18:48, Shanker Donthineni wrote: Separate the code logic that does the registration of vgic_v3/v2 ops to a new fucntion domain_vgic_register(). The intention of this s/fucntion/function/ separation is to record the required mmio count in vgic_v3/v2_init() and pass it to function domain_io_init() in the later patch. s/the later/a follow-up patch/ or s/the later/the last/. Although I prefer the former. Signed-off-by: Shanker Donthineni --- Changes since v1: Moved registration of vgic_v3/v2 functionality to a new domain_vgic_register(). xen/arch/arm/vgic.c | 33 + 1 file changed, 21 insertions(+), 12 deletions(-) diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c index 5df5f01..7627eff 100644 --- a/xen/arch/arm/vgic.c +++ b/xen/arch/arm/vgic.c @@ -88,19 +88,8 @@ static void vgic_rank_init(struct vgic_irq_rank *rank, uint8_t index, rank->vcpu[i] = vcpu; } -int domain_vgic_init(struct domain *d, unsigned int nr_spis) +static int domain_vgic_register(struct domain *d) { -int i; -int ret; - -d->arch.vgic.ctlr = 0; - -/* Limit the number of virtual SPIs supported to (1020 - 32) = 988 */ -if ( nr_spis > (1020 - NR_LOCAL_IRQS) ) -return -EINVAL; - -d->arch.vgic.nr_spis = nr_spis; - switch ( d->arch.vgic.version ) { #ifdef CONFIG_HAS_GICV3 @@ -119,6 +108,26 @@ int domain_vgic_init(struct domain *d, unsigned int nr_spis) return -ENODEV; } +return 0; +} + +int domain_vgic_init(struct domain *d, unsigned int nr_spis) +{ +int i; +int ret; + +d->arch.vgic.ctlr = 0; + +/* Limit the number of virtual SPIs supported to (1020 - 32) = 988 */ +if ( nr_spis > (1020 - NR_LOCAL_IRQS) ) +return -EINVAL; + +d->arch.vgic.nr_spis = nr_spis; + +ret = domain_vgic_register(d); +if ( ret < 0) Coding style: if ( ret < 0 ) +return ret; + spin_lock_init(&d->arch.vgic.lock); d->arch.vgic.shared_irqs = Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v12 6/6] vt-d: fix vt-d Device-TLB flush timeout issue
On June 27, 2016 4:24 PM, Jan Beulich wrote: > >>> On 24.06.16 at 07:51, wrote: > > @@ -199,24 +199,73 @@ static int __must_check > queue_invalidate_wait(struct iommu *iommu, > > return -EOPNOTSUPP; > > } > > > > -static int __must_check invalidate_sync(struct iommu *iommu, > > -bool_t flush_dev_iotlb) > > +static int __must_check invalidate_sync(struct iommu *iommu) > > { > > struct qi_ctrl *qi_ctrl = iommu_qi_ctrl(iommu); > > > > ASSERT(qi_ctrl->qinval_maddr); > > > > -return queue_invalidate_wait(iommu, 0, 1, 1, flush_dev_iotlb); > > +return queue_invalidate_wait(iommu, 0, 1, 1, 0); } > > + > > +static void dev_invalidate_iotlb_timeout(struct iommu *iommu, u16 did, > > + struct pci_dev *pdev) { > > +struct domain *d = NULL; > > + > > +if ( test_bit(did, iommu->domid_bitmap) ) > > +d = rcu_lock_domain_by_id(iommu->domid_map[did]); > > + > > +/* > > + * In case the domain has been freed or the IOMMU domid bitmap is > > + * not valid, the device no longer belongs to this domain. > > + */ > > +if ( d == NULL ) > > +return; > > + > > +pcidevs_lock(); > > +ASSERT(pdev->domain); > > +list_del(&pdev->domain_list); > > +pdev->domain = NULL; > > +pci_hide_existing_device(pdev); > > +pcidevs_unlock(); > > + > > +if ( !d->is_shutting_down && printk_ratelimit() ) > > +printk(XENLOG_WARNING VTDPREFIX > > + " dom%d: ATS device %04x:%02x:%02x.%u flush failed\n", > > + d->domain_id, pdev->seg, pdev->bus, > > + PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn)); > > + > > +if ( !is_hardware_domain(d) ) > > +domain_crash(d); > > + > > +rcu_unlock_domain(d); > > +} > > So in an earlier patch in this series you (supposedly) moved similar logic up > to > the vendor independent layer. I think this then would better get moved up > too, if at all possible. > To be honest, I have not much reason for leaving domain crash here and I was aware of this problem, but crash_domain() here is not harmful (as the 'd->is_shutting_down' is Set when to crash, and once the 'd->is_shutting_down' is Set then return in domain_shutdown() ). In case crash domain directly, it may help us narrow down the 'window' (the domain is still running).. To me, moving the logic up is acceptable. In next version, could I only drop: +if ( !is_hardware_domain(d) ) +domain_crash(d); In this patch, and leave the rest as is ? Quan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] making xenstore domain easy configurable
On 27/06/16 13:43, Juergen Gross wrote: > I'm just writing some patches to make it easy to switch between > xenstore daemon and xenstore domain. My plan is to achieve this > by a global configuration file containing configuration options > for the host (e.g. /etc/xen/xen.conf). > > With the current systemd support this is not easy. There are > systemd socket definitions to let systemd create the sockets for > xenstored. As the sockets are not to be created in case xenstore > is running in a xenstore domain things are becoming complicated. > > Today we have the following xenstore related systemd items: > > - xenstored_ro.socket and xenstored.socket > - xenstored.service depending on the sockets > - other services depending on xenstored.service > > A xenstore domain would need: > > - xenstore-domain.service > - other services depending on xenstore-domain.service > > Being able to switch between both schemes just via a config file > seems to be not easy, at least I don't know of any way to do the > socket creation only in case they are required without breaking > the dependency chain. > > So I'd suggest to remove xenstored_ro.socket and xenstored.socket > and let xenstored create the sockets (as it is doing without > systemd). I'm not aware of any disadvantage, as xenstored isn't > restartable and thus can't take advantage of the permanent sockets > offered by systemd. > > This would mean I could rip out the systemd specific stuff from > xenstored and oxenstored. I could create a single xenstore.service > script evaluating the config file and starting the correct xenstore > (xenstored or xenstore domain). The other services would then depend > on xenstore.service. This would remove the need to specify the > type of xenstore daemon/domain (ocaml based or C based) in the systemd > file, too. > > Is there a better way to achieve what I want? Any other opinions? This isn't the only advantage offered by socket activation. As currently configured, every service which depends on xenstored.socket can be started in parallel (as systemd creates the sockets ahead of time), with the dependent services blocking a little on the socket while xenstored starts up sufficiently to service the requests. In the case that xenstored is running in the local domain, socket activation is a useful function to have. OTOH, having anything explicitly depend on xenstored.socket is broken in a model where xenstored might be running in a separate domain. I don't suppose systemd has any way of specifying "conditionally might have a socket"? ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] making xenstore domain easy configurable
On 27/06/16 14:59, Andrew Cooper wrote: > On 27/06/16 13:43, Juergen Gross wrote: >> I'm just writing some patches to make it easy to switch between >> xenstore daemon and xenstore domain. My plan is to achieve this >> by a global configuration file containing configuration options >> for the host (e.g. /etc/xen/xen.conf). >> >> With the current systemd support this is not easy. There are >> systemd socket definitions to let systemd create the sockets for >> xenstored. As the sockets are not to be created in case xenstore >> is running in a xenstore domain things are becoming complicated. >> >> Today we have the following xenstore related systemd items: >> >> - xenstored_ro.socket and xenstored.socket >> - xenstored.service depending on the sockets >> - other services depending on xenstored.service >> >> A xenstore domain would need: >> >> - xenstore-domain.service >> - other services depending on xenstore-domain.service >> >> Being able to switch between both schemes just via a config file >> seems to be not easy, at least I don't know of any way to do the >> socket creation only in case they are required without breaking >> the dependency chain. >> >> So I'd suggest to remove xenstored_ro.socket and xenstored.socket >> and let xenstored create the sockets (as it is doing without >> systemd). I'm not aware of any disadvantage, as xenstored isn't >> restartable and thus can't take advantage of the permanent sockets >> offered by systemd. >> >> This would mean I could rip out the systemd specific stuff from >> xenstored and oxenstored. I could create a single xenstore.service >> script evaluating the config file and starting the correct xenstore >> (xenstored or xenstore domain). The other services would then depend >> on xenstore.service. This would remove the need to specify the >> type of xenstore daemon/domain (ocaml based or C based) in the systemd >> file, too. >> >> Is there a better way to achieve what I want? Any other opinions? > > This isn't the only advantage offered by socket activation. > > As currently configured, every service which depends on xenstored.socket > can be started in parallel (as systemd creates the sockets ahead of > time), with the dependent services blocking a little on the socket while > xenstored starts up sufficiently to service the requests. Okay, but this isn't true for the xenstore domain case, resulting in the need for an explicit dependency then. I don't think doing the same for xenstored is adding too much time to system startup. > In the case that xenstored is running in the local domain, socket > activation is a useful function to have. > > OTOH, having anything explicitly depend on xenstored.socket is broken in > a model where xenstored might be running in a separate domain. I don't > suppose systemd has any way of specifying "conditionally might have a > socket"? I don't know of any way to do this. Juergen ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] xen-blkfront: save uncompleted reqs in blkfront_resume()
On 06/27/2016 04:33 PM, Bob Liu wrote: > Uncompleted reqs used to be 'saved and resubmitted' in blkfront_recover() > during > migration, but that's too later after multi-queue introduced. > > After a migrate to another host (which may not have multiqueue support), the > number of rings (block hardware queues) may be changed and the ring and shadow > structure will also be reallocated. > So that blkfront_recover() can't 'save and resubmit' the real uncompleted reqs > because shadow structure has been reallocated. > > This patch fixes this issue by moving the 'save and resubmit' logic out of Fix: Just moved the 'save' logic to earlier place:blkfront_resume(), the 'resubmit' was no change and still in blkfront_recover(). > blkfront_recover() to earlier place:blkfront_resume(). > > Signed-off-by: Bob Liu > --- > drivers/block/xen-blkfront.c | 91 > +++- > 1 file changed, 40 insertions(+), 51 deletions(-) > > diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c > index 2e6d1e9..fcc5b4e 100644 > --- a/drivers/block/xen-blkfront.c > +++ b/drivers/block/xen-blkfront.c > @@ -207,6 +207,9 @@ struct blkfront_info > struct blk_mq_tag_set tag_set; > struct blkfront_ring_info *rinfo; > unsigned int nr_rings; > + /* Save uncomplete reqs and bios for migration. */ > + struct list_head requests; > + struct bio_list bio_list; > }; > > static unsigned int nr_minors; > @@ -2002,69 +2005,22 @@ static int blkif_recover(struct blkfront_info *info) > { > unsigned int i, r_index; > struct request *req, *n; > - struct blk_shadow *copy; > int rc; > struct bio *bio, *cloned_bio; > - struct bio_list bio_list, merge_bio; > unsigned int segs, offset; > int pending, size; > struct split_bio *split_bio; > - struct list_head requests; > > blkfront_gather_backend_features(info); > segs = info->max_indirect_segments ? : BLKIF_MAX_SEGMENTS_PER_REQUEST; > blk_queue_max_segments(info->rq, segs); > - bio_list_init(&bio_list); > - INIT_LIST_HEAD(&requests); > > for (r_index = 0; r_index < info->nr_rings; r_index++) { > - struct blkfront_ring_info *rinfo; > - > - rinfo = &info->rinfo[r_index]; > - /* Stage 1: Make a safe copy of the shadow state. */ > - copy = kmemdup(rinfo->shadow, sizeof(rinfo->shadow), > -GFP_NOIO | __GFP_REPEAT | __GFP_HIGH); > - if (!copy) > - return -ENOMEM; > - > - /* Stage 2: Set up free list. */ > - memset(&rinfo->shadow, 0, sizeof(rinfo->shadow)); > - for (i = 0; i < BLK_RING_SIZE(info); i++) > - rinfo->shadow[i].req.u.rw.id = i+1; > - rinfo->shadow_free = rinfo->ring.req_prod_pvt; > - rinfo->shadow[BLK_RING_SIZE(info)-1].req.u.rw.id = 0x0fff; > + struct blkfront_ring_info *rinfo = &info->rinfo[r_index]; > > rc = blkfront_setup_indirect(rinfo); > - if (rc) { > - kfree(copy); > + if (rc) > return rc; > - } > - > - for (i = 0; i < BLK_RING_SIZE(info); i++) { > - /* Not in use? */ > - if (!copy[i].request) > - continue; > - > - /* > - * Get the bios in the request so we can re-queue them. > - */ > - if (copy[i].request->cmd_flags & > - (REQ_FLUSH | REQ_FUA | REQ_DISCARD | REQ_SECURE)) { > - /* > - * Flush operations don't contain bios, so > - * we need to requeue the whole request > - */ > - list_add(©[i].request->queuelist, > &requests); > - continue; > - } > - merge_bio.head = copy[i].request->bio; > - merge_bio.tail = copy[i].request->biotail; > - bio_list_merge(&bio_list, &merge_bio); > - copy[i].request->bio = NULL; > - blk_end_request_all(copy[i].request, 0); > - } > - > - kfree(copy); > } > xenbus_switch_state(info->xbdev, XenbusStateConnected); > > @@ -2079,7 +2035,7 @@ static int blkif_recover(struct blkfront_info *info) > kick_pending_request_queues(rinfo); > } > > - list_for_each_entry_safe(req, n, &requests, queuelist) { > + list_for_each_entry_safe(req, n, &info->requests, queuelist) { > /* Requeue pending requests (flush or discard) */ > list_del_init(&req->queuelist); > BUG_ON(req->nr_phys_segments > segs); > @@ -2087,7 +2043,7 @@ static int blkif
Re: [Xen-devel] xc_domain_maximum_gpfn
On Sun, Jun 26, 2016 at 5:19 PM, sepanta s wrote: > Hi, > what exactly does this module do? > sorry, not module but the function. > I have got about 1 million after calling this function for a domain with 1 > Gigabyte ram and page size 4K. If the output of the function is the whole > gfns in a domain, shouldn't it be equal with (ram-size)/(page-size) ? > Is there any API to get the memory range of the domain to search through > its pages ? > ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH V2 09/10] xen/arm: io: Use binary search for mmio handler lookup
Hi Shanker, On 26/06/16 18:48, Shanker Donthineni wrote: As the number of I/O handlers increase, the overhead associated with linear lookup also increases. The system might have maximum of 144 (assuming CONFIG_NR_CPUS=128) mmio handlers. In worst case scenario, it would require 144 iterations for finding a matching handler. Now it is time for us to change from linear (complexity O(n)) to a binary search (complexity O(log n) for reducing mmio handler lookup overhead. Signed-off-by: Shanker Donthineni --- xen/arch/arm/io.c | 50 +++--- 1 file changed, 39 insertions(+), 11 deletions(-) diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c index a5b2c2d..abf49fb 100644 --- a/xen/arch/arm/io.c +++ b/xen/arch/arm/io.c @@ -20,6 +20,7 @@ #include #include #include +#include #include #include @@ -70,23 +71,38 @@ static int handle_write(const struct mmio_handler *handler, struct vcpu *v, handler->priv); } -int handle_mmio(mmio_info_t *info) +const struct mmio_handler *find_mmio_handler(struct vcpu *v, paddr_t addr) { -struct vcpu *v = current; -int i; -const struct mmio_handler *handler = NULL; const struct vmmio *vmmio = &v->domain->arch.vmmio; +const struct mmio_handler *handler = vmmio->handlers; +unsigned int eidx = vmmio->num_entries; +unsigned int midx = eidx / 2; +unsigned int sidx = 0; -for ( i = 0; i < vmmio->num_entries; i++ ) +/* Do binary search for matching mmio handler */ +while ( sidx != midx ) { -handler = &vmmio->handlers[i]; - -if ( (info->gpa >= handler->addr) && - (info->gpa < (handler->addr + handler->size)) ) -break; +if ( addr < handler[midx].addr ) +eidx = midx; +else +sidx = midx; +midx = sidx + (eidx - sidx) / 2; This binary search can be simplified. For instance, why do you want to compute midx at the end rather than at the beginning. This would avoid to have "unsigned int midx = eidx / 2" at the beginning. } -if ( i == vmmio->num_entries ) +if ( (addr >= handler[sidx].addr) && + (addr < (handler[sidx].addr + handler[sidx].size)) ) +return handler + sidx; Please use a temporary variable for handler[sidx]. So it will be easier to read the code. + +return NULL; +} + +int handle_mmio(mmio_info_t *info) +{ +const struct mmio_handler *handler; +struct vcpu *v = current; + +handler = find_mmio_handler(v, info->gpa); I would have expected some locking here. Could you explain why it is safe to looking find the handler with your solution? For what is worth, there was no locking before because register_mmio_handler was always adding the handler at the end of the array. This is not true anymore because you are sorting the array. +if ( !handler ) return 0; if ( info->dabt.write ) @@ -95,6 +111,14 @@ int handle_mmio(mmio_info_t *info) return handle_read(handler, v, info); } +static int cmp_mmio_handler(const void *key, const void *elem) +{ +const struct mmio_handler *handler0 = key; +const struct mmio_handler *handler1 = elem; + +return (handler0->addr < handler1->addr) ? -1 : 0; +} + void register_mmio_handler(struct domain *d, const struct mmio_handler_ops *ops, paddr_t addr, paddr_t size, void *priv) @@ -122,6 +146,10 @@ void register_mmio_handler(struct domain *d, vmmio->num_entries++; +/* Sort mmio handlers in ascending order based on base address */ +sort(vmmio->handlers, vmmio->num_entries, sizeof(struct mmio_handler), + cmp_mmio_handler, NULL); + spin_unlock(&vmmio->lock); } Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH V2 10/10] arm/vgic: Change fixed number of mmio handlers to variable number
Hi Shanker, On 26/06/16 18:48, Shanker Donthineni wrote: diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h index 29346c6..b205461 100644 --- a/xen/include/asm-arm/domain.h +++ b/xen/include/asm-arm/domain.h @@ -111,6 +111,7 @@ struct arch_domain int nr_regions; /* Number of rdist regions */ uint32_t rdist_stride; /* Re-Distributor stride */ #endif +uint32_t mmio_count;/* Number of mmio handlers */ Is it necessary to have this value part of the arch_domain? I.e Do we need this value after the initialization? If not, then it might be better to add a parameter to domain_vgic_register uint32_t *pointer. } vgic; struct vuart { diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h index fbb763a..1ce441c 100644 --- a/xen/include/asm-arm/vgic.h +++ b/xen/include/asm-arm/vgic.h @@ -307,6 +307,7 @@ extern void register_vgic_ops(struct domain *d, const struct vgic_ops *ops); int vgic_v2_init(struct domain *d); int vgic_v3_init(struct domain *d); +extern int domain_vgic_register(struct domain *d); extern int vcpu_vgic_free(struct vcpu *v); extern int vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, int virq, Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH V2 01/10] arm/gic-v3: Fix bug in function cmp_rdist()
On 06/27/2016 06:03 AM, Julien Grall wrote: Hi Shanker, On 26/06/16 18:48, Shanker Donthineni wrote: The cmp_rdist() is always returning value zero irrespective of the input Redistributor base addresses. Both the local variables 'l' and 'r' are pointing to the first argument 'a' causing the logical expression 'l->base < r->base' always evaluated as false which is wrong. Signed-off-by: Shanker Donthineni --- xen/arch/arm/gic-v3.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c index 8d3f149..b89c608 100644 --- a/xen/arch/arm/gic-v3.c +++ b/xen/arch/arm/gic-v3.c @@ -1133,7 +1133,7 @@ static const hw_irq_controller gicv3_guest_irq_type = { static int __init cmp_rdist(const void *a, const void *b) { -const struct rdist_region *l = a, *r = a; +const struct rdist_region *l = a, *r = b; Thank you for spotting the error. The sorting was required because of the way the vGIC emulated the re-distributors. However, this code has been reworked and sorted array is not necessary anymore. So I would directly drop the sorting here. Thanks, I'll drop this patch in patchset-v3. /* We assume that re-distributor regions can never overlap */ return ( l->base < r->base) ? -1 : 0; Regards, -- Shanker Donthineni Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [xen-unstable-smoke test] 96310: tolerable all pass - PUSHED
flight 96310 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/96310/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 12 migrate-support-checkfail never pass test-armhf-armhf-xl 12 migrate-support-checkfail never pass test-armhf-armhf-xl 13 saverestore-support-checkfail never pass version targeted for testing: xen 08cffe6696c047123bd552e095163924c8ef4353 baseline version: xen 8384dc2d95538c5910d98db3df3ff5448bf0af48 Last test of basis96214 2016-06-24 11:02:52 Z3 days Testing same since96307 2016-06-27 10:03:47 Z0 days2 attempts People who touched revisions under test: Daniel De Graaf Julien Grall jobs: build-amd64 pass build-armhf pass build-amd64-libvirt pass test-armhf-armhf-xl pass test-amd64-amd64-xl-qemuu-debianhvm-i386 pass test-amd64-amd64-libvirt pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Pushing revision : + branch=xen-unstable-smoke + revision=08cffe6696c047123bd552e095163924c8ef4353 + . ./cri-lock-repos ++ . ./cri-common +++ . ./cri-getconfig +++ umask 002 +++ getrepos getconfig Repos perl -e ' use Osstest; readglobalconfig(); print $c{"Repos"} or die $!; ' +++ local repos=/home/osstest/repos +++ '[' -z /home/osstest/repos ']' +++ '[' '!' -d /home/osstest/repos ']' +++ echo /home/osstest/repos ++ repos=/home/osstest/repos ++ repos_lock=/home/osstest/repos/lock ++ '[' x '!=' x/home/osstest/repos/lock ']' ++ OSSTEST_REPOS_LOCK_LOCKED=/home/osstest/repos/lock ++ exec with-lock-ex -w /home/osstest/repos/lock ./ap-push xen-unstable-smoke 08cffe6696c047123bd552e095163924c8ef4353 + branch=xen-unstable-smoke + revision=08cffe6696c047123bd552e095163924c8ef4353 + . ./cri-lock-repos ++ . ./cri-common +++ . ./cri-getconfig +++ umask 002 +++ getrepos getconfig Repos perl -e ' use Osstest; readglobalconfig(); print $c{"Repos"} or die $!; ' +++ local repos=/home/osstest/repos +++ '[' -z /home/osstest/repos ']' +++ '[' '!' -d /home/osstest/repos ']' +++ echo /home/osstest/repos ++ repos=/home/osstest/repos ++ repos_lock=/home/osstest/repos/lock ++ '[' x/home/osstest/repos/lock '!=' x/home/osstest/repos/lock ']' + . ./cri-common ++ . ./cri-getconfig ++ umask 002 + select_xenbranch + case "$branch" in + tree=xen + xenbranch=xen-unstable-smoke + qemuubranch=qemu-upstream-unstable + '[' xxen = xlinux ']' + linuxbranch= + '[' xqemu-upstream-unstable = x ']' + select_prevxenbranch ++ ./cri-getprevxenbranch xen-unstable-smoke + prevxenbranch=xen-4.7-testing + '[' x08cffe6696c047123bd552e095163924c8ef4353 = x ']' + : tested/2.6.39.x + . ./ap-common ++ : osst...@xenbits.xen.org +++ getconfig OsstestUpstream +++ perl -e ' use Osstest; readglobalconfig(); print $c{"OsstestUpstream"} or die $!; ' ++ : ++ : git://xenbits.xen.org/xen.git ++ : osst...@xenbits.xen.org:/home/xen/git/xen.git ++ : git://xenbits.xen.org/qemu-xen-traditional.git ++ : git://git.kernel.org ++ : git://git.kernel.org/pub/scm/linux/kernel/git ++ : git ++ : git://xenbits.xen.org/libvirt.git ++ : osst...@xenbits.xen.org:/home/xen/git/libvirt.git ++ : git://xenbits.xen.org/libvirt.git ++ : git://xenbits.xen.org/rumpuser-xen.git ++ : git ++ : git://xenbits.xen.org/rumpuser-xen.git ++ : osst...@xenbits.xen.org:/home/xen/git/rumpuser-xen.git +++ besteffort_repo https://github.com/rumpkernel/rumpkernel-netbsd-src +++ local repo=https://github.com/rumpkernel/rumpkernel-netbsd-src +++ cached_repo https://github.com/rumpkernel/rumpkernel-netbsd-src '[fetch=try]' +++ local repo=https://github.com/rumpkernel/rumpkernel-netbsd-src +++ local 'options=[fetch=try]' getconfig GitCacheProxy perl -e ' use Osstest; readglobalconfig(); print $c{"GitCacheProxy"} or die $!; ' +++ local cache=git://cache:9419/ +++ '[' xgit://cache:9419/ '!=' x ']' +++ echo 'git://cache:
Re: [Xen-devel] [Xen-users] Binary compatibility report for Xen base libraries
26.06.2016, 17:42, "Doug Goldstein": > On 6/24/16 9:21 AM, Ponomarenko Andrey wrote: >> Hello, >> >> I maintain a new project for backward compatibility analysis of the Linux >> ABIs. The report for Xen base libraries has been recently added to the >> project: http://abi-laboratory.pro/tracker/timeline/xen/ >> >> The report is generated daily with the help of the abi-compliance-checker, >> abi-dumper and abi-tracker tools: https://github.com/lvc/abi-tracker >> >> Hope this will help users, maintainers and developers to maintain backward >> compatibility. >> >> Thank you. > > Outstanding! I had mentioned your tools in the #xen-devel channel. And I > had talked about utilizing them as part of the Travis build process with > the goal of making this data available. Thank you for doing this. > > Do you have any advice for us if it would be positive to do some sort of > check on a per-commit basis or not? Hello, You can use basic tools to perform analysis per each commit: 1. Build shared objects of a library with debug info (with -g -Og additional GCC flags) 2. Extract ABI information with the help of the abi-dumper tool: abi-dumper xen-4.5.3/lib64/libxenguest.so.4.5.0 --vnum=4.5.3 --public-headers=xen-4.5.3/include --output=./ABI-4.5.3.dump abi-dumper xen-4.6.0/lib64/libxenguest.so.4.6.0 --vnum=4.6.0 --public-headers=xen-4.6.0/include --output=./ABI-4.6.0.dump 3. Compare ABI dumps to produce report: abi-compliance-checker -l libxenguest -old ABI-4.5.3.dump -new ABI-4.6.0.dump The abi-tracker and abi-monitor tools can also be used to perform analysis per each commit: 1. Create profile for a library (e.g. https://github.com/lvc/upstream-tracker/blob/master/profile/xen.json) and add the following property to it: "Git": "http://xenbits.xen.org/git-http/xen.git";, 2. Create build script for a library (e.g. https://github.com/lvc/upstream-tracker/blob/master/build_script/xen.sh) 3. Retry this command per each commit to pull source from git repository, download latest stable releases and build them: abi-monitor -get -build-new profile/xen.json 4. Run this command to update ABI report after each run of the abi-monitor: abi-tracker -build profile/xen.json 5. The latest version of the library from git will be analyzed against the latest stable release. Thank you. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH V2 06/10] arm/gic-v3: Remove an unused macro MAX_RDIST_COUNT
On 26/06/16 18:48, Shanker Donthineni wrote: The macro MAX_RDIST_COUNT is not being used after converting code to handle number of redistributor dynamically. So remove it from header file and the two other panic() messages that are not valid anymore. Signed-off-by: Shanker Donthineni Acked-by: Julien Grall --- xen/arch/arm/gic-v3.c | 8 xen/include/asm-arm/gic.h | 1 - 2 files changed, 9 deletions(-) diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c index 3977244..87f4ecf 100644 --- a/xen/arch/arm/gic-v3.c +++ b/xen/arch/arm/gic-v3.c @@ -1200,10 +1200,6 @@ static void __init gicv3_dt_init(void) &gicv3.rdist_count) ) gicv3.rdist_count = 1; -if ( gicv3.rdist_count > MAX_RDIST_COUNT ) -panic("GICv3: Number of redistributor regions is more than" - "%d (Increase MAX_RDIST_COUNT!!)\n", MAX_RDIST_COUNT); - rdist_regs = xzalloc_array(struct rdist_region, gicv3.rdist_count); if ( !rdist_regs ) panic("GICv3: Failed to allocate memory for rdist regions\n"); @@ -1518,10 +1514,6 @@ static void __init gicv3_acpi_init(void) gicr_table = false; } -if ( count > MAX_RDIST_COUNT ) -panic("GICv3: Number of redistributor regions is more than" - "%d (Increase MAX_RDIST_COUNT!!)\n", MAX_RDIST_COUNT); - rdist_regs = xzalloc_array(struct rdist_region, count); if ( !rdist_regs ) panic("GICv3: Failed to allocate memory for rdist regions\n"); diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h index fedf1fa..db7b2d0 100644 --- a/xen/include/asm-arm/gic.h +++ b/xen/include/asm-arm/gic.h @@ -20,7 +20,6 @@ #define NR_GIC_LOCAL_IRQS NR_LOCAL_IRQS #define NR_GIC_SGI 16 -#define MAX_RDIST_COUNT4 #define GICD_CTLR (0x000) #define GICD_TYPER (0x004) -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [xen-unstable test] 96296: tolerable FAIL
flight 96296 xen-unstable real [real] http://logs.test-lab.xenproject.org/osstest/logs/96296/ Failures :-/ but no regressions. Regressions which are regarded as allowable (not blocking): build-amd64-rumpuserxen 6 xen-buildfail like 96278 build-i386-rumpuserxen6 xen-buildfail like 96278 test-amd64-amd64-xl-rtds 9 debian-install fail like 96278 test-amd64-i386-xl-qemut-win7-amd64 16 guest-stop fail like 96278 test-amd64-amd64-xl-qemut-win7-amd64 16 guest-stop fail like 96278 test-amd64-amd64-xl-qemuu-win7-amd64 16 guest-stop fail like 96278 test-amd64-i386-xl-qemuu-win7-amd64 16 guest-stop fail like 96278 Tests which did not succeed, but are not blocking: test-amd64-i386-rumpuserxen-i386 1 build-check(1) blocked n/a test-amd64-amd64-rumpuserxen-amd64 1 build-check(1) blocked n/a test-amd64-amd64-xl-pvh-amd 11 guest-start fail never pass test-amd64-amd64-xl-pvh-intel 11 guest-start fail never pass test-armhf-armhf-libvirt-xsm 12 migrate-support-checkfail never pass test-armhf-armhf-libvirt-xsm 14 guest-saverestorefail never pass test-amd64-amd64-libvirt 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-xsm 12 migrate-support-checkfail never pass test-amd64-amd64-libvirt-vhd 11 migrate-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 11 migrate-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 13 guest-saverestorefail never pass test-amd64-amd64-libvirt-xsm 12 migrate-support-checkfail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check 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-armhf-armhf-libvirt 14 guest-saverestorefail never pass test-armhf-armhf-libvirt 12 migrate-support-checkfail never pass test-amd64-i386-libvirt 12 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 12 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 12 migrate-support-checkfail never pass test-armhf-armhf-xl 12 migrate-support-checkfail never pass test-armhf-armhf-xl 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 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-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-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-rtds 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-rtds 12 migrate-support-checkfail never pass version targeted for testing: xen 8384dc2d95538c5910d98db3df3ff5448bf0af48 baseline version: xen 8384dc2d95538c5910d98db3df3ff5448bf0af48 Last test of basis96296 2016-06-27 01:55:48 Z0 days Testing same since0 1970-01-01 00:00:00 Z 16979 days0 attempts 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-pvops
Re: [Xen-devel] [PATCH V2 03/10] arm/gic-v3: Fold GICR subtable parsing into a new function
On 06/27/2016 06:26 AM, Julien Grall wrote: Hi Shanker, Title: I think you want to say "Move GICR..." rather than "Fold GICR...". On 26/06/16 18:48, Shanker Donthineni wrote: Add a new function for parsing GICR subtable and move the code Add a new function to parse GICR... that is specific to GICR table to new function without changing to a new function the function gicv3_acpi_init() behavior. Signed-off-by: Shanker Donthineni --- Changes since v1: Removed the unnecessary GICR ioremap operation inside GICR table parse code. xen/arch/arm/gic-v3.c | 61 --- 1 file changed, 39 insertions(+), 22 deletions(-) diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c index 542c4f3..0471fea 100644 --- a/xen/arch/arm/gic-v3.c +++ b/xen/arch/arm/gic-v3.c @@ -1282,6 +1282,14 @@ static int gicv3_iomem_deny_access(const struct domain *d) } #ifdef CONFIG_ACPI +static void __init gic_acpi_add_rdist_region(u64 base_addr, u32 size) Please use paddr_t for both parameter. Also the suffix _addr is pointless. I'll fix. +{ +unsigned int idx = gicv3.rdist_count++; + +gicv3.rdist_regions[idx].base = base_addr; +gicv3.rdist_regions[idx].size = size; +} + static int gicv3_make_hwdom_madt(const struct domain *d, u32 offset) { struct acpi_subtable_header *header; @@ -1387,6 +1395,25 @@ gic_acpi_parse_madt_distributor(struct acpi_subtable_header *header, return 0; } + +static int __init +gic_acpi_parse_madt_redistributor(struct acpi_subtable_header *header, + const unsigned long end) +{ +struct acpi_madt_generic_redistributor *rdist; + +rdist = (struct acpi_madt_generic_redistributor *)header; +if ( BAD_MADT_ENTRY(rdist, end) ) +return -EINVAL; + +if ( !rdist->base_address || !rdist->length ) +return -EINVAL; In the commit message you said that the behavior is unchanged, however this check is not part of the previous code. Anyway, I don't think this check is necessary. Sure, I'll remove the validation check from here. + +gic_acpi_add_rdist_region(rdist->base_address, rdist->length); + +return 0; +} + static int __init gic_acpi_get_madt_redistributor_num(struct acpi_subtable_header *header, const unsigned long end) @@ -1402,7 +1429,7 @@ static void __init gicv3_acpi_init(void) struct acpi_table_header *table; struct rdist_region *rdist_regs; acpi_status status; -int count, i; +int count; status = acpi_get_table(ACPI_SIG_MADT, 0, &table); @@ -1433,37 +1460,27 @@ static void __init gicv3_acpi_init(void) if ( count <= 0 ) panic("GICv3: No valid GICR entries exists"); -gicv3.rdist_count = count; - -if ( gicv3.rdist_count > MAX_RDIST_COUNT ) +if ( count > MAX_RDIST_COUNT ) panic("GICv3: Number of redistributor regions is more than" "%d (Increase MAX_RDIST_COUNT!!)\n", MAX_RDIST_COUNT); -rdist_regs = xzalloc_array(struct rdist_region, gicv3.rdist_count); +rdist_regs = xzalloc_array(struct rdist_region, count); if ( !rdist_regs ) panic("GICv3: Failed to allocate memory for rdist regions\n"); -for ( i = 0; i < gicv3.rdist_count; i++ ) -{ -struct acpi_subtable_header *header; -struct acpi_madt_generic_redistributor *gic_rdist; - -header = acpi_table_get_entry_madt(ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR, - i); -if ( !header ) -panic("GICv3: Can't get GICR entry"); - -gic_rdist = - container_of(header, struct acpi_madt_generic_redistributor, header); -rdist_regs[i].base = gic_rdist->base_address; -rdist_regs[i].size = gic_rdist->length; -} +gicv3.rdist_regions = rdist_regs; + +/* Parse always-on power domain Re-distributor entries */ +count = acpi_parse_entries(ACPI_SIG_MADT, + sizeof(struct acpi_table_madt), + gic_acpi_parse_madt_redistributor, table, + ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR, count); Please use acpi_table_parse_madt here. Sure. +if ( count <= 0 ) +panic("GICv3: Can't get Redistributor entry"); /* The vGIC code requires the region to be sorted */ sort(rdist_regs, gicv3.rdist_count, sizeof(*rdist_regs), cmp_rdist, NULL); -gicv3.rdist_regions= rdist_regs; - /* Collect CPU base addresses */ count = acpi_parse_entries(ACPI_SIG_MADT, sizeof(struct acpi_table_madt), gic_acpi_parse_madt_cpu, table, Regards, -- Shanker Donthineni Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH V2 08/10] arm/io: Use separate memory allocation for mmio handlers
Hi Shanker, On 26/06/16 18:48, Shanker Donthineni wrote: The number of mmio handlers are limited to a compile time macro MAX_IO_HANDLER which is 16. This number is not at all sufficient to support per CPU distributor regions. Either it needs to be increased to a bigger number, at least CONFIG_NR_CPUS+16, or allocate a separate memory for mmio handlers dynamically during domain build. This patch uses the dynamic allocation strategy to reduce memory footprint for 'struct domain' instead of static allocation. Signed-off-by: Shanker Donthineni Acked-by: Julien Grall Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] Elaboration of "Question about sharing spinlock_t among VMs in Xen"
On Sun, Jun 26, 2016 at 10:44:53PM -0400, Dagaen Golomb wrote: > I wanted some elaboration on this question and answer posted recently. > > On 06/13/2016 01:43 PM, Meng Xu wrote: > >> Hi, > >> > >> I have a quick question about using the Linux spin_lock() in Xen > >> environment to protect some host-wide shared (memory) resource among > >> VMs. > >> > >> *** The question is as follows *** > >> Suppose I have two Linux VMs sharing the same spinlock_t lock (through > >> the sharing memory) on the same host. Suppose we have one process in > >> each VM. Each process uses the linux function spin_lock(&lock) [1] to > >> grab & release the lock. > >> Will these two processes in the two VMs have race on the shared lock? > > > You can't do this: depending on which Linux version you use you will > > find that kernel uses ticketlocks or qlocks locks which keep track of > > who is holding the lock (obviously this information is internal to VM). > > On top of this on Xen we use pvlocks which add another (internal) > > control layer. > > I wanted to see if this can be done with the correct combination of > versions and parameters. We are using 4.1.0 for all domains, which > still has the CONFIG_PARAVIRT_SPINLOCK option. I've recompiled the > guests with this option set to n, and have also added the boot > parameter xen_nopvspin to both domains and dom0 for good measure. A > basic ticketlock holds all the information inside the struct itself to > order the requests, and I believe this is the version I'm using. Hm, weird. B/c from arch/x86/include/asm/spinlock_types.h: 6 #ifdef CONFIG_PARAVIRT_SPINLOCKS 7 #define __TICKET_LOCK_INC 2 8 #define TICKET_SLOWPATH_FLAG((__ticket_t)1) 9 #else 10 #define __TICKET_LOCK_INC 1 11 #define TICKET_SLOWPATH_FLAG((__ticket_t)0) 12 #endif 13 Which means that one of your guests is adding '2' while another is adding '1'. Or one of them is putting the 'slowpath' flag which means that the paravirt spinlock is enabled. > > Do you think this *should* work? I am still getting a deadlock issue > but I do not believe its due to blocking vcpus, especially after the > above changes. Instead, I believe the spinlock struct is getting > corrupted. To be more precise, I only have two competing domains as a > test, both domUs. I print the raw spinlock struct out when I create it > and after a lock/unlock test. I get the following: > > Init: [ 00 00 00 00 ] > Lock: [ 00 00 02 00 ] > Unlock: [ 02 00 02 00 ] > Lock: [ 02 00 04 00 ] > Unlock: [ 04 00 04 00 ] > > It seems clear from the output and reading I've done that the first 2 > bytes are the "currently servicing" number and the next two are the > "next number to draw" value. With only two guests, one should always > be getting serviced while another waits, so I would expect these two > halves to stay nearly the same (within one grab actually) and end with > both values equal when both are done with their locking/unlocking. > Instead, after what seems to be deadlock I destroy the VMs and print > the spinlock values an its this: [ 11 1e 14 1e ]. Note the 11 and 14, > should these be an odd number apart? The accesses I see keep them > even. Please correct me if I am wrong! Seems practically every time > there is this issue, the first pair of bytes are 3 off and the last > pair match. Could this have something to do with the issue? The odd number would suggest that the TICKET_SLOWPATH_FLAG has been set. > > >> My speculation is that it should have the race on the shard lock when > >> the spin_lock() function in *two VMs* operate on the same lock. > >> > >> We did some quick experiment on this and we found one VM sometimes see > >> the soft lockup on the lock. But we want to make sure our > >> understanding is correct. > >> > >> We are exploring if we can use the spin_lock to protect the shared > >> resources among VMs, instead of using the PV drivers. If the > >> spin_lock() in linux can provide the host-wide atomicity (which will > >> surprise me, though), that will be great. Otherwise, we probably have > >> to expose the spin_lock in Xen to the Linux? > > > I'd think this has to be via the hypervisor (or some other third party). > > Otherwise what happens if one of the guests dies while holding the lock? > > -boris > > This is a valid point against locking in the guests, but itself won't > prevent a spinlock implementation from working! We may move this > direction for several reasons but I am interested in why the above is > not working when I've disabled the PV part that sleeps vcpus. > > Regards, > Dagaen Gol
Re: [Xen-devel] [PATCH V2 04/10] arm/gic-v3: Parse per-cpu redistributor entry in GICC subtable
On 06/27/2016 06:47 AM, Julien Grall wrote: Hi Shanker, On 26/06/16 18:48, Shanker Donthineni wrote: The redistributor address can be specified either as part of GICC or GICR subtable depending on the power domain. The current driver doesn't support parsing redistributor entry that is defined in GICC subtable. The GIC CPU subtable entry holds the associated Redistributor base address if it is not on always-on power domain. The per CPU Redistributor size is not defined in ACPI specification. Set it's size to SZ_256K if the GIC hardware is capable of Direct s/it's/its/, although I would use "the". Virtual LPI Injection feature otherwise SZ_128K. "Set the size to SZ_256K if the GIC hardware is supporting Direct Virtual LPI injection, SZ_128K otherwise". This patch adds necessary code to handle both types of Redistributors base addresses. Signed-off-by: Shanker Donthineni --- Changes since v1: Edited commit text and fixed white spaces. Added a new function for parsing per CPU Redistributor entry. xen/arch/arm/gic-v3.c | 84 ++- xen/include/asm-arm/gic.h | 1 + xen/include/asm-arm/gic_v3_defs.h | 1 + 3 files changed, 77 insertions(+), 9 deletions(-) diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c index 0471fea..3977244 100644 --- a/xen/arch/arm/gic-v3.c +++ b/xen/arch/arm/gic-v3.c @@ -659,6 +659,10 @@ static int __init gicv3_populate_rdist(void) smp_processor_id(), i, ptr); return 0; } + +if ( gicv3.rdist_regions[i].single_rdist ) +break; + if ( gicv3.rdist_stride ) ptr += gicv3.rdist_stride; else @@ -1282,14 +1286,21 @@ static int gicv3_iomem_deny_access(const struct domain *d) } #ifdef CONFIG_ACPI -static void __init gic_acpi_add_rdist_region(u64 base_addr, u32 size) +static void __init +gic_acpi_add_rdist_region(u64 base_addr, u32 size, bool single_rdist) { unsigned int idx = gicv3.rdist_count++; +gicv3.rdist_regions[idx].single_rdist = single_rdist; gicv3.rdist_regions[idx].base = base_addr; gicv3.rdist_regions[idx].size = size; } +static inline bool gic_dist_supports_dvis(void) +{ +return !!(readl_relaxed(GICD + GICD_TYPER) & GICD_TYPER_DVIS); +} + static int gicv3_make_hwdom_madt(const struct domain *d, u32 offset) { struct acpi_subtable_header *header; @@ -1397,6 +1408,42 @@ gic_acpi_parse_madt_distributor(struct acpi_subtable_header *header, } static int __init +gic_acpi_parse_cpu_redistributor(struct acpi_subtable_header *header, + const unsigned long end) +{ +struct acpi_madt_generic_interrupt *processor; +u32 size; + +processor = (struct acpi_madt_generic_interrupt *)header; +if ( BAD_MADT_ENTRY(processor, end) ) +return -EINVAL; + +if ( !processor->gicr_base_address ) +return -EINVAL; You already check it in gic_acpi_get_madt_cpu_num, so there is no reason to do it again. Other function just finds the number of valid cpu interfaces. I would prefer to keep the validation check here. + +if ( processor->flags & ACPI_MADT_ENABLED ) +{ +size = gic_dist_supports_dvis() ? 4 * SZ_64K : 2 * SZ_64K; + gic_acpi_add_rdist_region(processor->gicr_base_address, size, true); +} I would revert the condition to avoid one level of indentation. I.e I'll do. if ( !(processor->flags & ACPI_MADT_ENABLED) ) return 0; size = gic_acpi_add... return 0; However, it looks like that the other function that parses GICC within gic-v3.c (see gic_acpi_parse_madt_cpu) does not check if the CPU is usable. Disabled GICC entries should be skipped because its Redistributor region is not always-on power domain. Please look at my review comment to your KVM-ACPI patch http://www.gossamer-threads.com/lists/linux/kernel/2413670. I think we need to have the same parsing behavior on every function. + +return 0; +} + +static int __init +gic_acpi_get_madt_cpu_num(struct acpi_subtable_header *header, +const unsigned long end) +{ +struct acpi_madt_generic_interrupt *cpuif; + +cpuif = (struct acpi_madt_generic_interrupt *)header; +if ( BAD_MADT_ENTRY(cpuif, end) || !cpuif->gicr_base_address ) +return -EINVAL; + +return 0; +} + +static int __init gic_acpi_parse_madt_redistributor(struct acpi_subtable_header *header, const unsigned long end) { @@ -1409,7 +1456,7 @@ gic_acpi_parse_madt_redistributor(struct acpi_subtable_header *header, if ( !rdist->base_address || !rdist->length ) return -EINVAL; -gic_acpi_add_rdist_region(rdist->base_address, rdist->length); +gic_acpi_add_rdist_region(rdist->base_address, rdist->length, false); return 0; } @@ -1428,6 +1475,7 @@ static void __init gicv3_acpi_init(v
Re: [Xen-devel] [PATCH V2 05/10] xen/arm: vgic: Use dynamic memory allocation for vgic_rdist_region
On 06/27/2016 07:38 AM, Julien Grall wrote: Hi Shanker, On 26/06/16 18:48, Shanker Donthineni wrote: The number of Re-distributor regions allowed for dom0 is hardcoded s/Re-distributor/Redistributor/ to a compile time macro MAX_RDIST_COUNT which is 4. On some systems, s/a compile time macro/a define/ s/On some/Some/ especially latest server chips might have more than 4 redistributors. I would add a comma after 'chips'. NIT: s/redistributors/Redistributors/ Either we have to increase MAX_RDIST_COUNT to a bigger number or allocate memory based on number of redistributors that are found in s/based on number/based on the number/ MADT table. In the worst case scenario, the macro MAX_RDIST_COUNT should be equal to CONFIG_NR_CPUS in order to support per CPU Redistributors. Increasing MAX_RDIST_COUNT has side effect, it blows 'struct domain' s/has side/has a/ size and hits BUILD_BUG_ON() in domain build code path. struct domain *alloc_domain_struct(void) { struct domain *d; BUILD_BUG_ON(sizeof(*d) > PAGE_SIZE); d = alloc_xenheap_pages(0, 0); if ( d == NULL ) return NULL; ... This patch uses the second approach to fix the BUILD_BUG(). Signed-off-by: Shanker Donthineni --- Changes since v1: Keep 'struct vgic_rdist_region' definition inside 'struct arch_domain'. xen/arch/arm/vgic-v2.c | 6 ++ xen/arch/arm/vgic-v3.c | 22 +++--- xen/arch/arm/vgic.c | 1 + xen/include/asm-arm/domain.h | 2 +- xen/include/asm-arm/vgic.h | 2 ++ 5 files changed, 29 insertions(+), 4 deletions(-) diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c index 9adb4a9..f5778e6 100644 --- a/xen/arch/arm/vgic-v2.c +++ b/xen/arch/arm/vgic-v2.c @@ -699,9 +699,15 @@ static int vgic_v2_domain_init(struct domain *d) return 0; } +static void vgic_v2_domain_free(struct domain *d) +{ +/* Nothing to be cleanup for this driver */ +} + static const struct vgic_ops vgic_v2_ops = { .vcpu_init = vgic_v2_vcpu_init, .domain_init = vgic_v2_domain_init, +.domain_free = vgic_v2_domain_free, .max_vcpus = 8, }; diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c index b37a7c0..e877e9e 100644 --- a/xen/arch/arm/vgic-v3.c +++ b/xen/arch/arm/vgic-v3.c @@ -1393,7 +1393,19 @@ static int vgic_v3_vcpu_init(struct vcpu *v) static int vgic_v3_domain_init(struct domain *d) { -int i; +struct vgic_rdist_region *rdist_regions; +int rdist_count, i; + +/* Allocate memory for Re-distributor regions */ +rdist_count = is_hardware_domain(d) ? vgic_v3_hw.nr_rdist_regions : + GUEST_GICV3_RDIST_REGIONS; I would directly introduce the inline helper in this patch, rather than in patch #10. Okay, I'll add a helper function as part of this patch. + +rdist_regions = xzalloc_array(struct vgic_rdist_region, rdist_count); +if ( !rdist_regions ) +return -ENOMEM; + +d->arch.vgic.nr_regions = rdist_count; +d->arch.vgic.rdist_regions = rdist_regions; /* * Domain 0 gets the hardware address. [...] diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h index a2fccc0..fbb763a 100644 --- a/xen/include/asm-arm/vgic.h +++ b/xen/include/asm-arm/vgic.h @@ -128,6 +128,8 @@ struct vgic_ops { int (*vcpu_init)(struct vcpu *v); /* Domain specific initialization of vGIC */ int (*domain_init)(struct domain *d); +/* Release resources that are allocated by domain_init */ s/are/were/ +void (*domain_free)(struct domain *d); /* vGIC sysreg emulation */ int (*emulate_sysreg)(struct cpu_user_regs *regs, union hsr hsr); /* Maximum number of vCPU supported */ Regards, -- Shanker Donthineni Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH V2 09/10] xen/arm: io: Use binary search for mmio handler lookup
On 06/27/2016 08:31 AM, Julien Grall wrote: Hi Shanker, On 26/06/16 18:48, Shanker Donthineni wrote: As the number of I/O handlers increase, the overhead associated with linear lookup also increases. The system might have maximum of 144 (assuming CONFIG_NR_CPUS=128) mmio handlers. In worst case scenario, it would require 144 iterations for finding a matching handler. Now it is time for us to change from linear (complexity O(n)) to a binary search (complexity O(log n) for reducing mmio handler lookup overhead. Signed-off-by: Shanker Donthineni --- xen/arch/arm/io.c | 50 +++--- 1 file changed, 39 insertions(+), 11 deletions(-) diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c index a5b2c2d..abf49fb 100644 --- a/xen/arch/arm/io.c +++ b/xen/arch/arm/io.c @@ -20,6 +20,7 @@ #include #include #include +#include #include #include @@ -70,23 +71,38 @@ static int handle_write(const struct mmio_handler *handler, struct vcpu *v, handler->priv); } -int handle_mmio(mmio_info_t *info) +const struct mmio_handler *find_mmio_handler(struct vcpu *v, paddr_t addr) { -struct vcpu *v = current; -int i; -const struct mmio_handler *handler = NULL; const struct vmmio *vmmio = &v->domain->arch.vmmio; +const struct mmio_handler *handler = vmmio->handlers; +unsigned int eidx = vmmio->num_entries; +unsigned int midx = eidx / 2; +unsigned int sidx = 0; -for ( i = 0; i < vmmio->num_entries; i++ ) +/* Do binary search for matching mmio handler */ +while ( sidx != midx ) { -handler = &vmmio->handlers[i]; - -if ( (info->gpa >= handler->addr) && - (info->gpa < (handler->addr + handler->size)) ) -break; +if ( addr < handler[midx].addr ) +eidx = midx; +else +sidx = midx; +midx = sidx + (eidx - sidx) / 2; This binary search can be simplified. For instance, why do you want to compute midx at the end rather than at the beginning. This would avoid to have "unsigned int midx = eidx / 2" at the beginning. Let me try to use "do while()" loop logic to simplify the above code logic. } -if ( i == vmmio->num_entries ) +if ( (addr >= handler[sidx].addr) && + (addr < (handler[sidx].addr + handler[sidx].size)) ) +return handler + sidx; Please use a temporary variable for handler[sidx]. So it will be easier to read the code. + +return NULL; +} + +int handle_mmio(mmio_info_t *info) +{ +const struct mmio_handler *handler; +struct vcpu *v = current; + +handler = find_mmio_handler(v, info->gpa); I would have expected some locking here. Could you explain why it is safe to looking find the handler with your solution? For what is worth, there was no locking before because register_mmio_handler was always adding the handler at the end of the array. This is not true anymore because you are sorting the array. The function register_mmio_handler() is called only during dom0/domU domain build code path. We don't need locking here until unless some code inserting mmio handlers at runtime. +if ( !handler ) return 0; if ( info->dabt.write ) @@ -95,6 +111,14 @@ int handle_mmio(mmio_info_t *info) return handle_read(handler, v, info); } +static int cmp_mmio_handler(const void *key, const void *elem) +{ +const struct mmio_handler *handler0 = key; +const struct mmio_handler *handler1 = elem; + +return (handler0->addr < handler1->addr) ? -1 : 0; +} + void register_mmio_handler(struct domain *d, const struct mmio_handler_ops *ops, paddr_t addr, paddr_t size, void *priv) @@ -122,6 +146,10 @@ void register_mmio_handler(struct domain *d, vmmio->num_entries++; +/* Sort mmio handlers in ascending order based on base address */ +sort(vmmio->handlers, vmmio->num_entries, sizeof(struct mmio_handler), + cmp_mmio_handler, NULL); + spin_unlock(&vmmio->lock); } Regards, -- Shanker Donthineni Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH V2 10/10] arm/vgic: Change fixed number of mmio handlers to variable number
On 06/27/2016 08:35 AM, Julien Grall wrote: Hi Shanker, On 26/06/16 18:48, Shanker Donthineni wrote: diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h index 29346c6..b205461 100644 --- a/xen/include/asm-arm/domain.h +++ b/xen/include/asm-arm/domain.h @@ -111,6 +111,7 @@ struct arch_domain int nr_regions; /* Number of rdist regions */ uint32_t rdist_stride; /* Re-Distributor stride */ #endif +uint32_t mmio_count;/* Number of mmio handlers */ Is it necessary to have this value part of the arch_domain? I.e Do we need this value after the initialization? If not, then it might be better to add a parameter to domain_vgic_register uint32_t *pointer. Absolutely, we don't need this variable after the domain build process. I have taken this approach to avoid too many code changes. Your suggestion requires changes to functions vgic_v2/v3_init() prototype for adding a new parameter. } vgic; struct vuart { diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h index fbb763a..1ce441c 100644 --- a/xen/include/asm-arm/vgic.h +++ b/xen/include/asm-arm/vgic.h @@ -307,6 +307,7 @@ extern void register_vgic_ops(struct domain *d, const struct vgic_ops *ops); int vgic_v2_init(struct domain *d); int vgic_v3_init(struct domain *d); +extern int domain_vgic_register(struct domain *d); extern int vcpu_vgic_free(struct vcpu *v); extern int vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, int virq, Regards, -- Shanker Donthineni Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH V2 04/10] arm/gic-v3: Parse per-cpu redistributor entry in GICC subtable
On 27/06/16 15:17, Shanker Donthineni wrote: static int __init +gic_acpi_parse_cpu_redistributor(struct acpi_subtable_header *header, + const unsigned long end) +{ +struct acpi_madt_generic_interrupt *processor; +u32 size; + +processor = (struct acpi_madt_generic_interrupt *)header; +if ( BAD_MADT_ENTRY(processor, end) ) +return -EINVAL; + +if ( !processor->gicr_base_address ) +return -EINVAL; You already check it in gic_acpi_get_madt_cpu_num, so there is no reason to do it again. Other function just finds the number of valid cpu interfaces. I would prefer to keep the validation check here. The function acpi_parse_entries (& co) does not do what you think. The function will return an error as soon as one call to the handler (here gic_acpi_get_madt_cpu_num) return a non-zero value (see the implementation of acpi_parse_entries_array). So if the CPU interface is not valid (i.e gicr_base_address it a non-zero value), then it will return an error. Therefore this check is pointless. + +if ( processor->flags & ACPI_MADT_ENABLED ) +{ +size = gic_dist_supports_dvis() ? 4 * SZ_64K : 2 * SZ_64K; + gic_acpi_add_rdist_region(processor->gicr_base_address, size, true); +} I would revert the condition to avoid one level of indentation. I.e I'll do. if ( !(processor->flags & ACPI_MADT_ENABLED) ) return 0; size = gic_acpi_add... return 0; However, it looks like that the other function that parses GICC within gic-v3.c (see gic_acpi_parse_madt_cpu) does not check if the CPU is usable. Disabled GICC entries should be skipped because its Redistributor region is not always-on power domain. I am not sure to follow here. A usable CPU may have his Redistributor in the not always-on power domain. So the issue would be the same, correct? Please look at my review comment to your KVM-ACPI patch http://www.gossamer-threads.com/lists/linux/kernel/2413670. Well in this case the check needs to be done in the other function too (gic_acpi_parse_madt_cpu). Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v12 4/6] IOMMU/x86: using a struct pci_dev* instead of SBDF
>>> On 27.06.16 at 13:11, wrote: > On June 27, 2016 4:17 PM, Jan Beulich wrote: >> >>> On 24.06.16 at 07:51, wrote: >> > @@ -98,7 +104,13 @@ void disable_ats_device(int seg, int bus, int >> > devfn) >> >> For symmetry reasons this function would also get converted to taking const >> struct pci_dev *. >> > > What about ' struct pci_dev *', without const? Sure - since the other one apparently can't have the const added, this one doesn't need to either (but please nevertheless do add it it that's actually possible without having to cast away constness somewhere. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v12 6/6] vt-d: fix vt-d Device-TLB flush timeout issue
>>> On 27.06.16 at 14:56, wrote: > On June 27, 2016 4:24 PM, Jan Beulich wrote: >> >>> On 24.06.16 at 07:51, wrote: >> > @@ -199,24 +199,73 @@ static int __must_check >> queue_invalidate_wait(struct iommu *iommu, >> > return -EOPNOTSUPP; >> > } >> > >> > -static int __must_check invalidate_sync(struct iommu *iommu, >> > -bool_t flush_dev_iotlb) >> > +static int __must_check invalidate_sync(struct iommu *iommu) >> > { >> > struct qi_ctrl *qi_ctrl = iommu_qi_ctrl(iommu); >> > >> > ASSERT(qi_ctrl->qinval_maddr); >> > >> > -return queue_invalidate_wait(iommu, 0, 1, 1, flush_dev_iotlb); >> > +return queue_invalidate_wait(iommu, 0, 1, 1, 0); } >> > + >> > +static void dev_invalidate_iotlb_timeout(struct iommu *iommu, u16 did, >> > + struct pci_dev *pdev) { >> > +struct domain *d = NULL; >> > + >> > +if ( test_bit(did, iommu->domid_bitmap) ) >> > +d = rcu_lock_domain_by_id(iommu->domid_map[did]); >> > + >> > +/* >> > + * In case the domain has been freed or the IOMMU domid bitmap is >> > + * not valid, the device no longer belongs to this domain. >> > + */ >> > +if ( d == NULL ) >> > +return; >> > + >> > +pcidevs_lock(); >> > +ASSERT(pdev->domain); >> > +list_del(&pdev->domain_list); >> > +pdev->domain = NULL; >> > +pci_hide_existing_device(pdev); >> > +pcidevs_unlock(); >> > + >> > +if ( !d->is_shutting_down && printk_ratelimit() ) >> > +printk(XENLOG_WARNING VTDPREFIX >> > + " dom%d: ATS device %04x:%02x:%02x.%u flush failed\n", >> > + d->domain_id, pdev->seg, pdev->bus, >> > + PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn)); >> > + >> > +if ( !is_hardware_domain(d) ) >> > +domain_crash(d); >> > + >> > +rcu_unlock_domain(d); >> > +} >> >> So in an earlier patch in this series you (supposedly) moved similar logic >> up to >> the vendor independent layer. I think this then would better get moved up >> too, if at all possible. >> > > To be honest, I have not much reason for leaving domain crash here and I was > aware of this problem, but crash_domain() here is not harmful (as the > 'd->is_shutting_down' is Set when to crash, and once the > 'd->is_shutting_down' > is Set then return in domain_shutdown() ). > In case crash domain directly, it may help us narrow down the 'window' (the > domain is still running).. > > To me, moving the logic up is acceptable. > > In next version, could I only drop: > > +if ( !is_hardware_domain(d) ) > +domain_crash(d); > > In this patch, and leave the rest as is ? Not really - the entire function looks like it could move out of vtd/, as I can't see anything VT-d specific in it. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 18/19] xen: credit2: implement SMT support independent runq arrangement
On 18/06/16 00:13, Dario Faggioli wrote: In fact, right now, we recommend keepeing runqueues arranged per-core, so that it is the inter-runqueue load balancing code that automatically spreads the work in an SMT friendly way. This means that any other runq arrangement one may want to use falls short of SMT scheduling optimizations. This commit implements SMT awareness --similar to the one we have in Credit1-- for any possible runq arrangement. This turned out to be pretty easy to do, as the logic can live entirely in runq_tickle() (although, in order to avoid for_each_cpu loops in that function, we use a new cpumask which indeed needs to be updated in other places). In addition to disentangling SMT awareness from load balancing, this also allows us to support the sched_smt_power_savings parametar in Credit2 as well. Signed-off-by: Dario Faggioli Reviewed-by: Anshul Makkar --- Cc: George Dunlap Cc: Anshul Makkar Cc: David Vrabel --- xen/common/sched_credit2.c | 141 +++- 1 file changed, 126 insertions(+), 15 deletions(-) diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c index 93943fa..a8b3a85 100644 --- a/xen/common/sched_credit2.c +++ b/xen/common/sched_credit2.c @@ -351,7 +351,8 @@ struct csched2_runqueue_data { unsigned int max_weight; cpumask_t idle,/* Currently idle */ -tickled; /* Another cpu in the queue is already targeted for this one */ +smt_idle, /* Fully idle cores (as in all the siblings are idle) */ +tickled; /* Have been asked to go through schedule */ int load; /* Instantaneous load: Length of queue + num non-idle threads */ s_time_t load_last_update; /* Last time average was updated */ s_time_t avgload; /* Decaying queue load */ @@ -412,6 +413,73 @@ struct csched2_dom { }; /* + * Hyperthreading (SMT) support. + * + * We use a special per-runq mask (smt_idle) and update it according to the + * following logic: + * - when _all_ the SMT sibling in a core are idle, all their corresponding + *bits are set in the smt_idle mask; + * - when even _just_one_ of the SMT siblings in a core is not idle, all the + *bits correspondings to it and to all its siblings are clear in the + *smt_idle mask. + * + * Once we have such a mask, it is easy to implement a policy that, either: + * - uses fully idle cores first: it is enough to try to schedule the vcpus + *on pcpus from smt_idle mask first. This is what happens if + *sched_smt_power_savings was not set at boot (default), and it maximizes + *true parallelism, and hence performance; + * - uses already busy cores first: it is enough to try to schedule the vcpus + *on pcpus that are idle, but are not in smt_idle. This is what happens if + *sched_smt_power_savings is set at boot, and it allows as more cores as + *possible to stay in low power states, minimizing power consumption. + * + * This logic is entirely implemented in runq_tickle(), and that is enough. + * In fact, in this scheduler, placement of a vcpu on one of the pcpus of a + * runq, _always_ happens by means of tickling: + * - when a vcpu wakes up, it calls csched2_vcpu_wake(), which calls + *runq_tickle(); + * - when a migration is initiated in schedule.c, we call csched2_cpu_pick(), + *csched2_vcpu_migrate() (which calls migrate()) and csched2_vcpu_wake(). + *csched2_cpu_pick() looks for the least loaded runq and return just any + *of its processors. Then, csched2_vcpu_migrate() just moves the vcpu to + *the chosen runq, and it is again runq_tickle(), called by + *csched2_vcpu_wake() that actually decides what pcpu to use within the + *chosen runq; + * - when a migration is initiated in sched_credit2.c, by calling migrate() + *directly, that again temporarily use a random pcpu from the new runq, + *and then calls runq_tickle(), by itself. + */ + +/* + * If all the siblings of cpu (including cpu itself) are in idlers, + * set all their bits in mask. + * + * In order to properly take into account tickling, idlers needs to be + * set qeual to something like: + * + * rqd->idle & (~rqd->tickled) + * + * This is because cpus that have been tickled will very likely pick up some + * work as soon as the manage to schedule, and hence we should really consider + * them as busy. + */ +static inline +void smt_idle_mask_set(unsigned int cpu, cpumask_t *idlers, cpumask_t *mask) +{ +if ( cpumask_subset( per_cpu(cpu_sibling_mask, cpu), idlers) ) +cpumask_or(mask, mask, per_cpu(cpu_sibling_mask, cpu)); +} + +/* + * Clear the bits of all the siblings of cpu from mask. + */ +static inline +void smt_idle_mask_clear(unsigned int cpu, cpumask_t *mask) +{ +cpumask_andnot(mask, mask, per_cpu(cpu_sibling_mask, cpu)); +} + +/* * When a hard affinity change occurs, we may not be able to check some * (any!) of the other runqu
Re: [Xen-devel] Elaboration of "Question about sharing spinlock_t among VMs in Xen"
>> >> *** The question is as follows *** >> >> Suppose I have two Linux VMs sharing the same spinlock_t lock (through >> >> the sharing memory) on the same host. Suppose we have one process in >> >> each VM. Each process uses the linux function spin_lock(&lock) [1] to >> >> grab & release the lock. >> >> Will these two processes in the two VMs have race on the shared lock? >> >> > You can't do this: depending on which Linux version you use you will >> > find that kernel uses ticketlocks or qlocks locks which keep track of >> > who is holding the lock (obviously this information is internal to VM). >> > On top of this on Xen we use pvlocks which add another (internal) >> > control layer. >> >> I wanted to see if this can be done with the correct combination of >> versions and parameters. We are using 4.1.0 for all domains, which >> still has the CONFIG_PARAVIRT_SPINLOCK option. I've recompiled the >> guests with this option set to n, and have also added the boot >> parameter xen_nopvspin to both domains and dom0 for good measure. A >> basic ticketlock holds all the information inside the struct itself to >> order the requests, and I believe this is the version I'm using. > > Hm, weird. B/c from arch/x86/include/asm/spinlock_types.h: > 6 #ifdef CONFIG_PARAVIRT_SPINLOCKS > 7 #define __TICKET_LOCK_INC 2 > 8 #define TICKET_SLOWPATH_FLAG((__ticket_t)1) > 9 #else > 10 #define __TICKET_LOCK_INC 1 > 11 #define TICKET_SLOWPATH_FLAG((__ticket_t)0) > 12 #endif > 13 > > Which means that one of your guests is adding '2' while another is > adding '1'. Or one of them is putting the 'slowpath' flag > which means that the paravirt spinlock is enabled. Interesting. I went back to check on one of my guests, and the .config from the source tree I used, as well as the one in /boot/ for the current build both have it "not set" which shows as unchecked in make menuconfig, where the option was disabled. So this domain appears to be correctly configured. The thing is, the other domain is literally a copy of this domain. Either both are wrong or neither are. >> >> Do you think this *should* work? I am still getting a deadlock issue >> but I do not believe its due to blocking vcpus, especially after the >> above changes. Instead, I believe the spinlock struct is getting >> corrupted. To be more precise, I only have two competing domains as a >> test, both domUs. I print the raw spinlock struct out when I create it >> and after a lock/unlock test. I get the following: >> >> Init: [ 00 00 00 00 ] >> Lock: [ 00 00 02 00 ] >> Unlock: [ 02 00 02 00 ] >> Lock: [ 02 00 04 00 ] >> Unlock: [ 04 00 04 00 ] >> >> It seems clear from the output and reading I've done that the first 2 >> bytes are the "currently servicing" number and the next two are the >> "next number to draw" value. With only two guests, one should always >> be getting serviced while another waits, so I would expect these two >> halves to stay nearly the same (within one grab actually) and end with >> both values equal when both are done with their locking/unlocking. >> Instead, after what seems to be deadlock I destroy the VMs and print >> the spinlock values an its this: [ 11 1e 14 1e ]. Note the 11 and 14, >> should these be an odd number apart? The accesses I see keep them >> even. Please correct me if I am wrong! Seems practically every time >> there is this issue, the first pair of bytes are 3 off and the last >> pair match. Could this have something to do with the issue? > > The odd number would suggest that the TICKET_SLOWPATH_FLAG has been set. It would seem so, and from the default behavior where increments show an increase of two, both of these suggest paravirt spinlocking is still in use. Any idea how to turn these off? I would try disabling any paravirtual options in the configuration but I still need access to XenStore and grant pages, which I feel I would lose by doing so. Its odd that my boot config points to this option being not set, yet the behavior is that it is... >> >> >> My speculation is that it should have the race on the shard lock when >> >> the spin_lock() function in *two VMs* operate on the same lock. >> >> >> >> We did some quick experiment on this and we found one VM sometimes see >> >> the soft lockup on the lock. But we want to make sure our >> >> understanding is correct. >> >> >> >> We are exploring if we can use the spin_lock to protect the shared >> >> resources among VMs, instead of using the PV drivers. If the >> >> spin_lock() in linux can provide the host-wide atomicity (which will >> >> surprise me, though), that will be great. Otherwise, we probably have >> >> to expose the spin_lock in Xen to the Linux? >> >> > I'd think this has to be via the hypervisor (or some other third party). >> > Otherwise what happens if one of the guests dies while holding the lock? >> > -boris >> >> This is a valid point against locking in the guests, but itself won't >> prevent a spinlock implementation from working! We
Re: [Xen-devel] [PATCH V2 09/10] xen/arm: io: Use binary search for mmio handler lookup
Hi Shanker, On 27/06/16 15:50, Shanker Donthineni wrote: On 06/27/2016 08:31 AM, Julien Grall wrote: On 26/06/16 18:48, Shanker Donthineni wrote: As the number of I/O handlers increase, the overhead associated with linear lookup also increases. The system might have maximum of 144 (assuming CONFIG_NR_CPUS=128) mmio handlers. In worst case scenario, it would require 144 iterations for finding a matching handler. Now it is time for us to change from linear (complexity O(n)) to a binary search (complexity O(log n) for reducing mmio handler lookup overhead. Signed-off-by: Shanker Donthineni --- xen/arch/arm/io.c | 50 +++--- 1 file changed, 39 insertions(+), 11 deletions(-) diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c index a5b2c2d..abf49fb 100644 --- a/xen/arch/arm/io.c +++ b/xen/arch/arm/io.c @@ -20,6 +20,7 @@ #include #include #include +#include #include #include @@ -70,23 +71,38 @@ static int handle_write(const struct mmio_handler *handler, struct vcpu *v, handler->priv); } -int handle_mmio(mmio_info_t *info) +const struct mmio_handler *find_mmio_handler(struct vcpu *v, paddr_t addr) { -struct vcpu *v = current; -int i; -const struct mmio_handler *handler = NULL; const struct vmmio *vmmio = &v->domain->arch.vmmio; +const struct mmio_handler *handler = vmmio->handlers; +unsigned int eidx = vmmio->num_entries; +unsigned int midx = eidx / 2; +unsigned int sidx = 0; -for ( i = 0; i < vmmio->num_entries; i++ ) +/* Do binary search for matching mmio handler */ +while ( sidx != midx ) { -handler = &vmmio->handlers[i]; - -if ( (info->gpa >= handler->addr) && - (info->gpa < (handler->addr + handler->size)) ) -break; +if ( addr < handler[midx].addr ) +eidx = midx; +else +sidx = midx; +midx = sidx + (eidx - sidx) / 2; This binary search can be simplified. For instance, why do you want to compute midx at the end rather than at the beginning. This would avoid to have "unsigned int midx = eidx / 2" at the beginning. Let me try to use "do while()" loop logic to simplify the above code logic. Please don't try to re-invent your own binary search implementation and use a known one such as the one implemented by Linux (see bsearch). } -if ( i == vmmio->num_entries ) +if ( (addr >= handler[sidx].addr) && + (addr < (handler[sidx].addr + handler[sidx].size)) ) +return handler + sidx; Please use a temporary variable for handler[sidx]. So it will be easier to read the code. + +return NULL; +} + +int handle_mmio(mmio_info_t *info) +{ +const struct mmio_handler *handler; +struct vcpu *v = current; + +handler = find_mmio_handler(v, info->gpa); I would have expected some locking here. Could you explain why it is safe to looking find the handler with your solution? For what is worth, there was no locking before because register_mmio_handler was always adding the handler at the end of the array. This is not true anymore because you are sorting the array. The function register_mmio_handler() is called only during dom0/domU domain build code path. We don't need locking here until unless some code inserting mmio handlers at runtime. The current implementation of I/O handler is thread-safe because of the spin_lock and lock-less. We may have people building product on top of Xen using register_mmio_handler when the domain is running. So I will nack any patch that cause a regression on the behavior. Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] Xen 4.3, armhf, ocamlopt, wheezy
>>> On 27.06.16 at 13:29, wrote: > Since around January we have been testing our Xen 4.3 stable branches > (which still receive security support until very soon) with a version > of Debian which was current when 4.3 was also current - ie, wheezy. > > Debian wheezy has an `ocaml-nox' package, but it does not contain > `ocamlopt' on armhf. There is code in `configure' in > xen.git#staging-4.3 to attempt to discover whether ocamlopt exists. > That code appears to work, correctly detecting that ocamlopt does not > exist - but then the build tries to use it anyway: > > http://logs.test-lab.xenproject.org/osstest/logs/96291/build-armhf/5.ts-xen-buil > > d.log > > I would like to get one final push of Xen 4.3 before it goes out of > security support. I don't think this configuration bug in Xen 4.3 is > recently introduced. I think that previous tests we were probably > using squeeze, which probably did not have ocaml at all, and that we > were using an osstest which didn't try to use it. > > I propose to drop the ocaml package installation from osstest runs > using Debian squeeze or Debian wheezy. Fine with me, fwiw. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [libvirt] Fixing libvirt's libxl driver breakage -- where to define LIBXL_API_VERSION?
Jim Fehlig writes ("Re: [libvirt] [Xen-devel] Fixing libvirt's libxl driver breakage -- where to define LIBXL_API_VERSION?"): > I think that is a good idea too. I've sent a patch setting > LIBXL_API_VERSION to 0x040200 > > https://www.redhat.com/archives/libvir-list/2016-April/msg00771.html It seems that the libvirt LIBXL_API_VERSION is now rather higher, at 0x040400, since libvirt#fccf27253ced libxl: switch to using libxl_domain_create_restore from v4.4 API One unfortunate effect of this is to break the osstest tests of the Xen 4.3 stable branch, which for the moment is still allegedly in security support. I can't really see a way that this kind of problem could be avoided in principle, without - providing a more sophisticated way for libxl callers to set the requested version - providing more compatibility code in libvirt, too, and retaining it for some time I think instead that it would probably be better for osstest to "freeze" the version of libvirt that it is using, every time we branch Xen. So Xen 4.4 would be tested with whatever libvirt we were using when the stable branch for Xen 4.4 was made, and so on. Does that sound sensible ? Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH V2 03/10] arm/gic-v3: Fold GICR subtable parsing into a new function
On 06/27/2016 06:26 AM, Julien Grall wrote: Hi Shanker, Title: I think you want to say "Move GICR..." rather than "Fold GICR...". On 26/06/16 18:48, Shanker Donthineni wrote: Add a new function for parsing GICR subtable and move the code Add a new function to parse GICR... that is specific to GICR table to new function without changing to a new function the function gicv3_acpi_init() behavior. Signed-off-by: Shanker Donthineni --- Changes since v1: Removed the unnecessary GICR ioremap operation inside GICR table parse code. xen/arch/arm/gic-v3.c | 61 --- 1 file changed, 39 insertions(+), 22 deletions(-) diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c index 542c4f3..0471fea 100644 --- a/xen/arch/arm/gic-v3.c +++ b/xen/arch/arm/gic-v3.c @@ -1282,6 +1282,14 @@ static int gicv3_iomem_deny_access(const struct domain *d) } #ifdef CONFIG_ACPI +static void __init gic_acpi_add_rdist_region(u64 base_addr, u32 size) Please use paddr_t for both parameter. Also the suffix _addr is pointless. +{ +unsigned int idx = gicv3.rdist_count++; + +gicv3.rdist_regions[idx].base = base_addr; +gicv3.rdist_regions[idx].size = size; +} + static int gicv3_make_hwdom_madt(const struct domain *d, u32 offset) { struct acpi_subtable_header *header; @@ -1387,6 +1395,25 @@ gic_acpi_parse_madt_distributor(struct acpi_subtable_header *header, return 0; } + +static int __init +gic_acpi_parse_madt_redistributor(struct acpi_subtable_header *header, + const unsigned long end) +{ +struct acpi_madt_generic_redistributor *rdist; + +rdist = (struct acpi_madt_generic_redistributor *)header; +if ( BAD_MADT_ENTRY(rdist, end) ) +return -EINVAL; + +if ( !rdist->base_address || !rdist->length ) +return -EINVAL; In the commit message you said that the behavior is unchanged, however this check is not part of the previous code. Anyway, I don't think this check is necessary. + +gic_acpi_add_rdist_region(rdist->base_address, rdist->length); + +return 0; +} + static int __init gic_acpi_get_madt_redistributor_num(struct acpi_subtable_header *header, const unsigned long end) @@ -1402,7 +1429,7 @@ static void __init gicv3_acpi_init(void) struct acpi_table_header *table; struct rdist_region *rdist_regs; acpi_status status; -int count, i; +int count; status = acpi_get_table(ACPI_SIG_MADT, 0, &table); @@ -1433,37 +1460,27 @@ static void __init gicv3_acpi_init(void) if ( count <= 0 ) panic("GICv3: No valid GICR entries exists"); -gicv3.rdist_count = count; - -if ( gicv3.rdist_count > MAX_RDIST_COUNT ) +if ( count > MAX_RDIST_COUNT ) panic("GICv3: Number of redistributor regions is more than" "%d (Increase MAX_RDIST_COUNT!!)\n", MAX_RDIST_COUNT); -rdist_regs = xzalloc_array(struct rdist_region, gicv3.rdist_count); +rdist_regs = xzalloc_array(struct rdist_region, count); if ( !rdist_regs ) panic("GICv3: Failed to allocate memory for rdist regions\n"); -for ( i = 0; i < gicv3.rdist_count; i++ ) -{ -struct acpi_subtable_header *header; -struct acpi_madt_generic_redistributor *gic_rdist; - -header = acpi_table_get_entry_madt(ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR, - i); -if ( !header ) -panic("GICv3: Can't get GICR entry"); - -gic_rdist = - container_of(header, struct acpi_madt_generic_redistributor, header); -rdist_regs[i].base = gic_rdist->base_address; -rdist_regs[i].size = gic_rdist->length; -} +gicv3.rdist_regions = rdist_regs; + +/* Parse always-on power domain Re-distributor entries */ +count = acpi_parse_entries(ACPI_SIG_MADT, + sizeof(struct acpi_table_madt), + gic_acpi_parse_madt_redistributor, table, + ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR, count); Please use acpi_table_parse_madt here. How do we pass MADT table pointer that we are using to a function acpi_table_parse_madt()? We have already obtained the MADT table address by calling acpi_get_table() at the beginning of the function gicv3_acpi_init(). +if ( count <= 0 ) +panic("GICv3: Can't get Redistributor entry"); /* The vGIC code requires the region to be sorted */ sort(rdist_regs, gicv3.rdist_count, sizeof(*rdist_regs), cmp_rdist, NULL); -gicv3.rdist_regions= rdist_regs; - /* Collect CPU base addresses */ count = acpi_parse_entries(ACPI_SIG_MADT, sizeof(struct acpi_table_madt), gic_acpi_parse_madt_cpu, table, Regards, -- Shanker Donthineni Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ___ Xen-devel ma
Re: [Xen-devel] [PATCH V2 03/10] arm/gic-v3: Fold GICR subtable parsing into a new function
On 27/06/16 16:40, Shanker Donthineni wrote: +gicv3.rdist_regions = rdist_regs; + +/* Parse always-on power domain Re-distributor entries */ +count = acpi_parse_entries(ACPI_SIG_MADT, + sizeof(struct acpi_table_madt), + gic_acpi_parse_madt_redistributor, table, + ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR, count); Please use acpi_table_parse_madt here. How do we pass MADT table pointer that we are using to a function acpi_table_parse_madt()? We have already obtained the MADT table address by calling acpi_get_table() at the beginning of the function gicv3_acpi_init(). You don't need to pass it. The function will take care to get the MADT table for you. Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [libvirt] Fixing libvirt's libxl driver breakage -- where to define LIBXL_API_VERSION?
(Adding Jan Beulich) Ian Jackson writes ("Re: [libvirt] [Xen-devel] Fixing libvirt's libxl driver breakage -- where to define LIBXL_API_VERSION?"): > It seems that the libvirt LIBXL_API_VERSION is now rather higher, at > 0x040400, since libvirt#fccf27253ced > libxl: switch to using libxl_domain_create_restore from v4.4 API > > One unfortunate effect of this is to break the osstest tests of the > Xen 4.3 stable branch, which for the moment is still allegedly in > security support. > > I can't really see a way that this kind of problem could be avoided > in principle, without > - providing a more sophisticated way for libxl callers to set the > requested version > - providing more compatibility code in libvirt, too, and retaining > it for some time > > I think instead that it would probably be better for osstest to > "freeze" the version of libvirt that it is using, every time we branch > Xen. > > So Xen 4.4 would be tested with whatever libvirt we were using when > the stable branch for Xen 4.4 was made, and so on. > > Does that sound sensible ? In the assumption that it is, I have: Created the following branch refs on xenbits in the toplevel libvirt.git: osstest/frozen/xen-4.3-testing 9a0c7f5f834185db9017c34aabc03ad99cf37bed osstest/frozen/xen-4.4-testing 33fb8ff185846a7b4974105d2c9400690a6f95cf osstest/frozen/xen-4.5-testing cda1cc170f07b45911b3dad03e42c8ebfc210fa1 osstest/frozen/xen-4.6-testing eac167e2610d3e59b32f7ec7ba78cbc8c420a425 osstest/frozen/xen-4.7-testing 1a41ed5af5e1704dd9b0bdca40df5c9cacbdabfc These were those tested by the following `tolerable' osstest push gate flights for the corresponding Xen tree: xen-4.3-testing 9a0c7f5f8341 86673 xen-4.4-testing 33fb8ff18584 85031 xen-4.5-testing cda1cc170f07 83135 xen-4.6-testing eac167e2610d 96031 xen-4.7-testing 1a41ed5af5e1 95728 And I have prepared the patch below, which (together with a prerequisite, in my tree) will implement this in osstest. Ian. From 5d1c91d3c53b580305e96d62f8ca84f85f8d3011 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Mon, 27 Jun 2016 16:49:52 +0100 Subject: [OSSTEST PATCH] cr-daily-branch: libvirt: use frozen version on stable branches libvirt master might increase its LIBXL_API_VERSION. When this feeds through osstest it can cause the push gates of Xen stable branches to break. So for stable Xen branches do not track libvirt upstream. Instead, use a frozen revision. (Only for main push gate tests of stable Xen branches.) The frozen branch is never going to be updated so it is not suitable for other kinds of uses. In particular it won't get security fixes. So we call the refs osstest/frozen/xen-K.L-testing to discourage users from using them. Deployment note: The Xen release checklist needs a new item "add this frozen libvirt branch". Signed-off-by: Ian Jackson --- cr-daily-branch | 6 ++ 1 file changed, 6 insertions(+) diff --git a/cr-daily-branch b/cr-daily-branch index 8b7c789..21780b8 100755 --- a/cr-daily-branch +++ b/cr-daily-branch @@ -186,6 +186,12 @@ if [ "x$REVISION_OVMF" = x ]; then fi fi if [ "x$REVISION_LIBVIRT" = x ]; then + case "$xenbranch" in + xen-[0-9]*-testing) + BASE_TAG_LIBVIRT=osstest/frozen/$xenbranch + export BASE_TAG_LIBVIRT + ;; + esac determine_version REVISION_LIBVIRT libvirt LIBVIRT export REVISION_LIBVIRT fi -- 2.1.4 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [libvirt] Fixing libvirt's libxl driver breakage -- where to define LIBXL_API_VERSION?
On Mon, Jun 27, 2016 at 04:54:35PM +0100, Ian Jackson wrote: > (Adding Jan Beulich) > > Ian Jackson writes ("Re: [libvirt] [Xen-devel] Fixing libvirt's libxl driver > breakage -- where to define LIBXL_API_VERSION?"): > > It seems that the libvirt LIBXL_API_VERSION is now rather higher, at > > 0x040400, since libvirt#fccf27253ced > > libxl: switch to using libxl_domain_create_restore from v4.4 API > > > > One unfortunate effect of this is to break the osstest tests of the > > Xen 4.3 stable branch, which for the moment is still allegedly in > > security support. > > > > I can't really see a way that this kind of problem could be avoided > > in principle, without > > - providing a more sophisticated way for libxl callers to set the > > requested version > > - providing more compatibility code in libvirt, too, and retaining > > it for some time > > > > I think instead that it would probably be better for osstest to > > "freeze" the version of libvirt that it is using, every time we branch > > Xen. > > > > So Xen 4.4 would be tested with whatever libvirt we were using when > > the stable branch for Xen 4.4 was made, and so on. > > > > Does that sound sensible ? > > In the assumption that it is, I have: > > Created the following branch refs on xenbits in the toplevel > libvirt.git: > > osstest/frozen/xen-4.3-testing 9a0c7f5f834185db9017c34aabc03ad99cf37bed > osstest/frozen/xen-4.4-testing 33fb8ff185846a7b4974105d2c9400690a6f95cf > osstest/frozen/xen-4.5-testing cda1cc170f07b45911b3dad03e42c8ebfc210fa1 > osstest/frozen/xen-4.6-testing eac167e2610d3e59b32f7ec7ba78cbc8c420a425 > osstest/frozen/xen-4.7-testing 1a41ed5af5e1704dd9b0bdca40df5c9cacbdabfc How did you pick those hashes ? Would it make more sense to pick the nearest libvirt release tag ? eg v1.3.2 instead of 33fb8ff18584 ? > > These were those tested by the following `tolerable' osstest push gate > flights for the corresponding Xen tree: > > xen-4.3-testing 9a0c7f5f8341 86673 > xen-4.4-testing 33fb8ff18584 85031 > xen-4.5-testing cda1cc170f07 83135 > xen-4.6-testing eac167e2610d 96031 > xen-4.7-testing 1a41ed5af5e1 95728 > > And I have prepared the patch below, which (together with a > prerequisite, in my tree) will implement this in osstest. > > Ian. > > From 5d1c91d3c53b580305e96d62f8ca84f85f8d3011 Mon Sep 17 00:00:00 2001 > From: Ian Jackson > Date: Mon, 27 Jun 2016 16:49:52 +0100 > Subject: [OSSTEST PATCH] cr-daily-branch: libvirt: use frozen version on > stable branches > > libvirt master might increase its LIBXL_API_VERSION. When this feeds > through osstest it can cause the push gates of Xen stable branches to > break. > > So for stable Xen branches do not track libvirt upstream. Instead, > use a frozen revision. (Only for main push gate tests of stable Xen > branches.) > > The frozen branch is never going to be updated so it is not suitable > for other kinds of uses. In particular it won't get security fixes. > So we call the refs osstest/frozen/xen-K.L-testing to discourage > users from using them. > > Deployment note: The Xen release checklist needs a new item "add this > frozen libvirt branch". > > Signed-off-by: Ian Jackson > --- > cr-daily-branch | 6 ++ > 1 file changed, 6 insertions(+) > > diff --git a/cr-daily-branch b/cr-daily-branch > index 8b7c789..21780b8 100755 > --- a/cr-daily-branch > +++ b/cr-daily-branch > @@ -186,6 +186,12 @@ if [ "x$REVISION_OVMF" = x ]; then > fi > fi > if [ "x$REVISION_LIBVIRT" = x ]; then > + case "$xenbranch" in > + xen-[0-9]*-testing) > + BASE_TAG_LIBVIRT=osstest/frozen/$xenbranch > + export BASE_TAG_LIBVIRT > + ;; > + esac > determine_version REVISION_LIBVIRT libvirt LIBVIRT > export REVISION_LIBVIRT > fi Overall I think your approach makes sense. Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH V2 03/10] arm/gic-v3: Fold GICR subtable parsing into a new function
be On 06/27/2016 10:41 AM, Julien Grall wrote: On 27/06/16 16:40, Shanker Donthineni wrote: +gicv3.rdist_regions = rdist_regs; + +/* Parse always-on power domain Re-distributor entries */ +count = acpi_parse_entries(ACPI_SIG_MADT, + sizeof(struct acpi_table_madt), + gic_acpi_parse_madt_redistributor, table, + ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR, count); Please use acpi_table_parse_madt here. How do we pass MADT table pointer that we are using to a function acpi_table_parse_madt()? We have already obtained the MADT table address by calling acpi_get_table() at the beginning of the function gicv3_acpi_init(). You don't need to pass it. The function will take care to get the MADT table for you. Are you expecting me to modify current driver to replace acpi_parse_entries() calls with acpi_table_parse_madt() before my changes? Regards, -- Shanker Donthineni Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH V2 03/10] arm/gic-v3: Fold GICR subtable parsing into a new function
On 27/06/16 17:07, Shanker Donthineni wrote: On 06/27/2016 10:41 AM, Julien Grall wrote: On 27/06/16 16:40, Shanker Donthineni wrote: +gicv3.rdist_regions = rdist_regs; + +/* Parse always-on power domain Re-distributor entries */ +count = acpi_parse_entries(ACPI_SIG_MADT, + sizeof(struct acpi_table_madt), + gic_acpi_parse_madt_redistributor, table, + ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR, count); Please use acpi_table_parse_madt here. How do we pass MADT table pointer that we are using to a function acpi_table_parse_madt()? We have already obtained the MADT table address by calling acpi_get_table() at the beginning of the function gicv3_acpi_init(). You don't need to pass it. The function will take care to get the MADT table for you. Are you expecting me to modify current driver to replace acpi_parse_entries() calls with acpi_table_parse_madt() before my changes? No, I was just suggesting to modify the caller you changed/added within the different patch. Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [libvirt] Fixing libvirt's libxl driver breakage -- where to define LIBXL_API_VERSION?
Daniel P. Berrange writes ("Re: [libvirt] [Xen-devel] Fixing libvirt's libxl driver breakage -- where to define LIBXL_API_VERSION?"): > On Mon, Jun 27, 2016 at 04:54:35PM +0100, Ian Jackson wrote: > > Created the following branch refs on xenbits in the toplevel > > libvirt.git: > > > > osstest/frozen/xen-4.3-testing 9a0c7f5f834185db9017c34aabc03ad99cf37bed > > osstest/frozen/xen-4.4-testing 33fb8ff185846a7b4974105d2c9400690a6f95cf > > osstest/frozen/xen-4.5-testing cda1cc170f07b45911b3dad03e42c8ebfc210fa1 > > osstest/frozen/xen-4.6-testing eac167e2610d3e59b32f7ec7ba78cbc8c420a425 > > osstest/frozen/xen-4.7-testing 1a41ed5af5e1704dd9b0bdca40df5c9cacbdabfc > > How did you pick those hashes ? Would it make more sense to pick the > nearest libvirt release tag ? eg v1.3.2 instead of 33fb8ff18584 ? > > > These were those tested by the following `tolerable' osstest push gate > > flights for the corresponding Xen tree: > > > > xen-4.3-testing 9a0c7f5f8341 86673 > > xen-4.4-testing 33fb8ff18584 85031 > > xen-4.5-testing cda1cc170f07 83135 > > xen-4.6-testing eac167e2610d 96031 > > xen-4.7-testing 1a41ed5af5e1 95728 I picked them by searching my mail archives for osstest `tolerable' push gate flights - ie, passes in our CI system. That minimises the risk that the selected versions are themselves troublesome for some reason, needing another round of adjustment. It might indeed be better to convert them to nearby release tags. However: mariner:libvirt> git-describe 9a0c7f5f834185db9017c34aabc03ad99cf37bed v1.3.2-202-g9a0c7f5 mariner:libvirt> git-describe 33fb8ff185846a7b4974105d2c9400690a6f95cf v1.3.2-rc2-1-g33fb8ff mariner:libvirt> git-describe cda1cc170f07b45911b3dad03e42c8ebfc210fa1 v1.3.1-262-gcda1cc1 mariner:libvirt> git-describe eac167e2610d3e59b32f7ec7ba78cbc8c420a425 v1.3.5-318-geac167e mariner:libvirt> git-describe 1a41ed5af5e1704dd9b0bdca40df5c9cacbdabfc v1.3.5-129-g1a41ed5 mariner:libvirt> So in most cases these hashes are well away from a release tag. Does libvirt have stable release branches ? One approach would be to have osstest track a suitable libvirt stable release branche for each Xen stable release branch. That would involve setting up a push gate for each of the chosen libvirt stable branches. That would be worthwhile if we expect those stable branches to acquire commits which break Xen, and which we could like to be told about. But I'm not sure that's the case. Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH V2 03/10] arm/gic-v3: Fold GICR subtable parsing into a new function
On 06/27/2016 11:09 AM, Julien Grall wrote: On 27/06/16 17:07, Shanker Donthineni wrote: On 06/27/2016 10:41 AM, Julien Grall wrote: On 27/06/16 16:40, Shanker Donthineni wrote: +gicv3.rdist_regions = rdist_regs; + +/* Parse always-on power domain Re-distributor entries */ +count = acpi_parse_entries(ACPI_SIG_MADT, + sizeof(struct acpi_table_madt), + gic_acpi_parse_madt_redistributor, table, + ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR, count); Please use acpi_table_parse_madt here. How do we pass MADT table pointer that we are using to a function acpi_table_parse_madt()? We have already obtained the MADT table address by calling acpi_get_table() at the beginning of the function gicv3_acpi_init(). You don't need to pass it. The function will take care to get the MADT table for you. Are you expecting me to modify current driver to replace acpi_parse_entries() calls with acpi_table_parse_madt() before my changes? No, I was just suggesting to modify the caller you changed/added within the different patch. Code becomes ugly for readability purpose using two set of functions in a single function. Regards, -- Shanker Donthineni Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [libvirt] Fixing libvirt's libxl driver breakage -- where to define LIBXL_API_VERSION?
On Mon, Jun 27, 2016 at 05:12:52PM +0100, Ian Jackson wrote: > Daniel P. Berrange writes ("Re: [libvirt] [Xen-devel] Fixing libvirt's libxl > driver breakage -- where to define LIBXL_API_VERSION?"): > > On Mon, Jun 27, 2016 at 04:54:35PM +0100, Ian Jackson wrote: > > > Created the following branch refs on xenbits in the toplevel > > > libvirt.git: > > > > > > osstest/frozen/xen-4.3-testing 9a0c7f5f834185db9017c34aabc03ad99cf37bed > > > osstest/frozen/xen-4.4-testing 33fb8ff185846a7b4974105d2c9400690a6f95cf > > > osstest/frozen/xen-4.5-testing cda1cc170f07b45911b3dad03e42c8ebfc210fa1 > > > osstest/frozen/xen-4.6-testing eac167e2610d3e59b32f7ec7ba78cbc8c420a425 > > > osstest/frozen/xen-4.7-testing 1a41ed5af5e1704dd9b0bdca40df5c9cacbdabfc > > > > How did you pick those hashes ? Would it make more sense to pick the > > nearest libvirt release tag ? eg v1.3.2 instead of 33fb8ff18584 ? > > > > > These were those tested by the following `tolerable' osstest push gate > > > flights for the corresponding Xen tree: > > > > > > xen-4.3-testing 9a0c7f5f8341 86673 > > > xen-4.4-testing 33fb8ff18584 85031 > > > xen-4.5-testing cda1cc170f07 83135 > > > xen-4.6-testing eac167e2610d 96031 > > > xen-4.7-testing 1a41ed5af5e1 95728 > > I picked them by searching my mail archives for osstest `tolerable' > push gate flights - ie, passes in our CI system. > > That minimises the risk that the selected versions are themselves > troublesome for some reason, needing another round of adjustment. > > It might indeed be better to convert them to nearby release tags. > However: > > mariner:libvirt> git-describe 9a0c7f5f834185db9017c34aabc03ad99cf37bed > v1.3.2-202-g9a0c7f5 > mariner:libvirt> git-describe 33fb8ff185846a7b4974105d2c9400690a6f95cf > v1.3.2-rc2-1-g33fb8ff > mariner:libvirt> git-describe cda1cc170f07b45911b3dad03e42c8ebfc210fa1 > v1.3.1-262-gcda1cc1 > mariner:libvirt> git-describe eac167e2610d3e59b32f7ec7ba78cbc8c420a425 > v1.3.5-318-geac167e > mariner:libvirt> git-describe 1a41ed5af5e1704dd9b0bdca40df5c9cacbdabfc > v1.3.5-129-g1a41ed5 > mariner:libvirt> > > So in most cases these hashes are well away from a release tag. > > Does libvirt have stable release branches ? One approach would be to > have osstest track a suitable libvirt stable release branche for each > Xen stable release branch. Yep, there is a vN.N.N-maint branch for every release NB, with our new numbering that'll be changing nto vN.N-maint instead. > That would involve setting up a push gate for each of the chosen > libvirt stable branches. That would be worthwhile if we expect those > stable branches to acquire commits which break Xen, and which we could > like to be told about. But I'm not sure that's the case. Stuff goes onto the stable branches on an as-needed basis - mostly coming from the distro maintainers response to bug fixes from their users. There's probably not a whole lot that's touching xen on a regular basis and we're quite strict in what we accept for stable. Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH V2 03/10] arm/gic-v3: Fold GICR subtable parsing into a new function
On 27/06/16 17:17, Shanker Donthineni wrote: On 06/27/2016 11:09 AM, Julien Grall wrote: On 27/06/16 17:07, Shanker Donthineni wrote: On 06/27/2016 10:41 AM, Julien Grall wrote: On 27/06/16 16:40, Shanker Donthineni wrote: +gicv3.rdist_regions = rdist_regs; + +/* Parse always-on power domain Re-distributor entries */ +count = acpi_parse_entries(ACPI_SIG_MADT, + sizeof(struct acpi_table_madt), + gic_acpi_parse_madt_redistributor, table, + ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR, count); Please use acpi_table_parse_madt here. How do we pass MADT table pointer that we are using to a function acpi_table_parse_madt()? We have already obtained the MADT table address by calling acpi_get_table() at the beginning of the function gicv3_acpi_init(). You don't need to pass it. The function will take care to get the MADT table for you. Are you expecting me to modify current driver to replace acpi_parse_entries() calls with acpi_table_parse_madt() before my changes? No, I was just suggesting to modify the caller you changed/added within the different patch. Code becomes ugly for readability purpose using two set of functions in a single function. Feel free to modify the rest of the callers. Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [libvirt] Fixing libvirt's libxl driver breakage -- where to define LIBXL_API_VERSION?
On 06/27/2016 10:12 AM, Ian Jackson wrote: Daniel P. Berrange writes ("Re: [libvirt] [Xen-devel] Fixing libvirt's libxl driver breakage -- where to define LIBXL_API_VERSION?"): On Mon, Jun 27, 2016 at 04:54:35PM +0100, Ian Jackson wrote: Created the following branch refs on xenbits in the toplevel libvirt.git: osstest/frozen/xen-4.3-testing 9a0c7f5f834185db9017c34aabc03ad99cf37bed osstest/frozen/xen-4.4-testing 33fb8ff185846a7b4974105d2c9400690a6f95cf osstest/frozen/xen-4.5-testing cda1cc170f07b45911b3dad03e42c8ebfc210fa1 osstest/frozen/xen-4.6-testing eac167e2610d3e59b32f7ec7ba78cbc8c420a425 osstest/frozen/xen-4.7-testing 1a41ed5af5e1704dd9b0bdca40df5c9cacbdabfc How did you pick those hashes ? Would it make more sense to pick the nearest libvirt release tag ? eg v1.3.2 instead of 33fb8ff18584 ? These were those tested by the following `tolerable' osstest push gate flights for the corresponding Xen tree: xen-4.3-testing 9a0c7f5f8341 86673 xen-4.4-testing 33fb8ff18584 85031 xen-4.5-testing cda1cc170f07 83135 xen-4.6-testing eac167e2610d 96031 xen-4.7-testing 1a41ed5af5e1 95728 I picked them by searching my mail archives for osstest `tolerable' push gate flights - ie, passes in our CI system. That minimises the risk that the selected versions are themselves troublesome for some reason, needing another round of adjustment. It might indeed be better to convert them to nearby release tags. However: mariner:libvirt> git-describe 9a0c7f5f834185db9017c34aabc03ad99cf37bed v1.3.2-202-g9a0c7f5 mariner:libvirt> git-describe 33fb8ff185846a7b4974105d2c9400690a6f95cf v1.3.2-rc2-1-g33fb8ff mariner:libvirt> git-describe cda1cc170f07b45911b3dad03e42c8ebfc210fa1 v1.3.1-262-gcda1cc1 It seems odd that Xen 4.5 would use an older libvirt release than Xen 4.3. mariner:libvirt> git-describe eac167e2610d3e59b32f7ec7ba78cbc8c420a425 v1.3.5-318-geac167e mariner:libvirt> git-describe 1a41ed5af5e1704dd9b0bdca40df5c9cacbdabfc v1.3.5-129-g1a41ed5 mariner:libvirt> So in most cases these hashes are well away from a release tag. Does libvirt have stable release branches ? One approach would be to have osstest track a suitable libvirt stable release branche for each Xen stable release branch. I see Daniel already answered this question. That would involve setting up a push gate for each of the chosen libvirt stable branches. That would be worthwhile if we expect those stable branches to acquire commits which break Xen, and which we could like to be told about. But I'm not sure that's the case. I occasionally backport Xen bug fixes to -maint branches. Cole has also grabbed some Xen bug fixes when making a stable release of a -maint branch. But such backports should be trivial and obvious bug fixes that shouldn't cause build or runtime breakage with Xen. Regards, Jim ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [xen-4.3-testing test] 96304: regressions - FAIL
flight 96304 xen-4.3-testing real [real] http://logs.test-lab.xenproject.org/osstest/logs/96304/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-i386-libvirt5 libvirt-build fail REGR. vs. 87893 build-amd64-libvirt 5 libvirt-build fail REGR. vs. 87893 build-armhf 5 xen-build fail REGR. vs. 87893 Tests which are failing intermittently (not blocking): test-amd64-i386-xend-qemut-winxpsp3 9 windows-install fail pass in 96279 test-amd64-amd64-pv 17 guest-localmigrate/x10 fail pass in 96295 Regressions which are regarded as allowable (not blocking): test-amd64-amd64-xl-qemuu-win7-amd64 16 guest-stop fail like 87893 test-amd64-i386-xl-qemuu-win7-amd64 16 guest-stop fail like 87893 test-amd64-i386-xl-qemut-win7-amd64 16 guest-stop fail like 87893 Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt-vhd 1 build-check(1) blocked n/a test-armhf-armhf-libvirt-raw 1 build-check(1) blocked n/a test-amd64-amd64-libvirt 1 build-check(1) blocked n/a test-armhf-armhf-xl-credit2 1 build-check(1) blocked n/a test-armhf-armhf-xl-arndale 1 build-check(1) blocked n/a test-armhf-armhf-xl-multivcpu 1 build-check(1) blocked n/a test-armhf-armhf-xl-cubietruck 1 build-check(1) blocked n/a test-armhf-armhf-libvirt-qcow2 1 build-check(1) blocked n/a test-armhf-armhf-xl-vhd 1 build-check(1) blocked n/a test-armhf-armhf-libvirt 1 build-check(1) blocked n/a test-armhf-armhf-xl 1 build-check(1) blocked n/a test-amd64-i386-libvirt 1 build-check(1) blocked n/a test-amd64-i386-rumpuserxen-i386 1 build-check(1) blocked n/a test-amd64-amd64-rumpuserxen-amd64 1 build-check(1) blocked n/a build-armhf-libvirt 1 build-check(1) blocked n/a test-amd64-i386-xend-qemut-winxpsp3 20 leak-check/check fail in 96279 never pass test-amd64-i386-xl-qemuu-ovmf-amd64 9 debian-hvm-install fail never pass test-amd64-amd64-xl-qemuu-ovmf-amd64 9 debian-hvm-install fail never pass build-amd64-rumpuserxen 6 xen-buildfail never pass build-i386-rumpuserxen6 xen-buildfail never pass test-amd64-amd64-xl-qemut-win7-amd64 16 guest-stop fail never pass version targeted for testing: xen 0a8c94fae993dd8f2b27fd4cc694f61c21de84bf baseline version: xen 8fa31952e2d08ef63897c43b5e8b33475ebf5d93 Last test of basis87893 2016-03-29 13:49:52 Z 90 days Failing since 92180 2016-04-20 17:49:21 Z 67 days 35 attempts Testing same since96017 2016-06-20 17:22:27 Z6 days 14 attempts People who touched revisions under test: Andrew Cooper Anthony Liguori Anthony PERARD Gerd Hoffmann Ian Jackson Jan Beulich Jim Paris Stefan Hajnoczi Tim Deegan Wei Liu jobs: build-amd64 pass build-armhf fail build-i386 pass build-amd64-libvirt fail build-armhf-libvirt blocked build-i386-libvirt fail build-amd64-pvopspass build-armhf-pvopspass build-i386-pvops pass build-amd64-rumpuserxen fail build-i386-rumpuserxen fail test-amd64-amd64-xl pass test-armhf-armhf-xl blocked test-amd64-i386-xl pass test-amd64-i386-qemut-rhel6hvm-amd pass test-amd64-i386-qemuu-rhel6hvm-amd pass test-amd64-amd64-xl-qemut-debianhvm-amd64pass test-amd64-i386-xl-qemut-debianhvm-amd64 pass test-amd64-amd64-xl-qemuu-debianhvm-amd64pass test-amd64-i386-xl-qemuu-debianhvm-amd64 pass test-amd64-i386-freebsd10-amd64 pass test-amd64-amd64-xl-qemuu-ovmf-amd64 fail test-amd64-i386-xl-qemuu-ovmf-amd64 fail test-amd64-amd64-rumpuserxen-amd64 blocked test-am
[Xen-devel] [ovmf test] 96298: regressions - FAIL
flight 96298 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/96298/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-amd64-i386-xl-qemuu-ovmf-amd64 17 guest-start/debianhvm.repeat fail REGR. vs. 94748 test-amd64-amd64-xl-qemuu-ovmf-amd64 17 guest-start/debianhvm.repeat fail REGR. vs. 94748 version targeted for testing: ovmf fda7cd4f25f831f93b71c3264143618b3cebfb33 baseline version: ovmf dc99315b8732b6e3032d01319d3f534d440b43d0 Last test of basis94748 2016-05-24 22:43:25 Z 33 days Failing since 94750 2016-05-25 03:43:08 Z 33 days 59 attempts Testing same since96298 2016-06-27 03:32:35 Z0 days1 attempts People who touched revisions under test: Ard Biesheuvel Chao Zhang Cinnamon Shia Cohen, Eugene Dandan Bi Darbin Reyes Eric Dong Eugene Cohen Evan Lloyd Fu Siyuan Fu, Siyuan Gary Li Gary Lin Giri P Mudusuru Hao Wu Hegde Nagaraj P hegdenag Heyi Guo Jan D?bro? Jan Dabros Jeff Fan Jiaxin Wu Jiewen Yao Joe Zhou Katie Dellaquila Laszlo Ersek Liming Gao Lu, ShifeiX A lushifex Marcin Wojtas Marvin H?user Marvin Haeuser Maurice Ma Michael Zimmermann Qiu Shumin Ruiyu Ni Ryan Harkin Sami Mujawar Satya Yarlagadda Sriram Subramanian Star Zeng Sunny Wang Tapan Shah Thomas Palmer Yonghong Zhu Zhang Lubo Zhang, Chao B jobs: build-amd64-xsm pass build-i386-xsm pass build-amd64 pass build-i386 pass build-amd64-libvirt pass build-i386-libvirt pass build-amd64-pvopspass build-i386-pvops pass test-amd64-amd64-xl-qemuu-ovmf-amd64 fail test-amd64-i386-xl-qemuu-ovmf-amd64 fail sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Not pushing. (No revision log; it would be 3051 lines long.) ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [PATCH v4 03/16] xen/arm: Rename grant_table_gfpn into grant_table_gfn and use the typesafe gfn
The correct acronym for a guest physical frame is gfn. Also use the typesafe gfn to ensure that a guest frame is effectively used. Signed-off-by: Julien Grall Acked-by: Stefano Stabellini --- Changes in v4: - Add Stefano's acked-by Changes in v2: - Remove extra pair of brackets. --- xen/arch/arm/domain.c | 4 ++-- xen/arch/arm/mm.c | 2 +- xen/include/asm-arm/domain.h | 2 +- xen/include/asm-arm/grant_table.h | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index d8a804c..6ce4645 100644 --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -464,13 +464,13 @@ struct domain *alloc_domain_struct(void) return NULL; clear_page(d); -d->arch.grant_table_gpfn = xzalloc_array(xen_pfn_t, max_grant_frames); +d->arch.grant_table_gfn = xzalloc_array(gfn_t, max_grant_frames); return d; } void free_domain_struct(struct domain *d) { -xfree(d->arch.grant_table_gpfn); +xfree(d->arch.grant_table_gfn); free_xenheap_page(d); } diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c index 6882d54..0e408f8 100644 --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -1082,7 +1082,7 @@ int xenmem_add_to_physmap_one( return -EINVAL; } -d->arch.grant_table_gpfn[idx] = gfn_x(gfn); +d->arch.grant_table_gfn[idx] = gfn; t = p2m_ram_rw; diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h index 370cdeb..979f7de 100644 --- a/xen/include/asm-arm/domain.h +++ b/xen/include/asm-arm/domain.h @@ -51,7 +51,7 @@ struct arch_domain uint64_t vttbr; struct hvm_domain hvm_domain; -xen_pfn_t *grant_table_gpfn; +gfn_t *grant_table_gfn; struct vmmio vmmio; diff --git a/xen/include/asm-arm/grant_table.h b/xen/include/asm-arm/grant_table.h index 5e076cc..eb02423 100644 --- a/xen/include/asm-arm/grant_table.h +++ b/xen/include/asm-arm/grant_table.h @@ -30,7 +30,7 @@ static inline int replace_grant_supported(void) #define gnttab_shared_gmfn(d, t, i) \ ( ((i >= nr_grant_frames(d->grant_table)) && \ - (i < max_grant_frames)) ? 0 : (d->arch.grant_table_gpfn[i])) + (i < max_grant_frames)) ? 0 : gfn_x(d->arch.grant_table_gfn[i])) #define gnttab_need_iommu_mapping(d)\ (is_domain_direct_mapped(d) && need_iommu(d)) -- 1.9.1 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [PATCH v4 05/16] xen/mm: Introduce INVALID_GFN_T and INVALID_MFN_T
The two new defines will be a typesafe version of resp. INVALID_GFN and INVALID_MFN. Signed-off-by: Julien Grall --- Cc: Andrew Cooper Cc: George Dunlap Cc: Ian Jackson Cc: Jan Beulich Cc: Konrad Rzeszutek Wilk Cc: Stefano Stabellini Cc: Tim Deegan Cc: Wei Liu Changes in v4: - Patch added --- xen/include/xen/mm.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h index afbb1a1..978a62e 100644 --- a/xen/include/xen/mm.h +++ b/xen/include/xen/mm.h @@ -56,6 +56,7 @@ TYPE_SAFE(unsigned long, mfn); #define PRI_mfn "05lx" #define INVALID_MFN (~0UL) +#define INVALID_MFN_T_mfn(INVALID_MFN) #ifndef mfn_t #define mfn_t /* Grep fodder: mfn_t, _mfn() and mfn_x() are defined above */ @@ -85,6 +86,7 @@ static inline bool_t mfn_eq(mfn_t x, mfn_t y) TYPE_SAFE(unsigned long, gfn); #define PRI_gfn "05lx" #define INVALID_GFN (~0UL) +#define INVALID_GFN_T_gfn(INVALID_GFN) #ifndef gfn_t #define gfn_t /* Grep fodder: gfn_t, _gfn() and gfn_x() are defined above */ -- 1.9.1 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [PATCH v4 01/16] xen: Use typesafe gfn/mfn in guest_physmap_* helpers
Also rename some variables to gfn or mfn when it does not require much rework. Finally replace %hu with %d when printing the domain id in guest_physmap_add_entry (arch/x86/mm/p2m.c). Signed-off-by: Julien Grall Acked-by: Jan Beulich Acked-by: Stefano Stabellini --- Cc: Stefano Stabellini Cc: Jan Beulich Cc: Andrew Cooper Cc: Paul Durrant Cc: George Dunlap Cc: Ian Jackson Cc: Konrad Rzeszutek Wilk Cc: Tim Deegan Cc: Wei Liu Changes in v4: - Add Stefano's Acked-by Changes in v3: - Use %d to print the domain id rather than %hu - Add Jan's Acked-by for non-ARM bits Changes in v2: - Don't use a wrapper for x86. Instead use mfn_* to make the change simpler. --- xen/arch/arm/domain_build.c| 2 +- xen/arch/arm/mm.c | 10 ++--- xen/arch/arm/p2m.c | 20 +- xen/arch/x86/domain.c | 5 ++- xen/arch/x86/domain_build.c| 6 +-- xen/arch/x86/hvm/ioreq.c | 8 ++-- xen/arch/x86/mm.c | 12 +++--- xen/arch/x86/mm/p2m.c | 78 -- xen/common/grant_table.c | 7 ++-- xen/common/memory.c| 32 xen/drivers/passthrough/arm/smmu.c | 4 +- xen/include/asm-arm/p2m.h | 12 +++--- xen/include/asm-x86/p2m.h | 11 +++--- xen/include/xen/mm.h | 2 +- 14 files changed, 110 insertions(+), 99 deletions(-) diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 410bb4f..9035486 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -117,7 +117,7 @@ static bool_t insert_11_bank(struct domain *d, goto fail; } -res = guest_physmap_add_page(d, spfn, spfn, order); +res = guest_physmap_add_page(d, _gfn(spfn), _mfn(spfn), order); if ( res ) panic("Failed map pages to DOM0: %d", res); diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c index 2ec211b..5ab9b75 100644 --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -1153,7 +1153,7 @@ int xenmem_add_to_physmap_one( } /* Map at new location. */ -rc = guest_physmap_add_entry(d, gpfn, mfn, 0, t); +rc = guest_physmap_add_entry(d, _gfn(gpfn), _mfn(mfn), 0, t); /* If we fail to add the mapping, we need to drop the reference we * took earlier on foreign pages */ @@ -1282,8 +1282,8 @@ int create_grant_host_mapping(unsigned long addr, unsigned long frame, if ( flags & GNTMAP_readonly ) t = p2m_grant_map_ro; -rc = guest_physmap_add_entry(current->domain, addr >> PAGE_SHIFT, - frame, 0, t); +rc = guest_physmap_add_entry(current->domain, _gfn(addr >> PAGE_SHIFT), + _mfn(frame), 0, t); if ( rc ) return GNTST_general_error; @@ -1294,13 +1294,13 @@ int create_grant_host_mapping(unsigned long addr, unsigned long frame, int replace_grant_host_mapping(unsigned long addr, unsigned long mfn, unsigned long new_addr, unsigned int flags) { -unsigned long gfn = (unsigned long)(addr >> PAGE_SHIFT); +gfn_t gfn = _gfn(addr >> PAGE_SHIFT); struct domain *d = current->domain; if ( new_addr != 0 || (flags & GNTMAP_contains_pte) ) return GNTST_general_error; -guest_physmap_remove_page(d, gfn, mfn, 0); +guest_physmap_remove_page(d, gfn, _mfn(mfn), 0); return GNTST_okay; } diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index 5afae1d..0395a40 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -1292,26 +1292,26 @@ int map_dev_mmio_region(struct domain *d, } int guest_physmap_add_entry(struct domain *d, -unsigned long gpfn, -unsigned long mfn, +gfn_t gfn, +mfn_t mfn, unsigned long page_order, p2m_type_t t) { return apply_p2m_changes(d, INSERT, - pfn_to_paddr(gpfn), - pfn_to_paddr(gpfn + (1 << page_order)), - pfn_to_paddr(mfn), MATTR_MEM, 0, t, + pfn_to_paddr(gfn_x(gfn)), + pfn_to_paddr(gfn_x(gfn) + (1 << page_order)), + pfn_to_paddr(mfn_x(mfn)), MATTR_MEM, 0, t, d->arch.p2m.default_access); } void guest_physmap_remove_page(struct domain *d, - unsigned long gpfn, - unsigned long mfn, unsigned int page_order) + gfn_t gfn, + mfn_t mfn, unsigned int page_order) { apply_p2m_changes(d, REMOVE, - pfn_to_paddr(gpfn), - pfn_to_paddr(gpfn + (1
[Xen-devel] [PATCH v4 04/16] xen: Use the typesafe mfn and gfn in map_mmio_regions...
to avoid mixing machine frame with guest frame. Signed-off-by: Julien Grall Acked-by: Jan Beulich --- Cc: Stefano Stabellini Cc: Jan Beulich Cc: Andrew Cooper Cc: George Dunlap Cc: Ian Jackson Cc: Konrad Rzeszutek Wilk Cc: Tim Deegan Cc: Wei Liu Changes in v3: - Use mfn_add when it is possible - Add Jan's acked-by --- xen/arch/arm/domain_build.c | 4 ++-- xen/arch/arm/gic-v2.c| 4 ++-- xen/arch/arm/p2m.c | 22 +++--- xen/arch/arm/platforms/exynos5.c | 8 xen/arch/arm/platforms/omap5.c | 16 xen/arch/arm/vgic-v2.c | 4 ++-- xen/arch/x86/mm/p2m.c| 18 ++ xen/common/domctl.c | 4 ++-- xen/include/xen/p2m-common.h | 8 9 files changed, 45 insertions(+), 43 deletions(-) diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 9035486..49185f0 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -1036,9 +1036,9 @@ static int map_range_to_domain(const struct dt_device_node *dev, if ( need_mapping ) { res = map_mmio_regions(d, - paddr_to_pfn(addr), + _gfn(paddr_to_pfn(addr)), DIV_ROUND_UP(len, PAGE_SIZE), - paddr_to_pfn(addr)); + _mfn(paddr_to_pfn(addr))); if ( res < 0 ) { printk(XENLOG_ERR "Unable to map 0x%"PRIx64 diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c index 4e2f4c7..3893ece 100644 --- a/xen/arch/arm/gic-v2.c +++ b/xen/arch/arm/gic-v2.c @@ -601,9 +601,9 @@ static int gicv2_map_hwdown_extra_mappings(struct domain *d) d->domain_id, v2m_data->addr, v2m_data->size, v2m_data->spi_start, v2m_data->nr_spis); -ret = map_mmio_regions(d, paddr_to_pfn(v2m_data->addr), +ret = map_mmio_regions(d, _gfn(paddr_to_pfn(v2m_data->addr)), DIV_ROUND_UP(v2m_data->size, PAGE_SIZE), -paddr_to_pfn(v2m_data->addr)); +_mfn(paddr_to_pfn(v2m_data->addr))); if ( ret ) { printk(XENLOG_ERR "GICv2: Map v2m frame to d%d failed.\n", diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index 0395a40..34563bb 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -1245,27 +1245,27 @@ int unmap_regions_rw_cache(struct domain *d, } int map_mmio_regions(struct domain *d, - unsigned long start_gfn, + gfn_t start_gfn, unsigned long nr, - unsigned long mfn) + mfn_t mfn) { return apply_p2m_changes(d, INSERT, - pfn_to_paddr(start_gfn), - pfn_to_paddr(start_gfn + nr), - pfn_to_paddr(mfn), + pfn_to_paddr(gfn_x(start_gfn)), + pfn_to_paddr(gfn_x(start_gfn) + nr), + pfn_to_paddr(mfn_x(mfn)), MATTR_DEV, 0, p2m_mmio_direct, d->arch.p2m.default_access); } int unmap_mmio_regions(struct domain *d, - unsigned long start_gfn, + gfn_t start_gfn, unsigned long nr, - unsigned long mfn) + mfn_t mfn) { return apply_p2m_changes(d, REMOVE, - pfn_to_paddr(start_gfn), - pfn_to_paddr(start_gfn + nr), - pfn_to_paddr(mfn), + pfn_to_paddr(gfn_x(start_gfn)), + pfn_to_paddr(gfn_x(start_gfn) + nr), + pfn_to_paddr(mfn_x(mfn)), MATTR_DEV, 0, p2m_invalid, d->arch.p2m.default_access); } @@ -1280,7 +1280,7 @@ int map_dev_mmio_region(struct domain *d, if ( !(nr && iomem_access_permitted(d, mfn, mfn + nr - 1)) ) return 0; -res = map_mmio_regions(d, start_gfn, nr, mfn); +res = map_mmio_regions(d, _gfn(start_gfn), nr, _mfn(mfn)); if ( res < 0 ) { printk(XENLOG_G_ERR "Unable to map [%#lx - %#lx] in Dom%d\n", diff --git a/xen/arch/arm/platforms/exynos5.c b/xen/arch/arm/platforms/exynos5.c index bf4964d..c43934f 100644 --- a/xen/arch/arm/platforms/exynos5.c +++ b/xen/arch/arm/platforms/exynos5.c @@ -83,12 +83,12 @@ static int exynos5_init_time(void) static int exynos5250_specific_mapping(struct domain *d) { /* Map the chip ID */ -map_mmio_regions(d, paddr_to_pfn(EXYNOS5_PA_CHIPID), 1, - paddr_to_pfn(EXYNOS5_PA_CHIPID)); +map_mmio_regions(d, _gfn(paddr_to_pfn(EXYNOS5_PA_CHIPID)), 1, + _mfn(paddr_to_pfn(EXYNOS5_PA_CH
[Xen-devel] [PATCH v4 06/16] xen/arm: Rework the interface of p2m_lookup and use typesafe gfn and mfn
The prototype and the declaration of p2m_lookup disagree on how the function should be used. One expect a frame number whilst the other an address. Thankfully, everyone is using with an address today. However, most of the callers have to convert a guest frame to an address. Modify the interface to take a guest physical frame in parameter and return a machine frame. Whilst modifying the interface, use typesafe gfn and mfn for clarity and catching possible misusage. Signed-off-by: Julien Grall --- Changes in v4: - Use INVALID_MFN_T when possible --- xen/arch/arm/p2m.c| 43 +++ xen/arch/arm/traps.c | 21 +++-- xen/include/asm-arm/p2m.h | 7 +++ 3 files changed, 37 insertions(+), 34 deletions(-) diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index 34563bb..65818dd 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -140,14 +140,15 @@ void flush_tlb_domain(struct domain *d) } /* - * Lookup the MFN corresponding to a domain's PFN. + * Lookup the MFN corresponding to a domain's GFN. * * There are no processor functions to do a stage 2 only lookup therefore we * do a a software walk. */ -static paddr_t __p2m_lookup(struct domain *d, paddr_t paddr, p2m_type_t *t) +static mfn_t __p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t) { struct p2m_domain *p2m = &d->arch.p2m; +const paddr_t paddr = pfn_to_paddr(gfn_x(gfn)); const unsigned int offsets[4] = { zeroeth_table_offset(paddr), first_table_offset(paddr), @@ -158,7 +159,7 @@ static paddr_t __p2m_lookup(struct domain *d, paddr_t paddr, p2m_type_t *t) ZEROETH_MASK, FIRST_MASK, SECOND_MASK, THIRD_MASK }; lpae_t pte, *map; -paddr_t maddr = INVALID_PADDR; +mfn_t mfn = INVALID_MFN_T; paddr_t mask = 0; p2m_type_t _t; unsigned int level, root_table; @@ -216,21 +217,22 @@ static paddr_t __p2m_lookup(struct domain *d, paddr_t paddr, p2m_type_t *t) { ASSERT(mask); ASSERT(pte.p2m.type != p2m_invalid); -maddr = (pte.bits & PADDR_MASK & mask) | (paddr & ~mask); +mfn = _mfn(paddr_to_pfn((pte.bits & PADDR_MASK & mask) | +(paddr & ~mask))); *t = pte.p2m.type; } err: -return maddr; +return mfn; } -paddr_t p2m_lookup(struct domain *d, paddr_t paddr, p2m_type_t *t) +mfn_t p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t) { -paddr_t ret; +mfn_t ret; struct p2m_domain *p2m = &d->arch.p2m; spin_lock(&p2m->lock); -ret = __p2m_lookup(d, paddr, t); +ret = __p2m_lookup(d, gfn, t); spin_unlock(&p2m->lock); return ret; @@ -493,8 +495,9 @@ static int __p2m_get_mem_access(struct domain *d, gfn_t gfn, * No setting was found in the Radix tree. Check if the * entry exists in the page-tables. */ -paddr_t maddr = __p2m_lookup(d, gfn_x(gfn) << PAGE_SHIFT, NULL); -if ( INVALID_PADDR == maddr ) +mfn_t mfn = __p2m_lookup(d, gfn, NULL); + +if ( mfn_eq(mfn, INVALID_MFN_T) ) return -ESRCH; /* If entry exists then its rwx. */ @@ -1483,8 +1486,7 @@ int p2m_cache_flush(struct domain *d, xen_pfn_t start_mfn, xen_pfn_t end_mfn) mfn_t gfn_to_mfn(struct domain *d, gfn_t gfn) { -paddr_t p = p2m_lookup(d, pfn_to_paddr(gfn_x(gfn)), NULL); -return _mfn(p >> PAGE_SHIFT); +return p2m_lookup(d, gfn, NULL); } /* @@ -1498,8 +1500,8 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned long flag) { long rc; paddr_t ipa; -unsigned long maddr; -unsigned long mfn; +gfn_t gfn; +mfn_t mfn; xenmem_access_t xma; p2m_type_t t; struct page_info *page = NULL; @@ -1508,11 +1510,13 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned long flag) if ( rc < 0 ) goto err; +gfn = _gfn(paddr_to_pfn(ipa)); + /* * We do this first as this is faster in the default case when no * permission is set on the page. */ -rc = __p2m_get_mem_access(current->domain, _gfn(paddr_to_pfn(ipa)), &xma); +rc = __p2m_get_mem_access(current->domain, gfn, &xma); if ( rc < 0 ) goto err; @@ -1561,12 +1565,11 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned long flag) * We had a mem_access permission limiting the access, but the page type * could also be limiting, so we need to check that as well. */ -maddr = __p2m_lookup(current->domain, ipa, &t); -if ( maddr == INVALID_PADDR ) +mfn = __p2m_lookup(current->domain, gfn, &t); +if ( mfn_eq(mfn, INVALID_MFN_T) ) goto err; -mfn = maddr >> PAGE_SHIFT; -if ( !mfn_valid(mfn) ) +if ( !mfn_valid(mfn_x(mfn)) ) goto err; /* @@ -1575,7 +1578,7 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned long flag) if ( t != p2m_ram_rw ) goto err; -page = mfn_to_page
[Xen-devel] [PATCH v4 00/16] xen/arm: Use the typesafes gfn and mfn
Hello all, Some of the ARM functions are mixing gfn vs mfn and even physical vs frame. To avoid more confusion, this patch series makes use of the terminology described in xen/include/xen/mm.h and the associated typesafe. This series requires the patch [1] to be applied beforehand. I pushed a branch with this patch and this series applied on xenbits: git://xenbits.xen.org/people/julieng/xen-unstable.git branch typesafe-v4 The patches #5 and #8-#18 are brand new, after Stefano requested to push the gfn/mfn typesafe down to apply_p2m_changes for ARM. It requires some cleanup/fixes before been able to do the actual rework. For all the changes see in each patch. Yours sincerely, [1] http://lists.xenproject.org/archives/html/xen-devel/2016-06/msg01744.html Cc: Andrew Cooper Cc: George Dunlap Cc: Ian Jackson Cc: Jan Beulich Cc: Jun Nakajima Cc: Kevin Tian Cc: Konrad Rzeszutek Wilk Cc: Paul Durrant Cc: Stefano Stabellini Cc: Tim Deegan Cc: Wei Liu Julien Grall (16): xen: Use typesafe gfn/mfn in guest_physmap_* helpers xen: Use typesafe gfn in xenmem_add_to_physmap_one xen/arm: Rename grant_table_gfpn into grant_table_gfn and use the typesafe gfn xen: Use the typesafe mfn and gfn in map_mmio_regions... xen/mm: Introduce INVALID_GFN_T and INVALID_MFN_T xen/arm: Rework the interface of p2m_lookup and use typesafe gfn and mfn xen/arm: Rework the interface of p2m_cache_flush and use typesafe gfn xen: Replace _mfn(INVALID_MFN) with MFN_INVALID_T xen/arm: map_regions_rw_cache: Map the region with p2m->default_access xen/arm: dom0_build: Remove dead code in allocate_memory xen/arm: p2m: Remove unused operation ALLOCATE xen/arm: Use the typesafes mfn and gfn in map_dev_mmio_region... xen/arm: Use the typesafes mfn and gfn in map_regions_rw_cache ... xen/arm: p2m: Introduce helpers to insert and remove mapping xen/arm: p2m: Use typesafe gfn for {max,lowest}_mapped_gfn xen/arm: p2m: Rework the interface of apply_p2m_changes and use typesafe xen/arch/arm/domain.c | 4 +- xen/arch/arm/domain_build.c| 72 ++ xen/arch/arm/domctl.c | 2 +- xen/arch/arm/gic-v2.c | 4 +- xen/arch/arm/mm.c | 20 +-- xen/arch/arm/p2m.c | 265 ++--- xen/arch/arm/platforms/exynos5.c | 8 +- xen/arch/arm/platforms/omap5.c | 16 +-- xen/arch/arm/traps.c | 21 +-- xen/arch/arm/vgic-v2.c | 4 +- xen/arch/x86/domain.c | 5 +- xen/arch/x86/domain_build.c| 6 +- xen/arch/x86/hvm/ioreq.c | 8 +- xen/arch/x86/mm.c | 21 +-- xen/arch/x86/mm/guest_walk.c | 4 +- xen/arch/x86/mm/hap/hap.c | 2 +- xen/arch/x86/mm/p2m-ept.c | 2 +- xen/arch/x86/mm/p2m-pod.c | 18 +-- xen/arch/x86/mm/p2m-pt.c | 16 +-- xen/arch/x86/mm/p2m.c | 108 --- xen/arch/x86/mm/paging.c | 12 +- xen/arch/x86/mm/shadow/common.c| 32 ++--- xen/arch/x86/mm/shadow/multi.c | 32 ++--- xen/common/domctl.c| 4 +- xen/common/grant_table.c | 7 +- xen/common/memory.c| 38 +++--- xen/drivers/passthrough/arm/smmu.c | 4 +- xen/include/asm-arm/domain.h | 2 +- xen/include/asm-arm/grant_table.h | 2 +- xen/include/asm-arm/p2m.h | 44 +++--- xen/include/asm-x86/p2m.h | 11 +- xen/include/xen/mm.h | 6 +- xen/include/xen/p2m-common.h | 8 +- 33 files changed, 356 insertions(+), 452 deletions(-) -- 1.9.1 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [PATCH v4 08/16] xen: Replace _mfn(INVALID_MFN) with MFN_INVALID_T
This patch is a mechanical replacement. Command used: 42sh> ack -l "_mfn\(INVALID_MFN\)" | xargs sed -i -e 's/_mfn(INVALID_MFN)/INVALID_MFN_T/g' Signed-off-by: Julien Grall --- Cc: George Dunlap Cc: Jan Beulich Cc: Andrew Cooper Cc: Jun Nakajima Cc: Kevin Tian Cc: Tim Deegan Changes in v4: - Patch added --- xen/arch/x86/mm/guest_walk.c| 4 ++-- xen/arch/x86/mm/hap/hap.c | 2 +- xen/arch/x86/mm/p2m-ept.c | 2 +- xen/arch/x86/mm/p2m-pod.c | 18 +- xen/arch/x86/mm/p2m-pt.c| 16 xen/arch/x86/mm/p2m.c | 14 +++--- xen/arch/x86/mm/paging.c| 12 ++-- xen/arch/x86/mm/shadow/common.c | 32 xen/arch/x86/mm/shadow/multi.c | 32 9 files changed, 66 insertions(+), 66 deletions(-) diff --git a/xen/arch/x86/mm/guest_walk.c b/xen/arch/x86/mm/guest_walk.c index e850502..12b57d8 100644 --- a/xen/arch/x86/mm/guest_walk.c +++ b/xen/arch/x86/mm/guest_walk.c @@ -281,7 +281,7 @@ guest_walk_tables(struct vcpu *v, struct p2m_domain *p2m, start = _gfn((gfn_x(start) & ~GUEST_L3_GFN_MASK) + ((va >> PAGE_SHIFT) & GUEST_L3_GFN_MASK)); gw->l1e = guest_l1e_from_gfn(start, flags); -gw->l2mfn = gw->l1mfn = _mfn(INVALID_MFN); +gw->l2mfn = gw->l1mfn = INVALID_MFN_T; goto set_ad; } @@ -356,7 +356,7 @@ guest_walk_tables(struct vcpu *v, struct p2m_domain *p2m, start = _gfn((gfn_x(start) & ~GUEST_L2_GFN_MASK) + guest_l1_table_offset(va)); gw->l1e = guest_l1e_from_gfn(start, flags); -gw->l1mfn = _mfn(INVALID_MFN); +gw->l1mfn = INVALID_MFN_T; } else { diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c index 9c2cd49..cd18c73 100644 --- a/xen/arch/x86/mm/hap/hap.c +++ b/xen/arch/x86/mm/hap/hap.c @@ -430,7 +430,7 @@ static mfn_t hap_make_monitor_table(struct vcpu *v) oom: HAP_ERROR("out of memory building monitor pagetable\n"); domain_crash(d); -return _mfn(INVALID_MFN); +return INVALID_MFN_T; } static void hap_destroy_monitor_table(struct vcpu* v, mfn_t mmfn) diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c index 7166c71..25233f2 100644 --- a/xen/arch/x86/mm/p2m-ept.c +++ b/xen/arch/x86/mm/p2m-ept.c @@ -887,7 +887,7 @@ static mfn_t ept_get_entry(struct p2m_domain *p2m, int i; int ret = 0; bool_t recalc = 0; -mfn_t mfn = _mfn(INVALID_MFN); +mfn_t mfn = INVALID_MFN_T; struct ept_data *ept = &p2m->ept; *t = p2m_mmio_dm; diff --git a/xen/arch/x86/mm/p2m-pod.c b/xen/arch/x86/mm/p2m-pod.c index b7ab169..3cf905b 100644 --- a/xen/arch/x86/mm/p2m-pod.c +++ b/xen/arch/x86/mm/p2m-pod.c @@ -559,7 +559,7 @@ p2m_pod_decrease_reservation(struct domain *d, { /* All PoD: Mark the whole region invalid and tell caller * we're done. */ -p2m_set_entry(p2m, gpfn, _mfn(INVALID_MFN), order, p2m_invalid, +p2m_set_entry(p2m, gpfn, INVALID_MFN_T, order, p2m_invalid, p2m->default_access); p2m->pod.entry_count-=(1default_access); p2m->pod.entry_count -= n; BUG_ON(p2m->pod.entry_count < 0); @@ -624,7 +624,7 @@ p2m_pod_decrease_reservation(struct domain *d, page = mfn_to_page(mfn); -p2m_set_entry(p2m, gpfn + i, _mfn(INVALID_MFN), cur_order, +p2m_set_entry(p2m, gpfn + i, INVALID_MFN_T, cur_order, p2m_invalid, p2m->default_access); p2m_tlb_flush_sync(p2m); for ( j = 0; j < n; ++j ) @@ -671,7 +671,7 @@ void p2m_pod_dump_data(struct domain *d) static int p2m_pod_zero_check_superpage(struct p2m_domain *p2m, unsigned long gfn) { -mfn_t mfn, mfn0 = _mfn(INVALID_MFN); +mfn_t mfn, mfn0 = INVALID_MFN_T; p2m_type_t type, type0 = 0; unsigned long * map = NULL; int ret=0, reset = 0; @@ -754,7 +754,7 @@ p2m_pod_zero_check_superpage(struct p2m_domain *p2m, unsigned long gfn) } /* Try to remove the page, restoring old mapping if it fails. */ -p2m_set_entry(p2m, gfn, _mfn(INVALID_MFN), PAGE_ORDER_2M, +p2m_set_entry(p2m, gfn, INVALID_MFN_T, PAGE_ORDER_2M, p2m_populate_on_demand, p2m->default_access); p2m_tlb_flush_sync(p2m); @@ -871,7 +871,7 @@ p2m_pod_zero_check(struct p2m_domain *p2m, unsigned long *gfns, int count) } /* Try to remove the page, restoring old mapping if it fails. */ -p2m_set_entry(p2m, gfns[i], _mfn(INVALID_MFN),
[Xen-devel] [PATCH v4 13/16] xen/arm: Use the typesafes mfn and gfn in map_regions_rw_cache ...
to avoid mixing machine frame with guest frame. Also rename the parameters of the function and drop pointless PAGE_MASK in the caller. Signed-off-by: Julien Grall --- Changes in v4: - Patch added --- xen/arch/arm/domain_build.c | 8 xen/arch/arm/p2m.c | 20 ++-- xen/include/asm-arm/p2m.h | 12 ++-- 3 files changed, 20 insertions(+), 20 deletions(-) diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 923f48a..60db9e4 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -1522,9 +1522,9 @@ static void acpi_map_other_tables(struct domain *d) addr = acpi_gbl_root_table_list.tables[i].address; size = acpi_gbl_root_table_list.tables[i].length; res = map_regions_rw_cache(d, - paddr_to_pfn(addr & PAGE_MASK), + _gfn(paddr_to_pfn(addr)), DIV_ROUND_UP(size, PAGE_SIZE), - paddr_to_pfn(addr & PAGE_MASK)); + _mfn(paddr_to_pfn(addr))); if ( res ) { panic(XENLOG_ERR "Unable to map ACPI region 0x%"PRIx64 @@ -1878,9 +1878,9 @@ static int prepare_acpi(struct domain *d, struct kernel_info *kinfo) /* Map the EFI and ACPI tables to Dom0 */ rc = map_regions_rw_cache(d, - paddr_to_pfn(d->arch.efi_acpi_gpa), + _gfn(paddr_to_pfn(d->arch.efi_acpi_gpa)), PFN_UP(d->arch.efi_acpi_len), - paddr_to_pfn(virt_to_maddr(d->arch.efi_acpi_table))); + _mfn(paddr_to_pfn(virt_to_maddr(d->arch.efi_acpi_table; if ( rc != 0 ) { printk(XENLOG_ERR "Unable to map EFI/ACPI table 0x%"PRIx64 diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index faadebf..36ee0fe 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -1159,27 +1159,27 @@ out: } int map_regions_rw_cache(struct domain *d, - unsigned long start_gfn, + gfn_t gfn, unsigned long nr, - unsigned long mfn) + mfn_t mfn) { return apply_p2m_changes(d, INSERT, - pfn_to_paddr(start_gfn), - pfn_to_paddr(start_gfn + nr), - pfn_to_paddr(mfn), + pfn_to_paddr(gfn_x(gfn)), + pfn_to_paddr(gfn_x(gfn) + nr), + pfn_to_paddr(mfn_x(mfn)), MATTR_MEM, 0, p2m_mmio_direct, d->arch.p2m.default_access); } int unmap_regions_rw_cache(struct domain *d, - unsigned long start_gfn, + gfn_t gfn, unsigned long nr, - unsigned long mfn) + mfn_t mfn) { return apply_p2m_changes(d, REMOVE, - pfn_to_paddr(start_gfn), - pfn_to_paddr(start_gfn + nr), - pfn_to_paddr(mfn), + pfn_to_paddr(gfn_x(gfn)), + pfn_to_paddr(gfn_x(gfn) + nr), + pfn_to_paddr(mfn_x(mfn)), MATTR_MEM, 0, p2m_invalid, d->arch.p2m.default_access); } diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h index 8d29eda..6e258b9 100644 --- a/xen/include/asm-arm/p2m.h +++ b/xen/include/asm-arm/p2m.h @@ -142,14 +142,14 @@ mfn_t p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t); int p2m_cache_flush(struct domain *d, gfn_t start, unsigned long nr); int map_regions_rw_cache(struct domain *d, - unsigned long start_gfn, - unsigned long nr_mfns, - unsigned long mfn); + gfn_t gfn, + unsigned long nr, + mfn_t mfn); int unmap_regions_rw_cache(struct domain *d, - unsigned long start_gfn, - unsigned long nr_mfns, - unsigned long mfn); + gfn_t gfn, + unsigned long nr, + mfn_t mfn); int map_dev_mmio_region(struct domain *d, gfn_t gfn, -- 1.9.1 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [PATCH v4 14/16] xen/arm: p2m: Introduce helpers to insert and remove mapping
More the half of the arguments of INSERT and REMOVE are the same for each callers. Simplify the callers of apply_p2m_changes by adding new helpers which will fill common arguments with default values. Acked-by: Julien Grall --- Changes in v4: - Patch added --- xen/arch/arm/p2m.c | 70 -- 1 file changed, 36 insertions(+), 34 deletions(-) diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index 36ee0fe..3a50787 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -1158,17 +1158,40 @@ out: return rc; } +static inline int p2m_insert_mapping(struct domain *d, + gfn_t start_gfn, + unsigned long nr, + mfn_t mfn, + int mattr, p2m_type_t t) +{ +return apply_p2m_changes(d, INSERT, + pfn_to_paddr(gfn_x(start_gfn)), + pfn_to_paddr(gfn_x(start_gfn) + nr), + pfn_to_paddr(mfn_x(mfn)), + mattr, 0, t, d->arch.p2m.default_access); +} + +static inline int p2m_remove_mapping(struct domain *d, + gfn_t start_gfn, + unsigned long nr, + mfn_t mfn) +{ +return apply_p2m_changes(d, REMOVE, + pfn_to_paddr(gfn_x(start_gfn)), + pfn_to_paddr(gfn_x(start_gfn) + nr), + pfn_to_paddr(mfn_x(mfn)), + /* arguments below not used when removing mapping */ + MATTR_MEM, 0, p2m_invalid, + d->arch.p2m.default_access); +} + int map_regions_rw_cache(struct domain *d, gfn_t gfn, unsigned long nr, mfn_t mfn) { -return apply_p2m_changes(d, INSERT, - pfn_to_paddr(gfn_x(gfn)), - pfn_to_paddr(gfn_x(gfn) + nr), - pfn_to_paddr(mfn_x(mfn)), - MATTR_MEM, 0, p2m_mmio_direct, - d->arch.p2m.default_access); +return p2m_insert_mapping(d, gfn, nr, mfn, + MATTR_MEM, p2m_mmio_direct); } int unmap_regions_rw_cache(struct domain *d, @@ -1176,12 +1199,7 @@ int unmap_regions_rw_cache(struct domain *d, unsigned long nr, mfn_t mfn) { -return apply_p2m_changes(d, REMOVE, - pfn_to_paddr(gfn_x(gfn)), - pfn_to_paddr(gfn_x(gfn) + nr), - pfn_to_paddr(mfn_x(mfn)), - MATTR_MEM, 0, p2m_invalid, - d->arch.p2m.default_access); +return p2m_remove_mapping(d, gfn, nr, mfn); } int map_mmio_regions(struct domain *d, @@ -1189,12 +1207,8 @@ int map_mmio_regions(struct domain *d, unsigned long nr, mfn_t mfn) { -return apply_p2m_changes(d, INSERT, - pfn_to_paddr(gfn_x(start_gfn)), - pfn_to_paddr(gfn_x(start_gfn) + nr), - pfn_to_paddr(mfn_x(mfn)), - MATTR_DEV, 0, p2m_mmio_direct, - d->arch.p2m.default_access); +return p2m_insert_mapping(d, start_gfn, nr, mfn, + MATTR_MEM, p2m_mmio_direct); } int unmap_mmio_regions(struct domain *d, @@ -1202,12 +1216,7 @@ int unmap_mmio_regions(struct domain *d, unsigned long nr, mfn_t mfn) { -return apply_p2m_changes(d, REMOVE, - pfn_to_paddr(gfn_x(start_gfn)), - pfn_to_paddr(gfn_x(start_gfn) + nr), - pfn_to_paddr(mfn_x(mfn)), - MATTR_DEV, 0, p2m_invalid, - d->arch.p2m.default_access); +return p2m_remove_mapping(d, start_gfn, nr, mfn); } int map_dev_mmio_region(struct domain *d, @@ -1237,22 +1246,15 @@ int guest_physmap_add_entry(struct domain *d, unsigned long page_order, p2m_type_t t) { -return apply_p2m_changes(d, INSERT, - pfn_to_paddr(gfn_x(gfn)), - pfn_to_paddr(gfn_x(gfn) + (1 << page_order)), - pfn_to_paddr(mfn_x(mfn)), MATTR_MEM, 0, t, - d->arch.p2m.default_access); +return p2m_insert_mapping(d, gfn, (1 << page_order), mfn, + MATTR_MEM, t); } void guest_physmap_remove_page(struct domain *d, gfn_t gfn,
[Xen-devel] [PATCH v4 09/16] xen/arm: map_regions_rw_cache: Map the region with p2m->default_access
The parameter 'access' is used by memaccess to restrict temporarily the permission. This parameter should not be used for other purpose (such as restricting permanently the permission). The type p2m_mmio_direct will map the region Read-Write and non-executable. Note that this is already the current behavior with the combination of the type and the access. So there is no functional change. Signed-off-by: Julien Grall --- Cc: Shannon Zhao This patch is a candidate for Xen 4.7. Currently this function is only used to map ACPI regions. I am wondering if we should introduce a new p2m type for it. And map this region RO (I am not sure why a guest would want to modify this region). Changes in v4: - Patch added --- xen/arch/arm/p2m.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index 1a10019..4f3564f 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -1231,7 +1231,7 @@ int map_regions_rw_cache(struct domain *d, pfn_to_paddr(start_gfn + nr), pfn_to_paddr(mfn), MATTR_MEM, 0, p2m_mmio_direct, - p2m_access_rw); + d->arch.p2m.default_access); } int unmap_regions_rw_cache(struct domain *d, @@ -1244,7 +1244,7 @@ int unmap_regions_rw_cache(struct domain *d, pfn_to_paddr(start_gfn + nr), pfn_to_paddr(mfn), MATTR_MEM, 0, p2m_invalid, - p2m_access_rw); + d->arch.p2m.default_access); } int map_mmio_regions(struct domain *d, -- 1.9.1 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [PATCH v4 10/16] xen/arm: dom0_build: Remove dead code in allocate_memory
The code to allocate memory when dom0 does not use direct mapping is relying on the presence of memory node in the DT. However, they are not present when booting using UEFI or when using ACPI. Rather than fixing the code, remove it because dom0 is always direct memory mapped and therefore the code is never tested. Also add a check to avoid disabling direct memory mapped and not implementing the associated RAM bank allocation. Signed-off-by: Julien Grall --- Changes in v4: - Patch added --- xen/arch/arm/domain_build.c | 58 ++--- 1 file changed, 7 insertions(+), 51 deletions(-) diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 49185f0..923f48a 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -235,7 +235,7 @@ fail: * (as described above) we allow higher allocations and continue until * that runs out (or we have allocated sufficient dom0 memory). */ -static void allocate_memory_11(struct domain *d, struct kernel_info *kinfo) +static void allocate_memory(struct domain *d, struct kernel_info *kinfo) { const unsigned int min_low_order = get_order_from_bytes(min_t(paddr_t, dom0_mem, MB(128))); @@ -247,6 +247,12 @@ static void allocate_memory_11(struct domain *d, struct kernel_info *kinfo) bool_t lowmem = is_32bit_domain(d); unsigned int bits; +/* + * TODO: Implement memory bank allocation when DOM0 is not direct + * mapped + */ +BUG_ON(!dom0_11_mapping); + printk("Allocating 1:1 mappings totalling %ldMB for dom0:\n", /* Don't want format this as PRIpaddr (16 digit hex) */ (unsigned long)(kinfo->unassigned_mem >> 20)); @@ -343,56 +349,6 @@ static void allocate_memory_11(struct domain *d, struct kernel_info *kinfo) } } -static void allocate_memory(struct domain *d, struct kernel_info *kinfo) -{ - -struct dt_device_node *memory = NULL; -const void *reg; -u32 reg_len, reg_size; -unsigned int bank = 0; - -if ( dom0_11_mapping ) -return allocate_memory_11(d, kinfo); - -while ( (memory = dt_find_node_by_type(memory, "memory")) ) -{ -int l; - -dt_dprintk("memory node\n"); - -reg_size = dt_cells_to_size(dt_n_addr_cells(memory) + dt_n_size_cells(memory)); - -reg = dt_get_property(memory, "reg", ®_len); -if ( reg == NULL ) -panic("Memory node has no reg property"); - -for ( l = 0; - kinfo->unassigned_mem > 0 && l + reg_size <= reg_len - && kinfo->mem.nr_banks < NR_MEM_BANKS; - l += reg_size ) -{ -paddr_t start, size; - -if ( dt_device_get_address(memory, bank, &start, &size) ) -panic("Unable to retrieve the bank %u for %s", - bank, dt_node_full_name(memory)); - -if ( size > kinfo->unassigned_mem ) -size = kinfo->unassigned_mem; - -printk("Populate P2M %#"PRIx64"->%#"PRIx64"\n", - start, start + size); -if ( p2m_populate_ram(d, start, start + size) < 0 ) -panic("Failed to populate P2M"); -kinfo->mem.bank[kinfo->mem.nr_banks].start = start; -kinfo->mem.bank[kinfo->mem.nr_banks].size = size; -kinfo->mem.nr_banks++; - -kinfo->unassigned_mem -= size; -} -} -} - static int write_properties(struct domain *d, struct kernel_info *kinfo, const struct dt_device_node *node) { -- 1.9.1 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [PATCH v4 12/16] xen/arm: Use the typesafes mfn and gfn in map_dev_mmio_region...
to avoid mixing machine frame with guest frame. Also drop the prefix start_. Signed-off-by: Julien Grall --- Changes in v4: - Patch added --- xen/arch/arm/mm.c | 2 +- xen/arch/arm/p2m.c| 10 +- xen/include/asm-arm/p2m.h | 4 ++-- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c index 0e408f8..b5fc034 100644 --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -1145,7 +1145,7 @@ int xenmem_add_to_physmap_one( if ( extra.res0 ) return -EOPNOTSUPP; -rc = map_dev_mmio_region(d, gfn_x(gfn), 1, idx); +rc = map_dev_mmio_region(d, gfn, 1, _mfn(idx)); return rc; default: diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index 685dc94..faadebf 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -1211,20 +1211,20 @@ int unmap_mmio_regions(struct domain *d, } int map_dev_mmio_region(struct domain *d, -unsigned long start_gfn, +gfn_t gfn, unsigned long nr, -unsigned long mfn) +mfn_t mfn) { int res; -if ( !(nr && iomem_access_permitted(d, mfn, mfn + nr - 1)) ) +if ( !(nr && iomem_access_permitted(d, mfn_x(mfn), mfn_x(mfn) + nr - 1)) ) return 0; -res = map_mmio_regions(d, _gfn(start_gfn), nr, _mfn(mfn)); +res = map_mmio_regions(d, gfn, nr, mfn); if ( res < 0 ) { printk(XENLOG_G_ERR "Unable to map [%#lx - %#lx] in Dom%d\n", - mfn, mfn + nr - 1, d->domain_id); + mfn_x(mfn), mfn_x(mfn) + nr - 1, d->domain_id); return res; } diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h index 4752161..8d29eda 100644 --- a/xen/include/asm-arm/p2m.h +++ b/xen/include/asm-arm/p2m.h @@ -152,9 +152,9 @@ int unmap_regions_rw_cache(struct domain *d, unsigned long mfn); int map_dev_mmio_region(struct domain *d, -unsigned long start_gfn, +gfn_t gfn, unsigned long nr, -unsigned long mfn); +mfn_t mfn); int guest_physmap_add_entry(struct domain *d, gfn_t gfn, -- 1.9.1 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [PATCH v4 15/16] xen/arm: p2m: Use typesafe gfn for {max, lowest}_mapped_gfn
Signed-off-by: Julien Grall --- Changes in v4: - Patch added --- xen/arch/arm/mm.c | 2 +- xen/arch/arm/p2m.c| 18 +- xen/include/asm-arm/p2m.h | 4 ++-- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c index b5fc034..4e256c2 100644 --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -1004,7 +1004,7 @@ int page_is_ram_type(unsigned long mfn, unsigned long mem_type) unsigned long domain_get_maximum_gpfn(struct domain *d) { -return d->arch.p2m.max_mapped_gfn; +return gfn_x(d->arch.p2m.max_mapped_gfn); } void share_xen_page_with_guest(struct page_info *page, diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index 3a50787..3d076de 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -976,7 +976,7 @@ static int apply_p2m_changes(struct domain *d, * This is set in preempt_count_limit. * */ -p2m->lowest_mapped_gfn = addr >> PAGE_SHIFT; +p2m->lowest_mapped_gfn = _gfn(addr >> PAGE_SHIFT); rc = -ERESTART; goto out; @@ -1117,8 +1117,8 @@ static int apply_p2m_changes(struct domain *d, if ( op == INSERT ) { -p2m->max_mapped_gfn = max(p2m->max_mapped_gfn, egfn); -p2m->lowest_mapped_gfn = min(p2m->lowest_mapped_gfn, sgfn); +p2m->max_mapped_gfn = gfn_max(p2m->max_mapped_gfn, _gfn(egfn)); +p2m->lowest_mapped_gfn = gfn_min(p2m->lowest_mapped_gfn, _gfn(sgfn)); } rc = 0; @@ -1383,8 +1383,8 @@ int p2m_init(struct domain *d) p2m->root = NULL; -p2m->max_mapped_gfn = 0; -p2m->lowest_mapped_gfn = ULONG_MAX; +p2m->max_mapped_gfn = _gfn(0); +p2m->lowest_mapped_gfn = _gfn(ULONG_MAX); p2m->default_access = p2m_access_rwx; p2m->mem_access_enabled = false; @@ -1401,8 +1401,8 @@ int relinquish_p2m_mapping(struct domain *d) struct p2m_domain *p2m = &d->arch.p2m; return apply_p2m_changes(d, RELINQUISH, - pfn_to_paddr(p2m->lowest_mapped_gfn), - pfn_to_paddr(p2m->max_mapped_gfn), + pfn_to_paddr(gfn_x(p2m->lowest_mapped_gfn)), + pfn_to_paddr(gfn_x(p2m->max_mapped_gfn)), pfn_to_paddr(INVALID_MFN), MATTR_MEM, 0, p2m_invalid, d->arch.p2m.default_access); @@ -1413,8 +1413,8 @@ int p2m_cache_flush(struct domain *d, gfn_t start, unsigned long nr) struct p2m_domain *p2m = &d->arch.p2m; gfn_t end = gfn_add(start, nr); -start = gfn_max(start, _gfn(p2m->lowest_mapped_gfn)); -end = gfn_min(end, _gfn(p2m->max_mapped_gfn)); +start = gfn_max(start, p2m->lowest_mapped_gfn); +end = gfn_min(end, p2m->max_mapped_gfn); return apply_p2m_changes(d, CACHEFLUSH, pfn_to_paddr(gfn_x(start)), diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h index 6e258b9..34096bc 100644 --- a/xen/include/asm-arm/p2m.h +++ b/xen/include/asm-arm/p2m.h @@ -34,13 +34,13 @@ struct p2m_domain { /* Highest guest frame that's ever been mapped in the p2m * Only takes into account ram and foreign mapping */ -unsigned long max_mapped_gfn; +gfn_t max_mapped_gfn; /* Lowest mapped gfn in the p2m. When releasing mapped gfn's in a * preemptible manner this is update to track recall where to * resume the search. Apart from during teardown this can only * decrease. */ -unsigned long lowest_mapped_gfn; +gfn_t lowest_mapped_gfn; /* Gather some statistics for information purposes only */ struct { -- 1.9.1 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [PATCH v4 16/16] xen/arm: p2m: Rework the interface of apply_p2m_changes and use typesafe
Most of the callers of apply_p2m_changes have a GFN, a MFN and the number of frame to change in hand. Rather than asking each caller to convert the frame to an address, rework the interfaces to pass the GFN, MFN and the number of frame. Note that it would be possible to do more clean-up in apply_p2m_changes, but this will be done in a follow-up series. Signed-off-by: Julien Grall --- Changes in v4: - Patch added --- xen/arch/arm/p2m.c | 62 -- 1 file changed, 28 insertions(+), 34 deletions(-) diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index 3d076de..6366e8f 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -906,25 +906,26 @@ static void update_reference_mapping(struct page_info *page, static int apply_p2m_changes(struct domain *d, enum p2m_operation op, - paddr_t start_gpaddr, - paddr_t end_gpaddr, - paddr_t maddr, + gfn_t sgfn, + unsigned long nr, + mfn_t smfn, int mattr, uint32_t mask, p2m_type_t t, p2m_access_t a) { +paddr_t start_gpaddr = pfn_to_paddr(gfn_x(sgfn)); +paddr_t end_gpaddr = pfn_to_paddr(gfn_x(sgfn) + nr); +paddr_t maddr = pfn_to_paddr(mfn_x(smfn)); int rc, ret; struct p2m_domain *p2m = &d->arch.p2m; lpae_t *mappings[4] = { NULL, NULL, NULL, NULL }; struct page_info *pages[4] = { NULL, NULL, NULL, NULL }; -paddr_t addr, orig_maddr = maddr; +paddr_t addr; unsigned int level = 0; unsigned int cur_root_table = ~0; unsigned int cur_offset[4] = { ~0, ~0, ~0, ~0 }; unsigned int count = 0; -const unsigned long sgfn = paddr_to_pfn(start_gpaddr), -egfn = paddr_to_pfn(end_gpaddr); const unsigned int preempt_count_limit = (op == MEMACCESS) ? 1 : 0x2000; const bool_t preempt = !is_idle_vcpu(current); bool_t flush = false; @@ -986,9 +987,9 @@ static int apply_p2m_changes(struct domain *d, * Preempt setting mem_access permissions as required by XSA-89, * if it's not the last iteration. */ -uint32_t progress = paddr_to_pfn(addr) - sgfn + 1; +uint32_t progress = paddr_to_pfn(addr) - gfn_x(sgfn) + 1; -if ( (egfn - sgfn) > progress && !(progress & mask) ) +if ( nr > progress && !(progress & mask) ) { rc = progress; goto out; @@ -1117,8 +1118,9 @@ static int apply_p2m_changes(struct domain *d, if ( op == INSERT ) { -p2m->max_mapped_gfn = gfn_max(p2m->max_mapped_gfn, _gfn(egfn)); -p2m->lowest_mapped_gfn = gfn_min(p2m->lowest_mapped_gfn, _gfn(sgfn)); +p2m->max_mapped_gfn = gfn_max(p2m->max_mapped_gfn, + gfn_add(sgfn, nr)); +p2m->lowest_mapped_gfn = gfn_min(p2m->lowest_mapped_gfn, sgfn); } rc = 0; @@ -1127,7 +1129,7 @@ out: if ( flush ) { flush_tlb_domain(d); -ret = iommu_iotlb_flush(d, sgfn, egfn - sgfn); +ret = iommu_iotlb_flush(d, gfn_x(sgfn), nr); if ( !rc ) rc = ret; } @@ -1146,12 +1148,14 @@ out: if ( rc < 0 && ( op == INSERT ) && addr != start_gpaddr ) { +unsigned long gfn = paddr_to_pfn(addr); + BUG_ON(addr == end_gpaddr); /* * addr keeps the address of the end of the last successfully-inserted * mapping. */ -apply_p2m_changes(d, REMOVE, start_gpaddr, addr, orig_maddr, +apply_p2m_changes(d, REMOVE, sgfn, gfn - gfn_x(sgfn), smfn, mattr, 0, p2m_invalid, d->arch.p2m.default_access); } @@ -1164,10 +1168,7 @@ static inline int p2m_insert_mapping(struct domain *d, mfn_t mfn, int mattr, p2m_type_t t) { -return apply_p2m_changes(d, INSERT, - pfn_to_paddr(gfn_x(start_gfn)), - pfn_to_paddr(gfn_x(start_gfn) + nr), - pfn_to_paddr(mfn_x(mfn)), +return apply_p2m_changes(d, INSERT, start_gfn, nr, mfn, mattr, 0, t, d->arch.p2m.default_access); } @@ -1176,10 +1177,7 @@ static inline int p2m_remove_mapping(struct domain *d, unsigned long nr, mfn_t mfn) { -return apply_p2m_changes(d, REMOVE, - pfn_to_paddr(gfn_x(start_gfn)), - pfn_to_paddr(gfn_x(start_gfn) + nr), - pfn_to_paddr(mfn_x(mfn)), +return apply_p2m_changes(d, REMOVE, start_gfn, nr, mfn, /* argumen
[Xen-devel] [PATCH v4 11/16] xen/arm: p2m: Remove unused operation ALLOCATE
The operation ALLOCATE is unused. If we ever need it, it could be reimplemented with INSERT. Signed-off-by: Julien Grall --- Changes in v4: - Patch added --- xen/arch/arm/p2m.c| 67 ++- xen/include/asm-arm/p2m.h | 3 --- 2 files changed, 2 insertions(+), 68 deletions(-) diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index 4f3564f..685dc94 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -547,7 +547,6 @@ static int p2m_mem_access_radix_set(struct p2m_domain *p2m, unsigned long pfn, enum p2m_operation { INSERT, -ALLOCATE, REMOVE, RELINQUISH, CACHEFLUSH, @@ -667,7 +666,6 @@ static int apply_one_level(struct domain *d, { const paddr_t level_size = level_sizes[level]; const paddr_t level_mask = level_masks[level]; -const paddr_t level_shift = level_shifts[level]; struct p2m_domain *p2m = &d->arch.p2m; lpae_t pte; @@ -678,58 +676,6 @@ static int apply_one_level(struct domain *d, switch ( op ) { -case ALLOCATE: -ASSERT(level < 3 || !p2m_valid(orig_pte)); -ASSERT(*maddr == 0); - -if ( p2m_valid(orig_pte) ) -return P2M_ONE_DESCEND; - -if ( is_mapping_aligned(*addr, end_gpaddr, 0, level_size) && - /* We only create superpages when mem_access is not in use. */ - (level == 3 || (level < 3 && !p2m->mem_access_enabled)) ) -{ -struct page_info *page; - -page = alloc_domheap_pages(d, level_shift - PAGE_SHIFT, 0); -if ( page ) -{ -rc = p2m_mem_access_radix_set(p2m, paddr_to_pfn(*addr), a); -if ( rc < 0 ) -{ -free_domheap_page(page); -return rc; -} - -pte = mfn_to_p2m_entry(page_to_mfn(page), mattr, t, a); -if ( level < 3 ) -pte.p2m.table = 0; -p2m_write_pte(entry, pte, flush_cache); -p2m->stats.mappings[level]++; - -*addr += level_size; - -return P2M_ONE_PROGRESS; -} -else if ( level == 3 ) -return -ENOMEM; -} - -/* L3 is always suitably aligned for mapping (handled, above) */ -BUG_ON(level == 3); - -/* - * If we get here then we failed to allocate a sufficiently - * large contiguous region for this level (which can't be - * L3) or mem_access is in use. Create a page table and - * continue to descend so we try smaller allocations. - */ -rc = p2m_create_table(d, entry, 0, flush_cache); -if ( rc < 0 ) -return rc; - -return P2M_ONE_DESCEND; - case INSERT: if ( is_mapping_aligned(*addr, end_gpaddr, *maddr, level_size) && /* @@ -1169,7 +1115,7 @@ static int apply_p2m_changes(struct domain *d, } } -if ( op == ALLOCATE || op == INSERT ) +if ( op == INSERT ) { p2m->max_mapped_gfn = max(p2m->max_mapped_gfn, egfn); p2m->lowest_mapped_gfn = min(p2m->lowest_mapped_gfn, sgfn); @@ -1197,7 +1143,7 @@ out: spin_unlock(&p2m->lock); -if ( rc < 0 && ( op == INSERT || op == ALLOCATE ) && +if ( rc < 0 && ( op == INSERT ) && addr != start_gpaddr ) { BUG_ON(addr == end_gpaddr); @@ -1212,15 +1158,6 @@ out: return rc; } -int p2m_populate_ram(struct domain *d, - paddr_t start, - paddr_t end) -{ -return apply_p2m_changes(d, ALLOCATE, start, end, - 0, MATTR_MEM, 0, p2m_ram_rw, - d->arch.p2m.default_access); -} - int map_regions_rw_cache(struct domain *d, unsigned long start_gfn, unsigned long nr, diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h index 8a96e68..4752161 100644 --- a/xen/include/asm-arm/p2m.h +++ b/xen/include/asm-arm/p2m.h @@ -141,9 +141,6 @@ mfn_t p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t); /* Clean & invalidate caches corresponding to a region of guest address space */ int p2m_cache_flush(struct domain *d, gfn_t start, unsigned long nr); -/* Setup p2m RAM mapping for domain d from start-end. */ -int p2m_populate_ram(struct domain *d, paddr_t start, paddr_t end); - int map_regions_rw_cache(struct domain *d, unsigned long start_gfn, unsigned long nr_mfns, -- 1.9.1 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel