[ovmf test] 175435: regressions - FAIL
flight 175435 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/175435/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-amd64-i386-xl-qemuu-ovmf-amd64 12 debian-hvm-install fail REGR. vs. 175202 test-amd64-amd64-xl-qemuu-ovmf-amd64 12 debian-hvm-install fail REGR. vs. 175202 version targeted for testing: ovmf ec25e904c7da70302f2725e2005e3762f1ae891e baseline version: ovmf d103840cfb559c28831c2635b916d60118f671cc Last test of basis 175202 2022-12-14 13:42:59 Z6 days Failing since175214 2022-12-14 18:42:16 Z6 days 39 attempts Testing same since 175435 2022-12-21 02:45:59 Z0 days1 attempts People who touched revisions under test: Abner Chang Adam Dunlap Ard Biesheuvel Chun-Yi Lee Chun-Yi Lee de...@edk2.groups.io Dov Murik Gerd Hoffmann jdzhang Jeff Brasen Jeshua Smith Jian J Wang Jiaqi Gao Jiewen Yao Judah Vang Kavya Kuo, Ted MarsX Lin Matt DeVillier Michael Kubacki Min M Xu Min Xu Nishant C Mistry Ray Ni Sean Rhodes Sebastien Boeuf Ted Kuo Tom Lendacky Xie, Yuanhao Yuanhao Xie jobs: build-amd64-xsm pass build-i386-xsm pass build-amd64 pass build-i386 pass build-amd64-libvirt pass build-i386-libvirt pass build-amd64-pvopspass build-i386-pvops pass test-amd64-amd64-xl-qemuu-ovmf-amd64 fail test-amd64-i386-xl-qemuu-ovmf-amd64 fail sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Not pushing. (No revision log; it would be 946 lines long.)
Re: [RFC 1/7] x86/iommu: make AMD-Vi and Intel VT-d support configurable
On 19.12.2022 07:34, Xenia Ragiadakou wrote: > --- a/xen/drivers/passthrough/Kconfig > +++ b/xen/drivers/passthrough/Kconfig > @@ -37,6 +37,22 @@ config IPMMU_VMSA > > endif > > +config AMD_IOMMU > + bool "AMD IOMMU" > + depends on X86 > + default y > + ---help--- > + Enables I/O virtualization on platforms that implement the > + AMD I/O Virtualization Technology (IOMMU). > + > +config INTEL_VTD > + bool "Intel VT-d" > + depends on X86 > + default y > + ---help--- > + Enables I/O virtualization on platforms that implement the > + Intel Virtualization Technology for Directed I/O (Intel VT-d). One more thing Andrew and I have been talking about: As he has mentioned elsewhere, IOMMU support is needed to boot systems with more than 254 CPUs (depending on APIC ID layout the boundary may actually be lower). Hence it needs to at least be considered to make the prompts here (to be precise: in the much later patch adding the prompts) dependent on EXPERT, to prevent people from unknowingly building a non-functioning (on some systems) hypervisor. Jan
Re: [PATCH v2-ish] x86/boot: Factor move_xen() out of __start_xen()
On 20.12.2022 21:56, Andrew Cooper wrote:
> On 20/12/2022 1:51 pm, Jan Beulich wrote:
>> On 16.12.2022 21:17, Andrew Cooper wrote:
>>> +"mov%[cr4], %%cr4\n\t" /* CR4.PGE = 1 */
>>> +: [cr4] "=&a" (tmp) /* Could be "r", but "a" makes better asm */
>>> +: [cr3] "r" (__pa(idle_pg_table)),
>>> + [pge] "i" (X86_CR4_PGE)
>>> +: "memory" );
>> The removed stack copying worries me, to be honest. Yes, for local
>> variables of ours it doesn't matter because they are about to go out
>> of scope. But what if the compiler instantiates any for its own use,
>> e.g. ...
>>
>>> +/*
>>> + * End of the critical region. Updates to locals and globals now work
>>> as
>>> + * expected.
>>> + *
>>> + * Updates to local variables which have been spilled to the stack
>>> since
>>> + * the memcpy() have been lost. But we don't care, because they're all
>>> + * going out of scope imminently.
>>> + */
>>> +
>>> +printk("New Xen image base address: %#lx\n", xen_phys_start);
>> ... the result of the address calculation for the string literal
>> here? Such auxiliary calculations can happen at any point in the
>> function, and won't necessarily (hence the example chosen) become
>> impossible for the compiler to do with the memory clobber in the
>> asm(). And while the string literal's address is likely cheap
>> enough to calculate right in the function invocation sequence (and
>> an option would also be to retain the printk() in the caller),
>> other instrumentation options could be undermined by this as well.
>
> Right now, the compiler is free to calculate the address of the string
> literal in a register, and move it the other side of the TLB flush.
> This will work just fine.
>
> However, the compiler cannot now, or ever in the future, spill such a
> calculation to the stack.
I'm afraid the compiler's view at things is different: Whatever it puts
on the stack is viewed as virtual registers, unaffected by a memory
clobber (of course there can be effects resulting from uses of named
variables). Look at -O3 output of gcc12 (which is what I happened to
play with; I don't think it's overly version dependent) for this
(clearly contrived) piece of code:
int __attribute__((const)) calc(int);
int test(int i) {
int j = calc(i);
asm("nopl %0" : "+m" (j));
asm("nopq %%rsp" ::: "memory", "ax", "cx", "dx", "bx", "bp", "si", "di",
"r8", "r9", "r10", "r11", "r12", "r13", "r14",
"r15");
j = calc(i);
asm("nopl %0" :: "m" (j));
return j;
}
It instantiates a local on the stack for the result of calc(); it does
not re-invoke calc() a 2nd time. Which means the memory clobber in the
middle asm() does not affect that (and by implication: any) stack slot.
Using godbolt I can also see that clang15 agrees with gcc12 in this
regard. I didn't bother trying other versions.
> Whether it's spelt "memory", or something else in the future, OSes
> really do genuinely need a way of telling the compiler "you literally
> cannot move anything the other side of this asm()", because otherwise
> malfunctions will occur.
Something like this may indeed be desirable in situations like this one,
but I don't think such a construct exists.
Jan
Re: [PATCH v5 11/10] hvmloader: use memory type constants
On 20.12.2022 18:45, Demi Marie Obenour wrote:
> On Tue, Dec 20, 2022 at 05:13:04PM +0100, Jan Beulich wrote:
>> --- a/tools/firmware/hvmloader/cacheattr.c
>> +++ b/tools/firmware/hvmloader/cacheattr.c
>> @@ -22,6 +22,8 @@
>> #include "util.h"
>> #include "config.h"
>>
>> +#include
>> +
>> #define MSR_MTRRphysBase(reg) (0x200 + 2 * (reg))
>> #define MSR_MTRRphysMask(reg) (0x200 + 2 * (reg) + 1)
>> #define MSR_MTRRcap 0x00fe
>> @@ -71,23 +73,32 @@ void cacheattr_init(void)
>>
>> addr_mask = ((1ull << phys_bits) - 1) & ~((1ull << 12) - 1);
>> mtrr_cap = rdmsr(MSR_MTRRcap);
>> -mtrr_def = (1u << 11) | 6; /* E, default type WB */
>> +mtrr_def = (1u << 11) | X86_MT_WB; /* E, default type WB */
>>
>> /* Fixed-range MTRRs supported? */
>> if ( mtrr_cap & (1u << 8) )
>> {
>> +#define BCST2(mt) ((mt) | ((mt) << 8))
>> +#define BCST4(mt) (BCST2(mt) | (BCST2(mt) << 16))
>
> This should include a cast to uint32_t, just like BCST8 includes a cast
> to uint64_t. It doesn’t have any functional impact since none of the
> memory types have the high bit set, but it makes the correctness of the
> code much more obvious to readers.
I've already followed Andrew's suggestion.
> With the suggested change:
>
> Acked-by: Demi Marie Obenour
Thanks. Since I've addressed this differently, I'll hold back applying
of this.
>From a formal perspective I'd also like to point out that an Acked-by:
from a person not being maintainer of any code being changed by a patch
has no meaning at all. Only Reviewed-by: has a meaning (but of course
implies more thorough looking at the change than Acked-by: does).
Jan
[xen-4.17-testing test] 175425: regressions - trouble: blocked/broken/fail/pass
flight 175425 xen-4.17-testing real [real] http://logs.test-lab.xenproject.org/osstest/logs/175425/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-arm64-pvopsbroken build-arm64-pvops 4 host-install(4)broken REGR. vs. 175096 test-amd64-i386-qemut-rhel6hvm-amd 7 xen-installfail REGR. vs. 175096 test-amd64-i386-xl7 xen-install fail REGR. vs. 175096 Tests which did not succeed, but are not blocking: test-arm64-arm64-libvirt-raw 1 build-check(1) blocked n/a test-arm64-arm64-libvirt-xsm 1 build-check(1) blocked n/a test-arm64-arm64-xl 1 build-check(1) blocked n/a test-arm64-arm64-xl-credit1 1 build-check(1) blocked n/a test-arm64-arm64-xl-credit2 1 build-check(1) blocked n/a test-arm64-arm64-xl-seattle 1 build-check(1) blocked n/a test-arm64-arm64-xl-thunderx 1 build-check(1) blocked n/a test-arm64-arm64-xl-vhd 1 build-check(1) blocked n/a test-arm64-arm64-xl-xsm 1 build-check(1) blocked n/a test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 175096 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 175096 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 175096 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 175096 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 175096 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 175096 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 175096 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 175096 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 175096 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 175096 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 175096 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 175096 test-amd64-i386-xl-pvshim14 guest-start fail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-i386-libvirt 15 migrate-support-checkfail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-i386-libvirt-xsm 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-i386-libvirt-raw 14 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-rtds 15 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-cubietruck 15 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 15 migrate-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-raw 14 migrate-support-checkfail never pass version targeted for testing: xen a7a26da0b59da7233e6c6f63b180bab131398351 baseline version: xen 11560248ffda3f00f20bbdf3ae088af474f7f2a3 Last test of basis 175096 2022-12-08 18:07:02 Z 12 days Testing same since 175425 2022-12-20 13:06:46 Z0 days1 attempts People who touched revisions under test: Andrew Cooper Demi Marie Obenour Jan Beulich Neowutran Per Bilse Roger Pau Monné jobs: build-amd64-xsm
[ovmf test] 175432: regressions - FAIL
flight 175432 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/175432/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-amd64-i386-xl-qemuu-ovmf-amd64 12 debian-hvm-install fail REGR. vs. 175202 test-amd64-amd64-xl-qemuu-ovmf-amd64 12 debian-hvm-install fail REGR. vs. 175202 version targeted for testing: ovmf 3f378450dfafc11754bfdb64af28060ec8466acb baseline version: ovmf d103840cfb559c28831c2635b916d60118f671cc Last test of basis 175202 2022-12-14 13:42:59 Z6 days Failing since175214 2022-12-14 18:42:16 Z6 days 38 attempts Testing same since 175423 2022-12-20 10:40:57 Z0 days3 attempts People who touched revisions under test: Abner Chang Adam Dunlap Ard Biesheuvel Chun-Yi Lee Chun-Yi Lee de...@edk2.groups.io Dov Murik Gerd Hoffmann jdzhang Jeff Brasen Jeshua Smith Jian J Wang Jiaqi Gao Jiewen Yao Judah Vang Kavya Kuo, Ted MarsX Lin Michael Kubacki Min M Xu Min Xu Nishant C Mistry Ray Ni Sebastien Boeuf Ted Kuo Tom Lendacky Xie, Yuanhao Yuanhao Xie jobs: build-amd64-xsm pass build-i386-xsm pass build-amd64 pass build-i386 pass build-amd64-libvirt pass build-i386-libvirt pass build-amd64-pvopspass build-i386-pvops pass test-amd64-amd64-xl-qemuu-ovmf-amd64 fail test-amd64-i386-xl-qemuu-ovmf-amd64 fail sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Not pushing. (No revision log; it would be 894 lines long.)
[libvirt test] 175419: regressions - trouble: broken/fail/pass
flight 175419 libvirt real [real] http://logs.test-lab.xenproject.org/osstest/logs/175419/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-amd64-i386-libvirt-pair broken test-amd64-i386-libvirt-pair 7 host-install/dst_host(7) broken REGR. vs. 175306 test-arm64-arm64-libvirt-qcow2 17 guest-start/debian.repeat fail REGR. vs. 175306 Tests which did not succeed, but are not blocking: test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 175306 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 175306 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 175306 test-amd64-i386-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-i386-libvirt 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-arm64-arm64-libvirt-qcow2 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-qcow2 15 saverestore-support-checkfail never pass test-amd64-i386-libvirt-raw 14 migrate-support-checkfail never pass test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 15 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 14 migrate-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass version targeted for testing: libvirt 4102acc608819b15658ca3e37ceca7c89efae16b baseline version: libvirt b271d6f3b0d70e8e7be22252bf25deebe8536d39 Last test of basis 175306 2022-12-16 04:21:45 Z4 days Testing same since 175419 2022-12-20 04:18:52 Z0 days1 attempts People who touched revisions under test: Ján Tomko Peter Krempa 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 pass build-arm64-libvirt pass build-armhf-libvirt pass build-i386-libvirt pass build-amd64-pvopspass build-arm64-pvopspass build-armhf-pvopspass build-i386-pvops pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsmpass test-amd64-amd64-libvirt-xsm pass test-arm64-arm64-libvirt-xsm pass test-amd64-i386-libvirt-xsm pass test-amd64-amd64-libvirt pass test-arm64-arm64-libvirt pass test-armhf-armhf-libvirt pass test-amd64-i386-libvirt pass test-amd64-amd64-libvirt-pairpass test-amd64-i386-libvirt-pair broken test-arm64-arm64-libvirt-qcow2 fail test-armhf-armhf-libvirt-qcow2 pass test-arm64-arm64-libvirt-raw pass test-armhf-armhf-libvirt-raw pass test-amd64-i386-libvirt-raw pass test-amd64-amd64-libvirt-vhd pass --
[xen-unstable test] 175421: regressions - trouble: broken/fail/pass
flight 175421 xen-unstable real [real] http://logs.test-lab.xenproject.org/osstest/logs/175421/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-amd64-i386-migrupgrade broken test-amd64-i386-xl-pvshimbroken test-amd64-i386-xl-qemuu-dmrestrict-amd64-dmrestrictbroken test-amd64-amd64-xl-qemuu-dmrestrict-amd64-dmrestrict broken test-amd64-i386-xl-pvshim 5 host-install(5)broken REGR. vs. 175399 test-amd64-i386-migrupgrade 6 host-install/src_host(6) broken REGR. vs. 175399 test-amd64-i386-xl-qemuu-dmrestrict-amd64-dmrestrict 5 host-install(5) broken REGR. vs. 175399 test-amd64-amd64-xl-qemuu-dmrestrict-amd64-dmrestrict 5 host-install(5) broken REGR. vs. 175399 test-amd64-amd64-examine-uefi 5 host-install broken REGR. vs. 175399 test-amd64-i386-libvirt 7 xen-install fail REGR. vs. 175399 test-amd64-amd64-xl-qcow2 21 guest-start/debian.repeat fail REGR. vs. 175399 Regressions which are regarded as allowable (not blocking): test-amd64-amd64-examine-uefi 4 memdisk-try-append fail REGR. vs. 175399 Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 175399 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 175399 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 175399 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 175399 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 175399 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 175399 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 175399 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 175399 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 175399 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 175399 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 175399 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 175399 test-amd64-i386-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-xl 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit1 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-rtds 15 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-seattle 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-cubietruck 15 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 15 migrate-support-check
[linux-5.4 test] 175418: regressions - trouble: broken/fail/pass
flight 175418 linux-5.4 real [real] http://logs.test-lab.xenproject.org/osstest/logs/175418/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-amd64-i386-xl-qemut-debianhvm-amd64broken test-amd64-i386-xl-qemuu-debianhvm-amd64broken test-amd64-amd64-xl-qemut-debianhvm-amd64 broken test-amd64-amd64-dom0pvh-xl-intel broken test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm broken test-amd64-i386-xl-qemuu-ws16-amd64 broken test-amd64-amd64-xl-qemut-debianhvm-amd64 5 host-install(5) broken REGR. vs. 175197 test-amd64-i386-xl-qemuu-ws16-amd64 5 host-install(5) broken REGR. vs. 175197 test-amd64-i386-qemuu-rhel6hvm-intel broken in 175407 test-amd64-amd64-xl-credit2 broken in 175407 test-armhf-armhf-xl-multivcpu 14 guest-start fail REGR. vs. 175197 Tests which are failing intermittently (not blocking): test-amd64-i386-qemuu-rhel6hvm-intel 5 host-install(5) broken in 175407 pass in 175418 test-amd64-amd64-xl-credit2 5 host-install(5) broken in 175407 pass in 175418 test-amd64-i386-xl-qemuu-debianhvm-amd64 5 host-install(5) broken pass in 175407 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 5 host-install(5) broken pass in 175407 test-amd64-i386-xl-qemut-debianhvm-amd64 5 host-install(5) broken pass in 175407 test-amd64-amd64-dom0pvh-xl-intel 5 host-install(5) broken pass in 175407 test-amd64-i386-examine 5 host-install broken pass in 175407 test-arm64-arm64-libvirt-raw 18 guest-start.2 fail pass in 175407 test-amd64-i386-xl-vhd 21 guest-start/debian.repeat fail pass in 175407 test-armhf-armhf-xl-vhd 13 guest-startfail pass in 175407 test-armhf-armhf-xl-credit1 18 guest-start/debian.repeat fail pass in 175407 Tests which did not succeed, but are not blocking: test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail blocked in 175197 test-amd64-i386-xl-qemuu-debianhvm-i386-xsm 12 debian-hvm-install fail in 175407 like 175197 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail in 175407 never pass test-armhf-armhf-xl-vhd 14 migrate-support-check fail in 175407 never pass test-armhf-armhf-xl-vhd 15 saverestore-support-check fail in 175407 never pass test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 175197 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 175197 test-armhf-armhf-xl-rtds 18 guest-start/debian.repeatfail like 175197 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 175197 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 175197 test-armhf-armhf-xl-credit2 18 guest-start/debian.repeatfail like 175197 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 175197 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 175197 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 175197 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 175197 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 175197 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 175197 test-amd64-i386-xl-pvshim14 guest-start fail never pass test-amd64-i386-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 16 saverestore-support-checkfail never pass test-amd64-i386-libvirt 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-xl 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-arm64-arm64-xl-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail
[xen-unstable-smoke test] 175431: tolerable all pass - PUSHED
flight 175431 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/175431/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass version targeted for testing: xen dc380df12acfe53ccdcbeecaaee3510a3b0e374e baseline version: xen 4b40d68e663e87eb916b5bdf3da9743a17f43537 Last test of basis 175428 2022-12-20 16:00:30 Z0 days Testing same since 175431 2022-12-20 21:03:41 Z0 days1 attempts People who touched revisions under test: Jan Beulich Sergey Dyasli 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 4b40d68e66..dc380df12a dc380df12acfe53ccdcbeecaaee3510a3b0e374e -> smoke
[ovmf test] 175429: regressions - FAIL
flight 175429 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/175429/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-amd64-i386-xl-qemuu-ovmf-amd64 12 debian-hvm-install fail REGR. vs. 175202 test-amd64-amd64-xl-qemuu-ovmf-amd64 12 debian-hvm-install fail REGR. vs. 175202 version targeted for testing: ovmf 3f378450dfafc11754bfdb64af28060ec8466acb baseline version: ovmf d103840cfb559c28831c2635b916d60118f671cc Last test of basis 175202 2022-12-14 13:42:59 Z6 days Failing since175214 2022-12-14 18:42:16 Z6 days 37 attempts Testing same since 175423 2022-12-20 10:40:57 Z0 days2 attempts People who touched revisions under test: Abner Chang Adam Dunlap Ard Biesheuvel Chun-Yi Lee Chun-Yi Lee de...@edk2.groups.io Dov Murik Gerd Hoffmann jdzhang Jeff Brasen Jeshua Smith Jian J Wang Jiaqi Gao Jiewen Yao Judah Vang Kavya Kuo, Ted MarsX Lin Michael Kubacki Min M Xu Min Xu Nishant C Mistry Ray Ni Sebastien Boeuf Ted Kuo Tom Lendacky Xie, Yuanhao Yuanhao Xie jobs: build-amd64-xsm pass build-i386-xsm pass build-amd64 pass build-i386 pass build-amd64-libvirt pass build-i386-libvirt pass build-amd64-pvopspass build-i386-pvops pass test-amd64-amd64-xl-qemuu-ovmf-amd64 fail test-amd64-i386-xl-qemuu-ovmf-amd64 fail sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Not pushing. (No revision log; it would be 894 lines long.)
Re: [RFC 0/4] Adding Virtual Memory Fuses to Xen
On Tue, Dec 20, 2022 at 10:17:24PM +, Smith, Jackson wrote: > -Original Message- > > From: Julien Grall > > Sent: Friday, December 16, 2022 3:39 AM > > > > Hi Stefano, > > > > On 16/12/2022 01:46, Stefano Stabellini wrote: > > > On Thu, 15 Dec 2022, Julien Grall wrote: > > On 13/12/2022 19:48, Smith, Jackson wrote: > > >>> Yes, we are familiar with the "secret-free hypervisor" work. As > you > > >>> point out, both our work and the secret-free hypervisor remove the > > >>> directmap region to mitigate the risk of leaking sensitive guest > > >>> secrets. However, our work is slightly different because it > > >>> additionally prevents attackers from tricking Xen into remapping a > > guest. > > >> > > >> I understand your goal, but I don't think this is achieved (see > > >> above). You would need an entity to prevent write to TTBR0_EL2 in > > >> order to fully protect it. > > > > > > Without a way to stop Xen from reading/writing TTBR0_EL2, we > > cannot > > > claim that the guest's secrets are 100% safe. > > > > > > But the attacker would have to follow the sequence you outlines > > above > > > to change Xen's pagetables and remap guest memory before > > accessing it. > > > It is an additional obstacle for attackers that want to steal other > > guests' > > > secrets. The size of the code that the attacker would need to inject > > > in Xen would need to be bigger and more complex. > > > > Right, that's why I wrote with a bit more work. However, the nuance > > you mention doesn't seem to be present in the cover letter: > > > > "This creates what we call "Software Enclaves", ensuring that an > > adversary with arbitrary code execution in the hypervisor STILL cannot > > read/write guest memory." > > > > So if the end goal if really to protect against *all* sort of > arbitrary > > code, > > then I think we should have a rough idea how this will look like in > Xen. > > > > From a brief look, it doesn't look like it would be possible to > prevent > > modification to TTBR0_EL2 (even from EL3). We would need to > > investigate if there are other bits in the architecture to help us. > > > > > > > > Every little helps :-) > > > > I can see how making the life of the attacker more difficult is > > appealing. > > Yet, the goal needs to be clarified and the risk with the approach > > acknowledged (see above). > > > > You're right, we should have mentioned this weakness in our first email. > Sorry about the oversight! This is definitely still a limitation that we > have not yet overcome. However, we do think that the increase in > attacker workload that you and Stefano are discussing could still be > valuable to security conscious Xen users. > > It would nice to find additional architecture features that we can use > to close this hole on arm, but there aren't any that stand out to me > either. > > With this limitation in mind, what are the next steps we should take to > support this feature for the xen community? Is this increase in attacker > workload meaningful enough to justify the inclusion of VMF in Xen? Personally, I don’t think so. The kinds of workloads VMF is usable for (no hypercalls) are likely easily portable to other hypervisors, including formally verified microkernels such as seL4 that provide a significantly higher level of assurance. seL4’s proofs do need to be ported to each particular board, but this is fairly simple. Conversely, workloads that need Xen’s features cannot use VMF, so VMF again is not suitable. Have you considered other approaches to improving security, such as fuzzing Xen’s hypercall interface or even using formal methods? Those would benefit all users of Xen, not merely a small subset who already have alternatives available. -- Sincerely, Demi Marie Obenour (she/her/hers) Invisible Things Lab signature.asc Description: PGP signature
RE: [RFC 0/4] Adding Virtual Memory Fuses to Xen
-Original Message- > From: Julien Grall > Sent: Friday, December 16, 2022 3:39 AM > > Hi Stefano, > > On 16/12/2022 01:46, Stefano Stabellini wrote: > > On Thu, 15 Dec 2022, Julien Grall wrote: > On 13/12/2022 19:48, Smith, Jackson wrote: > >>> Yes, we are familiar with the "secret-free hypervisor" work. As you > >>> point out, both our work and the secret-free hypervisor remove the > >>> directmap region to mitigate the risk of leaking sensitive guest > >>> secrets. However, our work is slightly different because it > >>> additionally prevents attackers from tricking Xen into remapping a > guest. > >> > >> I understand your goal, but I don't think this is achieved (see > >> above). You would need an entity to prevent write to TTBR0_EL2 in > >> order to fully protect it. > > > > Without a way to stop Xen from reading/writing TTBR0_EL2, we > cannot > > claim that the guest's secrets are 100% safe. > > > > But the attacker would have to follow the sequence you outlines > above > > to change Xen's pagetables and remap guest memory before > accessing it. > > It is an additional obstacle for attackers that want to steal other > guests' > > secrets. The size of the code that the attacker would need to inject > > in Xen would need to be bigger and more complex. > > Right, that's why I wrote with a bit more work. However, the nuance > you mention doesn't seem to be present in the cover letter: > > "This creates what we call "Software Enclaves", ensuring that an > adversary with arbitrary code execution in the hypervisor STILL cannot > read/write guest memory." > > So if the end goal if really to protect against *all* sort of arbitrary > code, > then I think we should have a rough idea how this will look like in Xen. > > From a brief look, it doesn't look like it would be possible to prevent > modification to TTBR0_EL2 (even from EL3). We would need to > investigate if there are other bits in the architecture to help us. > > > > > Every little helps :-) > > I can see how making the life of the attacker more difficult is > appealing. > Yet, the goal needs to be clarified and the risk with the approach > acknowledged (see above). > You're right, we should have mentioned this weakness in our first email. Sorry about the oversight! This is definitely still a limitation that we have not yet overcome. However, we do think that the increase in attacker workload that you and Stefano are discussing could still be valuable to security conscious Xen users. It would nice to find additional architecture features that we can use to close this hole on arm, but there aren't any that stand out to me either. With this limitation in mind, what are the next steps we should take to support this feature for the xen community? Is this increase in attacker workload meaningful enough to justify the inclusion of VMF in Xen? Thanks, Jackson smime.p7s Description: S/MIME cryptographic signature
Re: [PATCH v2 15/19] tools/xenstore: switch hashtable to use the talloc framework
Hi Juergen,
On 13/12/2022 16:00, Juergen Gross wrote:
@@ -115,47 +117,32 @@ hashtable_expand(struct hashtable *h)
if (h->primeindex == (prime_table_length - 1)) return 0;
newsize = primes[++(h->primeindex)];
-newtable = (struct entry **)calloc(newsize, sizeof(struct entry*));
-if (NULL != newtable)
+newtable = talloc_realloc(h, h->table, struct entry *, newsize);
+if (!newtable)
{
-/* This algorithm is not 'stable'. ie. it reverses the list
- * when it transfers entries between the tables */
-for (i = 0; i < h->tablelength; i++) {
-while (NULL != (e = h->table[i])) {
-h->table[i] = e->next;
-index = indexFor(newsize,e->h);
+h->primeindex--;
+return 0;
+}
+
+h->table = newtable;
+memset(newtable + h->tablelength, 0,
+ (newsize - h->tablelength) * sizeof(*newtable));
+for (i = 0; i < h->tablelength; i++) {
I understand this code is taken from the realloc path. However, isn't
this algorithm also not stable? If so, then I think we move the comment
here.
+for (pE = &(newtable[i]), e = *pE; e != NULL; e = *pE) {
+index = indexFor(newsize,e->h);
Missing space after ",".
+if (index == i)
+{
+pE = &(e->next);
+}
+else
+{
+*pE = e->next;
e->next = newtable[index];
newtable[index] = e;
}
}
-free(h->table);
-h->table = newtable;
-}
-/* Plan B: realloc instead */
-else
-{
-newtable = (struct entry **)
- realloc(h->table, newsize * sizeof(struct entry *));
-if (NULL == newtable) { (h->primeindex)--; return 0; }
-h->table = newtable;
-memset(newtable + h->tablelength, 0,
- (newsize - h->tablelength) * sizeof(*newtable));
-for (i = 0; i < h->tablelength; i++) {
-for (pE = &(newtable[i]), e = *pE; e != NULL; e = *pE) {
-index = indexFor(newsize,e->h);
-if (index == i)
-{
-pE = &(e->next);
-}
-else
-{
-*pE = e->next;
-e->next = newtable[index];
-newtable[index] = e;
-}
-}
-}
}
+
h->tablelength = newsize;
h->loadlimit = (unsigned int)
(((uint64_t)newsize * max_load_factor) / 100);
@@ -184,7 +171,7 @@ hashtable_insert(struct hashtable *h, void *k, void *v)
* element may be ok. Next time we insert, we'll try expanding
again.*/
hashtable_expand(h);
}
-e = (struct entry *)calloc(1, sizeof(struct entry));
+e = talloc_zero(h, struct entry);
if (NULL == e) { --(h->entrycount); return 0; } /*oom*/
e->h = hash(h,k);
index = indexFor(h->tablelength,e->h);
@@ -238,8 +225,8 @@ hashtable_remove(struct hashtable *h, void *k)
h->entrycount--;
v = e->v;
if (h->flags & HASHTABLE_FREE_KEY)
-free(e->k);
-free(e);
+talloc_free(e->k);
+talloc_free(e);
return v;
}
pE = &(e->next);
@@ -280,25 +267,20 @@ void
hashtable_destroy(struct hashtable *h)
{
unsigned int i;
-struct entry *e, *f;
+struct entry *e;
struct entry **table = h->table;
for (i = 0; i < h->tablelength; i++)
{
-e = table[i];
-while (NULL != e)
+for (e = table[i]; e; e = e->next)
{
-f = e;
-e = e->next;
if (h->flags & HASHTABLE_FREE_KEY)
-free(f->k);
+talloc_free(e->k);
if (h->flags & HASHTABLE_FREE_VALUE)
-free(f->v);
-free(f);
AFAIU, the loop is reworked so you let talloc to free each element with
the parent. Using a while loop is definitely cleaner, but now you will
end up to have two separate loop for the elements.
There is a risk that the overall performance of hashtable_destroy() will
be worse as the data accessed in one loop may not fit in the cache. So
you will have to reload it on the second loop.
Therefore, I think it would be better to keep the loop as-is.
Cheers,
--
Julien Grall
Re: [RFC 1/7] x86/iommu: make AMD-Vi and Intel VT-d support configurable
On 12/20/22 23:00, Andrew Cooper wrote: On 20/12/2022 8:57 pm, Xenia Ragiadakou wrote: On 12/20/22 19:01, Jan Beulich wrote: On 19.12.2022 07:34, Xenia Ragiadakou wrote: Currently, for x86 platforms, Xen does not provide to the users any configuration control over the IOMMU support and can only be built with both AMD and Intel IOMMU drivers enabled. However, there are use cases, e.g in safety-critical systems, that require Xen to be able to be configured to exclude unused code. A smaller tailored configuration would help Xen to meet faster certification requirements for individual platforms. Introduce two new Kconfig options, AMD_IOMMU and INTEL_VTD, to allow code specific to each IOMMU technology to be separated and, when not required, stripped. AMD_IOMMU enables IOMMU support for platforms that implement the AMD I/O Virtualization Technology. INTEL_VTD enables IOMMU support for platforms that implement the Intel Virtualization Technology for Directed I/O. Since no functional change is intended regarding the default configuration of an x86 system, both options depend on x86 and default to 'y'. But do things also build successfully when one or both options are disabled? I have to say that I would be quite surprised if that worked without further adjustments. In which case initially these options want to be prompt-less, with the prompts only added once 'n' also works. Without applying the whole series, disabling any of them or both won't work. Ok. To do a multi-step implementation, you start with config FOO bool y Here, I think you mean def_bool y then rearrange them main code to use CONFIG_FOO as appropriate, then have a final patch that adds a Kconfig name, help text, etc which is what makes the config option user selectable and able to be turned off. Thank you both, for pointing that out. I will fix it. ~Andrew -- Xenia
Re: [RFC 2/7] x86/iommu: amd_iommu_perdev_intremap is AMD-Vi specific
On 12/20/22 19:04, Jan Beulich wrote: On 19.12.2022 07:34, Xenia Ragiadakou wrote: Move its definition to the AMD-Vi driver and use CONFIG_AMD_IOMMU to guard its usage in common code. Take the opportunity to replace bool_t with bool and 1 with true. Please further take the opportunity and use ... --- a/xen/drivers/passthrough/amd/iommu_init.c +++ b/xen/drivers/passthrough/amd/iommu_init.c @@ -36,6 +36,8 @@ static struct radix_tree_root ivrs_maps; LIST_HEAD_READ_MOSTLY(amd_iommu_head); bool_t iommuv2_enabled; +bool __read_mostly amd_iommu_perdev_intremap = true; ... __ro_after_init here. --- a/xen/include/xen/iommu.h +++ b/xen/include/xen/iommu.h @@ -104,7 +104,9 @@ static inline void clear_iommu_hap_pt_share(void) } extern bool_t iommu_debug; -extern bool_t amd_iommu_perdev_intremap; +#ifdef CONFIG_AMD_IOMMU +extern bool amd_iommu_perdev_intremap; +#endif We try to avoid such #ifdef-ary: There's no real harm from the declaration being in scope; in the worst case the build will fail not at the compile, but at the linking stage. That's true. I will leave it as it is. Jan -- Xenia
Re: [RFC 1/7] x86/iommu: make AMD-Vi and Intel VT-d support configurable
On 20/12/2022 8:57 pm, Xenia Ragiadakou wrote: > > On 12/20/22 19:01, Jan Beulich wrote: >> On 19.12.2022 07:34, Xenia Ragiadakou wrote: >>> Currently, for x86 platforms, Xen does not provide to the users any >>> configuration control over the IOMMU support and can only be built with >>> both AMD and Intel IOMMU drivers enabled. >>> However, there are use cases, e.g in safety-critical systems, that >>> require >>> Xen to be able to be configured to exclude unused code. A smaller >>> tailored >>> configuration would help Xen to meet faster certification >>> requirements for >>> individual platforms. >>> >>> Introduce two new Kconfig options, AMD_IOMMU and INTEL_VTD, to allow >>> code >>> specific to each IOMMU technology to be separated and, when not >>> required, >>> stripped. AMD_IOMMU enables IOMMU support for platforms that >>> implement the >>> AMD I/O Virtualization Technology. INTEL_VTD enables IOMMU support for >>> platforms that implement the Intel Virtualization Technology for >>> Directed I/O. >>> >>> Since no functional change is intended regarding the default >>> configuration >>> of an x86 system, both options depend on x86 and default to 'y'. >> >> But do things also build successfully when one or both options are >> disabled? >> I have to say that I would be quite surprised if that worked without >> further >> adjustments. In which case initially these options want to be >> prompt-less, >> with the prompts only added once 'n' also works. > > Without applying the whole series, disabling any of them or both won't > work. Ok. To do a multi-step implementation, you start with config FOO bool y then rearrange them main code to use CONFIG_FOO as appropriate, then have a final patch that adds a Kconfig name, help text, etc which is what makes the config option user selectable and able to be turned off. ~Andrew
Re: [RFC 1/7] x86/iommu: make AMD-Vi and Intel VT-d support configurable
On 12/20/22 19:01, Jan Beulich wrote: On 19.12.2022 07:34, Xenia Ragiadakou wrote: Currently, for x86 platforms, Xen does not provide to the users any configuration control over the IOMMU support and can only be built with both AMD and Intel IOMMU drivers enabled. However, there are use cases, e.g in safety-critical systems, that require Xen to be able to be configured to exclude unused code. A smaller tailored configuration would help Xen to meet faster certification requirements for individual platforms. Introduce two new Kconfig options, AMD_IOMMU and INTEL_VTD, to allow code specific to each IOMMU technology to be separated and, when not required, stripped. AMD_IOMMU enables IOMMU support for platforms that implement the AMD I/O Virtualization Technology. INTEL_VTD enables IOMMU support for platforms that implement the Intel Virtualization Technology for Directed I/O. Since no functional change is intended regarding the default configuration of an x86 system, both options depend on x86 and default to 'y'. But do things also build successfully when one or both options are disabled? I have to say that I would be quite surprised if that worked without further adjustments. In which case initially these options want to be prompt-less, with the prompts only added once 'n' also works. Without applying the whole series, disabling any of them or both won't work. Ok. Jan -- Xenia
Re: [PATCH v2-ish] x86/boot: Factor move_xen() out of __start_xen()
On 20/12/2022 1:51 pm, Jan Beulich wrote:
> On 16.12.2022 21:17, Andrew Cooper wrote:
>> Partly for clarity because there is a lot of subtle magic at work here.
>> Expand the commentary of what's going on.
>>
>> Also, because there is no need to double copy the stack (32kB) to retrieve
>> local values spilled to the stack under the old alias, when all of the
>> aforementioned local variables are going out of scope anyway.
>>
>> There is also a logic change when walking l2_xenmap[]. The old logic would
>> skip recursing into 4k mappings; this would corrupt Xen if it were used.
>> There are no 4k mappings in l2_xenmap[] this early on boot, so assert the
>> property instead of leaving a latent breakage.
>>
>> Signed-off-by: Andrew Cooper
>> ---
>> CC: Jan Beulich
>> CC: Roger Pau Monné
>> CC: Wei Liu
>>
>> We probably want to drop PGE from XEN_MINIMAL_CR4. It will simplify several
>> boot paths which don't need to care about global pages, and PGE is
>> conditional
>> anyway now with the PCID support.
> Perhaps, especially if - as you say - this allows for simplification.
>
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -467,6 +467,113 @@ static void __init move_memory(
>> }
>> }
>>
>> +static void __init noinline move_xen(void)
>> +{
>> +l4_pgentry_t *pl4e;
>> +l3_pgentry_t *pl3e;
>> +l2_pgentry_t *pl2e;
>> +unsigned long tmp;
>> +int i, j, k;
> Can these become "unsigned int" please at this occasion?
Fixed.
> (Perhaps as
> another reason to make the change, mention that the old instances of i and
> j did shadow outer scope variables in the same function?)
Oh, so it does. I'm slightly surprised that we haven't seen a compiler
objection from that.
>
>> +/*
>> + * The caller has selected xen_phys_start, ensuring that the old and new
>> + * locations do not overlap. Make it so.
> As a non-native speaker I'm struggling with what "Make it so" is supposed
> to tell me here.
"Actually do it"
I'll drop it. It's not overly important to the understanding here.
>
>> + * Prevent the compiler from reordering anything across this point.
>> Such
>> + * things will end badly.
>> + */
>> +barrier();
>> +
>> +/*
>> + * Copy out of the current alias, into the directmap at the new
>> location.
>> + * This includes a snapshot of the current stack.
>> + */
>> +memcpy(__va(__pa(_start)), _start, _end - _start);
>> +
>> +/*
>> + * We are now in a critical region. Any write (modifying a global,
>> + * spilling a local to the stack, etc) via the current alias will get
>> lost
>> + * when we switch to the new pagetables. This even means no printk()'s
>> + * debugging purposes.
> Nit: Missing "for"?
Fixed.
>
>> + * Walk the soon-to-be-used pagetables in the new location, relocating
>> all
>> + * intermediate (non-leaf) entries to point to their new-location
>> + * equivalents. All writes are via the directmap alias.
>> + */
>> +pl4e = __va(__pa(idle_pg_table));
>> +for ( i = 0 ; i < L4_PAGETABLE_ENTRIES; i++, pl4e++ )
>> +{
>> +if ( !(l4e_get_flags(*pl4e) & _PAGE_PRESENT) )
>> +continue;
>> +
>> +*pl4e = l4e_from_intpte(l4e_get_intpte(*pl4e) + xen_phys_start);
>> +pl3e = __va(l4e_get_paddr(*pl4e));
>> +for ( j = 0; j < L3_PAGETABLE_ENTRIES; j++, pl3e++ )
>> +{
>> +if ( !(l3e_get_flags(*pl3e) & _PAGE_PRESENT) ||
>> + (l3e_get_flags(*pl3e) & _PAGE_PSE) )
>> +continue;
>> +
>> +*pl3e = l3e_from_intpte(l3e_get_intpte(*pl3e) + xen_phys_start);
>> +pl2e = __va(l3e_get_paddr(*pl3e));
>> +for ( k = 0; k < L2_PAGETABLE_ENTRIES; k++, pl2e++ )
>> +{
>> +if ( !(l2e_get_flags(*pl2e) & _PAGE_PRESENT) ||
>> + (l2e_get_flags(*pl2e) & _PAGE_PSE) )
>> +continue;
>> +
>> +*pl2e = l2e_from_intpte(l2e_get_intpte(*pl2e) +
>> xen_phys_start);
>> +}
>> +}
>> +}
>> +
>> +/*
>> + * Walk the soon-to-be-used l2_xenmap[], relocating all the leaf
>> mappings
>> + * so text/data/bss etc refer to the new location in memory.
>> + */
>> +pl2e = __va(__pa(l2_xenmap));
>> +for ( i = 0; i < L2_PAGETABLE_ENTRIES; i++, pl2e++ )
>> +{
>> +if ( !(l2e_get_flags(*pl2e) & _PAGE_PRESENT) )
>> +continue;
>> +
>> +/*
>> + * We don't have 4k mappings in l2_xenmap[] at this point in boot,
>> nor
>> + * is this anticipated to change. Simply assert the fact, rather
>> than
>> + * introducing dead logic to decend into l1 tables.
> Nit: "descend"?
Fixed.
>
>> + */
>> +ASSERT(l2e_get_flags(*pl2e) & _PAGE_PSE);
> The warning about the use of printk() around here could make one think
> that using ASSERT() (or BUG()) is similarly bad. However, aiui using
> printk() isn't by itself a proble
Re: [RFC 2/7] x86/iommu: amd_iommu_perdev_intremap is AMD-Vi specific
On 12/20/22 12:31, Andrew Cooper wrote:
On 19/12/2022 6:34 am, Xenia Ragiadakou wrote:
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 5e2a720d29..1a02fdc453 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -58,7 +58,6 @@ bool __read_mostly iommu_hap_pt_share = true;
#endif
bool_t __read_mostly iommu_debug;
-bool_t __read_mostly amd_iommu_perdev_intremap = 1;
DEFINE_PER_CPU(bool_t, iommu_dont_flush_iotlb);
@@ -115,8 +114,10 @@ static int __init cf_check parse_iommu_param(const char *s)
if ( val )
iommu_verbose = 1;
}
+#ifdef CONFIG_AMD_IOMMU
else if ( (val = parse_boolean("amd-iommu-perdev-intremap", s, ss))
>= 0 )
amd_iommu_perdev_intremap = val;
+#endif
See parse_cet() and the use of no_config_param() so users get a bit of a
hint as to why the option they specified is getting ignored.
Ah, ok I see. Thx for pointing that out.
~Andrew
--
Xenia
Re: [RFC 1/7] x86/iommu: make AMD-Vi and Intel VT-d support configurable
On 12/20/22 12:26, Andrew Cooper wrote: On 19/12/2022 6:34 am, Xenia Ragiadakou wrote: diff --git a/xen/drivers/passthrough/Kconfig b/xen/drivers/passthrough/Kconfig index 479d7de57a..82465aa627 100644 --- a/xen/drivers/passthrough/Kconfig +++ b/xen/drivers/passthrough/Kconfig @@ -37,6 +37,22 @@ config IPMMU_VMSA endif +config AMD_IOMMU + bool "AMD IOMMU" + depends on X86 + default y + ---help--- We're trying to phase out ---help---, so please just use help. Ok. ~Andrew -- Xenia
Re: [RFC 0/7] Proposal to make x86 IOMMU driver support configurable
On 12/20/22 12:09, Jan Beulich wrote: On 19.12.2022 19:28, Andrew Cooper wrote: IOMMUs are more tricky still. They are (for most intents and purposes) mandatory on systems with >254 CPUs. We could in principle start supporting asymmetric IRQ routing on large systems, but Xen doesn't currently and it would be a lot work that's definitely not high on the priority list. At a minimum, this will need expressing in the Kconfig help text. We need to decide whether it is sensible to allow users to turn off IOMMU support. It probably is, because you could trivially do this by selecting CONFIG_INTEL only, and booting the result on an AMD system. One other thing Andrew and I have been talking about: We probably do not want to tie the IOMMU vendor control to CPU vendor one. IOW we'd like to be able to e.g. build a hypervisor supporting Intel (only) as the CPU vendor, but at the same time having support for an AMD IOMMU. I totally understand. I am not aiming to tie the AMD/INTEL IOMMU support to any particular CPU vendor. For the names, I don't think AMD_IOMMU vs INTEL_VTD is a sensible. Probably wants to be INTEL_IOMMU to match. Or simply VTD, covering the case than some other vendor comes up with a clone. But of course we have that same issue with "AMD" and Hygon ... I prefer INTEL_IOMMU over VTD, I think. Jan -- Xenia
Re: [RFC 0/7] Proposal to make x86 IOMMU driver support configurable
Hi, On 12/19/22 20:28, Andrew Cooper wrote: On 19/12/2022 6:34 am, Xenia Ragiadakou wrote: This series aims to provide a means to render the iommu driver support for x86 configurable. Currently, irrespectively of the target platform, both AMD and Intel iommu drivers are built. This is the case because the existent Kconfig infrastructure does not provide any facilities for finer-grained configuration. The series adds two new Kconfig options, AMD_IOMMU and INTEL_VTD, that can be used to generate a tailored iommu configuration for a given platform. This series will be part of a broader effort to separate platform specific code and it is sent as an RFC to gather feedback regarding the way the separation should be performed. Hello, Thanks for the series. From discussions in the past, we do want CONFIG_INTEL/AMD/etc and we do want these to be selectable and available for randconfig to test. Some sub-features are more complicated, because e.g. Centaur have CPUs with a VT-x implementation. This will need expressing in whatever Kconfig scheme we end up with. What about having configuration per cpu vendor, per virtualization technology and per IOMMU technology? I mean sth like [CPU_AMD, CPU_HYGON, CPU_INTEL, CPU_SHANGHAI, CPU_CENTAUR], [AMD_SVM, INTEL_VMX] and [AMD_IOMMU, INTEL_IOMMU], respectively? IOMMUs are more tricky still. They are (for most intents and purposes) mandatory on systems with >254 CPUs. We could in principle start supporting asymmetric IRQ routing on large systems, but Xen doesn't currently and it would be a lot work that's definitely not high on the priority list. At a minimum, this will need expressing in the Kconfig help text. Ok. We need to decide whether it is sensible to allow users to turn off IOMMU support. It probably is, because you could trivially do this by selecting CONFIG_INTEL only, and booting the result on an AMD system. I cannot understand. I guess that if the code for the target system is disabled, Xen won't boot ... hopefully :). If users are not allowed to turn off the IOMMU support, it can be always enabled unconditionally via Kconfig select based on the virtualization technology or other. For the names, I don't think AMD_IOMMU vs INTEL_VTD is a sensible. Probably wants to be INTEL_IOMMU to match. Ok. ~Andrew -- Xenia
[xen-unstable-smoke test] 175428: tolerable all pass - PUSHED
flight 175428 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/175428/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass version targeted for testing: xen 4b40d68e663e87eb916b5bdf3da9743a17f43537 baseline version: xen 0fc5fa9333b21122c6e77fa42f5683e31c81bbe5 Last test of basis 175422 2022-12-20 10:02:02 Z0 days Testing same since 175428 2022-12-20 16:00:30 Z0 days1 attempts People who touched revisions under test: Demi Marie Obenour Jan Beulich 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 0fc5fa9333..4b40d68e66 4b40d68e663e87eb916b5bdf3da9743a17f43537 -> smoke
Re: [PATCH v2 11/19] tools/xenstore: don't allow creating too many nodes in a transaction
Hi, On 13/12/2022 16:00, Juergen Gross wrote: The accounting for the number of nodes of a domain in an active transaction is not working correctly, as it allows to create arbitrary number of nodes. The transaction will finally fail due to exceeding the number of nodes quota, but before closing the transaction an unprivileged guest could cause Xenstore to use a lot of memory. As per the discussion in v1, the commit message needs to be reworded. I will look at this patch in more details once I have reached the 2nd series. Cheers, -- Julien Grall
Re: [PATCH v2 10/19] tools/xenstore: change per-domain node accounting interface
Hi,
On 13/12/2022 16:00, Juergen Gross wrote:
Rework the interface and the internals of the per-domain node
accounting:
- rename the functions to domain_nbentry_*() in order to better match
the related counter name
- switch from node pointer to domid as interface, as all nodes have the
owner filled in
The downside is now you have may place open-coding
"...->perms->p[0].id". IHMO this is making the code more complicated. So
can you introduce a few wrappers that would take a node and then convert
to the owner?
- use a common internal function for adding a value to the counter
For the transaction case add a helper function to get the list head
of the per-transaction changed domains, enabling to eliminate the
transaction_entry_*() functions.
Signed-off-by: Juergen Gross
---
tools/xenstore/xenstored_core.c| 22 ++---
tools/xenstore/xenstored_domain.c | 122 +++--
tools/xenstore/xenstored_domain.h | 10 +-
tools/xenstore/xenstored_transaction.c | 15 +--
tools/xenstore/xenstored_transaction.h | 7 +-
5 files changed, 72 insertions(+), 104 deletions(-)
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index f96714e1b8..61569cecbb 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -1459,7 +1459,7 @@ static void destroy_node_rm(struct connection *conn,
struct node *node)
static int destroy_node(struct connection *conn, struct node *node)
{
destroy_node_rm(conn, node);
- domain_entry_dec(conn, node);
+ domain_nbentry_dec(conn, node->perms.p[0].id);
/*
* It is not possible to easily revert the changes in a transaction.
@@ -1498,7 +1498,7 @@ static struct node *create_node(struct connection *conn,
const void *ctx,
for (i = node; i; i = i->parent) {
/* i->parent is set for each new node, so check quota. */
if (i->parent &&
- domain_entry(conn) >= quota_nb_entry_per_domain) {
+ domain_nbentry(conn) >= quota_nb_entry_per_domain) {
ret = ENOSPC;
goto err;
}
@@ -1509,7 +1509,7 @@ static struct node *create_node(struct connection *conn,
const void *ctx,
/* Account for new node */
if (i->parent) {
- if (domain_entry_inc(conn, i)) {
+ if (domain_nbentry_inc(conn, i->perms.p[0].id)) {
destroy_node_rm(conn, i);
return NULL;
}
@@ -1662,7 +1662,7 @@ static int delnode_sub(const void *ctx, struct connection
*conn,
watch_exact = strcmp(root, node->name);
fire_watches(conn, ctx, node->name, node, watch_exact, NULL);
- domain_entry_dec(conn, node);
+ domain_nbentry_dec(conn, node->perms.p[0].id);
return WALK_TREE_RM_CHILDENTRY;
}
@@ -1802,25 +1802,25 @@ static int do_set_perms(const void *ctx, struct
connection *conn,
return EPERM;
old_perms = node->perms;
- domain_entry_dec(conn, node);
+ domain_nbentry_dec(conn, node->perms.p[0].id);
node->perms = perms;
- if (domain_entry_inc(conn, node)) {
+ if (domain_nbentry_inc(conn, node->perms.p[0].id)) {
node->perms = old_perms;
/*
* This should never fail because we had a reference on the
* domain before and Xenstored is single-threaded.
*/
- domain_entry_inc(conn, node);
+ domain_nbentry_inc(conn, node->perms.p[0].id);
return ENOMEM;
}
if (write_node(conn, node, false)) {
int saved_errno = errno;
- domain_entry_dec(conn, node);
+ domain_nbentry_dec(conn, node->perms.p[0].id);
node->perms = old_perms;
/* No failure possible as above. */
- domain_entry_inc(conn, node);
+ domain_nbentry_inc(conn, node->perms.p[0].id);
errno = saved_errno;
return errno;
@@ -2392,7 +2392,7 @@ void setup_structure(bool live_update)
manual_node("/tool/xenstored", NULL);
manual_node("@releaseDomain", NULL);
manual_node("@introduceDomain", NULL);
- domain_entry_fix(dom0_domid, 5, true);
+ domain_nbentry_fix(dom0_domid, 5, true);
}
check_store();
@@ -3400,7 +3400,7 @@ void read_state_node(const void *ctx, const void *state)
if (write_node_raw(NULL, &key, node, true))
barf("write node error restoring node");
- if (domain_entry_inc(&conn, node))
+ if (domain_nbentry_inc(&conn, node->perms.p[0].id))
barf("node accounting error restoring node");
talloc_free(node);
diff --git a/tools/xenstore/xenstored_domain.c
b/tools/xenstore/xenstored_domain.c
index 3216119
Re: [PATCH v2 09/19] tools/xenstore: move changed domain handling
Hi Juergen, On 13/12/2022 16:00, Juergen Gross wrote: Move all code related to struct changed_domain from xenstored_transaction.c to xenstored_domain.c. This will be needed later in order to simplify the accounting data updates in cases of errors during a request. Split the code to have a more generic base framework. Signed-off-by: Juergen Gross Reviewed-by: Julien Grall Cheers, -- Julien Grall
Re: [PATCH v2 08/19] tools/xenstore: replace watch->relative_path with a prefix length
Hi Juergen,
On 13/12/2022 16:00, Juergen Gross wrote:
@@ -313,19 +302,19 @@ const char *dump_state_watches(FILE *fp, struct
connection *conn,
unsigned int conn_id)
{
const char *ret = NULL;
+ const char *watch_path;
struct watch *watch;
struct xs_state_watch sw;
struct xs_state_record_header head;
- const char *path;
head.type = XS_STATE_TYPE_WATCH;
list_for_each_entry(watch, &conn->watches, list) {
head.length = sizeof(sw);
+ watch_path = get_watch_path(watch, watch->node);
It is not clear to me why you call get_watch_path() earlier and also
rename the variable.
I don't mind the new name, but it doesn't feel like it belongs to this
patch as the code in duymp_state_watches() would not be changed otherwise.
Cheers,
sw.conn_id = conn_id;
- path = get_watch_path(watch, watch->node);
- sw.path_length = strlen(path) + 1;
+ sw.path_length = strlen(watch_path) + 1;
sw.token_length = strlen(watch->token) + 1;
head.length += sw.path_length + sw.token_length;
head.length = ROUNDUP(head.length, 3);
@@ -334,7 +323,7 @@ const char *dump_state_watches(FILE *fp, struct connection
*conn,
if (fwrite(&sw, sizeof(sw), 1, fp) != 1)
return "Dump watch state error";
- if (fwrite(path, sw.path_length, 1, fp) != 1)
+ if (fwrite(watch_path, sw.path_length, 1, fp) != 1)
return "Dump watch path error";
if (fwrite(watch->token, sw.token_length, 1, fp) != 1)
return "Dump watch token error";
Cheers,
--
Julien Grall
Re: [PATCH v2 07/19] tools/xenstore: introduce dummy nodes for special watch paths
Hi Juergen,
On 13/12/2022 16:00, Juergen Gross wrote:
Instead of special casing the permission handling and watch event
firing for the special watch paths "@introduceDomain" and
"@releaseDomain", use static dummy nodes added to the data base when
starting Xenstore.
The node accounting needs to reflect that change by adding the special
nodes in the domain_entry_fix() call in setup_structure().
Note that this requires to rework the calls of fire_watches() for the
special events in order to avoid leaking memory.
Move the check for a valid node name from get_node() to
get_node_canonicalized(), as it allows to use get_node() for the
special nodes, too.
In order to avoid read and write accesses to the special nodes use a
special variant for obtaining the current node data for the permission
handling.
This allows to simplify quite some code. In future sub-nodes of the
special nodes will be possible due to this change, allowing more fine
grained permission control of special events for specific domains.
Signed-off-by: Juergen Gross
---
V2:
- add get_spec_node()
- expand commit message (Julien Grall)
---
tools/xenstore/xenstored_control.c | 3 -
tools/xenstore/xenstored_core.c| 92 +---
tools/xenstore/xenstored_domain.c | 162 -
tools/xenstore/xenstored_domain.h | 6 --
tools/xenstore/xenstored_watch.c | 17 +--
5 files changed, 80 insertions(+), 200 deletions(-)
diff --git a/tools/xenstore/xenstored_control.c
b/tools/xenstore/xenstored_control.c
index d1aaa00bf4..41e6992591 100644
--- a/tools/xenstore/xenstored_control.c
+++ b/tools/xenstore/xenstored_control.c
@@ -676,9 +676,6 @@ static const char *lu_dump_state(const void *ctx, struct
connection *conn)
if (ret)
goto out;
ret = dump_state_connections(fp);
- if (ret)
- goto out;
- ret = dump_state_special_nodes(fp);
if (ret)
goto out;
ret = dump_state_nodes(fp, ctx);
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 1650821922..f96714e1b8 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -616,7 +616,8 @@ static void get_acc_data(TDB_DATA *key, struct
node_account_data *acc)
static unsigned int get_acc_domid(struct connection *conn, TDB_DATA *key,
unsigned int domid)
{
- return (!conn || key->dptr[0] == '/') ? domid : conn->id;
+ return (!conn || key->dptr[0] == '/' || key->dptr[0] == '@')
The comment on top of get_acc_domid() needs to be updated.
+ ? domid : conn->id;
}
[...]
+static void fire_special_watches(const char *name)
+{
+ void *ctx = talloc_new(NULL);
+ struct node *node;
+
+ if (!ctx)
+ return;
+
+ node = read_node(NULL, ctx, name);
+
+ if (node)
+ fire_watches(NULL, ctx, name, node, true, NULL);
NIT: I would consider to log an error (maybe only once) if 'node' is
NULL. The purpose is to help debugging Xenstored.
The rest of the code LGTM.
Cheers,
--
Julien Grall
Re: [PATCH v2 3/3] x86/ucode: load microcode earlier on boot CPU
On 20/12/2022 3:18 pm, Jan Beulich wrote: > On 20.12.2022 15:50, Andrew Cooper wrote: >> On 19/12/2022 2:45 pm, Sergey Dyasli wrote: >>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c >>> index 6bb5bc7c84..2d7c815e0a 100644 >>> --- a/xen/arch/x86/setup.c >>> +++ b/xen/arch/x86/setup.c >>> relocated = true; >>> @@ -1762,11 +1768,9 @@ void __init noreturn __start_xen(unsigned long mbi_p) >>> >>> init_IRQ(); >>> >>> -microcode_grab_module(module_map, mbi); >>> - >>> timer_init(); >>> >>> -early_microcode_init(); >>> +early_microcode_init_cache(module_map, mbi); >> microcode_init_cache(module_map, mbi); /* Needs xmalloc() */ >> >> Can fix on commit. > Are you merely after the added comment, or is the omission of the early_ > prefix also meaningful in some way? This isn't "early_microcode" and frankly wasn't "early" to begin with. Caching the blob can happen at any time after the heap is set up, so should not have anything like "early" in its name. The comment is just to make it easier in the future to figure out how to rearrange __start_xen(). ~Andrew
Re: [PATCH v2 06/19] tools/xenstore: add hashlist for finding struct domain by domid
Hi Juergen, On 13/12/2022 16:00, Juergen Gross wrote: Today finding a struct domain by its domain id requires to scan the list of domains until finding the correct domid. Add a hashlist for being able to speed this up. This allows to remove the linking of struct domain in a list. Note that the list of changed domains per transaction is kept as a list, as there are no known use cases with more than 4 domains being touched in a single transaction (this would be a device handled by a driver domain and being assigned to a HVM domain with device model in a stubdom, plus the control domain). Some simple performance tests comparing the scanning and hashlist have shown that the hashlist will win as soon as more than 6 entries need to be scanned. Signed-off-by: Juergen Gross Reviewed-by: Julien Grall Cheers, -- Julien Grall
Re: [PATCH v2 04/19] tools/xenstore: remove all watches when a domain has stopped
Hi Juergen, On 13/12/2022 16:00, Juergen Gross wrote: When a domain has been released by Xen tools, remove all its registered watches. This avoids sending watch events to the dead domain when all the nodes related to it are being removed by the Xen tools. AFAICT, the only user of the command in the tree is softreset. Would you be able to check this is still working as expected? Signed-off-by: Juergen Gross --- V2: - move call to do_release() (Julien Grall) --- tools/xenstore/xenstored_domain.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c index aa86892fed..e669c89e94 100644 --- a/tools/xenstore/xenstored_domain.c +++ b/tools/xenstore/xenstored_domain.c @@ -740,6 +740,9 @@ int do_release(const void *ctx, struct connection *conn, if (IS_ERR(domain)) return -PTR_ERR(domain); + /* Avoid triggering watch events when the domain's nodes are deleted. */ + conn_delete_all_watches(domain->conn); + talloc_free(domain->conn); send_ack(conn, XS_RELEASE); Cheers, -- Julien Grall
Re: [PATCH v5 11/10] hvmloader: use memory type constants
On Tue, Dec 20, 2022 at 05:13:04PM +0100, Jan Beulich wrote:
> Now that we have them available in a header which is okay to use from
> hvmloader sources, do away with respective literal numbers and silent
> assumptions.
>
> Signed-off-by: Jan Beulich
>
> --- a/tools/firmware/hvmloader/cacheattr.c
> +++ b/tools/firmware/hvmloader/cacheattr.c
> @@ -22,6 +22,8 @@
> #include "util.h"
> #include "config.h"
>
> +#include
> +
> #define MSR_MTRRphysBase(reg) (0x200 + 2 * (reg))
> #define MSR_MTRRphysMask(reg) (0x200 + 2 * (reg) + 1)
> #define MSR_MTRRcap 0x00fe
> @@ -71,23 +73,32 @@ void cacheattr_init(void)
>
> addr_mask = ((1ull << phys_bits) - 1) & ~((1ull << 12) - 1);
> mtrr_cap = rdmsr(MSR_MTRRcap);
> -mtrr_def = (1u << 11) | 6; /* E, default type WB */
> +mtrr_def = (1u << 11) | X86_MT_WB; /* E, default type WB */
>
> /* Fixed-range MTRRs supported? */
> if ( mtrr_cap & (1u << 8) )
> {
> +#define BCST2(mt) ((mt) | ((mt) << 8))
> +#define BCST4(mt) (BCST2(mt) | (BCST2(mt) << 16))
This should include a cast to uint32_t, just like BCST8 includes a cast
to uint64_t. It doesn’t have any functional impact since none of the
memory types have the high bit set, but it makes the correctness of the
code much more obvious to readers.
> +#define BCST8(mt) (BCST4(mt) | ((uint64_t)BCST4(mt) << 32))
> /* 0x0-0x9: Write Back (WB) */
> -content = 0x0606060606060606ull;
> +content = BCST8(X86_MT_WB);
> wrmsr(MSR_MTRRfix64K_0, content);
> wrmsr(MSR_MTRRfix16K_8, content);
> +
> /* 0xa-0xb: Write Combining (WC) */
> if ( mtrr_cap & (1u << 10) ) /* WC supported? */
> -content = 0x0101010101010101ull;
> +content = BCST8(X86_MT_WC);
> wrmsr(MSR_MTRRfix16K_A, content);
> +
> /* 0xc-0xf: Write Back (WB) */
> -content = 0x0606060606060606ull;
> +content = BCST8(X86_MT_WB);
> for ( i = 0; i < 8; i++ )
> wrmsr(MSR_MTRRfix4K_C + i, content);
> +#undef BCST8
> +#undef BCST4
> +#undef BCST2
> +
> mtrr_def |= 1u << 10; /* FE */
> printf("fixed MTRRs ... ");
> }
> @@ -106,7 +117,7 @@ void cacheattr_init(void)
> while ( ((base + size) < base) || ((base + size) > pci_mem_end) )
> size >>= 1;
>
> -wrmsr(MSR_MTRRphysBase(i), base);
> +wrmsr(MSR_MTRRphysBase(i), base | X86_MT_UC);
> wrmsr(MSR_MTRRphysMask(i), (~(size - 1) & addr_mask) | (1u <<
> 11));
>
> base += size;
> @@ -121,7 +132,7 @@ void cacheattr_init(void)
> while ( (base + size < base) || (base + size > pci_hi_mem_end) )
> size >>= 1;
>
> -wrmsr(MSR_MTRRphysBase(i), base);
> +wrmsr(MSR_MTRRphysBase(i), base | X86_MT_UC);
> wrmsr(MSR_MTRRphysMask(i), (~(size - 1) & addr_mask) | (1u <<
> 11));
>
> base += size;
>
With the suggested change:
Acked-by: Demi Marie Obenour
--
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab
signature.asc
Description: PGP signature
Re: [XEN v3] xen/arm: Probe the entry point address of an uImage correctly
Hi Julien,
I need a clarification.
On 20/12/2022 12:51, Ayan Kumar Halder wrote:
On 20/12/2022 12:14, Julien Grall wrote:
Hi Ayan,
Hi Julien,
On 20/12/2022 10:44, Ayan Kumar Halder wrote:
+
+ /*
+ * Currently, we support uImage headers for uncompressed
images only.
+ * Thus, it is valid for the load address and start address
to be the
+ * same. This is consistent with the uboot behavior (Refer
+ * "case IH_COMP_NONE" in image_decomp().
Please make it clear that you are referring to uboot function.
Could we avoid to mention the u-boot function? The reason I am
asking is the code is in a different repo and the function name can
easily change without us noticing (so the comment would rot).
Is the behavior documented somewhere in U-boot (or online)? If not,
what's the guarantee that it will not change?
I could not find any documentation which states this. I found this
from the following in uboot source code.
https://source.denx.de/u-boot/u-boot/-/blob/master/boot/image.c#L458
AFAIU when kernel_uimage_probe() get invoked, the image has already
been decompressed. So, I added this as a limitation.
I e looked at the U-boot code and, AFAIU, the check is merely to
avoid the memcpy() if the image start matches the start of the
decompression area. So I don't understand why we need the limitation
in Xen.
Now the lack of documentation (or at least I didn't find any) makes a
bit more tricky to understand what the fields in the U-boot header
are for. I have skimmed through the code and I disagree with the
implementation you chose for Xen.
The comment for 'ih_ep' suggests this is the entry point address.
That's may be different from the beginning of your blob. I think this
is what ih_load is for.
So I think we want to load the blob at ih_load and then set pc to
point to ih_ep. This will require a bit more work in Xen because the
assumption so far is the entry point matches the start of the blob.
Please let me known if I missed anything.
I think you are correct. I will rework this to correctly handle load
address and entry point.
Is it fine to keep these two constraints ?
/*
* While uboot considers 0x0 to be a valid load/start address, for Xen
* to mantain parity with zimage, we consider 0x0 to denote position
* independent image. That means Xen is free to load such an image at
* any valid address.
* Thus, we will print an appropriate message.
*/
if ( info->zimage.start == 0 )
printk(XENLOG_INFO
"No load address provided. Xen will decide where to load
it.\n");
/*
* If the image supports position independent execution, then user
cannot
* provide an entry point as Xen will load such an image at any
appropriate
* memory address. Thus, we need to return error
*/
if ( (info->zimage.start == 0) && (info->entry != 0) )
{
printk(XENLOG_ERROR
"Entry point cannot be non zero for PIE image.\n");
return -EINVAL;
}
- Ayan
- Ayan
Cheers,
Re: [RFC 2/7] x86/iommu: amd_iommu_perdev_intremap is AMD-Vi specific
On 19.12.2022 07:34, Xenia Ragiadakou wrote: > Move its definition to the AMD-Vi driver and use CONFIG_AMD_IOMMU > to guard its usage in common code. > > Take the opportunity to replace bool_t with bool and 1 with true. Please further take the opportunity and use ... > --- a/xen/drivers/passthrough/amd/iommu_init.c > +++ b/xen/drivers/passthrough/amd/iommu_init.c > @@ -36,6 +36,8 @@ static struct radix_tree_root ivrs_maps; > LIST_HEAD_READ_MOSTLY(amd_iommu_head); > bool_t iommuv2_enabled; > > +bool __read_mostly amd_iommu_perdev_intremap = true; ... __ro_after_init here. > --- a/xen/include/xen/iommu.h > +++ b/xen/include/xen/iommu.h > @@ -104,7 +104,9 @@ static inline void clear_iommu_hap_pt_share(void) > } > > extern bool_t iommu_debug; > -extern bool_t amd_iommu_perdev_intremap; > +#ifdef CONFIG_AMD_IOMMU > +extern bool amd_iommu_perdev_intremap; > +#endif We try to avoid such #ifdef-ary: There's no real harm from the declaration being in scope; in the worst case the build will fail not at the compile, but at the linking stage. Jan
Re: [RFC 1/7] x86/iommu: make AMD-Vi and Intel VT-d support configurable
On 19.12.2022 07:34, Xenia Ragiadakou wrote: > Currently, for x86 platforms, Xen does not provide to the users any > configuration control over the IOMMU support and can only be built with > both AMD and Intel IOMMU drivers enabled. > However, there are use cases, e.g in safety-critical systems, that require > Xen to be able to be configured to exclude unused code. A smaller tailored > configuration would help Xen to meet faster certification requirements for > individual platforms. > > Introduce two new Kconfig options, AMD_IOMMU and INTEL_VTD, to allow code > specific to each IOMMU technology to be separated and, when not required, > stripped. AMD_IOMMU enables IOMMU support for platforms that implement the > AMD I/O Virtualization Technology. INTEL_VTD enables IOMMU support for > platforms that implement the Intel Virtualization Technology for Directed I/O. > > Since no functional change is intended regarding the default configuration > of an x86 system, both options depend on x86 and default to 'y'. But do things also build successfully when one or both options are disabled? I have to say that I would be quite surprised if that worked without further adjustments. In which case initially these options want to be prompt-less, with the prompts only added once 'n' also works. Jan
Re: [PATCH v5 11/10] hvmloader: use memory type constants
On 20.12.2022 17:36, Andrew Cooper wrote:
> On 20/12/2022 4:13 pm, Jan Beulich wrote:
>> --- a/tools/firmware/hvmloader/cacheattr.c
>> +++ b/tools/firmware/hvmloader/cacheattr.c
>> @@ -22,6 +22,8 @@
>> #include "util.h"
>> #include "config.h"
>>
>> +#include
>> +
>> #define MSR_MTRRphysBase(reg) (0x200 + 2 * (reg))
>> #define MSR_MTRRphysMask(reg) (0x200 + 2 * (reg) + 1)
>> #define MSR_MTRRcap 0x00fe
>> @@ -71,23 +73,32 @@ void cacheattr_init(void)
>>
>> addr_mask = ((1ull << phys_bits) - 1) & ~((1ull << 12) - 1);
>
> Does this want to be PAGE_SHIFT ?
Not really (it's rather an MTRR layout related constant which we ought
to use here, which only happens to match PAGE_{SIZE,SHIFT}) and not in
this patch (see the title).
>> mtrr_cap = rdmsr(MSR_MTRRcap);
>> -mtrr_def = (1u << 11) | 6; /* E, default type WB */
>> +mtrr_def = (1u << 11) | X86_MT_WB; /* E, default type WB */
>>
>> /* Fixed-range MTRRs supported? */
>> if ( mtrr_cap & (1u << 8) )
>> {
>> +#define BCST2(mt) ((mt) | ((mt) << 8))
>> +#define BCST4(mt) (BCST2(mt) | (BCST2(mt) << 16))
>> +#define BCST8(mt) (BCST4(mt) | ((uint64_t)BCST4(mt) << 32))
>
> IMO this is clearer as
>
> #define BCST8(mt) ((mt) * 0x0101010101010101ULL)
>
> which saves several macros.
Ah, yes, will switch (and then name the thing just BCST()).
>> /* 0x0-0x9: Write Back (WB) */
>> -content = 0x0606060606060606ull;
>> +content = BCST8(X86_MT_WB);
>> wrmsr(MSR_MTRRfix64K_0, content);
>> wrmsr(MSR_MTRRfix16K_8, content);
>> +
>> /* 0xa-0xb: Write Combining (WC) */
>> if ( mtrr_cap & (1u << 10) ) /* WC supported? */
>> -content = 0x0101010101010101ull;
>> +content = BCST8(X86_MT_WC);
>> wrmsr(MSR_MTRRfix16K_A, content);
>
> This looks like it's latently buggy. We'll end up with WB if WC isn't
> supported, when it ought to be UC.
>
> I suppose it doesn't actually matter because if there is a VGA region
> there, it will actually be holes in the P2M and trap for emulation.
>
> Also, Xen being 64-bit, WC is always available.
Right, but we (or the admin) may elect to not surface availability to
the guest.
>> @@ -106,7 +117,7 @@ void cacheattr_init(void)
>> while ( ((base + size) < base) || ((base + size) > pci_mem_end)
>> )
>> size >>= 1;
>>
>> -wrmsr(MSR_MTRRphysBase(i), base);
>> +wrmsr(MSR_MTRRphysBase(i), base | X86_MT_UC);
>> wrmsr(MSR_MTRRphysMask(i), (~(size - 1) & addr_mask) | (1u <<
>> 11));
>
> If you're re-spinning for page_size or the macros, any chance of also
> gaining some constants for E/FE to avoid these naked shifts?
Again, yes in principle, but not in this patch. This requires shuffling
more stuff into x86-defns.h first.
Jan
[qemu-mainline test] 175417: regressions - trouble: broken/fail/pass
flight 175417 qemu-mainline real [real] http://logs.test-lab.xenproject.org/osstest/logs/175417/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-amd64-amd64-xl-qemuu-ovmf-amd64 broken test-amd64-amd64-xl-qemuu-debianhvm-amd64-shadowbroken test-amd64-amd64-xl-qemuu-debianhvm-amd64 broken test-amd64-amd64-xl broken test-amd64-amd64-xl-qemuu-debianhvm-amd64 5 host-install(5) broken REGR. vs. 175394 test-amd64-amd64-xl 5 host-install(5)broken REGR. vs. 175394 test-amd64-amd64-xl-qemuu-ovmf-amd64 5 host-install(5) broken REGR. vs. 175394 test-amd64-amd64-xl-qemuu-debianhvm-amd64-shadow 5 host-install(5) broken REGR. vs. 175394 test-amd64-i386-qemuu-rhel6hvm-amd 7 xen-installfail REGR. vs. 175394 test-armhf-armhf-xl-multivcpu 18 guest-start/debian.repeat fail REGR. vs. 175394 Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 175394 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 175394 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 175394 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 175394 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 175394 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 175394 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 175394 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 175394 test-amd64-i386-xl-pvshim14 guest-start fail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-i386-libvirt 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-xl 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 16 saverestore-support-checkfail never pass test-amd64-i386-libvirt-raw 14 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-xl-rtds 15 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 15 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 15 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 16 savere
Re: [PATCH v5 11/10] hvmloader: use memory type constants
On 20/12/2022 4:13 pm, Jan Beulich wrote:
> Now that we have them available in a header which is okay to use from
> hvmloader sources, do away with respective literal numbers and silent
> assumptions.
>
> Signed-off-by: Jan Beulich
>
> --- a/tools/firmware/hvmloader/cacheattr.c
> +++ b/tools/firmware/hvmloader/cacheattr.c
> @@ -22,6 +22,8 @@
> #include "util.h"
> #include "config.h"
>
> +#include
> +
> #define MSR_MTRRphysBase(reg) (0x200 + 2 * (reg))
> #define MSR_MTRRphysMask(reg) (0x200 + 2 * (reg) + 1)
> #define MSR_MTRRcap 0x00fe
> @@ -71,23 +73,32 @@ void cacheattr_init(void)
>
> addr_mask = ((1ull << phys_bits) - 1) & ~((1ull << 12) - 1);
Does this want to be PAGE_SHIFT ?
> mtrr_cap = rdmsr(MSR_MTRRcap);
> -mtrr_def = (1u << 11) | 6; /* E, default type WB */
> +mtrr_def = (1u << 11) | X86_MT_WB; /* E, default type WB */
>
> /* Fixed-range MTRRs supported? */
> if ( mtrr_cap & (1u << 8) )
> {
> +#define BCST2(mt) ((mt) | ((mt) << 8))
> +#define BCST4(mt) (BCST2(mt) | (BCST2(mt) << 16))
> +#define BCST8(mt) (BCST4(mt) | ((uint64_t)BCST4(mt) << 32))
IMO this is clearer as
#define BCST8(mt) ((mt) * 0x0101010101010101ULL)
which saves several macros.
> /* 0x0-0x9: Write Back (WB) */
> -content = 0x0606060606060606ull;
> +content = BCST8(X86_MT_WB);
> wrmsr(MSR_MTRRfix64K_0, content);
> wrmsr(MSR_MTRRfix16K_8, content);
> +
> /* 0xa-0xb: Write Combining (WC) */
> if ( mtrr_cap & (1u << 10) ) /* WC supported? */
> -content = 0x0101010101010101ull;
> +content = BCST8(X86_MT_WC);
> wrmsr(MSR_MTRRfix16K_A, content);
This looks like it's latently buggy. We'll end up with WB if WC isn't
supported, when it ought to be UC.
I suppose it doesn't actually matter because if there is a VGA region
there, it will actually be holes in the P2M and trap for emulation.
Also, Xen being 64-bit, WC is always available.
> +
> /* 0xc-0xf: Write Back (WB) */
> -content = 0x0606060606060606ull;
> +content = BCST8(X86_MT_WB);
> for ( i = 0; i < 8; i++ )
> wrmsr(MSR_MTRRfix4K_C + i, content);
> +#undef BCST8
> +#undef BCST4
> +#undef BCST2
> +
> mtrr_def |= 1u << 10; /* FE */
> printf("fixed MTRRs ... ");
> }
> @@ -106,7 +117,7 @@ void cacheattr_init(void)
> while ( ((base + size) < base) || ((base + size) > pci_mem_end) )
> size >>= 1;
>
> -wrmsr(MSR_MTRRphysBase(i), base);
> +wrmsr(MSR_MTRRphysBase(i), base | X86_MT_UC);
> wrmsr(MSR_MTRRphysMask(i), (~(size - 1) & addr_mask) | (1u <<
> 11));
If you're re-spinning for page_size or the macros, any chance of also
gaining some constants for E/FE to avoid these naked shifts?
~Andrew
>
> base += size;
> @@ -121,7 +132,7 @@ void cacheattr_init(void)
> while ( (base + size < base) || (base + size > pci_hi_mem_end) )
> size >>= 1;
>
> -wrmsr(MSR_MTRRphysBase(i), base);
> +wrmsr(MSR_MTRRphysBase(i), base | X86_MT_UC);
> wrmsr(MSR_MTRRphysMask(i), (~(size - 1) & addr_mask) | (1u <<
> 11));
>
> base += size;
>
>
Re: [XEN v1 8/9] xen/arm: Other adaptations required to support 32bit paddr
Hi,
On 20/12/2022 15:24, Ayan Kumar Halder wrote:
On 16/12/2022 10:23, Julien Grall wrote:
Each adaptations are distinct, so I would prefer if they are in in
separate patch.
This will also make clear which components you touched because I would
be surprised if this is really the only place where we need
adaptation. Maybe that's because you didn't compile everything (which
is fine).
On 15/12/2022 19:32, Ayan Kumar Halder wrote:
1. Supersections are supported only for paddr greater than 32 bits.
Two questions:
* Can you outline why we can't keep the code around?
For PA_32, the following bitoperation will overflow :-
*ipa |= (paddr_t)(pte.supersec.extbase1) <<
L1DESC_SUPERSECTION_EXT_BASE1_SHIFT;
*ipa |= (paddr_t)(pte.supersec.extbase2) <<
L1DESC_SUPERSECTION_EXT_BASE2_SHIFT;
Also, pte.supersec.extbase1 and pte.supersec.extbase2 are not valid for
PA_32. Refer xen/arch/arm/include/asm/short-desc.h
You are right about extbase1 and extbase2. See below for the
supersection support.
unsigned int extbase2:4; /* Extended base address, PA[39:36] */
unsigned int extbase1:4; /* Extended base address, PA[35:32] */
* Can you give a pointer to the Arm Arm that says supersection is not
supported?
I could not find any sentence in Arm Arm which says supersection is
**not** supported on 32 bit PA.
However,
Refer"ARM DDI 0487I.a ID081822", G5.4 "The VMSAv8-32 Short-descriptor
translation table format", G5-9163
"Support for Supersections is **optional**, except that an
implementation that supports more than 32 bits of PA must also support
Supersections to provide access to the entire PA space."
Also, G5.1.3 "Address spaces in VMSAv8-32", G5-9149
"AArch32 defines two translation table formats. The Long-descriptor
format gives access to the full 40-bit IPA or PA space at a granularity
of 4KB. The Short-descriptor format:
Gives access to a 32-bit PA space at 4KB granularity.
Gives access to a 40-bit PA space, but only at 16MB granularity, by
the use of Supersections."
from the above 2 snippets, I inferred that supersections are only
supported for PAs greater than 32 bits.
I could not find any evidence of supersections supported for 32 bit PA.
From what you quoted above supersection is optional unless the
processor support more than 32-bit PA. IOW, an implementer is free to
implement the feature even if it is not strictly necessary when the
processor only supports 32-bit PA. This because it is useful to reduce
the TLB contention.
So if we want to #ifdef some code, then only the two lines using
L1DESC_SUPERSECTION_EXT_BASE{1, 2}_SHIFT should be protected. The rest
should stay as-is.
Cheers,
--
Julien Grall
[PATCH v5 11/10] hvmloader: use memory type constants
Now that we have them available in a header which is okay to use from
hvmloader sources, do away with respective literal numbers and silent
assumptions.
Signed-off-by: Jan Beulich
--- a/tools/firmware/hvmloader/cacheattr.c
+++ b/tools/firmware/hvmloader/cacheattr.c
@@ -22,6 +22,8 @@
#include "util.h"
#include "config.h"
+#include
+
#define MSR_MTRRphysBase(reg) (0x200 + 2 * (reg))
#define MSR_MTRRphysMask(reg) (0x200 + 2 * (reg) + 1)
#define MSR_MTRRcap 0x00fe
@@ -71,23 +73,32 @@ void cacheattr_init(void)
addr_mask = ((1ull << phys_bits) - 1) & ~((1ull << 12) - 1);
mtrr_cap = rdmsr(MSR_MTRRcap);
-mtrr_def = (1u << 11) | 6; /* E, default type WB */
+mtrr_def = (1u << 11) | X86_MT_WB; /* E, default type WB */
/* Fixed-range MTRRs supported? */
if ( mtrr_cap & (1u << 8) )
{
+#define BCST2(mt) ((mt) | ((mt) << 8))
+#define BCST4(mt) (BCST2(mt) | (BCST2(mt) << 16))
+#define BCST8(mt) (BCST4(mt) | ((uint64_t)BCST4(mt) << 32))
/* 0x0-0x9: Write Back (WB) */
-content = 0x0606060606060606ull;
+content = BCST8(X86_MT_WB);
wrmsr(MSR_MTRRfix64K_0, content);
wrmsr(MSR_MTRRfix16K_8, content);
+
/* 0xa-0xb: Write Combining (WC) */
if ( mtrr_cap & (1u << 10) ) /* WC supported? */
-content = 0x0101010101010101ull;
+content = BCST8(X86_MT_WC);
wrmsr(MSR_MTRRfix16K_A, content);
+
/* 0xc-0xf: Write Back (WB) */
-content = 0x0606060606060606ull;
+content = BCST8(X86_MT_WB);
for ( i = 0; i < 8; i++ )
wrmsr(MSR_MTRRfix4K_C + i, content);
+#undef BCST8
+#undef BCST4
+#undef BCST2
+
mtrr_def |= 1u << 10; /* FE */
printf("fixed MTRRs ... ");
}
@@ -106,7 +117,7 @@ void cacheattr_init(void)
while ( ((base + size) < base) || ((base + size) > pci_mem_end) )
size >>= 1;
-wrmsr(MSR_MTRRphysBase(i), base);
+wrmsr(MSR_MTRRphysBase(i), base | X86_MT_UC);
wrmsr(MSR_MTRRphysMask(i), (~(size - 1) & addr_mask) | (1u << 11));
base += size;
@@ -121,7 +132,7 @@ void cacheattr_init(void)
while ( (base + size < base) || (base + size > pci_hi_mem_end) )
size >>= 1;
-wrmsr(MSR_MTRRphysBase(i), base);
+wrmsr(MSR_MTRRphysBase(i), base | X86_MT_UC);
wrmsr(MSR_MTRRphysMask(i), (~(size - 1) & addr_mask) | (1u << 11));
base += size;
[ovmf test] 175423: regressions - FAIL
flight 175423 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/175423/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-amd64-i386-xl-qemuu-ovmf-amd64 12 debian-hvm-install fail REGR. vs. 175202 test-amd64-amd64-xl-qemuu-ovmf-amd64 12 debian-hvm-install fail REGR. vs. 175202 version targeted for testing: ovmf 3f378450dfafc11754bfdb64af28060ec8466acb baseline version: ovmf d103840cfb559c28831c2635b916d60118f671cc Last test of basis 175202 2022-12-14 13:42:59 Z6 days Failing since175214 2022-12-14 18:42:16 Z5 days 36 attempts Testing same since 175423 2022-12-20 10:40:57 Z0 days1 attempts People who touched revisions under test: Abner Chang Adam Dunlap Ard Biesheuvel Chun-Yi Lee Chun-Yi Lee de...@edk2.groups.io Dov Murik Gerd Hoffmann jdzhang Jeff Brasen Jeshua Smith Jian J Wang Jiaqi Gao Jiewen Yao Judah Vang Kavya Kuo, Ted MarsX Lin Michael Kubacki Min M Xu Min Xu Nishant C Mistry Ray Ni Sebastien Boeuf Ted Kuo Tom Lendacky Xie, Yuanhao Yuanhao Xie jobs: build-amd64-xsm pass build-i386-xsm pass build-amd64 pass build-i386 pass build-amd64-libvirt pass build-i386-libvirt pass build-amd64-pvopspass build-i386-pvops pass test-amd64-amd64-xl-qemuu-ovmf-amd64 fail test-amd64-i386-xl-qemuu-ovmf-amd64 fail sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Not pushing. (No revision log; it would be 894 lines long.)
[xen-4.16-testing test] 175414: trouble: broken/fail/pass
flight 175414 xen-4.16-testing real [real] http://logs.test-lab.xenproject.org/osstest/logs/175414/ Failures and problems with tests :-( Tests which did not succeed and are blocking, including tests which could not be run: test-amd64-i386-qemut-rhel6hvm-intel broken test-amd64-i386-xl-vhd broken test-amd64-i386-freebsd10-amd64 broken test-xtf-amd64-amd64-2 broken test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm broken in 175403 test-amd64-i386-libvirt-xsm broken in 175403 Tests which are failing intermittently (not blocking): test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 5 host-install(5) broken in 175403 pass in 175414 test-amd64-i386-libvirt-xsm 5 host-install(5) broken in 175403 pass in 175414 test-amd64-i386-qemut-rhel6hvm-intel 5 host-install(5) broken pass in 175403 test-xtf-amd64-amd64-25 host-install(5) broken pass in 175403 test-amd64-i386-freebsd10-amd64 5 host-install(5) broken pass in 175403 test-amd64-i386-xl-vhd5 host-install(5) broken pass in 175403 Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 175155 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 175155 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 175155 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 175155 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 175155 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 175155 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 175155 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 175155 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 175155 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 175155 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 175155 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 175155 test-amd64-i386-xl-pvshim14 guest-start fail never pass test-arm64-arm64-xl-seattle 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 16 saverestore-support-checkfail never pass test-amd64-i386-libvirt 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-xl 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 16 saverestore-support-checkfail never pass test-amd64-i386-libvirt-raw 14 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-cubietruck 15 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 16 saverestore-support-checkfail never pass
Re: [PATCH 05/22] x86/srat: vmap the pages for acpi_slit
On 16.12.2022 12:48, Julien Grall wrote: > From: Hongyan Xia > > This avoids the assumption that boot pages are in the direct map. > > Signed-off-by: Hongyan Xia > Signed-off-by: Julien Grall Reviewed-by: Jan Beulich However, ... > --- a/xen/arch/x86/srat.c > +++ b/xen/arch/x86/srat.c > @@ -139,7 +139,8 @@ void __init acpi_numa_slit_init(struct acpi_table_slit > *slit) > return; > } > mfn = alloc_boot_pages(PFN_UP(slit->header.length), 1); > - acpi_slit = mfn_to_virt(mfn_x(mfn)); > + acpi_slit = vmap_contig_pages(mfn, PFN_UP(slit->header.length)); ... with the increased use of vmap space the VA range used will need growing. And that's perhaps better done ahead of time than late. > + BUG_ON(!acpi_slit); Similarly relevant for the earlier patch: It would be nice if boot failure for optional things like NUMA data could be avoided. But I understand this is somewhat orthogonal to this series (the more that alloc_boot_pages() itself is also affected). Yet not entirely so, since previously there was no mapping failure possible here. Jan
Re: [PATCH 04/22] xen/numa: vmap the pages for memnodemap
On 16.12.2022 12:48, Julien Grall wrote:
> From: Hongyan Xia
>
> This avoids the assumption that there is a direct map and boot pages
> fall inside the direct map.
>
> Clean up the variables so that mfn actually stores a type-safe mfn.
>
> Signed-off-by: Hongyan Xia
> Signed-off-by: Julien Grall
Reviewed-by: Jan Beulich
(obviously remains valid across ...
> --- a/xen/common/numa.c
> +++ b/xen/common/numa.c
> @@ -424,13 +424,13 @@ static int __init populate_memnodemap(const struct node
> *nodes,
> static int __init allocate_cachealigned_memnodemap(void)
> {
> unsigned long size = PFN_UP(memnodemapsize * sizeof(*memnodemap));
> -unsigned long mfn = mfn_x(alloc_boot_pages(size, 1));
> +mfn_t mfn = alloc_boot_pages(size, 1);
>
> -memnodemap = mfn_to_virt(mfn);
> -mfn <<= PAGE_SHIFT;
> +memnodemap = vmap_contig_pages(mfn, size);
... a possible rename of this function)
Jan
Re: [XEN v1 8/9] xen/arm: Other adaptations required to support 32bit paddr
On 16/12/2022 10:23, Julien Grall wrote:
Hi,
Hi Julien,
Each adaptations are distinct, so I would prefer if they are in in
separate patch.
This will also make clear which components you touched because I would
be surprised if this is really the only place where we need
adaptation. Maybe that's because you didn't compile everything (which
is fine).
On 15/12/2022 19:32, Ayan Kumar Halder wrote:
1. Supersections are supported only for paddr greater than 32 bits.
Two questions:
* Can you outline why we can't keep the code around?
For PA_32, the following bitoperation will overflow :-
*ipa |= (paddr_t)(pte.supersec.extbase1) <<
L1DESC_SUPERSECTION_EXT_BASE1_SHIFT;
*ipa |= (paddr_t)(pte.supersec.extbase2) <<
L1DESC_SUPERSECTION_EXT_BASE2_SHIFT;
Also, pte.supersec.extbase1 and pte.supersec.extbase2 are not valid for
PA_32. Refer xen/arch/arm/include/asm/short-desc.h
unsigned int extbase2:4; /* Extended base address, PA[39:36] */
unsigned int extbase1:4; /* Extended base address, PA[35:32] */
* Can you give a pointer to the Arm Arm that says supersection is not
supported?
I could not find any sentence in Arm Arm which says supersection is
**not** supported on 32 bit PA.
However,
Refer"ARM DDI 0487I.a ID081822", G5.4 "The VMSAv8-32 Short-descriptor
translation table format", G5-9163
"Support for Supersections is **optional**, except that an
implementation that supports more than 32 bits of PA must also support
Supersections to provide access to the entire PA space."
Also, G5.1.3 "Address spaces in VMSAv8-32", G5-9149
"AArch32 defines two translation table formats. The Long-descriptor
format gives access to the full 40-bit IPA or PA space at a granularity
of 4KB. The Short-descriptor format:
Gives access to a 32-bit PA space at 4KB granularity.
Gives access to a 40-bit PA space, but only at 16MB granularity, by
the use of Supersections."
from the above 2 snippets, I inferred that supersections are only
supported for PAs greater than 32 bits.
I could not find any evidence of supersections supported for 32 bit PA.
- Ayan
2. Use 0 wherever physical addresses are right shifted for 32 bit >
3. Use PRIx64 to print u64
It would be good to explain that the current use was buggy because we
are printing a PTE and not a physical address.
Signed-off-by: Ayan Kumar Halder
---
xen/arch/arm/guest_walk.c | 2 ++
xen/arch/arm/mm.c | 2 +-
xen/drivers/passthrough/arm/smmu.c | 5 +
3 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/xen/arch/arm/guest_walk.c b/xen/arch/arm/guest_walk.c
index 43d3215304..4384068285 100644
--- a/xen/arch/arm/guest_walk.c
+++ b/xen/arch/arm/guest_walk.c
@@ -149,6 +149,7 @@ static bool guest_walk_sd(const struct vcpu *v,
mask = (1ULL << L1DESC_SECTION_SHIFT) - 1;
*ipa = ((paddr_t)pte.sec.base << L1DESC_SECTION_SHIFT)
| (gva & mask);
}
+#ifndef CONFIG_ARM_PA_32
A "malicious" guest can still set that bit. So now, you will return
from guest_walk_sd() with 'ipa' not set at all.
If this is effectively not supported, then we should return 'false'
rather than claiming the translation was successful.
else /* Supersection */
{
mask = (1ULL << L1DESC_SUPERSECTION_SHIFT) - 1;
@@ -157,6 +158,7 @@ static bool guest_walk_sd(const struct vcpu *v,
*ipa |= (paddr_t)(pte.supersec.extbase1) <<
L1DESC_SUPERSECTION_EXT_BASE1_SHIFT;
*ipa |= (paddr_t)(pte.supersec.extbase2) <<
L1DESC_SUPERSECTION_EXT_BASE2_SHIFT;
}
+#endif
/* Set permissions so that the caller can check the flags
by herself. */
if ( !pte.sec.ro )
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index be939fb106..3bc9894008 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -229,7 +229,7 @@ void dump_pt_walk(paddr_t ttbr, paddr_t addr,
pte = mapping[offsets[level]];
- printk("%s[0x%03x] = 0x%"PRIpaddr"\n",
+ printk("%s[0x%03x] = 0x%"PRIx64"\n",
level_strs[level], offsets[level], pte.bits);
if ( level == 3 || !pte.walk.valid || !pte.walk.table )
diff --git a/xen/drivers/passthrough/arm/smmu.c
b/xen/drivers/passthrough/arm/smmu.c
index 5ae180a4cc..522a478ccf 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -1184,7 +1184,12 @@ static void arm_smmu_init_context_bank(struct
arm_smmu_domain *smmu_domain)
reg = (p2maddr & ((1ULL << 32) - 1));
writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0_LO);
+
+#ifndef CONFIG_ARM_PA_32
reg = (p2maddr >> 32);
+#else
+ reg = 0;
+#endif
I think it would be better if we implement writeq(). This will also
avoid the now strange ((1ULL << 32) - 1) when p2maddr is a paddr_t.
if (stage1)
reg |= ARM_SMMU_CB_ASID(cfg) << TTBRn_HI_ASID_SHIFT;
writel_relaxed(reg, cb_base + ARM_SMMU_CB_TTBR0_HI);
Re: [PATCH v2 3/3] x86/ucode: load microcode earlier on boot CPU
On 20.12.2022 15:50, Andrew Cooper wrote: > On 19/12/2022 2:45 pm, Sergey Dyasli wrote: >> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c >> index 6bb5bc7c84..2d7c815e0a 100644 >> --- a/xen/arch/x86/setup.c >> +++ b/xen/arch/x86/setup.c >> relocated = true; >> @@ -1762,11 +1768,9 @@ void __init noreturn __start_xen(unsigned long mbi_p) >> >> init_IRQ(); >> >> -microcode_grab_module(module_map, mbi); >> - >> timer_init(); >> >> -early_microcode_init(); >> +early_microcode_init_cache(module_map, mbi); > > microcode_init_cache(module_map, mbi); /* Needs xmalloc() */ > > Can fix on commit. Are you merely after the added comment, or is the omission of the early_ prefix also meaningful in some way? Jan
Re: [PATCH 03/22] acpi: vmap pages in acpi_os_alloc_memory
On 16.12.2022 12:48, Julien Grall wrote:
> --- a/xen/common/vmap.c
> +++ b/xen/common/vmap.c
> @@ -244,6 +244,11 @@ void *vmap(const mfn_t *mfn, unsigned int nr)
> return __vmap(mfn, 1, nr, 1, PAGE_HYPERVISOR, VMAP_DEFAULT);
> }
>
> +void *vmap_contig_pages(mfn_t mfn, unsigned int nr_pages)
I don't think the _pages suffix buys us much here. I also think parameter
names would better be consistent with other functions here, in particular
with vmap() (i.e. s/nr_pages/nr/).
> --- a/xen/drivers/acpi/osl.c
> +++ b/xen/drivers/acpi/osl.c
> @@ -221,7 +221,11 @@ void *__init acpi_os_alloc_memory(size_t sz)
> void *ptr;
>
> if (system_state == SYS_STATE_early_boot)
> - return mfn_to_virt(mfn_x(alloc_boot_pages(PFN_UP(sz), 1)));
> + {
> + mfn_t mfn = alloc_boot_pages(PFN_UP(sz), 1);
> +
> + return vmap_contig_pages(mfn, PFN_UP(sz));
> + }
Multiple pages may be allocated here, yet ...
> @@ -246,5 +250,10 @@ void __init acpi_os_free_memory(void *ptr)
> if (is_xmalloc_memory(ptr))
> xfree(ptr);
> else if (ptr && system_state == SYS_STATE_early_boot)
> - init_boot_pages(__pa(ptr), __pa(ptr) + PAGE_SIZE);
> + {
> + paddr_t addr = mfn_to_maddr(vmap_to_mfn(ptr));
> +
> + vunmap(ptr);
> + init_boot_pages(addr, addr + PAGE_SIZE);
> + }
... (as before) only one page would be freed here. With the move to
vmap() it ought to be possible to do better now. (If you want to
defer this to a later patch, please at least mention the aspect in
the description.)
Jan
Re: [PATCH 02/22] x86/setup: move vm_init() before acpi calls
On 16.12.2022 12:48, Julien Grall wrote:
> From: Wei Liu
>
> After the direct map removal, pages from the boot allocator are not
> mapped at all in the direct map. Although we have map_domain_page, they
Nit: "will not be mapped" or "are not going to be mapped", or else this
sounds like there's a bug somewhere.
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -870,6 +870,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> unsigned long eb_start, eb_end;
> bool acpi_boot_table_init_done = false, relocated = false;
> int ret;
> +bool vm_init_done = false;
Can this please be grouped with the other bool-s (even visible in context)?
> --- a/xen/common/vmap.c
> +++ b/xen/common/vmap.c
> @@ -34,9 +34,20 @@ void __init vm_init_type(enum vmap_region type, void
> *start, void *end)
>
> for ( i = 0, va = (unsigned long)vm_bitmap(type); i < nr; ++i, va +=
> PAGE_SIZE )
> {
> -struct page_info *pg = alloc_domheap_page(NULL, 0);
> +mfn_t mfn;
> +int rc;
>
> -map_pages_to_xen(va, page_to_mfn(pg), 1, PAGE_HYPERVISOR);
> +if ( system_state == SYS_STATE_early_boot )
> +mfn = alloc_boot_pages(1, 1);
> +else
> +{
> +struct page_info *pg = alloc_domheap_page(NULL, 0);
> +
> +BUG_ON(!pg);
> +mfn = page_to_mfn(pg);
> +}
> +rc = map_pages_to_xen(va, mfn, 1, PAGE_HYPERVISOR);
> +BUG_ON(rc);
The adding of a return value check is unrelated and not overly useful:
> clear_page((void *)va);
This will fault anyway if the mapping attempt failed.
Jan
Re: [PATCH v2 3/3] x86/ucode: load microcode earlier on boot CPU
On 19/12/2022 2:45 pm, Sergey Dyasli wrote: > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c > index 6bb5bc7c84..2d7c815e0a 100644 > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > relocated = true; > @@ -1762,11 +1768,9 @@ void __init noreturn __start_xen(unsigned long mbi_p) > > init_IRQ(); > > -microcode_grab_module(module_map, mbi); > - > timer_init(); > > -early_microcode_init(); > +early_microcode_init_cache(module_map, mbi); microcode_init_cache(module_map, mbi); /* Needs xmalloc() */ Can fix on commit. ~Andrew
[PATCH v1] xen/pvcalls: free active map buffer on pvcalls_front_free_map
Data buffer for active map is allocated in alloc_active_ring and freed
in free_active_ring function, which is used only for the error
cleanup. pvcalls_front_release is calling pvcalls_front_free_map which
ends foreign access for this buffer, but doesn't free allocated pages.
Call free_active_ring to clean all allocated resources.
Signed-off-by: Oleksii Moisieiev
---
drivers/xen/pvcalls-front.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/xen/pvcalls-front.c b/drivers/xen/pvcalls-front.c
index 1826e8e67125..9b569278788a 100644
--- a/drivers/xen/pvcalls-front.c
+++ b/drivers/xen/pvcalls-front.c
@@ -225,6 +225,8 @@ static irqreturn_t pvcalls_front_event_handler(int irq,
void *dev_id)
return IRQ_HANDLED;
}
+static void free_active_ring(struct sock_mapping *map);
+
static void pvcalls_front_free_map(struct pvcalls_bedata *bedata,
struct sock_mapping *map)
{
@@ -240,7 +242,7 @@ static void pvcalls_front_free_map(struct pvcalls_bedata
*bedata,
for (i = 0; i < (1 << PVCALLS_RING_ORDER); i++)
gnttab_end_foreign_access(map->active.ring->ref[i], NULL);
gnttab_end_foreign_access(map->active.ref, NULL);
- free_page((unsigned long)map->active.ring);
+ free_active_ring(map);
kfree(map);
}
--
2.25.1
Re: [PATCH v2 2/2] acpi: Add TPM2 interface definition.
On 15.12.2022 18:09, Jennifer Herbert wrote:
> --- a/tools/firmware/hvmloader/util.c
> +++ b/tools/firmware/hvmloader/util.c
> @@ -1009,6 +1009,13 @@ void hvmloader_acpi_build_tables(struct acpi_config
> *config,
> config->table_flags |= ACPI_HAS_TPM;
> config->tis_hdr = (uint16_t *)ACPI_TIS_HDR_ADDRESS;
> break;
> +case 2:
Nit: Blank line please between non-fall-through case blocks.
> +config->table_flags |= ACPI_HAS_TPM;
> +config->crb_hdr = (uint16_t *)ACPI_CRB_HDR_ADDRESS;
> +
> +mem_hole_populate_ram(TPM_LOG_AREA_ADDRESS >> PAGE_SHIFT,
> TPM_LOG_SIZE >> PAGE_SHIFT);
Nit: Overlong line.
> +memset((void *)(TPM_LOG_AREA_ADDRESS), 0, TPM_LOG_SIZE);
Nit: Excess pair of parentheses.
> --- a/tools/libacpi/Makefile
> +++ b/tools/libacpi/Makefile
> @@ -25,7 +25,7 @@ C_SRC-$(CONFIG_X86) = dsdt_anycpu.c dsdt_15cpu.c
> dsdt_anycpu_qemu_xen.c dsdt_pvh
> C_SRC-$(CONFIG_ARM_64) = dsdt_anycpu_arm.c
> DSDT_FILES ?= $(C_SRC-y)
> C_SRC = $(addprefix $(ACPI_BUILD_DIR)/, $(DSDT_FILES))
> -H_SRC = $(addprefix $(ACPI_BUILD_DIR)/, ssdt_s3.h ssdt_s4.h ssdt_pm.h
> ssdt_tpm.h ssdt_laptop_slate.h)
> +H_SRC = $(addprefix $(ACPI_BUILD_DIR)/, ssdt_s3.h ssdt_s4.h ssdt_pm.h
> ssdt_tpm.h ssdt_tpm2.h ssdt_laptop_slate.h)
This line could (the latest) now also do with splitting up.
> --- a/tools/libacpi/acpi2_0.h
> +++ b/tools/libacpi/acpi2_0.h
> @@ -121,6 +121,30 @@ struct acpi_20_tcpa {
> };
> #define ACPI_2_0_TCPA_LAML_SIZE (64*1024)
>
> +/*
> + * TPM2
> + */
> +struct acpi_20_tpm2 {
> +struct acpi_header header;
> +uint16_t platform_class;
> +uint16_t reserved;
> +uint64_t control_area_address;
> +uint32_t start_method;
> +uint8_t start_method_params[12];
> +uint32_t log_area_minimum_length;
> +uint64_t log_area_start_address;
> +};
> +#define TPM2_ACPI_CLASS_CLIENT 0
> +#define TPM2_START_METHOD_CRB 7
> +
> +#define TPM_CRB_ADDR_BASE 0xFED4
> +#define TPM_CRB_ADDR_CTRL (TPM_CRB_ADDR_BASE + 0x40)
What is the relation between these two and ACPI_CRB_HDR_ADDRESS
(0xFED40034)? Independent of the answer it would be nice to have a
BUILD_BUG_ON()-like check somewhere tying the two together (and I have
a vague recollection that I might have asked for such in a comment on
v1 already).
And since afaics the space at that address also isn't filled
anywhere in hvmloader, the description could also do with saying what
entity is doing that (qemu?) and hence with whom this needs to remain
in sync.
> @@ -449,6 +451,39 @@ static int construct_secondary_tables(struct acpi_ctxt
> *ctxt,
> tcpa->header.length);
> }
> break;
> +
> +case 2:
> +if (!config->crb_hdr ||
See the respective comment on the earlier patch.
Jan
Re: [PATCH v2 1/2] acpi: Make TPM version configurable.
On 15.12.2022 18:09, Jennifer Herbert wrote:
> --- a/docs/misc/xenstore-paths.pandoc
> +++ b/docs/misc/xenstore-paths.pandoc
> @@ -269,6 +269,14 @@ at the guest physical address in
> HVM_PARAM_VM_GENERATION_ID_ADDR.
> See Microsoft's "Virtual Machine Generation ID" specification for the
> circumstances where the generation ID needs to be changed.
>
> +
> + ~/platform/tpm_version = INTEGER [HVM,INTERNAL]
> +
> +The TPM version to be probed for.
> +
> +A value of 1 indicates to probe for TPM 1.2. If unset, or an
> +invalid version, then no TPM is probed.
And who's writing this new key? Aren't you regressing existing guests
by defaulting to "no TPM" in the absence of this key?
> --- a/tools/firmware/hvmloader/util.c
> +++ b/tools/firmware/hvmloader/util.c
> @@ -994,13 +994,22 @@ void hvmloader_acpi_build_tables(struct acpi_config
> *config,
> if ( !strncmp(xenstore_read("platform/acpi_laptop_slate", "0"), "1", 1)
> )
> config->table_flags |= ACPI_HAS_SSDT_LAPTOP_SLATE;
>
> -config->table_flags |= (ACPI_HAS_TCPA | ACPI_HAS_IOAPIC |
> +config->table_flags |= (ACPI_HAS_TPM | ACPI_HAS_IOAPIC |
I'm puzzled by ACPI_HAS_TPM being present both here and ...
> ACPI_HAS_WAET | ACPI_HAS_PMTIMER |
> ACPI_HAS_BUTTONS | ACPI_HAS_VGA |
> ACPI_HAS_8042 | ACPI_HAS_CMOS_RTC);
> config->acpi_revision = 4;
>
> -config->tis_hdr = (uint16_t *)ACPI_TIS_HDR_ADDRESS;
> +s = xenstore_read("platform/tpm_version", "0");
> +config->tpm_version = strtoll(s, NULL, 0);
> +
> +switch( config->tpm_version )
> +{
> +case 1:
> +config->table_flags |= ACPI_HAS_TPM;
.. here.
Also (nit): Missing blank after "switch".
> --- a/tools/libacpi/build.c
> +++ b/tools/libacpi/build.c
> @@ -409,38 +409,46 @@ static int construct_secondary_tables(struct acpi_ctxt
> *ctxt,
> memcpy(ssdt, ssdt_laptop_slate, sizeof(ssdt_laptop_slate));
> table_ptrs[nr_tables++] = ctxt->mem_ops.v2p(ctxt, ssdt);
> }
> -
> -/* TPM TCPA and SSDT. */
> -if ( (config->table_flags & ACPI_HAS_TCPA) &&
> - (config->tis_hdr[0] != 0 && config->tis_hdr[0] != 0x) &&
> - (config->tis_hdr[1] != 0 && config->tis_hdr[1] != 0x) )
> +/* TPM and SSDT. */
May I ask that since you're altering the comment, you make it "TPM and
its SSDT"?
> +if (config->table_flags & ACPI_HAS_TPM)
Nit: The file is predominantly using Xen style, so please adhere to that
(also again further down).
> {
> -ssdt = ctxt->mem_ops.alloc(ctxt, sizeof(ssdt_tpm), 16);
> -if (!ssdt) return -1;
> -memcpy(ssdt, ssdt_tpm, sizeof(ssdt_tpm));
> -table_ptrs[nr_tables++] = ctxt->mem_ops.v2p(ctxt, ssdt);
> -
> -tcpa = ctxt->mem_ops.alloc(ctxt, sizeof(struct acpi_20_tcpa), 16);
> -if (!tcpa) return -1;
> -memset(tcpa, 0, sizeof(*tcpa));
> -table_ptrs[nr_tables++] = ctxt->mem_ops.v2p(ctxt, tcpa);
> -
> -tcpa->header.signature = ACPI_2_0_TCPA_SIGNATURE;
> -tcpa->header.length= sizeof(*tcpa);
> -tcpa->header.revision = ACPI_2_0_TCPA_REVISION;
> -fixed_strcpy(tcpa->header.oem_id, ACPI_OEM_ID);
> -fixed_strcpy(tcpa->header.oem_table_id, ACPI_OEM_TABLE_ID);
> -tcpa->header.oem_revision = ACPI_OEM_REVISION;
> -tcpa->header.creator_id = ACPI_CREATOR_ID;
> -tcpa->header.creator_revision = ACPI_CREATOR_REVISION;
> -if ( (lasa = ctxt->mem_ops.alloc(ctxt, ACPI_2_0_TCPA_LAML_SIZE, 16))
> != NULL )
> +switch (config->tpm_version)
> {
> -tcpa->lasa = ctxt->mem_ops.v2p(ctxt, lasa);
> -tcpa->laml = ACPI_2_0_TCPA_LAML_SIZE;
> -memset(lasa, 0, tcpa->laml);
> -set_checksum(tcpa,
> - offsetof(struct acpi_header, checksum),
> - tcpa->header.length);
> +case 1:
> +if (!config->tis_hdr ||
There was no such check in the original code, and I think it would
better remain the case that the field is set if and only if
ACPI_HAS_TPM is set and (now) version is 1. (Aiui this check actually
covers for the double setting of ACPI_HAS_TPM commented on above.)
> +config->tis_hdr[0] == 0 || config->tis_hdr[0] == 0x ||
> +config->tis_hdr[1] == 0 || config->tis_hdr[1] == 0x)
> +break;
> +
> +ssdt = ctxt->mem_ops.alloc(ctxt, sizeof(ssdt_tpm), 16);
> +if (!ssdt) return -1;
> +memcpy(ssdt, ssdt_tpm, sizeof(ssdt_tpm));
> +table_ptrs[nr_tables++] = ctxt->mem_ops.v2p(ctxt, ssdt);
> +
> +tcpa = ctxt->mem_ops.alloc(ctxt, sizeof(struct acpi_20_tcpa),
> 16);
> +if (!tcpa) return -1;
> +memset(tcpa, 0, sizeof(*tcpa));
> +table_ptrs[nr_tables++] = ctxt->mem_ops.v2p(ctxt, tcpa);
> +
> +tcpa->header.signature = ACPI_2_
Re: [PATCH v2 3/3] x86/ucode: load microcode earlier on boot CPU
On 19.12.2022 15:45, Sergey Dyasli wrote: > Call early_microcode_init() straight after multiboot modules become > accessible. Modify it to load the ucode directly from the blob bypassing > populating microcode_cache because xmalloc is still not available at > that point during Xen boot. > > Introduce early_microcode_init_cache() for populating microcode_cache. > It needs to rescan the modules in order to find the new virtual address > of the ucode blob because it changes during the boot process, e.g. > from 0x010802fc to 0x83204dac52fc. > > While at it, drop alternative_vcall() from early_microcode_init() since > it's not useful in an __init fuction. > > Signed-off-by: Sergey Dyasli Reviewed-by: Jan Beulich
Re: [PATCH v2 2/3] x86/ucode: allow cpu_request_microcode() to skip memory allocation
On 19.12.2022 15:45, Sergey Dyasli wrote: > This is a preparatory step in order to do earlier microcode loading on > the boot CPU when the domain heap has not been initialized yet and > xmalloc still unavailable. > > Add make_copy argument which will allow to load microcode directly from > the blob bypassing microcode_cache. > > Signed-off-by: Sergey Dyasli Acked-by: Jan Beulich
Re: [PATCH v2 1/3] xen/multiboot: add proper struct definitions to typedefs
On 19.12.2022 15:45, Sergey Dyasli wrote:
> This allows to use them for forward declaration in other headers.
>
> Signed-off-by: Sergey Dyasli
Acked-by: Jan Beulich
with ...
> --- a/xen/include/xen/multiboot.h
> +++ b/xen/include/xen/multiboot.h
> @@ -46,23 +46,25 @@
> #ifndef __ASSEMBLY__
>
> /* The symbol table for a.out. */
> -typedef struct {
> +struct aout_symbol_table {
> u32 tabsize;
> u32 strsize;
> u32 addr;
> u32 reserved;
> -} aout_symbol_table_t;
> +};
> +typedef struct aout_symbol_table aout_symbol_table_t;
>
> /* The section header table for ELF. */
> -typedef struct {
> +struct elf_section_header_table{
... the missing blank added here.
I also have to admit that I would have preferred a variant with less
code churn: The typedef re-arrangement really isn't needed to fulfill
the goal pf the change.
Jan
Re: [PATCH v2-ish] x86/boot: Factor move_xen() out of __start_xen()
On 16.12.2022 21:17, Andrew Cooper wrote:
> Partly for clarity because there is a lot of subtle magic at work here.
> Expand the commentary of what's going on.
>
> Also, because there is no need to double copy the stack (32kB) to retrieve
> local values spilled to the stack under the old alias, when all of the
> aforementioned local variables are going out of scope anyway.
>
> There is also a logic change when walking l2_xenmap[]. The old logic would
> skip recursing into 4k mappings; this would corrupt Xen if it were used.
> There are no 4k mappings in l2_xenmap[] this early on boot, so assert the
> property instead of leaving a latent breakage.
>
> Signed-off-by: Andrew Cooper
> ---
> CC: Jan Beulich
> CC: Roger Pau Monné
> CC: Wei Liu
>
> We probably want to drop PGE from XEN_MINIMAL_CR4. It will simplify several
> boot paths which don't need to care about global pages, and PGE is conditional
> anyway now with the PCID support.
Perhaps, especially if - as you say - this allows for simplification.
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -467,6 +467,113 @@ static void __init move_memory(
> }
> }
>
> +static void __init noinline move_xen(void)
> +{
> +l4_pgentry_t *pl4e;
> +l3_pgentry_t *pl3e;
> +l2_pgentry_t *pl2e;
> +unsigned long tmp;
> +int i, j, k;
Can these become "unsigned int" please at this occasion? (Perhaps as
another reason to make the change, mention that the old instances of i and
j did shadow outer scope variables in the same function?)
> +/*
> + * The caller has selected xen_phys_start, ensuring that the old and new
> + * locations do not overlap. Make it so.
As a non-native speaker I'm struggling with what "Make it so" is supposed
to tell me here.
> + * Prevent the compiler from reordering anything across this point. Such
> + * things will end badly.
> + */
> +barrier();
> +
> +/*
> + * Copy out of the current alias, into the directmap at the new location.
> + * This includes a snapshot of the current stack.
> + */
> +memcpy(__va(__pa(_start)), _start, _end - _start);
> +
> +/*
> + * We are now in a critical region. Any write (modifying a global,
> + * spilling a local to the stack, etc) via the current alias will get
> lost
> + * when we switch to the new pagetables. This even means no printk()'s
> + * debugging purposes.
Nit: Missing "for"?
> + * Walk the soon-to-be-used pagetables in the new location, relocating
> all
> + * intermediate (non-leaf) entries to point to their new-location
> + * equivalents. All writes are via the directmap alias.
> + */
> +pl4e = __va(__pa(idle_pg_table));
> +for ( i = 0 ; i < L4_PAGETABLE_ENTRIES; i++, pl4e++ )
> +{
> +if ( !(l4e_get_flags(*pl4e) & _PAGE_PRESENT) )
> +continue;
> +
> +*pl4e = l4e_from_intpte(l4e_get_intpte(*pl4e) + xen_phys_start);
> +pl3e = __va(l4e_get_paddr(*pl4e));
> +for ( j = 0; j < L3_PAGETABLE_ENTRIES; j++, pl3e++ )
> +{
> +if ( !(l3e_get_flags(*pl3e) & _PAGE_PRESENT) ||
> + (l3e_get_flags(*pl3e) & _PAGE_PSE) )
> +continue;
> +
> +*pl3e = l3e_from_intpte(l3e_get_intpte(*pl3e) + xen_phys_start);
> +pl2e = __va(l3e_get_paddr(*pl3e));
> +for ( k = 0; k < L2_PAGETABLE_ENTRIES; k++, pl2e++ )
> +{
> +if ( !(l2e_get_flags(*pl2e) & _PAGE_PRESENT) ||
> + (l2e_get_flags(*pl2e) & _PAGE_PSE) )
> +continue;
> +
> +*pl2e = l2e_from_intpte(l2e_get_intpte(*pl2e) +
> xen_phys_start);
> +}
> +}
> +}
> +
> +/*
> + * Walk the soon-to-be-used l2_xenmap[], relocating all the leaf mappings
> + * so text/data/bss etc refer to the new location in memory.
> + */
> +pl2e = __va(__pa(l2_xenmap));
> +for ( i = 0; i < L2_PAGETABLE_ENTRIES; i++, pl2e++ )
> +{
> +if ( !(l2e_get_flags(*pl2e) & _PAGE_PRESENT) )
> +continue;
> +
> +/*
> + * We don't have 4k mappings in l2_xenmap[] at this point in boot,
> nor
> + * is this anticipated to change. Simply assert the fact, rather
> than
> + * introducing dead logic to decend into l1 tables.
Nit: "descend"?
> + */
> +ASSERT(l2e_get_flags(*pl2e) & _PAGE_PSE);
The warning about the use of printk() around here could make one think
that using ASSERT() (or BUG()) is similarly bad. However, aiui using
printk() isn't by itself a problem, just that the log message would be
lost as far as the circular buffer goes. The message would be observable
on the serial con sole though, at least with "sync_console". It is on
this basis that using ASSERT() here makes sense. Perhaps the printk()
related comment could be slightly adjusted to express this?
> +*pl2e = l2e_from_intpte(l2e_get_intpte(*pl2e) + xen_phys_start);
> +
[xen-4.13-testing test] 175412: FAIL
flight 175412 xen-4.13-testing real [real] http://logs.test-lab.xenproject.org/osstest/logs/175412/ Failures and problems with tests :-( Tests which did not succeed and are blocking, including tests which could not be run: test-amd64-amd64-xl-qemuu-ws16-amd64 broken in 175402 test-amd64-amd64-xl-pvshim broken in 175402 Tests which are failing intermittently (not blocking): test-amd64-amd64-xl-qemuu-ws16-amd64 5 host-install(5) broken in 175402 pass in 175412 test-amd64-amd64-xl-pvshim 5 host-install(5) broken in 175402 pass in 175412 test-amd64-i386-qemuu-rhel6hvm-intel 13 guest-stop fail in 175402 pass in 175412 test-amd64-amd64-pygrub 21 guest-start/debian.repeat fail in 175402 pass in 175412 test-armhf-armhf-libvirt-qcow2 20 leak-check/check fail pass in 175402 Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 174675 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 174675 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 174675 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 174675 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 174675 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 174675 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 174675 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 174675 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 174675 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 174675 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 174675 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 174675 test-amd64-i386-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-i386-xl-pvshim14 guest-start fail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-i386-libvirt 15 migrate-support-checkfail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-rtds 15 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail never pass test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-xl 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail never pass test-amd64-i386-libvirt-raw 14 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 15 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 16 saverestore-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit1 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 15 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 14 migrate-support-checkfail never pass test-
[xen-unstable-smoke test] 175422: tolerable all pass - PUSHED
flight 175422 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/175422/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass version targeted for testing: xen 0fc5fa9333b21122c6e77fa42f5683e31c81bbe5 baseline version: xen f1b9a28922d2913dda76fd82b0b79f3651d3fc8d Last test of basis 175415 2022-12-19 23:02:04 Z0 days Testing same since 175422 2022-12-20 10:02:02 Z0 days1 attempts People who touched revisions under test: Julien Grall Stewart Hildebrand 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 f1b9a28922..0fc5fa9333 0fc5fa9333b21122c6e77fa42f5683e31c81bbe5 -> smoke
[linux-linus test] 175411: regressions - trouble: broken/fail/pass
flight 175411 linux-linus real [real] http://logs.test-lab.xenproject.org/osstest/logs/175411/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-amd64-amd64-xl-shadow broken test-amd64-amd64-xl-qemuu-win7-amd64 broken test-amd64-amd64-xl-qemut-stubdom-debianhvm-amd64-xsm broken test-amd64-amd64-xl-qemut-debianhvm-i386-xsmbroken test-amd64-amd64-xl-qemut-debianhvm-amd64 broken test-amd64-amd64-xl-credit2 broken test-amd64-amd64-libvirt-pair broken test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm broken test-amd64-amd64-pairbroken test-amd64-amd64-xl-qemuu-win7-amd64 5 host-install(5) broken REGR. vs. 173462 test-amd64-amd64-xl-credit2 5 host-install(5)broken REGR. vs. 173462 test-amd64-amd64-xl-qemut-debianhvm-amd64 5 host-install(5) broken REGR. vs. 173462 test-amd64-amd64-xl-qemut-stubdom-debianhvm-amd64-xsm 5 host-install(5) broken REGR. vs. 173462 test-amd64-amd64-pair 7 host-install/dst_host(7) broken REGR. vs. 173462 test-amd64-amd64-xl-shadow5 host-install(5)broken REGR. vs. 173462 test-amd64-amd64-libvirt-pair 7 host-install/dst_host(7) broken REGR. vs. 173462 test-amd64-amd64-xl-qemut-debianhvm-i386-xsm 5 host-install(5) broken REGR. vs. 173462 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 5 host-install(5) broken REGR. vs. 173462 test-amd64-coresched-amd64-xl 8 xen-bootfail REGR. vs. 173462 test-amd64-amd64-xl-qemuu-ws16-amd64 8 xen-boot fail REGR. vs. 173462 test-amd64-amd64-xl-qemut-win7-amd64 8 xen-boot fail REGR. vs. 173462 test-amd64-amd64-qemuu-nested-amd 8 xen-bootfail REGR. vs. 173462 test-amd64-amd64-xl-vhd 8 xen-boot fail REGR. vs. 173462 test-amd64-amd64-qemuu-nested-intel 8 xen-boot fail REGR. vs. 173462 test-amd64-amd64-xl-pvhv2-amd 8 xen-bootfail REGR. vs. 173462 test-amd64-amd64-xl-qemuu-debianhvm-amd64-shadow 8 xen-boot fail REGR. vs. 173462 test-amd64-amd64-xl-qemut-ws16-amd64 8 xen-boot fail REGR. vs. 173462 test-amd64-amd64-xl-qemuu-debianhvm-amd64 8 xen-bootfail REGR. vs. 173462 test-amd64-amd64-freebsd11-amd64 8 xen-boot fail REGR. vs. 173462 test-amd64-amd64-freebsd12-amd64 8 xen-boot fail REGR. vs. 173462 test-amd64-amd64-xl-multivcpu 8 xen-bootfail REGR. vs. 173462 test-amd64-amd64-xl-qemuu-debianhvm-i386-xsm 8 xen-boot fail REGR. vs. 173462 test-amd64-amd64-xl-pvhv2-intel 8 xen-boot fail REGR. vs. 173462 test-amd64-amd64-xl-qemuu-dmrestrict-amd64-dmrestrict 8 xen-boot fail REGR. vs. 173462 test-amd64-amd64-xl-pvshim8 xen-boot fail REGR. vs. 173462 test-amd64-amd64-libvirt 8 xen-boot fail REGR. vs. 173462 test-amd64-amd64-libvirt-qcow2 8 xen-boot fail REGR. vs. 173462 test-arm64-arm64-xl 8 xen-boot fail REGR. vs. 173462 test-arm64-arm64-xl-vhd 8 xen-boot fail REGR. vs. 173462 test-arm64-arm64-libvirt-raw 8 xen-boot fail REGR. vs. 173462 test-arm64-arm64-xl-credit1 8 xen-boot fail REGR. vs. 173462 test-amd64-amd64-dom0pvh-xl-amd 14 guest-start fail REGR. vs. 173462 test-arm64-arm64-examine 8 reboot fail REGR. vs. 173462 test-amd64-amd64-dom0pvh-xl-intel 14 guest-start fail REGR. vs. 173462 test-arm64-arm64-xl-credit2 8 xen-boot fail REGR. vs. 173462 test-amd64-amd64-xl-xsm 8 xen-boot fail REGR. vs. 173462 test-amd64-amd64-libvirt-xsm 8 xen-boot fail REGR. vs. 173462 test-amd64-amd64-xl 8 xen-boot fail REGR. vs. 173462 test-amd64-amd64-xl-qemuu-ovmf-amd64 8 xen-boot fail REGR. vs. 173462 test-armhf-armhf-xl 8 xen-boot fail REGR. vs. 173462 test-armhf-armhf-xl-credit2 8 xen-boot fail REGR. vs. 173462 test-armhf-armhf-examine 8 reboot fail REGR. vs. 173462 test-amd64-amd64-xl-credit1 8 xen-boot fail REGR. vs. 173462 test-armhf-armhf-libvirt 8 xen-boot fail REGR. vs. 173462 test-amd64-amd64-examine-uefi 8 reboot fail REGR. vs. 173462 test-amd64-amd64-examine-bios 8 reboot fail REGR. vs. 173462 test-amd64-amd64-examine 8 reboot fail REGR. vs. 173462 test-amd64-amd64-pygrub 8 xen-boot fail REGR. vs. 173462 test-amd64-amd64-libvirt-raw 8 xen-boot fail REGR. vs. 173462 test-arm64-arm64-xl-xsm 8 xen-boot fail REGR. vs. 173462 test-arm64-arm64-xl-seattle 8 xen-boot fail REGR. vs. 173462 test-arm6
Re: [XEN v3] xen/arm: Probe the entry point address of an uImage correctly
On 20/12/2022 12:14, Julien Grall wrote: Hi Ayan, Hi Julien, On 20/12/2022 10:44, Ayan Kumar Halder wrote: + + /* + * Currently, we support uImage headers for uncompressed images only. + * Thus, it is valid for the load address and start address to be the + * same. This is consistent with the uboot behavior (Refer + * "case IH_COMP_NONE" in image_decomp(). Please make it clear that you are referring to uboot function. Could we avoid to mention the u-boot function? The reason I am asking is the code is in a different repo and the function name can easily change without us noticing (so the comment would rot). Is the behavior documented somewhere in U-boot (or online)? If not, what's the guarantee that it will not change? I could not find any documentation which states this. I found this from the following in uboot source code. https://source.denx.de/u-boot/u-boot/-/blob/master/boot/image.c#L458 AFAIU when kernel_uimage_probe() get invoked, the image has already been decompressed. So, I added this as a limitation. I e looked at the U-boot code and, AFAIU, the check is merely to avoid the memcpy() if the image start matches the start of the decompression area. So I don't understand why we need the limitation in Xen. Now the lack of documentation (or at least I didn't find any) makes a bit more tricky to understand what the fields in the U-boot header are for. I have skimmed through the code and I disagree with the implementation you chose for Xen. The comment for 'ih_ep' suggests this is the entry point address. That's may be different from the beginning of your blob. I think this is what ih_load is for. So I think we want to load the blob at ih_load and then set pc to point to ih_ep. This will require a bit more work in Xen because the assumption so far is the entry point matches the start of the blob. Please let me known if I missed anything. I think you are correct. I will rework this to correctly handle load address and entry point. - Ayan Cheers,
Re: [XEN v3] xen/arm: Probe the entry point address of an uImage correctly
Hi Ayan, On 20/12/2022 10:44, Ayan Kumar Halder wrote: + + /* + * Currently, we support uImage headers for uncompressed images only. + * Thus, it is valid for the load address and start address to be the + * same. This is consistent with the uboot behavior (Refer + * "case IH_COMP_NONE" in image_decomp(). Please make it clear that you are referring to uboot function. Could we avoid to mention the u-boot function? The reason I am asking is the code is in a different repo and the function name can easily change without us noticing (so the comment would rot). Is the behavior documented somewhere in U-boot (or online)? If not, what's the guarantee that it will not change? I could not find any documentation which states this. I found this from the following in uboot source code. https://source.denx.de/u-boot/u-boot/-/blob/master/boot/image.c#L458 AFAIU when kernel_uimage_probe() get invoked, the image has already been decompressed. So, I added this as a limitation. I e looked at the U-boot code and, AFAIU, the check is merely to avoid the memcpy() if the image start matches the start of the decompression area. So I don't understand why we need the limitation in Xen. Now the lack of documentation (or at least I didn't find any) makes a bit more tricky to understand what the fields in the U-boot header are for. I have skimmed through the code and I disagree with the implementation you chose for Xen. The comment for 'ih_ep' suggests this is the entry point address. That's may be different from the beginning of your blob. I think this is what ih_load is for. So I think we want to load the blob at ih_load and then set pc to point to ih_ep. This will require a bit more work in Xen because the assumption so far is the entry point matches the start of the blob. Please let me known if I missed anything. Cheers, -- Julien Grall
(Ab)using xenstored without Xen
I've been working on getting qemu to support Xen HVM guests 'natively'
under KVM:
https://lore.kernel.org/qemu-devel/20221216004117.862106-1-dw...@infradead.org/T/
The basic platform is mostly working and I can start XTF tests with
'qemu -kernel'. Now it really needs a xenstore.
I'm thinking of implementing the basic shared ring support on the qemu
side, then communicating with the real xenstored over its socket
interface. It would need a 'SU' command in the xenstored protocol to
make it treat that connection as an unprivileged connection from a
specific domid, analogous to 'INTRODUCE' but over the existing
connection.
However, there might be a bit of work to do first. At first, it seemed
like xenstored did start up OK and qemu could even connect to it when
not running under Xen. But that was a checkout from a few months ago,
and even then it would segfault the first time we try to actually
*write* any nodes.
Newer xenstored breaks even sooner because since commit 60e2f6020
("tools/xenstore: move the call of setup_structure() to dom0
introduction") it doesn't even have a tdb_ctx if you start it with the
-D option, so it segfaults even on running xenstore-ls. And if I move
the tdb part of setup_structure() back to be called from where it was,
we get a later crash in get_domain_info() because the xc_handle is
NULL.
Which is kind of fair enough, because xenstored is designed to run
under Xen :)
But what *is* the -D option for? It goes back to commit bddd41366 in
2005 whch talks about allowing multiple concurrent transactions, and
doesn't mention the new option at all. It seems to be fairly hosed at
the moment.
Is it reasonable to attempt "fixing" xenstored to run without actual
Xen, so that we can use it for virtual Xen support?
smime.p7s
Description: S/MIME cryptographic signature
Re: [RFC 5/7] x86/iommu: the code addressing CVE-2011-1898 is VT-d specific
On 19/12/2022 6:34 am, Xenia Ragiadakou wrote: > The variable untrusted_msi indicates whether the system is vulnerable to > CVE-2011-1898. This vulnerablity is VT-d specific. > Place the code that addresses the issue under CONFIG_INTEL_VTD. > > No functional change intended. > > Signed-off-by: Xenia Ragiadakou Actually, this variable is pretty bogus. I think I'd like to delete it entirely. There are systems with no IOMMU at all, and we certainly used to let PV Passthrough go ahead. (Not sure we do any more.) There are systems with DMA remapping only, but no interrupt remapping. These are known insecure. I'm honestly not convinced that an ISR read and crash is useful when the user has already constructed an known-unsafe configuration, because a malicious guest in that case can still fully mess with dom0 by sending vectors other than 0x80 and 0x82. In particular, this option does not get activated on AMD when the user elects to disable interrupt remapping, and I'm disinclined to wire it up in that case too. ~Andrew P.S. It occurs to me that FRED obsoletes the need for this anyway, seeing as it does properly distinguish the source of an event.
Re: [XEN v3] xen/arm: Probe the entry point address of an uImage correctly
Hi Julien/Michal,
On 20/12/2022 10:05, Julien Grall wrote:
On 20/12/2022 09:44, Michal Orzel wrote:
Hi Ayan,
On 17/12/2022 20:38, Ayan Kumar Halder wrote:
Currently, kernel_uimage_probe() does not set info->zimage.start. As a
result, it contains the default value (ie 0). This causes,
kernel_zimage_place() to treat the binary (contained within uImage) as
position independent executable. Thus, it loads it at an incorrect
address.
The correct approach would be to read "uimage.ep" and set
info->zimage.start. This will ensure that the binary is loaded at the
correct address. Also, it checks that the load address and entry
address
are the same. The reason being we currently support non compressed
images
for uImage header. And as seen in uboot sources(image_decomp(), case
IH_COMP_NONE), load address and entry address can be the same.
This behavior is applicable for both arm and arm64 platforms.
Earlier for arm32 and arm64 platforms, Xen was ignoring the entry point
address set in the uImage header. With this commit, Xen will use the
kernel entry point address as specified in the header. This makes the
behavior of Xen consistent with uboot for uimage headers.
Users who want to use Xen with statically partitioned domains, can
provide the fixed non zero load address for the dom0/domU kernel.
A deviation from uboot behaviour is that we consider load address ==
0x0,
to denote that the image supports position independent execution. This
is to make the behavior consistent across uImage and zImage.
Signed-off-by: Ayan Kumar Halder
---
Changes from v1 :-
1. Added a check to ensure load address and entry address are the same.
2. Considered load address == 0x0 as position independent execution.
3. Ensured that the uImage header interpretation is consistent across
arm32 and arm64.
v2 :-
1. Mentioned the change in existing behavior in booting.txt.
2. Updated booting.txt with a new section to document "Booting Guests".
docs/misc/arm/booting.txt | 21 +
xen/arch/arm/include/asm/kernel.h | 2 +-
xen/arch/arm/kernel.c | 27 ++-
3 files changed, 48 insertions(+), 2 deletions(-)
diff --git a/docs/misc/arm/booting.txt b/docs/misc/arm/booting.txt
index 3e0c03e065..01b12b49a5 100644
--- a/docs/misc/arm/booting.txt
+++ b/docs/misc/arm/booting.txt
@@ -23,6 +23,24 @@ The exceptions to this on 32-bit ARM are as follows:
There are no exception on 64-bit ARM.
+Booting Guests
+--
+
+Xen supports the legacy image protocol[3], zImage protocol for
32-bit ARM
uImage is not a protocol. It is just a header with no other
information \wrt
boot requirements.
+Linux[1] and Image protocol defined for ARM64[2].
+
+Earlier for legacy image protocol, Xen ignored the load address and
entry
+point specified in the header. This has now changed.
+
+Now, it loads the image at the load address provided in the header.
+For now, it supports images where load address is same as entry point.
Would be beneficial to add explanation why load address must be equal
to start address.
Answered below.
+
+A deviation from uboot is that, Xen treats "load address == 0x0" as
+position independent execution. Thus, Xen will load such an image
at an
+address it considers appropriate.
+
+Users who want to use Xen with statically partitioned domains, can
provide
+the fixed non zero load address for the dom0/domU kernel.
I think this section is missing a note that in case of not PIE, a
load/start address
specified by the user in a header must be within the memory region
allocated by Xen.
Firmware/bootloader requirements
@@ -39,3 +57,6 @@ Latest version:
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/t
[2] linux/Documentation/arm64/booting.rst
Latest version:
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/arm64/booting.rst
+
+[3] legacy format header
+Latest version:
https://source.denx.de/u-boot/u-boot/-/blob/master/include/image.h#L315
diff --git a/xen/arch/arm/include/asm/kernel.h
b/xen/arch/arm/include/asm/kernel.h
index 5bb30c3f2f..4617cdc83b 100644
--- a/xen/arch/arm/include/asm/kernel.h
+++ b/xen/arch/arm/include/asm/kernel.h
@@ -72,7 +72,7 @@ struct kernel_info {
#ifdef CONFIG_ARM_64
paddr_t text_offset; /* 64-bit Image only */
#endif
- paddr_t start; /* 32-bit zImage only */
+ paddr_t start; /* Must be 0 for 64-bit Image */
} zimage;
};
};
diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index 23b840ea9e..e9c18993ef 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -127,7 +127,7 @@ static paddr_t __init kernel_zimage_place(struct
kernel_info *info)
paddr_t load_addr;
#ifdef CONFIG_ARM_64
- if ( info->type == DOMAIN_64BIT )
+ if ( (info->type == DOMAIN_64BIT) && (info->zimage.start == 0) )
return info->mem.bank[0].start + info->zima
No package 'pixman-1' found
Hello, How to solve the pixman error: $ sudo ./configure checking build system type... x86_64-pc-solaris2.11 checking host system type... x86_64-pc-solaris2.11 Will build the following subsystems: xen tools stubdom docs configure: creating ./config.status config.status: creating config/Toplevel.mk config.status: creating config/Paths.mk === configuring in tools (/home/Hypervisor/xen-4.16.2/tools) configure: running /bin/sh ./configure --disable-option-checking '--prefix=/usr/local' --cache-file=/dev/null --srcdir=. checking build system type... x86_64-pc-solaris2.11 checking host system type... x86_64-pc-solaris2.11 checking for gcc... gcc checking whether the C compiler works... yes checking for C compiler default output file name... a.out checking for suffix of executables... checking whether we are cross compiling... no checking for suffix of object files... o checking whether we are using the GNU C compiler... yes checking whether gcc accepts -g... yes checking for gcc option to accept ISO C89... none needed checking for special C compiler options needed for large files... no checking for _FILE_OFFSET_BITS value needed for large files... no checking for gcc... (cached) gcc checking whether we are using the GNU C compiler... (cached) yes checking whether gcc accepts -g... (cached) yes checking for gcc option to accept ISO C89... (cached) none needed checking whether make sets $(MAKE)... yes checking for a BSD-compatible install... /usr/bin/install -c checking for flex... /usr/bin/flex checking for abi-dumper... no checking for perl... /usr/bin/perl checking for awk... /usr/bin/awk checking for ocamlc... no checking for ocaml... no checking for ocamldep... no checking for ocamlmktop... no checking for ocamlmklib... no checking for ocamldoc... no checking for ocamlbuild... no checking for ocamlfind... no checking for gawk... /usr/bin/awk checking for go... no checking for checkpolicy... no checking for bash... /usr/bin/bash checking for python3... python3 checking for python3... /usr/bin/python3 checking for python3... /usr/bin/python3 checking for python version >= 2.6 ... yes checking how to run the C preprocessor... gcc -E checking for grep that handles long lines and -e... /usr/bin/ggrep checking for egrep... /usr/bin/ggrep -E checking for ANSI C header files... yes checking for sys/types.h... yes checking for sys/stat.h... yes checking for stdlib.h... yes checking for string.h... yes checking for memory.h... yes checking for strings.h... yes checking for inttypes.h... yes checking for stdint.h... yes checking for unistd.h... yes checking for python3-config... /usr/bin/python3-config checking Python.h usability... yes checking Python.h presence... yes checking for Python.h... yes checking for PyArg_ParseTuple... yes checking whether Python setup.py brokenly enables -D_FORTIFY_SOURCE... no checking for iasl... /usr/sbin/iasl checking uuid/uuid.h usability... yes checking uuid/uuid.h presence... yes checking for uuid/uuid.h... yes checking for uuid_clear in -luuid... yes checking uuid.h usability... no checking uuid.h presence... no checking for uuid.h... no checking curses.h usability... yes checking curses.h presence... yes checking for curses.h... yes checking for clear in -lcurses... yes checking ncurses.h usability... no checking ncurses.h presence... no checking for ncurses.h... no checking for pkg-config... /usr/bin/pkg-config checking pkg-config is at least version 0.9.0... yes checking for glib... yes checking for pixman... no configure: error: Package requirements (pixman-1 >= 0.21.8) were not met: No package 'pixman-1' found Consider adjusting the PKG_CONFIG_PATH environment variable if you installed software in a non-standard prefix. Alternatively, you may set the environment variables pixman_CFLAGS and pixman_LIBS to avoid the need to call pkg-config. See the pkg-config man page for more details. configure: error: ./configure failed for tools Thank you.
Re: [RFC 2/7] x86/iommu: amd_iommu_perdev_intremap is AMD-Vi specific
On 19/12/2022 6:34 am, Xenia Ragiadakou wrote:
> diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
> index 5e2a720d29..1a02fdc453 100644
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -58,7 +58,6 @@ bool __read_mostly iommu_hap_pt_share = true;
> #endif
>
> bool_t __read_mostly iommu_debug;
> -bool_t __read_mostly amd_iommu_perdev_intremap = 1;
>
> DEFINE_PER_CPU(bool_t, iommu_dont_flush_iotlb);
>
> @@ -115,8 +114,10 @@ static int __init cf_check parse_iommu_param(const char
> *s)
> if ( val )
> iommu_verbose = 1;
> }
> +#ifdef CONFIG_AMD_IOMMU
> else if ( (val = parse_boolean("amd-iommu-perdev-intremap", s, ss))
> >= 0 )
> amd_iommu_perdev_intremap = val;
> +#endif
See parse_cet() and the use of no_config_param() so users get a bit of a
hint as to why the option they specified is getting ignored.
~Andrew
Re: [RFC 1/7] x86/iommu: make AMD-Vi and Intel VT-d support configurable
On 19/12/2022 6:34 am, Xenia Ragiadakou wrote: > diff --git a/xen/drivers/passthrough/Kconfig b/xen/drivers/passthrough/Kconfig > index 479d7de57a..82465aa627 100644 > --- a/xen/drivers/passthrough/Kconfig > +++ b/xen/drivers/passthrough/Kconfig > @@ -37,6 +37,22 @@ config IPMMU_VMSA > > endif > > +config AMD_IOMMU > + bool "AMD IOMMU" > + depends on X86 > + default y > + ---help--- We're trying to phase out ---help---, so please just use help. ~Andrew
[ovmf test] 175420: regressions - FAIL
flight 175420 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/175420/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-amd64-i386-xl-qemuu-ovmf-amd64 12 debian-hvm-install fail REGR. vs. 175202 test-amd64-amd64-xl-qemuu-ovmf-amd64 12 debian-hvm-install fail REGR. vs. 175202 version targeted for testing: ovmf ceb52713b00a4bec1a4ab551636b8d297139ea6f baseline version: ovmf d103840cfb559c28831c2635b916d60118f671cc Last test of basis 175202 2022-12-14 13:42:59 Z5 days Failing since175214 2022-12-14 18:42:16 Z5 days 35 attempts Testing same since 175420 2022-12-20 06:24:00 Z0 days1 attempts People who touched revisions under test: Adam Dunlap Ard Biesheuvel Chun-Yi Lee Chun-Yi Lee de...@edk2.groups.io Dov Murik Gerd Hoffmann jdzhang Jeff Brasen Jeshua Smith Jian J Wang Jiaqi Gao Jiewen Yao Judah Vang Kuo, Ted MarsX Lin Michael Kubacki Min M Xu Min Xu Nishant C Mistry Sebastien Boeuf Ted Kuo Tom Lendacky jobs: build-amd64-xsm pass build-i386-xsm pass build-amd64 pass build-i386 pass build-amd64-libvirt pass build-i386-libvirt pass build-amd64-pvopspass build-i386-pvops pass test-amd64-amd64-xl-qemuu-ovmf-amd64 fail test-amd64-i386-xl-qemuu-ovmf-amd64 fail sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Not pushing. (No revision log; it would be 779 lines long.)
Re: [RFC 0/7] Proposal to make x86 IOMMU driver support configurable
On 19.12.2022 19:28, Andrew Cooper wrote: > IOMMUs are more tricky still. They are (for most intents and purposes) > mandatory on systems with >254 CPUs. We could in principle start > supporting asymmetric IRQ routing on large systems, but Xen doesn't > currently and it would be a lot work that's definitely not high on the > priority list. At a minimum, this will need expressing in the Kconfig > help text. > > We need to decide whether it is sensible to allow users to turn off > IOMMU support. It probably is, because you could trivially do this by > selecting CONFIG_INTEL only, and booting the result on an AMD system. One other thing Andrew and I have been talking about: We probably do not want to tie the IOMMU vendor control to CPU vendor one. IOW we'd like to be able to e.g. build a hypervisor supporting Intel (only) as the CPU vendor, but at the same time having support for an AMD IOMMU. > For the names, I don't think AMD_IOMMU vs INTEL_VTD is a sensible. > Probably wants to be INTEL_IOMMU to match. Or simply VTD, covering the case than some other vendor comes up with a clone. But of course we have that same issue with "AMD" and Hygon ... Jan
Re: [XEN v3] xen/arm: Probe the entry point address of an uImage correctly
On 20/12/2022 09:44, Michal Orzel wrote:
Hi Ayan,
On 17/12/2022 20:38, Ayan Kumar Halder wrote:
Currently, kernel_uimage_probe() does not set info->zimage.start. As a
result, it contains the default value (ie 0). This causes,
kernel_zimage_place() to treat the binary (contained within uImage) as
position independent executable. Thus, it loads it at an incorrect address.
The correct approach would be to read "uimage.ep" and set
info->zimage.start. This will ensure that the binary is loaded at the
correct address. Also, it checks that the load address and entry address
are the same. The reason being we currently support non compressed images
for uImage header. And as seen in uboot sources(image_decomp(), case
IH_COMP_NONE), load address and entry address can be the same.
This behavior is applicable for both arm and arm64 platforms.
Earlier for arm32 and arm64 platforms, Xen was ignoring the entry point
address set in the uImage header. With this commit, Xen will use the
kernel entry point address as specified in the header. This makes the
behavior of Xen consistent with uboot for uimage headers.
Users who want to use Xen with statically partitioned domains, can
provide the fixed non zero load address for the dom0/domU kernel.
A deviation from uboot behaviour is that we consider load address == 0x0,
to denote that the image supports position independent execution. This
is to make the behavior consistent across uImage and zImage.
Signed-off-by: Ayan Kumar Halder
---
Changes from v1 :-
1. Added a check to ensure load address and entry address are the same.
2. Considered load address == 0x0 as position independent execution.
3. Ensured that the uImage header interpretation is consistent across
arm32 and arm64.
v2 :-
1. Mentioned the change in existing behavior in booting.txt.
2. Updated booting.txt with a new section to document "Booting Guests".
docs/misc/arm/booting.txt | 21 +
xen/arch/arm/include/asm/kernel.h | 2 +-
xen/arch/arm/kernel.c | 27 ++-
3 files changed, 48 insertions(+), 2 deletions(-)
diff --git a/docs/misc/arm/booting.txt b/docs/misc/arm/booting.txt
index 3e0c03e065..01b12b49a5 100644
--- a/docs/misc/arm/booting.txt
+++ b/docs/misc/arm/booting.txt
@@ -23,6 +23,24 @@ The exceptions to this on 32-bit ARM are as follows:
There are no exception on 64-bit ARM.
+Booting Guests
+--
+
+Xen supports the legacy image protocol[3], zImage protocol for 32-bit ARM
uImage is not a protocol. It is just a header with no other information \wrt
boot requirements.
+Linux[1] and Image protocol defined for ARM64[2].
+
+Earlier for legacy image protocol, Xen ignored the load address and entry
+point specified in the header. This has now changed.
+
+Now, it loads the image at the load address provided in the header.
+For now, it supports images where load address is same as entry point.
Would be beneficial to add explanation why load address must be equal to start
address.
+
+A deviation from uboot is that, Xen treats "load address == 0x0" as
+position independent execution. Thus, Xen will load such an image at an
+address it considers appropriate.
+
+Users who want to use Xen with statically partitioned domains, can provide
+the fixed non zero load address for the dom0/domU kernel.
I think this section is missing a note that in case of not PIE, a load/start
address
specified by the user in a header must be within the memory region allocated by
Xen.
Firmware/bootloader requirements
@@ -39,3 +57,6 @@ Latest version:
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/t
[2] linux/Documentation/arm64/booting.rst
Latest version:
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/arm64/booting.rst
+
+[3] legacy format header
+Latest version:
https://source.denx.de/u-boot/u-boot/-/blob/master/include/image.h#L315
diff --git a/xen/arch/arm/include/asm/kernel.h
b/xen/arch/arm/include/asm/kernel.h
index 5bb30c3f2f..4617cdc83b 100644
--- a/xen/arch/arm/include/asm/kernel.h
+++ b/xen/arch/arm/include/asm/kernel.h
@@ -72,7 +72,7 @@ struct kernel_info {
#ifdef CONFIG_ARM_64
paddr_t text_offset; /* 64-bit Image only */
#endif
-paddr_t start; /* 32-bit zImage only */
+paddr_t start; /* Must be 0 for 64-bit Image */
} zimage;
};
};
diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index 23b840ea9e..e9c18993ef 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -127,7 +127,7 @@ static paddr_t __init kernel_zimage_place(struct
kernel_info *info)
paddr_t load_addr;
#ifdef CONFIG_ARM_64
-if ( info->type == DOMAIN_64BIT )
+if ( (info->type == DOMAIN_64BIT) && (info->zimage.start == 0) )
return info->mem.bank[0].start + info->zimage.text_offset;
#endif
@@ -223,6 +223,31 @@ static int __init kernel_uimage_probe(str
Re: [PATCH RFC 04/10] domain: update GADDR based runstate guest area
On 20/12/2022 09:59, Jan Beulich wrote: On 20.12.2022 09:48, Julien Grall wrote: Hi Jan, On 20/12/2022 08:45, Jan Beulich wrote: On 20.12.2022 09:40, Julien Grall wrote: On 19/12/2022 12:48, Jan Beulich wrote: On 16.12.2022 13:26, Julien Grall wrote: On 19/10/2022 08:41, Jan Beulich wrote: RFC: HVM guests (on x86) can change bitness and hence layout (and size! and alignment) of the runstate area. I don't think it is an option to require 32-bit code to pass a range such that even the 64-bit layout wouldn't cross a page boundary (and be suitably aligned). I also don't see any other good solution, so for now a crude approach with an extra boolean is used (using has_32bit_shinfo() isn't race free and could hence lead to overrunning the mapped space). I think the extra check for 32-bit code to pass the check for 64-bit layout would be better. I'm afraid I can't derive from your reply what it is you actually want. I think for 32-bit call, we also want to check the address provide will also pass the 64-bit check (i.e. if used as a 64-bit layout, the area would not cross a page boundary and be suitably aligned). But that's specifically what I say I don't think is an option. First and foremost because of the implication on 32-bit callers: They're need to use magic to get hold of the size of the 64-bit variant of the struct. I understand that. But I am not aware of any other (simple) approach where you could have race free code. My reference to using has_32bit_shinfo() not being race free was to avoid suggestions in that direction. There's no use of this function in the patch here, nor in the one where the new boolean field is actually written to. The solution as presented is, afaict, both simple and race free. It merely isn't very nice. Ah! I misunderstood what you original wrote. Thanks for the clarification. Cheers, -- Julien Grall
Re: [PATCH RFC 04/10] domain: update GADDR based runstate guest area
On 20.12.2022 09:48, Julien Grall wrote: > Hi Jan, > > On 20/12/2022 08:45, Jan Beulich wrote: >> On 20.12.2022 09:40, Julien Grall wrote: >>> On 19/12/2022 12:48, Jan Beulich wrote: On 16.12.2022 13:26, Julien Grall wrote: > On 19/10/2022 08:41, Jan Beulich wrote: >> RFC: HVM guests (on x86) can change bitness and hence layout (and size! >> and alignment) of the runstate area. I don't think it is an >> option >> to require 32-bit code to pass a range such that even the 64-bit >> layout wouldn't cross a page boundary (and be suitably aligned). >> I >> also don't see any other good solution, so for now a crude >> approach >> with an extra boolean is used (using has_32bit_shinfo() isn't >> race >> free and could hence lead to overrunning the mapped space). > > I think the extra check for 32-bit code to pass the check for 64-bit > layout would be better. I'm afraid I can't derive from your reply what it is you actually want. >>> >>> I think for 32-bit call, we also want to check the address provide will >>> also pass the 64-bit check (i.e. if used as a 64-bit layout, the area >>> would not cross a page boundary and be suitably aligned). >> >> But that's specifically what I say I don't think is an option. First and >> foremost because of the implication on 32-bit callers: They're need to >> use magic to get hold of the size of the 64-bit variant of the struct. > > I understand that. But I am not aware of any other (simple) approach > where you could have race free code. My reference to using has_32bit_shinfo() not being race free was to avoid suggestions in that direction. There's no use of this function in the patch here, nor in the one where the new boolean field is actually written to. The solution as presented is, afaict, both simple and race free. It merely isn't very nice. Jan > So between a non-race free code and exposing the restriction to the > guest, I would chose the latter. > > Cheers, >
Re: [RFC PATCH 05/18] arm: cppcheck: fix misra rule 20.7 on arm/include/asm/string.h
Hi Jan, On 20/12/2022 09:38, Jan Beulich wrote: On 20.12.2022 10:12, Julien Grall wrote: On 20/12/2022 08:50, Luca Fancellu wrote: Cppcheck has found a violation of rule 20.7 for the macro memset about missing parenthesis for the "n" argument, while the parenthesis are not mandatory because the argument is never used in an expression, adding them will not harm code and readability, so fix the finding adding parenthesis for the argument. This is something I have argued against in the past (see [1]). So... Eclair and coverity does not report this finding. ... if neither Eclair nor Coverity report it then I think this should be a bug report against Cppcheck. Furthermore in reply to my "Arm32: tidy the memset() macro" you said [1] "In this case, Linux has removed __memzero() is patch ff5fdafc9e97 "ARM: 8745/1: get rid of __memzero()" because the performance difference with memset() was limited. For Xen, I think we should also remove the function." So either you want to follow that route, or it would rather be my patch which ought to be considered for merging, not the least because it also deals with yet another MISRA violation. I forgot that discussion, thanks for the reminder! I would still prefer if we port the Linux change to Xen. Cheers, -- Julien Grall
Re: [RFC PATCH 00/18] cppcheck rule 20.7 fixes
On 20.12.2022 09:50, Luca Fancellu wrote: > In this serie there are some fixes for the rule 20.7, mainly violation found > by > cppcheck, most of them are false positive but some of them can be fixed. > > The analysed build is arm64, to reproduce the reports here the command: > > ./xen/scripts/xen-analysis.py --cppcheck-misra --run-cppcheck -- > CROSS_COMPILE="aarch64-linux-gnu-" XEN_TARGET_ARCH="arm64" > O=/path/to/artifacts_folder > > Luca Fancellu (18): > arm: cppcheck: misra rule 20.7 deviations for alternative.h > arm: cppcheck: misra rule 20.7 deviation on processor.h > arm: cppcheck: misra rule 20.7 deviation on asm_defns.h > arm: cppcheck: misra rule 20.7 deviation on config.h > arm: cppcheck: fix misra rule 20.7 on arm/include/asm/string.h > public: cppcheck: misra rule 20.7 on public/arch-arm.h > xen: cppcheck: misra rule 20.7 deviation on compiler.h > xen: cppcheck: misra rule 20.7 deviation on init.h > xen: cppcheck: misra rule 20.7 deviation on kconfig.h > xen: cppcheck: misra rule 20.7 deviation on types.h > xen: cppcheck: misra rule 20.7 deviation on xmalloc.h > arm: cppcheck: misra rule 20.7 deviation on asm/arm64/sysregs.h > public/x86: cppcheck: misra rule 20.7 deviation on hvm/save.h > public/x86: cppcheck: misra rule 20.7 deviation on xen-x86_32.h > public/x86: cppcheck: misra rule 20.7 deviation on xen-x86_64.h > public/x86: cppcheck: misra rule 20.7 deviation on arch-x86/xen.h > public: misra rule 20.7 deviation on errno.h > public: misra rule 20.7 deviation on memory.h Like Julien I object to the massive addition of false positive markers just because of very basic shortcomings in cppcheck. I find this particularly bad in public headers - imo no such annotations should appear there at all. I would suggest that you split off the actual code changes, which are likely going to be less controversial. Jan
Re: [RFC PATCH 18/18] public: misra rule 20.7 deviation on memory.h
On 20.12.2022 09:51, Luca Fancellu wrote: > Cppcheck has found a violation of rule 20.7 for the macro > XENMEM_SHARING_OP_FIELD_MAKE_GREF, the argument "val" is used in an > expression, hence add parenthesis to the argument "val" to fix the > violation. > > Signed-off-by: Luca Fancellu Acked-by: Jan Beulich again with an adjustment to the title. Jan
Re: [RFC PATCH 17/18] public: misra rule 20.7 deviation on errno.h
On 20.12.2022 09:50, Luca Fancellu wrote: > Cppcheck has found a violation of rule 20.7 for the macro XEN_ERRNO, > while the macro parameter is never used as an expression, it doesn't > harm the code or the readability to add parenthesis, so add them. > > This finding is reported also by eclair and coverity. > > Signed-off-by: Luca Fancellu Acked-by: Jan Beulich But with the title adjusted - this isn't about a deviation, but actually addressing a finding. Jan
Re: [RFC PATCH 09/18] xen: cppcheck: misra rule 20.7 deviation on kconfig.h
On 20.12.2022 09:50, Luca Fancellu wrote:
> Cppcheck has found a violation of rule 20.7 for the macro
> __config_enabled but the preprocessor branch where this macro is
> defined should not be analysed by cppcheck when CPPCHECK macro is
> defined, hence this is a false positive of the tool and we can
> safely suppress the finding.
So what was commit 43aa3f6e72d3's ("xen/build: Add cppcheck and
cppcheck-html make rules") adjustment to the file about then?
Jan
Re: [XEN v3] xen/arm: Probe the entry point address of an uImage correctly
Hi Ayan,
On 17/12/2022 20:38, Ayan Kumar Halder wrote:
> Currently, kernel_uimage_probe() does not set info->zimage.start. As a
> result, it contains the default value (ie 0). This causes,
> kernel_zimage_place() to treat the binary (contained within uImage) as
> position independent executable. Thus, it loads it at an incorrect address.
>
> The correct approach would be to read "uimage.ep" and set
> info->zimage.start. This will ensure that the binary is loaded at the
> correct address. Also, it checks that the load address and entry address
> are the same. The reason being we currently support non compressed images
> for uImage header. And as seen in uboot sources(image_decomp(), case
> IH_COMP_NONE), load address and entry address can be the same.
>
> This behavior is applicable for both arm and arm64 platforms.
>
> Earlier for arm32 and arm64 platforms, Xen was ignoring the entry point
> address set in the uImage header. With this commit, Xen will use the
> kernel entry point address as specified in the header. This makes the
> behavior of Xen consistent with uboot for uimage headers.
>
> Users who want to use Xen with statically partitioned domains, can
> provide the fixed non zero load address for the dom0/domU kernel.
>
> A deviation from uboot behaviour is that we consider load address == 0x0,
> to denote that the image supports position independent execution. This
> is to make the behavior consistent across uImage and zImage.
>
> Signed-off-by: Ayan Kumar Halder
> ---
>
> Changes from v1 :-
> 1. Added a check to ensure load address and entry address are the same.
> 2. Considered load address == 0x0 as position independent execution.
> 3. Ensured that the uImage header interpretation is consistent across
> arm32 and arm64.
>
> v2 :-
> 1. Mentioned the change in existing behavior in booting.txt.
> 2. Updated booting.txt with a new section to document "Booting Guests".
>
> docs/misc/arm/booting.txt | 21 +
> xen/arch/arm/include/asm/kernel.h | 2 +-
> xen/arch/arm/kernel.c | 27 ++-
> 3 files changed, 48 insertions(+), 2 deletions(-)
>
> diff --git a/docs/misc/arm/booting.txt b/docs/misc/arm/booting.txt
> index 3e0c03e065..01b12b49a5 100644
> --- a/docs/misc/arm/booting.txt
> +++ b/docs/misc/arm/booting.txt
> @@ -23,6 +23,24 @@ The exceptions to this on 32-bit ARM are as follows:
>
> There are no exception on 64-bit ARM.
>
> +Booting Guests
> +--
> +
> +Xen supports the legacy image protocol[3], zImage protocol for 32-bit ARM
uImage is not a protocol. It is just a header with no other information \wrt
boot requirements.
> +Linux[1] and Image protocol defined for ARM64[2].
> +
> +Earlier for legacy image protocol, Xen ignored the load address and entry
> +point specified in the header. This has now changed.
> +
> +Now, it loads the image at the load address provided in the header.
> +For now, it supports images where load address is same as entry point.
Would be beneficial to add explanation why load address must be equal to start
address.
> +
> +A deviation from uboot is that, Xen treats "load address == 0x0" as
> +position independent execution. Thus, Xen will load such an image at an
> +address it considers appropriate.
> +
> +Users who want to use Xen with statically partitioned domains, can provide
> +the fixed non zero load address for the dom0/domU kernel.
I think this section is missing a note that in case of not PIE, a load/start
address
specified by the user in a header must be within the memory region allocated by
Xen.
>
> Firmware/bootloader requirements
>
> @@ -39,3 +57,6 @@ Latest version:
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/t
>
> [2] linux/Documentation/arm64/booting.rst
> Latest version:
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/arm64/booting.rst
> +
> +[3] legacy format header
> +Latest version:
> https://source.denx.de/u-boot/u-boot/-/blob/master/include/image.h#L315
> diff --git a/xen/arch/arm/include/asm/kernel.h
> b/xen/arch/arm/include/asm/kernel.h
> index 5bb30c3f2f..4617cdc83b 100644
> --- a/xen/arch/arm/include/asm/kernel.h
> +++ b/xen/arch/arm/include/asm/kernel.h
> @@ -72,7 +72,7 @@ struct kernel_info {
> #ifdef CONFIG_ARM_64
> paddr_t text_offset; /* 64-bit Image only */
> #endif
> -paddr_t start; /* 32-bit zImage only */
> +paddr_t start; /* Must be 0 for 64-bit Image */
> } zimage;
> };
> };
> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
> index 23b840ea9e..e9c18993ef 100644
> --- a/xen/arch/arm/kernel.c
> +++ b/xen/arch/arm/kernel.c
> @@ -127,7 +127,7 @@ static paddr_t __init kernel_zimage_place(struct
> kernel_info *info)
> paddr_t load_addr;
>
> #ifdef CONFIG_ARM_64
> -if ( info->type == DOMAIN_64BIT )
> +if ( (info->type == DOMAIN_64BIT) && (info->zimage.start == 0) )
>
Re: [RFC PATCH 08/18] xen: cppcheck: misra rule 20.7 deviation on init.h
On 20.12.2022 09:50, Luca Fancellu wrote:
> --- a/xen/include/xen/init.h
> +++ b/xen/include/xen/init.h
> @@ -15,6 +15,7 @@
> #define __initconstrel__section(".init.rodata.rel")
> #define __exitdata__used_section(".exit.data")
> #define __initsetup __used_section(".init.setup")
> +/* SAF-1-false-positive-cppcheck R20.7 argument as text substitution */
> #define __init_call(lvl) __used_section(".initcall" lvl ".init")
Would cppcheck also complain about
#define __init_call(lvl) __used_section(".initcall" #lvl ".init")
? If not, removing the double quotes at the two invocation sites to
balance the addition of # here might be the better route here.
Jan
Re: [RFC PATCH 05/18] arm: cppcheck: fix misra rule 20.7 on arm/include/asm/string.h
On 20.12.2022 10:12, Julien Grall wrote: > On 20/12/2022 08:50, Luca Fancellu wrote: >> Cppcheck has found a violation of rule 20.7 for the macro memset >> about missing parenthesis for the "n" argument, while the parenthesis >> are not mandatory because the argument is never used in an >> expression, adding them will not harm code and readability, so fix >> the finding adding parenthesis for the argument. > > This is something I have argued against in the past (see [1]). So... > >> >> Eclair and coverity does not report this finding. > > ... if neither Eclair nor Coverity report it then I think this should be > a bug report against Cppcheck. Furthermore in reply to my "Arm32: tidy the memset() macro" you said [1] "In this case, Linux has removed __memzero() is patch ff5fdafc9e97 "ARM: 8745/1: get rid of __memzero()" because the performance difference with memset() was limited. For Xen, I think we should also remove the function." So either you want to follow that route, or it would rather be my patch which ought to be considered for merging, not the least because it also deals with yet another MISRA violation. Jan [1] https://lists.xen.org/archives/html/xen-devel/2022-08/msg01185.html
Re: [PATCH v3] xen/arm: smmuv3: mark arm_smmu_disable_pasid __maybe_unused
Hi,
On 15/12/2022 21:26, Stewart Hildebrand wrote:
When building with clang 12 and CONFIG_ARM_SMMU_V3=y, we observe the
following build error:
drivers/passthrough/arm/smmu-v3.c:1408:20: error: unused function
'arm_smmu_disable_pasid' [-Werror,-Wunused-function]
static inline void arm_smmu_disable_pasid(struct arm_smmu_master *master) { }
^
arm_smmu_disable_pasid is not currently called from anywhere in Xen, but
it is inside a section of code guarded by CONFIG_PCI_ATS, which may be
helpful in the future if the PASID feature is to be implemented. Add the
attribute __maybe_unused to the function.
Signed-off-by: Stewart Hildebrand
Reviewed-by: Rahul Singh
I have committed the patch.
Cheers,
--
Julien Grall
Re: [PATCH] xsm/flask: mkflash.sh: Use const when generating initial_sid_to_string[]
Hi Daniel,
On 12/12/2022 11:31, Daniel P. Smith wrote:
On 12/12/22 04:36, Julien Grall wrote:
From: Julien Grall
The array initial_sid_to_string is storing pointer to literal strings
and is not meant to be modified. So change the type of the variable
to "const char * const ...[]".
Signed-off-by: Julien Grall
---
xen/xsm/flask/policy/mkflask.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/xsm/flask/policy/mkflask.sh
b/xen/xsm/flask/policy/mkflask.sh
index 591ce832a1d1..611689768167 100755
--- a/xen/xsm/flask/policy/mkflask.sh
+++ b/xen/xsm/flask/policy/mkflask.sh
@@ -34,7 +34,7 @@ BEGIN {
printf("/*\n * Security object class definitions\n */\n") >
debugfile;
printf(" S_(\"null\")\n") > debugfile;
printf("/* This file is automatically generated. Do not
edit. */\n") > debugfile2;
- printf("static char *initial_sid_to_string[] =\n{\n") >
debugfile2;
+ printf("static const char * const initial_sid_to_string[]
=\n{\n") > debugfile2;
printf(" \"null\",\n") > debugfile2;
}
/^[ \t]*#/ {
Ack-by: Daniel P. Smith
Thanks. Not sure if this was intended, but the tag is technically
Acked-by rather than Ack-by.
I have committed the patch.
Cheers,
--
Julien Grall
Re: [RFC PATCH 01/18] arm: cppcheck: misra rule 20.7 deviations for alternative.h
On 20.12.2022 10:07, Julien Grall wrote:
> On 20/12/2022 08:50, Luca Fancellu wrote:
>> --- a/docs/misra/false-positive-cppcheck.json
>> +++ b/docs/misra/false-positive-cppcheck.json
>> @@ -3,6 +3,20 @@
>> "content": [
>> {
>> "id": "SAF-0-false-positive-cppcheck",
>> +"violation-id": "misra-c2012-20.7",
>> +"tool-version": "2.7",
>> +"name": "R20.7 second operand of member-access operator",
>> +"text": "The second operand of a member access operator shall
>> be a name of a member of the type pointed to, so in this particular case it
>> is wrong to use parentheses on the macro parameter."
>> +},
>> +{
>> +"id": "SAF-1-false-positive-cppcheck",
>> +"violation-id": "misra-c2012-20.7",
>> +"tool-version": "2.7",
>> +"name": "R20.7 C macro parameters used only for text
>> substitution",
>> +"text": "The macro parameters used in this case are not part of
>> an expression, they are used for text substitution."
>> +},
> In both cases the constructs are commonly used in Xen to generate code.
> So I am a bit concerned to have to add deviation everywhere cppcheck is
> wrong.
>
> More generally, we are still at the beginning of MISRA in Xen and I
> don't think we should start with adding deviations from bugs in the
> tools. Instead, we should report those bugs and hopefully by the time
> Xen is mostly MISRA complaint the tools will not report the false positive.
>
> If they are still then we can decide to add deviations.
+1
Jan
Re: [RFC PATCH 05/18] arm: cppcheck: fix misra rule 20.7 on arm/include/asm/string.h
Hi,
On 20/12/2022 08:50, Luca Fancellu wrote:
Cppcheck has found a violation of rule 20.7 for the macro memset
about missing parenthesis for the "n" argument, while the parenthesis
are not mandatory because the argument is never used in an
expression, adding them will not harm code and readability, so fix
the finding adding parenthesis for the argument.
This is something I have argued against in the past (see [1]). So...
Eclair and coverity does not report this finding.
... if neither Eclair nor Coverity report it then I think this should be
a bug report against Cppcheck.
Also, typo: s/does not/do not/
Signed-off-by: Luca Fancellu
---
xen/arch/arm/include/asm/string.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/arch/arm/include/asm/string.h
b/xen/arch/arm/include/asm/string.h
index b485e4904419..f1c87d215b0b 100644
--- a/xen/arch/arm/include/asm/string.h
+++ b/xen/arch/arm/include/asm/string.h
@@ -30,7 +30,7 @@ void __memzero(void *ptr, size_t n);
#define memset(p, v, n) \
({ \
-void *__p = (p); size_t __n = n;\
+void *__p = (p); size_t __n = (n); \
if ((__n) != 0) { \
if (__builtin_constant_p((v)) && (v) == 0) \
__memzero((__p),(__n)); \
Cheers,
[1] 20220728134943.1185621-1-burzalod...@gmail.com
--
Julien Grall
Re: [RFC PATCH 01/18] arm: cppcheck: misra rule 20.7 deviations for alternative.h
Hi Luca,
On 20/12/2022 08:50, Luca Fancellu wrote:
Cppcheck reports violations of rule 20.7 in two macros of
alternative.h.
The first one is in the __ALT_PTR macro where cppcheck wants to have
parentheses on the second operand of a member access operator, which
is not allowed from the c language syntax, furthermore, the macro
parameter is never used as an expression, hence we can suppress the
finding.
Looking at the Misra Rule 20.7 examples [1], this looks similar to
GET_MEMBER(). So I don't understand why cppcheck is complaining.
The second finding is in the __ALTERNATIVE_CFG macro, where cppcheck
wants to have parentheses on the macro arguments, but the macro
parameters are never used as expressions and are used only for text
substitution, so cppcheck is not taking into account the context of
use of the macro arguments and adding parenthesis would break the
code, so we can suppress the finding.
This reads like you want to report a bug against cppcheck.
No error was shown by eclair and coverity for those lines.
Signed-off-by: Luca Fancellu
---
docs/misra/false-positive-cppcheck.json | 14 ++
xen/arch/arm/include/asm/alternative.h | 2 ++
This file is meant to be close to Linux. From my understanding of the
discussion yesterday, we didn't want to add deviation there.
2 files changed, 16 insertions(+)
diff --git a/docs/misra/false-positive-cppcheck.json
b/docs/misra/false-positive-cppcheck.json
index 5d4da2ce6170..5e7d9377f60b 100644
--- a/docs/misra/false-positive-cppcheck.json
+++ b/docs/misra/false-positive-cppcheck.json
@@ -3,6 +3,20 @@
"content": [
{
"id": "SAF-0-false-positive-cppcheck",
+"violation-id": "misra-c2012-20.7",
+"tool-version": "2.7",
+"name": "R20.7 second operand of member-access operator",
+"text": "The second operand of a member access operator shall be a name
of a member of the type pointed to, so in this particular case it is wrong to use parentheses on
the macro parameter."
+},
+{
+"id": "SAF-1-false-positive-cppcheck",
+"violation-id": "misra-c2012-20.7",
+"tool-version": "2.7",
+"name": "R20.7 C macro parameters used only for text substitution",
+"text": "The macro parameters used in this case are not part of an
expression, they are used for text substitution."
+},
In both cases the constructs are commonly used in Xen to generate code.
So I am a bit concerned to have to add deviation everywhere cppcheck is
wrong.
More generally, we are still at the beginning of MISRA in Xen and I
don't think we should start with adding deviations from bugs in the
tools. Instead, we should report those bugs and hopefully by the time
Xen is mostly MISRA complaint the tools will not report the false positive.
If they are still then we can decide to add deviations.
Cheers,
[1]
https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_20_07.c
--
Julien Grall
[RFC PATCH 16/18] public/x86: cppcheck: misra rule 20.7 deviation on arch-x86/xen.h
Cppcheck has found violations of rule 20.7 for the macros
___DEFINE_XEN_GUEST_HANDLE and set_xen_guest_handle_raw.
For the first one, the macro parameters are never used as an
expression, so it is safe to suppress the finding.
For the second one, while the argument "val" is never used
in an expression, it doesn't harm the code or the readability,
so add them.
Eclair and coverity does not report these findings.
Signed-off-by: Luca Fancellu
---
xen/include/public/arch-x86/xen.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/xen/include/public/arch-x86/xen.h
b/xen/include/public/arch-x86/xen.h
index c0f4551247f4..04f5bc899ae3 100644
--- a/xen/include/public/arch-x86/xen.h
+++ b/xen/include/public/arch-x86/xen.h
@@ -14,6 +14,7 @@
/* Structural guest handles introduced in 0x00030201. */
#if __XEN_INTERFACE_VERSION__ >= 0x00030201
+/* SAF-1-false-positive-cppcheck R20.7 argument as text substitution */
#define ___DEFINE_XEN_GUEST_HANDLE(name, type) \
typedef struct { type *p; } __guest_handle_ ## name
#else
@@ -36,7 +37,7 @@
#define __XEN_GUEST_HANDLE(name)__guest_handle_ ## name
#define XEN_GUEST_HANDLE(name) __XEN_GUEST_HANDLE(name)
#define XEN_GUEST_HANDLE_PARAM(name)XEN_GUEST_HANDLE(name)
-#define set_xen_guest_handle_raw(hnd, val) do { (hnd).p = val; } while (0)
+#define set_xen_guest_handle_raw(hnd, val) do { (hnd).p = (val); } while (0)
#define set_xen_guest_handle(hnd, val) set_xen_guest_handle_raw(hnd, val)
#if defined(__i386__)
--
2.17.1
[RFC PATCH 15/18] public/x86: cppcheck: misra rule 20.7 deviation on xen-x86_64.h
Cppcheck has found violations of rule 20.7 for the macros
__DECL_REG_LOHI, __DECL_REG_LO8 and __DECL_REG_LO16, but the macro
parameters are used as variable identifiers, so it is safe to
suppress the findings.
Eclair and coverity does not report these findings.
Signed-off-by: Luca Fancellu
---
xen/include/public/arch-x86/xen-x86_64.h | 3 +++
1 file changed, 3 insertions(+)
diff --git a/xen/include/public/arch-x86/xen-x86_64.h
b/xen/include/public/arch-x86/xen-x86_64.h
index 5d9035ed2230..7e375cd6f45f 100644
--- a/xen/include/public/arch-x86/xen-x86_64.h
+++ b/xen/include/public/arch-x86/xen-x86_64.h
@@ -115,6 +115,7 @@ struct iret_context {
#if defined(__XEN__) || defined(__XEN_TOOLS__)
/* Anonymous unions include all permissible names (e.g., al/ah/ax/eax/rax). */
+/* SAF-1-false-positive-cppcheck R20.7 argument as text substitution */
#define __DECL_REG_LOHI(which) union { \
uint64_t r ## which ## x; \
uint32_t e ## which ## x; \
@@ -124,12 +125,14 @@ struct iret_context {
uint8_t which ## h; \
}; \
}
+/* SAF-1-false-positive-cppcheck R20.7 argument as text substitution */
#define __DECL_REG_LO8(name) union { \
uint64_t r ## name; \
uint32_t e ## name; \
uint16_t name; \
uint8_t name ## l; \
}
+/* SAF-1-false-positive-cppcheck R20.7 argument as text substitution */
#define __DECL_REG_LO16(name) union { \
uint64_t r ## name; \
uint32_t e ## name; \
--
2.17.1
[RFC PATCH 18/18] public: misra rule 20.7 deviation on memory.h
Cppcheck has found a violation of rule 20.7 for the macro XENMEM_SHARING_OP_FIELD_MAKE_GREF, the argument "val" is used in an expression, hence add parenthesis to the argument "val" to fix the violation. Signed-off-by: Luca Fancellu --- xen/include/public/memory.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h index 29cf5c823902..c5f0d31e235d 100644 --- a/xen/include/public/memory.h +++ b/xen/include/public/memory.h @@ -485,7 +485,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_mem_access_op_t); #define XENMEM_SHARING_OP_FIELD_IS_GREF_FLAG (xen_mk_ullong(1) << 62) #define XENMEM_SHARING_OP_FIELD_MAKE_GREF(field, val) \ -(field) = (XENMEM_SHARING_OP_FIELD_IS_GREF_FLAG | val) +(field) = (XENMEM_SHARING_OP_FIELD_IS_GREF_FLAG | (val)) #define XENMEM_SHARING_OP_FIELD_IS_GREF(field) \ ((field) & XENMEM_SHARING_OP_FIELD_IS_GREF_FLAG) #define XENMEM_SHARING_OP_FIELD_GET_GREF(field)\ -- 2.17.1
[RFC PATCH 17/18] public: misra rule 20.7 deviation on errno.h
Cppcheck has found a violation of rule 20.7 for the macro XEN_ERRNO,
while the macro parameter is never used as an expression, it doesn't
harm the code or the readability to add parenthesis, so add them.
This finding is reported also by eclair and coverity.
Signed-off-by: Luca Fancellu
---
xen/include/public/errno.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/include/public/errno.h b/xen/include/public/errno.h
index 6bdc8c507990..5a78a7607c0d 100644
--- a/xen/include/public/errno.h
+++ b/xen/include/public/errno.h
@@ -31,7 +31,7 @@
#ifndef __ASSEMBLY__
-#define XEN_ERRNO(name, value) XEN_##name = value,
+#define XEN_ERRNO(name, value) XEN_##name = (value),
enum xen_errno {
#elif __XEN_INTERFACE_VERSION__ < 0x00040700
--
2.17.1
Re: [PATCH RFC 10/10] common: convert vCPU info area registration
Hi Jan,
On 19/12/2022 15:01, Jan Beulich wrote:
On 17.12.2022 16:53, Julien Grall wrote:
On 19/10/2022 08:45, Jan Beulich wrote:
Switch to using map_guest_area(). Noteworthy differences from
map_vcpu_info():
- remote vCPU-s are paused rather than checked for being down (which in
principle can change right after the check),
- the domain lock is taken for a much smaller region,
- areas could now be registered more than once, if we want this,
- as a corner case registering the area at GFN 0 offset 0 is no longer
possible (this is considered an invalid de-registration request).
Note that this eliminates a bug in copy_vcpu_settings(): The function
did allocate a new page regardless of the GFN already having a mapping,
thus in particular breaking the case of two vCPU-s having their info
areas on the same page.
Signed-off-by: Jan Beulich
---
RFC: While the "no re-registration" check is retained, it is now racy.
If we really think it needs retaining _and_ properly enforcing,
then locking will be needed, but I don't think we can hold the
domain lock around a call to map_guest_area(), first and foremost
because of its use of vcpu_pause().
function like evtchn_2l_unmask() may access vcpu_info that is not the
current one.
So I believe the check needs to be reatined and properly enforced.
Otherwise, the old structure may still be used for a short time even if
it has been freed.
Good point, I'll try to come up with something suitable.
RFC: Is the GFN 0 offset 0 behavioral change deemed acceptable?
I would say n (See the previous discussion for more details).
Of course - with 0 going to be replaced in map_guest_area(), the check
there needs to be adjusted accordingly. And ~0 was already invalid to
pass in here, due to the alignment check.
RFC: To have just a single instance of casts to vcpu_info_t *,
introducing a macro (or inline function if header dependencies
permit) might be nice. However, the only sensible identifier for it
would imo be vcpu_info(). Yet we already have a macro of that name.
With some trickery it might be possible to overload the macro
(making the "field" argument optional), but this may not be
desirable for other reasons (it could e.g. be deemed confusing).
You might be able to reduce the number of cast by using local variables.
But it is not entirely clear why we can't use the existing vcpu_info()
in many of the cases. For instance...
--- a/xen/arch/x86/include/asm/shared.h
+++ b/xen/arch/x86/include/asm/shared.h
@@ -27,16 +27,16 @@ static inline void arch_set_##field(stru
static inline type arch_get_##field(const struct vcpu *v) \
{ \
return !has_32bit_shinfo(v->domain) ? \
- v->vcpu_info->native.arch.field :\
- v->vcpu_info->compat.arch.field; \
+ ((const vcpu_info_t *)v->vcpu_info_area.map)->native.arch.field : \
+ ((const vcpu_info_t *)v->vcpu_info_area.map)->compat.arch.field; \
... here.
vcpu_info() has a property which gets in the way of using the macro
here. Since __vcpu_info() wants to return something which can also
be used as lvalue, the 2nd and 3rd operands of the conditional
operator need to resolve to the same pointer type. Hence the smaller
(compat) type is the only one which is safe to use. See the comment
next to __shared_info()'s definition (which is the one __vcpu_info()'s
refers to).
Thanks for the pointer! I hadn't noticed the subtlety. Then, the
open-cast (even if I dislike them) is probably the way go for now.
As I mentioned above, it would nice if they can be reduced by using
local variables (this will also return the length of the line).
Cheers,
--
Julien Grall
[RFC PATCH 14/18] public/x86: cppcheck: misra rule 20.7 deviation on xen-x86_32.h
Cppcheck has found violations of rule 20.7 for the macros
___DEFINE_XEN_GUEST_HANDLE, set_xen_guest_handle_raw, __DECL_REG_LO8
and __DECL_REG_LO16.
For set_xen_guest_handle_raw, while the "val" argument is never used
in an expression, it doesn't harm the code or the readability to add
parenthesis, so add them to the argument.
For the other findings, the macro parameters are never used as an
expression, cppcheck is not taking into account the context for their
use, so we can suppress the finding.
Eclair and coverity does not report these findings.
Signed-off-by: Luca Fancellu
---
xen/include/public/arch-x86/xen-x86_32.h | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/xen/include/public/arch-x86/xen-x86_32.h
b/xen/include/public/arch-x86/xen-x86_32.h
index 139438e83534..6755f12044e4 100644
--- a/xen/include/public/arch-x86/xen-x86_32.h
+++ b/xen/include/public/arch-x86/xen-x86_32.h
@@ -74,6 +74,7 @@
/* 32-/64-bit invariability for control interfaces (domctl/sysctl). */
#if defined(__XEN__) || defined(__XEN_TOOLS__)
#undef ___DEFINE_XEN_GUEST_HANDLE
+/* SAF-1-false-positive-cppcheck R20.7 argument as text substitution */
#define ___DEFINE_XEN_GUEST_HANDLE(name, type) \
typedef struct { type *p; } \
__guest_handle_ ## name;\
@@ -82,7 +83,7 @@
#undef set_xen_guest_handle_raw
#define set_xen_guest_handle_raw(hnd, val) \
do { if ( sizeof(hnd) == 8 ) *(uint64_t *)&(hnd) = 0; \
- (hnd).p = val; \
+ (hnd).p = (val); \
} while ( 0 )
#define int64_aligned_t int64_t __attribute__((aligned(8)))
#define uint64_aligned_t uint64_t __attribute__((aligned(8)))
@@ -96,6 +97,7 @@
/* nothing */
#elif defined(__XEN__) || defined(__XEN_TOOLS__)
/* Anonymous unions include all permissible names (e.g., al/ah/ax/eax). */
+/* SAF-1-false-positive-cppcheck R20.7 argument as text substitution */
#define __DECL_REG_LO8(which) union { \
uint32_t e ## which ## x; \
uint16_t which ## x; \
@@ -104,6 +106,7 @@
uint8_t which ## h; \
}; \
}
+/* SAF-1-false-positive-cppcheck R20.7 argument as text substitution */
#define __DECL_REG_LO16(name) union { \
uint32_t e ## name, _e ## name; \
uint16_t name; \
--
2.17.1
[RFC PATCH 13/18] public/x86: cppcheck: misra rule 20.7 deviation on hvm/save.h
Cppcheck has found a violation of rule 20.7 for the macro
XEN_HVM_VIOAPIC, but the first macro argument is never used
as an expression, cppcheck is not taking into account the context of
use for it, so we can suppress the finding.
Eclair and coverity does not report this finding.
Signed-off-by: Luca Fancellu
---
xen/include/public/arch-x86/hvm/save.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/xen/include/public/arch-x86/hvm/save.h
b/xen/include/public/arch-x86/hvm/save.h
index 7ecacadde165..08f221483dc8 100644
--- a/xen/include/public/arch-x86/hvm/save.h
+++ b/xen/include/public/arch-x86/hvm/save.h
@@ -366,6 +366,7 @@ union vioapic_redir_entry
#define VIOAPIC_NUM_PINS 48 /* 16 ISA IRQs, 32 non-legacy PCI IRQS. */
+/* SAF-1-false-positive-cppcheck R20.7 argument as text substitution */
#define XEN_HVM_VIOAPIC(name, cnt) \
struct name { \
uint64_t base_address; \
--
2.17.1
[RFC PATCH 10/18] xen: cppcheck: misra rule 20.7 deviation on types.h
Cppcheck has found a violation of rule 20.7 for the macro DECLARE_BITMAP, but the macro parameter is used as variable identifier, so it is safe to suppress the finding and don't put parenthesis. Eclair and coverity does not report this finding. Signed-off-by: Luca Fancellu --- xen/include/xen/types.h | 1 + 1 file changed, 1 insertion(+) diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h index 03f0fe612ed9..c734a52f24b1 100644 --- a/xen/include/xen/types.h +++ b/xen/include/xen/types.h @@ -7,6 +7,7 @@ #define BITS_TO_LONGS(bits) \ (((bits)+BITS_PER_LONG-1)/BITS_PER_LONG) +/* SAF-1-false-positive-cppcheck R20.7 argument as text substitution */ #define DECLARE_BITMAP(name,bits) \ unsigned long name[BITS_TO_LONGS(bits)] -- 2.17.1
[RFC PATCH 11/18] xen: cppcheck: misra rule 20.7 deviation on xmalloc.h
Cppcheck has found violations of rule 20.7 for the macros xmalloc, xzalloc, xmalloc_array, xzalloc_array, xzalloc_flex_struct and xmalloc_flex_struct. In all the cases the macro parameters are never used as an expression, so it is safe to suppress the findings. Eclair and coverity does not report these findings. Signed-off-by: Luca Fancellu --- xen/include/xen/xmalloc.h | 6 ++ 1 file changed, 6 insertions(+) diff --git a/xen/include/xen/xmalloc.h b/xen/include/xen/xmalloc.h index 16979a117c6a..5bf440502c20 100644 --- a/xen/include/xen/xmalloc.h +++ b/xen/include/xen/xmalloc.h @@ -10,7 +10,9 @@ */ /* Allocate space for typed object. */ +/* SAF-1-false-positive-cppcheck R20.7 argument as text substitution */ #define xmalloc(_type) ((_type *)_xmalloc(sizeof(_type), __alignof__(_type))) +/* SAF-1-false-positive-cppcheck R20.7 argument as text substitution */ #define xzalloc(_type) ((_type *)_xzalloc(sizeof(_type), __alignof__(_type))) /* @@ -30,15 +32,19 @@ }) /* Allocate space for array of typed objects. */ +/* SAF-1-false-positive-cppcheck R20.7 argument as text substitution */ #define xmalloc_array(_type, _num) \ ((_type *)_xmalloc_array(sizeof(_type), __alignof__(_type), _num)) +/* SAF-1-false-positive-cppcheck R20.7 argument as text substitution */ #define xzalloc_array(_type, _num) \ ((_type *)_xzalloc_array(sizeof(_type), __alignof__(_type), _num)) /* Allocate space for a structure with a flexible array of typed objects. */ +/* SAF-1-false-positive-cppcheck R20.7 argument as text substitution */ #define xzalloc_flex_struct(type, field, nr) \ ((type *)_xzalloc(offsetof(type, field[nr]), __alignof__(type))) +/* SAF-1-false-positive-cppcheck R20.7 argument as text substitution */ #define xmalloc_flex_struct(type, field, nr) \ ((type *)_xmalloc(offsetof(type, field[nr]), __alignof__(type))) -- 2.17.1
[RFC PATCH 12/18] arm: cppcheck: misra rule 20.7 deviation on asm/arm64/sysregs.h
Cppcheck has found a violation of rule 20.7 for the macro
WRITE_SYSREG64, the macro parameter "v" is never used in an
expression, but adding parenthesis to it doesn't harm the code
or the readability, so add parenthesis to it.
Eclair and coverity does not report this finding.
Signed-off-by: Luca Fancellu
---
xen/arch/arm/include/asm/arm64/sysregs.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/arch/arm/include/asm/arm64/sysregs.h
b/xen/arch/arm/include/asm/arm64/sysregs.h
index 463899951414..3f709d26a299 100644
--- a/xen/arch/arm/include/asm/arm64/sysregs.h
+++ b/xen/arch/arm/include/asm/arm64/sysregs.h
@@ -461,7 +461,7 @@
/* Access to system registers */
#define WRITE_SYSREG64(v, name) do {\
-uint64_t _r = v;\
+uint64_t _r = (v); \
asm volatile("msr "__stringify(name)", %0" : : "r" (_r)); \
} while (0)
#define READ_SYSREG64(name) ({ \
--
2.17.1
[RFC PATCH 09/18] xen: cppcheck: misra rule 20.7 deviation on kconfig.h
Cppcheck has found a violation of rule 20.7 for the macro
__config_enabled but the preprocessor branch where this macro is
defined should not be analysed by cppcheck when CPPCHECK macro is
defined, hence this is a false positive of the tool and we can
safely suppress the finding.
Eclair and coverity does not report this finding.
Signed-off-by: Luca Fancellu
---
docs/misra/false-positive-cppcheck.json | 7 +++
xen/include/xen/kconfig.h | 1 +
2 files changed, 8 insertions(+)
diff --git a/docs/misra/false-positive-cppcheck.json
b/docs/misra/false-positive-cppcheck.json
index 5e7d9377f60b..c8ee3c0c6317 100644
--- a/docs/misra/false-positive-cppcheck.json
+++ b/docs/misra/false-positive-cppcheck.json
@@ -17,6 +17,13 @@
},
{
"id": "SAF-2-false-positive-cppcheck",
+"violation-id": "misra-c2012-20.7",
+"tool-version": "2.7",
+"name": "R20.7 on preprocessor branch that should be disabled",
+"text": "This preprocessor branch should be disabled when CPPCHECK
macro is active, so there should not be violation."
+},
+{
+"id": "SAF-3-false-positive-cppcheck",
"violation-id": "",
"tool-version": "",
"name": "Sentinel",
diff --git a/xen/include/xen/kconfig.h b/xen/include/xen/kconfig.h
index a717b0819c2e..92373c018950 100644
--- a/xen/include/xen/kconfig.h
+++ b/xen/include/xen/kconfig.h
@@ -23,6 +23,7 @@
#define __ARG_PLACEHOLDER_1 0,
#define config_enabled(cfg) _config_enabled(cfg)
#define _config_enabled(value) __config_enabled(__ARG_PLACEHOLDER_##value)
+/* SAF-2-false-positive-cppcheck R20.7 but cppcheck should not check here */
#define __config_enabled(arg1_or_junk) ___config_enabled(arg1_or_junk 1, 0)
#define ___config_enabled(__ignored, val, ...) val
--
2.17.1
[RFC PATCH 07/18] xen: cppcheck: misra rule 20.7 deviation on compiler.h
Cppcheck has found a violation of rule 20.7 for the macro sizeof_field, but the parenthesis cannot be applied on the second operand of a member access operator, hence we can suppress the finding. Eclair and coverity does not report this finding. Signed-off-by: Luca Fancellu --- xen/include/xen/compiler.h | 1 + 1 file changed, 1 insertion(+) diff --git a/xen/include/xen/compiler.h b/xen/include/xen/compiler.h index a5631303348b..301ba55d6ecc 100644 --- a/xen/include/xen/compiler.h +++ b/xen/include/xen/compiler.h @@ -115,6 +115,7 @@ * @TYPE: The structure containing the field of interest * @MEMBER: The field to return the size of */ +/* SAF-0-false-positive-cppcheck R20.7 member-access operator */ #define sizeof_field(TYPE, MEMBER) sizeofTYPE *)0)->MEMBER)) #if !defined(__STDC_VERSION__) || __STDC_VERSION__ < 201112L -- 2.17.1
[RFC PATCH 08/18] xen: cppcheck: misra rule 20.7 deviation on init.h
Cppcheck has found violations of rule 20.7 for the macros
__init_call, presmp_initcall, __initcall and __exitcall.
For the first one, the macro parameter is never used as an expression,
it is used for text substitution but cppcheck is not taking into
account the context of use of the argument, so we can suppress the
finding.
For the other findings, the argument is not used as an expression,
but adding parenthesis doesn't harm the code or the readability,
hence add parenthesis on the argument.
Eclair and coverity does not report these findings.
Signed-off-by: Luca Fancellu
---
xen/include/xen/init.h | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/xen/include/xen/init.h b/xen/include/xen/init.h
index 0af0e234ec80..7c072b7c8cf8 100644
--- a/xen/include/xen/init.h
+++ b/xen/include/xen/init.h
@@ -15,6 +15,7 @@
#define __initconstrel__section(".init.rodata.rel")
#define __exitdata__used_section(".exit.data")
#define __initsetup __used_section(".init.setup")
+/* SAF-1-false-positive-cppcheck R20.7 argument as text substitution */
#define __init_call(lvl) __used_section(".initcall" lvl ".init")
#define __exit_call __used_section(".exitcall.exit")
@@ -65,11 +66,11 @@ typedef int (*initcall_t)(void);
typedef void (*exitcall_t)(void);
#define presmp_initcall(fn) \
-const static initcall_t __initcall_##fn __init_call("presmp") = fn
+const static initcall_t __initcall_##fn __init_call("presmp") = (fn)
#define __initcall(fn) \
-const static initcall_t __initcall_##fn __init_call("1") = fn
+const static initcall_t __initcall_##fn __init_call("1") = (fn)
#define __exitcall(fn) \
-static exitcall_t __exitcall_##fn __exit_call = fn
+static exitcall_t __exitcall_##fn __exit_call = (fn)
void do_presmp_initcalls(void);
void do_initcalls(void);
--
2.17.1
[RFC PATCH 06/18] public: cppcheck: misra rule 20.7 on public/arch-arm.h
Cppcheck has found violations of rule 20.7 for the macros
___DEFINE_XEN_GUEST_HANDLE, set_xen_guest_handle_raw and __DECL_REG.
For the first and third finding, the macro parameters are never
used in an expression, cppcheck is not taking into account the
context where the arguments are used, so we can suppress the findings.
For the set_xen_guest_handle_raw, the argument is not involved in any
expression but it doesn't harm the code or readability to have
parenthesis on it, so fix it.
Eclair and coverity does not report these findings.
Signed-off-by: Luca Fancellu
---
xen/include/public/arch-arm.h | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index 1528ced5097a..7afc0d1ca23d 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -156,6 +156,7 @@
#define uint64_aligned_t uint64_t __attribute__((aligned(8)))
#ifndef __ASSEMBLY__
+/* SAF-1-false-positive-cppcheck R20.7 argument as text substitution */
#define ___DEFINE_XEN_GUEST_HANDLE(name, type) \
typedef union { type *p; unsigned long q; } \
__guest_handle_ ## name;\
@@ -180,7 +181,7 @@
do {\
__typeof__(&(hnd)) _sxghr_tmp = &(hnd); \
_sxghr_tmp->q = 0; \
-_sxghr_tmp->p = val;\
+_sxghr_tmp->p = (val); \
} while ( 0 )
#define set_xen_guest_handle(hnd, val) set_xen_guest_handle_raw(hnd, val)
@@ -206,6 +207,7 @@ typedef uint64_t xen_ulong_t;
}
#else
/* Non-gcc sources must always use the proper 64-bit name (e.g., x0). */
+/* SAF-1-false-positive-cppcheck R20.7 argument as text substitution */
#define __DECL_REG(n64, n32) uint64_t n64
#endif
--
2.17.1
