Re: [PATCH 1/8] xen/guest_access: Harden copy_to_guest_offset to prevent const dest operand
On 31.03.2020 21:13, Julien Grall wrote: > On 31/03/2020 08:26, Jan Beulich wrote: >> On 30.03.2020 21:21, Julien Grall wrote: >>> From: Julien Grall >>> >>> At the moment, copy_to_guest_offset() will allow the hypervisor to copy >>> data to guest handle marked const. >>> >>> Thankfully, no users of the helper will do that. Rather than hoping this >>> can be caught during review, harden copy_to_guest_offset() so the build >>> will fail if such users are introduced. >> >> But there are other implications you break: >> >>> --- a/xen/include/asm-arm/guest_access.h >>> +++ b/xen/include/asm-arm/guest_access.h >>> @@ -126,7 +126,7 @@ int access_guest_memory_by_ipa(struct domain *d, >>> paddr_t ipa, void *buf, >>> #define __copy_to_guest_offset(hnd, off, ptr, nr) ({ \ >>> const typeof(*(ptr)) *_s = (ptr); \ >>> - char (*_d)[sizeof(*_s)] = (void *)(hnd).p; \ >>> + typeof(*((hnd).p)) *_d = (hnd).p; \ >>> ((void)((hnd).p == (ptr))); \ >>> __raw_copy_to_guest(_d+(off), _s, sizeof(*_s)*(nr));\ >> >> Until this change, it is "ptr" which all sizes get derived from, >> i.e. it is the internally used type rather than the handle type >> which controls this. I'm sure we use this in a few places, to >> copy to e.g. a handle derived from "void". Compatibility of types >> (disallowing other than void) is checked by the comparison on the >> line immediately after the line you change. Yes "_d+(off)" right >> above here then changes its result. I consider it pretty likely >> you'd notice this issue once you go beyond just build testing. > > I missed that part. To be honest, it feels wrong to me to have > "off" != 0 and use a void type for the handle. Would it make > sense to forbid it? I don't think so - the idea (aiui) has always been for the Xen internal object's type to control what gets copied, and hence a void handle is to be fine for both copy-in and copy-out. > As a side node, I have updated __copy_to_guest_offset() but > forgot to update copy_to_guest_offset(). I will look to apply > the modifications we agree on both side. Ah, sure. >> To address this, I guess we need to find an expression along the >> lines of that comparison, which does not cause any code to be >> generated, but which verifies the properties we care about. The >> line you change should be left alone, from all I can tell right >> now. > > I am not aware of any way before C11 to check if a variable is > const or not. If we wanted to keep allow void type the handle > then a possible approach would be: > > #define copy_to_guest_offset(hnd, off, ptr, nr) ({ \ > const typeof(*(ptr)) *_s = (ptr); \ > typeof(*((hnd).p)) *_d = (hnd).p; \ > size_t mul = (sizeof(*(hnd).p) > 1) ? 1 : sizeof (*_s); \ > ((void)((hnd).p == (ptr))); \ > raw_copy_to_guest(_d + (off) * mul, _s, sizeof(*_s)*(nr)); \ > }) > > I don't particularly like it but I could not come up with better so far. Not very nice indeed, and the conditional expression - at the first glance being the wrong way round - seems outright confusing to me. I'll try to find time to experiment some with this as well, since unless we can find a reasonably neat solution here, I'm inclined to suggest to leave this as it is now. Jan
[linux-5.4 test] 149244: regressions - FAIL
flight 149244 linux-5.4 real [real] http://logs.test-lab.xenproject.org/osstest/logs/149244/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-amd64-amd64-qemuu-nested-intel 17 debian-hvm-install/l1/l2 fail REGR. vs. 146121 Tests which are failing intermittently (not blocking): test-amd64-amd64-xl-rtds 18 guest-localmigrate/x10 fail pass in 149200 test-armhf-armhf-xl-rtds 16 guest-start/debian.repeat fail pass in 149200 Tests which did not succeed, but are not blocking: test-amd64-amd64-dom0pvh-xl-intel 12 guest-startfail baseline untested test-amd64-i386-xl-pvshim12 guest-start fail never pass test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 13 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 14 saverestore-support-checkfail never pass test-amd64-amd64-libvirt 13 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-i386-libvirt 13 migrate-support-checkfail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2 fail never pass test-arm64-arm64-xl-credit2 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 14 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 14 saverestore-support-checkfail never pass test-arm64-arm64-xl 13 migrate-support-checkfail never pass test-arm64-arm64-xl 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-arndale 13 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 14 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 12 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 13 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 14 saverestore-support-checkfail never pass test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stop fail never pass test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail never pass test-amd64-i386-xl-qemut-win7-amd64 17 guest-stop fail never pass test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stop fail never pass test-armhf-armhf-xl-credit2 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit1 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 13 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 14 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 13 migrate-support-checkfail never pass test-armhf-armhf-libvirt 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-cubietruck 13 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-rtds 13 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 14 saverestore-support-checkfail never pass test-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stop fail never pass test-amd64-amd64-xl-qemut-ws16-amd64 17 guest-stop fail never pass test-armhf-armhf-libvirt-raw 12 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-vhd 12 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 13 saverestore-support-checkfail never pass test-amd64-i386-xl-qemut-ws16-amd64 17 guest-stop fail never pass test-armhf-armhf-xl 13 migrate-support-checkfail never pass test-armhf-armhf-xl 14 saverestore-support-checkfail never pass test-amd64-i386-xl-qemuu-ws16-amd64 17 guest-stop fail never pass test-amd64-amd64-dom0pvh-xl-amd 2 hosts-allocatestarved in 149200 n/a version targeted for testing: linux462afcd6e7ea94a7027a96a3bb12d0140b0b4216 baseline version: linux122179cb7d648a6f36b20dd6bf34f953cb384c30 Las
Re: [PATCH v8 1/3] x86/tlb: introduce a flush HVM ASIDs flag
On 31.03.2020 18:45, Roger Pau Monné wrote: > On Tue, Mar 31, 2020 at 05:40:59PM +0200, Jan Beulich wrote: >> On 20.03.2020 19:42, Roger Pau Monne wrote: >>> --- a/xen/arch/x86/mm/hap/hap.c >>> +++ b/xen/arch/x86/mm/hap/hap.c >>> @@ -118,7 +118,7 @@ int hap_track_dirty_vram(struct domain *d, >>> p2m_change_type_range(d, begin_pfn, begin_pfn + nr, >>>p2m_ram_rw, p2m_ram_logdirty); >>> >>> -flush_tlb_mask(d->dirty_cpumask); >>> +flush_mask(d->dirty_cpumask, FLUSH_TLB | FLUSH_HVM_ASID_CORE); >>> >>> memset(dirty_bitmap, 0xff, size); /* consider all pages dirty >>> */ >>> } >>> @@ -205,7 +205,7 @@ static int hap_enable_log_dirty(struct domain *d, >>> bool_t log_global) >>> * to be read-only, or via hardware-assisted log-dirty. >>> */ >>> p2m_change_entry_type_global(d, p2m_ram_rw, p2m_ram_logdirty); >>> -flush_tlb_mask(d->dirty_cpumask); >>> +flush_mask(d->dirty_cpumask, FLUSH_TLB | FLUSH_HVM_ASID_CORE); >>> } >>> return 0; >>> } >>> @@ -234,7 +234,7 @@ static void hap_clean_dirty_bitmap(struct domain *d) >>> * be read-only, or via hardware-assisted log-dirty. >>> */ >>> p2m_change_entry_type_global(d, p2m_ram_rw, p2m_ram_logdirty); >>> -flush_tlb_mask(d->dirty_cpumask); >>> +flush_mask(d->dirty_cpumask, FLUSH_TLB | FLUSH_HVM_ASID_CORE); >>> } >>> >>> // >>> @@ -798,7 +798,7 @@ hap_write_p2m_entry(struct p2m_domain *p2m, unsigned >>> long gfn, l1_pgentry_t *p, >>> >>> safe_write_pte(p, new); >>> if ( old_flags & _PAGE_PRESENT ) >>> -flush_tlb_mask(d->dirty_cpumask); >>> +flush_mask(d->dirty_cpumask, FLUSH_TLB | FLUSH_HVM_ASID_CORE); >> >> For all four - why FLUSH_TLB? Doesn't the flushing here solely care >> about guest translations? > > Not on AMD, at least to my understanding, the AMD SDM states: > > "If a hypervisor modifies a nested page table by decreasing permission > levels, clearing present bits, or changing address translations and > intends to return to the same ASID, it should use either TLB command > 011b or 001b." > > It's in section 15.16.1. > > This to my understanding implies that on AMD hardware modifications to > the NPT require an ASID flush. I assume that on AMD ASIDs also cache > combined translations, guest linear -> host physical. I guess I don't follow - I asked about FLUSH_TLB. I agree there needs to be FLUSH_HVM_ASID_CORE here. >>> --- a/xen/arch/x86/mm/hap/nested_hap.c >>> +++ b/xen/arch/x86/mm/hap/nested_hap.c >>> @@ -84,7 +84,7 @@ nestedp2m_write_p2m_entry(struct p2m_domain *p2m, >>> unsigned long gfn, >>> safe_write_pte(p, new); >>> >>> if (old_flags & _PAGE_PRESENT) >>> -flush_tlb_mask(p2m->dirty_cpumask); >>> +flush_mask(p2m->dirty_cpumask, FLUSH_TLB | FLUSH_HVM_ASID_CORE); >> >> Same here then I guess. >> >>> --- a/xen/arch/x86/mm/p2m-pt.c >>> +++ b/xen/arch/x86/mm/p2m-pt.c >>> @@ -896,7 +896,8 @@ static void p2m_pt_change_entry_type_global(struct >>> p2m_domain *p2m, >>> unmap_domain_page(tab); >>> >>> if ( changed ) >>> - flush_tlb_mask(p2m->domain->dirty_cpumask); >>> + flush_mask(p2m->domain->dirty_cpumask, >>> +FLUSH_TLB | FLUSH_HVM_ASID_CORE); >> >> Given that this code is used in shadow mode as well, perhaps >> better to keep it here. Albeit maybe FLUSH_TLB could be dependent >> upon !hap_enabled()? >> >>> --- a/xen/arch/x86/mm/paging.c >>> +++ b/xen/arch/x86/mm/paging.c >>> @@ -613,7 +613,7 @@ void paging_log_dirty_range(struct domain *d, >>> >>> p2m_unlock(p2m); >>> >>> -flush_tlb_mask(d->dirty_cpumask); >>> +flush_mask(d->dirty_cpumask, FLUSH_TLB | FLUSH_HVM_ASID_CORE); >> >> Same here? > > I'm fine with doing further refinements, but I would like to be on the > conservative side and keep such flushes. Well, if hap.c had FLUSH_TLB dropped, for consistency it should become conditional here, imo. >>> @@ -993,7 +993,7 @@ static void shadow_blow_tables(struct domain *d) >>> pagetable_get_mfn(v->arch.shadow_table[i]), >>> 0); >>> >>> /* Make sure everyone sees the unshadowings */ >>> -flush_tlb_mask(d->dirty_cpumask); >>> +flush_mask(d->dirty_cpumask, FLUSH_TLB | FLUSH_HVM_ASID_CORE); >> >> Taking this as example, wouldn't it be more consistent overall if >> paths not being HVM-only would specify FLUSH_HVM_ASID_CORE only >> for HVM domains? > > I think there's indeed room for improvement here, as it's likely > possible to drop some of the ASID/VPID flushes. Given that previous to > this patch we would flush ASIDs on every TLB flush I think the current > approach is safe, and as said above further improvements can be done > afterwards. There's no safety implication from my suggestion. Needless FLUSH_HVM_ASID_CORE for non-HVM will result in a call to hvm_flush_guest
[linux-linus test] 149238: tolerable FAIL - PUSHED
flight 149238 linux-linus real [real] http://logs.test-lab.xenproject.org/osstest/logs/149238/ Failures :-/ but no regressions. Regressions which are regarded as allowable (not blocking): test-amd64-amd64-xl-rtds 18 guest-localmigrate/x10 fail REGR. vs. 133580 Tests which did not succeed, but are not blocking: test-amd64-amd64-dom0pvh-xl-intel 15 guest-saverestore fail baseline untested test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stopfail like 133580 test-armhf-armhf-libvirt 14 saverestore-support-checkfail like 133580 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stopfail like 133580 test-amd64-amd64-xl-qemut-ws16-amd64 17 guest-stopfail like 133580 test-amd64-i386-xl-qemut-win7-amd64 17 guest-stop fail like 133580 test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail like 133580 test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail like 133580 test-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stopfail like 133580 test-amd64-i386-xl-pvshim12 guest-start fail never pass test-amd64-i386-libvirt 13 migrate-support-checkfail never pass test-amd64-amd64-libvirt 13 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 13 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 14 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2 fail never pass test-arm64-arm64-xl-thunderx 13 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 14 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 14 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 14 saverestore-support-checkfail never pass test-arm64-arm64-xl 13 migrate-support-checkfail never pass test-arm64-arm64-xl 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-arndale 13 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 14 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 12 migrate-support-checkfail never pass test-armhf-armhf-xl 13 migrate-support-checkfail never pass test-armhf-armhf-xl 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-rtds 13 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 13 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-cubietruck 13 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 14 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 14 saverestore-support-checkfail never pass test-amd64-i386-xl-qemuu-ws16-amd64 17 guest-stop fail never pass test-armhf-armhf-xl-vhd 12 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 13 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 14 saverestore-support-checkfail never pass test-amd64-i386-xl-qemut-ws16-amd64 17 guest-stop fail never pass test-armhf-armhf-libvirt-raw 12 migrate-support-checkfail never pass version targeted for testing: linux458ef2a25e0cbdc216012aa2b9cf549d64133b08 baseline version: linux736706bee3298208343a76096370e4f6a5c55915 Last test of basis 133580 2019-03-04 19:53:09 Z 393 days Failing since133605 2019-03-05 20:03:14 Z 392 days 241 attempts Testing same since 149238 2020-03-31 07:17:53 Z0 days1 attempts 6534 people touched revisions under test,
[qemu-mainline test] 149236: regressions - FAIL
flight 149236 qemu-mainline real [real] http://logs.test-lab.xenproject.org/osstest/logs/149236/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-amd64-amd64-xl-qemuu-debianhvm-amd64 10 debian-hvm-install fail REGR. vs. 144861 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 10 debian-hvm-install fail REGR. vs. 144861 test-amd64-amd64-xl-qemuu-dmrestrict-amd64-dmrestrict 10 debian-hvm-install fail REGR. vs. 144861 test-amd64-amd64-xl-qemuu-win7-amd64 10 windows-install fail REGR. vs. 144861 test-amd64-i386-xl-qemuu-debianhvm-i386-xsm 10 debian-hvm-install fail REGR. vs. 144861 test-amd64-i386-xl-qemuu-ws16-amd64 10 windows-install fail REGR. vs. 144861 test-amd64-amd64-xl-qemuu-debianhvm-i386-xsm 10 debian-hvm-install fail REGR. vs. 144861 test-amd64-i386-xl-qemuu-debianhvm-amd64 10 debian-hvm-install fail REGR. vs. 144861 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 10 debian-hvm-install fail REGR. vs. 144861 test-amd64-amd64-xl-qemuu-ovmf-amd64 10 debian-hvm-install fail REGR. vs. 144861 test-amd64-amd64-xl-qemuu-ws16-amd64 10 windows-install fail REGR. vs. 144861 test-amd64-i386-freebsd10-i386 11 guest-startfail REGR. vs. 144861 test-amd64-i386-xl-qemuu-dmrestrict-amd64-dmrestrict 10 debian-hvm-install fail REGR. vs. 144861 test-amd64-i386-qemuu-rhel6hvm-intel 10 redhat-install fail REGR. vs. 144861 test-amd64-amd64-qemuu-nested-amd 10 debian-hvm-install fail REGR. vs. 144861 test-amd64-i386-freebsd10-amd64 11 guest-start fail REGR. vs. 144861 test-amd64-i386-qemuu-rhel6hvm-amd 10 redhat-install fail REGR. vs. 144861 test-amd64-amd64-libvirt 12 guest-start fail REGR. vs. 144861 test-amd64-i386-libvirt-pair 21 guest-start/debian fail REGR. vs. 144861 test-amd64-i386-xl-qemuu-win7-amd64 10 windows-install fail REGR. vs. 144861 test-amd64-amd64-libvirt-xsm 12 guest-start fail REGR. vs. 144861 test-amd64-i386-xl-qemuu-debianhvm-amd64-shadow 10 debian-hvm-install fail REGR. vs. 144861 test-amd64-amd64-libvirt-pair 21 guest-start/debian fail REGR. vs. 144861 test-amd64-amd64-xl-qemuu-debianhvm-amd64-shadow 10 debian-hvm-install fail REGR. vs. 144861 test-amd64-amd64-qemuu-nested-intel 10 debian-hvm-install fail REGR. vs. 144861 test-amd64-i386-xl-qemuu-ovmf-amd64 10 debian-hvm-install fail REGR. vs. 144861 test-amd64-i386-libvirt 12 guest-start fail REGR. vs. 144861 test-amd64-i386-libvirt-xsm 12 guest-start fail REGR. vs. 144861 test-arm64-arm64-libvirt-xsm 12 guest-start fail REGR. vs. 144861 test-armhf-armhf-libvirt 12 guest-start fail REGR. vs. 144861 test-amd64-amd64-libvirt-vhd 10 debian-di-installfail REGR. vs. 144861 test-amd64-amd64-xl-qcow210 debian-di-installfail REGR. vs. 144861 test-armhf-armhf-xl-vhd 10 debian-di-installfail REGR. vs. 144861 test-armhf-armhf-libvirt-raw 10 debian-di-installfail REGR. vs. 144861 Regressions which are regarded as allowable (not blocking): test-amd64-amd64-xl-rtds 16 guest-localmigrate fail REGR. vs. 144861 test-armhf-armhf-xl-rtds16 guest-start/debian.repeat fail REGR. vs. 144861 Tests which did not succeed, but are not blocking: test-amd64-i386-xl-pvshim12 guest-start fail never pass test-arm64-arm64-xl-seattle 13 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 14 saverestore-support-checkfail never pass test-arm64-arm64-xl 13 migrate-support-checkfail never pass test-arm64-arm64-xl 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 13 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-arndale 13 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 14 saverestore-support-checkfail never pass test-amd64-amd64-dom0pvh-xl-amd 20 guest-start/debian.repeat fail never pass test-armhf-armhf-xl-multivcpu 13 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 14 saverestore-support-checkfail never pass test-armhf-armhf-xl 13 migrate-support-checkf
Re: [PATCH v2] xen/arm: implement GICD_I[S/C]ACTIVER reads
On Mon, 30 Mar 2020, Julien Grall wrote: > Hi Stefano, > > On 30/03/2020 17:35, Stefano Stabellini wrote: > > On Sat, 28 Mar 2020, Julien Grall wrote: > > > qHi Stefano, > > > > > > On 27/03/2020 02:34, Stefano Stabellini wrote: > > > > This is a simple implementation of GICD_ICACTIVER / GICD_ISACTIVER > > > > reads. It doesn't take into account the latest state of interrupts on > > > > other vCPUs. Only the current vCPU is up-to-date. A full solution is > > > > not possible because it would require synchronization among all vCPUs, > > > > which would be very expensive in terms or latency. > > > > > > Your sentence suggests you have number showing that correctly emulating > > > the > > > registers would be too slow. Mind sharing them? > > > > No, I don't have any numbers. Would you prefer a different wording or a > > better explanation? I also realized there is a typo in there (or/of). > Let me start with I think correctness is more important than speed. > So I would have expected your commit message to contain some fact why > synchronization is going to be slow and why this is a problem. > > To give you a concrete example, the implementation of set/way instructions are > really slow (it could take a few seconds depending on the setup). However, > this was fine because not implementing them correctly would have a greater > impact on the guest (corruption) and they are not used often. > > I don't think the performance in our case will be in same order magnitude. It > is most likely to be in the range of milliseconds (if not less) which I think > is acceptable for emulation (particularly for the vGIC) and the current uses. Writing on the mailing list some of our discussions today. Correctness is not just in terms of compliance to a specification but it is also about not breaking guests. Introducing latency in the range of milliseconds, or hundreds of microseconds, would break any latency sensitive workloads. We don't have numbers so we don't know for certain the effect that your suggestion would have. It would be interesting to have those numbers, and I'll add to my TODO list to run the experiments you suggested, but I'll put it on the back-burner (from a Xilinx perspective it is low priority as no customers are affected.) > So lets take a step back and look how we could implement ISACTIVER/ICACTIVER > correctly. > > The only thing we need is a snapshot of the interrupts state a given point. I > originally thought it would be necessary to use domain_pause() which is quite > heavy, but I think we only need the vCPU to enter in Xen and sync the states > of the LRs. > > Roughly the code would look like (This is not optimized): > > for_each_vcpu(d, v) > { >if ( v->is_running ) > smp_call_function(do_nothing(), v->cpu); > } > > /* Read the state */ > > A few remarks: >* The function do_nothing() is basically a NOP. >* I am suggesting to use smp_call_function() rather > smp_send_event_check_cpu() is because we need to know when the vCPU has > synchronized the LRs. As the function do_nothing() will be call afterwards, > then we know the the snapshot of the LRs has been done >* It would be possible to everything in one vCPU. >* We can possibly optimize it for the SGIs/PPIs case > > This is still not perfect, but I don't think the performance would be that > bad.
[ovmf test] 149242: all pass - PUSHED
flight 149242 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/149242/ Perfect :-) All tests in this flight passed as required version targeted for testing: ovmf 8c944c938359cffda4c889259b3d2aba69e9ee7b baseline version: ovmf 3000c2963db319d055f474c394b062af910bbb2f Last test of basis 149207 2020-03-30 12:11:22 Z1 days Testing same since 149242 2020-03-31 09:59:29 Z0 days1 attempts People who touched revisions under test: Fan, ZhijuX Laszlo Ersek Liran Alon Maciej Rabeda Pete Batard Zhiju.Fan 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 pass test-amd64-i386-xl-qemuu-ovmf-amd64 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 : To xenbits.xen.org:/home/xen/git/osstest/ovmf.git 3000c2963d..8c944c9383 8c944c938359cffda4c889259b3d2aba69e9ee7b -> xen-tested-master
[xen-unstable test] 149231: regressions - trouble: fail/pass/starved
flight 149231 xen-unstable real [real] http://logs.test-lab.xenproject.org/osstest/logs/149231/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-amd64-amd64-dom0pvh-xl-intel 23 leak-check/checkfail REGR. vs. 149188 test-amd64-amd64-examine 4 memdisk-try-append fail REGR. vs. 149188 Regressions which are regarded as allowable (not blocking): test-armhf-armhf-xl-rtds16 guest-start/debian.repeat fail REGR. vs. 149188 Tests which did not succeed, but are not blocking: test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail like 149188 test-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stopfail like 149188 test-amd64-i386-xl-qemut-win7-amd64 17 guest-stop fail like 149188 test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stopfail like 149188 test-amd64-amd64-qemuu-nested-intel 17 debian-hvm-install/l1/l2 fail like 149188 test-armhf-armhf-libvirt 14 saverestore-support-checkfail like 149188 test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail like 149188 test-amd64-amd64-xl-qemut-ws16-amd64 17 guest-stopfail like 149188 test-amd64-i386-xl-qemuu-ws16-amd64 17 guest-stop fail like 149188 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stopfail like 149188 test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-i386-xl-pvshim12 guest-start fail never pass test-amd64-amd64-libvirt 13 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-i386-libvirt 13 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 13 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 14 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2 fail never pass test-arm64-arm64-xl 13 migrate-support-checkfail never pass test-arm64-arm64-xl 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 13 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 14 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 14 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 12 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 13 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 13 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 14 saverestore-support-checkfail never pass test-armhf-armhf-xl 13 migrate-support-checkfail never pass test-armhf-armhf-xl 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-rtds 13 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 14 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-raw 12 migrate-support-checkfail never pass test-amd64-i386-xl-qemut-ws16-amd64 17 guest-stop fail never pass test-armhf-armhf-xl-vhd 12 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-cubietruck 13 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit1 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 14 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 13 migrate-support-checkfail never pass test-amd64-amd64-dom0pvh-xl-amd 2 hosts-allocate starved n/a version targeted for testing: xen 2a94100dd5646fb8abcd29f48553ff10d0788cc7 baseline version: xen e19b4b3b55f84e0cfcc02fe5d66965969a81c965 La
RE: [EXT] Re: [Xen-devel] Having a DOM-U guest with 1:1 mapping in the second stage MMU.
On Tue, 31 Mar 2020, Andrei Cherechesu wrote: > > On Thu, 13 Feb 2020, Andrei Cherechesu wrote: > > > I used the Xen from Stefano's tree and made the updates to the partial > > > dtb that he specified. > > > > > > > This is mostly likely because Linux is trying to access a region > > > > that is not mapped in stage-2. You can rebuild Xen with debug > > > > enabled and you should see a message "traps.c:..." telling the exact > > > > physical address accessed. > > > > > > > > In general I would recommend to build Xen with debug enabled during > > > > development as the hypervisor will give you more information of what's > > > > going on. > > > > > > > > Cheers, > > > > > > > > -- > > > > Julien Grall > > > > > > I enabled debug config and gave it another try. But I'm still getting > > > the same unhandled fault error, that seems to match what Julien > > > described above. > > > > > > It is indeed a stage-2 abort caused by the guest. > > > > > > I attached the DomU1 crash log at [0]. > > > > > > [0] > > > > > > > > > How should I proceed in this case? > > > > Looking at the logs, you can see: > > > > (XEN) traps.c:1973:d1v0 HSR=0x939f0046 pc=0xff80083ac864 > > gva=0xff800800d048 gpa=0x00402f0048 > > > > So the guest was accessing address 0x402f0048, however, the MMIO address > > range of the device that you are remapping is 0x4002f000-0x4003. > > > > I spotted the mistake now: looking at the partial DTB again, the address of > > the device is: > > > > reg = <0x0 0x402f 0x1000>; > > > > but the address that you are remapping is: > > > > xen,reg = <0x0 0x4002f000 0x1000 0x0 0x4002f000>; > > > > They are not the same! :-) > > Thanks, Stefano! > > I changed the partial DTB and it did not crash anymore. Great to hear! > However, I am encountering another problem now: in Dom0 and in > dom0less-booted DomUs, > I cannot use /dev/hvc0. For dom0less-booted DomUs it is normal because they don't get a PV console, they get an emulated PL011 UART instead. Make sure to have a "vpl011" tag in device tree to enable it (ImageBuilder generates it by default.) The device name is usually ttyAMA0. > Even though I'm specifying "console=hvc0" in dom0-bootargs, when dom0 > finishes booting, > it looks like I cannot use the getty spawned on /dev/hvc0. > > This is the end of the boot log: > [2.947845] random: rngd: uninitialized urandom read (4 bytes read) > [2.958415] random: rngd: uninitialized urandom read (4 bytes read) > [2.965452] random: rngd: uninitialized urandom read (2500 bytes read) > . > [2.972410] random: crng init done > Starting OpenBSD Secure Shell server: sshd > done. > Starting /usr/sbin/xenstored... > Setting domain 0 name, domid and JSON config... > Done setting up Dom0 > Starting xenconsoled... > Starting QEMU as disk backend for dom0 > Starting domain watchdog daemon: xenwatchdogd startup > > [done] > > Auto Linux BSP 1.0 s32g274aevb /dev/hvc0 > > s32g274aevb login: > Auto Linux BSP 1.0 s32g274aevb /dev/ttyLF0 > > s32g274aevb login: > > - END - > > It seems that the getty spawned on /dev/ttyLF0 overwrites the one spawned on > /dev/hvc0. Which > I do not understand, since Dom0 should not have access (?) directly to ttyLF0 > (the serial console device > on our boards). If I remove the line which spawns the getty on ttyLF0 from > /etc/inittab, the system hangs > when waiting for the username, and it does not let me type in any characters. > For the record, hvc0 is > added to /etc/securetty. > > In a system where I boot DomU via xl from Dom0, I can switch to its console > with xl console, and hvc0 > works there. > > The problem that comes with this is that I can not use the CTRL-AAA command > to switch between Dom0 console > and DomU console in a dom0less case, and I cannot therefore test that the > passthrough works. But at least Dom0 > does not have an entry for it under /dev, anymore, and DomU boot prompt tells > that the driver has been registered. It looks like there is some kind of interference between the dom0 ttyLF0 driver and the Xen serial driver. Is your Xen UART driver marking the device as "used by Xen"? See for instance the pl011 driver, at the end of xen/drivers/char/pl011.c:pl011_dt_uart_init: dt_device_set_used_by(dev, DOMID_XEN); Devices that are marked as "used by Xen" are not exposed to dom0, so you shouldn't see the ttyLF0 device come up in Linux at all.
[qemu-mainline bisection] complete test-amd64-i386-xl-qemuu-dmrestrict-amd64-dmrestrict
branch xen-unstable xenbranch xen-unstable job test-amd64-i386-xl-qemuu-dmrestrict-amd64-dmrestrict testid debian-hvm-install Tree: linux git://xenbits.xen.org/linux-pvops.git Tree: linuxfirmware git://xenbits.xen.org/osstest/linux-firmware.git Tree: ovmf git://xenbits.xen.org/osstest/ovmf.git Tree: qemu git://xenbits.xen.org/qemu-xen-traditional.git Tree: qemuu git://git.qemu.org/qemu.git Tree: seabios git://xenbits.xen.org/osstest/seabios.git Tree: xen git://xenbits.xen.org/xen.git *** Found and reproduced problem changeset *** Bug is in tree: qemuu git://git.qemu.org/qemu.git Bug introduced: bd457782b3b0a313f3991038eb55bc44369c72c6 Bug not present: 9ad54686924b67ccf83698759ad0296ed5711bb8 Last fail repro: http://logs.test-lab.xenproject.org/osstest/logs/149255/ commit bd457782b3b0a313f3991038eb55bc44369c72c6 Author: Igor Mammedov Date: Wed Feb 19 11:09:17 2020 -0500 x86/pc: use memdev for RAM memory_region_allocate_system_memory() API is going away, so replace it with memdev allocated MemoryRegion. The later is initialized by generic code, so board only needs to opt in to memdev scheme by providing MachineClass::default_ram_id and using MachineState::ram instead of manually initializing RAM memory region. Signed-off-by: Igor Mammedov Reviewed-by: Michael S. Tsirkin Reviewed-by: Richard Henderson Message-Id: <20200219160953.13771-44-imamm...@redhat.com> For bisection revision-tuple graph see: http://logs.test-lab.xenproject.org/osstest/results/bisect/qemu-mainline/test-amd64-i386-xl-qemuu-dmrestrict-amd64-dmrestrict.debian-hvm-install.html Revision IDs in each graph node refer, respectively, to the Trees above. Running cs-bisection-step --graph-out=/home/logs/results/bisect/qemu-mainline/test-amd64-i386-xl-qemuu-dmrestrict-amd64-dmrestrict.debian-hvm-install --summary-out=tmp/149255.bisection-summary --basis-template=144861 --blessings=real,real-bisect qemu-mainline test-amd64-i386-xl-qemuu-dmrestrict-amd64-dmrestrict debian-hvm-install Searching for failure / basis pass: 149189 fail [host=albana1] / 147546 [host=elbling1] 147482 [host=fiano0] 147415 [host=fiano1] 147325 [host=huxelrebe0] 147241 [host=italia0] 147161 [host=rimava1] 147088 [host=chardonnay1] 147032 [host=debina1] 146978 [host=chardonnay0] 145664 [host=chardonnay0] 145649 [host=albana0] 145624 [host=huxelrebe0] 145592 [host=chardonnay1] 145573 ok. Failure / basis pass flights: 149189 / 145573 (tree with no url: minios) Tree: linux git://xenbits.xen.org/linux-pvops.git Tree: linuxfirmware git://xenbits.xen.org/osstest/linux-firmware.git Tree: ovmf git://xenbits.xen.org/osstest/ovmf.git Tree: qemu git://xenbits.xen.org/qemu-xen-traditional.git Tree: qemuu git://git.qemu.org/qemu.git Tree: seabios git://xenbits.xen.org/osstest/seabios.git Tree: xen git://xenbits.xen.org/xen.git Latest c3038e718a19fc596f7b1baba0f83d5146dc7784 c530a75c1e6a472b0eb9558310b518f0dfcd8860 6e9bd495b38e05ece5f53872df62d66052f29bb6 d0d8ad39ecb51cd7497cd524484fe09f50876798 5acad5bf480321f178866dc28e38eeda5a3f19bb 066a9956097b54530888b88ab9aa1ea02e42af5a e19b4b3b55f84e0cfcc02fe5d66965969a81c965 Basis pass b98aebd298246df37b472c52a2ee1023256d02e3 c530a75c1e6a472b0eb9558310b518f0dfcd8860 b948a496150f4ae4f656c0f0ab672608723c80e6 d0d8ad39ecb51cd7497cd524484fe09f50876798 f0dcfddecee8b860e015bb07d67cfcbdfbfd51d9 f21b5a4aeb020f2a5e2c6503f906a9349dd2f069 7b3c5b70a32303b46d0d051e695f18d72cce5ed0 Generating revisions with ./adhoc-revtuple-generator git://xenbits.xen.org/linux-pvops.git#b98aebd298246df37b472c52a2ee1023256d02e3-c3038e718a19fc596f7b1baba0f83d5146dc7784 git://xenbits.xen.org/osstest/linux-firmware.git#c530a75c1e6a472b0eb9558310b518f0dfcd8860-c530a75c1e6a472b0eb9558310b518f0dfcd8860 git://xenbits.xen.org/osstest/ovmf.git#b948a496150f4ae4f656c0f0ab672608723c80e6-6e9bd495b38e05ece5f53872df62d66052f29bb6 git://xenbits.xen.org/qemu-xen-traditional.git#d0d8ad39ecb51cd7497cd524484\ fe09f50876798-d0d8ad39ecb51cd7497cd524484fe09f50876798 git://git.qemu.org/qemu.git#f0dcfddecee8b860e015bb07d67cfcbdfbfd51d9-5acad5bf480321f178866dc28e38eeda5a3f19bb git://xenbits.xen.org/osstest/seabios.git#f21b5a4aeb020f2a5e2c6503f906a9349dd2f069-066a9956097b54530888b88ab9aa1ea02e42af5a git://xenbits.xen.org/xen.git#7b3c5b70a32303b46d0d051e695f18d72cce5ed0-e19b4b3b55f84e0cfcc02fe5d66965969a81c965 adhoc-revtuple-generator: tree discontiguous: linux-pvops Use of uninitialized value $parents in array dereference at ./adhoc-revtuple-generator line 465. Use of uninitialized value in concatenation (.) or string at ./adhoc-revtuple-generator line 465. Use of uninitialized value $parents in array dereference at ./adhoc-revtuple-generator line 465. Use of uninitialized value in concatenation (.) or string at ./adhoc-revtuple-generator line 465. Use of uninitialized value $parents in array dereference at
Re: [PATCH 1/8] xen/guest_access: Harden copy_to_guest_offset to prevent const dest operand
Hi Jan, On 31/03/2020 08:26, Jan Beulich wrote: On 30.03.2020 21:21, Julien Grall wrote: From: Julien Grall At the moment, copy_to_guest_offset() will allow the hypervisor to copy data to guest handle marked const. Thankfully, no users of the helper will do that. Rather than hoping this can be caught during review, harden copy_to_guest_offset() so the build will fail if such users are introduced. But there are other implications you break: --- a/xen/include/asm-arm/guest_access.h +++ b/xen/include/asm-arm/guest_access.h @@ -126,7 +126,7 @@ int access_guest_memory_by_ipa(struct domain *d, paddr_t ipa, void *buf, #define __copy_to_guest_offset(hnd, off, ptr, nr) ({\ const typeof(*(ptr)) *_s = (ptr); \ -char (*_d)[sizeof(*_s)] = (void *)(hnd).p; \ +typeof(*((hnd).p)) *_d = (hnd).p; \ ((void)((hnd).p == (ptr))); \ __raw_copy_to_guest(_d+(off), _s, sizeof(*_s)*(nr));\ Until this change, it is "ptr" which all sizes get derived from, i.e. it is the internally used type rather than the handle type which controls this. I'm sure we use this in a few places, to copy to e.g. a handle derived from "void". Compatibility of types (disallowing other than void) is checked by the comparison on the line immediately after the line you change. Yes "_d+(off)" right above here then changes its result. I consider it pretty likely you'd notice this issue once you go beyond just build testing. I missed that part. To be honest, it feels wrong to me to have "off" != 0 and use a void type for the handle. Would it make sense to forbid it? As a side node, I have updated __copy_to_guest_offset() but forgot to update copy_to_guest_offset(). I will look to apply the modifications we agree on both side. To address this, I guess we need to find an expression along the lines of that comparison, which does not cause any code to be generated, but which verifies the properties we care about. The line you change should be left alone, from all I can tell right now. I am not aware of any way before C11 to check if a variable is const or not. If we wanted to keep allow void type the handle then a possible approach would be: #define copy_to_guest_offset(hnd, off, ptr, nr) ({ \ const typeof(*(ptr)) *_s = (ptr); \ typeof(*((hnd).p)) *_d = (hnd).p; \ size_t mul = (sizeof(*(hnd).p) > 1) ? 1 : sizeof (*_s); \ ((void)((hnd).p == (ptr))); \ raw_copy_to_guest(_d + (off) * mul, _s, sizeof(*_s)*(nr)); \ }) I don't particularly like it but I could not come up with better so far. Cheers, -- Julien Grall
RE: [EXT] Re: [Xen-devel] Having a DOM-U guest with 1:1 mapping in the second stage MMU.
> On Thu, 13 Feb 2020, Andrei Cherechesu wrote: > > Hello, > > > > I used the Xen from Stefano's tree and made the updates to the partial > > dtb that he specified. > > > > > This is mostly likely because Linux is trying to access a region > > > that is not mapped in stage-2. You can rebuild Xen with debug > > > enabled and you should see a message "traps.c:..." telling the exact > > > physical address accessed. > > > > > > In general I would recommend to build Xen with debug enabled during > > > development as the hypervisor will give you more information of what's > > > going on. > > > > > > Cheers, > > > > > > -- > > > Julien Grall > > > > I enabled debug config and gave it another try. But I'm still getting > > the same unhandled fault error, that seems to match what Julien > > described above. > > > > It is indeed a stage-2 abort caused by the guest. > > > > I attached the DomU1 crash log at [0]. > > > > [0] > > > > > > How should I proceed in this case? > > Looking at the logs, you can see: > > (XEN) traps.c:1973:d1v0 HSR=0x939f0046 pc=0xff80083ac864 > gva=0xff800800d048 gpa=0x00402f0048 > > So the guest was accessing address 0x402f0048, however, the MMIO address > range of the device that you are remapping is 0x4002f000-0x4003. > > I spotted the mistake now: looking at the partial DTB again, the address of > the device is: > > reg = <0x0 0x402f 0x1000>; > > but the address that you are remapping is: > > xen,reg = <0x0 0x4002f000 0x1000 0x0 0x4002f000>; > > They are not the same! :-) Thanks, Stefano! I changed the partial DTB and it did not crash anymore. However, I am encountering another problem now: in Dom0 and in dom0less-booted DomUs, I cannot use /dev/hvc0. Even though I'm specifying "console=hvc0" in dom0-bootargs, when dom0 finishes booting, it looks like I cannot use the getty spawned on /dev/hvc0. This is the end of the boot log: [2.947845] random: rngd: uninitialized urandom read (4 bytes read) [2.958415] random: rngd: uninitialized urandom read (4 bytes read) [2.965452] random: rngd: uninitialized urandom read (2500 bytes read) . [2.972410] random: crng init done Starting OpenBSD Secure Shell server: sshd done. Starting /usr/sbin/xenstored... Setting domain 0 name, domid and JSON config... Done setting up Dom0 Starting xenconsoled... Starting QEMU as disk backend for dom0 Starting domain watchdog daemon: xenwatchdogd startup [done] Auto Linux BSP 1.0 s32g274aevb /dev/hvc0 s32g274aevb login: Auto Linux BSP 1.0 s32g274aevb /dev/ttyLF0 s32g274aevb login: - END - It seems that the getty spawned on /dev/ttyLF0 overwrites the one spawned on /dev/hvc0. Which I do not understand, since Dom0 should not have access (?) directly to ttyLF0 (the serial console device on our boards). If I remove the line which spawns the getty on ttyLF0 from /etc/inittab, the system hangs when waiting for the username, and it does not let me type in any characters. For the record, hvc0 is added to /etc/securetty. In a system where I boot DomU via xl from Dom0, I can switch to its console with xl console, and hvc0 works there. The problem that comes with this is that I can not use the CTRL-AAA command to switch between Dom0 console and DomU console in a dom0less case, and I cannot therefore test that the passthrough works. But at least Dom0 does not have an entry for it under /dev, anymore, and DomU boot prompt tells that the driver has been registered. Thank you very much again for your support, Andrei Cherechesu, NXP Semiconductors
Re: [PATCH v8 1/3] x86/tlb: introduce a flush HVM ASIDs flag
On Tue, Mar 31, 2020 at 05:40:59PM +0200, Jan Beulich wrote: > On 20.03.2020 19:42, Roger Pau Monne wrote: > > Introduce a specific flag to request a HVM guest linear TLB flush, > > which is an ASID/VPID tickle that forces a guest linear to guest > > physical TLB flush for all HVM guests. > > > > This was previously unconditionally done in each pre_flush call, but > > that's not required: HVM guests not using shadow don't require linear > > TLB flushes as Xen doesn't modify the guest page tables in that case > > (ie: when using HAP). Note that shadow paging code already takes care > > of issuing the necessary flushes when the shadow page tables are > > modified. > > > > In order to keep the previous behavior modify all shadow code TLB > > flushes to also flush the guest linear to physical TLB. I haven't > > looked at each specific shadow code TLB flush in order to figure out > > whether it actually requires a guest TLB flush or not, so there might > > be room for improvement in that regard. > > > > Also perform ASID/VPIT flushes when modifying the p2m tables as it's a > > requirement for AMD hardware. Finally keep the flush in > > switch_cr3_cr4, as it's not clear whether code could rely on > > switch_cr3_cr4 also performing a guest linear TLB flush. A following > > patch can remove the ASID/VPIT tickle from switch_cr3_cr4 if found to > > not be necessary. > > s/VPIT/VPID/ in this paragraph? Right, sorry. > > --- a/xen/arch/x86/mm/hap/hap.c > > +++ b/xen/arch/x86/mm/hap/hap.c > > @@ -118,7 +118,7 @@ int hap_track_dirty_vram(struct domain *d, > > p2m_change_type_range(d, begin_pfn, begin_pfn + nr, > >p2m_ram_rw, p2m_ram_logdirty); > > > > -flush_tlb_mask(d->dirty_cpumask); > > +flush_mask(d->dirty_cpumask, FLUSH_TLB | FLUSH_HVM_ASID_CORE); > > > > memset(dirty_bitmap, 0xff, size); /* consider all pages dirty > > */ > > } > > @@ -205,7 +205,7 @@ static int hap_enable_log_dirty(struct domain *d, > > bool_t log_global) > > * to be read-only, or via hardware-assisted log-dirty. > > */ > > p2m_change_entry_type_global(d, p2m_ram_rw, p2m_ram_logdirty); > > -flush_tlb_mask(d->dirty_cpumask); > > +flush_mask(d->dirty_cpumask, FLUSH_TLB | FLUSH_HVM_ASID_CORE); > > } > > return 0; > > } > > @@ -234,7 +234,7 @@ static void hap_clean_dirty_bitmap(struct domain *d) > > * be read-only, or via hardware-assisted log-dirty. > > */ > > p2m_change_entry_type_global(d, p2m_ram_rw, p2m_ram_logdirty); > > -flush_tlb_mask(d->dirty_cpumask); > > +flush_mask(d->dirty_cpumask, FLUSH_TLB | FLUSH_HVM_ASID_CORE); > > } > > > > // > > @@ -798,7 +798,7 @@ hap_write_p2m_entry(struct p2m_domain *p2m, unsigned > > long gfn, l1_pgentry_t *p, > > > > safe_write_pte(p, new); > > if ( old_flags & _PAGE_PRESENT ) > > -flush_tlb_mask(d->dirty_cpumask); > > +flush_mask(d->dirty_cpumask, FLUSH_TLB | FLUSH_HVM_ASID_CORE); > > For all four - why FLUSH_TLB? Doesn't the flushing here solely care > about guest translations? Not on AMD, at least to my understanding, the AMD SDM states: "If a hypervisor modifies a nested page table by decreasing permission levels, clearing present bits, or changing address translations and intends to return to the same ASID, it should use either TLB command 011b or 001b." It's in section 15.16.1. This to my understanding implies that on AMD hardware modifications to the NPT require an ASID flush. I assume that on AMD ASIDs also cache combined translations, guest linear -> host physical. In fact without doing such flushes when modifying the nested page tables XenRT was seeing multiple issues on AMD hardware. > > --- a/xen/arch/x86/mm/hap/nested_hap.c > > +++ b/xen/arch/x86/mm/hap/nested_hap.c > > @@ -84,7 +84,7 @@ nestedp2m_write_p2m_entry(struct p2m_domain *p2m, > > unsigned long gfn, > > safe_write_pte(p, new); > > > > if (old_flags & _PAGE_PRESENT) > > -flush_tlb_mask(p2m->dirty_cpumask); > > +flush_mask(p2m->dirty_cpumask, FLUSH_TLB | FLUSH_HVM_ASID_CORE); > > Same here then I guess. > > > --- a/xen/arch/x86/mm/p2m-pt.c > > +++ b/xen/arch/x86/mm/p2m-pt.c > > @@ -896,7 +896,8 @@ static void p2m_pt_change_entry_type_global(struct > > p2m_domain *p2m, > > unmap_domain_page(tab); > > > > if ( changed ) > > - flush_tlb_mask(p2m->domain->dirty_cpumask); > > + flush_mask(p2m->domain->dirty_cpumask, > > +FLUSH_TLB | FLUSH_HVM_ASID_CORE); > > Given that this code is used in shadow mode as well, perhaps > better to keep it here. Albeit maybe FLUSH_TLB could be dependent > upon !hap_enabled()? > > > --- a/xen/arch/x86/mm/paging.c > > +++ b/xen/arch/x86/mm/paging.c > > @@ -613,7 +613,7 @@ void paging_log_dirty_range(struct domain *d, > > > > p2m_unlock(p2m); > > > > -
[OSSTEST PATCH v2 1/2] host props/flags: Break out blessing_must_not_modify_host
This return value convention is convenient for the callers, since they want to print a message when we mayn't modify. Signed-off-by: Ian Jackson --- Osstest.pm | 10 +- Osstest/HostDB/Executive.pm | 4 ++-- ts-examine-hostprops-save | 4 ++-- 3 files changed, 13 insertions(+), 5 deletions(-) diff --git a/Osstest.pm b/Osstest.pm index 27136bc9..54f0085c 100644 --- a/Osstest.pm +++ b/Osstest.pm @@ -40,7 +40,7 @@ BEGIN { $dbh_tests db_retry db_retry_retry db_retry_abort db_readonly_report db_begin_work db_prepare - get_harness_rev + get_harness_rev blessing_must_not_modify_host ensuredir get_filecontents_core_quiet system_checked nonempty visible_undef show_abs_time %arch_debian2xen %arch_xen2debian @@ -407,6 +407,14 @@ sub get_harness_rev () { return $rev; } +sub blessing_must_not_modify_host ($) { +# returns '' (falseish) if we may modify a host's props etc., +# or a trueish string message if may not +my ($intended_blessing) = @_; +return '' if $intended_blessing eq "real"; +return "intended blessing $intended_blessing != real"; +} + sub get_filecontents_core_quiet ($) { # ENOENT => undef my ($path) = @_; if (!open GFC, '<', $path) { diff --git a/Osstest/HostDB/Executive.pm b/Osstest/HostDB/Executive.pm index a6dc4462..41cce046 100644 --- a/Osstest/HostDB/Executive.pm +++ b/Osstest/HostDB/Executive.pm @@ -55,8 +55,8 @@ sub modify_host ($$$) { my ($hd, $ho, $query) = @_; my $blessing = intended_blessing(); -die "Attempting to modify host with intended blessing $blessing != real" -if $blessing ne "real"; +my $wrong = blessing_must_not_modify_host($blessing); +die "Attempting to modify host but $wrong" if $wrong; db_retry($dbh_tests, [qw(resources)], $query); } diff --git a/ts-examine-hostprops-save b/ts-examine-hostprops-save index e50ea7fb..0c39b864 100755 --- a/ts-examine-hostprops-save +++ b/ts-examine-hostprops-save @@ -31,8 +31,8 @@ logm("setting host properties and flags"); # NB: in order to aid debug only attempt to save the host props on flights # with intended real blessing, for the rest just do a dry run. -our $dry_run = $blessing ne "real"; -logm("not saving host props/flags with intended blessing $blessing != real") +our $dry_run = blessing_must_not_modify_host($blessing); +logm("not saving host props/flags ($dry_run)") if $dry_run; foreach my $k (sort keys %r) { -- 2.11.0
[OSSTEST PATCH v2 2/2] host props/flags: Save for commissioning flights too
Obviously we want this! Signed-off-by: Ian Jackson --- Osstest.pm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Osstest.pm b/Osstest.pm index 54f0085c..1e381d8f 100644 --- a/Osstest.pm +++ b/Osstest.pm @@ -411,7 +411,7 @@ sub blessing_must_not_modify_host ($) { # returns '' (falseish) if we may modify a host's props etc., # or a trueish string message if may not my ($intended_blessing) = @_; -return '' if $intended_blessing eq "real"; +return '' if $intended_blessing =~ qr{^real$|^commission-}; return "intended blessing $intended_blessing != real"; } -- 2.11.0
[PATCH] x86/dom0: fix copy of low 1MB data for PVH
The orders of start and end are inverted in order to calculate the size of the copy operation. Signed-off-by: Roger Pau Monné --- xen/arch/x86/hvm/dom0_build.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c index 2afd44c8a4..12a82c9d7c 100644 --- a/xen/arch/x86/hvm/dom0_build.c +++ b/xen/arch/x86/hvm/dom0_build.c @@ -463,7 +463,7 @@ static int __init pvh_populate_p2m(struct domain *d) enum hvm_translation_result res = hvm_copy_to_guest_phys(mfn_to_maddr(_mfn(addr)), mfn_to_virt(addr), -d->arch.e820[i].addr - end, +end - d->arch.e820[i].addr, v); if ( res != HVMTRANS_okay ) -- 2.26.0
Re: [PATCH v2 2/2] x86emul: support SYSRET
On 31/03/2020 16:58, Jan Beulich wrote: > This is to augment SYSCALL, which we've been supporting for quite some > time. > > Signed-off-by: Jan Beulich Reviewed-by: Andrew Cooper In some copious free time I'll see about finishing off my XTF test for these cases, but that will have to wait for now.
Re: [PATCH v2 1/2] x86emul: vendor specific SYSCALL behavior
On 31/03/2020 16:58, Jan Beulich wrote: > AMD CPUs permit the insn everywhere (even outside of protected mode), > while Intel ones restrict it to 64-bit mode. While at it also comment > about the apparently missing CPUID bit check. > > Signed-off-by: Jan Beulich Reviewed-by: Andrew Cooper
Re: [PATCH 11/11] x86/ucode/amd: Rework parsing logic in cpu_request_microcode()
On 31.03.2020 17:55, Andrew Cooper wrote: > On 31/03/2020 16:27, Jan Beulich wrote: >> On 31.03.2020 17:19, Andrew Cooper wrote: >>> On 31/03/2020 16:07, Jan Beulich wrote: On 31.03.2020 12:05, Andrew Cooper wrote: > @@ -269,55 +265,25 @@ static int apply_microcode(const struct > microcode_patch *patch) > return 0; > } > > -static int scan_equiv_cpu_table( > -const void *data, > -size_t size_left, > -size_t *offset) > +static int scan_equiv_cpu_table(const struct container_equiv_table *et) > { > const struct cpu_signature *sig = &this_cpu(cpu_sig); > -const struct mpbhdr *mpbuf; > -const struct equiv_cpu_entry *eq; > -unsigned int i, nr; > - > -if ( size_left < (sizeof(*mpbuf) + 4) || > - (mpbuf = data + *offset + 4, > - size_left - sizeof(*mpbuf) - 4 < mpbuf->len) ) > -{ > -printk(XENLOG_WARNING "microcode: No space for equivalent cpu > table\n"); > -return -EINVAL; > -} > - > -*offset += mpbuf->len + CONT_HDR_SIZE; /* add header length */ > - > -if ( mpbuf->type != UCODE_EQUIV_CPU_TABLE_TYPE ) > -{ > -printk(KERN_ERR "microcode: Wrong microcode equivalent cpu table > type field\n"); > -return -EINVAL; > -} > - > -if ( mpbuf->len == 0 || mpbuf->len % sizeof(*eq) || > - (eq = (const void *)mpbuf->data, > - nr = mpbuf->len / sizeof(*eq), > - eq[nr - 1].installed_cpu) ) Did this last check get lost? I can't seem to be able to identify any possible replacement. >>> Given the lack of a spec, I'm unsure whether to keep it or not. >>> >>> It is necessary in the backport of patch 1, because find_equiv_cpu_id() >>> doesn't have mpbuf->len to hand, and relies on the sentinel to find the >>> end of the table. >>> >>> OTOH, the new logic will cope perfectly well without a sentinel. >> Okay. >> > static struct microcode_patch *cpu_request_microcode(const void *buf, > size_t size) > { > const struct microcode_patch *saved = NULL; > struct microcode_patch *patch = NULL; > -size_t offset = 0, saved_size = 0; > +size_t saved_size = 0; > int error = 0; > -unsigned int cpu = smp_processor_id(); > -const struct cpu_signature *sig = &per_cpu(cpu_sig, cpu); > > -if ( size < 4 || > - *(const uint32_t *)buf != UCODE_MAGIC ) > +while ( size ) > { > -printk(KERN_ERR "microcode: Wrong microcode patch file magic\n"); > -error = -EINVAL; > -goto out; > -} > - > -/* > - * Multiple container file support: > - * 1. check if this container file has equiv_cpu_id match > - * 2. If not, fast-fwd to next container file > - */ > -while ( offset < size ) > -{ > -error = scan_equiv_cpu_table(buf, size - offset, &offset); > - > -if ( !error || error != -ESRCH ) > -break; > +const struct container_equiv_table *et; > +bool skip_ucode; > > -error = container_fast_forward(buf, size - offset, &offset); > -if ( error == -ENODATA ) > +if ( size < 4 || *(const uint32_t *)buf != UCODE_MAGIC ) > { > -ASSERT(offset == size); > +printk(XENLOG_ERR "microcode: Wrong microcode patch file > magic\n"); > +error = -EINVAL; > break; > } > -if ( error ) > + > +/* Move over UCODE_MAGIC. */ > +buf += 4; > +size -= 4; > + > +if ( size < sizeof(*et) || > + (et = buf)->type != UCODE_EQUIV_CPU_TABLE_TYPE || > + size - sizeof(*et) < et->len || > + et->len % sizeof(et->eq[0]) ) > { > -printk(KERN_ERR "microcode: CPU%d incorrect or corrupt > container file\n" > - "microcode: Failed to update patch level. " > - "Current lvl:%#x\n", cpu, sig->rev); > +printk(XENLOG_ERR "microcode: Bad equivalent cpu table\n"); > +error = -EINVAL; > break; > } > -} > > -if ( error ) > -{ > -/* > - * -ENODATA here means that the blob was parsed fine but no > matching > - * ucode was found. Don't return it to the caller. > - */ > -if ( error == -ENODATA ) > -error = 0; > - > -goto out; > -} > +/* Move over the Equiv table. */ > +buf += sizeof(*et) + et->len; > +size -= sizeof(*et) + et->len; > + > +error = scan_equiv_cpu_tabl
[PATCH v2 2/2] x86emul: support SYSRET
This is to augment SYSCALL, which we've been supporting for quite some time. Signed-off-by: Jan Beulich --- v2: Replace CPUID bit check by comment. Limit RCX based canonical check to just Intel as vendor. Update SS selector on AMD and alike. --- a/xen/arch/x86/x86_emulate/x86_emulate.c +++ b/xen/arch/x86/x86_emulate/x86_emulate.c @@ -5977,6 +5977,82 @@ x86_emulate( goto done; break; +case X86EMUL_OPC(0x0f, 0x07): /* sysret */ +/* + * Inject #UD if syscall/sysret are disabled. EFER.SCE can't be set + * with the respective CPUID bit clear, so no need for an explicit + * check of that one. + */ +fail_if(!ops->read_msr); +if ( (rc = ops->read_msr(MSR_EFER, &msr_val, ctxt)) != X86EMUL_OKAY ) +goto done; +generate_exception_if(!(msr_val & EFER_SCE), EXC_UD); +generate_exception_if(!amd_like(ctxt) && !mode_64bit(), EXC_UD); +generate_exception_if(!mode_ring0(), EXC_GP, 0); +generate_exception_if(!in_protmode(ctxt, ops), EXC_GP, 0); +#ifdef __x86_64__ +/* + * Doing this for just Intel (rather than e.g. !amd_like()) as this is + * in fact risking to make guest OSes vulnerable to the equivalent of + * XSA-7 (CVE-2012-0217). + */ +generate_exception_if(ctxt->cpuid->x86_vendor == X86_VENDOR_INTEL && + op_bytes == 8 && !is_canonical_address(_regs.rcx), + EXC_GP, 0); +#endif + +if ( (rc = ops->read_msr(MSR_STAR, &msr_val, ctxt)) != X86EMUL_OKAY ) +goto done; + +sreg.sel = ((msr_val >> 48) + 8) | 3; /* SELECTOR_RPL_MASK */ +cs.sel = op_bytes == 8 ? sreg.sel + 8 : sreg.sel - 8; + +cs.base = sreg.base = 0; /* flat segment */ +cs.limit = sreg.limit = ~0u; /* 4GB limit */ +cs.attr = 0xcfb; /* G+DB+P+DPL3+S+Code */ +sreg.attr = 0xcf3; /* G+DB+P+DPL3+S+Data */ + +/* Only the selector part of SS gets updated by AMD and alike. */ +if ( amd_like(ctxt) ) +{ +fail_if(!ops->read_segment); +if ( (rc = ops->read_segment(x86_seg_ss, &sreg, + ctxt)) != X86EMUL_OKAY ) +goto done; + +/* There's explicitly no RPL adjustment here. */ +sreg.sel = (msr_val >> 48) + 8; +} + +#ifdef __x86_64__ +if ( mode_64bit() ) +{ +if ( op_bytes == 8 ) +{ +cs.attr = 0xafb; /* L+DB+P+DPL3+S+Code */ +_regs.rip = _regs.rcx; +} +else +_regs.rip = _regs.ecx; + +_regs.eflags = _regs.r11 & ~(X86_EFLAGS_RF | X86_EFLAGS_VM); +} +else +#endif +{ +_regs.r(ip) = _regs.ecx; +_regs.eflags |= X86_EFLAGS_IF; +} + +fail_if(!ops->write_segment); +if ( (rc = ops->write_segment(x86_seg_cs, &cs, ctxt)) != X86EMUL_OKAY || + (rc = ops->write_segment(x86_seg_ss, &sreg, + ctxt)) != X86EMUL_OKAY ) +goto done; + +singlestep = _regs.eflags & X86_EFLAGS_TF; +break; + case X86EMUL_OPC(0x0f, 0x08): /* invd */ case X86EMUL_OPC(0x0f, 0x09): /* wbinvd / wbnoinvd */ generate_exception_if(!mode_ring0(), EXC_GP, 0);
[PATCH v2 1/2] x86emul: vendor specific SYSCALL behavior
AMD CPUs permit the insn everywhere (even outside of protected mode), while Intel ones restrict it to 64-bit mode. While at it also comment about the apparently missing CPUID bit check. Signed-off-by: Jan Beulich --- v2: Replace CPUID bit check by comment. --- a/xen/arch/x86/x86_emulate/x86_emulate.c +++ b/xen/arch/x86/x86_emulate/x86_emulate.c @@ -5897,13 +5897,16 @@ x86_emulate( break; case X86EMUL_OPC(0x0f, 0x05): /* syscall */ -generate_exception_if(!in_protmode(ctxt, ops), EXC_UD); - -/* Inject #UD if syscall/sysret are disabled. */ +/* + * Inject #UD if syscall/sysret are disabled. EFER.SCE can't be set + * with the respective CPUID bit clear, so no need for an explicit + * check of that one. + */ fail_if(ops->read_msr == NULL); if ( (rc = ops->read_msr(MSR_EFER, &msr_val, ctxt)) != X86EMUL_OKAY ) goto done; generate_exception_if((msr_val & EFER_SCE) == 0, EXC_UD); +generate_exception_if(!amd_like(ctxt) && !mode_64bit(), EXC_UD); if ( (rc = ops->read_msr(MSR_STAR, &msr_val, ctxt)) != X86EMUL_OKAY ) goto done;
[PATCH v2 0/2] x86emul: (mainly) vendor specific behavior adjustments
Just the remaining two pieces of the original series. 1: vendor specific SYSCALL behavior 2: support SYSRET Jan
Re: [PATCH 11/11] x86/ucode/amd: Rework parsing logic in cpu_request_microcode()
On 31/03/2020 16:27, Jan Beulich wrote: > On 31.03.2020 17:19, Andrew Cooper wrote: >> On 31/03/2020 16:07, Jan Beulich wrote: >>> On 31.03.2020 12:05, Andrew Cooper wrote: @@ -269,55 +265,25 @@ static int apply_microcode(const struct microcode_patch *patch) return 0; } -static int scan_equiv_cpu_table( -const void *data, -size_t size_left, -size_t *offset) +static int scan_equiv_cpu_table(const struct container_equiv_table *et) { const struct cpu_signature *sig = &this_cpu(cpu_sig); -const struct mpbhdr *mpbuf; -const struct equiv_cpu_entry *eq; -unsigned int i, nr; - -if ( size_left < (sizeof(*mpbuf) + 4) || - (mpbuf = data + *offset + 4, - size_left - sizeof(*mpbuf) - 4 < mpbuf->len) ) -{ -printk(XENLOG_WARNING "microcode: No space for equivalent cpu table\n"); -return -EINVAL; -} - -*offset += mpbuf->len + CONT_HDR_SIZE;/* add header length */ - -if ( mpbuf->type != UCODE_EQUIV_CPU_TABLE_TYPE ) -{ -printk(KERN_ERR "microcode: Wrong microcode equivalent cpu table type field\n"); -return -EINVAL; -} - -if ( mpbuf->len == 0 || mpbuf->len % sizeof(*eq) || - (eq = (const void *)mpbuf->data, - nr = mpbuf->len / sizeof(*eq), - eq[nr - 1].installed_cpu) ) >>> Did this last check get lost? I can't seem to be able to identify >>> any possible replacement. >> Given the lack of a spec, I'm unsure whether to keep it or not. >> >> It is necessary in the backport of patch 1, because find_equiv_cpu_id() >> doesn't have mpbuf->len to hand, and relies on the sentinel to find the >> end of the table. >> >> OTOH, the new logic will cope perfectly well without a sentinel. > Okay. > static struct microcode_patch *cpu_request_microcode(const void *buf, size_t size) { const struct microcode_patch *saved = NULL; struct microcode_patch *patch = NULL; -size_t offset = 0, saved_size = 0; +size_t saved_size = 0; int error = 0; -unsigned int cpu = smp_processor_id(); -const struct cpu_signature *sig = &per_cpu(cpu_sig, cpu); -if ( size < 4 || - *(const uint32_t *)buf != UCODE_MAGIC ) +while ( size ) { -printk(KERN_ERR "microcode: Wrong microcode patch file magic\n"); -error = -EINVAL; -goto out; -} - -/* - * Multiple container file support: - * 1. check if this container file has equiv_cpu_id match - * 2. If not, fast-fwd to next container file - */ -while ( offset < size ) -{ -error = scan_equiv_cpu_table(buf, size - offset, &offset); - -if ( !error || error != -ESRCH ) -break; +const struct container_equiv_table *et; +bool skip_ucode; -error = container_fast_forward(buf, size - offset, &offset); -if ( error == -ENODATA ) +if ( size < 4 || *(const uint32_t *)buf != UCODE_MAGIC ) { -ASSERT(offset == size); +printk(XENLOG_ERR "microcode: Wrong microcode patch file magic\n"); +error = -EINVAL; break; } -if ( error ) + +/* Move over UCODE_MAGIC. */ +buf += 4; +size -= 4; + +if ( size < sizeof(*et) || + (et = buf)->type != UCODE_EQUIV_CPU_TABLE_TYPE || + size - sizeof(*et) < et->len || + et->len % sizeof(et->eq[0]) ) { -printk(KERN_ERR "microcode: CPU%d incorrect or corrupt container file\n" - "microcode: Failed to update patch level. " - "Current lvl:%#x\n", cpu, sig->rev); +printk(XENLOG_ERR "microcode: Bad equivalent cpu table\n"); +error = -EINVAL; break; } -} -if ( error ) -{ -/* - * -ENODATA here means that the blob was parsed fine but no matching - * ucode was found. Don't return it to the caller. - */ -if ( error == -ENODATA ) -error = 0; - -goto out; -} +/* Move over the Equiv table. */ +buf += sizeof(*et) + et->len; +size -= sizeof(*et) + et->len; + +error = scan_equiv_cpu_table(et); +if ( error && error != -ESRCH ) +break; >>> With this the only non-zero value left for error is -ESRCH. >>> Hence ... >>> +/*
Re: [PATCH 09/11] x86/ucode/amd: Remove gratuitous memory allocations from cpu_request_microcode()
On 31.03.2020 17:47, Andrew Cooper wrote: > On 31/03/2020 16:13, Jan Beulich wrote: >> On 31.03.2020 16:55, Andrew Cooper wrote: >>> On 31/03/2020 15:51, Jan Beulich wrote: On 31.03.2020 12:05, Andrew Cooper wrote: > @@ -497,57 +456,54 @@ static struct microcode_patch > *cpu_request_microcode(const void *buf, size_t siz > * It's possible the data file has multiple matching ucode, > * lets keep searching till the latest version > */ > -while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, size, > - &offset)) == 0 ) > +buf += offset; > +size -= offset; > { > -/* > - * If the new ucode covers current CPU, compare ucodes and store > the > - * one with higher revision. > - */ > -if ( (microcode_fits(mc_amd->mpb) != MIS_UCODE) && > - (!saved || (compare_header(mc_amd->mpb, saved) == > NEW_UCODE)) ) > +while ( size ) > { > -xfree(saved); > -saved = mc_amd->mpb; > -} > -else > -{ > -xfree(mc_amd->mpb); > -mc_amd->mpb = NULL; > -} > +const struct container_microcode *mc; > + > +if ( size < sizeof(*mc) || > + (mc = buf)->type != UCODE_UCODE_TYPE || > + size - sizeof(*mc) < mc->len || > + !verify_patch_size(mc->len) ) > +{ > +printk(XENLOG_ERR "microcode: Bad microcode data\n"); > +error = -EINVAL; > +break; > +} > > -if ( offset >= size ) > -break; > +/* > + * If the new ucode covers current CPU, compare ucodes and > store the > + * one with higher revision. > + */ > +if ( (microcode_fits(mc->patch) != MIS_UCODE) && > + (!saved || (compare_header(mc->patch, saved) == > NEW_UCODE)) ) > +{ > +saved = mc->patch; > +saved_size = mc->len; > +} > > -/* > - * 1. Given a situation where multiple containers exist and > correct > - *patch lives on a container that is not the last container. > - * 2. We match equivalent ids using find_equiv_cpu_id() from the > - *earlier while() (On this case, matches on earlier container > - *file and we break) > - * 3. Proceed to while ( (error = > get_ucode_from_buffer_amd(mc_amd, > - * buf, size, &offset)) == 0 ) > - * 4. Find correct patch using microcode_fits() and apply the > patch > - *(Assume: apply_microcode() is successful) > - * 5. The while() loop from (3) continues to parse the binary as > - *there is a subsequent container file, but... > - * 6. ...a correct patch can only be on one container and not on > any > - *subsequent ones. (Refer docs for more info) Therefore, we > - *don't have to parse a subsequent container. So, we can > abort > - *the process here. > - * 7. This ensures that we retain a success value (= 0) to > 'error' > - *before if ( mpbuf->type != UCODE_UCODE_TYPE ) evaluates to > - *false and returns -EINVAL. > - */ > -if ( offset + SECTION_HDR_SIZE <= size && > - *(const uint32_t *)(buf + offset) == UCODE_MAGIC ) > -break; > +/* Move over the microcode blob. */ > +buf += sizeof(*mc) + mc->len; > +size -= sizeof(*mc) + mc->len; > + > +/* > + * Peek ahead. If we see the start of another container, > we've > + * exhaused all microcode blobs in this container. Exit > cleanly. > + */ > +if ( size >= 4 && *(const uint32_t *)buf == UCODE_MAGIC ) > +break; While, as already indicated, I agree with shrinking the big comment, I think point 6 is what wants retaining in some form - it's not obvious at all why a subsequent container couldn't contain a higher rev ucode than what we've found. That comment refers us to docs, but I couldn't find anything to this effect in PM Vol 2. Assuming this indeed documented and true, with the comment extended accordingly Reviewed-by: Jan Beulich >>> I think it is referring to the internal PPR, which isn't even the one we >>> have access to. >>> >>> As to the multiple containers aspect, I've deliberately "fixed" that
Re: [PATCH 09/11] x86/ucode/amd: Remove gratuitous memory allocations from cpu_request_microcode()
On 31/03/2020 16:13, Jan Beulich wrote: > On 31.03.2020 16:55, Andrew Cooper wrote: >> On 31/03/2020 15:51, Jan Beulich wrote: >>> On 31.03.2020 12:05, Andrew Cooper wrote: @@ -497,57 +456,54 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, size_t siz * It's possible the data file has multiple matching ucode, * lets keep searching till the latest version */ -while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, size, - &offset)) == 0 ) +buf += offset; +size -= offset; { -/* - * If the new ucode covers current CPU, compare ucodes and store the - * one with higher revision. - */ -if ( (microcode_fits(mc_amd->mpb) != MIS_UCODE) && - (!saved || (compare_header(mc_amd->mpb, saved) == NEW_UCODE)) ) +while ( size ) { -xfree(saved); -saved = mc_amd->mpb; -} -else -{ -xfree(mc_amd->mpb); -mc_amd->mpb = NULL; -} +const struct container_microcode *mc; + +if ( size < sizeof(*mc) || + (mc = buf)->type != UCODE_UCODE_TYPE || + size - sizeof(*mc) < mc->len || + !verify_patch_size(mc->len) ) +{ +printk(XENLOG_ERR "microcode: Bad microcode data\n"); +error = -EINVAL; +break; +} -if ( offset >= size ) -break; +/* + * If the new ucode covers current CPU, compare ucodes and store the + * one with higher revision. + */ +if ( (microcode_fits(mc->patch) != MIS_UCODE) && + (!saved || (compare_header(mc->patch, saved) == NEW_UCODE)) ) +{ +saved = mc->patch; +saved_size = mc->len; +} -/* - * 1. Given a situation where multiple containers exist and correct - *patch lives on a container that is not the last container. - * 2. We match equivalent ids using find_equiv_cpu_id() from the - *earlier while() (On this case, matches on earlier container - *file and we break) - * 3. Proceed to while ( (error = get_ucode_from_buffer_amd(mc_amd, - * buf, size, &offset)) == 0 ) - * 4. Find correct patch using microcode_fits() and apply the patch - *(Assume: apply_microcode() is successful) - * 5. The while() loop from (3) continues to parse the binary as - *there is a subsequent container file, but... - * 6. ...a correct patch can only be on one container and not on any - *subsequent ones. (Refer docs for more info) Therefore, we - *don't have to parse a subsequent container. So, we can abort - *the process here. - * 7. This ensures that we retain a success value (= 0) to 'error' - *before if ( mpbuf->type != UCODE_UCODE_TYPE ) evaluates to - *false and returns -EINVAL. - */ -if ( offset + SECTION_HDR_SIZE <= size && - *(const uint32_t *)(buf + offset) == UCODE_MAGIC ) -break; +/* Move over the microcode blob. */ +buf += sizeof(*mc) + mc->len; +size -= sizeof(*mc) + mc->len; + +/* + * Peek ahead. If we see the start of another container, we've + * exhaused all microcode blobs in this container. Exit cleanly. + */ +if ( size >= 4 && *(const uint32_t *)buf == UCODE_MAGIC ) +break; >>> While, as already indicated, I agree with shrinking the big comment, >>> I think point 6 is what wants retaining in some form - it's not >>> obvious at all why a subsequent container couldn't contain a higher >>> rev ucode than what we've found. That comment refers us to docs, but >>> I couldn't find anything to this effect in PM Vol 2. Assuming this >>> indeed documented and true, with the comment extended accordingly >>> Reviewed-by: Jan Beulich >> I think it is referring to the internal PPR, which isn't even the one we >> have access to. >> >> As to the multiple containers aspect, I've deliberately "fixed" that in >> patch 11 so we do scan all the way to the end. > Right, meanwhile I've seen this. But shouldn't patch 11 then adjust at > least the "Exit cleanly" part of the
[libvirt test] 149234: regressions - FAIL
flight 149234 libvirt real [real] http://logs.test-lab.xenproject.org/osstest/logs/149234/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-amd64-libvirt 6 libvirt-buildfail REGR. vs. 146182 build-i386-libvirt6 libvirt-buildfail REGR. vs. 146182 build-arm64-libvirt 6 libvirt-buildfail REGR. vs. 146182 build-armhf-libvirt 6 libvirt-buildfail REGR. vs. 146182 Tests which did not succeed, but are not blocking: test-armhf-armhf-libvirt 1 build-check(1) blocked n/a test-armhf-armhf-libvirt-raw 1 build-check(1) blocked n/a test-amd64-i386-libvirt 1 build-check(1) blocked n/a test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 1 build-check(1) blocked n/a test-amd64-amd64-libvirt 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 1 build-check(1) blocked n/a test-amd64-i386-libvirt-pair 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-pair 1 build-check(1) blocked n/a test-amd64-i386-libvirt-xsm 1 build-check(1) blocked n/a test-arm64-arm64-libvirt 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-vhd 1 build-check(1) blocked n/a test-arm64-arm64-libvirt-xsm 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-xsm 1 build-check(1) blocked n/a test-arm64-arm64-libvirt-qcow2 1 build-check(1) blocked n/a version targeted for testing: libvirt 93f775eaa32ef63df5d07eb6a2c2193ca6d936ac baseline version: libvirt a1cd25b919509be2645dbe6f952d5263e0d4e4e5 Last test of basis 146182 2020-01-17 06:00:23 Z 74 days Failing since146211 2020-01-18 04:18:52 Z 73 days 70 attempts Testing same since 149234 2020-03-31 04:18:52 Z0 days1 attempts People who touched revisions under test: Andrea Bolognani Arnaud Patard Boris Fiuczynski Christian Ehrhardt Christian Schoenebeck Collin Walling Daniel Henrique Barboza Daniel P. Berrangé Daniel Veillard Dario Faggioli Erik Skultety Gaurav Agrawal Han Han Jim Fehlig Jiri Denemark Jonathon Jongsma Julio Faracco Ján Tomko Laine Stump Lin Ma Marc-André Lureau Marek Marczykowski-Górecki Mauro S. M. Rodrigues Michal Privoznik Nikolay Shirokovskiy Pavel Hrdina Pavel Mores Peter Krempa Pino Toscano Rafael Fonseca Richard W.M. Jones Rikard Falkeborn Ryan Moeller Sahid Orentino Ferdjaoui Sebastian Mitterle Seeteena Thoufeek Stefan Berger Stefan Berger Stefan Hajnoczi Thomas Huth Wu Qingliang Your Name Zhang Bo zhenwei pi Zhimin Feng jobs: build-amd64-xsm pass build-arm64-xsm pass build-i386-xsm pass build-amd64 pass build-arm64 pass build-armhf pass build-i386 pass build-amd64-libvirt fail build-arm64-libvirt fail build-armhf-libvirt fail build-i386-libvirt fail build-amd64-pvopspass build-arm64-pvopspass build-armhf-pvopspass build-i386-pvops pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm blocked test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsmblocked test-amd64-amd64-libvirt-xsm blocked test-arm64-arm64-libvirt-xsm blocked test-amd64-i386-libvirt-xsm blocked test-amd64-amd64-libvirt blocked test-arm64-arm64-libvirt blocked test-armhf-armhf-libvirt blocked test-amd64-i386-libvirt blocked test-amd64-amd64-libvirt-pairblocked test-amd64-i386-libvirt-pair blocked test-arm64-arm64-libvirt-qcow2 blocked test-armhf-armhf-libvirt-raw blocked test-amd64-amd64-libvirt-vhd
Re: [PATCH v8 1/3] x86/tlb: introduce a flush HVM ASIDs flag
On 20.03.2020 19:42, Roger Pau Monne wrote: > Introduce a specific flag to request a HVM guest linear TLB flush, > which is an ASID/VPID tickle that forces a guest linear to guest > physical TLB flush for all HVM guests. > > This was previously unconditionally done in each pre_flush call, but > that's not required: HVM guests not using shadow don't require linear > TLB flushes as Xen doesn't modify the guest page tables in that case > (ie: when using HAP). Note that shadow paging code already takes care > of issuing the necessary flushes when the shadow page tables are > modified. > > In order to keep the previous behavior modify all shadow code TLB > flushes to also flush the guest linear to physical TLB. I haven't > looked at each specific shadow code TLB flush in order to figure out > whether it actually requires a guest TLB flush or not, so there might > be room for improvement in that regard. > > Also perform ASID/VPIT flushes when modifying the p2m tables as it's a > requirement for AMD hardware. Finally keep the flush in > switch_cr3_cr4, as it's not clear whether code could rely on > switch_cr3_cr4 also performing a guest linear TLB flush. A following > patch can remove the ASID/VPIT tickle from switch_cr3_cr4 if found to > not be necessary. s/VPIT/VPID/ in this paragraph? > --- a/xen/arch/x86/mm/hap/hap.c > +++ b/xen/arch/x86/mm/hap/hap.c > @@ -118,7 +118,7 @@ int hap_track_dirty_vram(struct domain *d, > p2m_change_type_range(d, begin_pfn, begin_pfn + nr, >p2m_ram_rw, p2m_ram_logdirty); > > -flush_tlb_mask(d->dirty_cpumask); > +flush_mask(d->dirty_cpumask, FLUSH_TLB | FLUSH_HVM_ASID_CORE); > > memset(dirty_bitmap, 0xff, size); /* consider all pages dirty */ > } > @@ -205,7 +205,7 @@ static int hap_enable_log_dirty(struct domain *d, bool_t > log_global) > * to be read-only, or via hardware-assisted log-dirty. > */ > p2m_change_entry_type_global(d, p2m_ram_rw, p2m_ram_logdirty); > -flush_tlb_mask(d->dirty_cpumask); > +flush_mask(d->dirty_cpumask, FLUSH_TLB | FLUSH_HVM_ASID_CORE); > } > return 0; > } > @@ -234,7 +234,7 @@ static void hap_clean_dirty_bitmap(struct domain *d) > * be read-only, or via hardware-assisted log-dirty. > */ > p2m_change_entry_type_global(d, p2m_ram_rw, p2m_ram_logdirty); > -flush_tlb_mask(d->dirty_cpumask); > +flush_mask(d->dirty_cpumask, FLUSH_TLB | FLUSH_HVM_ASID_CORE); > } > > // > @@ -798,7 +798,7 @@ hap_write_p2m_entry(struct p2m_domain *p2m, unsigned long > gfn, l1_pgentry_t *p, > > safe_write_pte(p, new); > if ( old_flags & _PAGE_PRESENT ) > -flush_tlb_mask(d->dirty_cpumask); > +flush_mask(d->dirty_cpumask, FLUSH_TLB | FLUSH_HVM_ASID_CORE); For all four - why FLUSH_TLB? Doesn't the flushing here solely care about guest translations? > --- a/xen/arch/x86/mm/hap/nested_hap.c > +++ b/xen/arch/x86/mm/hap/nested_hap.c > @@ -84,7 +84,7 @@ nestedp2m_write_p2m_entry(struct p2m_domain *p2m, unsigned > long gfn, > safe_write_pte(p, new); > > if (old_flags & _PAGE_PRESENT) > -flush_tlb_mask(p2m->dirty_cpumask); > +flush_mask(p2m->dirty_cpumask, FLUSH_TLB | FLUSH_HVM_ASID_CORE); Same here then I guess. > --- a/xen/arch/x86/mm/p2m-pt.c > +++ b/xen/arch/x86/mm/p2m-pt.c > @@ -896,7 +896,8 @@ static void p2m_pt_change_entry_type_global(struct > p2m_domain *p2m, > unmap_domain_page(tab); > > if ( changed ) > - flush_tlb_mask(p2m->domain->dirty_cpumask); > + flush_mask(p2m->domain->dirty_cpumask, > +FLUSH_TLB | FLUSH_HVM_ASID_CORE); Given that this code is used in shadow mode as well, perhaps better to keep it here. Albeit maybe FLUSH_TLB could be dependent upon !hap_enabled()? > --- a/xen/arch/x86/mm/paging.c > +++ b/xen/arch/x86/mm/paging.c > @@ -613,7 +613,7 @@ void paging_log_dirty_range(struct domain *d, > > p2m_unlock(p2m); > > -flush_tlb_mask(d->dirty_cpumask); > +flush_mask(d->dirty_cpumask, FLUSH_TLB | FLUSH_HVM_ASID_CORE); Same here? > @@ -993,7 +993,7 @@ static void shadow_blow_tables(struct domain *d) > pagetable_get_mfn(v->arch.shadow_table[i]), > 0); > > /* Make sure everyone sees the unshadowings */ > -flush_tlb_mask(d->dirty_cpumask); > +flush_mask(d->dirty_cpumask, FLUSH_TLB | FLUSH_HVM_ASID_CORE); Taking this as example, wouldn't it be more consistent overall if paths not being HVM-only would specify FLUSH_HVM_ASID_CORE only for HVM domains? Also, seeing the large number of conversions, perhaps have another wrapper, e.g. flush_tlb_mask_hvm(), at least for the cases where both flags get specified unconditionally? Jan
[seabios test] 149211: tolerable FAIL - PUSHED
flight 149211 seabios real [real] http://logs.test-lab.xenproject.org/osstest/logs/149211/ Failures :-/ but no regressions. Tests which are failing intermittently (not blocking): test-amd64-amd64-qemuu-nested-intel 17 debian-hvm-install/l1/l2 fail in 149173 pass in 149211 test-amd64-i386-qemuu-rhel6hvm-intel 12 guest-start/redhat.repeat fail pass in 149173 Tests which did not succeed, but are not blocking: test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail like 148666 test-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stopfail like 148666 test-amd64-i386-xl-qemuu-ws16-amd64 17 guest-stop fail like 148666 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2 fail never pass test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stop fail starved in 148666 version targeted for testing: seabios 6a3b59ab9c7dc00331c21346052dfa6a0df45aa3 baseline version: seabios 066a9956097b54530888b88ab9aa1ea02e42af5a Last test of basis 148666 2020-03-17 13:39:45 Z 14 days Failing since148690 2020-03-18 06:43:59 Z 13 days 16 attempts Testing same since 149120 2020-03-28 03:28:10 Z3 days4 attempts People who touched revisions under test: Gerd Hoffmann Matt DeVillier Paul Menzel jobs: build-amd64-xsm pass build-i386-xsm pass build-amd64 pass build-i386 pass build-amd64-libvirt pass build-i386-libvirt pass build-amd64-pvopspass build-i386-pvops pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsmpass test-amd64-amd64-xl-qemuu-debianhvm-i386-xsm pass test-amd64-i386-xl-qemuu-debianhvm-i386-xsm pass test-amd64-amd64-qemuu-nested-amdfail test-amd64-i386-qemuu-rhel6hvm-amd pass test-amd64-amd64-xl-qemuu-debianhvm-amd64pass test-amd64-i386-xl-qemuu-debianhvm-amd64 pass test-amd64-amd64-xl-qemuu-win7-amd64 fail test-amd64-i386-xl-qemuu-win7-amd64 fail test-amd64-amd64-xl-qemuu-ws16-amd64 fail test-amd64-i386-xl-qemuu-ws16-amd64 fail test-amd64-amd64-xl-qemuu-dmrestrict-amd64-dmrestrictpass test-amd64-i386-xl-qemuu-dmrestrict-amd64-dmrestrict pass test-amd64-amd64-qemuu-nested-intel pass test-amd64-i386-qemuu-rhel6hvm-intel fail test-amd64-amd64-xl-qemuu-debianhvm-amd64-shadow pass test-amd64-i386-xl-qemuu-debianhvm-amd64-shadow 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 : To xenbits.xen.org:/home/xen/git/osstest/seabios.git 066a995..6a3b59a 6a3b59ab9c7dc00331c21346052dfa6a0df45aa3 -> xen-tested-master
Re: [PATCH 11/11] x86/ucode/amd: Rework parsing logic in cpu_request_microcode()
On 31.03.2020 17:19, Andrew Cooper wrote: > On 31/03/2020 16:07, Jan Beulich wrote: >> On 31.03.2020 12:05, Andrew Cooper wrote: >>> @@ -269,55 +265,25 @@ static int apply_microcode(const struct >>> microcode_patch *patch) >>> return 0; >>> } >>> >>> -static int scan_equiv_cpu_table( >>> -const void *data, >>> -size_t size_left, >>> -size_t *offset) >>> +static int scan_equiv_cpu_table(const struct container_equiv_table *et) >>> { >>> const struct cpu_signature *sig = &this_cpu(cpu_sig); >>> -const struct mpbhdr *mpbuf; >>> -const struct equiv_cpu_entry *eq; >>> -unsigned int i, nr; >>> - >>> -if ( size_left < (sizeof(*mpbuf) + 4) || >>> - (mpbuf = data + *offset + 4, >>> - size_left - sizeof(*mpbuf) - 4 < mpbuf->len) ) >>> -{ >>> -printk(XENLOG_WARNING "microcode: No space for equivalent cpu >>> table\n"); >>> -return -EINVAL; >>> -} >>> - >>> -*offset += mpbuf->len + CONT_HDR_SIZE; /* add header length */ >>> - >>> -if ( mpbuf->type != UCODE_EQUIV_CPU_TABLE_TYPE ) >>> -{ >>> -printk(KERN_ERR "microcode: Wrong microcode equivalent cpu table >>> type field\n"); >>> -return -EINVAL; >>> -} >>> - >>> -if ( mpbuf->len == 0 || mpbuf->len % sizeof(*eq) || >>> - (eq = (const void *)mpbuf->data, >>> - nr = mpbuf->len / sizeof(*eq), >>> - eq[nr - 1].installed_cpu) ) >> Did this last check get lost? I can't seem to be able to identify >> any possible replacement. > > Given the lack of a spec, I'm unsure whether to keep it or not. > > It is necessary in the backport of patch 1, because find_equiv_cpu_id() > doesn't have mpbuf->len to hand, and relies on the sentinel to find the > end of the table. > > OTOH, the new logic will cope perfectly well without a sentinel. Okay. >>> static struct microcode_patch *cpu_request_microcode(const void *buf, >>> size_t size) >>> { >>> const struct microcode_patch *saved = NULL; >>> struct microcode_patch *patch = NULL; >>> -size_t offset = 0, saved_size = 0; >>> +size_t saved_size = 0; >>> int error = 0; >>> -unsigned int cpu = smp_processor_id(); >>> -const struct cpu_signature *sig = &per_cpu(cpu_sig, cpu); >>> >>> -if ( size < 4 || >>> - *(const uint32_t *)buf != UCODE_MAGIC ) >>> +while ( size ) >>> { >>> -printk(KERN_ERR "microcode: Wrong microcode patch file magic\n"); >>> -error = -EINVAL; >>> -goto out; >>> -} >>> - >>> -/* >>> - * Multiple container file support: >>> - * 1. check if this container file has equiv_cpu_id match >>> - * 2. If not, fast-fwd to next container file >>> - */ >>> -while ( offset < size ) >>> -{ >>> -error = scan_equiv_cpu_table(buf, size - offset, &offset); >>> - >>> -if ( !error || error != -ESRCH ) >>> -break; >>> +const struct container_equiv_table *et; >>> +bool skip_ucode; >>> >>> -error = container_fast_forward(buf, size - offset, &offset); >>> -if ( error == -ENODATA ) >>> +if ( size < 4 || *(const uint32_t *)buf != UCODE_MAGIC ) >>> { >>> -ASSERT(offset == size); >>> +printk(XENLOG_ERR "microcode: Wrong microcode patch file >>> magic\n"); >>> +error = -EINVAL; >>> break; >>> } >>> -if ( error ) >>> + >>> +/* Move over UCODE_MAGIC. */ >>> +buf += 4; >>> +size -= 4; >>> + >>> +if ( size < sizeof(*et) || >>> + (et = buf)->type != UCODE_EQUIV_CPU_TABLE_TYPE || >>> + size - sizeof(*et) < et->len || >>> + et->len % sizeof(et->eq[0]) ) >>> { >>> -printk(KERN_ERR "microcode: CPU%d incorrect or corrupt >>> container file\n" >>> - "microcode: Failed to update patch level. " >>> - "Current lvl:%#x\n", cpu, sig->rev); >>> +printk(XENLOG_ERR "microcode: Bad equivalent cpu table\n"); >>> +error = -EINVAL; >>> break; >>> } >>> -} >>> >>> -if ( error ) >>> -{ >>> -/* >>> - * -ENODATA here means that the blob was parsed fine but no >>> matching >>> - * ucode was found. Don't return it to the caller. >>> - */ >>> -if ( error == -ENODATA ) >>> -error = 0; >>> - >>> -goto out; >>> -} >>> +/* Move over the Equiv table. */ >>> +buf += sizeof(*et) + et->len; >>> +size -= sizeof(*et) + et->len; >>> + >>> +error = scan_equiv_cpu_table(et); >>> +if ( error && error != -ESRCH ) >>> +break; >> With this the only non-zero value left for error is -ESRCH. >> Hence ... >> >>> +/* -ESRCH means no applicable microcode in this container. */ >>> +skip_ucode = error == -ESRCH; >> ... perhaps omit the "== -ESRCH" here, moving the comment up >> ahead of the i
Re: [PATCH 11/11] x86/ucode/amd: Rework parsing logic in cpu_request_microcode()
On 31/03/2020 16:07, Jan Beulich wrote: > On 31.03.2020 12:05, Andrew Cooper wrote: >> @@ -269,55 +265,25 @@ static int apply_microcode(const struct >> microcode_patch *patch) >> return 0; >> } >> >> -static int scan_equiv_cpu_table( >> -const void *data, >> -size_t size_left, >> -size_t *offset) >> +static int scan_equiv_cpu_table(const struct container_equiv_table *et) >> { >> const struct cpu_signature *sig = &this_cpu(cpu_sig); >> -const struct mpbhdr *mpbuf; >> -const struct equiv_cpu_entry *eq; >> -unsigned int i, nr; >> - >> -if ( size_left < (sizeof(*mpbuf) + 4) || >> - (mpbuf = data + *offset + 4, >> - size_left - sizeof(*mpbuf) - 4 < mpbuf->len) ) >> -{ >> -printk(XENLOG_WARNING "microcode: No space for equivalent cpu >> table\n"); >> -return -EINVAL; >> -} >> - >> -*offset += mpbuf->len + CONT_HDR_SIZE; /* add header length */ >> - >> -if ( mpbuf->type != UCODE_EQUIV_CPU_TABLE_TYPE ) >> -{ >> -printk(KERN_ERR "microcode: Wrong microcode equivalent cpu table >> type field\n"); >> -return -EINVAL; >> -} >> - >> -if ( mpbuf->len == 0 || mpbuf->len % sizeof(*eq) || >> - (eq = (const void *)mpbuf->data, >> - nr = mpbuf->len / sizeof(*eq), >> - eq[nr - 1].installed_cpu) ) > Did this last check get lost? I can't seem to be able to identify > any possible replacement. Given the lack of a spec, I'm unsure whether to keep it or not. It is necessary in the backport of patch 1, because find_equiv_cpu_id() doesn't have mpbuf->len to hand, and relies on the sentinel to find the end of the table. OTOH, the new logic will cope perfectly well without a sentinel. > >> static struct microcode_patch *cpu_request_microcode(const void *buf, >> size_t size) >> { >> const struct microcode_patch *saved = NULL; >> struct microcode_patch *patch = NULL; >> -size_t offset = 0, saved_size = 0; >> +size_t saved_size = 0; >> int error = 0; >> -unsigned int cpu = smp_processor_id(); >> -const struct cpu_signature *sig = &per_cpu(cpu_sig, cpu); >> >> -if ( size < 4 || >> - *(const uint32_t *)buf != UCODE_MAGIC ) >> +while ( size ) >> { >> -printk(KERN_ERR "microcode: Wrong microcode patch file magic\n"); >> -error = -EINVAL; >> -goto out; >> -} >> - >> -/* >> - * Multiple container file support: >> - * 1. check if this container file has equiv_cpu_id match >> - * 2. If not, fast-fwd to next container file >> - */ >> -while ( offset < size ) >> -{ >> -error = scan_equiv_cpu_table(buf, size - offset, &offset); >> - >> -if ( !error || error != -ESRCH ) >> -break; >> +const struct container_equiv_table *et; >> +bool skip_ucode; >> >> -error = container_fast_forward(buf, size - offset, &offset); >> -if ( error == -ENODATA ) >> +if ( size < 4 || *(const uint32_t *)buf != UCODE_MAGIC ) >> { >> -ASSERT(offset == size); >> +printk(XENLOG_ERR "microcode: Wrong microcode patch file >> magic\n"); >> +error = -EINVAL; >> break; >> } >> -if ( error ) >> + >> +/* Move over UCODE_MAGIC. */ >> +buf += 4; >> +size -= 4; >> + >> +if ( size < sizeof(*et) || >> + (et = buf)->type != UCODE_EQUIV_CPU_TABLE_TYPE || >> + size - sizeof(*et) < et->len || >> + et->len % sizeof(et->eq[0]) ) >> { >> -printk(KERN_ERR "microcode: CPU%d incorrect or corrupt >> container file\n" >> - "microcode: Failed to update patch level. " >> - "Current lvl:%#x\n", cpu, sig->rev); >> +printk(XENLOG_ERR "microcode: Bad equivalent cpu table\n"); >> +error = -EINVAL; >> break; >> } >> -} >> >> -if ( error ) >> -{ >> -/* >> - * -ENODATA here means that the blob was parsed fine but no matching >> - * ucode was found. Don't return it to the caller. >> - */ >> -if ( error == -ENODATA ) >> -error = 0; >> - >> -goto out; >> -} >> +/* Move over the Equiv table. */ >> +buf += sizeof(*et) + et->len; >> +size -= sizeof(*et) + et->len; >> + >> +error = scan_equiv_cpu_table(et); >> +if ( error && error != -ESRCH ) >> +break; > With this the only non-zero value left for error is -ESRCH. > Hence ... > >> +/* -ESRCH means no applicable microcode in this container. */ >> +skip_ucode = error == -ESRCH; > ... perhaps omit the "== -ESRCH" here, moving the comment up > ahead of the if()? That doesn't work, because you've got to reset error to 0 somewhere (to avoid it leaking out if you don't find suitable microcode), and it can't be before checking for errors in general. It can
Re: [Xen-devel] [PATCH] drm/xen: fix passing zero to 'PTR_ERR' warning
On Tue, Mar 31, 2020 at 05:50:10PM +0300, Oleksandr Andrushchenko wrote: > On 3/30/20 12:59, Ding Xiang wrote: > > Fix a static code checker warning: > > drivers/gpu/drm/xen/xen_drm_front.c:404 xen_drm_drv_dumb_create() > > warn: passing zero to 'PTR_ERR' > > > > Signed-off-by: Ding Xiang > Reviewed-by: Oleksandr Andrushchenko merged to drm-misc-next-fixese. -Daniel > > --- > > drivers/gpu/drm/xen/xen_drm_front.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/xen/xen_drm_front.c > > b/drivers/gpu/drm/xen/xen_drm_front.c > > index 4be49c1..3741420 100644 > > --- a/drivers/gpu/drm/xen/xen_drm_front.c > > +++ b/drivers/gpu/drm/xen/xen_drm_front.c > > @@ -401,7 +401,7 @@ static int xen_drm_drv_dumb_create(struct drm_file > > *filp, > > obj = xen_drm_front_gem_create(dev, args->size); > > if (IS_ERR_OR_NULL(obj)) { > > - ret = PTR_ERR(obj); > > + ret = PTR_ERR_OR_ZERO(obj); > > goto fail; > > } -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [PATCH 09/11] x86/ucode/amd: Remove gratuitous memory allocations from cpu_request_microcode()
On 31.03.2020 16:55, Andrew Cooper wrote: > On 31/03/2020 15:51, Jan Beulich wrote: >> On 31.03.2020 12:05, Andrew Cooper wrote: >>> @@ -497,57 +456,54 @@ static struct microcode_patch >>> *cpu_request_microcode(const void *buf, size_t siz >>> * It's possible the data file has multiple matching ucode, >>> * lets keep searching till the latest version >>> */ >>> -while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, size, >>> - &offset)) == 0 ) >>> +buf += offset; >>> +size -= offset; >>> { >>> -/* >>> - * If the new ucode covers current CPU, compare ucodes and store >>> the >>> - * one with higher revision. >>> - */ >>> -if ( (microcode_fits(mc_amd->mpb) != MIS_UCODE) && >>> - (!saved || (compare_header(mc_amd->mpb, saved) == NEW_UCODE)) >>> ) >>> +while ( size ) >>> { >>> -xfree(saved); >>> -saved = mc_amd->mpb; >>> -} >>> -else >>> -{ >>> -xfree(mc_amd->mpb); >>> -mc_amd->mpb = NULL; >>> -} >>> +const struct container_microcode *mc; >>> + >>> +if ( size < sizeof(*mc) || >>> + (mc = buf)->type != UCODE_UCODE_TYPE || >>> + size - sizeof(*mc) < mc->len || >>> + !verify_patch_size(mc->len) ) >>> +{ >>> +printk(XENLOG_ERR "microcode: Bad microcode data\n"); >>> +error = -EINVAL; >>> +break; >>> +} >>> >>> -if ( offset >= size ) >>> -break; >>> +/* >>> + * If the new ucode covers current CPU, compare ucodes and >>> store the >>> + * one with higher revision. >>> + */ >>> +if ( (microcode_fits(mc->patch) != MIS_UCODE) && >>> + (!saved || (compare_header(mc->patch, saved) == >>> NEW_UCODE)) ) >>> +{ >>> +saved = mc->patch; >>> +saved_size = mc->len; >>> +} >>> >>> -/* >>> - * 1. Given a situation where multiple containers exist and correct >>> - *patch lives on a container that is not the last container. >>> - * 2. We match equivalent ids using find_equiv_cpu_id() from the >>> - *earlier while() (On this case, matches on earlier container >>> - *file and we break) >>> - * 3. Proceed to while ( (error = get_ucode_from_buffer_amd(mc_amd, >>> - * buf, size, &offset)) == 0 ) >>> - * 4. Find correct patch using microcode_fits() and apply the patch >>> - *(Assume: apply_microcode() is successful) >>> - * 5. The while() loop from (3) continues to parse the binary as >>> - *there is a subsequent container file, but... >>> - * 6. ...a correct patch can only be on one container and not on >>> any >>> - *subsequent ones. (Refer docs for more info) Therefore, we >>> - *don't have to parse a subsequent container. So, we can abort >>> - *the process here. >>> - * 7. This ensures that we retain a success value (= 0) to 'error' >>> - *before if ( mpbuf->type != UCODE_UCODE_TYPE ) evaluates to >>> - *false and returns -EINVAL. >>> - */ >>> -if ( offset + SECTION_HDR_SIZE <= size && >>> - *(const uint32_t *)(buf + offset) == UCODE_MAGIC ) >>> -break; >>> +/* Move over the microcode blob. */ >>> +buf += sizeof(*mc) + mc->len; >>> +size -= sizeof(*mc) + mc->len; >>> + >>> +/* >>> + * Peek ahead. If we see the start of another container, we've >>> + * exhaused all microcode blobs in this container. Exit >>> cleanly. >>> + */ >>> +if ( size >= 4 && *(const uint32_t *)buf == UCODE_MAGIC ) >>> +break; >> While, as already indicated, I agree with shrinking the big comment, >> I think point 6 is what wants retaining in some form - it's not >> obvious at all why a subsequent container couldn't contain a higher >> rev ucode than what we've found. That comment refers us to docs, but >> I couldn't find anything to this effect in PM Vol 2. Assuming this >> indeed documented and true, with the comment extended accordingly >> Reviewed-by: Jan Beulich > > I think it is referring to the internal PPR, which isn't even the one we > have access to. > > As to the multiple containers aspect, I've deliberately "fixed" that in > patch 11 so we do scan all the way to the end. Right, meanwhile I've seen this. But shouldn't patch 11 then adjust at least the "Exit cleanly" part of the comment? You're merely breaking the inner loop then ... Jan
Re: [PATCH 11/11] x86/ucode/amd: Rework parsing logic in cpu_request_microcode()
On 31.03.2020 12:05, Andrew Cooper wrote: > @@ -269,55 +265,25 @@ static int apply_microcode(const struct microcode_patch > *patch) > return 0; > } > > -static int scan_equiv_cpu_table( > -const void *data, > -size_t size_left, > -size_t *offset) > +static int scan_equiv_cpu_table(const struct container_equiv_table *et) > { > const struct cpu_signature *sig = &this_cpu(cpu_sig); > -const struct mpbhdr *mpbuf; > -const struct equiv_cpu_entry *eq; > -unsigned int i, nr; > - > -if ( size_left < (sizeof(*mpbuf) + 4) || > - (mpbuf = data + *offset + 4, > - size_left - sizeof(*mpbuf) - 4 < mpbuf->len) ) > -{ > -printk(XENLOG_WARNING "microcode: No space for equivalent cpu > table\n"); > -return -EINVAL; > -} > - > -*offset += mpbuf->len + CONT_HDR_SIZE; /* add header length */ > - > -if ( mpbuf->type != UCODE_EQUIV_CPU_TABLE_TYPE ) > -{ > -printk(KERN_ERR "microcode: Wrong microcode equivalent cpu table > type field\n"); > -return -EINVAL; > -} > - > -if ( mpbuf->len == 0 || mpbuf->len % sizeof(*eq) || > - (eq = (const void *)mpbuf->data, > - nr = mpbuf->len / sizeof(*eq), > - eq[nr - 1].installed_cpu) ) Did this last check get lost? I can't seem to be able to identify any possible replacement. > static struct microcode_patch *cpu_request_microcode(const void *buf, size_t > size) > { > const struct microcode_patch *saved = NULL; > struct microcode_patch *patch = NULL; > -size_t offset = 0, saved_size = 0; > +size_t saved_size = 0; > int error = 0; > -unsigned int cpu = smp_processor_id(); > -const struct cpu_signature *sig = &per_cpu(cpu_sig, cpu); > > -if ( size < 4 || > - *(const uint32_t *)buf != UCODE_MAGIC ) > +while ( size ) > { > -printk(KERN_ERR "microcode: Wrong microcode patch file magic\n"); > -error = -EINVAL; > -goto out; > -} > - > -/* > - * Multiple container file support: > - * 1. check if this container file has equiv_cpu_id match > - * 2. If not, fast-fwd to next container file > - */ > -while ( offset < size ) > -{ > -error = scan_equiv_cpu_table(buf, size - offset, &offset); > - > -if ( !error || error != -ESRCH ) > -break; > +const struct container_equiv_table *et; > +bool skip_ucode; > > -error = container_fast_forward(buf, size - offset, &offset); > -if ( error == -ENODATA ) > +if ( size < 4 || *(const uint32_t *)buf != UCODE_MAGIC ) > { > -ASSERT(offset == size); > +printk(XENLOG_ERR "microcode: Wrong microcode patch file > magic\n"); > +error = -EINVAL; > break; > } > -if ( error ) > + > +/* Move over UCODE_MAGIC. */ > +buf += 4; > +size -= 4; > + > +if ( size < sizeof(*et) || > + (et = buf)->type != UCODE_EQUIV_CPU_TABLE_TYPE || > + size - sizeof(*et) < et->len || > + et->len % sizeof(et->eq[0]) ) > { > -printk(KERN_ERR "microcode: CPU%d incorrect or corrupt container > file\n" > - "microcode: Failed to update patch level. " > - "Current lvl:%#x\n", cpu, sig->rev); > +printk(XENLOG_ERR "microcode: Bad equivalent cpu table\n"); > +error = -EINVAL; > break; > } > -} > > -if ( error ) > -{ > -/* > - * -ENODATA here means that the blob was parsed fine but no matching > - * ucode was found. Don't return it to the caller. > - */ > -if ( error == -ENODATA ) > -error = 0; > - > -goto out; > -} > +/* Move over the Equiv table. */ > +buf += sizeof(*et) + et->len; > +size -= sizeof(*et) + et->len; > + > +error = scan_equiv_cpu_table(et); > +if ( error && error != -ESRCH ) > +break; With this the only non-zero value left for error is -ESRCH. Hence ... > +/* -ESRCH means no applicable microcode in this container. */ > +skip_ucode = error == -ESRCH; ... perhaps omit the "== -ESRCH" here, moving the comment up ahead of the if()? Jan
Re: [PATCH 09/11] x86/ucode/amd: Remove gratuitous memory allocations from cpu_request_microcode()
On 31/03/2020 15:51, Jan Beulich wrote: > On 31.03.2020 12:05, Andrew Cooper wrote: >> @@ -497,57 +456,54 @@ static struct microcode_patch >> *cpu_request_microcode(const void *buf, size_t siz >> * It's possible the data file has multiple matching ucode, >> * lets keep searching till the latest version >> */ >> -while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, size, >> - &offset)) == 0 ) >> +buf += offset; >> +size -= offset; >> { >> -/* >> - * If the new ucode covers current CPU, compare ucodes and store the >> - * one with higher revision. >> - */ >> -if ( (microcode_fits(mc_amd->mpb) != MIS_UCODE) && >> - (!saved || (compare_header(mc_amd->mpb, saved) == NEW_UCODE)) ) >> +while ( size ) >> { >> -xfree(saved); >> -saved = mc_amd->mpb; >> -} >> -else >> -{ >> -xfree(mc_amd->mpb); >> -mc_amd->mpb = NULL; >> -} >> +const struct container_microcode *mc; >> + >> +if ( size < sizeof(*mc) || >> + (mc = buf)->type != UCODE_UCODE_TYPE || >> + size - sizeof(*mc) < mc->len || >> + !verify_patch_size(mc->len) ) >> +{ >> +printk(XENLOG_ERR "microcode: Bad microcode data\n"); >> +error = -EINVAL; >> +break; >> +} >> >> -if ( offset >= size ) >> -break; >> +/* >> + * If the new ucode covers current CPU, compare ucodes and >> store the >> + * one with higher revision. >> + */ >> +if ( (microcode_fits(mc->patch) != MIS_UCODE) && >> + (!saved || (compare_header(mc->patch, saved) == >> NEW_UCODE)) ) >> +{ >> +saved = mc->patch; >> +saved_size = mc->len; >> +} >> >> -/* >> - * 1. Given a situation where multiple containers exist and correct >> - *patch lives on a container that is not the last container. >> - * 2. We match equivalent ids using find_equiv_cpu_id() from the >> - *earlier while() (On this case, matches on earlier container >> - *file and we break) >> - * 3. Proceed to while ( (error = get_ucode_from_buffer_amd(mc_amd, >> - * buf, size, &offset)) == 0 ) >> - * 4. Find correct patch using microcode_fits() and apply the patch >> - *(Assume: apply_microcode() is successful) >> - * 5. The while() loop from (3) continues to parse the binary as >> - *there is a subsequent container file, but... >> - * 6. ...a correct patch can only be on one container and not on any >> - *subsequent ones. (Refer docs for more info) Therefore, we >> - *don't have to parse a subsequent container. So, we can abort >> - *the process here. >> - * 7. This ensures that we retain a success value (= 0) to 'error' >> - *before if ( mpbuf->type != UCODE_UCODE_TYPE ) evaluates to >> - *false and returns -EINVAL. >> - */ >> -if ( offset + SECTION_HDR_SIZE <= size && >> - *(const uint32_t *)(buf + offset) == UCODE_MAGIC ) >> -break; >> +/* Move over the microcode blob. */ >> +buf += sizeof(*mc) + mc->len; >> +size -= sizeof(*mc) + mc->len; >> + >> +/* >> + * Peek ahead. If we see the start of another container, we've >> + * exhaused all microcode blobs in this container. Exit >> cleanly. >> + */ >> +if ( size >= 4 && *(const uint32_t *)buf == UCODE_MAGIC ) >> +break; > While, as already indicated, I agree with shrinking the big comment, > I think point 6 is what wants retaining in some form - it's not > obvious at all why a subsequent container couldn't contain a higher > rev ucode than what we've found. That comment refers us to docs, but > I couldn't find anything to this effect in PM Vol 2. Assuming this > indeed documented and true, with the comment extended accordingly > Reviewed-by: Jan Beulich I think it is referring to the internal PPR, which isn't even the one we have access to. As to the multiple containers aspect, I've deliberately "fixed" that in patch 11 so we do scan all the way to the end. Its a much more obvious way to do things, even if the default case is to only provide a single container applicable to a specific family. ~Andrew
Re: [PATCH 10/11] x86/ucode/amd: Fold structures together
On 31.03.2020 12:05, Andrew Cooper wrote: > With all the necessary cleanup now in place, fold struct microcode_header_amd > into struct microcode_patch and drop the struct microcode_amd temporary > ifdef-ary. > > This removes the memory allocation of struct microcode_amd which is a single > pointer to a separately allocated object, and therefore a waste. > > Signed-off-by: Andrew Cooper Reviewed-by: Jan Beulich
Re: [PATCH 09/11] x86/ucode/amd: Remove gratuitous memory allocations from cpu_request_microcode()
On 31.03.2020 12:05, Andrew Cooper wrote: > @@ -497,57 +456,54 @@ static struct microcode_patch > *cpu_request_microcode(const void *buf, size_t siz > * It's possible the data file has multiple matching ucode, > * lets keep searching till the latest version > */ > -while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, size, > - &offset)) == 0 ) > +buf += offset; > +size -= offset; > { > -/* > - * If the new ucode covers current CPU, compare ucodes and store the > - * one with higher revision. > - */ > -if ( (microcode_fits(mc_amd->mpb) != MIS_UCODE) && > - (!saved || (compare_header(mc_amd->mpb, saved) == NEW_UCODE)) ) > +while ( size ) > { > -xfree(saved); > -saved = mc_amd->mpb; > -} > -else > -{ > -xfree(mc_amd->mpb); > -mc_amd->mpb = NULL; > -} > +const struct container_microcode *mc; > + > +if ( size < sizeof(*mc) || > + (mc = buf)->type != UCODE_UCODE_TYPE || > + size - sizeof(*mc) < mc->len || > + !verify_patch_size(mc->len) ) > +{ > +printk(XENLOG_ERR "microcode: Bad microcode data\n"); > +error = -EINVAL; > +break; > +} > > -if ( offset >= size ) > -break; > +/* > + * If the new ucode covers current CPU, compare ucodes and store > the > + * one with higher revision. > + */ > +if ( (microcode_fits(mc->patch) != MIS_UCODE) && > + (!saved || (compare_header(mc->patch, saved) == NEW_UCODE)) > ) > +{ > +saved = mc->patch; > +saved_size = mc->len; > +} > > -/* > - * 1. Given a situation where multiple containers exist and correct > - *patch lives on a container that is not the last container. > - * 2. We match equivalent ids using find_equiv_cpu_id() from the > - *earlier while() (On this case, matches on earlier container > - *file and we break) > - * 3. Proceed to while ( (error = get_ucode_from_buffer_amd(mc_amd, > - * buf, size, &offset)) == 0 ) > - * 4. Find correct patch using microcode_fits() and apply the patch > - *(Assume: apply_microcode() is successful) > - * 5. The while() loop from (3) continues to parse the binary as > - *there is a subsequent container file, but... > - * 6. ...a correct patch can only be on one container and not on any > - *subsequent ones. (Refer docs for more info) Therefore, we > - *don't have to parse a subsequent container. So, we can abort > - *the process here. > - * 7. This ensures that we retain a success value (= 0) to 'error' > - *before if ( mpbuf->type != UCODE_UCODE_TYPE ) evaluates to > - *false and returns -EINVAL. > - */ > -if ( offset + SECTION_HDR_SIZE <= size && > - *(const uint32_t *)(buf + offset) == UCODE_MAGIC ) > -break; > +/* Move over the microcode blob. */ > +buf += sizeof(*mc) + mc->len; > +size -= sizeof(*mc) + mc->len; > + > +/* > + * Peek ahead. If we see the start of another container, we've > + * exhaused all microcode blobs in this container. Exit cleanly. > + */ > +if ( size >= 4 && *(const uint32_t *)buf == UCODE_MAGIC ) > +break; While, as already indicated, I agree with shrinking the big comment, I think point 6 is what wants retaining in some form - it's not obvious at all why a subsequent container couldn't contain a higher rev ucode than what we've found. That comment refers us to docs, but I couldn't find anything to this effect in PM Vol 2. Assuming this indeed documented and true, with the comment extended accordingly Reviewed-by: Jan Beulich Jan
Re: [Xen-devel] [PATCH] drm/xen: fix passing zero to 'PTR_ERR' warning
On 3/30/20 12:59, Ding Xiang wrote: Fix a static code checker warning: drivers/gpu/drm/xen/xen_drm_front.c:404 xen_drm_drv_dumb_create() warn: passing zero to 'PTR_ERR' Signed-off-by: Ding Xiang Reviewed-by: Oleksandr Andrushchenko --- drivers/gpu/drm/xen/xen_drm_front.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/xen/xen_drm_front.c b/drivers/gpu/drm/xen/xen_drm_front.c index 4be49c1..3741420 100644 --- a/drivers/gpu/drm/xen/xen_drm_front.c +++ b/drivers/gpu/drm/xen/xen_drm_front.c @@ -401,7 +401,7 @@ static int xen_drm_drv_dumb_create(struct drm_file *filp, obj = xen_drm_front_gem_create(dev, args->size); if (IS_ERR_OR_NULL(obj)) { - ret = PTR_ERR(obj); + ret = PTR_ERR_OR_ZERO(obj); goto fail; }
Re: [PATCH 08/11] x86/ucode/amd: Rename bufsize to size in cpu_request_microcode()
On 31/03/2020 15:41, Jan Beulich wrote: > On 31.03.2020 12:05, Andrew Cooper wrote: >> To simplify future cleanup, rename this variable. >> >> No functional change. >> >> Signed-off-by: Andrew Cooper > Acked-by: Jan Beulich > >> @@ -438,7 +437,7 @@ static struct microcode_patch >> *cpu_request_microcode(const void *buf, >> unsigned int cpu = smp_processor_id(); >> const struct cpu_signature *sig = &per_cpu(cpu_sig, cpu); >> >> -if ( bufsize < 4 || >> +if ( size < 4 || >> *(const uint32_t *)buf != UCODE_MAGIC ) > Take the opportunity and put this on a single line? I have actually done that in a later patch. This was introduced in patch 1, so I'll fix there and rebase it throughout the series. ~Andrew
Re: [PATCH 08/11] x86/ucode/amd: Rename bufsize to size in cpu_request_microcode()
On 31.03.2020 12:05, Andrew Cooper wrote: > To simplify future cleanup, rename this variable. > > No functional change. > > Signed-off-by: Andrew Cooper Acked-by: Jan Beulich > @@ -438,7 +437,7 @@ static struct microcode_patch > *cpu_request_microcode(const void *buf, > unsigned int cpu = smp_processor_id(); > const struct cpu_signature *sig = &per_cpu(cpu_sig, cpu); > > -if ( bufsize < 4 || > +if ( size < 4 || > *(const uint32_t *)buf != UCODE_MAGIC ) Take the opportunity and put this on a single line? Jan
Re: [PATCH 07/11] x86/ucode/amd: Alter API for microcode_fits()
On 31.03.2020 12:05, Andrew Cooper wrote: > Although it is logically a step in the wrong direction overall, it simplifies > the rearranging of cpu_request_microcode() substantially for microcode_fits() > to take struct microcode_header_amd directly, and not require an intermediate > struct microcode_amd pointing at it. > > Make this change (taking time to rename 'mc_amd' to its eventual 'patch' to > reduce the churn in the series), and a later cleanup will make it uniformly > take a struct microcode_patch. > > No functional change. > > Signed-off-by: Andrew Cooper Acked-by: Jan Beulich
Re: [PATCH 06/11] x86/ucode/amd: Move verify_patch_size() into get_ucode_from_buffer_amd()
On 31.03.2020 12:05, Andrew Cooper wrote: > We only stash the microcode blob size so it can be audited in > microcode_fits(). However, the patch size check depends only on the CPU > family. > > Move the check earlier to when we are parsing the container, which avoids > caching bad microcode in the first place, and allows us to avoid storing the > size at all. > > Signed-off-by: Andrew Cooper Reviewed-by: Jan Beulich
Re: [PATCH 05/11] x86/ucode/amd: Overhaul the equivalent cpu table handling completely
On 31.03.2020 12:05, Andrew Cooper wrote: > We currently copy the entire equivalency table, and the single correct > microcode. This is not safe to heterogeneous scenarios, and as Xen doesn't > support such situations to being with, can be used to simplify things further. s/being/begin/ ? > The CPUID.1.EAX => processor_rev_id mapping is fixed for an individual part. > We can cache the single appropriate entry on first discovery, and forgo > duplicating the entire table. > > Alter install_equiv_cpu_table() to be scan_equiv_cpu_table() which is > responsible for checking the equivalency table and caching appropriate > details. It now has a check for finding a different mapping (which indicates > that one of the tables we've seen is definitely wrong). > > A return value of -ESRCH is now used to signify "everything fine, but nothing > applicable for the current CPU", which is used to select the > container_fast_forward() path. > > Drop the printk(), as each applicable error path in scan_equiv_cpu_table() > already prints diagnostics. > > Signed-off-by: Andrew Cooper Reviewed-by: Jan Beulich
Re: [PATCH 04/11] x86/ucode/amd: Collect CPUID.1.EAX in collect_cpu_info()
On 31.03.2020 12:05, Andrew Cooper wrote: > ... rather than collecting it repeatedly in microcode_fits(). This brings the > behaviour in line with the Intel side. > > Signed-off-by: Andrew Cooper Reviewed-by: Jan Beulich
Re: [PATCH 03/11] x86/ucode/amd: Don't use void * for microcode_patch->mpb
On 31.03.2020 12:05, Andrew Cooper wrote: > All code works fine with it having its correct type, and it even allows us to > drop two casts in a printk(). > > No functional change. > > Signed-off-by: Andrew Cooper Reviewed-by: Jan Beulich
Re: [PATCH 02/11] x86/ucode/amd: Move check_final_patch_levels() to apply_microcode()
On 31.03.2020 12:05, Andrew Cooper wrote: > The microcode revision of whichever CPU runs cpu_request_microcode() is not > necessarily applicable to other CPUs. > > If the BIOS left us with asymmetric microcode, rejecting updates in > cpu_request_microcode() would prevent us levelling the system even if only up > to the final level. Also, failing to cache microcode misses an opportunity to > get beyond the final level via the S3 path. > > Move check_final_patch_levels() earlier and use it in apply_microcode(). > Reword the error message to be more informative, and use -ENXIO as this corner > case has nothing to do with permissions. > > Signed-off-by: Andrew Cooper Reviewed-by: Jan Beulich
Re: [PATCH v2 6/7] x86/ucode/intel: Clean up microcode_sanity_check()
On 31/03/2020 15:18, Jan Beulich wrote: > On 27.03.2020 13:29, Andrew Cooper wrote: >> @@ -160,93 +153,85 @@ static int collect_cpu_info(struct cpu_signature *csig) >> return 0; >> } >> >> -static int microcode_sanity_check(const struct microcode_patch *mc) >> +/* >> + * Sanity check a blob which is expected to be a microcode patch. The 48 >> byte >> + * header is of a known format, and together with totalsize are within the >> + * bounds of the container. Everything else is unchecked. >> + */ >> +static int microcode_sanity_check(const struct microcode_patch *patch) >> { >> -const struct microcode_header_intel *mc_header = &mc->hdr; >> -const struct extended_sigtable *ext_header = NULL; >> -const struct extended_signature *ext_sig; >> -unsigned long total_size, data_size, ext_table_size; >> -unsigned int ext_sigcount = 0, i; >> -uint32_t sum, orig_sum; >> - >> -total_size = get_totalsize(mc); >> -data_size = get_datasize(mc); >> -if ( (data_size + MC_HEADER_SIZE) > total_size ) >> +const struct extended_sigtable *ext; >> +const uint32_t *ptr; >> +unsigned int total_size = get_totalsize(patch); >> +unsigned int data_size = get_datasize(patch); >> +unsigned int i, ext_size; >> +uint32_t sum; >> + >> +/* >> + * Total size must be a multiple of 1024 bytes. Data size and the >> header >> + * must fit within it. >> + */ >> +if ( (total_size & 1023) || >> + data_size > (total_size - MC_HEADER_SIZE) ) >> { >> -printk(KERN_ERR "microcode: error! " >> - "Bad data size in microcode data file\n"); >> +printk(XENLOG_WARNING "microcode: Bad size\n"); >> return -EINVAL; >> } >> >> -if ( (mc_header->ldrver != 1) || (mc_header->hdrver != 1) ) >> -{ >> -printk(KERN_ERR "microcode: error! " >> - "Unknown microcode update format\n"); >> +/* Checksum the main header and data. */ >> +for ( sum = 0, ptr = (const uint32_t *)patch; >> + ptr < (const uint32_t *)&patch->data[data_size]; ++ptr ) >> +sum += *ptr; >> + >> +if ( sum != 0 ) >> return -EINVAL; > The error message for this looks to have been lost, or ... > >> -} >> -ext_table_size = total_size - (MC_HEADER_SIZE + data_size); >> -if ( ext_table_size ) >> + >> +/* Look to see if there is an extended signature table. */ >> +ext_size = total_size - data_size - MC_HEADER_SIZE; >> + >> +/* No extended signature table? All done. */ >> +if ( ext_size == 0 ) >> { >> -if ( (ext_table_size < EXT_HEADER_SIZE) || >> - ((ext_table_size - EXT_HEADER_SIZE) % EXT_SIGNATURE_SIZE) ) >> -{ >> -printk(KERN_ERR "microcode: error! " >> - "Small exttable size in microcode data file\n"); >> -return -EINVAL; >> -} >> -ext_header = (void *)mc + MC_HEADER_SIZE + data_size; >> -if ( ext_table_size != exttable_size(ext_header) ) >> -{ >> -printk(KERN_ERR "microcode: error! " >> - "Bad exttable size in microcode data file\n"); >> -return -EFAULT; >> -} >> -ext_sigcount = ext_header->count; >> +printk(XENLOG_WARNING "microcode: Bad checksum\n"); >> +return 0; > ... to have got mistakenly moved here. It was mistakenly moved. I found and fixed that at some point after sending this series. > With this addressed > Reviewed-by: Jan Beulich Thanks, ~Andrew
Re: [PATCH v2 7/7] x86/ucode/intel: Fold structures together
On 27.03.2020 13:29, Andrew Cooper wrote: > With all the necessary cleanup now in place, fold struct > microcode_header_intel into struct microcode_patch and drop the struct > microcode_intel temporary ifdef-ary. > > No functional change. > > Signed-off-by: Andrew Cooper Reviewed-by: Jan Beulich
Re: [PATCH v2 6/7] x86/ucode/intel: Clean up microcode_sanity_check()
On 27.03.2020 13:29, Andrew Cooper wrote: > @@ -160,93 +153,85 @@ static int collect_cpu_info(struct cpu_signature *csig) > return 0; > } > > -static int microcode_sanity_check(const struct microcode_patch *mc) > +/* > + * Sanity check a blob which is expected to be a microcode patch. The 48 > byte > + * header is of a known format, and together with totalsize are within the > + * bounds of the container. Everything else is unchecked. > + */ > +static int microcode_sanity_check(const struct microcode_patch *patch) > { > -const struct microcode_header_intel *mc_header = &mc->hdr; > -const struct extended_sigtable *ext_header = NULL; > -const struct extended_signature *ext_sig; > -unsigned long total_size, data_size, ext_table_size; > -unsigned int ext_sigcount = 0, i; > -uint32_t sum, orig_sum; > - > -total_size = get_totalsize(mc); > -data_size = get_datasize(mc); > -if ( (data_size + MC_HEADER_SIZE) > total_size ) > +const struct extended_sigtable *ext; > +const uint32_t *ptr; > +unsigned int total_size = get_totalsize(patch); > +unsigned int data_size = get_datasize(patch); > +unsigned int i, ext_size; > +uint32_t sum; > + > +/* > + * Total size must be a multiple of 1024 bytes. Data size and the header > + * must fit within it. > + */ > +if ( (total_size & 1023) || > + data_size > (total_size - MC_HEADER_SIZE) ) > { > -printk(KERN_ERR "microcode: error! " > - "Bad data size in microcode data file\n"); > +printk(XENLOG_WARNING "microcode: Bad size\n"); > return -EINVAL; > } > > -if ( (mc_header->ldrver != 1) || (mc_header->hdrver != 1) ) > -{ > -printk(KERN_ERR "microcode: error! " > - "Unknown microcode update format\n"); > +/* Checksum the main header and data. */ > +for ( sum = 0, ptr = (const uint32_t *)patch; > + ptr < (const uint32_t *)&patch->data[data_size]; ++ptr ) > +sum += *ptr; > + > +if ( sum != 0 ) > return -EINVAL; The error message for this looks to have been lost, or ... > -} > -ext_table_size = total_size - (MC_HEADER_SIZE + data_size); > -if ( ext_table_size ) > + > +/* Look to see if there is an extended signature table. */ > +ext_size = total_size - data_size - MC_HEADER_SIZE; > + > +/* No extended signature table? All done. */ > +if ( ext_size == 0 ) > { > -if ( (ext_table_size < EXT_HEADER_SIZE) || > - ((ext_table_size - EXT_HEADER_SIZE) % EXT_SIGNATURE_SIZE) ) > -{ > -printk(KERN_ERR "microcode: error! " > - "Small exttable size in microcode data file\n"); > -return -EINVAL; > -} > -ext_header = (void *)mc + MC_HEADER_SIZE + data_size; > -if ( ext_table_size != exttable_size(ext_header) ) > -{ > -printk(KERN_ERR "microcode: error! " > - "Bad exttable size in microcode data file\n"); > -return -EFAULT; > -} > -ext_sigcount = ext_header->count; > +printk(XENLOG_WARNING "microcode: Bad checksum\n"); > +return 0; ... to have got mistakenly moved here. With this addressed Reviewed-by: Jan Beulich Jan
Re: [PATCH v2 3/7] x86/ucode/intel: Remove gratuitous memory allocations from cpu_request_microcode()
On 31/03/2020 15:09, Jan Beulich wrote: > On 27.03.2020 13:28, Andrew Cooper wrote: >> cpu_request_microcode() needs to scan its container and duplicate one blob, >> but the get_next_ucode_from_buffer() helper duplicates every blob in turn. >> Furthermore, the length checking is only safe from overflow in 64bit builds. >> >> Delete get_next_ucode_from_buffer() and alter the purpose of the saved >> variable to simply point somewhere in buf until we're ready to return. >> >> This is only a modest reduction in absolute code size (-144), but avoids >> making memory allocations for every blob in the container. >> >> Signed-off-by: Andrew Cooper > Reviewed-by: Jan Beulich > >> v2: >> * Rebase over struct microcode_patch re-work >> * Reinstate printk() for bad data > Ooi, did the number mentioned above indeed no change with this? > (I don't mean you to adjust it, as it's precise value is not > really meaningful anyway without also knowing compiler version > etc.) I actually stripped the number after re-reading this on xen-devel. I didn't go back to check, but it almost certainly isn't the same. ~Andrew
Re: [PATCH v2 3/7] x86/ucode/intel: Remove gratuitous memory allocations from cpu_request_microcode()
On 27.03.2020 13:28, Andrew Cooper wrote: > cpu_request_microcode() needs to scan its container and duplicate one blob, > but the get_next_ucode_from_buffer() helper duplicates every blob in turn. > Furthermore, the length checking is only safe from overflow in 64bit builds. > > Delete get_next_ucode_from_buffer() and alter the purpose of the saved > variable to simply point somewhere in buf until we're ready to return. > > This is only a modest reduction in absolute code size (-144), but avoids > making memory allocations for every blob in the container. > > Signed-off-by: Andrew Cooper Reviewed-by: Jan Beulich > v2: > * Rebase over struct microcode_patch re-work > * Reinstate printk() for bad data Ooi, did the number mentioned above indeed no change with this? (I don't mean you to adjust it, as it's precise value is not really meaningful anyway without also knowing compiler version etc.) Jan
RE: Ping²: [PATCH] x86/HVM: fix AMD ECS handling for Fam 10
> -Original Message- > From: Jan Beulich > Sent: 31 March 2020 13:16 > To: Paul Durrant > Cc: Andrew Cooper ; > xen-devel@lists.xenproject.org; Wei Liu ; > Roger Pau Monné > Subject: Ping²: [PATCH] x86/HVM: fix AMD ECS handling for Fam 10 > > On 16.03.2020 14:41, Andrew Cooper wrote: > > On 16/03/2020 11:00, Jan Beulich wrote: > >> The involved comparison was, very likely inadvertently, converted from > >>> = to > when making changes unrelated to the actual family range. > >> > >> Fixes: 9841eb71ea87 ("x86/cpuid: Drop a guests cached x86 family and model > >> information") > >> Signed-off-by: Jan Beulich > > > > Reviewed-by: Andrew Cooper > > Paul? > Sorry, missed that. My mail filters appear to have let me down... > >> --- a/xen/arch/x86/hvm/ioreq.c > >> +++ b/xen/arch/x86/hvm/ioreq.c > >> @@ -1284,7 +1284,7 @@ struct hvm_ioreq_server *hvm_select_iore > >> if ( CF8_ADDR_HI(cf8) && > >> d->arch.cpuid->x86_vendor == X86_VENDOR_AMD && > >> (x86_fam = get_cpu_family( > >> - d->arch.cpuid->basic.raw_fms, NULL, NULL)) > 0x10 && > >> + d->arch.cpuid->basic.raw_fms, NULL, NULL)) >= 0x10 && LGTM Reviewed-by: Paul Durrant > >> x86_fam < 0x17 ) > >> { > >> uint64_t msr_val; > >> > >
Ping²: [PATCH] x86/HVM: fix AMD ECS handling for Fam 10
On 16.03.2020 14:41, Andrew Cooper wrote: > On 16/03/2020 11:00, Jan Beulich wrote: >> The involved comparison was, very likely inadvertently, converted from >>> = to > when making changes unrelated to the actual family range. >> >> Fixes: 9841eb71ea87 ("x86/cpuid: Drop a guests cached x86 family and model >> information") >> Signed-off-by: Jan Beulich > > Reviewed-by: Andrew Cooper Paul? >> --- a/xen/arch/x86/hvm/ioreq.c >> +++ b/xen/arch/x86/hvm/ioreq.c >> @@ -1284,7 +1284,7 @@ struct hvm_ioreq_server *hvm_select_iore >> if ( CF8_ADDR_HI(cf8) && >> d->arch.cpuid->x86_vendor == X86_VENDOR_AMD && >> (x86_fam = get_cpu_family( >> - d->arch.cpuid->basic.raw_fms, NULL, NULL)) > 0x10 && >> + d->arch.cpuid->basic.raw_fms, NULL, NULL)) >= 0x10 && >> x86_fam < 0x17 ) >> { >> uint64_t msr_val; >> >
Re: [PATCH 3/3] xen/x86: ioapic: Simplify ioapic_init()
On 31.03.2020 13:51, Julien Grall wrote: > Hi Jan, > > On 31/03/2020 12:22, Jan Beulich wrote: >> On 27.03.2020 20:05, Julien Grall wrote: >>> --- a/xen/arch/x86/io_apic.c >>> +++ b/xen/arch/x86/io_apic.c >>> @@ -2537,34 +2537,25 @@ static __init bool bad_ioapic_register(unsigned int >>> idx) >>> return false; >>> } >>> -void __init init_ioapic(void) >>> +static void __init init_ioapic_mappings(void) >>> { >>> - unsigned long ioapic_phys; >>> unsigned int i, idx = FIX_IO_APIC_BASE_0; >>> - union IO_APIC_reg_01 reg_01; >>> - if ( smp_found_config ) >>> - nr_irqs_gsi = 0; >>> for ( i = 0; i < nr_ioapics; i++ ) >>> { >>> - if ( smp_found_config ) >>> - { >>> - ioapic_phys = mp_ioapics[i].mpc_apicaddr; >>> - if ( !ioapic_phys ) >>> - { >>> - printk(KERN_ERR "WARNING: bogus zero IO-APIC address " >>> - "found in MPTABLE, disabling IO/APIC support!\n"); >>> - smp_found_config = false; >>> - skip_ioapic_setup = true; >>> - goto fake_ioapic_page; >>> - } >>> - } >>> - else >>> + union IO_APIC_reg_01 reg_01; >>> + unsigned long ioapic_phys = mp_ioapics[i].mpc_apicaddr; >>> + >>> + ioapic_phys = mp_ioapics[i].mpc_apicaddr; >>> + if ( !ioapic_phys ) >>> { >>> - fake_ioapic_page: >>> - ioapic_phys = __pa(alloc_xenheap_page()); >>> - clear_page(__va(ioapic_phys)); >>> + printk(KERN_ERR >>> + "WARNING: bogus zero IO-APIC address found in MPTABLE, >>> disabling IO/APIC support!\n"); >>> + smp_found_config = false; >>> + skip_ioapic_setup = true; >>> + break; >>> } >>> + >>> set_fixmap_nocache(idx, ioapic_phys); >>> apic_printk(APIC_VERBOSE, "mapped IOAPIC to %08Lx (%08lx)\n", >>> __fix_to_virt(idx), ioapic_phys); >>> @@ -2576,18 +2567,24 @@ void __init init_ioapic(void) >>> continue; >>> } >>> - if ( smp_found_config ) >>> - { >>> - /* The number of IO-APIC IRQ registers (== #pins): */ >>> - reg_01.raw = io_apic_read(i, 1); >>> - nr_ioapic_entries[i] = reg_01.bits.entries + 1; >>> - nr_irqs_gsi += nr_ioapic_entries[i]; >>> - >>> - if ( rangeset_add_singleton(mmio_ro_ranges, >>> - ioapic_phys >> PAGE_SHIFT) ) >>> - printk(KERN_ERR "Failed to mark IO-APIC page %lx >>> read-only\n", >>> - ioapic_phys); >>> - } >>> + /* The number of IO-APIC IRQ registers (== #pins): */ >>> + reg_01.raw = io_apic_read(i, 1); >>> + nr_ioapic_entries[i] = reg_01.bits.entries + 1; >>> + nr_irqs_gsi += nr_ioapic_entries[i]; >>> + >>> + if ( rangeset_add_singleton(mmio_ro_ranges, >>> + ioapic_phys >> PAGE_SHIFT) ) >>> + printk(KERN_ERR "Failed to mark IO-APIC page %lx read-only\n", >>> + ioapic_phys); >>> + } >>> +} >>> + >>> +void __init init_ioapic(void) >>> +{ >>> + if ( smp_found_config ) >>> + { >>> + nr_irqs_gsi = 0; >> >> This would seem to also belong into the function, e.g. as part of >> the loop header: >> >> for ( nr_irqs_gsi = i = 0; i < nr_ioapics; i++ ) > > While the initial value of the 'i' and 'nr_irqs_gsi' is the same, > the variables have very different meaning and may confuse the reader. I expected reservations like this, hence the "e.g."; i.e. ... > So I would rather not follow your suggestion here. Although, I can > move the zeroing right before the for loop. ... I'm fine with this as well. Jan
Re: [PATCH 2/2] xen/mm: Introduce PGC_state_uninitialised
On 19.03.2020 22:21, David Woodhouse wrote: > --- a/xen/arch/x86/mm.c > +++ b/xen/arch/x86/mm.c > @@ -491,7 +491,8 @@ void share_xen_page_with_guest(struct page_info *page, > struct domain *d, > > page_set_owner(page, d); > smp_wmb(); /* install valid domain ptr before updating refcnt. */ > -ASSERT((page->count_info & ~PGC_xen_heap) == 0); > +ASSERT((page->count_info & ~PGC_xen_heap) == PGC_state_inuse || > + (page->count_info & ~PGC_xen_heap) == PGC_state_uninitialised); Like for patch 1, there's a risk of the page transitioning from uninitialised to inuse between these two comparisons, making the ASSERT() trigger when it shouldn't. As you've got two more similar constructs further down in the patch, maybe this also warrants a helper function/macro? > --- a/xen/common/page_alloc.c > +++ b/xen/common/page_alloc.c > @@ -252,6 +252,8 @@ struct bootmem_region { > static struct bootmem_region __initdata > bootmem_region_list[PAGE_SIZE / sizeof(struct bootmem_region)]; > static unsigned int __initdata nr_bootmem_regions; > +static void init_heap_pages(struct page_info *pg, unsigned long nr_pages, > +bool scrub); > > struct scrub_region { > unsigned long offset; > @@ -1390,6 +1392,17 @@ static void free_heap_pages( > ASSERT(order <= MAX_ORDER); > ASSERT(node >= 0); > > +if ( page_state_is(pg, uninitialised) ) > +{ > +init_heap_pages(pg, 1 << order, need_scrub); While, at least for now, it shouldn't matter in practice, and while code overall is very inconsistent in this regard, with the respective parameter type being "unsigned long" may I suggest to use 1UL here? Jan
Re: [PATCH 1/2] xen/mm: fold PGC_broken into PGC_state bits
On 19.03.2020 22:21, David Woodhouse wrote: > --- a/xen/arch/x86/domctl.c > +++ b/xen/arch/x86/domctl.c > @@ -422,7 +422,7 @@ long arch_do_domctl( > if ( page->u.inuse.type_info & PGT_pinned ) > type |= XEN_DOMCTL_PFINFO_LPINTAB; > > -if ( page->count_info & PGC_broken ) > +if ( page_is_broken(page) ) > type = XEN_DOMCTL_PFINFO_BROKEN; Coming back to an earlier request of mine: There are no locks being held here afaics, so with #define page_is_broken(pg) (pgc_is_broken((pg)->count_info)) and #define pgc_is_broken(pgc) (pgc_is(pgc, broken) || \ pgc_is(pgc, broken_offlining)) there's a chance that the page gets transitioned from broken_offlining to broken (by another CPU) between these two checks, resulting in wrong returned state. Either the latter macro uses an intermediate variable and ACCESS_ONCE() or, as suggested before, enumerators get arranged such that the check can be done (e.g. using binary masking operations) with a single evaluation of pgc. This may or may not also be an issue for the other two pgc_is_*(), but I think at least for symmetry they should then follow suit. At the very least all three macros need to be immune to uses like page_is_offlined(pg++) or similar argument expressions with side effects. > @@ -1699,19 +1706,18 @@ unsigned int online_page(mfn_t mfn, uint32_t *status) > do { > ret = *status = 0; > > -if ( y & PGC_broken ) > +if ( pgc_is_broken(y) ) > { > ret = -EINVAL; > -*status = PG_ONLINE_FAILED |PG_ONLINE_BROKEN; > +*status = PG_ONLINE_FAILED | PG_ONLINE_BROKEN; > break; > } > - > -if ( (y & PGC_state) == PGC_state_offlined ) > +else if ( pgc_is(y, offlined) ) At the risk of getting flamed again: Even if it was a matter of taste in new code whether to use "else" in a case like this one, this isn't new code, and it is in no way necessary to change what we have for the purpose of this patch. I.e. without even having to resort to the question of whether personal taste decisions are to be accepted, this simply falls under "no unrelated / unnecessary changes please". (FAOD this includes the deletion of the blank line then as well.) > --- a/xen/include/asm-x86/mm.h > +++ b/xen/include/asm-x86/mm.h > @@ -67,16 +67,32 @@ > /* 3-bit PAT/PCD/PWT cache-attribute hint. */ > #define PGC_cacheattr_base PG_shift(6) > #define PGC_cacheattr_mask PG_mask(7, 6) > - /* Page is broken? */ > -#define _PGC_broken PG_shift(7) > -#define PGC_brokenPG_mask(1, 7) > - /* Mutually-exclusive page states: { inuse, offlining, offlined, free }. */ > -#define PGC_state PG_mask(3, 9) > -#define PGC_state_inuse PG_mask(0, 9) > -#define PGC_state_offlining PG_mask(1, 9) > -#define PGC_state_offlined PG_mask(2, 9) > -#define PGC_state_freePG_mask(3, 9) > -#define page_state_is(pg, st) (((pg)->count_info&PGC_state) == > PGC_state_##st) > + /* > + * Mutually-exclusive page states: > + * { inuse, offlining, offlined, free, broken_offlining, broken } > + */ > +#define PGC_state PG_mask(7, 9) > +#define PGC_state_inusePG_mask(0, 9) > +#define PGC_state_offliningPG_mask(1, 9) > +#define PGC_state_offlined PG_mask(2, 9) > +#define PGC_state_free PG_mask(3, 9) > +#define PGC_state_broken_offlining PG_mask(4, 9) /* Broken and offlining */ > +#define PGC_state_broken PG_mask(5, 9) /* Broken and offlined */ > + > +#define pgc_is(pgc, st)(((pgc) & PGC_state) == PGC_state_##st) > +#define page_state_is(pg, st) pgc_is((pg)->count_info, st) > + > +#define pgc_is_broken(pgc) (pgc_is(pgc, broken) || \ > +pgc_is(pgc, broken_offlining)) > +#define pgc_is_offlined(pgc) (pgc_is(pgc, offlined) || \ > +pgc_is(pgc, broken)) > +#define pgc_is_offlining(pgc) (pgc_is(pgc, offlining) || \ > +pgc_is(pgc, broken_offlining)) > + > +#define page_is_broken(pg) (pgc_is_broken((pg)->count_info)) > +#define page_is_offlined(pg) (pgc_is_broken((pg)->count_info)) > +#define page_is_offlining(pg) (pgc_is_broken((pg)->count_info)) Copy-and-paste mistake (rhs is the same for all three; same for Arm)? Also there's no need here for the outer pairs of parentheses. Also, for the next version, may I ask that you number versions in the subject's tag and that you provide a brief description of changes from the previous version (if any, but there ought to be some in a series for there to be a point to send out)? Thanks. Jan
RE: [PATCH v4] x86: irq: Do not BUG_ON multiple unbind calls for shared pirqs
> -Original Message- > From: Jan Beulich > Sent: 31 March 2020 08:41 > To: p...@xen.org > Cc: xen-devel@lists.xenproject.org; 'Varad Gautam' ; 'Julien > Grall' ; > 'Roger Pau Monné' ; 'Andrew Cooper' > > Subject: Re: [PATCH v4] x86: irq: Do not BUG_ON multiple unbind calls for > shared pirqs > > On 17.03.2020 16:23, Paul Durrant wrote: > > That looks like it will do the job. I'll see if I can get it tested. > > Any luck with this, yet? > I have asked Varad to test it. He hopes to get to it this week. Paul
Re: [PATCH 3/3] xen/x86: ioapic: Simplify ioapic_init()
Hi Jan, On 31/03/2020 12:22, Jan Beulich wrote: On 27.03.2020 20:05, Julien Grall wrote: --- a/xen/arch/x86/io_apic.c +++ b/xen/arch/x86/io_apic.c @@ -2537,34 +2537,25 @@ static __init bool bad_ioapic_register(unsigned int idx) return false; } -void __init init_ioapic(void) +static void __init init_ioapic_mappings(void) { -unsigned long ioapic_phys; unsigned int i, idx = FIX_IO_APIC_BASE_0; -union IO_APIC_reg_01 reg_01; -if ( smp_found_config ) -nr_irqs_gsi = 0; for ( i = 0; i < nr_ioapics; i++ ) { -if ( smp_found_config ) -{ -ioapic_phys = mp_ioapics[i].mpc_apicaddr; -if ( !ioapic_phys ) -{ -printk(KERN_ERR "WARNING: bogus zero IO-APIC address " - "found in MPTABLE, disabling IO/APIC support!\n"); -smp_found_config = false; -skip_ioapic_setup = true; -goto fake_ioapic_page; -} -} -else +union IO_APIC_reg_01 reg_01; +unsigned long ioapic_phys = mp_ioapics[i].mpc_apicaddr; + +ioapic_phys = mp_ioapics[i].mpc_apicaddr; +if ( !ioapic_phys ) { - fake_ioapic_page: -ioapic_phys = __pa(alloc_xenheap_page()); -clear_page(__va(ioapic_phys)); +printk(KERN_ERR + "WARNING: bogus zero IO-APIC address found in MPTABLE, disabling IO/APIC support!\n"); +smp_found_config = false; +skip_ioapic_setup = true; +break; } + set_fixmap_nocache(idx, ioapic_phys); apic_printk(APIC_VERBOSE, "mapped IOAPIC to %08Lx (%08lx)\n", __fix_to_virt(idx), ioapic_phys); @@ -2576,18 +2567,24 @@ void __init init_ioapic(void) continue; } -if ( smp_found_config ) -{ -/* The number of IO-APIC IRQ registers (== #pins): */ -reg_01.raw = io_apic_read(i, 1); -nr_ioapic_entries[i] = reg_01.bits.entries + 1; -nr_irqs_gsi += nr_ioapic_entries[i]; - -if ( rangeset_add_singleton(mmio_ro_ranges, -ioapic_phys >> PAGE_SHIFT) ) -printk(KERN_ERR "Failed to mark IO-APIC page %lx read-only\n", - ioapic_phys); -} +/* The number of IO-APIC IRQ registers (== #pins): */ +reg_01.raw = io_apic_read(i, 1); +nr_ioapic_entries[i] = reg_01.bits.entries + 1; +nr_irqs_gsi += nr_ioapic_entries[i]; + +if ( rangeset_add_singleton(mmio_ro_ranges, +ioapic_phys >> PAGE_SHIFT) ) +printk(KERN_ERR "Failed to mark IO-APIC page %lx read-only\n", + ioapic_phys); +} +} + +void __init init_ioapic(void) +{ +if ( smp_found_config ) +{ +nr_irqs_gsi = 0; This would seem to also belong into the function, e.g. as part of the loop header: for ( nr_irqs_gsi = i = 0; i < nr_ioapics; i++ ) While the initial value of the 'i' and 'nr_irqs_gsi' is the same, the variables have very different meaning and may confuse the reader. So I would rather not follow your suggestion here. Although, I can move the zeroing right before the for loop. Cheers, -- Julien Grall
Re: [PATCH 3/3] xen/x86: ioapic: Simplify ioapic_init()
On 27.03.2020 20:05, Julien Grall wrote: > --- a/xen/arch/x86/io_apic.c > +++ b/xen/arch/x86/io_apic.c > @@ -2537,34 +2537,25 @@ static __init bool bad_ioapic_register(unsigned int > idx) > return false; > } > > -void __init init_ioapic(void) > +static void __init init_ioapic_mappings(void) > { > -unsigned long ioapic_phys; > unsigned int i, idx = FIX_IO_APIC_BASE_0; > -union IO_APIC_reg_01 reg_01; > > -if ( smp_found_config ) > -nr_irqs_gsi = 0; > for ( i = 0; i < nr_ioapics; i++ ) > { > -if ( smp_found_config ) > -{ > -ioapic_phys = mp_ioapics[i].mpc_apicaddr; > -if ( !ioapic_phys ) > -{ > -printk(KERN_ERR "WARNING: bogus zero IO-APIC address " > - "found in MPTABLE, disabling IO/APIC support!\n"); > -smp_found_config = false; > -skip_ioapic_setup = true; > -goto fake_ioapic_page; > -} > -} > -else > +union IO_APIC_reg_01 reg_01; > +unsigned long ioapic_phys = mp_ioapics[i].mpc_apicaddr; > + > +ioapic_phys = mp_ioapics[i].mpc_apicaddr; > +if ( !ioapic_phys ) > { > - fake_ioapic_page: > -ioapic_phys = __pa(alloc_xenheap_page()); > -clear_page(__va(ioapic_phys)); > +printk(KERN_ERR > + "WARNING: bogus zero IO-APIC address found in MPTABLE, > disabling IO/APIC support!\n"); > +smp_found_config = false; > +skip_ioapic_setup = true; > +break; > } > + > set_fixmap_nocache(idx, ioapic_phys); > apic_printk(APIC_VERBOSE, "mapped IOAPIC to %08Lx (%08lx)\n", > __fix_to_virt(idx), ioapic_phys); > @@ -2576,18 +2567,24 @@ void __init init_ioapic(void) > continue; > } > > -if ( smp_found_config ) > -{ > -/* The number of IO-APIC IRQ registers (== #pins): */ > -reg_01.raw = io_apic_read(i, 1); > -nr_ioapic_entries[i] = reg_01.bits.entries + 1; > -nr_irqs_gsi += nr_ioapic_entries[i]; > - > -if ( rangeset_add_singleton(mmio_ro_ranges, > -ioapic_phys >> PAGE_SHIFT) ) > -printk(KERN_ERR "Failed to mark IO-APIC page %lx > read-only\n", > - ioapic_phys); > -} > +/* The number of IO-APIC IRQ registers (== #pins): */ > +reg_01.raw = io_apic_read(i, 1); > +nr_ioapic_entries[i] = reg_01.bits.entries + 1; > +nr_irqs_gsi += nr_ioapic_entries[i]; > + > +if ( rangeset_add_singleton(mmio_ro_ranges, > +ioapic_phys >> PAGE_SHIFT) ) > +printk(KERN_ERR "Failed to mark IO-APIC page %lx read-only\n", > + ioapic_phys); > +} > +} > + > +void __init init_ioapic(void) > +{ > +if ( smp_found_config ) > +{ > +nr_irqs_gsi = 0; This would seem to also belong into the function, e.g. as part of the loop header: for ( nr_irqs_gsi = i = 0; i < nr_ioapics; i++ ) Jan
Re: [PATCH 0/8] Fix build with using OCaml 4.06.1 and -safe-string
Julien Grall writes ("[PATCH 0/8] Fix build with using OCaml 4.06.1 and -safe-string"): > This series is meant to solve the build issue reported by Dario when > using recent version of OCaml and -safe-string. Thanks. I have reviewed the C tools parts here. I think the ocaml parts ought to have a review from someone familiar with the ocaml FFI. > I took the opportunity to harden a bit more the code by using const more > often. I approve. Perhaps we should start building our C code with -Wwrite-strings, which makes "" have type const char* ? Result would be a giant constification patch, probably. Ian.
Re: [PATCH 4/8] tools/libxc: misc: Mark const the parameter 'params' of xc_set_parameters()
Julien Grall writes ("[PATCH 4/8] tools/libxc: misc: Mark const the parameter 'params' of xc_set_parameters()"): > From: Julien Grall > > The parameter 'params' of xc_set_parameters() should never be modified. > So mark it as const. Reviewed-by: Ian Jackson
Re: [PATCH 3/8] tools/libxc: misc: Mark const the parameter 'keys' of xc_send_debug_keys()
Julien Grall writes ("[PATCH 3/8] tools/libxc: misc: Mark const the parameter 'keys' of xc_send_debug_keys()"): > From: Julien Grall > > OCaml is using a string to describe the parameter 'keys' of > xc_send_debug_keys(). Since Ocaml 4.06.01, String_val() will return a > const char * when using -safe-string. This will result to a build > failure because xc_send_debug_keys() expects a char *. > > The function should never modify the parameter 'keys' and therefore the > parameter should be const. Unfortunately, this is not directly possible > because DECLARE_HYPERCALL_BOUNCE() is expecting a non-const variable. > > A new macro DECLARE_HYPERCALL_BOUNCE_IN() is introduced and will take > care of const parameter. The first user will be xc_send_debug_keys() but > this can be used in more place in the future. > > Reported-by: Dario Faggioli > Signed-off-by: Julien Grall Reviewed-by: Ian Jackson
[xen-unstable-smoke test] 149240: tolerable all pass - PUSHED
flight 149240 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/149240/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 14 saverestore-support-checkfail never pass test-armhf-armhf-xl 13 migrate-support-checkfail never pass test-armhf-armhf-xl 14 saverestore-support-checkfail never pass version targeted for testing: xen 5af4698d98d881e786c0909b6308f04696586c49 baseline version: xen 2a94100dd5646fb8abcd29f48553ff10d0788cc7 Last test of basis 149225 2020-03-30 18:01:07 Z0 days Testing same since 149240 2020-03-31 08:00:30 Z0 days1 attempts People who touched revisions under test: Andrew Cooper Jan Beulich Simran Singhal jobs: build-arm64-xsm pass build-amd64 pass build-armhf pass build-amd64-libvirt pass test-armhf-armhf-xl pass test-arm64-arm64-xl-xsm pass test-amd64-amd64-xl-qemuu-debianhvm-amd64pass 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 : To xenbits.xen.org:/home/xen/git/xen.git 2a94100dd5..5af4698d98 5af4698d98d881e786c0909b6308f04696586c49 -> smoke
Re: [PATCH 2/3] xen/x86: ioapic: Rename init_ioapic_mappings() to init_ioapic()
On 30.03.2020 12:37, Roger Pau Monné wrote: > On Fri, Mar 27, 2020 at 07:05:45PM +, Julien Grall wrote: >> From: Julien Grall >> >> The function init_ioapic_mappings() is doing more than initialization >> mappings. It is also initialization the number of IRQs/GSIs supported. >> >> So rename the function to init_ioapic(). This will allow us to re-use >> the name in a follow-up patch. >> >> Signed-off-by: Julien Grall > > Reviewed-by: Roger Pau Monné > > I've got one comment/request however. > >> --- >> xen/arch/x86/apic.c | 2 +- >> xen/arch/x86/io_apic.c| 2 +- >> xen/include/asm-x86/io_apic.h | 2 +- >> 3 files changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c >> index cde67cd87e..c7a54cafc3 100644 >> --- a/xen/arch/x86/apic.c >> +++ b/xen/arch/x86/apic.c >> @@ -978,7 +978,7 @@ __next: >> boot_cpu_physical_apicid = get_apic_id(); >> x86_cpu_to_apicid[0] = get_apic_id(); >> >> -init_ioapic_mappings(); >> +init_ioapic(); > > I would rename this to ioapic_init instead since you are already > changing it. I usually prefer the subsystem name to be prefixed to the > action the function performs instead of the other way around. With this adjustment Acked-by: Jan Beulich Jan
Re: [PATCH 1/3] xen/x86: ioapic: Use true/false in bad_ioapic_register()
On 28.03.2020 12:01, Wei Liu wrote: > On Fri, Mar 27, 2020 at 07:05:44PM +, Julien Grall wrote: >> From: Julien Grall >> >> bad_ioapic_register() is return a bool, so we should switch to >> true/false. > > is return -> returns / is returning > >> >> Signed-off-by: Julien Grall > > Reviewed-by: Wei Liu Acked-by: Jan Beulich
[linux-5.4 test] 149200: regressions - trouble: fail/pass/starved
flight 149200 linux-5.4 real [real] http://logs.test-lab.xenproject.org/osstest/logs/149200/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-amd64-amd64-qemuu-nested-intel 17 debian-hvm-install/l1/l2 fail REGR. vs. 146121 Tests which did not succeed, but are not blocking: test-amd64-amd64-dom0pvh-xl-intel 12 guest-startfail baseline untested test-amd64-i386-xl-pvshim12 guest-start fail never pass test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 13 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 14 saverestore-support-checkfail never pass test-amd64-amd64-libvirt 13 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-i386-libvirt 13 migrate-support-checkfail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2 fail never pass test-arm64-arm64-xl-credit2 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 13 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 14 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 14 saverestore-support-checkfail never pass test-arm64-arm64-xl 13 migrate-support-checkfail never pass test-arm64-arm64-xl 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-arndale 13 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 14 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 12 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 13 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 14 saverestore-support-checkfail never pass test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stop fail never pass test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail never pass test-amd64-i386-xl-qemut-win7-amd64 17 guest-stop fail never pass test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stop fail never pass test-armhf-armhf-xl-credit2 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit1 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 13 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 14 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 13 migrate-support-checkfail never pass test-armhf-armhf-libvirt 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-cubietruck 13 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-rtds 13 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 14 saverestore-support-checkfail never pass test-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stop fail never pass test-amd64-amd64-xl-qemut-ws16-amd64 17 guest-stop fail never pass test-armhf-armhf-libvirt-raw 12 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-vhd 12 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 13 saverestore-support-checkfail never pass test-amd64-i386-xl-qemut-ws16-amd64 17 guest-stop fail never pass test-armhf-armhf-xl 13 migrate-support-checkfail never pass test-armhf-armhf-xl 14 saverestore-support-checkfail never pass test-amd64-i386-xl-qemuu-ws16-amd64 17 guest-stop fail never pass test-amd64-amd64-dom0pvh-xl-amd 2 hosts-allocate starved n/a version targeted for testing: linux462afcd6e7ea94a7027a96a3bb12d0140b0b4216 baseline version: linux122179cb7d648a6f36b20dd6bf34f953cb384c30 Last test of basis 146121 2020-01-15 17:42:04 Z 75 days Failing since146178 2020-01-17 02:59:07 Z 74 days 100 attempts Testing same since 149052 2020-03-26 11:07:11 Z4 days5 attempts -
[XEN PATCH v4 15/18] xen/build: use if_changed to build guest_%.o
Use if_changed for building all guest_%.o objects, and make use of command that already exist. This patch make a change to the way guest_%.o files are built, and now run the same commands that enforce unique symbols. But with patch "xen,symbols: rework file symbols selection", ./symbols should still select the file symbols directive intended to be used for guest_%.o objects. Signed-off-by: Anthony PERARD --- Notes: v4: - remove the introduction of Kbuild's CFLAGS_$@ and simply use make's per-target variable customization. Mostly to avoid using $(eval ) which might not work as expected on make 3.80. xen/arch/x86/mm/Makefile| 14 -- xen/arch/x86/mm/hap/Makefile| 15 +-- xen/arch/x86/mm/shadow/Makefile | 14 -- 3 files changed, 25 insertions(+), 18 deletions(-) diff --git a/xen/arch/x86/mm/Makefile b/xen/arch/x86/mm/Makefile index a2431fde6bb4..a66a57314489 100644 --- a/xen/arch/x86/mm/Makefile +++ b/xen/arch/x86/mm/Makefile @@ -11,11 +11,13 @@ obj-y += p2m.o p2m-pt.o obj-$(CONFIG_HVM) += p2m-ept.o p2m-pod.o obj-y += paging.o -guest_walk_%.o: guest_walk.c Makefile - $(CC) $(c_flags) -DGUEST_PAGING_LEVELS=$* -c $< -o $@ +guest_walk_%.o guest_walk_%.i guest_walk_%.s: CFLAGS-y += -DGUEST_PAGING_LEVELS=$* -guest_walk_%.i: guest_walk.c Makefile - $(CPP) $(filter-out -Wa$(comma)%,$(c_flags)) -DGUEST_PAGING_LEVELS=$* -c $< -o $@ +guest_walk_%.o: guest_walk.c FORCE + $(call if_changed_rule,cc_o_c) -guest_walk_%.s: guest_walk.c Makefile - $(CC) $(filter-out -Wa$(comma)%,$(c_flags)) -DGUEST_PAGING_LEVELS=$* -S $< -o $@ +guest_walk_%.i: guest_walk.c FORCE + $(call if_changed,cpp_i_c) + +guest_walk_%.s: guest_walk.c FORCE + $(call if_changed,cc_s_c) diff --git a/xen/arch/x86/mm/hap/Makefile b/xen/arch/x86/mm/hap/Makefile index 22e7ad54bd33..34720b2fbe2e 100644 --- a/xen/arch/x86/mm/hap/Makefile +++ b/xen/arch/x86/mm/hap/Makefile @@ -5,11 +5,14 @@ obj-y += guest_walk_4level.o obj-y += nested_hap.o obj-y += nested_ept.o -guest_walk_%level.o: guest_walk.c Makefile - $(CC) $(c_flags) -DGUEST_PAGING_LEVELS=$* -c $< -o $@ +guest_walk_%level.o guest_walk_%level.i guest_walk_%level.s: \ +CFLAGS-y += -DGUEST_PAGING_LEVELS=$* -guest_walk_%level.i: guest_walk.c Makefile - $(CPP) $(filter-out -Wa$(comma)%,$(c_flags)) -DGUEST_PAGING_LEVELS=$* -c $< -o $@ +guest_walk_%level.o: guest_walk.c FORCE + $(call if_changed_rule,cc_o_c) -guest_walk_%level.s: guest_walk.c Makefile - $(CC) $(filter-out -Wa$(comma)%,$(c_flags)) -DGUEST_PAGING_LEVELS=$* -S $< -o $@ +guest_walk_%level.i: guest_walk.c FORCE + $(call if_changed,cpp_i_c) + +guest_walk_%level.s: guest_walk.c FORCE + $(call if_changed,cc_s_c) diff --git a/xen/arch/x86/mm/shadow/Makefile b/xen/arch/x86/mm/shadow/Makefile index 23d3ff10802c..e00f9cb1aaba 100644 --- a/xen/arch/x86/mm/shadow/Makefile +++ b/xen/arch/x86/mm/shadow/Makefile @@ -6,11 +6,13 @@ else obj-y += none.o endif -guest_%.o: multi.c Makefile - $(CC) $(c_flags) -DGUEST_PAGING_LEVELS=$* -c $< -o $@ +guest_%.o guest_%.i guest_%.s: CFLAGS-y += -DGUEST_PAGING_LEVELS=$* -guest_%.i: multi.c Makefile - $(CPP) $(filter-out -Wa$(comma)%,$(c_flags)) -DGUEST_PAGING_LEVELS=$* -c $< -o $@ +guest_%.o: multi.c FORCE + $(call if_changed_rule,cc_o_c) -guest_%.s: multi.c Makefile - $(CC) $(filter-out -Wa$(comma)%,$(c_flags)) -DGUEST_PAGING_LEVELS=$* -S $< -o $@ +guest_%.i: multi.c FORCE + $(call if_changed,cpp_i_c) + +guest_%.s: multi.c FORCE + $(call if_changed,cc_s_c) -- Anthony PERARD
[XEN PATCH v4 13/18] xen/build: Use if_changed for prelink*.o
We change the dependencies of prelink-efi.o so that we can use the same command line. The dependency on efi/built_in.o isn't needed because, we already have: efi/*.o: efi/built_in.o to build efi/*.o files that prelink-efi.o needs. Signed-off-by: Anthony PERARD Reviewed-by: Roger Pau Monné Acked-by: Jan Beulich --- Notes: v4: - fix rebuild, prelink.o and prelink-efi.o needs to be in targets xen/arch/x86/Makefile | 10 ++ 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile index eb6f7a6aceca..7676fb1c5bc8 100644 --- a/xen/arch/x86/Makefile +++ b/xen/arch/x86/Makefile @@ -128,11 +128,13 @@ prelink.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink_lto.o prelink-efi.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink-efi_lto.o efi/boot.init.o $(LD) $(XEN_LDFLAGS) -r -o $@ $^ else -prelink.o: $(ALL_OBJS) - $(LD) $(XEN_LDFLAGS) -r -o $@ $^ +prelink.o: $(ALL_OBJS) FORCE + $(call if_changed,ld) + +prelink-efi.o: $(filter-out %/efi/built_in.o,$(ALL_OBJS)) efi/boot.init.o efi/runtime.o efi/compat.o FORCE + $(call if_changed,ld) -prelink-efi.o: $(ALL_OBJS) efi/boot.init.o efi/runtime.o efi/compat.o - $(LD) $(XEN_LDFLAGS) -r -o $@ $(filter-out %/efi/built_in.o,$^) +targets += prelink.o prelink-efi.o endif $(TARGET)-syms: prelink.o xen.lds -- Anthony PERARD
[XEN PATCH v4 14/18] xen,symbols: rework file symbols selection
We want to use the same rune to build mm/*/guest_*.o as the one use to build every other *.o object. The consequence it that file symbols that the program ./symbols prefer changes with CONFIG_ENFORCE_UNIQUE_SYMBOLS=y. (1) Currently we have those two file symbols: guest_walk.c guest_walk_2.o (2) with CONFIG_ENFORCE_UNIQUE_SYMBOLS used on guest_walk.c, we will have: arch/x86/mm/guest_walk.c guest_walk_2.o The order in which those symbols are present may be different. Currently, in case (1) ./symbols chooses the *.o symbol (object file name). But in case (2), may choose the *.c symbol (source file name with path component) if it is first We want to have ./symbols choose the object file name symbol in both cases. So this patch changes that ./symbols prefer the "object file name" symbol over the "source file name with path component" symbols. The new intended order of preference is: - first object file name symbol - first source file name with path components symbol - last source file name without any path component symbol Signed-off-by: Anthony PERARD --- Notes: v4: - rescope enum symbol_type - remove setting values to enums, as it's not needed. - rename the enumeration symbols commmit rewriting: We want to use the same rune to build mm/*/guest_*.o as the one use to build every other *.o object. The consequence it that file symbols that the program ./symbols prefere changes with CONFIG_ENFORCE_UNIQUE_SYMBOLS=y. (1) Currently we have those two file symboles: guest_walk.c guest_walk_2.o (2) with CONFIG_ENFORCE_UNIQUE_SYMBOLS used on guest_walk.c, we will have: arch/x86/mm/guest_walk.c guest_walk_2.o The order in which those symbols are present may be different. Currently, in case (1) ./symbols chooses the *.o symbol (object file name). But in case (2), may choose the *.c symbol (source file name with path component) if it is first. This patch changes that ./symbols prefere the "object file name" symbol over the "source file name with path component" symbols. xen/tools/symbols.c | 20 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/xen/tools/symbols.c b/xen/tools/symbols.c index 9f9e2c990061..b3a9465b32d3 100644 --- a/xen/tools/symbols.c +++ b/xen/tools/symbols.c @@ -84,7 +84,12 @@ static int read_symbol(FILE *in, struct sym_entry *s) { char str[500], type[20] = ""; char *sym, stype; - static enum { symbol, single_source, multi_source } last; + static enum symbol_type { + symbol, + file_source, + path_source, + obj_file, + } last; static char *filename; int rc = -1; @@ -125,13 +130,20 @@ static int read_symbol(FILE *in, struct sym_entry *s) * prefer the first one if that names an object file or has a * directory component (to cover multiply compiled files). */ - bool multi = strchr(str, '/') || (sym && sym[1] == 'o'); + enum symbol_type current; - if (multi || last != multi_source) { + if (sym && sym[1] == 'o') + current = obj_file; + else if (strchr(str, '/')) + current = path_source; + else + current = file_source; + + if (current > last || last == file_source) { free(filename); filename = *str ? strdup(str) : NULL; + last = current; } - last = multi ? multi_source : single_source; goto skip_tail; } -- Anthony PERARD
[XEN PATCH v4 10/18] xen/build: use if_changed on built_in.o
In the case where $(obj-y) is empty, we also replace $(c_flags) by $(XEN_CFLAGS) to avoid generating an .%.d dependency file. This avoid make trying to include %.h file in the ld command if $(obj-y) isn't empty anymore on a second run. Signed-off-by: Anthony PERARD --- Notes: v4: - Have cmd_ld_builtin depends on CONFIG_LTO, which simplify built_in.o rule. xen/Rules.mk | 21 +++-- 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/xen/Rules.mk b/xen/Rules.mk index 5e668f5ba0d8..c744175fd6f0 100644 --- a/xen/Rules.mk +++ b/xen/Rules.mk @@ -130,15 +130,24 @@ include $(BASEDIR)/arch/$(TARGET_ARCH)/Rules.mk c_flags += $(CFLAGS-y) a_flags += $(CFLAGS-y) $(AFLAGS-y) -built_in.o: $(obj-y) $(extra-y) -ifeq ($(obj-y),) - $(CC) $(c_flags) -c -x c /dev/null -o $@ -else +quiet_cmd_ld_builtin = LD $@ ifeq ($(CONFIG_LTO),y) - $(LD_LTO) -r -o $@ $(filter-out $(extra-y),$^) +cmd_ld_builtin = \ +$(LD_LTO) -r -o $@ $(filter-out $(extra-y),$(real-prereqs)) else - $(LD) $(XEN_LDFLAGS) -r -o $@ $(filter-out $(extra-y),$^) +cmd_ld_builtin = \ +$(LD) $(XEN_LDFLAGS) -r -o $@ $(filter-out $(extra-y),$(real-prereqs)) endif + +quiet_cmd_cc_builtin = LD $@ +cmd_cc_builtin = \ +$(CC) $(XEN_CFLAGS) -c -x c /dev/null -o $@ + +built_in.o: $(obj-y) $(extra-y) FORCE +ifeq ($(obj-y),) + $(call if_changed,cc_builtin) +else + $(call if_changed,ld_builtin) endif targets += built_in.o -- Anthony PERARD
[XEN PATCH v4 16/18] build,xsm: Fix multiple call
Both script mkflask.sh and mkaccess_vector.sh generates multiple files. Exploits the 'multi-target pattern rule' trick to call each scripts only once. Signed-off-by: Anthony PERARD --- Notes: v4: - new patch xen/xsm/flask/Makefile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/xen/xsm/flask/Makefile b/xen/xsm/flask/Makefile index 7d0831e2b865..48577cbe3f04 100644 --- a/xen/xsm/flask/Makefile +++ b/xen/xsm/flask/Makefile @@ -26,14 +26,14 @@ mkflask := policy/mkflask.sh quiet_cmd_mkflask = MKFLASK $@ cmd_mkflask = $(CONFIG_SHELL) $(mkflask) $(AWK) include $(FLASK_H_DEPEND) -$(FLASK_H_FILES): $(FLASK_H_DEPEND) $(mkflask) FORCE +$(patsubst include/%,\%/%,$(FLASK_H_FILES)): $(FLASK_H_DEPEND) $(mkflask) FORCE $(call if_changed,mkflask) mkaccess := policy/mkaccess_vector.sh quiet_cmd_mkaccess = MKACCESS VECTOR $@ cmd_mkaccess = $(CONFIG_SHELL) $(mkaccess) $(AWK) $(AV_H_DEPEND) -$(AV_H_FILES): $(AV_H_DEPEND) $(mkaccess) FORCE +$(patsubst include/%,\%/%,$(AV_H_FILES)): $(AV_H_DEPEND) $(mkaccess) FORCE $(call if_changed,mkaccess) obj-bin-$(CONFIG_XSM_FLASK_POLICY) += flask-policy.o -- Anthony PERARD
[XEN PATCH v4 12/18] xen/build: factorise generation of the linker scripts
In Arm and X86 makefile, generating the linker script is the same, so we can simply have both call the same macro. We need to add *.lds files into extra-y so that Rules.mk can find the .*.cmd dependency file and load it. Change made to the command line: - Use of $(CPP) instead of $(CC) -E - Remove -Ui386. We don't compile Xen for i386 anymore, so that macro is never defined. Also it doesn't make sense on Arm. Signed-off-by: Anthony PERARD --- Notes: v4: - fix rebuild by adding FORCE as dependency - Use $(CPP) - remove -Ui386 - avoid using "define" for cmd_cc_lds_S, as adding '; \' on each line is still mandatory for if_changed (or cmd) macro to work. xen/Rules.mk | 6 ++ xen/arch/arm/Makefile | 8 xen/arch/x86/Makefile | 8 3 files changed, 14 insertions(+), 8 deletions(-) diff --git a/xen/Rules.mk b/xen/Rules.mk index e126e4972dec..616c6ae179d8 100644 --- a/xen/Rules.mk +++ b/xen/Rules.mk @@ -236,6 +236,12 @@ cmd_s_S = $(CPP) $(filter-out -Wa$(comma)%,$(a_flags)) $< -o $@ %.s: %.S FORCE $(call if_changed,cpp_s_S) +# Linker scripts, .lds.S -> .lds +quiet_cmd_cc_lds_S = LDS $@ +cmd_cc_lds_S = $(CPP) -P $(filter-out -Wa$(comma)%,$(a_flags)) -o $@ $<; \ +sed -e 's/.*\.lds\.o:/$(@F):/g' <$(dot-target).d >$(dot-target).d.new; \ +mv -f $(dot-target).d.new $(dot-target).d + # Add intermediate targets: # When building objects with specific suffix patterns, add intermediate # targets that the final targets are derived from. diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile index b79ad55646bd..45484d6d11b2 100644 --- a/xen/arch/arm/Makefile +++ b/xen/arch/arm/Makefile @@ -64,6 +64,8 @@ obj-y += vpsci.o obj-y += vuart.o extra-y += $(TARGET_SUBARCH)/head.o +extra-y += xen.lds + #obj-bin-y += o ifdef CONFIG_DTB_FILE @@ -122,10 +124,8 @@ $(TARGET)-syms: prelink.o xen.lds asm-offsets.s: $(TARGET_SUBARCH)/asm-offsets.c $(CC) $(filter-out -flto,$(c_flags)) -S -o $@ $< -xen.lds: xen.lds.S - $(CC) -P -E -Ui386 $(a_flags) -o $@ $< - sed -e 's/xen\.lds\.o:/xen\.lds:/g' <.xen.lds.d >.xen.lds.d.new - mv -f .xen.lds.d.new .xen.lds.d +xen.lds: xen.lds.S FORCE + $(call if_changed,cc_lds_S) dtb.o: $(CONFIG_DTB_FILE) diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile index 44137d919b66..eb6f7a6aceca 100644 --- a/xen/arch/x86/Makefile +++ b/xen/arch/x86/Makefile @@ -72,6 +72,7 @@ obj-y += hpet.o obj-y += vm_event.o obj-y += xstate.o extra-y += asm-macros.i +extra-y += xen.lds x86_emulate.o: x86_emulate/x86_emulate.c x86_emulate/x86_emulate.h @@ -173,6 +174,7 @@ export XEN_BUILD_EFI := $(shell $(CC) $(XEN_CFLAGS) -c efi/check.c -o efi/check. # Check if the linker supports PE. XEN_BUILD_PE := $(if $(XEN_BUILD_EFI),$(shell $(LD) -mi386pep --subsystem=10 -o efi/check.efi efi/check.o 2>/dev/null && echo y)) CFLAGS-$(XEN_BUILD_EFI) += -DXEN_BUILD_EFI +extra-$(XEN_BUILD_PE) += efi.lds $(TARGET).efi: VIRT_BASE = 0x$(shell $(NM) efi/relocs-dummy.o | sed -n 's, A VIRT_START$$,,p') $(TARGET).efi: ALT_BASE = 0x$(shell $(NM) efi/relocs-dummy.o | sed -n 's, A ALT_START$$,,p') @@ -240,10 +242,8 @@ $(BASEDIR)/include/asm-x86/asm-macros.h: asm-macros.i Makefile $(call move-if-changed,$@.new,$@) efi.lds: AFLAGS-y += -DEFI -xen.lds efi.lds: xen.lds.S - $(CC) -P -E -Ui386 $(filter-out -Wa$(comma)%,$(a_flags)) -o $@ $< - sed -e 's/.*\.lds\.o:/$(@F):/g' <.$(@F).d >.$(@F).d.new - mv -f .$(@F).d.new .$(@F).d +xen.lds efi.lds: xen.lds.S FORCE + $(call if_changed,cc_lds_S) boot/mkelf32: boot/mkelf32.c $(HOSTCC) $(HOSTCFLAGS) -o $@ $< -- Anthony PERARD
[XEN PATCH v4 11/18] xen/build: Use if_changed_rules with %.o:%.c targets
Use $(dot-target) to have the target name prefix with a dot. Now, when the CC command has run, it is recorded in .*.cmd file, then if_changed_rules will compare it on subsequent runs. Signed-off-by: Anthony PERARD Reviewed-by: Jan Beulich --- xen/Rules.mk | 26 +- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/xen/Rules.mk b/xen/Rules.mk index c744175fd6f0..e126e4972dec 100644 --- a/xen/Rules.mk +++ b/xen/Rules.mk @@ -173,19 +173,27 @@ FORCE: SRCPATH := $(patsubst $(BASEDIR)/%,%,$(CURDIR)) -%.o: %.c Makefile +quiet_cmd_cc_o_c = CC $@ ifeq ($(CONFIG_ENFORCE_UNIQUE_SYMBOLS),y) - $(CC) $(c_flags) -c $< -o $(@D)/.$(@F).tmp -MQ $@ -ifeq ($(CONFIG_CC_IS_CLANG),y) - $(OBJCOPY) --redefine-sym $<=$(SRCPATH)/$< $(@D)/.$(@F).tmp $@ -else - $(OBJCOPY) --redefine-sym $(
[XEN PATCH v4 18/18] build,include: rework compat-build-header.py
Replace a mix of shell script and python script by all python script. Remove the unnecessary "grep -v '^# [0-9]'". It is to hide the linemarkers generated by the preprocessor. But adding -P inhibit there generation, thus the grep isn't needed anymore. gcc -E -P and clang -E -P have different behavior. While both don't generates linemarkers, gcc also removes all empty lines while clang keep them all. We don't need those empty lines, so we don't generates them in the final compat/%.h headers. (This replace `uniq` which was only de-duplicating empty line.) The only changes in the final generated headers it that they don't have empty lines anymore. Signed-off-by: Anthony PERARD --- Notes: v4: - new patch xen/include/Makefile | 13 ++-- xen/tools/compat-build-header.py | 36 ++-- 2 files changed, 36 insertions(+), 13 deletions(-) diff --git a/xen/include/Makefile b/xen/include/Makefile index 74b26a028902..7e2d0ff667e8 100644 --- a/xen/include/Makefile +++ b/xen/include/Makefile @@ -53,20 +53,11 @@ all: $(headers-y) xlat_lst = xlat.lst compat/%.h: compat/%.i Makefile $(BASEDIR)/tools/compat-build-header.py - set -e; id=_$$(echo $@ | tr '[:lower:]-/.' '[:upper:]___'); \ - echo "#ifndef $$id" >$@.new; \ - echo "#define $$id" >>$@.new; \ - echo "#include " >>$@.new; \ - $(if $(filter-out compat/arch-%.h,$@),echo "#include <$(patsubst compat/%,public/%,$@)>" >>$@.new;) \ - $(if $(prefix-y),echo "$(prefix-y)" >>$@.new;) \ - grep -v '^# [0-9]' $< | \ - $(PYTHON) $(BASEDIR)/tools/compat-build-header.py | uniq >>$@.new; \ - $(if $(suffix-y),echo "$(suffix-y)" >>$@.new;) \ - echo "#endif /* $$id */" >>$@.new + $(PYTHON) $(BASEDIR)/tools/compat-build-header.py <$< $@ "$(prefix-y)" "$(suffix-y)" >>$@.new; \ mv -f $@.new $@ compat/%.i: compat/%.c Makefile - $(CPP) $(filter-out -Wa$(comma)% -include %/include/xen/config.h,$(XEN_CFLAGS)) $(cppflags-y) -o $@ $< + $(CPP) -P $(filter-out -Wa$(comma)% -include %/include/xen/config.h,$(XEN_CFLAGS)) $(cppflags-y) -o $@ $< compat/%.c: public/%.h $(xlat_lst) Makefile $(BASEDIR)/tools/compat-build-source.py mkdir -p $(@D) diff --git a/xen/tools/compat-build-header.py b/xen/tools/compat-build-header.py index b85c43f13faf..d89a900ea02c 100755 --- a/xen/tools/compat-build-header.py +++ b/xen/tools/compat-build-header.py @@ -2,6 +2,12 @@ import re,sys +try: +type(str.maketrans) +except AttributeError: +# For python2 +import string as str + pats = [ [ r"__InClUdE__(.*)", r"#include\1\n#pragma pack(4)" ], [ r"__IfDeF__ (XEN_HAVE.*)", r"#ifdef \1" ], @@ -20,7 +26,33 @@ pats = [ [ r"(^|[^\w])long([^\w]|$$)", r"\1int\2" ] ]; +output_filename = sys.argv[1] + +# tr '[:lower:]-/.' '[:upper:]___' +header_id = '_' + \ +output_filename.upper().translate(str.maketrans('-/.','___')) + +header = """#ifndef {0} +#define {0} +#include """.format(header_id) + +print(header) + +if not re.match("compat/arch-.*.h$", output_filename): +x = output_filename.replace("compat/","public/") +print('#include <%s>' % x) + +def print_if_nonempty(s): +if len(s): +print(s) + +print_if_nonempty(sys.argv[2]) + for line in sys.stdin.readlines(): for pat in pats: -line = re.subn(pat[0], pat[1], line)[0] -print(line.rstrip()) +line = re.sub(pat[0], pat[1], line.rstrip()) +print_if_nonempty(line) + +print_if_nonempty(sys.argv[3]) + +print("#endif /* %s */" % header_id) -- Anthony PERARD
[XEN PATCH v4 17/18] build,include: rework compat-build-source.py
Improvement are: - give the path to xlat.lst as argument - include `grep -v` in compat-build-source.py script, we don't need to write this in several scripted language. - have 'xlat.lst' path as a variable. No changes in final compat/%.h headers. Signed-off-by: Anthony PERARD --- Notes: v4: - new patch xen/include/Makefile | 11 ++- xen/tools/compat-build-source.py | 8 +++- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/xen/include/Makefile b/xen/include/Makefile index 2a10725d689b..74b26a028902 100644 --- a/xen/include/Makefile +++ b/xen/include/Makefile @@ -50,6 +50,8 @@ public-$(CONFIG_ARM) := $(wildcard public/arch-arm/*.h public/arch-arm/*/*.h) .PHONY: all all: $(headers-y) +xlat_lst = xlat.lst + compat/%.h: compat/%.i Makefile $(BASEDIR)/tools/compat-build-header.py set -e; id=_$$(echo $@ | tr '[:lower:]-/.' '[:upper:]___'); \ echo "#ifndef $$id" >$@.new; \ @@ -66,10 +68,9 @@ compat/%.h: compat/%.i Makefile $(BASEDIR)/tools/compat-build-header.py compat/%.i: compat/%.c Makefile $(CPP) $(filter-out -Wa$(comma)% -include %/include/xen/config.h,$(XEN_CFLAGS)) $(cppflags-y) -o $@ $< -compat/%.c: public/%.h xlat.lst Makefile $(BASEDIR)/tools/compat-build-source.py +compat/%.c: public/%.h $(xlat_lst) Makefile $(BASEDIR)/tools/compat-build-source.py mkdir -p $(@D) - grep -v 'DEFINE_XEN_GUEST_HANDLE(long)' $< | \ - $(PYTHON) $(BASEDIR)/tools/compat-build-source.py >$@.new + $(PYTHON) $(BASEDIR)/tools/compat-build-source.py $(xlat_lst) <$< >$@.new mv -f $@.new $@ compat/.xlat/%.h: compat/%.h compat/.xlat/%.lst $(BASEDIR)/tools/get-fields.sh Makefile @@ -80,12 +81,12 @@ compat/.xlat/%.h: compat/%.h compat/.xlat/%.lst $(BASEDIR)/tools/get-fields.sh M mv -f $@.new $@ .PRECIOUS: compat/.xlat/%.lst -compat/.xlat/%.lst: xlat.lst Makefile +compat/.xlat/%.lst: $(xlat_lst) Makefile mkdir -p $(@D) grep -v '^[[:blank:]]*#' $< | sed -ne 's,@arch@,$(compat-arch-y),g' -re 's,[[:blank:]]+$*\.h[[:blank:]]*$$,,p' >$@.new $(call move-if-changed,$@.new,$@) -xlat-y := $(shell sed -ne 's,@arch@,$(compat-arch-y),g' -re 's,^[?!][[:blank:]]+[^[:blank:]]+[[:blank:]]+,,p' xlat.lst | uniq) +xlat-y := $(shell sed -ne 's,@arch@,$(compat-arch-y),g' -re 's,^[?!][[:blank:]]+[^[:blank:]]+[[:blank:]]+,,p' $(xlat_lst) | uniq) xlat-y := $(filter $(patsubst compat/%,%,$(headers-y)),$(xlat-y)) compat/xlat.h: $(addprefix compat/.xlat/,$(xlat-y)) Makefile diff --git a/xen/tools/compat-build-source.py b/xen/tools/compat-build-source.py index c664eb85e633..c7fc2c53db61 100755 --- a/xen/tools/compat-build-source.py +++ b/xen/tools/compat-build-source.py @@ -12,7 +12,11 @@ pats = [ [ r"XEN_GUEST_HANDLE(_[0-9A-Fa-f]+)?", r"COMPAT_HANDLE" ], ]; -xlatf = open('xlat.lst', 'r') +try: +xlatf = open(sys.argv[1], 'r') +except IndexError: +print('missing path to xlat.lst argument') +sys.exit(1) for line in xlatf.readlines(): match = re.subn(r"^\s*\?\s+(\w*)\s.*", r"\1", line.rstrip()) if match[1]: @@ -24,6 +28,8 @@ for pat in pats: pat[0] = re.compile(pat[0]) for line in sys.stdin.readlines(): +if 'DEFINE_XEN_GUEST_HANDLE(long)' in line: +continue for pat in pats: line = re.sub(pat[0], pat[1], line) print(line.rstrip()) -- Anthony PERARD
[XEN PATCH v4 01/18] xen/arm: Rename all early printk macro
We are going to move the generation of the early printk macro into Kconfig. This means all macro will be prefix with CONFIG_. We do that ahead of the change. We also take the opportunity to better name some variables, which are used by only one driver and wouldn't make sens for other UART driver. Thus, - EARLY_UART_REG_SHIFT became CONFIG_EARLY_UART_8250_REG_SHIFT - EARLY_PRINTK_VERSION_* became CONFIG_EARLY_UART_SCIF_VERSION_* The other variables are change to have the prefix CONFIG_EARLY_UART_ when they change a parameter of the driver. So we have now: - CONFIG_EARLY_UART_BAUD_RATE - CONFIG_EARLY_UART_BASE_ADDRESS - CONFIG_EARLY_UART_INIT Signed-off-by: Anthony PERARD Acked-by: Julien Grall Tested-by: Stefano Stabellini --- Notes: v4: - cleanup in head.S, removing extra () and space Patch v3 and early where in "xen/arm: Configure early printk via Kconfig" series. v3: - Revert the renaming of EARLY_PRINTK to CONFIG_EARLY_PRINTK in the makefiles, as this doesn't work well with user provided CONFIG_EARLY_PRINTK. This is done in the following patch instead. - rename CONFIG_EARLY_UART_BAUD_RATE to CONFIG_EARLY_UART_PL011_BAUD_RATE xen/arch/arm/Rules.mk | 14 +++--- xen/arch/arm/arm32/debug-8250.inc | 2 +- xen/arch/arm/arm32/debug-pl011.inc | 4 ++-- xen/arch/arm/arm32/debug-scif.inc | 4 ++-- xen/arch/arm/arm32/debug.S | 4 ++-- xen/arch/arm/arm32/head.S | 10 +- xen/arch/arm/arm64/debug-8250.inc | 4 ++-- xen/arch/arm/arm64/debug-pl011.inc | 4 ++-- xen/arch/arm/arm64/debug.S | 4 ++-- xen/arch/arm/arm64/head.S | 10 +- xen/include/asm-arm/early_printk.h | 2 +- 11 files changed, 31 insertions(+), 31 deletions(-) diff --git a/xen/arch/arm/Rules.mk b/xen/arch/arm/Rules.mk index 022a3a6f82ba..faa09ea111ec 100644 --- a/xen/arch/arm/Rules.mk +++ b/xen/arch/arm/Rules.mk @@ -66,9 +66,9 @@ endif endif ifeq ($(EARLY_PRINTK_INC),scif) ifneq ($(word 3,$(EARLY_PRINTK_CFG)),) -CFLAGS-y += -DEARLY_PRINTK_VERSION_$(word 3,$(EARLY_PRINTK_CFG)) +CFLAGS-y += -DCONFIG_EARLY_UART_SCIF_VERSION_$(word 3,$(EARLY_PRINTK_CFG)) else -CFLAGS-y += -DEARLY_PRINTK_VERSION_NONE +CFLAGS-y += -DCONFIG_EARLY_UART_SCIF_VERSION_NONE endif endif @@ -77,11 +77,11 @@ EARLY_PRINTK := y endif CFLAGS-$(EARLY_PRINTK) += -DCONFIG_EARLY_PRINTK -CFLAGS-$(EARLY_PRINTK_INIT_UART) += -DEARLY_PRINTK_INIT_UART -CFLAGS-$(EARLY_PRINTK) += -DEARLY_PRINTK_INC=\"debug-$(EARLY_PRINTK_INC).inc\" -CFLAGS-$(EARLY_PRINTK) += -DEARLY_PRINTK_BAUD=$(EARLY_PRINTK_BAUD) -CFLAGS-$(EARLY_PRINTK) += -DEARLY_UART_BASE_ADDRESS=$(EARLY_UART_BASE_ADDRESS) -CFLAGS-$(EARLY_PRINTK) += -DEARLY_UART_REG_SHIFT=$(EARLY_UART_REG_SHIFT) +CFLAGS-$(EARLY_PRINTK_INIT_UART) += -DCONFIG_EARLY_UART_INIT +CFLAGS-$(EARLY_PRINTK) += -DCONFIG_EARLY_PRINTK_INC=\"debug-$(EARLY_PRINTK_INC).inc\" +CFLAGS-$(EARLY_PRINTK) += -DCONFIG_EARLY_UART_PL011_BAUD_RATE=$(EARLY_PRINTK_BAUD) +CFLAGS-$(EARLY_PRINTK) += -DCONFIG_EARLY_UART_BASE_ADDRESS=$(EARLY_UART_BASE_ADDRESS) +CFLAGS-$(EARLY_PRINTK) += -DCONFIG_EARLY_UART_8250_REG_SHIFT=$(EARLY_UART_REG_SHIFT) else # !CONFIG_DEBUG diff --git a/xen/arch/arm/arm32/debug-8250.inc b/xen/arch/arm/arm32/debug-8250.inc index 0759a27ee157..c47e8be4aaf3 100644 --- a/xen/arch/arm/arm32/debug-8250.inc +++ b/xen/arch/arm/arm32/debug-8250.inc @@ -23,7 +23,7 @@ */ .macro early_uart_ready rb rc 1: -ldr \rc, [\rb, #(UART_LSR << EARLY_UART_REG_SHIFT)] /* Read LSR */ +ldr \rc, [\rb, #(UART_LSR << CONFIG_EARLY_UART_8250_REG_SHIFT)] /* Read LSR */ tst \rc, #UART_LSR_THRE /* Check Xmit holding register flag */ beq 1b /* Wait for the UART to be ready */ .endm diff --git a/xen/arch/arm/arm32/debug-pl011.inc b/xen/arch/arm/arm32/debug-pl011.inc index ec462eabab5c..214f68dc95bd 100644 --- a/xen/arch/arm/arm32/debug-pl011.inc +++ b/xen/arch/arm/arm32/debug-pl011.inc @@ -25,9 +25,9 @@ * rd: scratch register 2 (unused here) */ .macro early_uart_init rb, rc, rd -mov \rc, #(7372800 / EARLY_PRINTK_BAUD % 16) +mov \rc, #(7372800 / CONFIG_EARLY_UART_PL011_BAUD_RATE % 16) str \rc, [\rb, #FBRD] /* -> UARTFBRD (Baud divisor fraction) */ -mov \rc, #(7372800 / EARLY_PRINTK_BAUD / 16) +mov \rc, #(7372800 / CONFIG_EARLY_UART_PL011_BAUD_RATE / 16) str \rc, [\rb, #IBRD] /* -> UARTIBRD (Baud divisor integer) */ mov \rc, #0x60/* 8n1 */ str \rc, [\rb, #LCR_H] /* -> UARTLCR_H (Line control) */ diff --git a/xen/arch/arm/arm32/debug-scif.inc b/xen/arch/arm/arm32/debug-scif.inc index 3f01c909c238..b2b82501e792 100644 --- a/xen/arch/arm/arm32/debug-scif.inc +++ b/xen/arch/arm/arm32/debug-scif.inc @@ -19,10 +19,10 @@ #include -#ifdef EARLY_PRINTK_VERSION_NONE +#ifdef CONFIG_EARLY_UART_SCIF_VERSION_NONE #define STATUS_R
[XEN PATCH v4 02/18] xen/arm: Configure early printk via Kconfig
At the moment, early printk can only be configured on the make command line. It is not very handy because a user has to remove the option everytime it is using another command other than compiling the hypervisor. Furthermore, early printk is one of the few odds one that are not using Kconfig. So this is about time to move it to Kconfig. The new kconfigs options allow a user to eather select a UART driver to use at boot time, and set the parameters, or it is still possible to select a platform which will set the parameters. If CONFIG_EARLY_PRINTK is present in the environment or on the make command line, make will return an error. Signed-off-by: Julien Grall Signed-off-by: Anthony PERARD Tested-by: Stefano Stabellini --- Notes: v4: - Add range to EARLY_UART_BASE_ADDRESS so that kconfig won't accept address higher than 4G on ARM_32 - have EARLY_PRINTK_THUNDERX depends on ARM_64 because the default base address is above 4G - Add deprecation warning to the help of the choice of early printk driver/platform. - in early-printk.txt, add that early printk is available with EXPERT. Patch v3 and early where in "xen/arm: Configure early printk via Kconfig" series. v3: - rename EARLY_PRINK to CONFIG_EARLY_PRINTK in makefile here (which select which object to build). - rename EARLY_UART_BAUD_RATE to EARLY_UART_PL011_BAUD_RATE - typos - drop the list of aliases in early-printk.txt. Kconfig choice menu should be enough. - reword early-printk.txt. - rework how EARLY_PRINTK is set to Y and use that instead of a list of all EARLY_UART_* - Add a check to ask user to use Kconfig to set early printk. - rework the possible choice to have all uart driver and platform specific option together. - have added or reword prompt and help messages of the different options. The platform specific option don't have extended help, the prompt is probably enough. (The non-platform specific options have the help message that Julien have written in the first version.) - have made EARLY_UART_INIT dependent on the value of EARLY_UART_PL011_BAUD_RATE so that there is no need to expose _INIT to users. docs/misc/arm/early-printk.txt| 71 ++--- xen/Kconfig.debug | 2 + xen/arch/arm/Kconfig.debug| 289 ++ xen/arch/arm/Makefile | 2 +- xen/arch/arm/Rules.mk | 74 + xen/arch/arm/arm32/Makefile | 2 +- xen/arch/arm/arm64/Makefile | 2 +- .../minios.cfg => xen/arch/x86/Kconfig.debug | 0 8 files changed, 319 insertions(+), 123 deletions(-) create mode 100644 xen/arch/arm/Kconfig.debug copy stubdom/c/minios.cfg => xen/arch/x86/Kconfig.debug (100%) diff --git a/docs/misc/arm/early-printk.txt b/docs/misc/arm/early-printk.txt index 89e081e51eaf..aa22826075a4 100644 --- a/docs/misc/arm/early-printk.txt +++ b/docs/misc/arm/early-printk.txt @@ -1,64 +1,39 @@ How to enable early printk -Early printk can only be enabled if debug=y. You may want to enable it if -you are debbuging code that executes before the console is initialized. +Early printk can only be enabled if CONFIG_DEBUG=y or in EXPERT mode. You +may want to enable it if you are debugging code that executes before the +console is initialized. Note that selecting this option will limit Xen to a single UART definition. Attempting to boot Xen image on a different platform *will not work*, so this option should not be enable for Xens that are intended to be portable. -CONFIG_EARLY_PRINTK=,, +Select one of the "Early printk via * UART" in the choice possible for +"Early printk" in the "Debugging options" of Kconfig. You will then need to +set other options, which depends on the driver selected. - and are mandatory arguments: +CONFIG_EARLY_UART_BASE_ADDRESS is a mandatory argument, it is the base +physical address of the UART to use. - - is the name of the driver, see xen/arch/arm/arm{32,64}/debug-*.inc -(where corresponds to the wildcarded *). - - is the base physical address of the UART to use +Other options depends on the driver selected: + - 8250 +- CONFIG_EARLY_UART_8250_REG_SHIFT is, optionally, the left-shift to + apply to the register offsets within the uart. + - pl011 +- CONFIG_EARLY_UART_PL011_BAUD_RATE is, optionally, a baud rate which + should be used to configure the UART at start of day. - varies depending on : + If CONFIG_EARLY_UART_PL011_BAUD_RATE is set to 0 then the code will + not try to initialize the UART, so that bootloader or firmware + settings can be used for maximum compatibility. + - scif +- CONFIG_EARLY_UART_SCIF_VERSION_* is, optionally, the interface version + of the UART. Default to version NONE. - - 8250,, -- is, optionally, the left-shift to apply to
[XEN PATCH v4 00/18] xen: Build system improvements
Patch series available in this git branch: https://xenbits.xen.org/git-http/people/aperard/xen-unstable.git br.build-system-xen-v4 v4: - some patch already applied. - Have added patches from "xen/arm: Configure early printk via Kconfig" series. - Some new patch to add documentation or fix issues, and patch to improve compat header generation. Other changes are detailed in patches. v3: - new patches that do some cleanup or fix issues - have rework most patches, to have better commit message or change the coding style, or fix issues that I've seen. There were some cases where CFLAGS were missing. See patch notes for details - introduce if_changed*. That plenty of new patches on top of what we had in v2. (those changes ignore CONFIG_LTO=y, I'll see about fixing that later) (There is more to come in order to use fixdep from Linux, but that's not ready) v2.1: - some fixes v2: Rather than taking Kbuild and making it work with Xen, the v2 takes the opposite approach of slowly transforming our current build system into Kbuild. That have the advantage of keeping all the feature we have and making the patches much easier to review. Kconfig update is done in an other patch series. Hi, I have work toward building Xen (the hypervisor) with Linux's build system, Kbuild. The main reason for that is to be able to have out-of-tree build. It's annoying when a build fail because of the pvshim. Other benefit is a much faster rebuild, and `make clean` doesn't take ages, and better dependencies to figure out what needs to be rebuild. So, we are not there yet, but the series already contain quite a few improvement and cleanup. More patches are going to be added to the series. Cheers, Anthony PERARD (18): xen/arm: Rename all early printk macro xen/arm: Configure early printk via Kconfig build,arm: Fix deps check of head.o xen/build: include include/config/auto.conf in main Makefile xen/build: use new $(c_flags) and $(a_flags) instead of $(CFLAGS) xen/build: have the root Makefile generates the CFLAGS build: Introduce documentation for xen Makefiles xen/build: introduce if_changed and if_changed_rule xen/build: Start using if_changed xen/build: use if_changed on built_in.o xen/build: Use if_changed_rules with %.o:%.c targets xen/build: factorise generation of the linker scripts xen/build: Use if_changed for prelink*.o xen,symbols: rework file symbols selection xen/build: use if_changed to build guest_%.o build,xsm: Fix multiple call build,include: rework compat-build-source.py build,include: rework compat-build-header.py .gitignore| 1 + docs/misc/arm/early-printk.txt| 71 ++--- docs/misc/xen-makefiles/makefiles.rst | 186 +++ xen/Kconfig.debug | 2 + xen/Makefile | 209 +++-- xen/Rules.mk | 235 -- xen/arch/arm/Kconfig.debug| 289 ++ xen/arch/arm/Makefile | 27 +- xen/arch/arm/Rules.mk | 93 -- xen/arch/arm/{Rules.mk => arch.mk}| 79 + xen/arch/arm/arm32/Makefile | 2 +- xen/arch/arm/arm32/debug-8250.inc | 2 +- xen/arch/arm/arm32/debug-pl011.inc| 4 +- xen/arch/arm/arm32/debug-scif.inc | 4 +- xen/arch/arm/arm32/debug.S| 4 +- xen/arch/arm/arm32/head.S | 10 +- xen/arch/arm/arm64/Makefile | 2 +- xen/arch/arm/arm64/debug-8250.inc | 4 +- xen/arch/arm/arm64/debug-pl011.inc| 4 +- xen/arch/arm/arm64/debug.S| 4 +- xen/arch/arm/arm64/head.S | 10 +- xen/arch/arm/efi/Makefile | 2 +- .../minios.cfg => xen/arch/x86/Kconfig.debug | 0 xen/arch/x86/Makefile | 41 +-- xen/arch/x86/Rules.mk | 91 +- xen/arch/x86/{Rules.mk => arch.mk}| 17 +- xen/arch/x86/efi/Makefile | 9 +- xen/arch/x86/mm/Makefile | 14 +- xen/arch/x86/mm/hap/Makefile | 15 +- xen/arch/x86/mm/shadow/Makefile | 14 +- xen/common/libelf/Makefile| 14 +- xen/common/libfdt/Makefile| 13 +- xen/include/Makefile | 24 +- xen/include/asm-arm/early_printk.h| 2 +- xen/scripts/Kbuild.include| 112 +++ xen/tools/compat-build-header.py | 36 ++- xen/tools/compat-build-source.py | 8 +- xen/tools/symbols.c | 20 +- xen/xsm/flask/Makefile| 19 +- xen/xsm/flask/ss/Makefile | 2 +- 40 files changed, 1145 insertions(+), 550 deletions(-) create mode 100644 docs/misc/xen-make
[XEN PATCH v4 03/18] build,arm: Fix deps check of head.o
arm*/head.o isn't in obj-y or extra-y, so make don't load the associated .*.d file (or .*.cmd file when if_changed will be used). There is a workaround where .*.d file is added manually into DEPS. Changing DEPS isn't needed, we can simply add head.o into extra-y and the dependency files will be loaded. Signed-off-by: Anthony PERARD --- Notes: v4: - new patch, fix rebuild of head.o with "xen/build: Start using if_changed" applied xen/arch/arm/Makefile | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile index 12f92a4bd3f9..7273f356f190 100644 --- a/xen/arch/arm/Makefile +++ b/xen/arch/arm/Makefile @@ -62,6 +62,7 @@ obj-$(CONFIG_SBSA_VUART_CONSOLE) += vpl011.o obj-y += vsmc.o obj-y += vpsci.o obj-y += vuart.o +extra-y += $(TARGET_SUBARCH)/head.o #obj-bin-y += o @@ -72,8 +73,6 @@ endif ALL_OBJS := $(TARGET_SUBARCH)/head.o $(ALL_OBJS) -DEPS += $(TARGET_SUBARCH)/.head.o.d - ifdef CONFIG_LIVEPATCH all_symbols = --all-symbols ifdef CONFIG_FAST_SYMBOL_LOOKUP -- Anthony PERARD
[XEN PATCH v4 05/18] xen/build: use new $(c_flags) and $(a_flags) instead of $(CFLAGS)
In a later patch ("xen/build: have the root Makefile generates the CFLAGS), we want to generate the CFLAGS in xen/Makefile, then export it and have Rules.mk use a CFLAGS from the environment variables. That changes the flavor of the CFLAGS and flags intended for one target (like -D__OBJECT_FILE__ and -M%) gets propagated and duplicated. So we start by moving such flags out of $(CFLAGS) and into $(c_flags) which is to be modified by only Rules.mk. __OBJECT_FILE__ is only used by arch/x86/mm/*.c files, so having it in $(c_flags) is enough, we don't need it in $(a_flags). For include/Makefile and as-insn we can keep using CFLAGS, but since it doesn't have -M* flags anymore there is no need to filter them out. The XEN_BUILD_EFI tests in arch/x86/Makefile was filtering out CFLAGS-y, but according to dd40177c1bc8 ("x86-64/EFI: add CFLAGS to check compile"), it was done to filter out -MF. CFLAGS doesn't have those flags anymore, so no filtering is needed. This is inspired by the way Kbuild generates CFLAGS for each targets. Signed-off-by: Anthony PERARD Reviewed-by: Roger Pau Monné --- Notes: v4: - drop change in as-insn macro, and keep filtering-out -M% %.d v3: - include/Makefile: Keep using CFLAGS, but since it doesn't have -M* flags anymore, no need to filter it. - Write c_flags and a_flags on a single line. - arch/x86/Makefile: remove the filter-out of dependency flags they are remove from CFLAGS anyway. (was intended to be done in xen/build: have the root Makefile generates the CFLAGS originally, move the change to this patch). - also modify as-insn as it is now xen/ only. xen/Rules.mk| 23 +++ xen/arch/arm/Makefile | 4 ++-- xen/arch/x86/Makefile | 6 +++--- xen/arch/x86/mm/Makefile| 6 +++--- xen/arch/x86/mm/hap/Makefile| 6 +++--- xen/arch/x86/mm/shadow/Makefile | 6 +++--- xen/include/Makefile| 2 +- 7 files changed, 26 insertions(+), 27 deletions(-) diff --git a/xen/Rules.mk b/xen/Rules.mk index 9079df7978a7..3408a35dbf53 100644 --- a/xen/Rules.mk +++ b/xen/Rules.mk @@ -57,7 +57,6 @@ CFLAGS += -Werror -Wredundant-decls -Wno-pointer-arith $(call cc-option-add,CFLAGS,CC,-Wvla) CFLAGS += -pipe -D__XEN__ -include $(BASEDIR)/include/xen/config.h CFLAGS-$(CONFIG_DEBUG_INFO) += -g -CFLAGS += '-D__OBJECT_FILE__="$@"' ifneq ($(CONFIG_CC_IS_CLANG),y) # Clang doesn't understand this command line argument, and doesn't appear to @@ -70,9 +69,6 @@ AFLAGS += -D__ASSEMBLY__ ALL_OBJS := $(ALL_OBJS-y) -# Get gcc to generate the dependencies for us. -CFLAGS-y += -MMD -MP -MF $(@D)/.$(@F).d - CFLAGS += $(CFLAGS-y) # allow extra CFLAGS externally via EXTRA_CFLAGS_XEN_CORE CFLAGS += $(EXTRA_CFLAGS_XEN_CORE) @@ -146,9 +142,12 @@ endif # Always build obj-bin files as binary even if they come from C source. $(obj-bin-y): CFLAGS := $(filter-out -flto,$(CFLAGS)) +c_flags = -MMD -MP -MF $(@D)/.$(@F).d $(CFLAGS) '-D__OBJECT_FILE__="$@"' +a_flags = -MMD -MP -MF $(@D)/.$(@F).d $(AFLAGS) + built_in.o: $(obj-y) $(extra-y) ifeq ($(obj-y),) - $(CC) $(CFLAGS) -c -x c /dev/null -o $@ + $(CC) $(c_flags) -c -x c /dev/null -o $@ else ifeq ($(CONFIG_LTO),y) $(LD_LTO) -r -o $@ $(filter-out $(extra-y),$^) @@ -159,7 +158,7 @@ endif built_in_bin.o: $(obj-bin-y) $(extra-y) ifeq ($(obj-bin-y),) - $(CC) $(AFLAGS) -c -x assembler /dev/null -o $@ + $(CC) $(a_flags) -c -x assembler /dev/null -o $@ else $(LD) $(LDFLAGS) -r -o $@ $(filter-out $(extra-y),$^) endif @@ -178,7 +177,7 @@ SRCPATH := $(patsubst $(BASEDIR)/%,%,$(CURDIR)) %.o: %.c Makefile ifeq ($(CONFIG_ENFORCE_UNIQUE_SYMBOLS),y) - $(CC) $(CFLAGS) -c $< -o $(@D)/.$(@F).tmp -MQ $@ + $(CC) $(c_flags) -c $< -o $(@D)/.$(@F).tmp -MQ $@ ifeq ($(CONFIG_CC_IS_CLANG),y) $(OBJCOPY) --redefine-sym $<=$(SRCPATH)/$< $(@D)/.$(@F).tmp $@ else @@ -186,11 +185,11 @@ else endif rm -f $(@D)/.$(@F).tmp else - $(CC) $(CFLAGS) -c $< -o $@ + $(CC) $(c_flags) -c $< -o $@ endif %.o: %.S Makefile - $(CC) $(AFLAGS) -c $< -o $@ + $(CC) $(a_flags) -c $< -o $@ $(filter %.init.o,$(obj-y) $(obj-bin-y) $(extra-y)): %.init.o: %.o Makefile $(OBJDUMP) -h $< | sed -n '/[0-9]/{s,00*,0,g;p;}' | while read idx name sz rest; do \ @@ -205,12 +204,12 @@ $(filter %.init.o,$(obj-y) $(obj-bin-y) $(extra-y)): %.init.o: %.o Makefile $(OBJCOPY) $(foreach s,$(SPECIAL_DATA_SECTIONS),--rename-section .$(s)=.init.$(s)) $< $@ %.i: %.c Makefile - $(CPP) $(filter-out -Wa$(comma)%,$(CFLAGS)) $< -o $@ + $(CPP) $(filter-out -Wa$(comma)%,$(c_flags)) $< -o $@ %.s: %.c Makefile - $(CC) $(filter-out -Wa$(comma)%,$(CFLAGS)) -S $< -o $@ + $(CC) $(filter-out -Wa$(comma)%,$(c_flags)) -S $< -o $@ %.s: %.S Makefile - $(CPP) $(filter-out -Wa$(comma)%,$(AFLAGS)) $< -o $@ + $(CPP) $(filter-out -Wa$(comma)%,$(a_flag
Re: [PATCH] Revert "domctl: improve locking during domain destruction"
Hi Jim, On 26/03/2020 16:55, Jim Fehlig wrote: On 3/25/20 1:11 AM, Jan Beulich wrote: On 24.03.2020 19:39, Julien Grall wrote: On 24/03/2020 16:13, Jan Beulich wrote: On 24.03.2020 16:21, Hongyan Xia wrote: From: Hongyan Xia In contrast, after dropping that commit, parallel domain destructions will just fail to take the domctl lock, creating a hypercall continuation and backing off immediately, allowing the thread that holds the lock to destroy a domain much more quickly and allowing backed-off threads to process events and irqs. On a 144-core server with 4TiB of memory, destroying 32 guests (each with 4 vcpus and 122GiB memory) simultaneously takes: before the revert: 29 minutes after the revert: 6 minutes This wants comparing against numbers demonstrating the bad effects of the global domctl lock. Iirc they were quite a bit higher than 6 min, perhaps depending on guest properties. Your original commit message doesn't contain any clue in which cases the domctl lock was an issue. So please provide information on the setups you think it will make it worse. I did never observe the issue myself - let's see whether one of the SUSE people possibly involved in this back then recall (or have further pointers; Jim, Charles?), or whether any of the (partly former) Citrix folks do. My vague recollection is that the issue was the tool stack as a whole stalling for far too long in particular when destroying very large guests. I too only have a vague memory of the issue but do recall shutting down large guests (e.g. 500GB) taking a long time and blocking other toolstack operations. I haven't checked on the behavior in quite some time though. It might be worth checking how toolstack operations (such as domain creating) is affected by the revert. @Hongyan would you be able to test it? One important aspect not discussed in the commit message at all is that holding the domctl lock block basically _all_ tool stack operations (including e.g. creation of new guests), whereas the new issue attempted to be addressed is limited to just domain cleanup. I more vaguely recall shutting down the host taking a *long* time when dom0 had large amounts of memory, e.g. when it had all host memory (no dom0_mem= setting and autoballooning enabled). AFAIK, we never relinquish memory from dom0. So I am not sure how a large amount of memory in Dom0 would affect the host shutting down. Cheers, -- Julien Grall
[XEN PATCH v4 06/18] xen/build: have the root Makefile generates the CFLAGS
Instead of generating the CFLAGS in Rules.mk everytime we enter a new subdirectory, we are going to generate most of them a single time, and export the result in the environment so that Rules.mk can use it. The only flags left to be generated are the ones that depend on the targets, but the variable $(c_flags) takes care of that. Arch specific CFLAGS are generated by a new file "arch/*/arch.mk" which is included by the root Makefile. We export the *FLAGS via the environment variables XEN_*FLAGS because Rules.mk still includes Config.mk and would add duplicated flags to CFLAGS. When running Rules.mk in the root directory (xen/), the variable `root-make-done' is set, so `need-config' will remain undef and so the root Makefile will not generate the cflags again. We can't use CFLAGS in subdirectories to add flags to particular targets, instead start to use CFLAGS-y. Idem for AFLAGS. So there are two different CFLAGS-y, the one in xen/Makefile (and arch.mk), and the one in subdirs that Rules.mk is going to use. We can't add to XEN_CFLAGS because it is exported, so making change to it might be propagated to subdirectory which isn't intended. Some style change are introduced in this patch: when LDFLAGS_DIRECT is included in LDFLAGS use of CFLAGS-$(CONFIG_INDIRECT_THUNK) instead of ifeq(). There is on FIXME added about LTO build, but since LTO is marked as BROKEN, this commit doesn't attempt to filter -flto flags out of the CFLAGS. Signed-off-by: Anthony PERARD --- Notes: v4: - typos - Adding $(AFLAGS-y) to $(AFLAGS) v3: - squash "xen/build: introduce ccflags-y and CFLAGS_$@" here, with those changes: - rename ccflags-y to simply CFLAGS-y and start using AFLAGS-y in subdirs. - remove CFLAGS_$@, we don't need it yet. - fix build of xen.lds and efi.lds which needed -D to be a_flags - remove arch_ccflags, and modify c_flags directly with that change, reorder c_flags, so that target specific flags are last. - remove HAVE_AS_QUOTED_SYM from envvar and check XEN_CFLAGS to find if it's there when adding -D__OBJECT_LABEL__. - fix missing some flags in AFLAGS (like -fshort-wchar in xen/arch/x86/efi/Makefile, and -D__OBJECT_LABEL__ and CFLAGS-stack-boundary) - keep COV_FLAGS generation in Rules.mk since it doesn't invovle to call CC - fix clang test for "asm()-s support .include." (in a new patch done ahead) - include Kconfig.include in xen/Makefile because as-option-add is defined there now. xen/Makefile | 58 +++ xen/Rules.mk | 74 +++- xen/arch/arm/Makefile | 10 ++-- xen/arch/arm/Rules.mk | 23 xen/arch/arm/{Rules.mk => arch.mk} | 5 -- xen/arch/arm/efi/Makefile | 2 +- xen/arch/x86/Makefile | 24 xen/arch/x86/Rules.mk | 91 ++ xen/arch/x86/{Rules.mk => arch.mk} | 17 ++ xen/arch/x86/efi/Makefile | 2 +- xen/common/libelf/Makefile | 4 +- xen/common/libfdt/Makefile | 4 +- xen/include/Makefile | 2 +- xen/xsm/flask/Makefile | 2 +- xen/xsm/flask/ss/Makefile | 2 +- 15 files changed, 115 insertions(+), 205 deletions(-) copy xen/arch/arm/{Rules.mk => arch.mk} (85%) copy xen/arch/x86/{Rules.mk => arch.mk} (87%) diff --git a/xen/Makefile b/xen/Makefile index 8375070e0d41..372692841913 100644 --- a/xen/Makefile +++ b/xen/Makefile @@ -115,6 +115,64 @@ $(KCONFIG_CONFIG): include/config/%.conf include/config/%.conf.cmd: $(KCONFIG_CONFIG) $(MAKE) $(kconfig) syncconfig +ifeq ($(CONFIG_DEBUG),y) +CFLAGS += -O1 +else +CFLAGS += -O2 +endif + +ifeq ($(CONFIG_FRAME_POINTER),y) +CFLAGS += -fno-omit-frame-pointer +else +CFLAGS += -fomit-frame-pointer +endif + +CFLAGS += -nostdinc -fno-builtin -fno-common +CFLAGS += -Werror -Wredundant-decls -Wno-pointer-arith +$(call cc-option-add,CFLAGS,CC,-Wvla) +CFLAGS += -pipe -D__XEN__ -include $(BASEDIR)/include/xen/config.h +CFLAGS-$(CONFIG_DEBUG_INFO) += -g + +ifneq ($(CONFIG_CC_IS_CLANG),y) +# Clang doesn't understand this command line argument, and doesn't appear to +# have an suitable alternative. The resulting compiled binary does function, +# but has an excessively large symbol table. +CFLAGS += -Wa,--strip-local-absolute +endif + +AFLAGS += -D__ASSEMBLY__ + +CFLAGS += $(CFLAGS-y) +# allow extra CFLAGS externally via EXTRA_CFLAGS_XEN_CORE +CFLAGS += $(EXTRA_CFLAGS_XEN_CORE) + +# Most CFLAGS are safe for assembly files: +# -std=gnu{89,99} gets confused by #-prefixed end-of-line comments +# -flto makes no sense and annoys clang +AFLAGS += $(filter-out -std=gnu% -flto,$(CFLAGS)) $(AFLAGS-y) + +# LDFLAGS are only passed directly to $(LD) +LDFLAGS += $(LDFLAGS_DIRECT) $(LDFLAGS-y) + +ifeq ($(CONFIG_UBSAN),y) +CFLAGS_UBSAN := -fsanitize=undefined +else +CFLAGS_UBSAN := +endif
[XEN PATCH v4 09/18] xen/build: Start using if_changed
This patch start to use if_changed introduced in a previous commit. Whenever if_changed is called, the target must have FORCE as dependency so that if_changed can check if the command line to be run has changed, so the macro $(real-prereqs) must be used to discover the dependencies without "FORCE". Whenever a target isn't in obj-y, it should be added to extra-y so the .*.cmd dependency file associated with the target can be loaded. This is done for xsm/flask/ and both common/lib{elf,fdt}/ and arch/x86/Makefile. For the targets that generate .*.d dependency files, there's going to be two dependency files (.*.d and .*.cmd) until we can merge them together in a later patch via fixdep from Linux. One cleanup, libelf-relocate.o doesn't exist anymore. We import cmd_ld and cmd_objcopy from Linux v5.4. Signed-off-by: Anthony PERARD Reviewed-by: Roger Pau Monné Acked-by: Jan Beulich --- Notes: v4: - typos - fix missing FORCE in libfdt/Makefile - typo flask vs flash in xsm - in xsm, use *_H_DEPEND in command lines of mkaccess and mkflask instead of $(real-prereq) to avoid including the script as argument of itself. xen/Rules.mk | 68 +++--- xen/arch/arm/Makefile | 4 +-- xen/arch/x86/Makefile | 1 + xen/arch/x86/efi/Makefile | 7 ++-- xen/common/libelf/Makefile | 12 --- xen/common/libfdt/Makefile | 11 +++--- xen/xsm/flask/Makefile | 17 +++--- 7 files changed, 85 insertions(+), 35 deletions(-) diff --git a/xen/Rules.mk b/xen/Rules.mk index f531fd5e342d..5e668f5ba0d8 100644 --- a/xen/Rules.mk +++ b/xen/Rules.mk @@ -56,6 +56,18 @@ SPECIAL_DATA_SECTIONS := rodata $(foreach a,1 2 4 8 16, \ include Makefile +# Linking +# --- + +quiet_cmd_ld = LD $@ +cmd_ld = $(LD) $(XEN_LDFLAGS) -r -o $@ $(real-prereqs) + +# Objcopy +# --- + +quiet_cmd_objcopy = OBJCOPY $@ +cmd_objcopy = $(OBJCOPY) $(OBJCOPYFLAGS) $< $@ + define gendep ifneq ($(1),$(subst /,:,$(1))) DEPS += $(dir $(1)).$(notdir $(1)).d @@ -165,29 +177,47 @@ else $(CC) $(c_flags) -c $< -o $@ endif -%.o: %.S Makefile - $(CC) $(a_flags) -c $< -o $@ +quiet_cmd_cc_o_S = CC $@ +cmd_cc_o_S = $(CC) $(a_flags) -c $< -o $@ + +%.o: %.S FORCE + $(call if_changed,cc_o_S) + + +quiet_cmd_obj_init_o = INIT_O $@ +define cmd_obj_init_o +$(OBJDUMP) -h $< | sed -n '/[0-9]/{s,00*,0,g;p;}' | while read idx name sz rest; do \ +case "$$name" in \ +.*.local) ;; \ +.text|.text.*|.data|.data.*|.bss) \ +test $$sz != 0 || continue; \ +echo "Error: size of $<:$$name is 0x$$sz" >&2; \ +exit $$(expr $$idx + 1);; \ +esac; \ +done; \ +$(OBJCOPY) $(foreach s,$(SPECIAL_DATA_SECTIONS),--rename-section .$(s)=.init.$(s)) $< $@ +endef + +$(filter %.init.o,$(obj-y) $(obj-bin-y) $(extra-y)): %.init.o: %.o FORCE + $(call if_changed,obj_init_o) + +quiet_cmd_cpp_i_c = CPP $@ +cmd_cpp_i_c = $(CPP) $(filter-out -Wa$(comma)%,$(c_flags)) $< -o $@ + +quiet_cmd_cc_s_c = CC $@ +cmd_cc_s_c = $(CC) $(filter-out -Wa$(comma)%,$(c_flags)) -S $< -o $@ -$(filter %.init.o,$(obj-y) $(obj-bin-y) $(extra-y)): %.init.o: %.o Makefile - $(OBJDUMP) -h $< | sed -n '/[0-9]/{s,00*,0,g;p;}' | while read idx name sz rest; do \ - case "$$name" in \ - .*.local) ;; \ - .text|.text.*|.data|.data.*|.bss) \ - test $$sz != 0 || continue; \ - echo "Error: size of $<:$$name is 0x$$sz" >&2; \ - exit $$(expr $$idx + 1);; \ - esac; \ - done - $(OBJCOPY) $(foreach s,$(SPECIAL_DATA_SECTIONS),--rename-section .$(s)=.init.$(s)) $< $@ +quiet_cmd_s_S = CPP $@ +cmd_s_S = $(CPP) $(filter-out -Wa$(comma)%,$(a_flags)) $< -o $@ -%.i: %.c Makefile - $(CPP) $(filter-out -Wa$(comma)%,$(c_flags)) $< -o $@ +%.i: %.c FORCE + $(call if_changed,cpp_i_c) -%.s: %.c Makefile - $(CC) $(filter-out -Wa$(comma)%,$(c_flags)) -S $< -o $@ +%.s: %.c FORCE + $(call if_changed,cc_s_c) -%.s: %.S Makefile - $(CPP) $(filter-out -Wa$(comma)%,$(a_flags)) $< -o $@ +%.s: %.S FORCE + $(call if_changed,cpp_s_S) # Add intermediate targets: # When building objects with specific suffix patterns, add intermediate diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile index 9f1ab2335756..b79ad55646bd 100644 --- a/xen/arch/arm/Makefile +++ b/xen/arch/arm/Makefile @@ -97,8 +97,8 @@ prelink_lto.o: $(ALL_OBJS) prelink.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink_lto.o $(LD) $(XEN_LDFLAGS) -r -o $@ $^ else -prelink.o: $(ALL_OBJS) - $(LD) $(XEN_LDFLAGS) -r -o $@ $^ +prelink.o: $(ALL_OBJS) FORCE + $(call if_changed,ld) endif $(TARGET)-syms: prelink.o xen.lds d
[XEN PATCH v4 07/18] build: Introduce documentation for xen Makefiles
This start explainning the variables that can be used in the many Makefiles in xen/. Most of the document copies and modifies text from Linux v5.4 document linux.git/Documentation/kbuild/makefiles.rst. Modification are mostly to avoid mentioning kbuild. Thus I've added the SPDX tag which was only in index.rst in linux.git. Signed-off-by: Anthony PERARD --- Notes: v4: - new patch docs/misc/xen-makefiles/makefiles.rst | 87 +++ xen/Rules.mk | 4 ++ 2 files changed, 91 insertions(+) create mode 100644 docs/misc/xen-makefiles/makefiles.rst diff --git a/docs/misc/xen-makefiles/makefiles.rst b/docs/misc/xen-makefiles/makefiles.rst new file mode 100644 index ..a86e3a612d61 --- /dev/null +++ b/docs/misc/xen-makefiles/makefiles.rst @@ -0,0 +1,87 @@ +.. SPDX-License-Identifier: GPL-2.0 + += +Xen Makefiles += + +Documentation for the build system of Xen, found in xen.git/xen/. + +Makefile files +== + +Description of the syntax that can be used in most Makefiles named +'Makefile'. ('xen/Makefile' isn't part of the description.) + +'Makefile's are consumed by 'Rules.mk' when building. + +Goal definitions + + + Goal definitions are the main part (heart) of the Makefile. + These lines define the files to be built, any special compilation + options, and any subdirectories to be entered recursively. + + The most simple makefile contains one line: + + Example:: + + obj-y += foo.o + + This tells the build system that there is one object in that + directory, named foo.o. foo.o will be built from foo.c or foo.S. + + The following pattern is often used to have object selected + depending on the configuration: + + Example:: + + obj-$(CONFIG_FOO) += foo.o + + $(CONFIG_FOO) can evaluates to y. + If CONFIG_FOO is not y, then the file will not be compiled nor linked. + +Descending down in directories +-- + + A Makefile is only responsible for building objects in its own + directory. Files in subdirectories should be taken care of by + Makefiles in these subdirs. The build system will automatically + invoke make recursively in subdirectories, provided you let it know of + them. + + To do so, obj-y is used. + acpi lives in a separate directory, and the Makefile present in + drivers/ tells the build system to descend down using the following + assignment. + + Example:: + + #drivers/Makefile + obj-$(CONFIG_ACPI) += acpi/ + + If CONFIG_ACPI is set to 'y' + the corresponding obj- variable will be set, and the build system + will descend down in the apci directory. + The build system only uses this information to decide that it needs + to visit the directory, it is the Makefile in the subdirectory that + specifies what is modular and what is built-in. + + It is good practice to use a `CONFIG_` variable when assigning directory + names. This allows the build system to totally skip the directory if the + corresponding `CONFIG_` option is 'y'. + +Compilation flags +- + +CFLAGS-y and AFLAGS-y + These two flags apply only to the makefile in which they + are assigned. They are used for all the normal cc, as and ld + invocations happening during a recursive build. + + $(CFLAGS-y) is necessary because the top Makefile owns the + variable $(XEN_CFLAGS) and uses it for compilation flags for the + entire tree. And the variable $(CFLAGS) is modified by Config.mk + which evaluated in every subdirs. + + CFLAGS-y specifies options for compiling with $(CC). + AFLAGS-y specifies assembler options. diff --git a/xen/Rules.mk b/xen/Rules.mk index 0def40a00a09..7f28c3bc6c13 100644 --- a/xen/Rules.mk +++ b/xen/Rules.mk @@ -1,3 +1,7 @@ +# +# See docs/misc/xen-makefiles/makefiles.rst on variables that can be used in +# Makefile and are consumed by Rules.mk +# -include $(BASEDIR)/include/config/auto.conf -- Anthony PERARD
[XEN PATCH v4 04/18] xen/build: include include/config/auto.conf in main Makefile
We are going to generate the CFLAGS early from "xen/Makefile" instead of in "Rules.mk", but we need to include "config/auto.conf", so include it in "Makefile". Before including "config/auto.conf" we check which make target a user is calling, as some targets don't need "auto.conf". For targets that needs auto.conf, make will generate it (and a default .config if missing). root-make-done is to avoid doing the calculation again once Rules.mk takes over and is been executed with the root Makefile. When Rules.mk is including xen/Makefile, `config-build' and `need-config' are undefined so auto.conf will not be included again (it is already included by Rules.mk) and kconfig target are out of reach of Rules.mk. We are introducing a target %config to catch all targets for kconfig. So we need an extra target %/.config to prevent make from trying to regenerate $(XEN_ROOT)/.config that is included in Config.mk. The way targets are filtered is inspired by Kbuild, with some code imported from Linux. That's why there is PHONY variable that isn't used yet, for example. Signed-off-by: Anthony PERARD --- Notes: v4: - check that root-make-done hasn't been set to an expected value instead of checking if it has been set at all. - Add a shorthand $(kconfig) to run kconfig targets. v3: - filter only for %config instead of both config %config - keep the multi-target pattern rule trick for include/config/auto.conf instead of using Linux's newer pattern (we dont have tristate.conf so don't need to change it) - use y/n for root-make-done, config-build, need-config instead of relying on ifdef and ifndef and on assigning an empty value meaning undef - use space for indentation - explain why %/.config is suddenly needed. xen/Makefile | 98 +- xen/scripts/Kbuild.include | 5 ++ 2 files changed, 80 insertions(+), 23 deletions(-) diff --git a/xen/Makefile b/xen/Makefile index e5f7b1ae13bc..8375070e0d41 100644 --- a/xen/Makefile +++ b/xen/Makefile @@ -49,7 +49,73 @@ default: build .PHONY: dist dist: install -build install:: include/config/auto.conf + +ifneq ($(root-make-done),y) +# section to run before calling Rules.mk, but only once. +# +# To make sure we do not include .config for any of the *config targets +# catch them early, and hand them over to tools/kconfig/Makefile + +clean-targets := %clean +no-dot-config-targets := $(clean-targets) \ + uninstall debug cloc \ + cscope TAGS tags MAP gtags \ + xenversion + +config-build:= n +need-config := y + +ifneq ($(filter $(no-dot-config-targets), $(MAKECMDGOALS)),) +ifeq ($(filter-out $(no-dot-config-targets), $(MAKECMDGOALS)),) +need-config := n +endif +endif + +ifneq ($(filter %config,$(MAKECMDGOALS)),) +config-build := y +endif + +export root-make-done := y +endif # root-make-done + +include scripts/Kbuild.include + +ifeq ($(config-build),y) +# === +# *config targets only - make sure prerequisites are updated, and descend +# in tools/kconfig to make the *config target + +config: FORCE + $(MAKE) $(kconfig) $@ + +# Config.mk tries to include .config file, don't try to remake it +%/.config: ; + +%config: FORCE + $(MAKE) $(kconfig) $@ + +else # !config-build + +ifeq ($(need-config),y) +include include/config/auto.conf +# Read in dependencies to all Kconfig* files, make sure to run syncconfig if +# changes are detected. +include include/config/auto.conf.cmd + +# Allow people to just run `make` as before and not force them to configure +$(KCONFIG_CONFIG): + $(MAKE) $(kconfig) defconfig + +# The actual configuration files used during the build are stored in +# include/generated/ and include/config/. Update them if .config is newer than +# include/config/auto.conf (which mirrors .config). +# +# This exploits the 'multi-target pattern rule' trick. +# The syncconfig should be executed only once to make all the targets. +include/config/%.conf include/config/%.conf.cmd: $(KCONFIG_CONFIG) + $(MAKE) $(kconfig) syncconfig + +endif # need-config .PHONY: build install uninstall clean distclean MAP build install uninstall debug clean distclean MAP:: @@ -254,9 +320,6 @@ cscope: _MAP: $(NM) -n $(TARGET)-syms | grep -v '\(compiled\)\|\(\.o$$\)\|\( [aUw] \)\|\(\.\.ng$$\)\|\(LASH[RL]DI\)' > System.map -.PHONY: FORCE -FORCE: - %.o %.i %.s: %.c FORCE $(MAKE) -f $(BASEDIR)/Rules.mk -C $(*D) $(@F) @@ -277,25 +340,6 @@ $(foreach base,arch/x86/mm/guest_walk_% \ arch/x86/mm/shadow/guest_%, \ $(foreach ext,o i s,$(call build-intermediate,$(base).$(ext -kconfig := oldconfig config menuconfig defconfig allyesconfig allnoconfig \ - nconfig xconfig gconfig savedefconfig listnewconfig olddefconfig \ - randconfig $(notdir $(wildcard a
[XEN PATCH v4 08/18] xen/build: introduce if_changed and if_changed_rule
The if_changed macro from Linux, in addition to check if any files needs an update, check if the command line has changed since the last invocation. The latter will force a rebuild if any options to the executable have changed. if_changed_rule checks dependencies like if_changed, but execute rule_$(1) instead of cmd_$(1) when a target needs to be rebuilt. A rule_ macro can call more than one cmd_ macro. One of the cmd_ macro in a rule need to be call using a macro that record the command line, so cmd_and_record is introduced. It is similar to cmd_and_fixup from Linux but without a call to fixdep which we don't have yet. (We will later replace cmd_and_record by cmd_and_fixup.) Example of a rule_ macro: define rule_cc_o_c $(call cmd_and_record,cc_o_o) $(call cmd,objcopy) endef This needs one of the call to use cmd_and_record, otherwise no .*.cmd file will be created, and the target will keep been rebuilt. In order for if_changed to works correctly, we need to load the .%.cmd files that the macro generates, this is done by adding targets in to the $(targets) variable. We use intermediate_targets to add %.init.o dependency %.o to target since there aren't in obj-y. We also add $(MAKECMDGOALS) to targets so that when running for example `make common/memory.i`, make will load the associated .%.cmd dependency file. Beside the if_changed*, we import the machinery used for a "beautify output". The important one is when running make with V=2 which help to debug the makefiles by printing why a target is been rebuilt, via the $(echo-why) macro. if_changed and if_changed_rule aren't used yet. Most of this code is copied from Linux v5.4, including the documentation. Signed-off-by: Anthony PERARD --- Notes: v4: - Use := in make whenever possible (instead of =) - insert new string in .gitignore somewhere more plausible. - import documentation from Linux .gitignore| 1 + docs/misc/xen-makefiles/makefiles.rst | 99 xen/Makefile | 53 - xen/Rules.mk | 33 +++- xen/scripts/Kbuild.include| 107 ++ 5 files changed, 291 insertions(+), 2 deletions(-) diff --git a/.gitignore b/.gitignore index 4ca679ddbc9a..bfa53723b38b 100644 --- a/.gitignore +++ b/.gitignore @@ -1,4 +1,5 @@ .hg +.*.cmd .*.tmp *.orig *~ diff --git a/docs/misc/xen-makefiles/makefiles.rst b/docs/misc/xen-makefiles/makefiles.rst index a86e3a612d61..9efd8464a763 100644 --- a/docs/misc/xen-makefiles/makefiles.rst +++ b/docs/misc/xen-makefiles/makefiles.rst @@ -85,3 +85,102 @@ Compilation flags CFLAGS-y specifies options for compiling with $(CC). AFLAGS-y specifies assembler options. + + +Build system infrastructure +=== + +This chapter describe some of the macro used when building Xen. + +Macros +-- + + +if_changed + if_changed is the infrastructure used for the following commands. + + Usage:: + + target: source(s) FORCE + $(call if_changed,ld/objcopy/...) + + When the rule is evaluated, it is checked to see if any files + need an update, or the command line has changed since the last + invocation. The latter will force a rebuild if any options + to the executable have changed. + Any target that utilises if_changed must be listed in $(targets), + otherwise the command line check will fail, and the target will + always be built. + if_changed may be used in conjunction with custom commands as + defined in "Custom commands". + + Note: It is a typical mistake to forget the FORCE prerequisite. + Another common pitfall is that whitespace is sometimes + significant; for instance, the below will fail (note the extra space + after the comma):: + + target: source(s) FORCE + + **WRONG!** $(call if_changed, ld/objcopy/...) + +Note: + if_changed should not be used more than once per target. + It stores the executed command in a corresponding .cmd + +file and multiple calls would result in overwrites and +unwanted results when the target is up to date and only the +tests on changed commands trigger execution of commands. + +ld + Link target. + + Example:: + + targets += setup setup.o bootsect bootsect.o + $(obj)/setup $(obj)/bootsect: %: %.o FORCE + $(call if_changed,ld) + + $(targets) are assigned all potential targets, by which the build + system knows the targets and will: + + 1) check for commandline changes + + The ": %: %.o" part of the prerequisite is a shorthand that + frees us from listing the setup.o and bootsect.o files. + + Note: + It is a common mistake to forget the "targets :=" assignment, +
[PATCH 11/11] x86/ucode/amd: Rework parsing logic in cpu_request_microcode()
cpu_request_microcode() is still a confusing mess to follow, with sub functions responsible for maintaining offset. Rewrite it so all container structure handling is in this one function. Rewrite struct mpbhdr as struct container_equiv_table to aid parsing. Drop container_fast_forward() entirely, and shrink scan_equiv_cpu_table() to just its searching/caching logic. container_fast_forward() gets logically folded into the microcode blob scanning loop, except that a skip path is inserted, which is conditional on whether scan_equiv_cpu_table() thinks there is appropriate microcode to find. With this change, we now scan to the end of all provided microcode containers, and no longer give up at the first applicable one. Signed-off-by: Andrew Cooper --- CC: Jan Beulich CC: Wei Liu CC: Roger Pau Monné --- xen/arch/x86/cpu/microcode/amd.c | 169 ++- 1 file changed, 44 insertions(+), 125 deletions(-) diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c index f9c50b43bf..0ada50797b 100644 --- a/xen/arch/x86/cpu/microcode/amd.c +++ b/xen/arch/x86/cpu/microcode/amd.c @@ -25,10 +25,6 @@ #define pr_debug(x...) ((void)0) -#define CONT_HDR_SIZE 12 -#define SECTION_HDR_SIZE8 -#define PATCH_HDR_SIZE 32 - struct __packed equiv_cpu_entry { uint32_t installed_cpu; uint32_t fixed_errata_mask; @@ -58,10 +54,10 @@ struct microcode_patch { #define UCODE_EQUIV_CPU_TABLE_TYPE 0x #define UCODE_UCODE_TYPE 0x0001 -struct mpbhdr { -uint32_t type; +struct container_equiv_table { +uint32_t type; /* UCODE_EQUIV_CPU_TABLE_TYPE */ uint32_t len; -uint8_t data[]; +struct equiv_cpu_entry eq[]; }; struct container_microcode { uint32_t type; /* UCODE_UCODE_TYPE */ @@ -269,55 +265,25 @@ static int apply_microcode(const struct microcode_patch *patch) return 0; } -static int scan_equiv_cpu_table( -const void *data, -size_t size_left, -size_t *offset) +static int scan_equiv_cpu_table(const struct container_equiv_table *et) { const struct cpu_signature *sig = &this_cpu(cpu_sig); -const struct mpbhdr *mpbuf; -const struct equiv_cpu_entry *eq; -unsigned int i, nr; - -if ( size_left < (sizeof(*mpbuf) + 4) || - (mpbuf = data + *offset + 4, - size_left - sizeof(*mpbuf) - 4 < mpbuf->len) ) -{ -printk(XENLOG_WARNING "microcode: No space for equivalent cpu table\n"); -return -EINVAL; -} - -*offset += mpbuf->len + CONT_HDR_SIZE; /* add header length */ - -if ( mpbuf->type != UCODE_EQUIV_CPU_TABLE_TYPE ) -{ -printk(KERN_ERR "microcode: Wrong microcode equivalent cpu table type field\n"); -return -EINVAL; -} - -if ( mpbuf->len == 0 || mpbuf->len % sizeof(*eq) || - (eq = (const void *)mpbuf->data, - nr = mpbuf->len / sizeof(*eq), - eq[nr - 1].installed_cpu) ) -{ -printk(KERN_ERR "microcode: Wrong microcode equivalent cpu table length\n"); -return -EINVAL; -} +unsigned int i, nr = et->len / sizeof(et->eq[0]); /* Search the equiv_cpu_table for the current CPU. */ -for ( i = 0; i < nr && eq[i].installed_cpu; ++i ) +for ( i = 0; i < nr && et->eq[i].installed_cpu; ++i ) { -if ( eq[i].installed_cpu != sig->sig ) +if ( et->eq[i].installed_cpu != sig->sig ) continue; if ( !equiv.sig ) /* Cache details on first find. */ { equiv.sig = sig->sig; -equiv.id = eq[i].equiv_cpu; +equiv.id = et->eq[i].equiv_cpu; return 0; } -if ( equiv.sig != sig->sig || equiv.id != eq[i].equiv_cpu ) +if ( equiv.sig != sig->sig || equiv.id != et->eq[i].equiv_cpu ) { /* * This can only occur if two equiv tables have been seen with @@ -327,7 +293,7 @@ static int scan_equiv_cpu_table( */ printk(XENLOG_ERR "microcode: Equiv mismatch: cpu %08x, got %04x, cached %04x\n", - sig->sig, eq[i].equiv_cpu, equiv.id); + sig->sig, et->eq[i].equiv_cpu, equiv.id); return -EINVAL; } @@ -338,101 +304,51 @@ static int scan_equiv_cpu_table( return -ESRCH; } -static int container_fast_forward(const void *data, size_t size_left, size_t *offset) -{ -for ( ; ; ) -{ -size_t size; -const uint32_t *header; - -if ( size_left < SECTION_HDR_SIZE ) -return -EINVAL; - -header = data + *offset; - -if ( header[0] == UCODE_MAGIC && - header[1] == UCODE_EQUIV_CPU_TABLE_TYPE ) -break; - -if ( header[0] != UCODE_UCODE_TYPE ) -return -EINVAL; -size = header[1] + SECTION_HDR_SIZE; -if ( size < PATCH_HDR_SIZE || size_left < size ) -return -EINVAL; - -size_lef
[PATCH 10/11] x86/ucode/amd: Fold structures together
With all the necessary cleanup now in place, fold struct microcode_header_amd into struct microcode_patch and drop the struct microcode_amd temporary ifdef-ary. This removes the memory allocation of struct microcode_amd which is a single pointer to a separately allocated object, and therefore a waste. Signed-off-by: Andrew Cooper --- CC: Jan Beulich CC: Wei Liu CC: Roger Pau Monné --- xen/arch/x86/cpu/microcode/amd.c | 70 1 file changed, 20 insertions(+), 50 deletions(-) diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c index ae1276988f..f9c50b43bf 100644 --- a/xen/arch/x86/cpu/microcode/amd.c +++ b/xen/arch/x86/cpu/microcode/amd.c @@ -37,7 +37,7 @@ struct __packed equiv_cpu_entry { uint16_t reserved; }; -struct __packed microcode_header_amd { +struct microcode_patch { uint32_t data_code; uint32_t patch_id; uint8_t mc_patch_data_id[2]; @@ -58,13 +58,6 @@ struct __packed microcode_header_amd { #define UCODE_EQUIV_CPU_TABLE_TYPE 0x #define UCODE_UCODE_TYPE 0x0001 -struct microcode_patch { -struct microcode_header_amd *mpb; -}; - -/* Temporary, until the microcode_* structure are disentangled. */ -#define microcode_amd microcode_patch - struct mpbhdr { uint32_t type; uint32_t len; @@ -73,7 +66,7 @@ struct mpbhdr { struct container_microcode { uint32_t type; /* UCODE_UCODE_TYPE */ uint32_t len; -struct microcode_header_amd patch[]; +struct microcode_patch patch[]; }; /* @@ -178,7 +171,7 @@ static bool check_final_patch_levels(const struct cpu_signature *sig) } static enum microcode_match_result microcode_fits( -const struct microcode_header_amd *patch) +const struct microcode_patch *patch) { unsigned int cpu = smp_processor_id(); const struct cpu_signature *sig = &per_cpu(cpu_sig, cpu); @@ -201,37 +194,31 @@ static enum microcode_match_result microcode_fits( static bool match_cpu(const struct microcode_patch *patch) { -return patch && (microcode_fits(patch->mpb) == NEW_UCODE); +return patch && (microcode_fits(patch) == NEW_UCODE); } -static void free_patch(struct microcode_patch *mc_amd) +static void free_patch(struct microcode_patch *patch) { -if ( mc_amd ) -{ -xfree(mc_amd->mpb); -xfree(mc_amd); -} +xfree(patch); } static enum microcode_match_result compare_header( -const struct microcode_header_amd *new_header, -const struct microcode_header_amd *old_header) +const struct microcode_patch *new, const struct microcode_patch *old) { -if ( new_header->processor_rev_id == old_header->processor_rev_id ) -return (new_header->patch_id > old_header->patch_id) ? NEW_UCODE - : OLD_UCODE; +if ( new->processor_rev_id != old->processor_rev_id ) +return MIS_UCODE; -return MIS_UCODE; +return new->patch_id > old->patch_id ? NEW_UCODE : OLD_UCODE; } static enum microcode_match_result compare_patch( const struct microcode_patch *new, const struct microcode_patch *old) { /* Both patches to compare are supposed to be applicable to local CPU. */ -ASSERT(microcode_fits(new->mpb) != MIS_UCODE); -ASSERT(microcode_fits(old->mpb) != MIS_UCODE); +ASSERT(microcode_fits(new) != MIS_UCODE); +ASSERT(microcode_fits(old) != MIS_UCODE); -return compare_header(new->mpb, old->mpb); +return compare_header(new, old); } static int apply_microcode(const struct microcode_patch *patch) @@ -239,7 +226,6 @@ static int apply_microcode(const struct microcode_patch *patch) int hw_err; unsigned int cpu = smp_processor_id(); struct cpu_signature *sig = &per_cpu(cpu_sig, cpu); -const struct microcode_header_amd *hdr; uint32_t rev, old_rev = sig->rev; if ( !patch ) @@ -256,9 +242,7 @@ static int apply_microcode(const struct microcode_patch *patch) return -ENXIO; } -hdr = patch->mpb; - -hw_err = wrmsr_safe(MSR_AMD_PATCHLOADER, (unsigned long)hdr); +hw_err = wrmsr_safe(MSR_AMD_PATCHLOADER, (unsigned long)patch); /* get patch id after patching */ rdmsrl(MSR_AMD_PATCHLEVEL, rev); @@ -268,14 +252,14 @@ static int apply_microcode(const struct microcode_patch *patch) * Some processors leave the ucode blob mapping as UC after the update. * Flush the mapping to regain normal cacheability. */ -flush_area_local(hdr, FLUSH_TLB_GLOBAL | FLUSH_ORDER(0)); +flush_area_local(patch, FLUSH_TLB_GLOBAL | FLUSH_ORDER(0)); /* check current patch id and patch's id for match */ -if ( hw_err || (rev != hdr->patch_id) ) +if ( hw_err || (rev != patch->patch_id) ) { printk(XENLOG_ERR "microcode: CPU%u update rev %#x to %#x failed, result %#x\n", - cpu, old_rev, hdr->patch_id, rev); + cpu, old_rev, patch->patch_id, rev);
Re: [PATCH v13 1/3] xen/mem_sharing: VM forking
Hi, On 30/03/2020 16:02, Tamas K Lengyel wrote: VM forking is the process of creating a domain with an empty memory space and a parent domain specified from which to populate the memory when necessary. For the new domain to be functional the VM state is copied over as part of the fork operation (HVM params, hap allocation, etc). Signed-off-by: Tamas K Lengyel Acked-by: Jan Beulich --- v13: Address issues pointed out by Roger & Jan Introduce & use PAGE_OFFSET to calculate vcpu_info offset --- xen/arch/x86/domain.c | 13 ++ xen/arch/x86/hvm/hvm.c| 4 +- xen/arch/x86/mm/hap/hap.c | 3 +- xen/arch/x86/mm/mem_sharing.c | 351 ++ xen/arch/x86/mm/p2m.c | 9 +- xen/include/asm-arm/page.h| 1 + For the Arm change: Acked-by: Julien Grall Cheers, -- Julien Grall
[PATCH 04/11] x86/ucode/amd: Collect CPUID.1.EAX in collect_cpu_info()
... rather than collecting it repeatedly in microcode_fits(). This brings the behaviour in line with the Intel side. Signed-off-by: Andrew Cooper --- CC: Jan Beulich CC: Wei Liu CC: Roger Pau Monné --- xen/arch/x86/cpu/microcode/amd.c | 11 +++ xen/include/asm-x86/microcode.h | 2 +- 2 files changed, 4 insertions(+), 9 deletions(-) diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c index 3f3a05fad2..d2ecc7ae87 100644 --- a/xen/arch/x86/cpu/microcode/amd.c +++ b/xen/arch/x86/cpu/microcode/amd.c @@ -79,6 +79,7 @@ static int collect_cpu_info(struct cpu_signature *csig) { memset(csig, 0, sizeof(*csig)); +csig->sig = cpuid_eax(1); rdmsrl(MSR_AMD_PATCHLEVEL, csig->rev); pr_debug("microcode: CPU%d collect_cpu_info: patch_id=%#x\n", @@ -177,12 +178,9 @@ static enum microcode_match_result microcode_fits( const struct cpu_signature *sig = &per_cpu(cpu_sig, cpu); const struct microcode_header_amd *mc_header = mc_amd->mpb; const struct equiv_cpu_entry *equiv_cpu_table = mc_amd->equiv_cpu_table; -unsigned int current_cpu_id; unsigned int equiv_cpu_id; -current_cpu_id = cpuid_eax(0x0001); - -if ( !find_equiv_cpu_id(equiv_cpu_table, current_cpu_id, &equiv_cpu_id) ) +if ( !find_equiv_cpu_id(equiv_cpu_table, sig->sig, &equiv_cpu_id) ) return MIS_UCODE; if ( (mc_header->processor_rev_id) != equiv_cpu_id ) @@ -419,13 +417,10 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, struct microcode_patch *patch = NULL; size_t offset = 0, saved_size = 0; int error = 0; -unsigned int current_cpu_id; unsigned int equiv_cpu_id; unsigned int cpu = smp_processor_id(); const struct cpu_signature *sig = &per_cpu(cpu_sig, cpu); -current_cpu_id = cpuid_eax(0x0001); - if ( bufsize < 4 || *(const uint32_t *)buf != UCODE_MAGIC ) { @@ -456,7 +451,7 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, break; } -if ( find_equiv_cpu_id(mc_amd->equiv_cpu_table, current_cpu_id, +if ( find_equiv_cpu_id(mc_amd->equiv_cpu_table, sig->sig, &equiv_cpu_id) ) break; diff --git a/xen/include/asm-x86/microcode.h b/xen/include/asm-x86/microcode.h index 3a8e4e8221..cbbe28cb45 100644 --- a/xen/include/asm-x86/microcode.h +++ b/xen/include/asm-x86/microcode.h @@ -7,7 +7,7 @@ #include struct cpu_signature { -/* CPU signature (CPUID.1.EAX). Only written on Intel. */ +/* CPU signature (CPUID.1.EAX). */ unsigned int sig; /* Platform Flags. Only applicable to Intel. */ -- 2.11.0
[PATCH 09/11] x86/ucode/amd: Remove gratuitous memory allocations from cpu_request_microcode()
Just as on the Intel side, there is no point having get_ucode_from_buffer_amd() make $N memory allocations and free $N-1 of them. Delete get_ucode_from_buffer_amd() and rewrite the loop in cpu_request_microcode() to have 'saved' point into 'buf' until we finally decide to duplicate that blob and return it to our caller. Introduce a new struct container_microcode to simplify interpreting the container format. Doubly indent the logic to substantially reduce the churn in a later change. Signed-off-by: Andrew Cooper --- CC: Jan Beulich CC: Wei Liu CC: Roger Pau Monné --- xen/arch/x86/cpu/microcode/amd.c | 138 +-- 1 file changed, 47 insertions(+), 91 deletions(-) diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c index 980e61c547..ae1276988f 100644 --- a/xen/arch/x86/cpu/microcode/amd.c +++ b/xen/arch/x86/cpu/microcode/amd.c @@ -70,6 +70,11 @@ struct mpbhdr { uint32_t len; uint8_t data[]; }; +struct container_microcode { +uint32_t type; /* UCODE_UCODE_TYPE */ +uint32_t len; +struct microcode_header_amd patch[]; +}; /* * Microcode updates for different CPUs are distinguished by their @@ -280,52 +285,6 @@ static int apply_microcode(const struct microcode_patch *patch) return 0; } -static int get_ucode_from_buffer_amd( -struct microcode_amd *mc_amd, -const void *buf, -size_t bufsize, -size_t *offset) -{ -const struct mpbhdr *mpbuf = buf + *offset; - -/* No more data */ -if ( *offset >= bufsize ) -{ -printk(KERN_ERR "microcode: Microcode buffer overrun\n"); -return -EINVAL; -} - -if ( mpbuf->type != UCODE_UCODE_TYPE ) -{ -printk(KERN_ERR "microcode: Wrong microcode payload type field\n"); -return -EINVAL; -} - -if ( (*offset + mpbuf->len) > bufsize ) -{ -printk(KERN_ERR "microcode: Bad data in microcode data file\n"); -return -EINVAL; -} - -if ( !verify_patch_size(mpbuf->len) ) -{ -printk(XENLOG_ERR "microcode: patch size mismatch\n"); -return -EINVAL; -} - -mc_amd->mpb = xmemdup_bytes(mpbuf->data, mpbuf->len); -if ( !mc_amd->mpb ) -return -ENOMEM; - -pr_debug("microcode: CPU%d size %zu, block size %u offset %zu equivID %#x rev %#x\n", - smp_processor_id(), bufsize, mpbuf->len, *offset, - mc_amd->mpb->processor_rev_id, mc_amd->mpb->patch_id); - -*offset += mpbuf->len + SECTION_HDR_SIZE; - -return 0; -} - static int scan_equiv_cpu_table( const void *data, size_t size_left, @@ -430,9 +389,9 @@ static int container_fast_forward(const void *data, size_t size_left, size_t *of static struct microcode_patch *cpu_request_microcode(const void *buf, size_t size) { struct microcode_amd *mc_amd; -struct microcode_header_amd *saved = NULL; +const struct microcode_header_amd *saved = NULL; struct microcode_patch *patch = NULL; -size_t offset = 0; +size_t offset = 0, saved_size = 0; int error = 0; unsigned int cpu = smp_processor_id(); const struct cpu_signature *sig = &per_cpu(cpu_sig, cpu); @@ -497,57 +456,54 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, size_t siz * It's possible the data file has multiple matching ucode, * lets keep searching till the latest version */ -while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, size, - &offset)) == 0 ) +buf += offset; +size -= offset; { -/* - * If the new ucode covers current CPU, compare ucodes and store the - * one with higher revision. - */ -if ( (microcode_fits(mc_amd->mpb) != MIS_UCODE) && - (!saved || (compare_header(mc_amd->mpb, saved) == NEW_UCODE)) ) +while ( size ) { -xfree(saved); -saved = mc_amd->mpb; -} -else -{ -xfree(mc_amd->mpb); -mc_amd->mpb = NULL; -} +const struct container_microcode *mc; + +if ( size < sizeof(*mc) || + (mc = buf)->type != UCODE_UCODE_TYPE || + size - sizeof(*mc) < mc->len || + !verify_patch_size(mc->len) ) +{ +printk(XENLOG_ERR "microcode: Bad microcode data\n"); +error = -EINVAL; +break; +} -if ( offset >= size ) -break; +/* + * If the new ucode covers current CPU, compare ucodes and store the + * one with higher revision. + */ +if ( (microcode_fits(mc->patch) != MIS_UCODE) && + (!saved || (compare_header(mc->patch, saved) == NEW_UCODE)) ) +{ +saved = mc->patch; +saved_size = mc->len; +} -/* - * 1. Given a situation where mu
[PATCH 06/11] x86/ucode/amd: Move verify_patch_size() into get_ucode_from_buffer_amd()
We only stash the microcode blob size so it can be audited in microcode_fits(). However, the patch size check depends only on the CPU family. Move the check earlier to when we are parsing the container, which avoids caching bad microcode in the first place, and allows us to avoid storing the size at all. Signed-off-by: Andrew Cooper --- CC: Jan Beulich CC: Wei Liu CC: Roger Pau Monné --- xen/arch/x86/cpu/microcode/amd.c | 18 +++--- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c index d3439b0c6c..8318664f85 100644 --- a/xen/arch/x86/cpu/microcode/amd.c +++ b/xen/arch/x86/cpu/microcode/amd.c @@ -60,7 +60,6 @@ struct __packed microcode_header_amd { struct microcode_patch { struct microcode_header_amd *mpb; -size_t mpb_size; }; /* Temporary, until the microcode_* structure are disentangled. */ @@ -184,12 +183,6 @@ static enum microcode_match_result microcode_fits( equiv.id != mc_header->processor_rev_id ) return MIS_UCODE; -if ( !verify_patch_size(mc_amd->mpb_size) ) -{ -pr_debug("microcode: patch size mismatch\n"); -return MIS_UCODE; -} - if ( mc_header->patch_id <= sig->rev ) { pr_debug("microcode: patch is already at required level or greater.\n"); @@ -318,10 +311,15 @@ static int get_ucode_from_buffer_amd( return -EINVAL; } +if ( !verify_patch_size(mpbuf->len) ) +{ +printk(XENLOG_ERR "microcode: patch size mismatch\n"); +return -EINVAL; +} + mc_amd->mpb = xmemdup_bytes(mpbuf->data, mpbuf->len); if ( !mc_amd->mpb ) return -ENOMEM; -mc_amd->mpb_size = mpbuf->len; pr_debug("microcode: CPU%d size %zu, block size %u offset %zu equivID %#x rev %#x\n", smp_processor_id(), bufsize, mpbuf->len, *offset, @@ -439,7 +437,7 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, struct microcode_amd *mc_amd; struct microcode_header_amd *saved = NULL; struct microcode_patch *patch = NULL; -size_t offset = 0, saved_size = 0; +size_t offset = 0; int error = 0; unsigned int cpu = smp_processor_id(); const struct cpu_signature *sig = &per_cpu(cpu_sig, cpu); @@ -516,7 +514,6 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, { xfree(saved); saved = mc_amd->mpb; -saved_size = mc_amd->mpb_size; } else { @@ -555,7 +552,6 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, if ( saved ) { mc_amd->mpb = saved; -mc_amd->mpb_size = saved_size; patch = mc_amd; } else -- 2.11.0
[PATCH 01/11] x86/ucode/amd: Fix more potential buffer overruns with microcode parsing
cpu_request_microcode() doesn't know the buffer is at least 4 bytes long before inspecting UCODE_MAGIC. install_equiv_cpu_table() doesn't know the boundary of the buffer it is interpreting as an equivalency table. This case was clearly observed at one point in the past, given the subsequent overrun detection, but without comprehending that the damage was already done. Make the logic consistent with container_fast_forward() and pass size_left in to install_equiv_cpu_table(). Signed-off-by: Andrew Cooper Reviewed-by: Jan Beulich --- CC: Jan Beulich CC: Wei Liu CC: Roger Pau Monné --- xen/arch/x86/cpu/microcode/amd.c | 27 +-- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c index 6bf3a054d3..796745e928 100644 --- a/xen/arch/x86/cpu/microcode/amd.c +++ b/xen/arch/x86/cpu/microcode/amd.c @@ -303,11 +303,20 @@ static int get_ucode_from_buffer_amd( static int install_equiv_cpu_table( struct microcode_amd *mc_amd, const void *data, +size_t size_left, size_t *offset) { -const struct mpbhdr *mpbuf = data + *offset + 4; +const struct mpbhdr *mpbuf; const struct equiv_cpu_entry *eq; +if ( size_left < (sizeof(*mpbuf) + 4) || + (mpbuf = data + *offset + 4, + size_left - sizeof(*mpbuf) - 4 < mpbuf->len) ) +{ +printk(XENLOG_WARNING "microcode: No space for equivalent cpu table\n"); +return -EINVAL; +} + *offset += mpbuf->len + CONT_HDR_SIZE; /* add header length */ if ( mpbuf->type != UCODE_EQUIV_CPU_TABLE_TYPE ) @@ -417,7 +426,8 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, current_cpu_id = cpuid_eax(0x0001); -if ( *(const uint32_t *)buf != UCODE_MAGIC ) +if ( bufsize < 4 || + *(const uint32_t *)buf != UCODE_MAGIC ) { printk(KERN_ERR "microcode: Wrong microcode patch file magic\n"); error = -EINVAL; @@ -447,24 +457,13 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, */ while ( offset < bufsize ) { -error = install_equiv_cpu_table(mc_amd, buf, &offset); +error = install_equiv_cpu_table(mc_amd, buf, bufsize - offset, &offset); if ( error ) { printk(KERN_ERR "microcode: installing equivalent cpu table failed\n"); break; } -/* - * Could happen as we advance 'offset' early - * in install_equiv_cpu_table - */ -if ( offset > bufsize ) -{ -printk(KERN_ERR "microcode: Microcode buffer overrun\n"); -error = -EINVAL; -break; -} - if ( find_equiv_cpu_id(mc_amd->equiv_cpu_table, current_cpu_id, &equiv_cpu_id) ) break; -- 2.11.0
[PATCH 07/11] x86/ucode/amd: Alter API for microcode_fits()
Although it is logically a step in the wrong direction overall, it simplifies the rearranging of cpu_request_microcode() substantially for microcode_fits() to take struct microcode_header_amd directly, and not require an intermediate struct microcode_amd pointing at it. Make this change (taking time to rename 'mc_amd' to its eventual 'patch' to reduce the churn in the series), and a later cleanup will make it uniformly take a struct microcode_patch. No functional change. Signed-off-by: Andrew Cooper --- CC: Jan Beulich CC: Wei Liu CC: Roger Pau Monné --- xen/arch/x86/cpu/microcode/amd.c | 22 +- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c index 8318664f85..254f3dd4d7 100644 --- a/xen/arch/x86/cpu/microcode/amd.c +++ b/xen/arch/x86/cpu/microcode/amd.c @@ -173,31 +173,30 @@ static bool check_final_patch_levels(const struct cpu_signature *sig) } static enum microcode_match_result microcode_fits( -const struct microcode_amd *mc_amd) +const struct microcode_header_amd *patch) { unsigned int cpu = smp_processor_id(); const struct cpu_signature *sig = &per_cpu(cpu_sig, cpu); -const struct microcode_header_amd *mc_header = mc_amd->mpb; if ( equiv.sig != sig->sig || - equiv.id != mc_header->processor_rev_id ) + equiv.id != patch->processor_rev_id ) return MIS_UCODE; -if ( mc_header->patch_id <= sig->rev ) +if ( patch->patch_id <= sig->rev ) { pr_debug("microcode: patch is already at required level or greater.\n"); return OLD_UCODE; } pr_debug("microcode: CPU%d found a matching microcode update with version %#x (current=%#x)\n", - cpu, mc_header->patch_id, sig->rev); + cpu, patch->patch_id, sig->rev); return NEW_UCODE; } static bool match_cpu(const struct microcode_patch *patch) { -return patch && (microcode_fits(patch) == NEW_UCODE); +return patch && (microcode_fits(patch->mpb) == NEW_UCODE); } static void free_patch(struct microcode_patch *mc_amd) @@ -223,14 +222,11 @@ static enum microcode_match_result compare_header( static enum microcode_match_result compare_patch( const struct microcode_patch *new, const struct microcode_patch *old) { -const struct microcode_header_amd *new_header = new->mpb; -const struct microcode_header_amd *old_header = old->mpb; - /* Both patches to compare are supposed to be applicable to local CPU. */ -ASSERT(microcode_fits(new) != MIS_UCODE); -ASSERT(microcode_fits(old) != MIS_UCODE); +ASSERT(microcode_fits(new->mpb) != MIS_UCODE); +ASSERT(microcode_fits(old->mpb) != MIS_UCODE); -return compare_header(new_header, old_header); +return compare_header(new->mpb, old->mpb); } static int apply_microcode(const struct microcode_patch *patch) @@ -509,7 +505,7 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, * If the new ucode covers current CPU, compare ucodes and store the * one with higher revision. */ -if ( (microcode_fits(mc_amd) != MIS_UCODE) && +if ( (microcode_fits(mc_amd->mpb) != MIS_UCODE) && (!saved || (compare_header(mc_amd->mpb, saved) == NEW_UCODE)) ) { xfree(saved); -- 2.11.0
[PATCH 03/11] x86/ucode/amd: Don't use void * for microcode_patch->mpb
All code works fine with it having its correct type, and it even allows us to drop two casts in a printk(). No functional change. Signed-off-by: Andrew Cooper --- CC: Jan Beulich CC: Wei Liu CC: Roger Pau Monné --- xen/arch/x86/cpu/microcode/amd.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c index 4245dc13bb..3f3a05fad2 100644 --- a/xen/arch/x86/cpu/microcode/amd.c +++ b/xen/arch/x86/cpu/microcode/amd.c @@ -59,7 +59,7 @@ struct __packed microcode_header_amd { #define UCODE_UCODE_TYPE 0x0001 struct microcode_patch { -void *mpb; +struct microcode_header_amd *mpb; size_t mpb_size; struct equiv_cpu_entry *equiv_cpu_table; size_t equiv_cpu_table_size; @@ -330,8 +330,7 @@ static int get_ucode_from_buffer_amd( pr_debug("microcode: CPU%d size %zu, block size %u offset %zu equivID %#x rev %#x\n", smp_processor_id(), bufsize, mpbuf->len, *offset, - ((struct microcode_header_amd *)mc_amd->mpb)->processor_rev_id, - ((struct microcode_header_amd *)mc_amd->mpb)->patch_id); + mc_amd->mpb->processor_rev_id, mc_amd->mpb->patch_id); *offset += mpbuf->len + SECTION_HDR_SIZE; -- 2.11.0
[PATCH 08/11] x86/ucode/amd: Rename bufsize to size in cpu_request_microcode()
To simplify future cleanup, rename this variable. No functional change. Signed-off-by: Andrew Cooper --- CC: Jan Beulich CC: Wei Liu CC: Roger Pau Monné --- xen/arch/x86/cpu/microcode/amd.c | 21 ++--- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c index 254f3dd4d7..980e61c547 100644 --- a/xen/arch/x86/cpu/microcode/amd.c +++ b/xen/arch/x86/cpu/microcode/amd.c @@ -427,8 +427,7 @@ static int container_fast_forward(const void *data, size_t size_left, size_t *of return 0; } -static struct microcode_patch *cpu_request_microcode(const void *buf, - size_t bufsize) +static struct microcode_patch *cpu_request_microcode(const void *buf, size_t size) { struct microcode_amd *mc_amd; struct microcode_header_amd *saved = NULL; @@ -438,7 +437,7 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, unsigned int cpu = smp_processor_id(); const struct cpu_signature *sig = &per_cpu(cpu_sig, cpu); -if ( bufsize < 4 || +if ( size < 4 || *(const uint32_t *)buf != UCODE_MAGIC ) { printk(KERN_ERR "microcode: Wrong microcode patch file magic\n"); @@ -459,17 +458,17 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, * 1. check if this container file has equiv_cpu_id match * 2. If not, fast-fwd to next container file */ -while ( offset < bufsize ) +while ( offset < size ) { -error = scan_equiv_cpu_table(buf, bufsize - offset, &offset); +error = scan_equiv_cpu_table(buf, size - offset, &offset); if ( !error || error != -ESRCH ) break; -error = container_fast_forward(buf, bufsize - offset, &offset); +error = container_fast_forward(buf, size - offset, &offset); if ( error == -ENODATA ) { -ASSERT(offset == bufsize); +ASSERT(offset == size); break; } if ( error ) @@ -498,7 +497,7 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, * It's possible the data file has multiple matching ucode, * lets keep searching till the latest version */ -while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, bufsize, +while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, size, &offset)) == 0 ) { /* @@ -517,7 +516,7 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, mc_amd->mpb = NULL; } -if ( offset >= bufsize ) +if ( offset >= size ) break; /* @@ -527,7 +526,7 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, *earlier while() (On this case, matches on earlier container *file and we break) * 3. Proceed to while ( (error = get_ucode_from_buffer_amd(mc_amd, - * buf, bufsize,&offset)) == 0 ) + * buf, size, &offset)) == 0 ) * 4. Find correct patch using microcode_fits() and apply the patch *(Assume: apply_microcode() is successful) * 5. The while() loop from (3) continues to parse the binary as @@ -540,7 +539,7 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, *before if ( mpbuf->type != UCODE_UCODE_TYPE ) evaluates to *false and returns -EINVAL. */ -if ( offset + SECTION_HDR_SIZE <= bufsize && +if ( offset + SECTION_HDR_SIZE <= size && *(const uint32_t *)(buf + offset) == UCODE_MAGIC ) break; } -- 2.11.0
[PATCH 02/11] x86/ucode/amd: Move check_final_patch_levels() to apply_microcode()
The microcode revision of whichever CPU runs cpu_request_microcode() is not necessarily applicable to other CPUs. If the BIOS left us with asymmetric microcode, rejecting updates in cpu_request_microcode() would prevent us levelling the system even if only up to the final level. Also, failing to cache microcode misses an opportunity to get beyond the final level via the S3 path. Move check_final_patch_levels() earlier and use it in apply_microcode(). Reword the error message to be more informative, and use -ENXIO as this corner case has nothing to do with permissions. Signed-off-by: Andrew Cooper --- CC: Jan Beulich CC: Wei Liu CC: Roger Pau Monné --- xen/arch/x86/cpu/microcode/amd.c | 83 ++-- 1 file changed, 38 insertions(+), 45 deletions(-) diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c index 796745e928..4245dc13bb 100644 --- a/xen/arch/x86/cpu/microcode/amd.c +++ b/xen/arch/x86/cpu/microcode/amd.c @@ -119,6 +119,36 @@ static bool_t verify_patch_size(uint32_t patch_size) return (patch_size <= max_size); } +static bool check_final_patch_levels(const struct cpu_signature *sig) +{ +/* + * The 'final_levels' of patch ids have been obtained empirically. + * Refer bug https://bugzilla.suse.com/show_bug.cgi?id=913996 + * for details of the issue. The short version is that people + * using certain Fam10h systems noticed system hang issues when + * trying to update microcode levels beyond the patch IDs below. + * From internal discussions, we gathered that OS/hypervisor + * cannot reliably perform microcode updates beyond these levels + * due to hardware issues. Therefore, we need to abort microcode + * update process if we hit any of these levels. + */ +static const unsigned int final_levels[] = { +0x0198, +0x019f, +0x01af, +}; +unsigned int i; + +if ( boot_cpu_data.x86 != 0x10 ) +return false; + +for ( i = 0; i < ARRAY_SIZE(final_levels); i++ ) +if ( sig->rev == final_levels[i] ) +return true; + +return false; +} + static bool_t find_equiv_cpu_id(const struct equiv_cpu_entry *equiv_cpu_table, unsigned int current_cpu_id, unsigned int *equiv_cpu_id) @@ -229,6 +259,14 @@ static int apply_microcode(const struct microcode_patch *patch) if ( !match_cpu(patch) ) return -EINVAL; +if ( check_final_patch_levels(sig) ) +{ +printk(XENLOG_ERR + "microcode: CPU%u current rev %#x unsafe to update\n", + cpu, sig->rev); +return -ENXIO; +} + hdr = patch->mpb; hw_err = wrmsr_safe(MSR_AMD_PATCHLOADER, (unsigned long)hdr); @@ -374,43 +412,6 @@ static int container_fast_forward(const void *data, size_t size_left, size_t *of return 0; } -/* - * The 'final_levels' of patch ids have been obtained empirically. - * Refer bug https://bugzilla.suse.com/show_bug.cgi?id=913996 - * for details of the issue. The short version is that people - * using certain Fam10h systems noticed system hang issues when - * trying to update microcode levels beyond the patch IDs below. - * From internal discussions, we gathered that OS/hypervisor - * cannot reliably perform microcode updates beyond these levels - * due to hardware issues. Therefore, we need to abort microcode - * update process if we hit any of these levels. - */ -static const unsigned int final_levels[] = { -0x0198, -0x019f, -0x01af -}; - -static bool_t check_final_patch_levels(unsigned int cpu) -{ -/* - * Check the current patch levels on the cpu. If they are equal to - * any of the 'final_levels', then we should not update the microcode - * patch on the cpu as system will hang otherwise. - */ -const struct cpu_signature *sig = &per_cpu(cpu_sig, cpu); -unsigned int i; - -if ( boot_cpu_data.x86 != 0x10 ) -return 0; - -for ( i = 0; i < ARRAY_SIZE(final_levels); i++ ) -if ( sig->rev == final_levels[i] ) -return 1; - -return 0; -} - static struct microcode_patch *cpu_request_microcode(const void *buf, size_t bufsize) { @@ -434,14 +435,6 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, goto out; } -if ( check_final_patch_levels(cpu) ) -{ -printk(XENLOG_INFO - "microcode: Cannot update microcode patch on the cpu as we hit a final level\n"); -error = -EPERM; -goto out; -} - mc_amd = xzalloc(struct microcode_amd); if ( !mc_amd ) { -- 2.11.0
[PATCH 05/11] x86/ucode/amd: Overhaul the equivalent cpu table handling completely
We currently copy the entire equivalency table, and the single correct microcode. This is not safe to heterogeneous scenarios, and as Xen doesn't support such situations to being with, can be used to simplify things further. The CPUID.1.EAX => processor_rev_id mapping is fixed for an individual part. We can cache the single appropriate entry on first discovery, and forgo duplicating the entire table. Alter install_equiv_cpu_table() to be scan_equiv_cpu_table() which is responsible for checking the equivalency table and caching appropriate details. It now has a check for finding a different mapping (which indicates that one of the tables we've seen is definitely wrong). A return value of -ESRCH is now used to signify "everything fine, but nothing applicable for the current CPU", which is used to select the container_fast_forward() path. Drop the printk(), as each applicable error path in scan_equiv_cpu_table() already prints diagnostics. Signed-off-by: Andrew Cooper --- CC: Jan Beulich CC: Wei Liu CC: Roger Pau Monné Naming of 'equiv' subject to improvement. An alternative would be to embed the full equivelancy table, but it is fairly large, and would need adjusting every time a new model/stepping was released. --- xen/arch/x86/cpu/microcode/amd.c | 112 ++- 1 file changed, 64 insertions(+), 48 deletions(-) diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c index d2ecc7ae87..d3439b0c6c 100644 --- a/xen/arch/x86/cpu/microcode/amd.c +++ b/xen/arch/x86/cpu/microcode/amd.c @@ -61,8 +61,6 @@ struct __packed microcode_header_amd { struct microcode_patch { struct microcode_header_amd *mpb; size_t mpb_size; -struct equiv_cpu_entry *equiv_cpu_table; -size_t equiv_cpu_table_size; }; /* Temporary, until the microcode_* structure are disentangled. */ @@ -74,6 +72,31 @@ struct mpbhdr { uint8_t data[]; }; +/* + * Microcode updates for different CPUs are distinguished by their + * processor_rev_id in the header. This denotes the format of the internals + * of the microcode engine, and is fixed for an individual CPU. + * + * There is a mapping from the CPU signature (CPUID.1.EAX - + * family/model/stepping) to the "equivalent CPU identifier" which is + * similarly fixed. In some cases, multiple different CPU signatures map to + * the same equiv_id for processor lines which share identical microcode + * facilities. + * + * This mapping can't be calculated in the general case, but is provided in + * the microcode container, so the correct piece of microcode for the CPU can + * be identified. We cache it the first time we encounter the correct mapping + * for this system. + * + * Note: for now, we assume a fully homogeneous setup, meaning that there is + * exactly one equiv_id we need to worry about for microcode blob + * identification. This may need revisiting in due course. + */ +static struct { +uint32_t sig; +uint16_t id; +} equiv __read_mostly; + /* See comment in start_update() for cases when this routine fails */ static int collect_cpu_info(struct cpu_signature *csig) { @@ -150,40 +173,15 @@ static bool check_final_patch_levels(const struct cpu_signature *sig) return false; } -static bool_t find_equiv_cpu_id(const struct equiv_cpu_entry *equiv_cpu_table, -unsigned int current_cpu_id, -unsigned int *equiv_cpu_id) -{ -unsigned int i; - -if ( !equiv_cpu_table ) -return 0; - -for ( i = 0; equiv_cpu_table[i].installed_cpu != 0; i++ ) -{ -if ( current_cpu_id == equiv_cpu_table[i].installed_cpu ) -{ -*equiv_cpu_id = equiv_cpu_table[i].equiv_cpu & 0x; -return 1; -} -} - -return 0; -} - static enum microcode_match_result microcode_fits( const struct microcode_amd *mc_amd) { unsigned int cpu = smp_processor_id(); const struct cpu_signature *sig = &per_cpu(cpu_sig, cpu); const struct microcode_header_amd *mc_header = mc_amd->mpb; -const struct equiv_cpu_entry *equiv_cpu_table = mc_amd->equiv_cpu_table; -unsigned int equiv_cpu_id; -if ( !find_equiv_cpu_id(equiv_cpu_table, sig->sig, &equiv_cpu_id) ) -return MIS_UCODE; - -if ( (mc_header->processor_rev_id) != equiv_cpu_id ) +if ( equiv.sig != sig->sig || + equiv.id != mc_header->processor_rev_id ) return MIS_UCODE; if ( !verify_patch_size(mc_amd->mpb_size) ) @@ -213,7 +211,6 @@ static void free_patch(struct microcode_patch *mc_amd) { if ( mc_amd ) { -xfree(mc_amd->equiv_cpu_table); xfree(mc_amd->mpb); xfree(mc_amd); } @@ -335,14 +332,15 @@ static int get_ucode_from_buffer_amd( return 0; } -static int install_equiv_cpu_table( -struct microcode_amd *mc_amd, +static int scan_equiv_cpu_table( const void *data, size_t size_left, size_t *offset) { +const struct
[PATCH 00/11] x86/ucode: Cleanup and fixes - Part 4/n (AMD)
The first patch definitely needs backporting. Second is a good candidate as well. Everything else probably not. This follows similar cleanup on the Intel side, removing gratuitous memory allocations (both interms of number, and indirection), and fixes several things to be more uniform (handling of cpu_sig->sig, and parsing of multiple containers. Andrew Cooper (11): x86/ucode/amd: Fix more potential buffer overruns with microcode parsing x86/ucode/amd: Move check_final_patch_levels() to apply_microcode() x86/ucode/amd: Don't use void * for microcode_patch->mpb x86/ucode/amd: Collect CPUID.1.EAX in collect_cpu_info() x86/ucode/amd: Overhaul the equivalent cpu table handling completely x86/ucode/amd: Move verify_patch_size() into get_ucode_from_buffer_amd() x86/ucode/amd: Alter API for microcode_fits() x86/ucode/amd: Rename bufsize to size in cpu_request_microcode() x86/ucode/amd: Remove gratuitous memory allocations from cpu_request_microcode() x86/ucode/amd: Fold structures together x86/ucode/amd: Rework parsing logic in cpu_request_microcode() xen/arch/x86/cpu/microcode/amd.c | 512 +-- xen/include/asm-x86/microcode.h | 2 +- 2 files changed, 176 insertions(+), 338 deletions(-) -- 2.11.0
[ovmf test] 149207: all pass - PUSHED
flight 149207 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/149207/ Perfect :-) All tests in this flight passed as required version targeted for testing: ovmf 3000c2963db319d055f474c394b062af910bbb2f baseline version: ovmf d671d1fa48dbb3f22b68c1d67914c55ba1d58454 Last test of basis 149176 2020-03-29 17:13:07 Z1 days Testing same since 149207 2020-03-30 12:11:22 Z0 days1 attempts People who touched revisions under test: Ard Biesheuvel GuoMinJ Hao A Wu 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 pass test-amd64-i386-xl-qemuu-ovmf-amd64 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 : To xenbits.xen.org:/home/xen/git/osstest/ovmf.git d671d1fa48..3000c2963d 3000c2963db319d055f474c394b062af910bbb2f -> xen-tested-master
Re: [PATCH V7] x86/altp2m: Hypercall to set altp2m view visibility
On 31.03.2020 09:54, Isaila Alexandru wrote: > > > On 31.03.2020 10:43, Jan Beulich wrote: >> On 30.03.2020 08:54, Alexandru Isaila wrote: >>> At this moment a guest can call vmfunc to change the altp2m view. This >>> should be limited in order to avoid any unwanted view switch. >>> >>> The new xc_altp2m_set_visibility() solves this by making views invisible >>> to vmfunc. >>> This is done by having a separate arch.altp2m_working_eptp that is >>> populated and made invalid in the same places as altp2m_eptp. This is >>> written to EPTP_LIST_ADDR. >>> The views are made in/visible by marking them with INVALID_MFN or >>> copying them back from altp2m_eptp. >>> To have consistency the visibility also applies to >>> p2m_switch_domain_altp2m_by_id(). >>> >>> The usage of this hypercall is aimed at dom0 having a logic with a number >>> of views >>> created and at some time there is a need to be sure that only some of the >>> views >>> can be switched, saving the rest and making them visible when the time >>> is right. >>> >>> Note: If altp2m mode is set to mixed the guest is able to change the view >>> visibility and then call vmfunc. >>> >>> Signed-off-by: Alexandru Isaila >> >> For v6 I did provide a hypervisor side R-b; I didn't think ... > > No you didn't. https://lists.xenproject.org/archives/html/xen-devel/2020-03/msg00145.html Jan