Re: [XEN v2] xen/arm64: io: Decode 32-bit ldr/str post-indexing instructions

[Jan Beulich]

On 29.11.2021 20:16, Ayan Kumar Halder wrote:
> At the moment, Xen is only handling data abort with valid syndrome (i.e.
> ISV=0). Unfortunately, this doesn't cover all the instructions a domain
> could use to access MMIO regions.
> 
> For instance, Xilinx baremetal OS will use:
> 
> volatile u32 *LocalAddr = (volatile u32 *)Addr;
> *LocalAddr = Value;
> 
> This leave the compiler to decide which store instructions to use. This
> may be a post-index store instruction where the HW will not provide a
> valid syndrome.
> 
> In order to handle post-indexing store/load instructions, Xen will need
> to fetch and decode the instruction.
> 
> This patch only cover post-index store/load instructions from AArch64 mode.
> For now, this is left unimplemented for trap from AArch32 mode.
> 
> Signed-off-by: Ayan Kumar Halder 

Just a couple of general remarks, with no judgement towards its use
in the codebase, and leaving out several obvious style issues.

> Changelog :-
> 
> v2 :- 1. Updated the rn register after reading from it. (Pointed by Julien,
>  Stefano)
> 2. Used a union to represent the instruction opcode (Suggestd by Bertrand)
> 3. Fixed coding style issues (Pointed by Julien)
> 4. In the previous patch, I was updating dabt->sign based on the signedness of
> imm9. This was incorrect. As mentioned in ARMv8 ARM  DDI 0487G.b, Page 3221,
> SSE indicates the signedness of the data item loaded. In our case, the data 
> item
> loaded is always unsigned.
> 
> This has been tested for the following cases :-
> ldr x2, [x1], #4

DYM "ldr w2, [x1], #4" or "ldr x2, [x1], #8" here?

> ldr w2, [x1], #-4
> 
> str x2, [x1], #4

Similar aspect here.

> str w2, [x1], #-4
> 
> The reason being  I am testing on 32bit MMIO registers only. I don't see a 
> 8bit
> or 16bit MMIO register.

As per this, perhaps the former of the two.

> --- a/xen/arch/arm/decode.c
> +++ b/xen/arch/arm/decode.c
> @@ -84,6 +84,66 @@ bad_thumb2:
>  return 1;
>  }
>  
> +static int decode_32bit_loadstore_postindexing(register_t pc,
> +   struct hsr_dabt *dabt,
> +   union ldr_str_instr_class 
> *instr)
> +{
> +if ( raw_copy_from_guest(>value, (void * __user)pc, sizeof 
> (instr)) )
> +return -EFAULT;
> +
> +/* First, let's check for the fixed values */
> +if ( !((instr->code.fixed1 == 1) && (instr->code.fixed2 == 0) &&
> + (instr->code.fixed3 == 0) && (instr->code.fixed4 == 7)) )
> +{
> +gprintk(XENLOG_ERR, "Decoding not supported for instructions other 
> than"
> +" ldr/str post indexing\n");
> +goto bad_32bit_loadstore;
> +}
> +
> +if ( instr->code.size != 2 )
> +{
> +gprintk(XENLOG_ERR,
> +"ldr/str post indexing is supported for 32 bit variant only\n");
> +goto bad_32bit_loadstore;
> +}
> +
> +if ( instr->code.v != 0 )
> +{
> +gprintk(XENLOG_ERR,
> +"ldr/str post indexing for vector types are not supported\n");
> +goto bad_32bit_loadstore;
> +}
> +
> +/* Check for STR (immediate) - 32 bit variant */
> +if ( instr->code.opc == 0 )
> +{
> +dabt->write = 1;
> +}
> +/* Check for LDR (immediate) - 32 bit variant */
> +else if ( instr->code.opc == 1 )
> +{
> +dabt->write = 0;
> +}
> +else
> +{
> +gprintk(XENLOG_ERR,
> +"Decoding ldr/str post indexing is not supported for this 
> variant\n");
> +goto bad_32bit_loadstore;
> +}
> +
> +gprintk(XENLOG_INFO,
> +"instr->code.rt = 0x%x, instr->code.size = 0x%x, instr->code.imm9 = 
> %d\n",
> +instr->code.rt, instr->code.size, instr->code.imm9);
> +
> +update_dabt(dabt, instr->code.rt, instr->code.size, false);
> +dabt->valid = 1;
> +
> +return 0;
> +bad_32bit_loadstore:

Please indent labels by at least a blank. I also think this would
benefit from a preceding blank line.

> --- a/xen/arch/arm/io.c
> +++ b/xen/arch/arm/io.c
> @@ -65,6 +65,16 @@ static enum io_state handle_write(const struct 
> mmio_handler *handler,
>  return ret ? IO_HANDLED : IO_ABORT;
>  }
>  
> +static void post_incremenet_register(union ldr_str_instr_class *instr)

I think you mean post_increment_register()?

> +{
> +struct cpu_user_regs *regs = guest_cpu_user_regs();
> +unsigned int val;
> +
> +val = get_user_reg(regs, instr->code.rn);
> +val += instr->code.imm9;
> +set_user_reg(regs, instr->code.rn, val);

I don't think this handles the SP case correctly, and I also don't see
that case getting rejected elsewhere.

> --- a/xen/include/asm-arm/hsr.h
> +++ b/xen/include/asm-arm/hsr.h
> @@ -15,6 +15,32 @@ enum dabt_size {
>  DABT_DOUBLE_WORD = 3,
>  };
>  
> +/*
> + * Refer to the ARMv8 ARM (DDI 0487G.b), Section C4.1.4 Loads and Stores
> + * Page 318 specifies the following bit pattern for
> + * "load/store register (immediate post-indexed)".
> + *
> + 

Re: [PATCH] x86/boot: Drop incorrect mapping at l2_xenmap[0]

[Jan Beulich]

On 29.11.2021 18:42, Andrew Cooper wrote:
> On 29/11/2021 17:26, Andrew Cooper wrote:
>> It has been 4 years since the default load address changed from 1M to 2M, and
>> _stext ceased residing in l2_xenmap[0].  We should not be inserting an unused
>> mapping.
>>
>> To ensure we don't create/remove mappings accidentally, loop from 0 and obey
>> _PAGE_PRESENT on all entries.
>>
>> Fixes: 7ed93f3a0dff ("x86: change default load address from 1 MiB to 2 MiB")
>> Signed-off-by: Andrew Cooper 
>> ---
>> CC: Jan Beulich 
>> CC: Roger Pau Monné 
>> CC: Wei Liu 
>>
>> I ought to have spotted this in c/s 52975142d154 ("x86/boot: Create the
>> l2_xenmap[] mappings dynamically") too.
>> ---
>>  xen/arch/x86/setup.c | 10 +++---
>>  1 file changed, 3 insertions(+), 7 deletions(-)
>>
>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>> index da47cdea14a1..6f241048425c 100644
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -1279,16 +1279,12 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>>  
>>  /* The only data mappings to be relocated are in the Xen area. 
>> */
>>  pl2e = __va(__pa(l2_xenmap));
>> -/*
>> - * Undo the temporary-hooking of the l1_directmap.  
>> __2M_text_start
>> - * is contained in this PTE.
>> - */
>> +
>>  BUG_ON(using_2M_mapping() &&
>> l2_table_offset((unsigned long)_erodata) ==
>> l2_table_offset((unsigned long)_stext));
>> -*pl2e++ = l2e_from_pfn(xen_phys_start >> PAGE_SHIFT,
>> -   PAGE_HYPERVISOR_RX | _PAGE_PSE);
>> -for ( i = 1; i < L2_PAGETABLE_ENTRIES; i++, pl2e++ )
>> +
>> +for ( i = 0; i < L2_PAGETABLE_ENTRIES; i++, pl2e++ )
>>  {
>>  unsigned int flags;
>>  
> 
> Actually, on further consideration, this hunk should be included too, to
> cross-check that we don't zap any of the dynamically created mappings.

I agree in principle, but ...

> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1320,7 +1320,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>  }
>  else
>  {
> -    *pl2e = l2e_empty();
> +    ASSERT_UNREACHABLE();
>  continue;
>  }

... this isn't going to help non-debug builds. Dropping the zapping would
mean release builds then run with an unadjusted PTE. I can see two options:
Extract the flags from the existing PTE as fallback (i.e. keeping the
ASSERT_UNREACHABLE()) or use BUG() instead.

Jan

Re: Xen on LFS

[Sai Kiran Kumar Reddy]

Ok, thanks for the reply. Will do that.

On Tue, Nov 30, 2021 at 12:52 PM Jan Beulich  wrote:

> On 30.11.2021 07:50, Sai Kiran Kumar Reddy wrote:
> > I am Sai Kiran. I am currently working on installing xen on Linux From
> > Scratch(LFS) system. One of the dependencies of xen is "xorg" package.
> This
> > package is present in Beyond
> >  Linux From
> > Scratch(BLFS) <
> https://www.linuxfromscratch.org/blfs/view/svn/x/xorg7.html>
> > manual. But, there are a lot of packages to be installed. I am not sure
> if
> > all these packages are required for Xen. Also, is xorg a must, to build
> and
> > install xen?
> >
> > Kindly help me out here. Thanks in advance, for the support.
>
> Thanks for your interest, but I'm afraid your question isn't fitting
> xen-devel.
> Please raise it on xen-users.
>
> Jan
>
>

Re: Xen on LFS

[Jan Beulich]

On 30.11.2021 07:50, Sai Kiran Kumar Reddy wrote:
> I am Sai Kiran. I am currently working on installing xen on Linux From
> Scratch(LFS) system. One of the dependencies of xen is "xorg" package. This
> package is present in Beyond
>  Linux From
> Scratch(BLFS) 
> manual. But, there are a lot of packages to be installed. I am not sure if
> all these packages are required for Xen. Also, is xorg a must, to build and
> install xen?
> 
> Kindly help me out here. Thanks in advance, for the support.

Thanks for your interest, but I'm afraid your question isn't fitting xen-devel.
Please raise it on xen-users.

Jan

Re: [PATCH] Fixed an incorrect value

[Jan Beulich]

On 29.11.2021 20:44, Ayan Kumar Halder wrote:
> GENMASK(30, 21) should be 0x07fe0

Please can this have a meaningful title? E.g. "bitops: fix incorrect
value in comment"?

> --- a/xen/include/xen/bitops.h
> +++ b/xen/include/xen/bitops.h
> @@ -5,7 +5,7 @@
>  /*
>   * Create a contiguous bitmask starting at bit position @l and ending at
>   * position @h. For example
> - * GENMASK(30, 21) gives us the 32bit vector 0x01fe0.
> + * GENMASK(30, 21) gives us the 32bit vector 0x07fe0.

Once at it I think you also want to
- replace the word "vector",
- drop the odd leading 0: The number would better be 8 digits or
  16 digits, but not 9.

Jan

Xen on LFS

[Sai Kiran Kumar Reddy]

Hi,

I am Sai Kiran. I am currently working on installing xen on Linux From
Scratch(LFS) system. One of the dependencies of xen is "xorg" package. This
package is present in Beyond
 Linux From
Scratch(BLFS) 
manual. But, there are a lot of packages to be installed. I am not sure if
all these packages are required for Xen. Also, is xorg a must, to build and
install xen?

Kindly help me out here. Thanks in advance, for the support.

Regards,
Sai Kiran.

[qemu-mainline test] 166953: tolerable FAIL - PUSHED

[osstest service owner]

flight 166953 qemu-mainline real [real]
http://logs.test-lab.xenproject.org/osstest/logs/166953/

Failures :-/ but no regressions.

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 166952
 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 166952
 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 166952
 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail  like 166952
 test-armhf-armhf-libvirt 16 saverestore-support-checkfail  like 166952
 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check   fail like 166952
 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 166952
 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 166952
 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-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-amd64-i386-xl-pvshim14 guest-start  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-xl  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  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-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   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-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail 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-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-multivcpu 15 migrate-support-checkfail  never pass
 test-armhf-armhf-xl-multivcpu 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-armhf-armhf-xl  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  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-cubietruck 15 migrate-support-checkfail never pass
 test-armhf-armhf-xl-cubietruck 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-armhf-armhf-libvirt-raw 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 15 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass

version targeted for testing:
 qemuua0fd8a5492240379a07c0b39c8dae3b8341b458f
baseline version:
 qemuue750c10167fa8ad3fcc98236a474c46e52e7c18c

Last test of basis   166952  2021-11-29 14:09:14 Z0 days
Testing same since   166953  2021-11-29 20:37:58 Z0 days1 attempts


People who touched revisions under test:
  Alex Bennée 
  Andreas Schwab 
  Cindy Lu 
  

Re: [patch 05/22] genirq/msi: Fixup includes

[Thomas Gleixner]

Cedric,

On Mon, Nov 29 2021 at 08:33, Cédric Le Goater wrote:
> On 11/27/21 02:18, Thomas Gleixner wrote:
>> Remove the kobject.h include from msi.h as it's not required and add a
>> sysfs.h include to the core code instead.
>> 
>> Signed-off-by: Thomas Gleixner 
>
>
> This patch breaks compile on powerpc :
>
>CC  arch/powerpc/kernel/msi.o
> In file included from ../arch/powerpc/kernel/msi.c:7:
> ../include/linux/msi.h:410:65: error: ‘struct cpumask’ declared inside 
> parameter list will not be visible outside of this definition or declaration 
> [-Werror]
>410 | int msi_domain_set_affinity(struct irq_data *data, const struct 
> cpumask *mask,
>| 
> ^~~
> cc1: all warnings being treated as errors
>
> Below is fix you can merge in patch 5.

thanks for having a look. I fixed up this and other fallout and pushed out an
updated series (all 4 parts) to:

git://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel msi

Thanks,

tglx

[qemu-mainline test] 166952: tolerable FAIL - PUSHED

[osstest service owner]

flight 166952 qemu-mainline real [real]
http://logs.test-lab.xenproject.org/osstest/logs/166952/

Failures :-/ but no regressions.

Tests which did not succeed, but are not blocking:
 test-armhf-armhf-xl-rtds 18 guest-start/debian.repeatfail  like 166948
 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 166950
 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 166950
 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 166950
 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail  like 166950
 test-armhf-armhf-libvirt 16 saverestore-support-checkfail  like 166950
 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check   fail like 166950
 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 166950
 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 166950
 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-xsm  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  15 migrate-support-checkfail   never pass
 test-amd64-i386-xl-pvshim14 guest-start  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-xl  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  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-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   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-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail 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-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-multivcpu 15 migrate-support-checkfail  never pass
 test-armhf-armhf-xl-multivcpu 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-armhf-armhf-xl  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  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-cubietruck 15 migrate-support-checkfail never pass
 test-armhf-armhf-xl-cubietruck 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-armhf-armhf-libvirt-raw 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 15 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass

version targeted for testing:
 qemuue750c10167fa8ad3fcc98236a474c46e52e7c18c
baseline version:
 qemuudd4b0de45965538f19bb40c7ddaaba384a8c613a

Last test of basis   166950  2021-11-29 07:36:32 Z0 days
Testing same since   166952  2021-11-29 14:09:14 Z0 days1 attempts


People who 

[PATCH] Fixed an incorrect value

[Ayan Kumar Halder]

GENMASK(30, 21) should be 0x07fe0

Signed-off-by: Ayan Kumar Halder 
---

 xen/include/xen/bitops.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/include/xen/bitops.h b/xen/include/xen/bitops.h
index a64595f68e..2c8522d218 100644
--- a/xen/include/xen/bitops.h
+++ b/xen/include/xen/bitops.h
@@ -5,7 +5,7 @@
 /*
  * Create a contiguous bitmask starting at bit position @l and ending at
  * position @h. For example
- * GENMASK(30, 21) gives us the 32bit vector 0x01fe0.
+ * GENMASK(30, 21) gives us the 32bit vector 0x07fe0.
  */
 #define GENMASK(h, l) \
 (((~0UL) << (l)) & (~0UL >> (BITS_PER_LONG - 1 - (h
-- 
2.17.1

Re: [RFC PATCH] Added the logic to decode 32 bit ldr/str post-indexing instructions

[Ayan Kumar Halder]

Hi All,

Thanks for giving your inputs.

On 26/11/2021 17:39, Andre Przywara wrote:

On Fri, 26 Nov 2021 15:28:06 +
Ayan Kumar Halder  wrote:

Hi Ayan,


Many thanks for your inputs.
Apologies if I sound dumb, but I need a few clarifications.

On 26/11/2021 13:14, Andre Przywara wrote:

On Fri, 19 Nov 2021 16:52:02 +
Ayan Kumar Halder  wrote:

Hi,
   

At present, post indexing instructions are not emulated by Xen.
When Xen gets the exception, EL2_ESR.ISV bit not set. Thus as a
result, data abort is triggered.

Added the logic to decode ldr/str post indexing instructions.
With this, Xen can decode instructions like these:-
ldr w2, [x1], #4
Thus, domU can read ioreg with post indexing instructions.


Where do those instructions come from? A (C) compiler? (Some mail in
another thread from Stefano suggests so)
If yes, I would argue that is broken:
IIUC C compilers assume normal memory attributes for every pointer they
handle, so they are free to use unaligned accesses, load/store exclusives,
split accesses (two halfword reads) and what not when generating code.
The GIC needs to be mapped as device memory, which explicitly forbids
unaligned accesses and exclusives (as in: always traps), so you cannot let
compiler-generated code access the GIC (or most other MMIO devices, for
that matter).
I know, this somewhat works(TM) in practise, because a uint32_t assignment
is very likely to end up in an ldr/str, but please let me know which car
this code ends up in, so that can I avoid this brand ;-)

You can tell the compiler to avoid unaligned accesses with -mstrict-align
(and should definitely do so when you are running C code with the MMU
off), but that still leaves exclusives and split accesses at the
compiler's discretion. A variation on the topic of split access is merged
writes, where the compiler uses NEON or SVE instructions, for instance, to
cover multiple words at once, possibly via some memset()/memcpy() routine.


I understand that we should be using inline assembly instructions to
access any MMIO region. This is to prevent the compiler doing any tricks.

But is there a restriction that post indexing instructions can never be
used to access MMIO region ?


No, this is a pure virtualisation restriction, see below. On real
hardware/bare-metal, ldr/str with post or pre-indexing works and is fine
to use for MMIO.
But we need to have the right access width, matching the MMIO device's
expectation. So ldp/stp would probably be problematic, for instance.


On top there is this architectural restriction of the ARMv7/v8
virtualisation extension to not decode many "advanced" load/store
instructions in ESR_EL2.

Where do I find this restriction ?


That's described in the ESR_ELx syndrome description in the ARMv8 ARM (DDI
0487G.b), section "ISS encoding for an exception from a Data Abort" (page
D13-3219 in my Issue G.b copy):
"For other faults reported in ESR_EL2, ISV is 0 except for the following stage 2 
aborts: "


Are you telling me that load/store with post indexing is an "advanced"
instruction and ArmV8 does not allow decoding of these instructions in
ESR_EL2 ?


Yes, it is in the group of instructions for which the hardware does not
provide syndrome information in ESR_EL2: "  but excluding Load
Exclusive or Store Exclusive and excluding those with writeback)."


Isn't that a very strong limitation ?


I don't know about that, it's what it is and what it was for years. Linux
deliberately chose ldr/str only for readl/writel to be able to trap and
handle MMIO aborts in hypervisors.

If you do MMIO accesses the right way, using (inline) assembly only, then
you don't have the problem, and also avoid many others, see my previous
mail.

If you think of it from an architectural and implementation point of view
(and keep the RISC idea in mind): it should happen rarely, but would
require many gates for something that you can do in software as well.


Also what is your opinion on Xen decoding these instructions ?


I would be careful, we deliberately avoid this in KVM. This bubbles up
from time to time, though, so we now allow delegating this case to
userland, so the VMM can do the decoding there.
In Xen you have less issues with walking the guest's page tables,
though (a major problem in KVM), but it still adds complexity to a
hypervisor which aims to be lean by design.
Another argument would be that just post/pre does not cover everything, and
the cases start to pile up quickly: what about the immediate versions,
ldxr, stp, NEON/SVE load/stores, etc. Since many of those are not safe for
MMIO anyway, you add a lot of code for little use (and which gets little
testing!).


Many thanks for your explanation. It makes some sense. Unfortunately, I 
don't have much experience with hypervisors. So I will rely on you and 
other experts opinions on this. :)


I have sent out a v2 patch "[XEN v2] xen/arm64: io: Decode 32-bit 
ldr/str post-indexing instructions". Please have a look.


Stefano/Julien/Bertrand/Jan :- 

[XEN v2] xen/arm64: io: Decode 32-bit ldr/str post-indexing instructions

[Ayan Kumar Halder]

At the moment, Xen is only handling data abort with valid syndrome (i.e.
ISV=0). Unfortunately, this doesn't cover all the instructions a domain
could use to access MMIO regions.

For instance, Xilinx baremetal OS will use:

volatile u32 *LocalAddr = (volatile u32 *)Addr;
*LocalAddr = Value;

This leave the compiler to decide which store instructions to use. This
may be a post-index store instruction where the HW will not provide a
valid syndrome.

In order to handle post-indexing store/load instructions, Xen will need
to fetch and decode the instruction.

This patch only cover post-index store/load instructions from AArch64 mode.
For now, this is left unimplemented for trap from AArch32 mode.

Signed-off-by: Ayan Kumar Halder 
---

Changelog :-

v2 :- 1. Updated the rn register after reading from it. (Pointed by Julien,
 Stefano)
2. Used a union to represent the instruction opcode (Suggestd by Bertrand)
3. Fixed coding style issues (Pointed by Julien)
4. In the previous patch, I was updating dabt->sign based on the signedness of
imm9. This was incorrect. As mentioned in ARMv8 ARM  DDI 0487G.b, Page 3221,
SSE indicates the signedness of the data item loaded. In our case, the data item
loaded is always unsigned.

This has been tested for the following cases :-
ldr x2, [x1], #4

ldr w2, [x1], #-4

str x2, [x1], #4

str w2, [x1], #-4

The reason being  I am testing on 32bit MMIO registers only. I don't see a 8bit
or 16bit MMIO register.

 xen/arch/arm/decode.c | 68 ++-
 xen/arch/arm/decode.h |  3 +-
 xen/arch/arm/io.c | 40 +++
 xen/include/asm-arm/hsr.h | 26 +++
 4 files changed, 129 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/decode.c b/xen/arch/arm/decode.c
index 792c2e92a7..0b3e8fcbc6 100644
--- a/xen/arch/arm/decode.c
+++ b/xen/arch/arm/decode.c
@@ -84,6 +84,66 @@ bad_thumb2:
 return 1;
 }
 
+static int decode_32bit_loadstore_postindexing(register_t pc,
+   struct hsr_dabt *dabt,
+   union ldr_str_instr_class 
*instr)
+{
+if ( raw_copy_from_guest(>value, (void * __user)pc, sizeof (instr)) 
)
+return -EFAULT;
+
+/* First, let's check for the fixed values */
+if ( !((instr->code.fixed1 == 1) && (instr->code.fixed2 == 0) &&
+ (instr->code.fixed3 == 0) && (instr->code.fixed4 == 7)) )
+{
+gprintk(XENLOG_ERR, "Decoding not supported for instructions other 
than"
+" ldr/str post indexing\n");
+goto bad_32bit_loadstore;
+}
+
+if ( instr->code.size != 2 )
+{
+gprintk(XENLOG_ERR,
+"ldr/str post indexing is supported for 32 bit variant only\n");
+goto bad_32bit_loadstore;
+}
+
+if ( instr->code.v != 0 )
+{
+gprintk(XENLOG_ERR,
+"ldr/str post indexing for vector types are not supported\n");
+goto bad_32bit_loadstore;
+}
+
+/* Check for STR (immediate) - 32 bit variant */
+if ( instr->code.opc == 0 )
+{
+dabt->write = 1;
+}
+/* Check for LDR (immediate) - 32 bit variant */
+else if ( instr->code.opc == 1 )
+{
+dabt->write = 0;
+}
+else
+{
+gprintk(XENLOG_ERR,
+"Decoding ldr/str post indexing is not supported for this 
variant\n");
+goto bad_32bit_loadstore;
+}
+
+gprintk(XENLOG_INFO,
+"instr->code.rt = 0x%x, instr->code.size = 0x%x, instr->code.imm9 = 
%d\n",
+instr->code.rt, instr->code.size, instr->code.imm9);
+
+update_dabt(dabt, instr->code.rt, instr->code.size, false);
+dabt->valid = 1;
+
+return 0;
+bad_32bit_loadstore:
+gprintk(XENLOG_ERR, "unhandled 32bit Arm instruction 0x%x\n", 
instr->value);
+return 1;
+}
+
 static int decode_thumb(register_t pc, struct hsr_dabt *dabt)
 {
 uint16_t instr;
@@ -150,11 +210,17 @@ bad_thumb:
 return 1;
 }
 
-int decode_instruction(const struct cpu_user_regs *regs, struct hsr_dabt *dabt)
+int decode_instruction(const struct cpu_user_regs *regs, struct hsr_dabt *dabt,
+   union ldr_str_instr_class *instr)
 {
 if ( is_32bit_domain(current->domain) && regs->cpsr & PSR_THUMB )
 return decode_thumb(regs->pc, dabt);
 
+if ( (is_64bit_domain(current->domain) && !psr_mode_is_32bit(regs)) )
+{
+return decode_32bit_loadstore_postindexing(regs->pc, dabt, instr);
+}
+
 /* TODO: Handle ARM instruction */
 gprintk(XENLOG_ERR, "unhandled ARM instruction\n");
 
diff --git a/xen/arch/arm/decode.h b/xen/arch/arm/decode.h
index 4613763bdb..d82fc4a0f6 100644
--- a/xen/arch/arm/decode.h
+++ b/xen/arch/arm/decode.h
@@ -35,7 +35,8 @@
  */
 
 int decode_instruction(const struct cpu_user_regs *regs,
-   struct hsr_dabt *dabt);
+   struct hsr_dabt *dabt,
+   union ldr_str_instr_class *instr);
 
 #endif /* 

Re: Stable backports of Xen related patches

[Greg Kroah-Hartman]

On Mon, Nov 29, 2021 at 04:15:31PM +0100, Juergen Gross wrote:
> Hi Greg,
> 
> attached are git bundles for some patches you merged into the 5.10
> stable kernel already this morning.
> 
> Naming should be obvious, the patches are on the branch "back" in
> each bundle.

All now queued up, thanks!

greg k-h

Re: [PATCH] x86/boot: Drop incorrect mapping at l2_xenmap[0]

[Andrew Cooper]

On 29/11/2021 17:26, Andrew Cooper wrote:
> It has been 4 years since the default load address changed from 1M to 2M, and
> _stext ceased residing in l2_xenmap[0].  We should not be inserting an unused
> mapping.
>
> To ensure we don't create/remove mappings accidentally, loop from 0 and obey
> _PAGE_PRESENT on all entries.
>
> Fixes: 7ed93f3a0dff ("x86: change default load address from 1 MiB to 2 MiB")
> Signed-off-by: Andrew Cooper 
> ---
> CC: Jan Beulich 
> CC: Roger Pau Monné 
> CC: Wei Liu 
>
> I ought to have spotted this in c/s 52975142d154 ("x86/boot: Create the
> l2_xenmap[] mappings dynamically") too.
> ---
>  xen/arch/x86/setup.c | 10 +++---
>  1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index da47cdea14a1..6f241048425c 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1279,16 +1279,12 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>  
>  /* The only data mappings to be relocated are in the Xen area. */
>  pl2e = __va(__pa(l2_xenmap));
> -/*
> - * Undo the temporary-hooking of the l1_directmap.  
> __2M_text_start
> - * is contained in this PTE.
> - */
> +
>  BUG_ON(using_2M_mapping() &&
> l2_table_offset((unsigned long)_erodata) ==
> l2_table_offset((unsigned long)_stext));
> -*pl2e++ = l2e_from_pfn(xen_phys_start >> PAGE_SHIFT,
> -   PAGE_HYPERVISOR_RX | _PAGE_PSE);
> -for ( i = 1; i < L2_PAGETABLE_ENTRIES; i++, pl2e++ )
> +
> +for ( i = 0; i < L2_PAGETABLE_ENTRIES; i++, pl2e++ )
>  {
>  unsigned int flags;
>  

Actually, on further consideration, this hunk should be included too, to
cross-check that we don't zap any of the dynamically created mappings.

~Andrew

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 6f241048425c..5c8026b30e3e 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1320,7 +1320,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
 }
 else
 {
-    *pl2e = l2e_empty();
+    ASSERT_UNREACHABLE();
 continue;
 }
 

Re: [XEN PATCH 0/1] Add support for SMBIOS tables 7,8,9,26,27,28.

[Roger Pau Monné]

On Mon, Nov 29, 2021 at 12:59:28PM +, Anton Belousov wrote:
> This update is done to improve virtual machine stealth from malware. There 
> are AntiVM techniques that use WMI-queries to detect presence of this SMBIOS 
> tables. Example: 
> "https://github.com/LordNoteworthy/al-khaser/blob/master/al-khaser/AntiVM/Generic.cpp;

Aren't there many other hints at whether an OS is running inside of a
VM? I could imagine for example the ACPI tables, the list or models of
exposed devices, or the cpuid data?

Thanks, Roger.

Re: [patch 03/10] genirq/msi: Make MSI descriptor alloc/free ready for range allocations

[Thomas Gleixner]

On Sun, Nov 28 2021 at 20:17, Thomas Gleixner wrote:
> On Sun, Nov 28 2021 at 15:57, Marc Zyngier wrote:
> Hrm. The stupid search should terminated nevertheless. Let me stare at
> it again.

Found it. Just my inability to read xarray documentation.

Thanks,

tglx

Re: [XEN PATCH 1/1] Add suport for SMBIOS tables 7,8,9,26,27,28 to improve virtual machine stealth from malware.

[Roger Pau Monné]

Hello,

On Mon, Nov 29, 2021 at 12:59:29PM +, Anton Belousov wrote:

Than ks for the patch, I'm afraid this requires a proper commit
message and a Signed-off-by tag. See:

https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches#Title_and_description_of_the_patch

> ---
>  tools/firmware/hvmloader/smbios.c   | 146 
>  tools/firmware/hvmloader/smbios_types.h |  76 
>  2 files changed, 222 insertions(+)
> 
> diff --git a/tools/firmware/hvmloader/smbios.c 
> b/tools/firmware/hvmloader/smbios.c
> index 97a054e9e3..f5e61c1159 100644
> --- a/tools/firmware/hvmloader/smbios.c
> +++ b/tools/firmware/hvmloader/smbios.c
> @@ -33,12 +33,18 @@
>  #define SMBIOS_HANDLE_TYPE2   0x0200
>  #define SMBIOS_HANDLE_TYPE3   0x0300
>  #define SMBIOS_HANDLE_TYPE4   0x0400
> +#define SMBIOS_HANDLE_TYPE7   0x0700
> +#define SMBIOS_HANDLE_TYPE8   0x0800
> +#define SMBIOS_HANDLE_TYPE9   0x0900
>  #define SMBIOS_HANDLE_TYPE11  0x0B00
>  #define SMBIOS_HANDLE_TYPE16  0x1000
>  #define SMBIOS_HANDLE_TYPE17  0x1100
>  #define SMBIOS_HANDLE_TYPE19  0x1300
>  #define SMBIOS_HANDLE_TYPE20  0x1400
>  #define SMBIOS_HANDLE_TYPE22  0x1600
> +#define SMBIOS_HANDLE_TYPE26  0x1A00
> +#define SMBIOS_HANDLE_TYPE27  0x1B00
> +#define SMBIOS_HANDLE_TYPE28  0x1C00
>  #define SMBIOS_HANDLE_TYPE32  0x2000
>  #define SMBIOS_HANDLE_TYPE39  0x2700
>  #define SMBIOS_HANDLE_TYPE127 0x7f00
> @@ -77,6 +83,12 @@ static void *
>  smbios_type_4_init(void *start, unsigned int cpu_number,
> char *cpu_manufacturer);
>  static void *
> +smbios_type_7_init(void *start);
> +static void *
> +smbios_type_8_init(void *start);
> +static void *
> +smbios_type_9_init(void *start);
> +static void *
>  smbios_type_11_init(void *start);
>  static void *
>  smbios_type_16_init(void *start, uint32_t memory_size_mb, int nr_mem_devs);
> @@ -89,6 +101,12 @@ smbios_type_20_init(void *start, uint32_t memory_size_mb, 
> int instance);
>  static void *
>  smbios_type_22_init(void *start);
>  static void *
> +smbios_type_26_init(void *start);
> +static void *
> +smbios_type_27_init(void *start);
> +static void *
> +smbios_type_28_init(void *start);
> +static void *
>  smbios_type_32_init(void *start);
>  static void *
>  smbios_type_39_init(void *start);
> @@ -205,6 +223,9 @@ write_smbios_tables(void *ep, void *start,
>  do_struct(smbios_type_3_init(p));
>  for ( cpu_num = 1; cpu_num <= vcpus; cpu_num++ )
>  do_struct(smbios_type_4_init(p, cpu_num, cpu_manufacturer));
> +do_struct(smbios_type_7_init(p));
> +do_struct(smbios_type_8_init(p));
> +do_struct(smbios_type_9_init(p));
>  do_struct(smbios_type_11_init(p));
>  
>  /* Each 'memory device' covers up to 16GB of address space. */
> @@ -221,6 +242,9 @@ write_smbios_tables(void *ep, void *start,
>  }
>  
>  do_struct(smbios_type_22_init(p));
> +do_struct(smbios_type_26_init(p));
> +do_struct(smbios_type_28_init(p));
> +do_struct(smbios_type_27_init(p));
>  do_struct(smbios_type_32_init(p));
>  do_struct(smbios_type_39_init(p));
>  do_struct(smbios_type_vendor_oem_init(p));
> @@ -700,6 +724,66 @@ smbios_type_4_init(
>  return start+1;
>  }
>  
> +/* Type 7 -- Cache Information */
> +static void *
> +smbios_type_7_init(void *start)
> +{
> +struct smbios_type_7 *p = (struct smbios_type_7 *)start;
> +
> +void *pts;
> +uint32_t length;
> +
> +pts = get_smbios_pt_struct(7, );
> +if ( (pts != NULL)&&(length > 0) )
> +{
> +memcpy(start, pts, length);
> +p->header.handle = SMBIOS_HANDLE_TYPE7;
> +return (start + length);
> +}

Here and below for the added types: would it make sense to fill them
with some default information in the absence of any data passed in?

I'm afraid this requires some commit message in order to properly
review it.

Thanks, Roger.

[PATCH] x86/boot: Drop incorrect mapping at l2_xenmap[0]

[Andrew Cooper]

It has been 4 years since the default load address changed from 1M to 2M, and
_stext ceased residing in l2_xenmap[0].  We should not be inserting an unused
mapping.

To ensure we don't create/remove mappings accidentally, loop from 0 and obey
_PAGE_PRESENT on all entries.

Fixes: 7ed93f3a0dff ("x86: change default load address from 1 MiB to 2 MiB")
Signed-off-by: Andrew Cooper 
---
CC: Jan Beulich 
CC: Roger Pau Monné 
CC: Wei Liu 

I ought to have spotted this in c/s 52975142d154 ("x86/boot: Create the
l2_xenmap[] mappings dynamically") too.
---
 xen/arch/x86/setup.c | 10 +++---
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index da47cdea14a1..6f241048425c 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1279,16 +1279,12 @@ void __init noreturn __start_xen(unsigned long mbi_p)
 
 /* The only data mappings to be relocated are in the Xen area. */
 pl2e = __va(__pa(l2_xenmap));
-/*
- * Undo the temporary-hooking of the l1_directmap.  __2M_text_start
- * is contained in this PTE.
- */
+
 BUG_ON(using_2M_mapping() &&
l2_table_offset((unsigned long)_erodata) ==
l2_table_offset((unsigned long)_stext));
-*pl2e++ = l2e_from_pfn(xen_phys_start >> PAGE_SHIFT,
-   PAGE_HYPERVISOR_RX | _PAGE_PSE);
-for ( i = 1; i < L2_PAGETABLE_ENTRIES; i++, pl2e++ )
+
+for ( i = 0; i < L2_PAGETABLE_ENTRIES; i++, pl2e++ )
 {
 unsigned int flags;
 
-- 
2.11.0

Re: ACPI/UEFI support for Xen/ARM status?

[Elliott Mitchell]

On Mon, Nov 15, 2021 at 07:09:45PM +, Julien Grall wrote:
> On 15/11/2021 10:13, Jan Beulich wrote:
> > On 12.11.2021 17:02, Julien Grall wrote:
> >> On 12/11/2021 15:44, Elliott Mitchell wrote:
> >>>
> >>> Julien Grall and Stefano Stabellini had been proposing doing ACPI table
> >>> parsing in a stub domain, but I'm unaware of the status.  Not finding
> >>> much suggests it hasn't gone very far yet.
> >>
> >> This was a very early proposal in case we needed to parse the DSDT in
> >> Xen. This hasn't been needed so far, hence why this is not implemented
> >> and no-one worked on it.
> >>
> >> I am not very familiar how the framebuffer is detected in ACPI. Can you
> >> provide more details on what exactly you want to parse?
> > 
> > I don't think there's any ACPI support involved there. Instead UEFI data
> > needs propagating to Dom0, as that can't access EFI boot services itself.
> > At least this is all that's needed on the x86 side (and all the needed
> > code is there, just presumably not [fully] wired up on Arm).
> 
> Thanks for the feedback. At the moment, we don't enable EFI runtime 
> services nor propagate it to Dom0. So this needs to be wired up.
> 
> However, for Elliott's case, I am not sure this is going to sufficient. 
> The Raspberry PI has some devices that can only DMA into the first 1GB 
> of the RAM (the GPU seems to be one). So we need to make sure Xen is 
> allocating enough memory for Dom0 below that limit.
> 
> Do you have similar problem on x86? If so, how do you deal with it?

Is it just me or has anyone else commented you seem to have become
obsessed with DMA?  Otherwise are detail-oriented tendencies getting out
of control?

You keep bringing up DMA, but once the initial teething troubles (DMA
addresses versus memory addresses) were over there hadn't been any
issues attributed to DMA.  While bounce buffers are suboptimal, they are
good enough for most cases.

For DMA what most concerns me is the design of the allocate_memory_11().
The approach strongly favors *large* allocations.  Notably it requires a
chunk of 128MB or larger, if Domain 0 has been allocated at least 128MB.
If allocate_memory_11() finds a 128MB chunk it then prefers to enlarge
that chunk, rather than emphasizing allocating low addresses.

It has been a while since I last tried the Tianocore build, but there are
two crucial observations.  I don't recall the actual size, but Tianocore
was giving the framebuffer/GPU either 16MB or 64MB, and this was placed
below 512MB.  Combine this with the behavior of allocate_memory_11() and
guess what Xen/ARM/ACPI allocated to Domain 0.

I recall the lowest memory address being given to Domain 0 being above
the 512MB line.  I think it was at 768MB, but this is from my memory.
The rest of the memory allocated to Domain 0 was at higher addresses.

I would suggest allocate_memory_11()'s behavior needs to be adjusted
roughly as follows.  If a Domain 0 is allocated more than 256MB,
allocate_memory_11() should try for DMA-capable memory for at least half
of Domain 0's allocation (I'm unsure whether there should be 128MB of
non-DMA memory, versus only half).  I would emphasize lower addresses
over size for the DMA-capable memory.


I'm unsure how ACPI/UEFI/Tianocore were expressing the framebuffer
memory.  I've got the impression that memory allocation is marked
distinctly from the rest of main memory.

I don't know what was happening, but I suspect Xen had been leaving that
memory blank and never touching it (display never showed any output,
even pixel dust).  I'm suspecting Xen/ARM's ACPI implementation was
handling the specially marked memory by dropping it on the floor.
Instead of handing it to Domain 0 as a specialized region.


Could also be the complete lack of EFI runtime services was the problem.


Julien Grall might I inquire as to your general location/situation?  I
suspect giving you a Raspberry PI 4B could be *highly* valuable to the
Xen Project.  You would suddenly have a Xen-capable system with ACPI and
framebuffer.

A Raspberry PI 4B is cheap enough to need minimal expense justification.
I suspect you've got a spare TTL-serial on hand.  Only issue might be the
mini-HDMI.

I wouldn't dare send one without a SD Card, out of fear it might end up
being used with device-trees instead of Tianocore...


-- 
(\___(\___(\__  --=> 8-) EHM <=--  __/)___/)___/)
 \BS (| ehem+sig...@m5p.com  PGP 87145445 |)   /
  \_CS\   |  _  -O #include  O-   _  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445

Re: [PATCH 00/65] x86: Support for CET Indirect Branch Tracking

[Jan Beulich]

On 29.11.2021 16:09, Andrew Cooper wrote:
> On 29/11/2021 14:44, Jan Beulich wrote:
>> One question though: With the large number of __init functions gaining
>> cf_check, wouldn't it be possible to turn on CET-IBT only when we're
>> done using .init.text?
> 
> 233 to be precise.
> 
> GCC can't distinguish init from non-init functions as far as the
> improved typechecking (patch 56) goes, so omitting __init would cause
> compilation issues.

Oh, right. Should have been obvious to me, but wasn't.

Jan

> Furthermore, CET-IBT is only one Control Flow Integrity scheme
> attempting to use a nominally arch-neutral interface.  There are others
> (PaX RAP, and Intel's FineIBT) which have software components to them,
> and for those, the function pointer callers and callees need to have all
> appropriate ABI modifications.
> 
> I'm pretty certain that if we skipped annotation of the __init functions
> for now, we'd have to add them in due course anyway.
> 
> We could defer enabling CET-IBT until late on boot, but enabling it
> early gets us better coverage of issues until the first software scheme
> turns up.
> 
> ~Andrew
> 

Re: [RFC PATCH V3] xen/gnttab: Store frame GFN in struct page_info on Arm

[Oleksandr]

Hi Julien


[snip]






! Please note, there is still unresolved locking question here for 
which

I failed to find a suitable solution. So, it is still an RFC !

According to the internal conversation:
Now the GFN field in the struct page_info is accessed from
gnttab_set_frame_gfn() in the grant table code and from 
page_set_frame_gfn()

in the P2M code (the former uses the latter).

We need to prevent the concurrent access to this field. But, we 
cannot grab
the grant lock from the P2M code because we will introduce a lock 
inversion.
The page_set_frame_gfn() will be called from the P2M code with the 
p2m lock held
and then acquire the grant table lock. The gnttab_map_frame() will 
do the inverse.


This is a tricky one. I think, we will:

  1) Need to use the P2M lock to protect the access to the GFN in the 
struct page_info *.
  2) Defer the call to page_set_frame_gfn() from gnttab_map_frame() 
to xenmem_add_to_physmap_one()
  3) In xenmem_add_to_physmap_one() hold the P2M lock while checking 
the page was not already mapped (e.g. page_get_frame_gfn() == 
INVALID_GFN) and do the mapping. Call page_set_frame_gfn() on success.


This would still allow the guest to shot itself in the foot (e.g. 
potentially removing the wrong mapping) if it tries concurrent 
hypercall but I believe we would not introduce issue like XSA-380.


At the end this would look quite similar to how x86 deal with the M2P 
update.


Thank you for the suggestion, I need to analyze the code to better 
understand your idea and technical possibility to implement it, I will 
come up with questions if any.


I experimented a bit... Could you please clarify, is the code snippet 
below is close to what you meant?



diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index b594db4..dba9258 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1488,8 +1488,27 @@ int xenmem_add_to_physmap_one(
 return -ENOSYS;
 }

-    /* Map at new location. */
-    rc = guest_physmap_add_entry(d, gfn, mfn, 0, t);
+    if ( space != XENMAPSPACE_grant_table )
+    /* Map at new location. */
+    rc = guest_physmap_add_entry(d, gfn, mfn, 0, t);
+    else
+    {
+#ifdef CONFIG_GRANT_TABLE
+    struct p2m_domain *p2m = p2m_get_hostp2m(d);
+
+    p2m_write_lock(p2m);
+    if ( gfn_eq(page_get_frame_gfn(page), INVALID_GFN) )
+    {
+    rc = p2m_set_entry(p2m, gfn, 1, mfn, t, p2m->default_access);
+    if ( !rc )
+    page_set_frame_gfn(page, gfn);
+    }
+    p2m_write_unlock(p2m);
+#else
+    ASSERT_UNREACHABLE();
+    rc = -EINVAL;
+#endif
+    }

 /*
  * For XENMAPSPACE_gmfn_foreign if we failed to add the mapping, 
we need

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 59604b1..64e9e77 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -4167,10 +4167,8 @@ int gnttab_map_frame(struct domain *d, unsigned 
long idx, gfn_t gfn, mfn_t *mfn)
  * Make sure gnttab_unpopulate_status_frames() won't 
(successfully)

  * free the page until our caller has completed its operation.
  */
-    if ( get_page(mfn_to_page(*mfn), d) )
-    gnttab_set_frame_gfn(gt, status, idx, gfn);
-    else
-    rc = -EBUSY;
+    if ( !get_page(mfn_to_page(*mfn), d) )
+   rc = -EBUSY;
 }

 grant_write_unlock(gt);
(END)

If yes *and* I correctly understand the code, then looks like 
gnttab_set_frame_gfn becomes useless on Arm and can be dropped globally 
(x86's variant is already dummy).



[snip]

--
Regards,

Oleksandr Tyshchenko

[PATCH v5 07/12] libs/guest: introduce helper set cpu topology in cpu policy

[Roger Pau Monne]

This logic is pulled out from xc_cpuid_apply_policy and placed into a
separate helper. Note the legacy part of the introduced function, as
long term Xen will require a proper topology setter function capable
of expressing a more diverse set of topologies.

No functional change intended.

Signed-off-by: Roger Pau Monné 
---
 - s/xc_cpu_policy_topology/xc_cpu_policy_legacy_topology/
---
 tools/include/xenguest.h|   4 +
 tools/libs/guest/xg_cpuid_x86.c | 186 +---
 2 files changed, 105 insertions(+), 85 deletions(-)

diff --git a/tools/include/xenguest.h b/tools/include/xenguest.h
index 281454dc60..bea02cb542 100644
--- a/tools/include/xenguest.h
+++ b/tools/include/xenguest.h
@@ -821,6 +821,10 @@ bool xc_cpu_policy_is_compatible(xc_interface *xch, 
xc_cpu_policy_t *host,
 int xc_cpu_policy_make_compat_4_12(xc_interface *xch, xc_cpu_policy_t *policy,
bool hvm);
 
+/* Setup the legacy policy topology. */
+int xc_cpu_policy_legacy_topology(xc_interface *xch, xc_cpu_policy_t *policy,
+  bool hvm);
+
 int xc_get_cpu_levelling_caps(xc_interface *xch, uint32_t *caps);
 int xc_get_cpu_featureset(xc_interface *xch, uint32_t index,
   uint32_t *nr_features, uint32_t *featureset);
diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index bcbf9576c4..eafc1ec7c1 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -429,13 +429,11 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t 
domid, bool restore,
 {
 int rc;
 xc_dominfo_t di;
-unsigned int i, nr_leaves, nr_msrs;
+unsigned int nr_leaves, nr_msrs;
 xen_cpuid_leaf_t *leaves = NULL;
 struct cpuid_policy *p = NULL;
 struct xc_cpu_policy policy = { };
 uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
-uint32_t host_featureset[FEATURESET_NR_ENTRIES] = {};
-uint32_t len = ARRAY_SIZE(host_featureset);
 
 if ( xc_domain_getinfo(xch, domid, 1, ) != 1 ||
  di.domid != domid )
@@ -458,22 +456,6 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t 
domid, bool restore,
  (p = calloc(1, sizeof(*p))) == NULL )
 goto out;
 
-/* Get the host policy. */
-rc = xc_get_cpu_featureset(xch, XEN_SYSCTL_cpu_featureset_host,
-   , host_featureset);
-if ( rc )
-{
-/* Tolerate "buffer too small", as we've got the bits we need. */
-if ( errno == ENOBUFS )
-rc = 0;
-else
-{
-PERROR("Failed to obtain host featureset");
-rc = -errno;
-goto out;
-}
-}
-
 /* Get the domain's default policy. */
 nr_msrs = 0;
 rc = get_system_cpu_policy(xch, di.hvm ? XEN_SYSCTL_cpu_policy_hvm_default
@@ -557,72 +539,11 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t 
domid, bool restore,
 }
 }
 
-if ( !di.hvm )
-{
-/*
- * On hardware without CPUID Faulting, PV guests see real topology.
- * As a consequence, they also need to see the host htt/cmp fields.
- */
-p->basic.htt   = test_bit(X86_FEATURE_HTT, host_featureset);
-p->extd.cmp_legacy = test_bit(X86_FEATURE_CMP_LEGACY, host_featureset);
-}
-else
-{
-/*
- * Topology for HVM guests is entirely controlled by Xen.  For now, we
- * hardcode APIC_ID = vcpu_id * 2 to give the illusion of no SMT.
- */
-p->basic.htt = true;
-p->extd.cmp_legacy = false;
-
-/*
- * Leaf 1 EBX[23:16] is Maximum Logical Processors Per Package.
- * Update to reflect vLAPIC_ID = vCPU_ID * 2, but make sure to avoid
- * overflow.
- */
-if ( !p->basic.lppp )
-p->basic.lppp = 2;
-else if ( !(p->basic.lppp & 0x80) )
-p->basic.lppp *= 2;
-
-switch ( p->x86_vendor )
-{
-case X86_VENDOR_INTEL:
-for ( i = 0; (p->cache.subleaf[i].type &&
-  i < ARRAY_SIZE(p->cache.raw)); ++i )
-{
-p->cache.subleaf[i].cores_per_package =
-(p->cache.subleaf[i].cores_per_package << 1) | 1;
-p->cache.subleaf[i].threads_per_cache = 0;
-}
-break;
-
-case X86_VENDOR_AMD:
-case X86_VENDOR_HYGON:
-/*
- * Leaf 0x8008 ECX[15:12] is ApicIdCoreSize.
- * Leaf 0x8008 ECX[7:0] is NumberOfCores (minus one).
- * Update to reflect vLAPIC_ID = vCPU_ID * 2.  But avoid
- * - overflow,
- * - going out of sync with leaf 1 EBX[23:16],
- * - incrementing ApicIdCoreSize when it's zero (which changes the
- *   meaning of bits 7:0).
- *
- * UPDATE: I addition to avoiding overflow, some
- * proprietary operating systems have 

[PATCH v5 10/12] libs/{light,guest}: implement xc_cpuid_apply_policy in libxl

[Roger Pau Monne]

With the addition of the xc_cpu_policy_* now libxl can have better
control over the cpu policy, this allows removing the
xc_cpuid_apply_policy function and instead coding the required bits by
libxl in libxl__cpuid_legacy directly.

Remove xc_cpuid_apply_policy.

Signed-off-by: Roger Pau Monné 
Reviewed-by: Anthony PERARD 
---
Changes since v4:
 - Correctly account for PVH guests being HVM in libxl__cpuid_legacy.
 - PAE option is only available to HVM guests (_not_ including PVH).

Changes since v2:
 - Use 'r' for libxc return values.
 - Fix comment about making a cpu policy compatible.
 - Use LOG*D macros.
---
 tools/include/xenctrl.h |  18 -
 tools/libs/guest/xg_cpuid_x86.c | 122 
 tools/libs/light/libxl_cpuid.c  |  92 ++--
 3 files changed, 86 insertions(+), 146 deletions(-)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 95bd5eca67..745d67c970 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -1829,24 +1829,6 @@ struct xc_xend_cpuid {
 char *policy[4];
 };
 
-/*
- * Make adjustments to the CPUID settings for a domain.
- *
- * This path is used in two cases.  First, for fresh boots of the domain, and
- * secondly for migrate-in/restore of pre-4.14 guests (where CPUID data was
- * missing from the stream).  The @restore parameter distinguishes these
- * cases, and the generated policy must be compatible with a 4.13.
- *
- * Either pass a full new @featureset (and @nr_features), or adjust individual
- * features (@pae, @itsc, @nested_virt).
- *
- * Then (optionally) apply legacy XEND overrides (@xend) to the result.
- */
-int xc_cpuid_apply_policy(xc_interface *xch,
-  uint32_t domid, bool restore,
-  const uint32_t *featureset,
-  unsigned int nr_features, bool pae, bool itsc,
-  bool nested_virt, const struct xc_xend_cpuid *xend);
 int xc_mca_op(xc_interface *xch, struct xen_mc *mc);
 int xc_mca_op_inject_v2(xc_interface *xch, unsigned int flags,
 xc_cpumap_t cpumap, unsigned int nr_cpus);
diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index 41c6e66b6f..f4ed632d60 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -379,128 +379,6 @@ int xc_cpu_policy_apply_cpuid(xc_interface *xch, 
xc_cpu_policy_t *policy,
 return rc;
 }
 
-int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
-  const uint32_t *featureset, unsigned int nr_features,
-  bool pae, bool itsc, bool nested_virt,
-  const struct xc_xend_cpuid *cpuid)
-{
-int rc;
-xc_dominfo_t di;
-unsigned int nr_leaves, nr_msrs;
-xen_cpuid_leaf_t *leaves = NULL;
-struct cpuid_policy *p = NULL;
-struct xc_cpu_policy policy = { };
-uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
-
-if ( xc_domain_getinfo(xch, domid, 1, ) != 1 ||
- di.domid != domid )
-{
-ERROR("Failed to obtain d%d info", domid);
-rc = -ESRCH;
-goto out;
-}
-
-rc = xc_cpu_policy_get_size(xch, _leaves, _msrs);
-if ( rc )
-{
-PERROR("Failed to obtain policy info size");
-rc = -errno;
-goto out;
-}
-
-rc = -ENOMEM;
-if ( (leaves = calloc(nr_leaves, sizeof(*leaves))) == NULL ||
- (p = calloc(1, sizeof(*p))) == NULL )
-goto out;
-
-/* Get the domain's default policy. */
-nr_msrs = 0;
-rc = get_system_cpu_policy(xch, di.hvm ? XEN_SYSCTL_cpu_policy_hvm_default
-   : XEN_SYSCTL_cpu_policy_pv_default,
-   _leaves, leaves, _msrs, NULL);
-if ( rc )
-{
-PERROR("Failed to obtain %s default policy", di.hvm ? "hvm" : "pv");
-rc = -errno;
-goto out;
-}
-
-rc = x86_cpuid_copy_from_buffer(p, leaves, nr_leaves,
-_leaf, _subleaf);
-if ( rc )
-{
-ERROR("Failed to deserialise CPUID (err leaf %#x, subleaf %#x) (%d = 
%s)",
-  err_leaf, err_subleaf, -rc, strerror(-rc));
-goto out;
-}
-
-if ( restore )
-{
-policy.cpuid = *p;
-xc_cpu_policy_make_compat_4_12(xch, , di.hvm);
-*p = policy.cpuid;
-}
-
-if ( featureset )
-{
-policy.cpuid = *p;
-rc = xc_cpu_policy_apply_featureset(xch, , featureset,
-nr_features);
-if ( rc )
-{
-ERROR("Failed to apply featureset to policy");
-goto out;
-}
-*p = policy.cpuid;
-}
-else
-{
-p->extd.itsc = itsc;
-
-if ( di.hvm )
-{
-p->basic.pae = pae;
-p->basic.vmx = nested_virt;
-p->extd.svm = nested_virt;
-}
-}
-
-policy.cpuid = 

[PATCH v5 12/12] x86/CPUID: shrink max_{,sub}leaf fields according to actual leaf contents

[Roger Pau Monne]

From: Jan Beulich 

Zapping leaf data for out of range leaves is just one half of it: To
avoid guests (bogusly or worse) inferring information from mere leaf
presence, also shrink maximum indicators such that the respective
trailing entry is not all blank (unless of course it's the initial
subleaf of a leaf that's not the final one).

This is also in preparation of bumping the maximum basic leaf we
support, to ensure guests not getting exposed related features won't
observe a change in behavior.

Note that such shrinking is only done when creating a policy for a
domain from scratch. Migrated in domains keep their previous policy if
present untouched, and for migrated in domains not having CPUID data
the crafted Xen pre-4.13 policy is not trimmed to keep a behavior
compatible with those older Xen versions.

Signed-off-by: Jan Beulich 
Signed-off-by: Roger Pau Monné 
---
Changes since v4:
 - New in this version, picked up from 540d911c2813.
 - Only shrink policies for newly created domains.
---
 tools/include/xenguest.h |   3 +
 tools/libs/guest/xg_cpuid_x86.c  |   5 ++
 tools/libs/light/libxl_cpuid.c   |   7 ++
 tools/tests/cpu-policy/test-cpu-policy.c | 101 +++
 xen/include/xen/lib/x86/cpuid.h  |   7 ++
 xen/lib/x86/cpuid.c  |  39 +
 6 files changed, 162 insertions(+)

diff --git a/tools/include/xenguest.h b/tools/include/xenguest.h
index 3462d27516..e8b0d3ff16 100644
--- a/tools/include/xenguest.h
+++ b/tools/include/xenguest.h
@@ -830,6 +830,9 @@ int xc_cpu_policy_apply_featureset(xc_interface *xch, 
xc_cpu_policy_t *policy,
const uint32_t *featureset,
unsigned int nr_features);
 
+/* Sanitize a policy: can change the contents of the passed policy. */
+void xc_cpu_policy_sanitize(xc_interface *xch, xc_cpu_policy_t *policy);
+
 int xc_get_cpu_levelling_caps(xc_interface *xch, uint32_t *caps);
 int xc_get_cpu_featureset(xc_interface *xch, uint32_t index,
   uint32_t *nr_features, uint32_t *featureset);
diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index 7ce0a08580..eca844b921 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -690,3 +690,8 @@ int xc_cpu_policy_apply_featureset(xc_interface *xch, 
xc_cpu_policy_t *policy,
 
 return 0;
 }
+
+void xc_cpu_policy_sanitize(xc_interface *xch, xc_cpu_policy_t *policy)
+{
+x86_cpuid_policy_shrink_max_leaves(>cpuid);
+}
diff --git a/tools/libs/light/libxl_cpuid.c b/tools/libs/light/libxl_cpuid.c
index bf710ba196..5b2690a7d7 100644
--- a/tools/libs/light/libxl_cpuid.c
+++ b/tools/libs/light/libxl_cpuid.c
@@ -689,6 +689,13 @@ int libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, 
bool restore,
 goto out;
 }
 
+/*
+ * Do not attempt any modifications if creating a policy that aims to be
+ * compatible with pre-4.13 Xen versions.
+ */
+if (!restore)
+xc_cpu_policy_sanitize(ctx->xch, policy);
+
 r = xc_cpu_policy_set_domain(ctx->xch, domid, policy);
 if (r) {
 LOGED(ERROR, domid, "Failed to set domain CPUID policy");
diff --git a/tools/tests/cpu-policy/test-cpu-policy.c 
b/tools/tests/cpu-policy/test-cpu-policy.c
index 686d7a886c..20419a6108 100644
--- a/tools/tests/cpu-policy/test-cpu-policy.c
+++ b/tools/tests/cpu-policy/test-cpu-policy.c
@@ -8,10 +8,13 @@
 #include 
 
 #include 
+#include 
 #include 
 #include 
 #include 
 
+#define XSTATE_FP_SSE  (X86_XCR0_FP | X86_XCR0_SSE)
+
 static unsigned int nr_failures;
 #define fail(fmt, ...)  \
 ({  \
@@ -671,6 +674,103 @@ static void test_msr_get_entry(void)
 }
 }
 
+static void test_cpuid_maximum_leaf_shrinking(void)
+{
+static const struct test {
+const char *name;
+struct cpuid_policy p;
+} tests[] = {
+{
+.name = "basic",
+.p = {
+/* Very basic information only. */
+.basic.max_leaf = 1,
+.basic.raw_fms = 0xc2,
+},
+},
+{
+.name = "cache",
+.p = {
+/* Cache subleaves present. */
+.basic.max_leaf = 4,
+.cache.subleaf[0].type = 1,
+},
+},
+{
+.name = "feat#0",
+.p = {
+/* Subleaf 0 only with some valid bit. */
+.basic.max_leaf = 7,
+.feat.max_subleaf = 0,
+.feat.fsgsbase = 1,
+},
+},
+{
+.name = "feat#1",
+.p = {
+/* Subleaf 1 only with some valid bit. */
+.basic.max_leaf = 7,
+.feat.max_subleaf = 1,
+.feat.avx_vnni = 1,
+},
+},
+{
+.name = "topo",
+.p = {
+

[PATCH v5 11/12] libs/guest: (re)move xc_cpu_policy_apply_cpuid

[Roger Pau Monne]

Move the logic from xc_cpu_policy_apply_cpuid into libxl, now that the
xc_cpu_policy_* helpers allow modifying a cpu policy. By moving such
parsing into libxl directly we can get rid of xc_xend_cpuid, as libxl
will now implement it's own private type for storing CPUID
information, which currently matches xc_xend_cpuid.

Note the function logic is moved as-is, but requires adapting to the
libxl coding style.

No functional change intended.

Signed-off-by: Roger Pau Monné 
Reviewed-by: Anthony PERARD 
---
Changes since v2:
 - Use LOG*D.
 - Pass a gc to apply_policy.
 - Use 'r' for libxc return values.
---
 tools/include/libxl.h |   6 +-
 tools/include/xenctrl.h   |  26 --
 tools/include/xenguest.h  |   4 -
 tools/libs/guest/xg_cpuid_x86.c   | 125 --
 tools/libs/light/libxl_cpuid.c| 142 --
 tools/libs/light/libxl_internal.h |  26 ++
 6 files changed, 165 insertions(+), 164 deletions(-)

diff --git a/tools/include/libxl.h b/tools/include/libxl.h
index 2bbbd21f0b..8a8032ba25 100644
--- a/tools/include/libxl.h
+++ b/tools/include/libxl.h
@@ -1420,10 +1420,10 @@ void libxl_bitmap_init(libxl_bitmap *map);
 void libxl_bitmap_dispose(libxl_bitmap *map);
 
 /*
- * libxl_cpuid_policy is opaque in the libxl ABI.  Users of both libxl and
- * libxc may not make assumptions about xc_xend_cpuid.
+ * libxl_cpuid_policy is opaque in the libxl ABI. Users of libxl may not make
+ * assumptions about libxl__cpuid_policy.
  */
-typedef struct xc_xend_cpuid libxl_cpuid_policy;
+typedef struct libxl__cpuid_policy libxl_cpuid_policy;
 typedef libxl_cpuid_policy * libxl_cpuid_policy_list;
 void libxl_cpuid_dispose(libxl_cpuid_policy_list *cpuid_list);
 int libxl_cpuid_policy_list_length(const libxl_cpuid_policy_list *l);
diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 745d67c970..79169f8ace 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -1803,32 +1803,6 @@ int xc_domain_debug_control(xc_interface *xch,
 
 #if defined(__i386__) || defined(__x86_64__)
 
-/*
- * CPUID policy data, expressed in the legacy XEND format.
- *
- * Policy is an array of strings, 32 chars long:
- *   policy[0] = eax
- *   policy[1] = ebx
- *   policy[2] = ecx
- *   policy[3] = edx
- *
- * The format of the string is the following:
- *   '1' -> force to 1
- *   '0' -> force to 0
- *   'x' -> we don't care (use default)
- *   'k' -> pass through host value
- *   's' -> legacy alias for 'k'
- */
-struct xc_xend_cpuid {
-union {
-struct {
-uint32_t leaf, subleaf;
-};
-uint32_t input[2];
-};
-char *policy[4];
-};
-
 int xc_mca_op(xc_interface *xch, struct xen_mc *mc);
 int xc_mca_op_inject_v2(xc_interface *xch, unsigned int flags,
 xc_cpumap_t cpumap, unsigned int nr_cpus);
diff --git a/tools/include/xenguest.h b/tools/include/xenguest.h
index 8f05d8aa66..3462d27516 100644
--- a/tools/include/xenguest.h
+++ b/tools/include/xenguest.h
@@ -825,10 +825,6 @@ int xc_cpu_policy_make_compat_4_12(xc_interface *xch, 
xc_cpu_policy_t *policy,
 int xc_cpu_policy_legacy_topology(xc_interface *xch, xc_cpu_policy_t *policy,
   bool hvm);
 
-/* Apply an xc_xend_cpuid object to the policy. */
-int xc_cpu_policy_apply_cpuid(xc_interface *xch, xc_cpu_policy_t *policy,
-  const struct xc_xend_cpuid *cpuid, bool hvm);
-
 /* Apply a featureset to the policy. */
 int xc_cpu_policy_apply_featureset(xc_interface *xch, xc_cpu_policy_t *policy,
const uint32_t *featureset,
diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index f4ed632d60..7ce0a08580 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -254,131 +254,6 @@ int xc_set_domain_cpu_policy(xc_interface *xch, uint32_t 
domid,
 return ret;
 }
 
-int xc_cpu_policy_apply_cpuid(xc_interface *xch, xc_cpu_policy_t *policy,
-  const struct xc_xend_cpuid *cpuid, bool hvm)
-{
-int rc;
-xc_cpu_policy_t *host = NULL, *def = NULL;
-
-host = xc_cpu_policy_init();
-def = xc_cpu_policy_init();
-if ( !host || !def )
-{
-PERROR("Failed to init policies");
-rc = -ENOMEM;
-goto out;
-}
-
-/* Get the domain type's default policy. */
-rc = xc_cpu_policy_get_system(xch, hvm ? XEN_SYSCTL_cpu_policy_hvm_default
-   : XEN_SYSCTL_cpu_policy_pv_default,
-  def);
-if ( rc )
-{
-PERROR("Failed to obtain %s def policy", hvm ? "hvm" : "pv");
-goto out;
-}
-
-/* Get the host policy. */
-rc = xc_cpu_policy_get_system(xch, XEN_SYSCTL_cpu_policy_host, host);
-if ( rc )
-{
-PERROR("Failed to obtain host policy");
-goto out;
-}
-
-rc = -EINVAL;
-for ( ; cpuid->leaf != 

[PATCH v5 08/12] libs/guest: rework xc_cpuid_xend_policy

[Roger Pau Monne]

Rename xc_cpuid_xend_policy to xc_cpu_policy_apply_cpuid and make it
public. Modify the function internally to use the new xc_cpu_policy_*
set of functions. Also don't apply the passed policy to a domain
directly, and instead modify the provided xc_cpu_policy_t. The caller
will be responsible of applying the modified cpu policy to the domain.

Note that further patches will end up removing this function, as the
callers should have the necessary helpers to modify an xc_cpu_policy_t
themselves.

The find_leaf helper and related comparison function is also removed,
as it's no longer needed to search for cpuid leafs as finding the
matching leaves is now done using xc_cpu_policy_get_cpuid.

No functional change intended.

Signed-off-by: Roger Pau Monné 
---
Changes since v3:
 - Drop find_leaf and comparison helper.
---
 tools/include/xenguest.h|   4 +
 tools/libs/guest/xg_cpuid_x86.c | 200 +---
 2 files changed, 83 insertions(+), 121 deletions(-)

diff --git a/tools/include/xenguest.h b/tools/include/xenguest.h
index bea02cb542..9912116a51 100644
--- a/tools/include/xenguest.h
+++ b/tools/include/xenguest.h
@@ -825,6 +825,10 @@ int xc_cpu_policy_make_compat_4_12(xc_interface *xch, 
xc_cpu_policy_t *policy,
 int xc_cpu_policy_legacy_topology(xc_interface *xch, xc_cpu_policy_t *policy,
   bool hvm);
 
+/* Apply an xc_xend_cpuid object to the policy. */
+int xc_cpu_policy_apply_cpuid(xc_interface *xch, xc_cpu_policy_t *policy,
+  const struct xc_xend_cpuid *cpuid, bool hvm);
+
 int xc_get_cpu_levelling_caps(xc_interface *xch, uint32_t *caps);
 int xc_get_cpu_featureset(xc_interface *xch, uint32_t index,
   uint32_t *nr_features, uint32_t *featureset);
diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index eafc1ec7c1..121bce1a92 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -254,144 +254,107 @@ int xc_set_domain_cpu_policy(xc_interface *xch, 
uint32_t domid,
 return ret;
 }
 
-static int compare_leaves(const void *l, const void *r)
-{
-const xen_cpuid_leaf_t *lhs = l;
-const xen_cpuid_leaf_t *rhs = r;
-
-if ( lhs->leaf != rhs->leaf )
-return lhs->leaf < rhs->leaf ? -1 : 1;
-
-if ( lhs->subleaf != rhs->subleaf )
-return lhs->subleaf < rhs->subleaf ? -1 : 1;
-
-return 0;
-}
-
-static xen_cpuid_leaf_t *find_leaf(
-xen_cpuid_leaf_t *leaves, unsigned int nr_leaves,
-const struct xc_xend_cpuid *xend)
-{
-const xen_cpuid_leaf_t key = { xend->leaf, xend->subleaf };
-
-return bsearch(, leaves, nr_leaves, sizeof(*leaves), compare_leaves);
-}
-
-static int xc_cpuid_xend_policy(
-xc_interface *xch, uint32_t domid, const struct xc_xend_cpuid *xend)
+int xc_cpu_policy_apply_cpuid(xc_interface *xch, xc_cpu_policy_t *policy,
+  const struct xc_xend_cpuid *cpuid, bool hvm)
 {
 int rc;
-xc_dominfo_t di;
-unsigned int nr_leaves, nr_msrs;
-uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
-/*
- * Three full policies.  The host, default for the domain type,
- * and domain current.
- */
-xen_cpuid_leaf_t *host = NULL, *def = NULL, *cur = NULL;
-unsigned int nr_host, nr_def, nr_cur;
+xc_cpu_policy_t *host = NULL, *def = NULL;
 
-if ( xc_domain_getinfo(xch, domid, 1, ) != 1 ||
- di.domid != domid )
-{
-ERROR("Failed to obtain d%d info", domid);
-rc = -ESRCH;
-goto fail;
-}
-
-rc = xc_cpu_policy_get_size(xch, _leaves, _msrs);
-if ( rc )
-{
-PERROR("Failed to obtain policy info size");
-rc = -errno;
-goto fail;
-}
-
-rc = -ENOMEM;
-if ( (host = calloc(nr_leaves, sizeof(*host))) == NULL ||
- (def  = calloc(nr_leaves, sizeof(*def)))  == NULL ||
- (cur  = calloc(nr_leaves, sizeof(*cur)))  == NULL )
-{
-ERROR("Unable to allocate memory for %u CPUID leaves", nr_leaves);
-goto fail;
-}
-
-/* Get the domain's current policy. */
-nr_msrs = 0;
-nr_cur = nr_leaves;
-rc = get_domain_cpu_policy(xch, domid, _cur, cur, _msrs, NULL);
-if ( rc )
+host = xc_cpu_policy_init();
+def = xc_cpu_policy_init();
+if ( !host || !def )
 {
-PERROR("Failed to obtain d%d current policy", domid);
-rc = -errno;
-goto fail;
+PERROR("Failed to init policies");
+rc = -ENOMEM;
+goto out;
 }
 
 /* Get the domain type's default policy. */
-nr_msrs = 0;
-nr_def = nr_leaves;
-rc = get_system_cpu_policy(xch, di.hvm ? XEN_SYSCTL_cpu_policy_hvm_default
+rc = xc_cpu_policy_get_system(xch, hvm ? XEN_SYSCTL_cpu_policy_hvm_default
: XEN_SYSCTL_cpu_policy_pv_default,
-   _def, def, _msrs, NULL);
+  def);
 if ( rc )
 {
- 

[PATCH v5 09/12] libs/guest: apply a featureset into a cpu policy

[Roger Pau Monne]

Pull out the code from xc_cpuid_apply_policy that applies a featureset
to a cpu policy and place it on it's own standalone function that's
part of the public interface.

No functional change intended.

Signed-off-by: Roger Pau Monné 
---
 tools/include/xenguest.h|  5 ++
 tools/libs/guest/xg_cpuid_x86.c | 95 -
 2 files changed, 62 insertions(+), 38 deletions(-)

diff --git a/tools/include/xenguest.h b/tools/include/xenguest.h
index 9912116a51..8f05d8aa66 100644
--- a/tools/include/xenguest.h
+++ b/tools/include/xenguest.h
@@ -829,6 +829,11 @@ int xc_cpu_policy_legacy_topology(xc_interface *xch, 
xc_cpu_policy_t *policy,
 int xc_cpu_policy_apply_cpuid(xc_interface *xch, xc_cpu_policy_t *policy,
   const struct xc_xend_cpuid *cpuid, bool hvm);
 
+/* Apply a featureset to the policy. */
+int xc_cpu_policy_apply_featureset(xc_interface *xch, xc_cpu_policy_t *policy,
+   const uint32_t *featureset,
+   unsigned int nr_features);
+
 int xc_get_cpu_levelling_caps(xc_interface *xch, uint32_t *caps);
 int xc_get_cpu_featureset(xc_interface *xch, uint32_t index,
   uint32_t *nr_features, uint32_t *featureset);
diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index 121bce1a92..41c6e66b6f 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -443,46 +443,15 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t 
domid, bool restore,
 
 if ( featureset )
 {
-uint32_t disabled_features[FEATURESET_NR_ENTRIES],
-feat[FEATURESET_NR_ENTRIES] = {};
-static const uint32_t deep_features[] = INIT_DEEP_FEATURES;
-unsigned int i, b;
-
-/*
- * The user supplied featureset may be shorter or longer than
- * FEATURESET_NR_ENTRIES.  Shorter is fine, and we will zero-extend.
- * Longer is fine, so long as it only padded with zeros.
- */
-unsigned int user_len = min(FEATURESET_NR_ENTRIES + 0u, nr_features);
-
-/* Check for truncated set bits. */
-rc = -EOPNOTSUPP;
-for ( i = user_len; i < nr_features; ++i )
-if ( featureset[i] != 0 )
-goto out;
-
-memcpy(feat, featureset, sizeof(*featureset) * user_len);
-
-/* Disable deep dependencies of disabled features. */
-for ( i = 0; i < ARRAY_SIZE(disabled_features); ++i )
-disabled_features[i] = ~feat[i] & deep_features[i];
-
-for ( b = 0; b < sizeof(disabled_features) * CHAR_BIT; ++b )
+policy.cpuid = *p;
+rc = xc_cpu_policy_apply_featureset(xch, , featureset,
+nr_features);
+if ( rc )
 {
-const uint32_t *dfs;
-
-if ( !test_bit(b, disabled_features) ||
- !(dfs = x86_cpuid_lookup_deep_deps(b)) )
-continue;
-
-for ( i = 0; i < ARRAY_SIZE(disabled_features); ++i )
-{
-feat[i] &= ~dfs[i];
-disabled_features[i] &= ~dfs[i];
-}
+ERROR("Failed to apply featureset to policy");
+goto out;
 }
-
-cpuid_featureset_to_policy(feat, p);
+*p = policy.cpuid;
 }
 else
 {
@@ -918,3 +887,53 @@ int xc_cpu_policy_legacy_topology(xc_interface *xch, 
xc_cpu_policy_t *policy,
 
 return 0;
 }
+
+int xc_cpu_policy_apply_featureset(xc_interface *xch, xc_cpu_policy_t *policy,
+   const uint32_t *featureset,
+   unsigned int nr_features)
+{
+uint32_t disabled_features[FEATURESET_NR_ENTRIES],
+feat[FEATURESET_NR_ENTRIES] = {};
+static const uint32_t deep_features[] = INIT_DEEP_FEATURES;
+unsigned int i, b;
+
+/*
+ * The user supplied featureset may be shorter or longer than
+ * FEATURESET_NR_ENTRIES.  Shorter is fine, and we will zero-extend.
+ * Longer is fine, so long as it only padded with zeros.
+ */
+unsigned int user_len = min(FEATURESET_NR_ENTRIES + 0u, nr_features);
+
+/* Check for truncated set bits. */
+for ( i = user_len; i < nr_features; ++i )
+if ( featureset[i] != 0 )
+{
+errno = EOPNOTSUPP;
+return -1;
+}
+
+memcpy(feat, featureset, sizeof(*featureset) * user_len);
+
+/* Disable deep dependencies of disabled features. */
+for ( i = 0; i < ARRAY_SIZE(disabled_features); ++i )
+disabled_features[i] = ~feat[i] & deep_features[i];
+
+for ( b = 0; b < sizeof(disabled_features) * CHAR_BIT; ++b )
+{
+const uint32_t *dfs;
+
+if ( !test_bit(b, disabled_features) ||
+ !(dfs = x86_cpuid_lookup_deep_deps(b)) )
+continue;
+
+for ( i = 0; i < ARRAY_SIZE(disabled_features); ++i )
+{
+feat[i] &= ~dfs[i];
+   

[PATCH v5 05/12] libs/guest: allow fetching a specific MSR entry from a cpu policy

[Roger Pau Monne]

Introduce an interface that returns a specific MSR entry from a cpu
policy in xen_msr_entry_t format.

This is useful to callers can peek data from the opaque
xc_cpu_policy_t type.

No caller of the interface introduced on this patch.

Signed-off-by: Roger Pau Monné 
---
Changes since v3:
 - Use x86_msr_get_entry.

Changes since v1:
 - Introduce a helper to perform a binary search of the MSR entries
   array.
---
 tools/include/xenguest.h|  2 ++
 tools/libs/guest/xg_cpuid_x86.c | 20 
 2 files changed, 22 insertions(+)

diff --git a/tools/include/xenguest.h b/tools/include/xenguest.h
index 0a6fd99306..2672fd043c 100644
--- a/tools/include/xenguest.h
+++ b/tools/include/xenguest.h
@@ -810,6 +810,8 @@ int xc_cpu_policy_update_msrs(xc_interface *xch, 
xc_cpu_policy_t *policy,
 int xc_cpu_policy_get_cpuid(xc_interface *xch, const xc_cpu_policy_t *policy,
 uint32_t leaf, uint32_t subleaf,
 xen_cpuid_leaf_t *out);
+int xc_cpu_policy_get_msr(xc_interface *xch, const xc_cpu_policy_t *policy,
+  uint32_t msr, xen_msr_entry_t *out);
 
 /* Compatibility calculations. */
 bool xc_cpu_policy_is_compatible(xc_interface *xch, xc_cpu_policy_t *host,
diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index 7779a3e1dd..859c885c15 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -878,6 +878,26 @@ int xc_cpu_policy_get_cpuid(xc_interface *xch, const 
xc_cpu_policy_t *policy,
 return 0;
 }
 
+int xc_cpu_policy_get_msr(xc_interface *xch, const xc_cpu_policy_t *policy,
+  uint32_t msr, xen_msr_entry_t *out)
+{
+const uint64_t *val;
+
+*out = (xen_msr_entry_t){};
+
+val = x86_msr_get_entry(>msr, msr);
+if ( !val )
+{
+errno = ENOENT;
+return -1;
+}
+
+out->idx = msr;
+out->val = *val;
+
+return 0;
+}
+
 bool xc_cpu_policy_is_compatible(xc_interface *xch, xc_cpu_policy_t *host,
  xc_cpu_policy_t *guest)
 {
-- 
2.33.0

[PATCH v5 06/12] libs/guest: make a cpu policy compatible with older Xen versions

[Roger Pau Monne]

Older Xen versions used to expose some CPUID bits which are no longer
exposed by default. In order to keep a compatible behavior with
guests migrated from versions of Xen that don't encode the CPUID data
on the migration stream introduce a function that sets the same bits
as older Xen versions.

This is pulled out from xc_cpuid_apply_policy which already has this
logic present.

No functional change intended.

Signed-off-by: Roger Pau Monné 
---
Changes since v3:
 - Rename function to xc_cpu_policy_make_compat_4_12.

Changes since v1:
 - Move comments and explicitly mention pre-4.13 Xen.
---
 tools/include/xenguest.h|  4 +++
 tools/libs/guest/xg_cpuid_x86.c | 62 -
 2 files changed, 49 insertions(+), 17 deletions(-)

diff --git a/tools/include/xenguest.h b/tools/include/xenguest.h
index 2672fd043c..281454dc60 100644
--- a/tools/include/xenguest.h
+++ b/tools/include/xenguest.h
@@ -817,6 +817,10 @@ int xc_cpu_policy_get_msr(xc_interface *xch, const 
xc_cpu_policy_t *policy,
 bool xc_cpu_policy_is_compatible(xc_interface *xch, xc_cpu_policy_t *host,
  xc_cpu_policy_t *guest);
 
+/* Make a policy compatible with pre-4.13 Xen versions. */
+int xc_cpu_policy_make_compat_4_12(xc_interface *xch, xc_cpu_policy_t *policy,
+   bool hvm);
+
 int xc_get_cpu_levelling_caps(xc_interface *xch, uint32_t *caps);
 int xc_get_cpu_featureset(xc_interface *xch, uint32_t index,
   uint32_t *nr_features, uint32_t *featureset);
diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index 859c885c15..bcbf9576c4 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -432,6 +432,7 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t 
domid, bool restore,
 unsigned int i, nr_leaves, nr_msrs;
 xen_cpuid_leaf_t *leaves = NULL;
 struct cpuid_policy *p = NULL;
+struct xc_cpu_policy policy = { };
 uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
 uint32_t host_featureset[FEATURESET_NR_ENTRIES] = {};
 uint32_t len = ARRAY_SIZE(host_featureset);
@@ -496,23 +497,9 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t 
domid, bool restore,
 
 if ( restore )
 {
-/*
- * Account for feature which have been disabled by default since Xen 
4.13,
- * so migrated-in VM's don't risk seeing features disappearing.
- */
-p->basic.rdrand = test_bit(X86_FEATURE_RDRAND, host_featureset);
-p->feat.hle = test_bit(X86_FEATURE_HLE, host_featureset);
-p->feat.rtm = test_bit(X86_FEATURE_RTM, host_featureset);
-
-if ( di.hvm )
-{
-p->feat.mpx = test_bit(X86_FEATURE_MPX, host_featureset);
-}
-
-/* Clamp maximum leaves to the ones supported on 4.12. */
-p->basic.max_leaf = min(p->basic.max_leaf, 0xdu);
-p->feat.max_subleaf = 0;
-p->extd.max_leaf = min(p->extd.max_leaf, 0x801c);
+policy.cpuid = *p;
+xc_cpu_policy_make_compat_4_12(xch, , di.hvm);
+*p = policy.cpuid;
 }
 
 if ( featureset )
@@ -916,3 +903,44 @@ bool xc_cpu_policy_is_compatible(xc_interface *xch, 
xc_cpu_policy_t *host,
 
 return false;
 }
+
+int xc_cpu_policy_make_compat_4_12(xc_interface *xch, xc_cpu_policy_t *policy,
+   bool hvm)
+{
+xc_cpu_policy_t *host;
+int rc;
+
+host = xc_cpu_policy_init();
+if ( !host )
+{
+errno = ENOMEM;
+return -1;
+}
+
+rc = xc_cpu_policy_get_system(xch, XEN_SYSCTL_cpu_policy_host, host);
+if ( rc )
+{
+ERROR("Failed to get host policy");
+goto out;
+}
+
+/*
+ * Account for features which have been disabled by default since Xen 4.13,
+ * so migrated-in VM's don't risk seeing features disappearing.
+ */
+policy->cpuid.basic.rdrand = host->cpuid.basic.rdrand;
+policy->cpuid.feat.hle = host->cpuid.feat.hle;
+policy->cpuid.feat.rtm = host->cpuid.feat.rtm;
+
+if ( hvm )
+policy->cpuid.feat.mpx = host->cpuid.feat.mpx;
+
+/* Clamp maximum leaves to the ones supported on pre-4.13. */
+policy->cpuid.basic.max_leaf = min(policy->cpuid.basic.max_leaf, 0xdu);
+policy->cpuid.feat.max_subleaf = 0;
+policy->cpuid.extd.max_leaf = min(policy->cpuid.extd.max_leaf, 0x801c);
+
+ out:
+xc_cpu_policy_destroy(host);
+return rc;
+}
-- 
2.33.0

[PATCH v5 04/12] libx86: introduce helper to fetch msr entry

[Roger Pau Monne]

Use such helper in order to replace the code in
x86_msr_copy_from_buffer. Note the introduced helper should not be
directly called and instead x86_msr_get_entry should be used that will
properly deal with const and non-const inputs.

Note this requires making the raw fields uint64_t so that it can
accommodate the maximum size of MSRs values, and in turn removing the
truncation tests.

Suggested-by: Andrew Cooper 
Signed-off-by: Roger Pau Monné 
---
Changes since v4:
 - Rename _x86_msr_get_entry to x86_msr_get_entry_const.
 - Add newline before endif.

Changes since v3:
 - New in this version.
---
 tools/tests/cpu-policy/test-cpu-policy.c | 48 +++-
 xen/include/xen/lib/x86/msr.h| 20 +-
 xen/lib/x86/msr.c| 41 ++--
 3 files changed, 76 insertions(+), 33 deletions(-)

diff --git a/tools/tests/cpu-policy/test-cpu-policy.c 
b/tools/tests/cpu-policy/test-cpu-policy.c
index 3f777fc1fc..686d7a886c 100644
--- a/tools/tests/cpu-policy/test-cpu-policy.c
+++ b/tools/tests/cpu-policy/test-cpu-policy.c
@@ -386,16 +386,6 @@ static void test_msr_deserialise_failure(void)
 .msr = { .idx = 0xce, .flags = 1 },
 .rc = -EINVAL,
 },
-{
-.name = "truncated val",
-.msr = { .idx = 0xce, .val = ~0ull },
-.rc = -EOVERFLOW,
-},
-{
-.name = "truncated val",
-.msr = { .idx = 0x10a, .val = ~0ull },
-.rc = -EOVERFLOW,
-},
 };
 
 printf("Testing MSR deserialise failure:\n");
@@ -644,6 +634,43 @@ static void test_cpuid_get_leaf_failure(void)
 }
 }
 
+static void test_msr_get_entry(void)
+{
+static const struct test {
+const char *name;
+unsigned int idx;
+bool success;
+} tests[] = {
+{
+.name = "bad msr index",
+.idx = -1,
+},
+{
+.name = "good msr index",
+.idx = 0xce,
+.success = true,
+},
+};
+const struct msr_policy pc;
+const uint64_t *ec;
+struct msr_policy p;
+uint64_t *e;
+
+/* Constness build test. */
+ec = x86_msr_get_entry(, 0);
+e = x86_msr_get_entry(, 0);
+
+printf("Testing MSR get leaf:\n");
+
+for ( size_t i = 0; i < ARRAY_SIZE(tests); ++i )
+{
+const struct test *t = [i];
+
+if ( !!x86_msr_get_entry(, t->idx) != t->success )
+fail("  Test %s failed\n", t->name);
+}
+}
+
 static void test_is_compatible_success(void)
 {
 static struct test {
@@ -763,6 +790,7 @@ int main(int argc, char **argv)
 
 test_msr_serialise_success();
 test_msr_deserialise_failure();
+test_msr_get_entry();
 
 test_is_compatible_success();
 test_is_compatible_failure();
diff --git a/xen/include/xen/lib/x86/msr.h b/xen/include/xen/lib/x86/msr.h
index 48ba4a59c0..4d84b7cf27 100644
--- a/xen/include/xen/lib/x86/msr.h
+++ b/xen/include/xen/lib/x86/msr.h
@@ -17,7 +17,7 @@ struct msr_policy
  * is dependent on real hardware support.
  */
 union {
-uint32_t raw;
+uint64_t raw;
 struct {
 uint32_t :31;
 bool cpuid_faulting:1;
@@ -32,7 +32,7 @@ struct msr_policy
  * fixed in hardware.
  */
 union {
-uint32_t raw;
+uint64_t raw;
 struct {
 bool rdcl_no:1;
 bool ibrs_all:1;
@@ -91,6 +91,22 @@ int x86_msr_copy_from_buffer(struct msr_policy *policy,
  const msr_entry_buffer_t msrs, uint32_t 
nr_entries,
  uint32_t *err_msr);
 
+/**
+ * Get a MSR entry from a policy object.
+ *
+ * @param policy  The msr_policy object.
+ * @param idx The index.
+ * @returns a pointer to the requested leaf or NULL in case of error.
+ *
+ * Do not call this function directly and instead use x86_msr_get_entry that
+ * will deal with both const and non-const policies returning a pointer with
+ * constness matching that of the input.
+ */
+const uint64_t *x86_msr_get_entry_const(const struct msr_policy *policy,
+uint32_t idx);
+#define x86_msr_get_entry(p, i) \
+((__typeof__(&(p)->platform_info.raw))x86_msr_get_entry_const(p, i))
+
 #endif /* !XEN_LIB_X86_MSR_H */
 
 /*
diff --git a/xen/lib/x86/msr.c b/xen/lib/x86/msr.c
index 7d71e92a38..e9b337dd70 100644
--- a/xen/lib/x86/msr.c
+++ b/xen/lib/x86/msr.c
@@ -74,6 +74,8 @@ int x86_msr_copy_from_buffer(struct msr_policy *p,
 
 for ( i = 0; i < nr_entries; i++ )
 {
+uint64_t *val;
+
 if ( copy_from_buffer_offset(, msrs, i, 1) )
 return -EFAULT;
 
@@ -83,31 +85,13 @@ int x86_msr_copy_from_buffer(struct msr_policy *p,
 goto err;
 }
 
-switch ( data.idx )
+val = x86_msr_get_entry(p, data.idx);
+if ( !val )
 {
-/*
- * Assign data.val to p->field, checking for 

[PATCH v5 03/12] libs/guest: allow fetching a specific CPUID leaf from a cpu policy

[Roger Pau Monne]

Introduce an interface that returns a specific leaf/subleaf from a cpu
policy in xen_cpuid_leaf_t format.

This is useful to callers can peek data from the opaque
xc_cpu_policy_t type.

No caller of the interface introduced on this patch.

Note that callers of find_leaf need to be slightly adjusted to use the
new helper parameters.

Signed-off-by: Roger Pau Monné 
---
Changes since v3:
 - Use x86_cpuid_get_leaf.

Changes since v1:
 - Use find leaf.
---
 tools/include/xenguest.h|  3 +++
 tools/libs/guest/xg_cpuid_x86.c | 23 +++
 2 files changed, 26 insertions(+)

diff --git a/tools/include/xenguest.h b/tools/include/xenguest.h
index e01f494b77..0a6fd99306 100644
--- a/tools/include/xenguest.h
+++ b/tools/include/xenguest.h
@@ -807,6 +807,9 @@ int xc_cpu_policy_update_cpuid(xc_interface *xch, 
xc_cpu_policy_t *policy,
uint32_t nr);
 int xc_cpu_policy_update_msrs(xc_interface *xch, xc_cpu_policy_t *policy,
   const xen_msr_entry_t *msrs, uint32_t nr);
+int xc_cpu_policy_get_cpuid(xc_interface *xch, const xc_cpu_policy_t *policy,
+uint32_t leaf, uint32_t subleaf,
+xen_cpuid_leaf_t *out);
 
 /* Compatibility calculations. */
 bool xc_cpu_policy_is_compatible(xc_interface *xch, xc_cpu_policy_t *host,
diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index b9e827ce7e..7779a3e1dd 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -855,6 +855,29 @@ int xc_cpu_policy_update_msrs(xc_interface *xch, 
xc_cpu_policy_t *policy,
 return rc;
 }
 
+int xc_cpu_policy_get_cpuid(xc_interface *xch, const xc_cpu_policy_t *policy,
+uint32_t leaf, uint32_t subleaf,
+xen_cpuid_leaf_t *out)
+{
+const struct cpuid_leaf *tmp;
+
+tmp = x86_cpuid_get_leaf(>cpuid, leaf, subleaf);
+if ( !tmp )
+{
+/* Unable to find a matching leaf. */
+errno = ENOENT;
+return -1;
+}
+
+out->leaf = leaf;
+out->subleaf = subleaf;
+out->a = tmp->a;
+out->b = tmp->b;
+out->c = tmp->c;
+out->d = tmp->d;
+return 0;
+}
+
 bool xc_cpu_policy_is_compatible(xc_interface *xch, xc_cpu_policy_t *host,
  xc_cpu_policy_t *guest)
 {
-- 
2.33.0

[PATCH v5 02/12] libx86: introduce helper to fetch cpuid leaf

[Roger Pau Monne]

Introduce a helper based on the current Xen guest_cpuid code in order
to fetch a cpuid leaf from a policy. The newly introduced function in
cpuid.c should not be directly called and instead the provided
x86_cpuid_get_leaf macro should be used that will properly deal with
const and non-const inputs.

Also add a test to check that the introduced helper doesn't go over
the bounds of the policy.

Note the code in x86_cpuid_copy_from_buffer is not switched to use the
new function because of the boundary checks against the max fields of
the policy, which might not be properly set at the point where
x86_cpuid_copy_from_buffer get called, for example when filling an
empty policy from scratch.

Suggested-by: Andrew Cooper 
Signed-off-by: Roger Pau Monné 
---
Changes since v4:
 - Rename _x86_cpuid_get_leaf to x86_cpuid_get_leaf_const.

Changes since v3:
 - New in this version.
---
Regarding safety of the usage of array_access_nospec to obtain a
pointer to an element of an array, there are already other instances
of this usage, for example in viridian_time_wrmsr, so I would assume
this is fine.
---
 tools/tests/cpu-policy/test-cpu-policy.c | 75 
 xen/arch/x86/cpuid.c | 55 +++--
 xen/include/xen/lib/x86/cpuid.h  | 19 ++
 xen/lib/x86/cpuid.c  | 52 
 4 files changed, 153 insertions(+), 48 deletions(-)

diff --git a/tools/tests/cpu-policy/test-cpu-policy.c 
b/tools/tests/cpu-policy/test-cpu-policy.c
index ed450a0997..3f777fc1fc 100644
--- a/tools/tests/cpu-policy/test-cpu-policy.c
+++ b/tools/tests/cpu-policy/test-cpu-policy.c
@@ -570,6 +570,80 @@ static void test_cpuid_out_of_range_clearing(void)
 }
 }
 
+static void test_cpuid_get_leaf_failure(void)
+{
+static const struct test {
+struct cpuid_policy p;
+const char *name;
+uint32_t leaf, subleaf;
+} tests[] = {
+/* Bound checking logic. */
+{
+.name = "Basic max leaf >= array size",
+.p = {
+.basic.max_leaf = CPUID_GUEST_NR_BASIC,
+},
+},
+{
+.name = "Feature max leaf >= array size",
+.p = {
+.basic.max_leaf = CPUID_GUEST_NR_BASIC - 1,
+.feat.max_subleaf = CPUID_GUEST_NR_FEAT,
+},
+.leaf = 0x0007,
+},
+{
+.name = "Extended max leaf >= array size",
+.p = {
+.extd.max_leaf = 0x8000 + CPUID_GUEST_NR_EXTD,
+},
+.leaf = 0x8000,
+},
+
+{
+.name = "Basic leaf >= max leaf",
+.p = {
+.basic.max_leaf = CPUID_GUEST_NR_BASIC - 1,
+},
+.leaf = CPUID_GUEST_NR_BASIC,
+},
+{
+.name = "Feature leaf >= max leaf",
+.p = {
+.basic.max_leaf = CPUID_GUEST_NR_BASIC - 1,
+.feat.max_subleaf = CPUID_GUEST_NR_FEAT - 1,
+},
+.leaf = 0x0007,
+.subleaf = CPUID_GUEST_NR_FEAT,
+},
+{
+.name = "Extended leaf >= max leaf",
+.p = {
+.extd.max_leaf = 0x8000 + CPUID_GUEST_NR_EXTD - 1,
+},
+.leaf = 0x8000 + CPUID_GUEST_NR_EXTD,
+},
+};
+const struct cpuid_policy pc;
+const struct cpuid_leaf *lc;
+struct cpuid_policy p;
+struct cpuid_leaf *l;
+
+/* Constness build test. */
+lc = x86_cpuid_get_leaf(, 0, 0);
+l = x86_cpuid_get_leaf(, 0, 0);
+
+printf("Testing CPUID get leaf bound checking:\n");
+
+for ( size_t i = 0; i < ARRAY_SIZE(tests); ++i )
+{
+const struct test *t = [i];
+
+if ( x86_cpuid_get_leaf(>p, t->leaf, t->subleaf) )
+fail("  Test %s get leaf fail\n", t->name);
+}
+}
+
 static void test_is_compatible_success(void)
 {
 static struct test {
@@ -685,6 +759,7 @@ int main(int argc, char **argv)
 test_cpuid_serialise_success();
 test_cpuid_deserialise_failure();
 test_cpuid_out_of_range_clearing();
+test_cpuid_get_leaf_failure();
 
 test_msr_serialise_success();
 test_msr_deserialise_failure();
diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index 151944f657..4db2df3b52 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -764,48 +764,16 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf,
 switch ( leaf )
 {
 case 0 ... CPUID_GUEST_NR_BASIC - 1:
-ASSERT(p->basic.max_leaf < ARRAY_SIZE(p->basic.raw));
-if ( leaf > min_t(uint32_t, p->basic.max_leaf,
-  ARRAY_SIZE(p->basic.raw) - 1) )
-return;
-
-switch ( leaf )
-{
-case 0x4:
-if ( subleaf >= ARRAY_SIZE(p->cache.raw) )
-return;
-
-*res = array_access_nospec(p->cache.raw, subleaf);
-break;
-
-case 0x7:
-   

[PATCH v5 01/12] libs/guest: move cpu policy related prototypes to xenguest.h

[Roger Pau Monne]

Do this before adding any more stuff to xg_cpuid_x86.c.

The placement in xenctrl.h is wrong, as they are implemented by the
xenguest library. Note that xg_cpuid_x86.c needs to include
xg_private.h, and in turn also fix xg_private.h to include
xc_bitops.h. The bitops definition of BITS_PER_LONG needs to be
changed to not be an expression, so that xxhash.h can use it in a
preprocessor if directive.

As a result also modify xen-cpuid and the ocaml stubs to include
xenguest.h.

Reported-by: Andrew Cooper 
Signed-off-by: Roger Pau Monné 
Acked-by: Andrew Cooper 
---
Changes since v1:
 - Include xenguest.h in ocaml stubs.
 - Change BITS_PER_LONG definition in xc_bitops.h.
---
 tools/include/xenctrl.h | 55 
 tools/include/xenguest.h| 56 +
 tools/libs/ctrl/xc_bitops.h |  6 +++-
 tools/libs/guest/xg_cpuid_x86.c |  1 -
 tools/libs/guest/xg_private.h   |  1 +
 tools/misc/xen-cpuid.c  |  1 +
 6 files changed, 63 insertions(+), 57 deletions(-)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 07b96e6671..95bd5eca67 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -2528,61 +2528,6 @@ int xc_psr_get_domain_data(xc_interface *xch, uint32_t 
domid,
uint64_t *data);
 int xc_psr_get_hw_info(xc_interface *xch, uint32_t socket,
xc_psr_feat_type type, xc_psr_hw_info *hw_info);
-
-typedef struct xc_cpu_policy xc_cpu_policy_t;
-
-/* Create and free a xc_cpu_policy object. */
-xc_cpu_policy_t *xc_cpu_policy_init(void);
-void xc_cpu_policy_destroy(xc_cpu_policy_t *policy);
-
-/* Retrieve a system policy, or get/set a domains policy. */
-int xc_cpu_policy_get_system(xc_interface *xch, unsigned int policy_idx,
- xc_cpu_policy_t *policy);
-int xc_cpu_policy_get_domain(xc_interface *xch, uint32_t domid,
- xc_cpu_policy_t *policy);
-int xc_cpu_policy_set_domain(xc_interface *xch, uint32_t domid,
- xc_cpu_policy_t *policy);
-
-/* Manipulate a policy via architectural representations. */
-int xc_cpu_policy_serialise(xc_interface *xch, const xc_cpu_policy_t *policy,
-xen_cpuid_leaf_t *leaves, uint32_t *nr_leaves,
-xen_msr_entry_t *msrs, uint32_t *nr_msrs);
-int xc_cpu_policy_update_cpuid(xc_interface *xch, xc_cpu_policy_t *policy,
-   const xen_cpuid_leaf_t *leaves,
-   uint32_t nr);
-int xc_cpu_policy_update_msrs(xc_interface *xch, xc_cpu_policy_t *policy,
-  const xen_msr_entry_t *msrs, uint32_t nr);
-
-/* Compatibility calculations. */
-bool xc_cpu_policy_is_compatible(xc_interface *xch, xc_cpu_policy_t *host,
- xc_cpu_policy_t *guest);
-
-int xc_get_cpu_levelling_caps(xc_interface *xch, uint32_t *caps);
-int xc_get_cpu_featureset(xc_interface *xch, uint32_t index,
-  uint32_t *nr_features, uint32_t *featureset);
-
-int xc_cpu_policy_get_size(xc_interface *xch, uint32_t *nr_leaves,
-   uint32_t *nr_msrs);
-int xc_set_domain_cpu_policy(xc_interface *xch, uint32_t domid,
- uint32_t nr_leaves, xen_cpuid_leaf_t *leaves,
- uint32_t nr_msrs, xen_msr_entry_t *msrs,
- uint32_t *err_leaf_p, uint32_t *err_subleaf_p,
- uint32_t *err_msr_p);
-
-uint32_t xc_get_cpu_featureset_size(void);
-
-enum xc_static_cpu_featuremask {
-XC_FEATUREMASK_KNOWN,
-XC_FEATUREMASK_SPECIAL,
-XC_FEATUREMASK_PV_MAX,
-XC_FEATUREMASK_PV_DEF,
-XC_FEATUREMASK_HVM_SHADOW_MAX,
-XC_FEATUREMASK_HVM_SHADOW_DEF,
-XC_FEATUREMASK_HVM_HAP_MAX,
-XC_FEATUREMASK_HVM_HAP_DEF,
-};
-const uint32_t *xc_get_static_cpu_featuremask(enum xc_static_cpu_featuremask);
-
 #endif
 
 int xc_livepatch_upload(xc_interface *xch,
diff --git a/tools/include/xenguest.h b/tools/include/xenguest.h
index 61d0a82f48..e01f494b77 100644
--- a/tools/include/xenguest.h
+++ b/tools/include/xenguest.h
@@ -782,4 +782,60 @@ xen_pfn_t *xc_map_m2p(xc_interface *xch,
   unsigned long max_mfn,
   int prot,
   unsigned long *mfn0);
+
+#if defined(__i386__) || defined(__x86_64__)
+typedef struct xc_cpu_policy xc_cpu_policy_t;
+
+/* Create and free a xc_cpu_policy object. */
+xc_cpu_policy_t *xc_cpu_policy_init(void);
+void xc_cpu_policy_destroy(xc_cpu_policy_t *policy);
+
+/* Retrieve a system policy, or get/set a domains policy. */
+int xc_cpu_policy_get_system(xc_interface *xch, unsigned int policy_idx,
+ xc_cpu_policy_t *policy);
+int xc_cpu_policy_get_domain(xc_interface *xch, uint32_t domid,
+ xc_cpu_policy_t *policy);
+int xc_cpu_policy_set_domain(xc_interface *xch, uint32_t domid,

[PATCH v5 00/12] libs/guest: new CPUID/MSR interface

[Roger Pau Monne]

Hello,

The following series introduces a new CPUID/MSR interface for the
xenguest library. Such interface handles both CPUID and MSRs using the
same opaque object, and provides some helpers for the user to peek or
modify such data without exposing the backing type. This is useful for
future development as CPUID and MSRs are closely related, so it makes
handling those much easier if they are inside the same object (ie: a
change to a CPUID bit might expose or hide an MSR).

In this patch series libxl and other in tree users have been switched to
use the new interface, so it shouldn't result in any functional change
from a user point of view.

Note there are still some missing pieces likely. The way to modify CPUID
data is not ideal, as it requires fetching a leaf and modifying it
directly. We might want some kind of interface in order to set specific
CPUID features more easily, but that's to be discussed, and would be
done as a follow up series.

The addition of a helper to generate compatible policies given two
inputs has been removed from this iteration, sine Andrew Cooper has
posted a patch to set the foundation for that, and further work should
be done against that baseline.

Thanks, Roger.

Jan Beulich (1):
  x86/CPUID: shrink max_{,sub}leaf fields according to actual leaf
contents

Roger Pau Monne (11):
  libs/guest: move cpu policy related prototypes to xenguest.h
  libx86: introduce helper to fetch cpuid leaf
  libs/guest: allow fetching a specific CPUID leaf from a cpu policy
  libx86: introduce helper to fetch msr entry
  libs/guest: allow fetching a specific MSR entry from a cpu policy
  libs/guest: make a cpu policy compatible with older Xen versions
  libs/guest: introduce helper set cpu topology in cpu policy
  libs/guest: rework xc_cpuid_xend_policy
  libs/guest: apply a featureset into a cpu policy
  libs/{light,guest}: implement xc_cpuid_apply_policy in libxl
  libs/guest: (re)move xc_cpu_policy_apply_cpuid

 tools/include/libxl.h|   6 +-
 tools/include/xenctrl.h  |  99 
 tools/include/xenguest.h |  77 +++
 tools/libs/ctrl/xc_bitops.h  |   6 +-
 tools/libs/guest/xg_cpuid_x86.c  | 647 ---
 tools/libs/guest/xg_private.h|   1 +
 tools/libs/light/libxl_cpuid.c   | 233 +++-
 tools/libs/light/libxl_internal.h|  26 +
 tools/misc/xen-cpuid.c   |   1 +
 tools/tests/cpu-policy/test-cpu-policy.c | 224 +++-
 xen/arch/x86/cpuid.c |  55 +-
 xen/include/xen/lib/x86/cpuid.h  |  26 +
 xen/include/xen/lib/x86/msr.h|  20 +-
 xen/lib/x86/cpuid.c  |  91 
 xen/lib/x86/msr.c|  41 +-
 15 files changed, 948 insertions(+), 605 deletions(-)

-- 
2.33.0

Stable backports of Xen related patches

[Juergen Gross]

Hi Greg,

attached are git bundles for some patches you merged into the 5.10
stable kernel already this morning.

Naming should be obvious, the patches are on the branch "back" in
each bundle.


Juergen


back-4.4
Description: Binary data


back-4.9
Description: Binary data


back-4.14
Description: Binary data


back-4.19
Description: Binary data


back-5.4
Description: Binary data


OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature

Re: [PATCH 00/65] x86: Support for CET Indirect Branch Tracking

[Andrew Cooper]

On 29/11/2021 14:44, Jan Beulich wrote:
> On 26.11.2021 13:33, Andrew Cooper wrote:
>> CET Indirect Branch Tracking is a hardware feature designed to protect 
>> against
>> forward-edge control flow hijacking (Call/Jump oriented programming), and is 
>> a
>> companion feature to CET Shadow Stacks added in Xen 4.14.
>>
>> This series depends on lots of previously posted patches.  See
>> xenbits/xen-cet-ibt for the full branch with all dependencies.
>>
>> Patch 1 introduces some compile time infrastructure.
>>
>> Patches 2 thru 56 annotate all function pointer targets in the common and x86
>> hypervisor code.  Patches are split by API and in no particular order, and
>> largely mechanical.  As such, I'm limiting review mainly to The Rest.  While
>> doing this work does depend on an experimental GCC change (patch 56), the
>> result does actually work properly with GCC 9 onwards.
>>
>> Patches 57 thru 65 do the final enablement of CET-IBT.
>>
>> I have developed this on a TigerLake NUC.  Many thanks to Marek who has also
>> given the series a spin on a TigerLake laptop.
>>
>> Some CI runs, green across the board:
>>   https://gitlab.com/xen-project/people/andyhhp/xen/-/pipelines/416737379
>>   https://cirrus-ci.com/build/6547947216175104
>>
>> Various note accumulated through the work:
>>   * I have already posted patches fixing some of the most egregious (ab)uses 
>> of
>> function pointers.  There are plenty of other areas which could do with
>> cleanup.
>>   * With everything turned on, we get 1688 runtime endbr64's, and 233 init
>> time.  The number of runtime endbr64's is expected to reduce with
>> Juergen's hypercall series (see later), and in common deployment cases
>> where not everything is compiled in by default.
>>   * I have not checked for misaligned endbr64's, and I'm not sure there is
>> anything useful we could do upon discovering that there were any.
>> Naively, there is a 1 in 2^32 chance (endbr64 being 4 bytes long), but
>> this doesn't account for the structure of x86 code, which is most
>> certainly not a uniform random distribution of bytes.
>>   * I have followup work to reduce the number of runtime endbr64's using boot
>> time patching, which further improves the security benefit.
>>   * Patches 2 and 3 are a minimal subset of Juergen's hypercall series, with
>> patch 4 annotating every hypercall.  I failed to get the full series pass
>> in CI, so put this together as a stopgap.  It reduces the dependencies
>> involved, and patch 4 can be dropped if the hypercall series gets in
>> first.
>>   * The x86 MTRR code is a complete mess, and as far as I can tell, is mostly
>> pre-64bit support.  It could do with a prune.
>>   * We do many passes of the MADT counting objects.  This is a waste of time
>> and we should count them on a single pass.
>>   * The NUMA setup (SRAT parsing) appears to happen twice.  I'm pretty sure
>> this is one too many.
>>
>> Andrew Cooper (63):
>>   x86: Introduce support for CET-IBT
>>   x86/hypercall: Annotate fnptr targets
>>   xen: Annotate fnptr targets from custom_param()
>>   xen: Annotate fnptr targets from __initcall()
>>   xen: Annotate fnptr targets from notifier callbacks
>>   xen: Annotate fnptr targets from acpi_table_parse()
>>   xen: Annotate fnptr targets from continue_hypercall_on_cpu()
>>   xen: Annotate fnptr targets from init_timer()
>>   xen: Annotate fnptr targets from call_rcu()
>>   xen: Annotate fnptr targets from IPIs
>>   xen: Annotate fnptr targets from open_softirq()
>>   xsm/flask:  Annotate fnptr targets in the security server
>>   xsm: Annotate fnptr targets
>>   xen/sched: Annotate fnptr targets
>>   xen/evtchn: Annotate fnptr targets
>>   xen/hypfs: Annotate fnptr targets
>>   xen/tasklet: Annotate fnptr targets
>>   xen/keyhandler: Annotate fnptr targets
>>   xen/vpci: Annotate fnptr targets
>>   xen/decompress: Annotate fnptr targets
>>   xen/iommu: Annotate fnptr targets
>>   xen/video: Annotate fnptr targets
>>   xen/console: Annotate fnptr targets
>>   xen/misc: Annotate fnptr targets
>>   x86: Annotate fnptr targets from request_irq()
>>   x86/hvm: Annotate fnptr targets from hvm_funcs
>>   x86/hvm: Annotate fnptr targets from device emulation
>>   x86/emul: Annotate fnptr targets
>>   x86/ucode: Annotate fnptr targets
>>   x86/power: Annotate fnptr targets
>>   x86/apic: Annotate fnptr targets
>>   x86/nmi: Annotate fnptr targets
>>   x86/mtrr: Annotate fnptr targets
>>   x86/idle: Annotate fnptr targets
>>   x86/quirks: Annotate fnptr targets
>>   x86/hvmsave: Annotate fnptr targets
>>   x86/mce: Annotate fnptr targets
>>   x86/pmu: Annotate fnptr targets
>>   x86/cpu: Annotate fnptr targets
>>   x86/guest: Annotate fnptr targets
>>   x86/logdirty: Annotate fnptr targets
>>   x86/shadow: Annotate fnptr targets
>>   x86/hap: Annotate fnptr targets
>>   x86/p2m: Annotate fnptr targets
>>   x86/irq: Annotate fnptr targets
>>   x86/aepi: Annotate fnptr targets

Re: [PATCH 00/65] x86: Support for CET Indirect Branch Tracking

[Jan Beulich]

On 26.11.2021 13:33, Andrew Cooper wrote:
> CET Indirect Branch Tracking is a hardware feature designed to protect against
> forward-edge control flow hijacking (Call/Jump oriented programming), and is a
> companion feature to CET Shadow Stacks added in Xen 4.14.
> 
> This series depends on lots of previously posted patches.  See
> xenbits/xen-cet-ibt for the full branch with all dependencies.
> 
> Patch 1 introduces some compile time infrastructure.
> 
> Patches 2 thru 56 annotate all function pointer targets in the common and x86
> hypervisor code.  Patches are split by API and in no particular order, and
> largely mechanical.  As such, I'm limiting review mainly to The Rest.  While
> doing this work does depend on an experimental GCC change (patch 56), the
> result does actually work properly with GCC 9 onwards.
> 
> Patches 57 thru 65 do the final enablement of CET-IBT.
> 
> I have developed this on a TigerLake NUC.  Many thanks to Marek who has also
> given the series a spin on a TigerLake laptop.
> 
> Some CI runs, green across the board:
>   https://gitlab.com/xen-project/people/andyhhp/xen/-/pipelines/416737379
>   https://cirrus-ci.com/build/6547947216175104
> 
> Various note accumulated through the work:
>   * I have already posted patches fixing some of the most egregious (ab)uses 
> of
> function pointers.  There are plenty of other areas which could do with
> cleanup.
>   * With everything turned on, we get 1688 runtime endbr64's, and 233 init
> time.  The number of runtime endbr64's is expected to reduce with
> Juergen's hypercall series (see later), and in common deployment cases
> where not everything is compiled in by default.
>   * I have not checked for misaligned endbr64's, and I'm not sure there is
> anything useful we could do upon discovering that there were any.
> Naively, there is a 1 in 2^32 chance (endbr64 being 4 bytes long), but
> this doesn't account for the structure of x86 code, which is most
> certainly not a uniform random distribution of bytes.
>   * I have followup work to reduce the number of runtime endbr64's using boot
> time patching, which further improves the security benefit.
>   * Patches 2 and 3 are a minimal subset of Juergen's hypercall series, with
> patch 4 annotating every hypercall.  I failed to get the full series pass
> in CI, so put this together as a stopgap.  It reduces the dependencies
> involved, and patch 4 can be dropped if the hypercall series gets in
> first.
>   * The x86 MTRR code is a complete mess, and as far as I can tell, is mostly
> pre-64bit support.  It could do with a prune.
>   * We do many passes of the MADT counting objects.  This is a waste of time
> and we should count them on a single pass.
>   * The NUMA setup (SRAT parsing) appears to happen twice.  I'm pretty sure
> this is one too many.
> 
> Andrew Cooper (63):
>   x86: Introduce support for CET-IBT
>   x86/hypercall: Annotate fnptr targets
>   xen: Annotate fnptr targets from custom_param()
>   xen: Annotate fnptr targets from __initcall()
>   xen: Annotate fnptr targets from notifier callbacks
>   xen: Annotate fnptr targets from acpi_table_parse()
>   xen: Annotate fnptr targets from continue_hypercall_on_cpu()
>   xen: Annotate fnptr targets from init_timer()
>   xen: Annotate fnptr targets from call_rcu()
>   xen: Annotate fnptr targets from IPIs
>   xen: Annotate fnptr targets from open_softirq()
>   xsm/flask:  Annotate fnptr targets in the security server
>   xsm: Annotate fnptr targets
>   xen/sched: Annotate fnptr targets
>   xen/evtchn: Annotate fnptr targets
>   xen/hypfs: Annotate fnptr targets
>   xen/tasklet: Annotate fnptr targets
>   xen/keyhandler: Annotate fnptr targets
>   xen/vpci: Annotate fnptr targets
>   xen/decompress: Annotate fnptr targets
>   xen/iommu: Annotate fnptr targets
>   xen/video: Annotate fnptr targets
>   xen/console: Annotate fnptr targets
>   xen/misc: Annotate fnptr targets
>   x86: Annotate fnptr targets from request_irq()
>   x86/hvm: Annotate fnptr targets from hvm_funcs
>   x86/hvm: Annotate fnptr targets from device emulation
>   x86/emul: Annotate fnptr targets
>   x86/ucode: Annotate fnptr targets
>   x86/power: Annotate fnptr targets
>   x86/apic: Annotate fnptr targets
>   x86/nmi: Annotate fnptr targets
>   x86/mtrr: Annotate fnptr targets
>   x86/idle: Annotate fnptr targets
>   x86/quirks: Annotate fnptr targets
>   x86/hvmsave: Annotate fnptr targets
>   x86/mce: Annotate fnptr targets
>   x86/pmu: Annotate fnptr targets
>   x86/cpu: Annotate fnptr targets
>   x86/guest: Annotate fnptr targets
>   x86/logdirty: Annotate fnptr targets
>   x86/shadow: Annotate fnptr targets
>   x86/hap: Annotate fnptr targets
>   x86/p2m: Annotate fnptr targets
>   x86/irq: Annotate fnptr targets
>   x86/aepi: Annotate fnptr targets
>   x86/psr: Annotate fnptr targets
>   x86/dpci: Annotate fnptr targets
>   x86/pt: Annotate fnptr targets
>   x86/time: Annotate fnptr 

Re: [PATCH 54/65] x86/stack: Annotate fnptr targets

[Jan Beulich]

On 26.11.2021 13:34, Andrew Cooper wrote:
> --- a/xen/include/asm-x86/current.h
> +++ b/xen/include/asm-x86/current.h
> @@ -173,7 +173,6 @@ unsigned long get_stack_dump_bottom (unsigned long sp);
>  #define switch_stack_and_jump(fn, instr, constr)\
>  ({  \
>  unsigned int tmp;   \
> -(void)((fn) == (void (*)(void))NULL);   \
>  BUILD_BUG_ON(!ssaj_has_attr_noreturn(fn));  \
>  __asm__ __volatile__ (  \
>  SHADOW_STACK_WORK   \
> @@ -198,6 +197,7 @@ unsigned long get_stack_dump_bottom (unsigned long sp);
>  
>  /* The constraint may only specify non-call-clobbered registers. */
>  #define reset_stack_and_jump_ind(fn)\
> +(void)((fn) == (void (*)(void))NULL);   \
>  switch_stack_and_jump(fn, "INDIRECT_JMP %", "b")
>  

While the risk of use in a context really requiring it is low, I
still think we'd be better off wrapping the whole thing in ({ })
then.

Jan

Re: [PATCH 48/65] x86/aepi: Annotate fnptr targets

[Jan Beulich]

On 26.11.2021 13:34, Andrew Cooper wrote:
> Signed-off-by: Andrew Cooper 

Nit: In the title I think you mean "x86/apei:".

Jan

Re: [PATCH 30/65] x86/emul: Annotate fnptr targets

[Jan Beulich]

On 26.11.2021 13:34, Andrew Cooper wrote:
> pv_emul_is_mem_write() only has a single user.

Aiui you mean a single file using it; there I can spot three uses.

>  Having it as a static inline
> is pointless because it can't be inlined to begin with.

As long as it's only used to set function pointer fields, yes.

Jan

Re: [PATCH 28/65] x86/hvm: Annotate fnptr targets from hvm_funcs

[Jan Beulich]

On 26.11.2021 13:34, Andrew Cooper wrote:
> In svm.c, make a few rearrangements.  svm_update_guest_cr() has no external
> callers so can become static, but needs moving along with svm_fpu_enter() to
> avoid a forward declaration.  Also move svm_update_guest_efer() to drop its
> forward declaration.

Sadly this means svm_fpu_leave() is now far away from svm_fpu_enter(). Can't
the two be kept together?

Jan

[XEN PATCH 0/1] Add support for SMBIOS tables 7,8,9,26,27,28.

[Anton Belousov]

This update is done to improve virtual machine stealth from malware. There are 
AntiVM techniques that use WMI-queries to detect presence of this SMBIOS 
tables. Example: 
"https://github.com/LordNoteworthy/al-khaser/blob/master/al-khaser/AntiVM/Generic.cpp;

Anton Belousov (1):
  Add suport for SMBIOS tables 7,8,9,26,27,28 to improve virtual machine
stealth from malware.

 tools/firmware/hvmloader/smbios.c   | 146 
 tools/firmware/hvmloader/smbios_types.h |  76 
 2 files changed, 222 insertions(+)

-- 
2.25.1

[XEN PATCH 1/1] Add suport for SMBIOS tables 7,8,9,26,27,28 to improve virtual machine stealth from malware.

[Anton Belousov]

---
 tools/firmware/hvmloader/smbios.c   | 146 
 tools/firmware/hvmloader/smbios_types.h |  76 
 2 files changed, 222 insertions(+)

diff --git a/tools/firmware/hvmloader/smbios.c 
b/tools/firmware/hvmloader/smbios.c
index 97a054e9e3..f5e61c1159 100644
--- a/tools/firmware/hvmloader/smbios.c
+++ b/tools/firmware/hvmloader/smbios.c
@@ -33,12 +33,18 @@
 #define SMBIOS_HANDLE_TYPE2   0x0200
 #define SMBIOS_HANDLE_TYPE3   0x0300
 #define SMBIOS_HANDLE_TYPE4   0x0400
+#define SMBIOS_HANDLE_TYPE7   0x0700
+#define SMBIOS_HANDLE_TYPE8   0x0800
+#define SMBIOS_HANDLE_TYPE9   0x0900
 #define SMBIOS_HANDLE_TYPE11  0x0B00
 #define SMBIOS_HANDLE_TYPE16  0x1000
 #define SMBIOS_HANDLE_TYPE17  0x1100
 #define SMBIOS_HANDLE_TYPE19  0x1300
 #define SMBIOS_HANDLE_TYPE20  0x1400
 #define SMBIOS_HANDLE_TYPE22  0x1600
+#define SMBIOS_HANDLE_TYPE26  0x1A00
+#define SMBIOS_HANDLE_TYPE27  0x1B00
+#define SMBIOS_HANDLE_TYPE28  0x1C00
 #define SMBIOS_HANDLE_TYPE32  0x2000
 #define SMBIOS_HANDLE_TYPE39  0x2700
 #define SMBIOS_HANDLE_TYPE127 0x7f00
@@ -77,6 +83,12 @@ static void *
 smbios_type_4_init(void *start, unsigned int cpu_number,
char *cpu_manufacturer);
 static void *
+smbios_type_7_init(void *start);
+static void *
+smbios_type_8_init(void *start);
+static void *
+smbios_type_9_init(void *start);
+static void *
 smbios_type_11_init(void *start);
 static void *
 smbios_type_16_init(void *start, uint32_t memory_size_mb, int nr_mem_devs);
@@ -89,6 +101,12 @@ smbios_type_20_init(void *start, uint32_t memory_size_mb, 
int instance);
 static void *
 smbios_type_22_init(void *start);
 static void *
+smbios_type_26_init(void *start);
+static void *
+smbios_type_27_init(void *start);
+static void *
+smbios_type_28_init(void *start);
+static void *
 smbios_type_32_init(void *start);
 static void *
 smbios_type_39_init(void *start);
@@ -205,6 +223,9 @@ write_smbios_tables(void *ep, void *start,
 do_struct(smbios_type_3_init(p));
 for ( cpu_num = 1; cpu_num <= vcpus; cpu_num++ )
 do_struct(smbios_type_4_init(p, cpu_num, cpu_manufacturer));
+do_struct(smbios_type_7_init(p));
+do_struct(smbios_type_8_init(p));
+do_struct(smbios_type_9_init(p));
 do_struct(smbios_type_11_init(p));
 
 /* Each 'memory device' covers up to 16GB of address space. */
@@ -221,6 +242,9 @@ write_smbios_tables(void *ep, void *start,
 }
 
 do_struct(smbios_type_22_init(p));
+do_struct(smbios_type_26_init(p));
+do_struct(smbios_type_28_init(p));
+do_struct(smbios_type_27_init(p));
 do_struct(smbios_type_32_init(p));
 do_struct(smbios_type_39_init(p));
 do_struct(smbios_type_vendor_oem_init(p));
@@ -700,6 +724,66 @@ smbios_type_4_init(
 return start+1;
 }
 
+/* Type 7 -- Cache Information */
+static void *
+smbios_type_7_init(void *start)
+{
+struct smbios_type_7 *p = (struct smbios_type_7 *)start;
+
+void *pts;
+uint32_t length;
+
+pts = get_smbios_pt_struct(7, );
+if ( (pts != NULL)&&(length > 0) )
+{
+memcpy(start, pts, length);
+p->header.handle = SMBIOS_HANDLE_TYPE7;
+return (start + length);
+}
+
+return start;
+}
+
+/* Type 8 -- Port Connector Information */
+static void *
+smbios_type_8_init(void *start)
+{
+struct smbios_type_8 *p = (struct smbios_type_8 *)start;
+
+void *pts;
+uint32_t length;
+
+pts = get_smbios_pt_struct(8, );
+if ( (pts != NULL)&&(length > 0) )
+{
+memcpy(start, pts, length);
+p->header.handle = SMBIOS_HANDLE_TYPE8;
+return (start + length);
+}
+
+return start;
+}
+
+/* Type 9 -- System Slots */
+static void *
+smbios_type_9_init(void *start)
+{
+struct smbios_type_9 *p = (struct smbios_type_9 *)start;
+
+void *pts;
+uint32_t length;
+
+pts = get_smbios_pt_struct(9, );
+if ( (pts != NULL)&&(length > 0) )
+{
+memcpy(start, pts, length);
+p->header.handle = SMBIOS_HANDLE_TYPE9;
+return (start + length);
+}
+
+return start;
+}
+
 /* Type 11 -- OEM Strings */
 static void *
 smbios_type_11_init(void *start) 
@@ -923,6 +1007,68 @@ smbios_type_22_init(void *start)
 return start+1; 
 }
 
+/* Type 26 -- Voltage Probe */
+static void *
+smbios_type_26_init(void *start)
+{
+struct smbios_type_26 *p = (struct smbios_type_26 *)start;
+
+void *pts;
+uint32_t length;
+
+pts = get_smbios_pt_struct(26, );
+if ( (pts != NULL)&&(length > 0) )
+{
+memcpy(start, pts, length);
+p->header.handle = SMBIOS_HANDLE_TYPE26;
+return (start + length);
+}
+
+return start;
+}
+
+/* Type 27 -- Cooling Device */
+static void *
+smbios_type_27_init(void *start)
+{
+struct smbios_type_27 *p = (struct smbios_type_27 *)start;
+
+void *pts;
+uint32_t length;
+
+pts = get_smbios_pt_struct(27, );
+if ( (pts != NULL)&&(length > 0) )
+{
+memcpy(start, pts, length);
+p->header.handle = 

Re: [PATCH 19/65] xen/tasklet: Annotate fnptr targets

[Jan Beulich]

On 26.11.2021 13:34, Andrew Cooper wrote:
> The function pointer cast in hvm_vcpu_initialise() is undefined behaviour.
> 
> While it happens to function correctly before this point, it is not
> incompatible with control flow typechecking,

DYM "is now incompatible" or "is not compatible"?

> so introduce a new
> hvm_assert_evtchn_irq_tasklet() to handle the parameter type conversion in a
> legal way.
> 
> Signed-off-by: Andrew Cooper 
> ---
> CC: Jan Beulich 
> CC: Stefano Stabellini 
> CC: Wei Liu 
> CC: Julien Grall 
> CC: Roger Pau Monné 
> ---
>  xen/arch/x86/hvm/hvm.c| 7 ++-
>  xen/arch/x86/hvm/vlapic.c | 2 +-
>  xen/arch/x86/mm/shadow/common.c   | 2 +-
>  xen/common/domain.c   | 2 +-
>  xen/common/keyhandler.c   | 6 +++---
>  xen/common/livepatch.c| 2 +-
>  xen/common/stop_machine.c | 2 +-
>  xen/common/trace.c| 2 +-
>  xen/drivers/char/console.c| 2 +-
>  xen/drivers/passthrough/amd/iommu_guest.c | 2 +-
>  xen/drivers/passthrough/amd/iommu_init.c  | 4 ++--
>  xen/drivers/passthrough/arm/smmu-v3.c | 6 +++---

Wrt my remark in an earlier patch - any reason that here you do touch an
Arm-only file?

Jan

Re: [PATCH 17/65] xen/evtchn: Annotate fnptr targets

[Jan Beulich]

On 26.11.2021 13:33, Andrew Cooper wrote:
> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -99,7 +99,8 @@ static xen_event_channel_notification_t __read_mostly
>  xen_consumers[NR_XEN_CONSUMERS];
>  
>  /* Default notification action: wake up from wait_on_xen_event_channel(). */
> -static void default_xen_notification_fn(struct vcpu *v, unsigned int port)
> +static void cf_check default_xen_notification_fn(
> +struct vcpu *v, unsigned int port)
>  {
>  /* Consumer needs notification only if blocked. */
>  if ( test_and_clear_bit(_VPF_blocked_in_xen, >pause_flags) )

Alongside this shouldn't you also annotate mem_paging_notification(),
monitor_notification(), and mem_sharing_notification() right here
(following how previous patches were organized)?

I take that you're specifically leaving out anything in Arm code, no
matter whether such annotations may gain a meaning at some point (not
that I would know of any plans).

Jan

[qemu-mainline test] 166950: tolerable FAIL - PUSHED

[osstest service owner]

flight 166950 qemu-mainline real [real]
http://logs.test-lab.xenproject.org/osstest/logs/166950/

Failures :-/ but no regressions.

Tests which are failing intermittently (not blocking):
 test-amd64-amd64-xl-rtds   18 guest-localmigrate fail in 166948 pass in 166950
 test-arm64-arm64-libvirt-raw 12 debian-di-install  fail pass in 166948

Tests which did not succeed, but are not blocking:
 test-armhf-armhf-xl-rtds 18 guest-start/debian.repeat fail in 166948 blocked 
in 166370
 test-arm64-arm64-libvirt-raw 14 migrate-support-check fail in 166948 never pass
 test-arm64-arm64-libvirt-raw 15 saverestore-support-check fail in 166948 never 
pass
 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 166370
 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 166370
 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 166370
 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail  like 166370
 test-armhf-armhf-libvirt 16 saverestore-support-checkfail  like 166370
 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check   fail like 166370
 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 166370
 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 166370
 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-xsm  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  15 migrate-support-checkfail   never pass
 test-amd64-i386-xl-pvshim14 guest-start  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-xl  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  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-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-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-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail 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-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-multivcpu 15 migrate-support-checkfail  never pass
 test-armhf-armhf-xl-multivcpu 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-armhf-armhf-xl  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  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-cubietruck 15 migrate-support-checkfail never pass
 test-armhf-armhf-xl-cubietruck 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-armhf-armhf-libvirt-raw 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 15 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass

version targeted for testing:
 qemuudd4b0de45965538f19bb40c7ddaaba384a8c613a
baseline version:
 qemuu

Re: Patches for stable 5.10 kernel

[Greg Kroah-Hartman]

On Mon, Nov 29, 2021 at 08:19:18AM +0100, Juergen Gross wrote:
> Hi Greg,
> 
> could you please add the following upstream patches to the stable 5.10
> kernel (I'll send separate mails for the older stable kernels as some
> of the patches don't apply for those)? They are hardening Xen PV
> frontends against attacks from related backends.
> 
> Qubes-OS has asked for those patches to be added to stable, too.
> 
> 629a5d87e26fe96b ("xen: sync include/xen/interface/io/ring.h with Xen's
> newest version")
> 71b66243f9898d0e ("xen/blkfront: read response from backend only once")
> 8f5a695d99000fc3 ("xen/blkfront: don't take local copy of a request from the
> ring page")
> b94e4b147fd1992a ("xen/blkfront: don't trust the backend response data
> blindly")
> 8446066bf8c1f9f7 ("xen/netfront: read response from backend only once")
> 162081ec33c2686a ("xen/netfront: don't read data from request on the ring
> page")
> 21631d2d741a64a0 ("xen/netfront: disentangle tx_skb_freelist")
> a884daa61a7d9165 ("xen/netfront: don't trust the backend response data
> blindly")
> e679004dec37566f ("tty: hvc: replace BUG_ON() with negative return value")
> 

All now queued up, thanks.

But people should be moving to the 5.15 kernel by now and not sticking
with 5.10 anymore for stuff like this.

greg k-h

[ovmf test] 166951: all pass - PUSHED

[osstest service owner]

flight 166951 ovmf real [real]
http://logs.test-lab.xenproject.org/osstest/logs/166951/

Perfect :-)
All tests in this flight passed as required
version targeted for testing:
 ovmf 76a1ce4d5fec7cd6906e6ea4ed6a4276b700e7ae
baseline version:
 ovmf ef9a059cdb15844fe52a49af2bf7d86b9dd3e9bf

Last test of basis   166949  2021-11-29 06:11:36 Z0 days
Testing same since   166951  2021-11-29 08:42:05 Z0 days1 attempts


People who touched revisions under test:
  Michael D Kinney 
  Michael Kubacki 
  Sean Brogan 

jobs:
 build-amd64-xsm  pass
 build-i386-xsm   pass
 build-amd64  pass
 build-i386   pass
 build-amd64-libvirt  pass
 build-i386-libvirt   pass
 build-amd64-pvopspass
 build-i386-pvops pass
 test-amd64-amd64-xl-qemuu-ovmf-amd64 pass
 test-amd64-i386-xl-qemuu-ovmf-amd64  pass



sg-report-flight on osstest.test-lab.xenproject.org
logs: /home/logs/logs
images: /home/logs/images

Logs, config files, etc. are available at
http://logs.test-lab.xenproject.org/osstest/logs

Explanation of these reports, and of osstest in general, is at
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master

Test harness code can be found at
http://xenbits.xen.org/gitweb?p=osstest.git;a=summary


Pushing revision :

To xenbits.xen.org:/home/xen/git/osstest/ovmf.git
   ef9a059cdb..76a1ce4d5f  76a1ce4d5fec7cd6906e6ea4ed6a4276b700e7ae -> 
xen-tested-master

Re: [PATCH v2 28/45] mfd: rn5t618: Use devm_register_power_handler()

[Dmitry Osipenko]

29.11.2021 14:55, Lee Jones пишет:
> On Thu, 28 Oct 2021, Dmitry Osipenko wrote:
> 
>> Use devm_register_power_handler() that replaces global pm_power_off
>> variable and allows to register multiple power-off handlers. It also
>> provides restart-handler support, i.e. all in one API.
>>
>> Signed-off-by: Dmitry Osipenko 
>> ---
>>  drivers/mfd/rn5t618.c | 56 ---
>>  1 file changed, 21 insertions(+), 35 deletions(-)
> 
> For my own reference (apply this as-is to your sign-off block):
> 
>   Acked-for-MFD-by: Lee Jones 
> 

Thanks you. This and other driver patches will be slightly changed
because the power-handler was renamed to sys-off handler starting with
the v3 of this series, but yours ack still will be valid here.

Re: [PATCH v2 28/45] mfd: rn5t618: Use devm_register_power_handler()

[Lee Jones]

On Thu, 28 Oct 2021, Dmitry Osipenko wrote:

> Use devm_register_power_handler() that replaces global pm_power_off
> variable and allows to register multiple power-off handlers. It also
> provides restart-handler support, i.e. all in one API.
> 
> Signed-off-by: Dmitry Osipenko 
> ---
>  drivers/mfd/rn5t618.c | 56 ---
>  1 file changed, 21 insertions(+), 35 deletions(-)

For my own reference (apply this as-is to your sign-off block):

  Acked-for-MFD-by: Lee Jones 

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v4 08/25] kernel: Add combined power-off+restart handler call chain API

[Dmitry Osipenko]

29.11.2021 03:36, Michał Mirosław пишет:
> On Mon, Nov 29, 2021 at 12:53:51AM +0300, Dmitry Osipenko wrote:
>> 29.11.2021 00:17, Michał Mirosław пишет:
 I'm having trouble with parsing this comment. Could you please try to
 rephrase it? I don't see how you could check whether power-off handler
 is available if you'll mix all handlers together.
>>> If notify_call_chain() would be fixed to return NOTIFY_OK if any call
>>> returned NOTIFY_OK, then this would be a clear way to gather the
>>> answer if any of the handlers will attempt the final action (reboot or
>>> power off).
>> Could you please show a code snippet that implements your suggestion?
> 
> A rough idea is this:
> 
>  static int notifier_call_chain(struct notifier_block **nl,
>  unsigned long val, void *v,
>  int nr_to_call, int *nr_calls)
>  {
> - int ret = NOTIFY_DONE;
> + int ret, result = NOTIFY_DONE;
>   struct notifier_block *nb, *next_nb;
>  
>   nb = rcu_dereference_raw(*nl);
>  
>   while (nb && nr_to_call) {
> ...
>   ret = nb->notifier_call(nb, val, v);
> +
> + /* Assuming NOTIFY_STOP-carrying return is always greater than 
> non-stopping one. */
> + if (result < ret)
> + result = ret;
> ... 
>   }
> - return ret;
> + return result;
>  }
> 
> Then:
> 
> bool prepare_reboot()
> {
>   int ret = xx_notifier_call_chain(_notifier, PREPARE_REBOOT, 
> ...);
>   return ret == NOTIFY_OK;
> }
> 
> And the return value would signify whether the reboot will be attempted
> when calling the chain for the REBOOT action. (Analogously for powering off.)

If you started to execute call chain, then you began the power-off /
restart sequence, this is a point of no return. Sorry, I still don't
understand what you're trying to achieve.

The approach of having separate call chains is simple and intuitive, I
don't see reasons to change it.

Re: [PATCH v4 05/25] reboot: Warn if restart handler has duplicated priority

[Dmitry Osipenko]

29.11.2021 03:26, Michał Mirosław пишет:
> On Mon, Nov 29, 2021 at 12:06:19AM +0300, Dmitry Osipenko wrote:
>> 28.11.2021 03:28, Michał Mirosław пишет:
>>> On Fri, Nov 26, 2021 at 09:00:41PM +0300, Dmitry Osipenko wrote:
 Add sanity check which ensures that there are no two restart handlers
 registered with the same priority. Normally it's a direct sign of a
 problem if two handlers use the same priority.
>>>
>>> The patch doesn't ensure the property that there are no duplicated-priority
>>> entries on the chain.
>>
>> It's not the exact point of this patch.
>>
>>> I'd rather see a atomic_notifier_chain_register_unique() that returns
>>> -EBUSY or something istead of adding an entry with duplicate priority.
>>> That way it would need only one list traversal unless you want to
>>> register the duplicate anyway (then you would call the older
>>> atomic_notifier_chain_register() after reporting the error).
>>
>> The point of this patch is to warn developers about the problem that
>> needs to be fixed. We already have such troubling drivers in mainline.
>>
>> It's not critical to register different handlers with a duplicated
>> priorities, but such cases really need to be corrected. We shouldn't
>> break users' machines during transition to the new API, meanwhile
>> developers should take action of fixing theirs drivers.
>>
>>> (Or you could return > 0 when a duplicate is registered in
>>> atomic_notifier_chain_register() if the callers are prepared
>>> for that. I don't really like this way, though.)
>>
>> I had a similar thought at some point before and decided that I'm not in
>> favor of this approach. It's nicer to have a dedicated function that
>> verifies the uniqueness, IMO.
> 
> I don't like the part that it traverses the list second time to check
> the uniqueness. But actually you could avoid that if
> notifier_chain_register() would always add equal-priority entries in
> reverse order:
> 
>  static int notifier_chain_register(struct notifier_block **nl,
>   struct notifier_block *n)
>  {
>   while ((*nl) != NULL) {
>   if (unlikely((*nl) == n)) {
>   WARN(1, "double register detected");
>   return 0;
>   }
> - if (n->priority > (*nl)->priority)
> + if (n->priority >= (*nl)->priority)
>   break;
>   nl = &((*nl)->next);
>   }
>   n->next = *nl;
>   rcu_assign_pointer(*nl, n);
>   return 0;
>  }
> 
> Then the check for uniqueness after adding would be:
> 
>  WARN(nb->next && nb->priority == nb->next->priority);

We can't just change the registration order because invocation order of
the call chain depends on the registration order and some of current
users may rely on that order. I'm pretty sure that changing the order
will have unfortunate consequences.

Re: [PATCH 01/65] x86: Introduce support for CET-IBT

[Andrew Cooper]

On 29/11/2021 09:27, Jan Beulich wrote:
> On 26.11.2021 13:33, Andrew Cooper wrote:
>> --- a/xen/arch/x86/arch.mk
>> +++ b/xen/arch/x86/arch.mk
>> @@ -46,6 +46,12 @@ CFLAGS-$(CONFIG_INDIRECT_THUNK) += 
>> -mindirect-branch=thunk-extern
>>  CFLAGS-$(CONFIG_INDIRECT_THUNK) += -mindirect-branch-register
>>  CFLAGS-$(CONFIG_INDIRECT_THUNK) += -fno-jump-tables
>>  
>> +ifdef CONFIG_HAS_CC_CET_IBT
>> +CFLAGS += -fcf-protection=branch -mmanual-endbr
> Don't you mean to check XEN_IBT here rather than the underlying
> compiler capability?

I did, and elsewhere in the patch (already fixed up).  I added
CONFIG_XEN_IBT rather late in the dev cycle, and missed a few conversions.

~Andrew

Re: [patch 09/22] MIPS: Octeon: Use arch_setup_msi_irq()

[Thomas Bogendoerfer]

On Sat, Nov 27, 2021 at 02:18:48AM +0100, Thomas Gleixner wrote:
> The core code provides the same loop code except for the MSI-X reject. Move
> that to arch_setup_msi_irq() and remove the duplicated code.
> 
> No functional change.
> 
> Signed-off-by: Thomas Gleixner 
> Cc: Thomas Bogendoerfer 
> Cc: linux-m...@vger.kernel.org
> ---
>  arch/mips/pci/msi-octeon.c |   32 +++-
>  1 file changed, 3 insertions(+), 29 deletions(-)
> 
> --- a/arch/mips/pci/msi-octeon.c
> +++ b/arch/mips/pci/msi-octeon.c
> @@ -68,6 +68,9 @@ int arch_setup_msi_irq(struct pci_dev *d
>   u64 search_mask;
>   int index;
>  
> + if (desc->pci.msi_attrib.is_msix)
> + return -EINVAL;
> +
>   /*
>* Read the MSI config to figure out how many IRQs this device
>* wants.  Most devices only want 1, which will give
> @@ -182,35 +185,6 @@ int arch_setup_msi_irq(struct pci_dev *d
>   return 0;
>  }
>  
> -int arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
> -{
> - struct msi_desc *entry;
> - int ret;
> -
> - /*
> -  * MSI-X is not supported.
> -  */
> - if (type == PCI_CAP_ID_MSIX)
> - return -EINVAL;
> -
> - /*
> -  * If an architecture wants to support multiple MSI, it needs to
> -  * override arch_setup_msi_irqs()
> -  */
> - if (type == PCI_CAP_ID_MSI && nvec > 1)
> - return 1;
> -
> - for_each_pci_msi_entry(entry, dev) {
> - ret = arch_setup_msi_irq(dev, entry);
> - if (ret < 0)
> - return ret;
> - if (ret > 0)
> - return -ENOSPC;
> - }
> -
> - return 0;
> -}
> -
>  /**
>   * Called when a device no longer needs its MSI interrupts. All
>   * MSI interrupts for the device are freed.

Acked-by: Thomas Bogendoerfer 

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.[ RFC1925, 2.3 ]

Re: [PATCH 04/65] x86/hypercall: Annotate fnptr targets

[Jan Beulich]

On 29.11.2021 10:38, Jan Beulich wrote:
> On 26.11.2021 15:28, Andrew Cooper wrote:
>> On 26/11/2021 14:21, Jan Beulich wrote:
>>> On 26.11.2021 13:33, Andrew Cooper wrote:
 --- a/xen/include/xen/hypercall.h
 +++ b/xen/include/xen/hypercall.h
 @@ -18,12 +18,12 @@
  #include 
  #include 
  
 -extern long
 +extern long cf_check
  do_sched_op(
  int cmd,
  XEN_GUEST_HANDLE_PARAM(void) arg);
>>> What purpose does the attribute serve on a declaration? On the surface
>>> I would consider it meaningful only on definitions, like e.g. __init.
>>
>> Because GCC treats cf_check (and nocf_check) as part of the function
>> type.  Simply getting it wrong will yield a "definition doesn't match
>> prototype" error.
>>
>> Furthermore, it needs to be visible across translation units so one TU
>> can spot (and complain at) creating a function pointer to a non-local
>> non-endbr'd function.
> 
> Hmm, it might well be that way, but that's not what the doc (for 11.2)
> says. While suggesting things are along the lines of what you say for
> "nocf_check", for "cf_check" it really only talks about code generation:
> "The cf_check attribute on a function is used to inform the compiler
> that ENDBR instruction should be placed at the function entry when
> ‘-fcf-protection=branch’ is enabled." And even for "nocf_check" it talks
> about extra compile time checks only when the attribute is applied to a
> function pointer variable/field, not when applied to a function.

The two attributes looks to indeed be as different as the doc suggests,
after some experimenting. But I now understand that's what patch 56 is
about.

Jan

Re: [patch 00/22] genirq/msi, PCI/MSI: Spring cleaning - Part 1

[Cédric Le Goater]

On 11/27/21 02:18, Thomas Gleixner wrote:

The [PCI] MSI code has gained quite some warts over time. A recent
discussion unearthed a shortcoming: the lack of support for expanding
PCI/MSI-X vectors after initialization of MSI-X.

PCI/MSI-X has no requirement to setup all vectors when MSI-X is enabled in
the device. The non-used vectors have just to be masked in the vector
table. For PCI/MSI this is not possible because the number of vectors
cannot be changed after initialization.

The PCI/MSI code, but also the core MSI irq domain code are built around
the assumption that all required vectors are installed at initialization
time and freed when the device is shut down by the driver.

Supporting dynamic expansion at least for MSI-X is important for VFIO so
that the host side interrupts for passthrough devices can be installed on
demand.

This is the first part of a large (total 101 patches) series which
refactors the [PCI]MSI infrastructure to make runtime expansion of MSI-X
vectors possible. The last part (10 patches) provide this functionality.

The first part is mostly a cleanup which consolidates code, moves the PCI
MSI code into a separate directory and splits it up into several parts.

No functional change intended except for patch 2/N which changes the
behaviour of pci_get_vector()/affinity() to get rid of the assumption that
the provided index is the "index" into the descriptor list instead of using
it as the actual MSI[X] index as seen by the hardware. This would break
users of sparse allocated MSI-X entries, but non of them use these
functions.

This series is based on 5.16-rc2 and also available via git:

  git://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git msi-v1-part-1

For the curious who can't wait for the next part to arrive the full series
is available via:

  git://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git msi-v1-part-4


After fixing the compile failures, I didn't see any regressions on
these platforms :

  PowerNV, pSeries under KVM and PowerVM, using POWER8/9 processors.

Thanks,

C.


Thanks,

tglx
---
  arch/powerpc/platforms/4xx/msi.c|  281 
  b/Documentation/driver-api/pci/pci.rst  |2
  b/arch/mips/pci/msi-octeon.c|   32 -
  b/arch/powerpc/platforms/4xx/Makefile   |1
  b/arch/powerpc/platforms/cell/axon_msi.c|2
  b/arch/powerpc/platforms/powernv/pci-ioda.c |4
  b/arch/powerpc/platforms/pseries/msi.c  |6
  b/arch/powerpc/sysdev/Kconfig   |6
  b/arch/s390/pci/pci_irq.c   |4
  b/arch/sparc/kernel/pci_msi.c   |4
  b/arch/x86/hyperv/irqdomain.c   |   55 --
  b/arch/x86/include/asm/x86_init.h   |6
  b/arch/x86/include/asm/xen/hypervisor.h |8
  b/arch/x86/kernel/apic/msi.c|8
  b/arch/x86/kernel/x86_init.c|   12
  b/arch/x86/pci/xen.c|   19
  b/drivers/irqchip/irq-gic-v2m.c |1
  b/drivers/irqchip/irq-gic-v3-its-pci-msi.c  |1
  b/drivers/irqchip/irq-gic-v3-mbi.c  |1
  b/drivers/net/wireless/ath/ath11k/pci.c |2
  b/drivers/pci/Makefile  |3
  b/drivers/pci/msi/Makefile  |7
  b/drivers/pci/msi/irqdomain.c   |  267 +++
  b/drivers/pci/msi/legacy.c  |   79 +++
  b/drivers/pci/msi/msi.c |  645 

  b/drivers/pci/msi/msi.h |   39 +
  b/drivers/pci/msi/pcidev_msi.c  |   43 +
  b/drivers/pci/pci-sysfs.c   |7
  b/drivers/pci/xen-pcifront.c|2
  b/include/linux/msi.h   |  135 ++---
  b/include/linux/pci.h   |1
  b/kernel/irq/msi.c  |   41 +
  32 files changed, 696 insertions(+), 1028 deletions(-)

Re: [PATCH 04/65] x86/hypercall: Annotate fnptr targets

[Jan Beulich]

On 26.11.2021 15:28, Andrew Cooper wrote:
> On 26/11/2021 14:21, Jan Beulich wrote:
>> On 26.11.2021 13:33, Andrew Cooper wrote:
>>> Signed-off-by: Andrew Cooper 
>> I understand there's not much to say here, but the title saying just
>> "annotate" without any context as to the purpose of the annotation
>> is too little information imo. I guess this then goes for many more
>> titles in this series.
> 
> I really couldn't think of anything useful to say.  Lots of these
> patches are entirely mechanical.

Them being mechanical is imo unrelated to having a subject which is
halfway meaningful even if one looks at a shortlog in a couple of
years time. All it would take to disambiguate the titles would look
to be "...: Annotate fnptr targets for CET-IBT". Arguably this arch-
specific feature may be a little odd to encounter in common code
logs, but that's still better than being entirely unspecific about
the purpose of the annotations.

>>> --- a/xen/include/xen/hypercall.h
>>> +++ b/xen/include/xen/hypercall.h
>>> @@ -18,12 +18,12 @@
>>>  #include 
>>>  #include 
>>>  
>>> -extern long
>>> +extern long cf_check
>>>  do_sched_op(
>>>  int cmd,
>>>  XEN_GUEST_HANDLE_PARAM(void) arg);
>> What purpose does the attribute serve on a declaration? On the surface
>> I would consider it meaningful only on definitions, like e.g. __init.
> 
> Because GCC treats cf_check (and nocf_check) as part of the function
> type.  Simply getting it wrong will yield a "definition doesn't match
> prototype" error.
> 
> Furthermore, it needs to be visible across translation units so one TU
> can spot (and complain at) creating a function pointer to a non-local
> non-endbr'd function.

Hmm, it might well be that way, but that's not what the doc (for 11.2)
says. While suggesting things are along the lines of what you say for
"nocf_check", for "cf_check" it really only talks about code generation:
"The cf_check attribute on a function is used to inform the compiler
that ENDBR instruction should be placed at the function entry when
‘-fcf-protection=branch’ is enabled." And even for "nocf_check" it talks
about extra compile time checks only when the attribute is applied to a
function pointer variable/field, not when applied to a function.

Jan

Re: [patch 10/22] genirq/msi, treewide: Use a named struct for PCI/MSI attributes

[Kalle Valo]

Thomas Gleixner  writes:

> The unnamed struct sucks and is in the way of further cleanups. Stick the
> PCI related MSI data into a real data structure and cleanup all users.
>
> No functional change.
>
> Signed-off-by: Thomas Gleixner 
> Cc: Greg Kroah-Hartman 
> Cc: sparcli...@vger.kernel.org
> Cc: x...@kernel.org
> Cc: xen-devel@lists.xenproject.org
> Cc: ath...@lists.infradead.org
> ---
>  arch/powerpc/platforms/cell/axon_msi.c|2 
>  arch/powerpc/platforms/powernv/pci-ioda.c |4 -
>  arch/powerpc/platforms/pseries/msi.c  |6 -
>  arch/sparc/kernel/pci_msi.c   |4 -
>  arch/x86/kernel/apic/msi.c|2 
>  arch/x86/pci/xen.c|6 -
>  drivers/net/wireless/ath/ath11k/pci.c |2 

For ath11k:

Acked-by: Kalle Valo 

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH 01/65] x86: Introduce support for CET-IBT

[Jan Beulich]

On 26.11.2021 13:33, Andrew Cooper wrote:
> --- a/xen/arch/x86/arch.mk
> +++ b/xen/arch/x86/arch.mk
> @@ -46,6 +46,12 @@ CFLAGS-$(CONFIG_INDIRECT_THUNK) += 
> -mindirect-branch=thunk-extern
>  CFLAGS-$(CONFIG_INDIRECT_THUNK) += -mindirect-branch-register
>  CFLAGS-$(CONFIG_INDIRECT_THUNK) += -fno-jump-tables
>  
> +ifdef CONFIG_HAS_CC_CET_IBT
> +CFLAGS += -fcf-protection=branch -mmanual-endbr

Don't you mean to check XEN_IBT here rather than the underlying
compiler capability?

Jan

Re: [PATCH 01/65] x86: Introduce support for CET-IBT

[Jan Beulich]

On 26.11.2021 16:21, Andrew Cooper wrote:
> On 26/11/2021 14:10, Jan Beulich wrote:
>> On 26.11.2021 13:33, Andrew Cooper wrote:
>>> @@ -124,6 +129,18 @@ config XEN_SHSTK
>>>   When CET-SS is active, 32bit PV guests cannot be used.  Backwards
>>>   compatiblity can be provided via the PV Shim mechanism.
>>>  
>>> +config XEN_IBT
>>> +   bool "Supervisor Indirect Branch Tracking"
>>> +   depends on HAS_CC_CET_IBT
>>> +   default y
>>> +   help
>>> + Control-flow Enforcement Technology (CET) is a set of features in
>>> + hardware designed to combat Return-oriented Programming (ROP, also
>>> + call/jump COP/JOP) attacks.  Indirect Branch Tracking is one CET
>>> + feature designed to provide function pointer protection.
>>> +
>>> + This option arranges for Xen to use CET-IBT for its own protection.
>> Shouldn't this depend on BROKEN until it's actually functional?
> 
> It compiles fine right from now, and making it BROKEN would inhibit
> bisection through the series.
> 
> Nothing actually matters until patch 65 turns on MSR_S_CET.ENDBR_EN.

"Nothing" except that until then the promised extra security isn't
there.

>>> --- a/xen/arch/x86/x86_emulate/x86_emulate.h
>>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
>>> @@ -35,6 +35,11 @@
>>>  # error Unknown compilation width
>>>  #endif
>>>  
>>> +#ifndef cf_check
>>> +/* Cope with userspace build not knowing about CET-IBT */
>>> +#define cf_check
>>> +#endif
>> Imo this shouldn't go here, but in tools/tests/x86_emulator/x86-emulate.h,
>> and then presumably without #ifdef.
> 
> I considered that, but the test harness isn't the only userspace
> harness.  There is the fuzzing harness too, and I'm not sure we want to
> force every userspace harness to provide the same workaround.

But that's the idea of putting it where I suggested: This header gets
re-used by the fuzzing harness:

x86-emulate.c x86-emulate.h wrappers.c: %:
[ -L $* ] || ln -sf $(XEN_ROOT)/tools/tests/x86_emulator/$*

Jan

Re: [PATCH 00/65] x86: Support for CET Indirect Branch Tracking

[Jan Beulich]

On 27.11.2021 00:49, Andrew Cooper wrote:
> Given that Marek has kindly hacked us up a check which should find any
> arbitrary violations, and on a small sample of builds, there are no
> violations, I suggest that we clean it up and put it as a check in the
> real build and enable it by default seeing as we're right at the start
> of the 4.17 dev window.
> 
> If it is seen to trip (and it might well not), we can judge at that
> point whether to rearrange the code to avoid it, or drop the check. 
> Until then however, it gives us a very strong security statement.

Sounds like a plan.

Jan

[PATCH 3/3] x86/vPMU: move vpmu_ops to .init.data

[Jan Beulich]

Both vendors' code populates all hooks, so there's no need to have any
NULL checks before invoking the hook functions. With that the only
remaining uses of the object are in alternative_{,v}call(), i.e. none
after alternatives patching.

In vpmu_arch_initialise() the check gets replaced by an opt_vpmu_enabled
one, as I couldn't convince myself that the pre-existing checks would be
sufficient to cover all possible cases.

Signed-off-by: Jan Beulich 

--- a/xen/arch/x86/cpu/vpmu.c
+++ b/xen/arch/x86/cpu/vpmu.c
@@ -49,7 +49,7 @@ CHECK_pmu_params;
 static unsigned int __read_mostly opt_vpmu_enabled;
 unsigned int __read_mostly vpmu_mode = XENPMU_MODE_OFF;
 unsigned int __read_mostly vpmu_features = 0;
-static struct arch_vpmu_ops __read_mostly vpmu_ops;
+static struct arch_vpmu_ops __initdata vpmu_ops;
 
 static DEFINE_SPINLOCK(vpmu_lock);
 static unsigned vpmu_count;
@@ -136,12 +136,10 @@ int vpmu_do_msr(unsigned int msr, uint64
 if ( !vpmu_is_set(vpmu, VPMU_INITIALIZED) )
 goto nop;
 
-if ( is_write && vpmu_ops.do_wrmsr )
+if ( is_write )
 ret = alternative_call(vpmu_ops.do_wrmsr, msr, *msr_content, 
supported);
-else if ( !is_write && vpmu_ops.do_rdmsr )
-ret = alternative_call(vpmu_ops.do_rdmsr, msr, msr_content);
 else
-goto nop;
+ret = alternative_call(vpmu_ops.do_rdmsr, msr, msr_content);
 
 /*
  * We may have received a PMU interrupt while handling MSR access
@@ -375,7 +373,7 @@ void vpmu_save(struct vcpu *v)
 int vpmu_load(struct vcpu *v, bool_t from_guest)
 {
 struct vpmu_struct *vpmu = vcpu_vpmu(v);
-int pcpu = smp_processor_id();
+int pcpu = smp_processor_id(), ret;
 struct vcpu *prev = NULL;
 
 if ( !vpmu_is_set(vpmu, VPMU_CONTEXT_ALLOCATED) )
@@ -423,21 +421,13 @@ int vpmu_load(struct vcpu *v, bool_t fro
  vpmu_is_set(vpmu, VPMU_CACHED)) )
 return 0;
 
-if ( vpmu_ops.arch_vpmu_load )
-{
-int ret;
-
-apic_write(APIC_LVTPC, vpmu->hw_lapic_lvtpc);
-/* Arch code needs to set VPMU_CONTEXT_LOADED */
-ret = alternative_call(vpmu_ops.arch_vpmu_load, v, from_guest);
-if ( ret )
-{
-apic_write(APIC_LVTPC, vpmu->hw_lapic_lvtpc | APIC_LVT_MASKED);
-return ret;
-}
-}
+apic_write(APIC_LVTPC, vpmu->hw_lapic_lvtpc);
+/* Arch code needs to set VPMU_CONTEXT_LOADED */
+ret = alternative_call(vpmu_ops.arch_vpmu_load, v, from_guest);
+if ( ret )
+apic_write(APIC_LVTPC, vpmu->hw_lapic_lvtpc | APIC_LVT_MASKED);
 
-return 0;
+return ret;
 }
 
 static int vpmu_arch_initialise(struct vcpu *v)
@@ -458,7 +448,7 @@ static int vpmu_arch_initialise(struct v
 if ( !vpmu_available(v) || vpmu_mode == XENPMU_MODE_OFF )
 return 0;
 
-if ( !vpmu_ops.initialise )
+if ( !opt_vpmu_enabled )
 {
 if ( vpmu_mode != XENPMU_MODE_OFF )
 {
@@ -564,18 +554,15 @@ static void vpmu_arch_destroy(struct vcp
 on_selected_cpus(cpumask_of(vpmu->last_pcpu),
  vpmu_clear_last, v, 1);
 
-if ( vpmu_ops.arch_vpmu_destroy )
-{
-/*
- * Unload VPMU first if VPMU_CONTEXT_LOADED being set.
- * This will stop counters.
- */
-if ( vpmu_is_set(vpmu, VPMU_CONTEXT_LOADED) )
-on_selected_cpus(cpumask_of(vcpu_vpmu(v)->last_pcpu),
- vpmu_save_force, v, 1);
+/*
+ * Unload VPMU first if VPMU_CONTEXT_LOADED being set.
+ * This will stop counters.
+ */
+if ( vpmu_is_set(vpmu, VPMU_CONTEXT_LOADED) )
+on_selected_cpus(cpumask_of(vcpu_vpmu(v)->last_pcpu),
+ vpmu_save_force, v, 1);
 
- alternative_vcall(vpmu_ops.arch_vpmu_destroy, v);
-}
+alternative_vcall(vpmu_ops.arch_vpmu_destroy, v);
 
 vpmu_reset(vpmu, VPMU_CONTEXT_ALLOCATED);
 }
@@ -681,8 +668,7 @@ static void pvpmu_finish(struct domain *
 /* Dump some vpmu information to console. Used in keyhandler dump_domains(). */
 void vpmu_dump(struct vcpu *v)
 {
-if ( vpmu_is_set(vcpu_vpmu(v), VPMU_INITIALIZED) &&
- vpmu_ops.arch_vpmu_dump )
+if ( vpmu_is_set(vcpu_vpmu(v), VPMU_INITIALIZED) )
 alternative_vcall(vpmu_ops.arch_vpmu_dump, v);
 }
 

[PATCH 2/3] x86/vPMU: invoke _vpmu_initialise() through a hook as well

[Jan Beulich]

I see little point in having an open-coded switch() statement to achieve
the same; like other vendor-specific operations the function can be
supplied in the respective ops structure instances.

Signed-off-by: Jan Beulich 

--- a/xen/arch/x86/cpu/vpmu.c
+++ b/xen/arch/x86/cpu/vpmu.c
@@ -455,21 +455,11 @@ static int vpmu_arch_initialise(struct v
 
 ASSERT(!(vpmu->flags & ~VPMU_AVAILABLE) && !vpmu->context);
 
-if ( !vpmu_available(v) )
+if ( !vpmu_available(v) || vpmu_mode == XENPMU_MODE_OFF )
 return 0;
 
-switch ( vendor )
+if ( !vpmu_ops.initialise )
 {
-case X86_VENDOR_AMD:
-case X86_VENDOR_HYGON:
-ret = svm_vpmu_initialise(v);
-break;
-
-case X86_VENDOR_INTEL:
-ret = vmx_vpmu_initialise(v);
-break;
-
-default:
 if ( vpmu_mode != XENPMU_MODE_OFF )
 {
 printk(XENLOG_G_WARNING "VPMU: Unknown CPU vendor %d. "
@@ -480,12 +470,17 @@ static int vpmu_arch_initialise(struct v
 return -EINVAL;
 }
 
-vpmu->hw_lapic_lvtpc = PMU_APIC_VECTOR | APIC_LVT_MASKED;
-
+ret = alternative_call(vpmu_ops.initialise, v);
 if ( ret )
+{
 printk(XENLOG_G_WARNING "VPMU: Initialization failed for %pv\n", v);
+return ret;
+}
+
+vpmu->hw_lapic_lvtpc = PMU_APIC_VECTOR | APIC_LVT_MASKED;
+vpmu_set(vpmu, VPMU_INITIALIZED);
 
-return ret;
+return 0;
 }
 
 static void get_vpmu(struct vcpu *v)
--- a/xen/arch/x86/cpu/vpmu_amd.c
+++ b/xen/arch/x86/cpu/vpmu_amd.c
@@ -483,24 +483,11 @@ static void amd_vpmu_dump(const struct v
 }
 }
 
-static const struct arch_vpmu_ops __initconstrel amd_vpmu_ops = {
-.do_wrmsr = amd_vpmu_do_wrmsr,
-.do_rdmsr = amd_vpmu_do_rdmsr,
-.do_interrupt = amd_vpmu_do_interrupt,
-.arch_vpmu_destroy = amd_vpmu_destroy,
-.arch_vpmu_save = amd_vpmu_save,
-.arch_vpmu_load = amd_vpmu_load,
-.arch_vpmu_dump = amd_vpmu_dump
-};
-
-int svm_vpmu_initialise(struct vcpu *v)
+static int svm_vpmu_initialise(struct vcpu *v)
 {
 struct xen_pmu_amd_ctxt *ctxt;
 struct vpmu_struct *vpmu = vcpu_vpmu(v);
 
-if ( vpmu_mode == XENPMU_MODE_OFF )
-return 0;
-
 if ( !counters )
 return -EINVAL;
 
@@ -529,11 +516,22 @@ int svm_vpmu_initialise(struct vcpu *v)
offsetof(struct xen_pmu_amd_ctxt, regs));
 }
 
-vpmu_set(vpmu, VPMU_INITIALIZED | VPMU_CONTEXT_ALLOCATED);
+vpmu_set(vpmu, VPMU_CONTEXT_ALLOCATED);
 
 return 0;
 }
 
+static const struct arch_vpmu_ops __initconstrel amd_vpmu_ops = {
+.initialise = svm_vpmu_initialise,
+.do_wrmsr = amd_vpmu_do_wrmsr,
+.do_rdmsr = amd_vpmu_do_rdmsr,
+.do_interrupt = amd_vpmu_do_interrupt,
+.arch_vpmu_destroy = amd_vpmu_destroy,
+.arch_vpmu_save = amd_vpmu_save,
+.arch_vpmu_load = amd_vpmu_load,
+.arch_vpmu_dump = amd_vpmu_dump
+};
+
 static const struct arch_vpmu_ops *__init common_init(void)
 {
 unsigned int i;
--- a/xen/arch/x86/cpu/vpmu_intel.c
+++ b/xen/arch/x86/cpu/vpmu_intel.c
@@ -819,25 +819,12 @@ static void core2_vpmu_destroy(struct vc
 vpmu_clear(vpmu);
 }
 
-static const struct arch_vpmu_ops __initconstrel core2_vpmu_ops = {
-.do_wrmsr = core2_vpmu_do_wrmsr,
-.do_rdmsr = core2_vpmu_do_rdmsr,
-.do_interrupt = core2_vpmu_do_interrupt,
-.arch_vpmu_destroy = core2_vpmu_destroy,
-.arch_vpmu_save = core2_vpmu_save,
-.arch_vpmu_load = core2_vpmu_load,
-.arch_vpmu_dump = core2_vpmu_dump
-};
-
-int vmx_vpmu_initialise(struct vcpu *v)
+static int vmx_vpmu_initialise(struct vcpu *v)
 {
 struct vpmu_struct *vpmu = vcpu_vpmu(v);
 u64 msr_content;
 static bool_t ds_warned;
 
-if ( vpmu_mode == XENPMU_MODE_OFF )
-return 0;
-
 if ( v->domain->arch.cpuid->basic.pmu_version <= 1 ||
  v->domain->arch.cpuid->basic.pmu_version >= 6 )
 return -EINVAL;
@@ -893,11 +880,20 @@ int vmx_vpmu_initialise(struct vcpu *v)
 if ( is_pv_vcpu(v) && !core2_vpmu_alloc_resource(v) )
 return -EIO;
 
-vpmu_set(vpmu, VPMU_INITIALIZED);
-
 return 0;
 }
 
+static const struct arch_vpmu_ops __initconstrel core2_vpmu_ops = {
+.initialise = vmx_vpmu_initialise,
+.do_wrmsr = core2_vpmu_do_wrmsr,
+.do_rdmsr = core2_vpmu_do_rdmsr,
+.do_interrupt = core2_vpmu_do_interrupt,
+.arch_vpmu_destroy = core2_vpmu_destroy,
+.arch_vpmu_save = core2_vpmu_save,
+.arch_vpmu_load = core2_vpmu_load,
+.arch_vpmu_dump = core2_vpmu_dump
+};
+
 const struct arch_vpmu_ops *__init core2_vpmu_init(void)
 {
 unsigned int version = 0;
--- a/xen/include/asm-x86/vpmu.h
+++ b/xen/include/asm-x86/vpmu.h
@@ -39,6 +39,7 @@
 
 /* Arch specific operations shared by all vpmus */
 struct arch_vpmu_ops {
+int (*initialise)(struct vcpu *v);
 int (*do_wrmsr)(unsigned int msr, uint64_t msr_content,
 uint64_t supported);
 int (*do_rdmsr)(unsigned int msr, uint64_t *msr_content);
@@ -50,10 +51,8 @@ struct arch_vpmu_ops {
 };
 
 

[PATCH 1/3] x86/vPMU: convert vendor hook invocations to altcall

[Jan Beulich]

At least some vPMU functions will be invoked (and hence can further be
speculated into) even in the vPMU-disabled case. Convert vpmu_ops to
the standard single-instance model being a prerequisite to engaging the
alternative_call() machinery, and convert all respective calls. Note
that this requires vpmu_init() to become a pre-SMP initcall.

This change then also helps performance.

To replace a few vpmu->arch_vpmu_ops NULL checks, introduce a new
VPMU_INITIALIZED state, such that in the absence of any other suitable
vmpu_is_set() checks this state can be checked for.

While adding the inclusion of xen/err.h, also prune other xen/*.h
inclusions.

Signed-off-by: Jan Beulich 

--- a/xen/arch/x86/cpu/vpmu.c
+++ b/xen/arch/x86/cpu/vpmu.c
@@ -17,12 +17,12 @@
  *
  * Author: Haitao Shan 
  */
-#include 
-#include 
-#include 
-#include 
 #include 
+#include 
 #include 
+#include 
+#include 
+#include 
 #include 
 #include 
 #include 
@@ -49,6 +49,7 @@ CHECK_pmu_params;
 static unsigned int __read_mostly opt_vpmu_enabled;
 unsigned int __read_mostly vpmu_mode = XENPMU_MODE_OFF;
 unsigned int __read_mostly vpmu_features = 0;
+static struct arch_vpmu_ops __read_mostly vpmu_ops;
 
 static DEFINE_SPINLOCK(vpmu_lock);
 static unsigned vpmu_count;
@@ -120,7 +121,6 @@ int vpmu_do_msr(unsigned int msr, uint64
 {
 struct vcpu *curr = current;
 struct vpmu_struct *vpmu;
-const struct arch_vpmu_ops *ops;
 int ret = 0;
 
 /*
@@ -133,14 +133,13 @@ int vpmu_do_msr(unsigned int msr, uint64
  goto nop;
 
 vpmu = vcpu_vpmu(curr);
-ops = vpmu->arch_vpmu_ops;
-if ( !ops )
+if ( !vpmu_is_set(vpmu, VPMU_INITIALIZED) )
 goto nop;
 
-if ( is_write && ops->do_wrmsr )
-ret = ops->do_wrmsr(msr, *msr_content, supported);
-else if ( !is_write && ops->do_rdmsr )
-ret = ops->do_rdmsr(msr, msr_content);
+if ( is_write && vpmu_ops.do_wrmsr )
+ret = alternative_call(vpmu_ops.do_wrmsr, msr, *msr_content, 
supported);
+else if ( !is_write && vpmu_ops.do_rdmsr )
+ret = alternative_call(vpmu_ops.do_rdmsr, msr, msr_content);
 else
 goto nop;
 
@@ -153,7 +152,7 @@ int vpmu_do_msr(unsigned int msr, uint64
 vpmu_is_set(vpmu, VPMU_CACHED) )
 {
 vpmu_set(vpmu, VPMU_CONTEXT_SAVE);
-ops->arch_vpmu_save(curr, 0);
+alternative_vcall(vpmu_ops.arch_vpmu_save, curr, 0);
 vpmu_reset(vpmu, VPMU_CONTEXT_SAVE | VPMU_CONTEXT_LOADED);
 }
 
@@ -202,7 +201,7 @@ void vpmu_do_interrupt(struct cpu_user_r
 sampling = sampled;
 
 vpmu = vcpu_vpmu(sampling);
-if ( !vpmu->arch_vpmu_ops )
+if ( !vpmu_is_set(vpmu, VPMU_INITIALIZED) )
 return;
 
 /* PV(H) guest */
@@ -220,7 +219,7 @@ void vpmu_do_interrupt(struct cpu_user_r
 
 /* PV guest will be reading PMU MSRs from xenpmu_data */
 vpmu_set(vpmu, VPMU_CONTEXT_SAVE | VPMU_CONTEXT_LOADED);
-vpmu->arch_vpmu_ops->arch_vpmu_save(sampling, 1);
+alternative_vcall(vpmu_ops.arch_vpmu_save, sampling, 1);
 vpmu_reset(vpmu, VPMU_CONTEXT_SAVE | VPMU_CONTEXT_LOADED);
 
 if ( is_hvm_vcpu(sampled) )
@@ -321,7 +320,7 @@ void vpmu_do_interrupt(struct cpu_user_r
 /* We don't support (yet) HVM dom0 */
 ASSERT(sampling == sampled);
 
-if ( !vpmu->arch_vpmu_ops->do_interrupt(regs) ||
+if ( !alternative_call(vpmu_ops.do_interrupt, regs) ||
  !is_vlapic_lvtpc_enabled(vlapic) )
 return;
 
@@ -349,8 +348,7 @@ static void vpmu_save_force(void *arg)
 
 vpmu_set(vpmu, VPMU_CONTEXT_SAVE);
 
-if ( vpmu->arch_vpmu_ops )
-(void)vpmu->arch_vpmu_ops->arch_vpmu_save(v, 0);
+alternative_vcall(vpmu_ops.arch_vpmu_save, v, 0);
 
 vpmu_reset(vpmu, VPMU_CONTEXT_SAVE);
 
@@ -368,9 +366,8 @@ void vpmu_save(struct vcpu *v)
 vpmu->last_pcpu = pcpu;
 per_cpu(last_vcpu, pcpu) = v;
 
-if ( vpmu->arch_vpmu_ops )
-if ( vpmu->arch_vpmu_ops->arch_vpmu_save(v, 0) )
-vpmu_reset(vpmu, VPMU_CONTEXT_LOADED);
+if ( alternative_call(vpmu_ops.arch_vpmu_save, v, 0) )
+vpmu_reset(vpmu, VPMU_CONTEXT_LOADED);
 
 apic_write(APIC_LVTPC, PMU_APIC_VECTOR | APIC_LVT_MASKED);
 }
@@ -426,13 +423,13 @@ int vpmu_load(struct vcpu *v, bool_t fro
  vpmu_is_set(vpmu, VPMU_CACHED)) )
 return 0;
 
-if ( vpmu->arch_vpmu_ops && vpmu->arch_vpmu_ops->arch_vpmu_load )
+if ( vpmu_ops.arch_vpmu_load )
 {
 int ret;
 
 apic_write(APIC_LVTPC, vpmu->hw_lapic_lvtpc);
 /* Arch code needs to set VPMU_CONTEXT_LOADED */
-ret = vpmu->arch_vpmu_ops->arch_vpmu_load(v, from_guest);
+ret = alternative_call(vpmu_ops.arch_vpmu_load, v, from_guest);
 if ( ret )
 {
 apic_write(APIC_LVTPC, vpmu->hw_lapic_lvtpc | APIC_LVT_MASKED);
@@ -572,7 +569,7 @@ static void vpmu_arch_destroy(struct vcp
 on_selected_cpus(cpumask_of(vpmu->last_pcpu),
  vpmu_clear_last, v, 1);
 
-

[PATCH 0/3] x86/vPMU: adjustements to vendor hooks treatment

[Jan Beulich]

In the context of [1] the 1st patch will be of interest, whereas the
other changes are a result of observations while doing that conversion.

1: convert vendor hook invocations to altcall
2: invoke _vpmu_initialise() through a hook as well
3: move vpmu_ops to .init.data

Jan

[1] https://lists.xen.org/archives/html/xen-devel/2021-11/msg01822.html

[PATCH] x86/HVM: convert most remaining hvm_funcs hook invocations to alt-call

[Jan Beulich]

The aim being to have as few indirect calls as possible (see [1]),
whereas during initial conversion performance was the main aspect and
hence rarely used hooks didn't get converted. Apparently one use of
get_interrupt_shadow() was missed at the time.

While I've intentionally left alone the cpu_{up,down}() etc hooks for
not being guest reachable, the nhvm_hap_walk_L1_p2m() one can't
currently be converted as the framework supports only up to 6 arguments.
Down the road the three booleans perhaps want folding into a single
parameter/argument.

While doing this, drop NULL checks ahead of .nhvm_*() calls when the
hook is always present. Also convert the .nhvm_vcpu_reset() call to
alternative_vcall(), as the return value is unused and the caller has
currently no way of propagating it.

Signed-off-by: Jan Beulich 

[1] https://lists.xen.org/archives/html/xen-devel/2021-11/msg01822.html
---
Another candidate for dropping the conditional would be
.enable_msr_interception(), but this would then want the wrapper to also
return void (hence perhaps better done separately).

--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -2548,7 +2548,7 @@ static int hvmemul_vmfunc(
 
 if ( !hvm_funcs.altp2m_vcpu_emulate_vmfunc )
 return X86EMUL_UNHANDLEABLE;
-rc = hvm_funcs.altp2m_vcpu_emulate_vmfunc(ctxt->regs);
+rc = alternative_call(hvm_funcs.altp2m_vcpu_emulate_vmfunc, ctxt->regs);
 if ( rc == X86EMUL_EXCEPTION )
 x86_emul_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC, ctxt);
 
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -703,7 +703,7 @@ int hvm_domain_initialise(struct domain
 if ( rc )
 goto fail2;
 
-rc = hvm_funcs.domain_initialise(d);
+rc = alternative_call(hvm_funcs.domain_initialise, d);
 if ( rc != 0 )
 goto fail2;
 
@@ -736,7 +736,7 @@ void hvm_domain_relinquish_resources(str
 alternative_vcall(hvm_funcs.domain_relinquish_resources, d);
 
 if ( hvm_funcs.nhvm_domain_relinquish_resources )
-hvm_funcs.nhvm_domain_relinquish_resources(d);
+alternative_vcall(hvm_funcs.nhvm_domain_relinquish_resources, d);
 
 viridian_domain_deinit(d);
 
@@ -866,7 +866,7 @@ static int hvm_save_cpu_ctxt(struct vcpu
 return 0;
 
 /* Architecture-specific vmcs/vmcb bits */
-hvm_funcs.save_cpu_ctxt(v, );
+alternative_vcall(hvm_funcs.save_cpu_ctxt, v, );
 
 hvm_get_segment_register(v, x86_seg_idtr, );
 ctxt.idtr_limit = seg.limit;
@@ -1089,14 +1089,14 @@ static int hvm_load_cpu_ctxt(struct doma
 #undef UNFOLD_ARBYTES
 
 /* Architecture-specific vmcs/vmcb bits */
-if ( hvm_funcs.load_cpu_ctxt(v, ) < 0 )
+if ( alternative_call(hvm_funcs.load_cpu_ctxt, v, ) < 0 )
 return -EINVAL;
 
 v->arch.hvm.guest_cr[2] = ctxt.cr2;
 hvm_update_guest_cr(v, 2);
 
 if ( hvm_funcs.tsc_scaling.setup )
-hvm_funcs.tsc_scaling.setup(v);
+alternative_vcall(hvm_funcs.tsc_scaling.setup, v);
 
 v->arch.msrs->tsc_aux = ctxt.msr_tsc_aux;
 
@@ -1559,7 +1559,8 @@ int hvm_vcpu_initialise(struct vcpu *v)
 if ( rc != 0 ) /* teardown: vlapic_destroy */
 goto fail2;
 
-if ( (rc = hvm_funcs.vcpu_initialise(v)) != 0 ) /* teardown: 
hvm_funcs.vcpu_destroy */
+rc = alternative_call(hvm_funcs.vcpu_initialise, v);
+if ( rc != 0 ) /* teardown: hvm_funcs.vcpu_destroy */
 goto fail3;
 
 softirq_tasklet_init(>arch.hvm.assert_evtchn_irq_tasklet,
@@ -1607,7 +1608,7 @@ int hvm_vcpu_initialise(struct vcpu *v)
 free_compat_arg_xlat(v);
  fail4:
 hvmemul_cache_destroy(v);
-hvm_funcs.vcpu_destroy(v);
+alternative_vcall(hvm_funcs.vcpu_destroy, v);
  fail3:
 vlapic_destroy(v);
  fail2:
@@ -1631,7 +1632,7 @@ void hvm_vcpu_destroy(struct vcpu *v)
 free_compat_arg_xlat(v);
 
 tasklet_kill(>arch.hvm.assert_evtchn_irq_tasklet);
-hvm_funcs.vcpu_destroy(v);
+alternative_vcall(hvm_funcs.vcpu_destroy, v);
 
 vlapic_destroy(v);
 
@@ -3863,7 +3864,7 @@ enum hvm_intblk hvm_interrupt_blocked(st
  !(guest_cpu_user_regs()->eflags & X86_EFLAGS_IF) )
 return hvm_intblk_rflags_ie;
 
-intr_shadow = hvm_funcs.get_interrupt_shadow(v);
+intr_shadow = alternative_call(hvm_funcs.get_interrupt_shadow, v);
 
 if ( intr_shadow & (HVM_INTR_SHADOW_STI|HVM_INTR_SHADOW_MOV_SS) )
 return hvm_intblk_shadow;
@@ -3979,7 +3980,7 @@ void hvm_vcpu_reset_state(struct vcpu *v
 hvm_set_segment_register(v, x86_seg_idtr, );
 
 if ( hvm_funcs.tsc_scaling.setup )
-hvm_funcs.tsc_scaling.setup(v);
+alternative_vcall(hvm_funcs.tsc_scaling.setup, v);
 
 /* Sync AP's TSC with BSP's. */
 v->arch.hvm.cache_tsc_offset =
--- a/xen/arch/x86/hvm/nestedhvm.c
+++ b/xen/arch/x86/hvm/nestedhvm.c
@@ -54,8 +54,7 @@ nestedhvm_vcpu_reset(struct vcpu *v)
 
 hvm_asid_flush_vcpu_asid(>nv_n2asid);
 
-if ( hvm_funcs.nhvm_vcpu_reset )
-hvm_funcs.nhvm_vcpu_reset(v);
+

Re: [PATCH 0/4] x86: Further harden function pointers

[Jan Beulich]

On 26.11.2021 22:22, Andrew Cooper wrote:
> Slightly RFC, because patch 2 has some minor structure (ab)use, but the result
> works alarmingly well.  So far, this demonstrates converting two subsystems.
> 
> hvm_funcs is the other area of especially low hanging fruit, but IOMMU, vPMU
> also look like good candidates.  Anything which is partially altcall'd already
> would benefit from being fully altcall'd.

I'll post patches for hvm_funcs and vPMU hopefully later today. I intend
to look into the remaining unconverted IOMMU instances (so far I've
spotted one, but proper auditing may turn up more). For hvm_funcs what I
have leaves a few ones still unconverted; I guess we can discuss whether
to go beyond what I have in the context of that patch.

> Should we consider introducing __ro_after_init right now (as an alias to
> __read_mostly) as this conversion is touching a lot of ares where true
> post-init immutability ought to be enforced.

Well, it's largely orthogonal, but this might indeed be a good opportunity
to at least make a first step. I'd go slightly beyond what you say and at
least also introduce a respective new section, rather than aliasing
__read_mostly.

Jan

> Andrew Cooper (4):
>   x86/altcall: Check and optimise altcall targets
>   x86/altcall: Optimise away endbr64 instruction where possible
>   xen/xsm: Use __init_data_cf_clobber for xsm_ops
>   x86/ucode: Use altcall, and __initdata_cf_clobber
> 
>  xen/arch/x86/alternative.c   | 60 
> 
>  xen/arch/x86/cpu/microcode/amd.c |  2 +-
>  xen/arch/x86/cpu/microcode/core.c| 38 ---
>  xen/arch/x86/cpu/microcode/intel.c   |  2 +-
>  xen/arch/x86/cpu/microcode/private.h |  2 +-
>  xen/arch/x86/xen.lds.S   |  5 +++
>  xen/include/xen/init.h   |  2 ++
>  xen/xsm/dummy.c  |  2 +-
>  xen/xsm/flask/hooks.c|  2 +-
>  xen/xsm/silo.c   |  2 +-
>  10 files changed, 93 insertions(+), 24 deletions(-)
> 

[ovmf test] 166949: all pass - PUSHED

[osstest service owner]

flight 166949 ovmf real [real]
http://logs.test-lab.xenproject.org/osstest/logs/166949/

Perfect :-)
All tests in this flight passed as required
version targeted for testing:
 ovmf ef9a059cdb15844fe52a49af2bf7d86b9dd3e9bf
baseline version:
 ovmf bb1bba3d776733c41dbfa2d1dc0fe234819a79f2

Last test of basis   166826  2021-11-26 09:11:08 Z2 days
Testing same since   166949  2021-11-29 06:11:36 Z0 days1 attempts


People who touched revisions under test:
  Michael D Kinney 

jobs:
 build-amd64-xsm  pass
 build-i386-xsm   pass
 build-amd64  pass
 build-i386   pass
 build-amd64-libvirt  pass
 build-i386-libvirt   pass
 build-amd64-pvopspass
 build-i386-pvops pass
 test-amd64-amd64-xl-qemuu-ovmf-amd64 pass
 test-amd64-i386-xl-qemuu-ovmf-amd64  pass



sg-report-flight on osstest.test-lab.xenproject.org
logs: /home/logs/logs
images: /home/logs/images

Logs, config files, etc. are available at
http://logs.test-lab.xenproject.org/osstest/logs

Explanation of these reports, and of osstest in general, is at
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master

Test harness code can be found at
http://xenbits.xen.org/gitweb?p=osstest.git;a=summary


Pushing revision :

To xenbits.xen.org:/home/xen/git/osstest/ovmf.git
   bb1bba3d77..ef9a059cdb  ef9a059cdb15844fe52a49af2bf7d86b9dd3e9bf -> 
xen-tested-master