[linux-5.4 test] 181525: trouble: blocked/broken/fail/pass

[osstest service owner]

flight 181525 linux-5.4 real [real]
http://logs.test-lab.xenproject.org/osstest/logs/181525/

Failures and problems with tests :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 build-armhf  broken
 build-armhf   4 host-install(4)broken REGR. vs. 181363

Tests which are failing intermittently (not blocking):
 test-amd64-i386-qemuu-rhel6hvm-amd 7 xen-install fail in 181508 pass in 181525
 test-amd64-i386-freebsd10-amd64  7 xen-install fail pass in 181508
 test-amd64-i386-pair 10 xen-install/src_host   fail pass in 181508

Tests which did not succeed, but are not blocking:
 build-armhf-libvirt   1 build-check(1)   blocked  n/a
 test-armhf-armhf-examine  1 build-check(1)   blocked  n/a
 test-armhf-armhf-libvirt  1 build-check(1)   blocked  n/a
 test-armhf-armhf-libvirt-qcow2  1 build-check(1)   blocked  n/a
 test-armhf-armhf-libvirt-raw  1 build-check(1)   blocked  n/a
 test-armhf-armhf-xl   1 build-check(1)   blocked  n/a
 test-armhf-armhf-xl-arndale   1 build-check(1)   blocked  n/a
 test-armhf-armhf-xl-credit1   1 build-check(1)   blocked  n/a
 test-armhf-armhf-xl-credit2   1 build-check(1)   blocked  n/a
 test-armhf-armhf-xl-multivcpu  1 build-check(1)   blocked  n/a
 test-armhf-armhf-xl-rtds  1 build-check(1)   blocked  n/a
 test-armhf-armhf-xl-vhd   1 build-check(1)   blocked  n/a
 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 181363
 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 181363
 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 181363
 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 181363
 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 181363
 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 181363
 test-amd64-amd64-xl-qcow221 guest-start/debian.repeatfail  like 181363
 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 181363
 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 181363
 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 181363
 test-amd64-i386-xl-pvshim14 guest-start  fail   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-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail never pass
 test-amd64-i386-libvirt  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-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  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  16 saverestore-support-checkfail   never pass
 test-amd64-i386-libvirt-raw  14 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail 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-arm64-arm64-xl-vhd   3 hosts-allocate   starved in 181508 n/a

version targeted for testing:
 linux61a2f83e4762ee0c766f86944e612305f5888bcb
baseline version:
 linuxf568a20f058fa1e37069cff4aac4187c1650a0e9

Last test of basis   181363  2023-06-10 15:39:59 Z   10 days
Testing same since   181425  2023-06-14 10:50:49 Z6 days   12 attempts


People who touched revisions under test:
  Ai Chao 
  Alan Stern 
  Alex Deucher 
  Alexander Sverdlin 
  Arnd B

Re: [XEN PATCH 03/13] xen/x86: fixed violations of MISRA C:2012 Rule 7.2

[Jan Beulich]

On 20.06.2023 22:49, Stefano Stabellini wrote:
> On Tue, 20 Jun 2023, Simone Ballarin wrote:
>> From: Gianluca Luparini 
>>
>> The xen sources contains violations of MISRA C:2012 Rule 7.2 whose headline 
>> states:
>> "A "u" or "U" suffix shall be applied to all integer constants that are 
>> represented in an unsigned type".
>>
>> I propose to use "U" as a suffix to explicitly state when an integer 
>> constant is represented in an unsigned type.
>>
>> Signed-off-by: Simone Ballarin 
> 
> Commit message aside:
> 
> Reviewed-by: Stefano Stabellini 
> 
> FYI I would use the following as commit message title to make it unique:
> 
> xen/x86/hvm/svm: fixed violations of MISRA C:2012 Rule 7.2

Along the lines of my earlier comment:

x86/svm: fix violations of MISRA C:2012 Rule 7.2

or (imo less desirable)

xen/svm: fix violations of MISRA C:2012 Rule 7.2

At the very least the hvm/ infix is fully redundant.

Jan

Re: [XEN PATCH 06/13] xen/efi: fixed violations of MISRA C:2012 Rule 7.2

[Jan Beulich]

On 20.06.2023 22:56, Stefano Stabellini wrote:
> On Tue, 20 Jun 2023, Jan Beulich wrote:
>> On 20.06.2023 12:34, Simone Ballarin wrote:
>>> From: Gianluca Luparini 
>>>
>>> The xen sources contains violations of MISRA C:2012 Rule 7.2 whose headline 
>>> states:
>>> "A "u" or "U" suffix shall be applied to all integer constants that are 
>>> represented in an unsigned type".
>>>
>>> I propose to use "U" as a suffix to explicitly state when an integer 
>>> constant is represented in an unsigned type.
>>> For homogeneity, I also added the "U" suffix in some cases that the tool 
>>> didn't report as violations.
>>>
>>> Signed-off-by: Simone Ballarin 
>>> ---
>>>  xen/arch/x86/include/asm/x86_64/efibind.h | 10 +-
>>
>> This file as well as ...
>>
>>>  xen/common/efi/boot.c |  8 
>>>  xen/common/efi/runtime.c  |  2 +-
>>>  xen/include/efi/efiapi.h  | 10 +-
>>>  xen/include/efi/efidef.h  |  2 +-
>>>  xen/include/efi/efiprot.h | 22 +++---
>>
>> ... the last three here are imported from the gnu-efi package. I'm wary
>> of touching them, and thus getting them more out of sync with their
>> original than strictly necessary. To allow the other changes to go in
>> no matter what, I'd like to suggest splitting the patch.
> 
> Should we add either those files individually or the directory
> xen/include/efi (plus xen/arch/x86/include/asm/x86_64/efibind.h) to
> docs/misra/exclude-list.json ?

Probably, and in the former case imo the entire directory.

Jan

Re: [PATCH v1 01/23] pc/xen: Xen Q35 support: provide IRQ handling for PCI devices

[Daniel P . Berrangé]

On Tue, Jun 20, 2023 at 01:24:34PM -0400, Joel Upham wrote:
> 
> Signed-off-by: Alexey Gerasimenko 

This isn't a valid email address for Alexey - I presume you grabbed
these patches from the xen-devel mail archives, which have mangled
the addresses for anti-spam reasons.

Fortunately there are alternative archives which don't mangle the
patches:

  
https://lore.kernel.org/xen-devel/6067bc3c91c9ee629a35723dfb474ef168ff4ebf.1520867955.git.x19...@gmail.com/

  Signed-off-by: Alexey Gerasimenko 

This affects all patches in the series, but I won't repeat my
comment on each one.

> Signed-off-by: Joel Upham 
> ---
>  hw/i386/pc_piix.c |  3 +-
>  hw/i386/xen/xen-hvm.c |  7 +++--
>  hw/isa/lpc_ich9.c | 53 ---
>  hw/isa/piix3.c|  2 +-
>  include/hw/southbridge/ich9.h |  1 +
>  include/hw/xen/xen.h  |  4 +--
>  stubs/xen-hw-stub.c   |  4 +--
>  7 files changed, 61 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index d5b0dcd1fe..8c1b20f3bc 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -62,6 +62,7 @@
>  #endif
>  #include "hw/xen/xen-x86.h"
>  #include "hw/xen/xen.h"
> +#include "sysemu/xen.h"
>  #include "migration/global_state.h"
>  #include "migration/misc.h"
>  #include "sysemu/numa.h"
> @@ -233,7 +234,7 @@ static void pc_init1(MachineState *machine,
>x86ms->above_4g_mem_size,
>pci_memory, ram_memory);
>  pci_bus_map_irqs(pci_bus,
> - xen_enabled() ? xen_pci_slot_get_pirq
> + xen_enabled() ? xen_cmn_pci_slot_get_pirq
> : pc_pci_slot_get_pirq);
>  pcms->bus = pci_bus;
>  
> diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
> index 56641a550e..540ac46639 100644
> --- a/hw/i386/xen/xen-hvm.c
> +++ b/hw/i386/xen/xen-hvm.c
> @@ -15,6 +15,7 @@
>  #include "hw/pci/pci.h"
>  #include "hw/pci/pci_host.h"
>  #include "hw/i386/pc.h"
> +#include "hw/southbridge/ich9.h"
>  #include "hw/irq.h"
>  #include "hw/hw.h"
>  #include "hw/i386/apic-msidef.h"
> @@ -136,14 +137,14 @@ typedef struct XenIOState {
>  Notifier wakeup;
>  } XenIOState;
>  
> -/* Xen specific function for piix pci */
> +/* Xen-specific functions for pci dev IRQ handling */
>  
> -int xen_pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num)
> +int xen_cmn_pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num)
>  {
>  return irq_num + (PCI_SLOT(pci_dev->devfn) << 2);
>  }
>  
> -void xen_piix3_set_irq(void *opaque, int irq_num, int level)
> +void xen_cmn_set_irq(void *opaque, int irq_num, int level)
>  {
>  xen_set_pci_intx_level(xen_domid, 0, 0, irq_num >> 2,
> irq_num & 3, level);
> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index 9c47a2f6c7..733a99d443 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -51,6 +51,9 @@
>  #include "hw/core/cpu.h"
>  #include "hw/nvram/fw_cfg.h"
>  #include "qemu/cutils.h"
> +#include "hw/xen/xen.h"
> +#include "sysemu/xen.h"
> +#include "hw/southbridge/piix.h"
>  #include "hw/acpi/acpi_aml_interface.h"
>  #include "trace.h"
>  
> @@ -535,11 +538,49 @@ static int ich9_lpc_post_load(void *opaque, int 
> version_id)
>  return 0;
>  }
>  
> +static void ich9_lpc_config_write_xen(PCIDevice *d,
> +  uint32_t addr, uint32_t val, int len)
> +{
> +static bool pirqe_f_warned = false;
> +if (ranges_overlap(addr, len, ICH9_LPC_PIRQA_ROUT, 4)) {
> +/* handle PIRQA..PIRQD routing */
> +/* Scan for updates to PCI link routes (0x60-0x63). */
> +int i;
> +for (i = 0; i < len; i++) {
> +uint8_t v = (val >> (8 * i)) & 0xff;
> +if (v & 0x80) {
> +v = 0;
> +}
> +v &= 0xf;
> +if (((addr + i) >= PIIX_PIRQCA) && ((addr + i) <= PIIX_PIRQCD)) {
> +xen_set_pci_link_route(addr + i - PIIX_PIRQCA, v);
> +}
> +}
> +} else if (ranges_overlap(addr, len, ICH9_LPC_PIRQE_ROUT, 4)) {
> +while (len--) {
> +if (range_covers_byte(ICH9_LPC_PIRQE_ROUT, 4, addr) &&
> +(val & 0x80) == 0) {
> +/* print warning only once */
> +if (!pirqe_f_warned) {
> +pirqe_f_warned = true;
> +fprintf(stderr, "WARNING: guest domain attempted to use 
> PIRQ%c "
> +"routing which is not supported for Xen/Q35 
> currently\n",
> +(char)(addr - ICH9_LPC_PIRQE_ROUT + 'E'));
> +break;
> +}
> +}
> +addr++, val >>= 8;
> +}
> +}
> +}
> +
>  static void ich9_lpc_config_write(PCIDevice *d,
>uint32_t addr, uint32_t val, int len)
>  {
>  ICH9LPCState *lpc = 

Re: [XEN PATCH 12/13] xen/x86: fixed violations of MISRA C:2012 Rule 7.2

[Jan Beulich]

First of all - please trim replies.

On 20.06.2023 23:23, Stefano Stabellini wrote:
> On Tue, 20 Jun 2023, Simone Ballarin wrote:
>> --- a/xen/arch/x86/percpu.c
>> +++ b/xen/arch/x86/percpu.c
>> @@ -12,7 +12,7 @@ unsigned long __per_cpu_offset[NR_CPUS];
>>   * possible #PF at (NULL + a little) which has security implications in the
>>   * context of PV guests.
>>   */
>> -#define INVALID_PERCPU_AREA (0x8000L - (long)__per_cpu_start)
>> +#define INVALID_PERCPU_AREA (0x8000UL - (long)__per_cpu_start)
>>  #define PERCPU_ORDER get_order_from_bytes(__per_cpu_data_end - 
>> __per_cpu_start)
> 
> Hi Jan, should this be ULL?

Not really, no - we're 64-bit only. Furthermore the expression is
about addresses, which correspond to "unsigned long" in our world.

>> --- a/xen/include/public/arch-x86/xen-x86_64.h
>> +++ b/xen/include/public/arch-x86/xen-x86_64.h
>> @@ -53,10 +53,10 @@
>>  #define FLAT_USER_SS32 FLAT_RING3_SS32
>>  #define FLAT_USER_SS   FLAT_USER_SS64
>>  
>> -#define __HYPERVISOR_VIRT_START 0x8000
>> -#define __HYPERVISOR_VIRT_END   0x8800
>> -#define __MACH2PHYS_VIRT_START  0x8000
>> -#define __MACH2PHYS_VIRT_END0x8040
>> +#define __HYPERVISOR_VIRT_START 0x8000U
>> +#define __HYPERVISOR_VIRT_END   0x8800U
>> +#define __MACH2PHYS_VIRT_START  0x8000U
>> +#define __MACH2PHYS_VIRT_END0x8040U
> 
> Also here ULL?

UL yes, but as above I don't think ULL is good to use for addresses.
That said, things are a little less clear in the public headers: In
principle it could be a goal for them to be usable on foreign
architectures (say e.g. a 32-bit tool stack, or an analysis tool
which can be run without being on the original host architecture) as
well.

Furthermore, open-coded use of ULL would make this no-longer-C89.
If anything, it would then need to be UINT64_C(), yet even that would
grow our public header dependencies on C99-like infrastructure being
available (right now we only require certain stdint.h-like types;
Arm alone, interestingly, also uses PRIx64 and PRIu64, which may be
kind of unavoidable there).

Jan

Re: [PATCH] docs/misra: add Rules 8.2, 8.3, 8.14

[Jan Beulich]

On 21.06.2023 03:26, Stefano Stabellini wrote:
> --- a/docs/misra/rules.rst
> +++ b/docs/misra/rules.rst
> @@ -213,6 +213,17 @@ maintainers if you want to suggest a change.
>   - Types shall be explicitly specified
>   -
>  
> +   * - `Rule 8.2 
> `_
> + - Required
> + - Function types shall be in prototype form with named parameters
> + -
> +
> +   * - `Rule 8.3 
> `_
> + - Required
> + - All declarations of an object or function shall use the same
> +   names and type qualifiers
> + -

I think we want to deal with uses of const when not qualifying a pointed-to
type: One approach is to simply say we don't use const like this (and the
few uses there are should then go away). The other, if we deem this a
valuable feature, would be to make a project-wide exception for this case,
as having such const in declarations is meaningless and hence at the risk
of being confusing or hampering readability.

Jan

Re: [PATCH] x86: Drop opt_pku entirely

[Jan Beulich]

On 20.06.2023 19:47, Andrew Cooper wrote:
> --- a/CHANGELOG.md
> +++ b/CHANGELOG.md
> @@ -25,6 +25,9 @@ The format is based on [Keep a 
> Changelog](https://keepachangelog.com/en/1.0.0/)
>   - Add support for AVX512-FP16 on x86.
>   - On Arm, Xen supports guests running SVE/SVE2 instructions. (Tech Preview)
>  
> +### Removed
> + - On x86, the "pku" command line option has been removed.  It has never
> +   behaved precisely as described, and redundant with "cpuid=no-pku".

Nit: Missing "was"?

> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -1950,16 +1950,6 @@ for all of them (`true`), only for those subject to 
> XPTI (`xpti`) or for
>  those not subject to XPTI (`no-xpti`). The feature is used only in case
>  INVPCID is supported and not disabled via `invpcid=false`.
>  
> -### pku (x86)
> -> `= `
> -
> -> Default: `true`
> -
> -Flag to enable Memory Protection Keys.
> -
> -The protection-key feature provides an additional mechanism by which IA-32e
> -paging controls access to usermode addresses.
> -
>  ### ple_gap
>  > `= `

Elsewhere you said that we kind of imply that only the explicitly named
sub-options of cpuid= are supported. If that's the case (which could do
with saying more explicitly), you will want to add "pku" there in order
to not regress what is (deemed) supported.

It also looks as if the speculation control related enumeration there has
gone stale / is now incomplete.

Jan

Re: [PATCH v4 06/15] cpufreq: Add Hardware P-State (HWP) driver

[Jan Beulich]

On 20.06.2023 20:14, Jason Andryuk wrote:
> On Mon, Jun 19, 2023 at 7:38 AM Jan Beulich  wrote:
>> On 14.06.2023 20:02, Jason Andryuk wrote:
>>> Falling back from cpufreq=hwp to cpufreq=xen is a more user-friendly
>>> choice than disabling cpufreq when HWP is not available.  Specifying
>>> cpufreq=hwp indicates the user wants cpufreq, so, if HWP isn't
>>> available, it makes sense to give them the cpufreq that can be
>>> supported.  i.e. I can't see a user only wanting cpufreq=hwp or
>>> cpufreq=none, but not cpufreq=xen.
>>
>> I continue to disagree with this, and I'd like to ask for another
>> maintainer's opinion. To me the described behavior is like getting
>> pears when having asked for apples. I nevertheless agree that
>> having said fallback as an option, but I'd see that done by giving
>> meaning to something like "cpufreq=hwp,xen" (without having checked
>> whether that doesn't have meaning already, i.e. just to give you an
>> idea).
> 
> I know you disagree with the approach.  I implemented what would be
> useful to me and Marek.  It felt counterproductive to implement a hard
> failure mode that is unsuitable for my use case.  Also there was the
> possibility you wouldn't mind when you saw how it was implemented.
> 
> Yeah, asking for an apple and getting a pear can be the wrong thing if
> your recipe calls for apples.  But getting *some* fruit can be better
> than no fruit if you are hungry.

;-)

> As implemented here, with HWP default disabled,
> no command line -> default cpufreq=xen
> cpufreq=xen -> only cpufreq=xen
> cpufreq=hwp -> try hwp and fallback to cpufreq=xen
> 
> If/when HWP is default enabled:
> no command line -> try hwp and fallback to cpufreq=xen
> cpufreq=hwp -> try hwp and fallback to cpufreq=xen
> cpufreq=xen -> cpufreq=xen

At which point the question would be why to have such an option in
the first place. Plus how to specify that fallback to "xen" is not
wanted.

> Unless some other idea comes to me, I guess I'll look at implementing
> "cpufreq=hwp,xen".

Thanks.

>>> +static bool hwp_handle_option(const char *s, const char *end)
>>> +{
>>> +int ret;
>>> +
>>> +if ( strncmp(s, "verbose", 7) == 0 )
>>> +{
>>> +s += 7;
>>> +if ( *s == '=' )
>>> +{
>>> +s++;
>>> +cpufreq_verbose = !!simple_strtoul(s, NULL, 0);
>>> +
>>> +return true;
>>> +}
>>> +
>>> +if ( end == NULL ||
>>> + (end == s && (*end == '\0' || *end == ',')) )
>>> +{
>>> +cpufreq_verbose = true;
>>> +
>>> +return true;
>>> +}
>>> +
>>> +return false;
>>> +}
>>
>> Would be nice if the handling of "verbose" didn't need duplicating here.
>> However, if that's unavoidable, did you consider handling this as a
>> proper boolean instead of the custom way cpufreq_handle_common_option()
>> also uses?
> 
> It seemed more complicated to try to re-use the "verbose" handling
> from cpufreq_handle_common_option(), especially since minfreq and
> maxfreq are also in there.
> 
> I didn't use proper boolean parsing to remain consistent with
> cpufreq_handle_common_option() and the command line option
> documentation.  I'm fine with switching it to a proper boolean if
> that's what you want.

It would be nice if you could do that here, but I won't insist.

>>> +goto error;
>>> +}
>>> +
>>> +/*
>>> + * Check for APERF/MPERF support in hardware
>>> + * also check for boost/turbo support
>>> + */
>>> +intel_feature_detect(policy);
>>> +
>>> +if ( feature_hdc )
>>> +{
>>> +if ( hdc_set_pkg_hdc_ctl(policy->cpu, opt_cpufreq_hdc) )
>>> +goto error;
>>> +if ( hdc_set_pm_ctl1(policy->cpu, opt_cpufreq_hdc) )
>>> +goto error;
>>
>> If either of these fails the very first time through (presumably for the
>> BSP), wouldn't a better course of action be to clear feature_hdc?
> 
> So if HWP is working, but the HDC part fails, just clear feature_hdc
> but keep using HWP?  I don't know how likely that is to happen, but
> that seems reasonable.

Just to answer the question - yes, that's what I was thinking of. Maybe
accompanied by a log message.

>>> +static int cf_check hwp_cpufreq_target(struct cpufreq_policy *policy,
>>> +   unsigned int target_freq,
>>> +   unsigned int relation)
>>> +{
>>> +unsigned int cpu = policy->cpu;
>>> +struct hwp_drv_data *data = per_cpu(hwp_drv_data, cpu);
>>> +/* Zero everything to ensure reserved bits are zero... */
>>> +union hwp_request hwp_req = { .raw = 0 };
>>> +
>>> +/* .. and update from there */
>>> +hwp_req.min_perf = data->minimum;
>>> +hwp_req.max_perf = data->maximum;
>>> +hwp_req.desired = data->desired;
>>> +hwp_req.energy_perf = data->energy_perf;
>>> +if ( feature_hwp_activity_window )
>>> +hwp_req.activity_window = data->activity_window;
>>> +
>>> +if (

[XEN PATCH v2] xen/include: avoid undefined behavior.

[Nicola Vetrini]

Redefine BUILD_BUG_ON_ZERO to fully comply with C99 avoiding
undefined behavior 58 ("A structure or union is defined as
containing no named members (6.7.2.1)."

The chosen ill-formed construct is a negative bitwidth in a
bitfield within a struct containing at least one named member,
which prevents the UB while keeping the semantics of the construct
for any memory layout of the struct (this motivates the
"sizeof(unsigned) * 8" in the definition of the macro).

Signed-off-by: Nicola Vetrini 
---
Changes in V2:
- Avoid using a VLA as the compile-time assertion
- Do not drop _Static_assert
---
 xen/include/xen/lib.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
index 67fc7c1d7e..e57d272772 100644
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -51,9 +51,10 @@
e.g. in a structure initializer (or where-ever else comma expressions
aren't permitted). */
 #define BUILD_BUG_ON_ZERO(cond) \
-sizeof(struct { _Static_assert(!(cond), "!(" #cond ")"); })
+(sizeof(struct { char c; _Static_assert(!(cond), "!(" #cond ")"); }) - 1U)
 #else
-#define BUILD_BUG_ON_ZERO(cond) sizeof(struct { int:-!!(cond); })
+#define BUILD_BUG_ON_ZERO(cond) \
+(sizeof(struct { unsigned u : (cond) ? -1 : sizeof(unsigned) * 8; }) - 
sizeof(unsigned))
 #define BUILD_BUG_ON(cond) ((void)BUILD_BUG_ON_ZERO(cond))
 #endif
 
-- 
2.34.1

Re: [PATCH v4 09/15] cpufreq: Export HWP parameters to userspace as CPPC

[Jan Beulich]

On 20.06.2023 20:41, Jason Andryuk wrote:
> On Mon, Jun 19, 2023 at 10:24 AM Jan Beulich  wrote:
>> On 14.06.2023 20:02, Jason Andryuk wrote:
>>> +else
>>>  {
>>> +if ( !(scaling_available_governors =
>>> +   xzalloc_array(char, gov_num * CPUFREQ_NAME_LEN)) )
>>> +return -ENOMEM;
>>> +if ( (ret = read_scaling_available_governors(
>>> +scaling_available_governors,
>>> +gov_num * CPUFREQ_NAME_LEN * sizeof(char))) )
>>
>> I realize you only re-indent this, but since you need to touch it anyway,
>> may I suggest to also switch to siezof(*scaling_available_governors)?
> 
> How about dropping sizeof(*scaling_available_governors)?  This length ...
> 
>>> +{
>>> +xfree(scaling_available_governors);
>>> +return ret;
>>> +}
>>> +ret = copy_to_guest(op->u.get_para.scaling_available_governors,
>>> +scaling_available_governors, gov_num * 
>>> CPUFREQ_NAME_LEN);
>>
>> Similarly here: Please adjust indentation while you touch this code.
> 
> ... should match here, but this second one lacks the "* sizeof($foo)".

Indeed it does, because that multiplication happens inside copy_to_guest()
(really copy_to_guest_offset()).

> They are strings, so multiplying by sizeof() is unusual.

Kind of, but in code which may want switching to Unicode (and not just
UTF-8,but e.g. UCS2 or UCS4) at some point it's a good idea to have such
right away. We don't mean to do any such switch, but I think it's good
practice to not assume that strings / string literals only ever consist
of plain char elements.

> FTAOD, you want the indenting as:
> ret = copy_to_guest(op->u.get_para.scaling_available_governors,
> scaling_available_governors,
> gov_num * CPUFREQ_NAME_LEN);
> ?

That's one conforming way, yes. Another would be

ret = copy_to_guest(
  op->u.get_para.scaling_available_governors,
  scaling_available_governors,
  gov_num * CPUFREQ_NAME_LEN);

>>> --- a/xen/include/public/sysctl.h
>>> +++ b/xen/include/public/sysctl.h
>>> @@ -296,6 +296,61 @@ struct xen_ondemand {
>>>  uint32_t up_threshold;
>>>  };
>>>
>>> +struct xen_cppc_para {
>>> +/* OUT */
>>> +/* activity_window supported if 1 */
>>> +#define XEN_SYSCTL_CPPC_FEAT_ACT_WINDOW  (1 << 0)
>>
>> I think 1 isn't very helpful, looking forward. Perhaps better "set" or
>> "flag set"?
> 
> "set" works for me.
> 
>>> +uint32_t features; /* bit flags for features */
>>> +/*
>>> + * See Intel SDM: HWP Performance Range and Dynamic Capabilities
>>> + *
>>> + * These four are 0-255 hardware-provided values.  They "continuous,
>>> + * abstract unit-less, performance" values.  smaller numbers are slower
>>> + * and larger ones are faster.
>>> + */
>>> +uint32_t lowest;
>>> +uint32_t lowest_nonlinear; /* most_efficient */
>>
>> Why non_linear in the external interface when internally you use
>> most_efficient (merely put in the comment here)?
>>
>>> +uint32_t nominal; /* guaranteed */
>>
>> Similar question for the name choice here.
> 
> There is a naming mismatch between the HWP fields and the CPPC fields.
> The commit message includes:
> The HWP most_efficient is mapped to CPPC lowest_nonlinear, and guaranteed is
> mapped to nominal.  CPPC has a guaranteed that is optional while nominal
> is required.  ACPI spec says "If this register is not implemented, OSPM
> assumes guaranteed performance is always equal to nominal performance."
> 
> So the comments were to help with the mapping.  Should I prefix the
> comments like "HWP: most_efficient"?

Yes please. (I was going to suggest exactly that when I hadn't read this
question, yet.)

Jan

Re: [PATCH] x86/vpmu: Simplify is_pmc_quirk

[Jan Beulich]

On 20.06.2023 19:45, Andrew Cooper wrote:
> This should be static, and there's no need for a separate (non-init, even)
> function to perform a simple equality test.  Drop the is_ prefix which is
> gramatically questionable, and make it __ro_after_init.
> 
> Leave a TODO, because the behaviour is definitely wrong to be applied to ~all
> modern Intel CPUs, and has been raised on xen-devel previously without
> conclusion.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper 

Reviewed-by: Jan Beulich 
with one request:

> @@ -967,7 +960,8 @@ const struct arch_vpmu_ops *__init core2_vpmu_init(void)
>sizeof(uint64_t) * fixed_pmc_cnt +
>sizeof(struct xen_pmu_cntr_pair) * arch_pmc_cnt;
>  
> -check_pmc_quirk();
> +/* TODO: This is surely wrong. */
> +pmc_quirk = current_cpu_data.x86 == 6;

In the description you say "~all modern Intel CPUs", which suggests it might
be correct for old enough ones. Would you mind weakening the comment to
"This surely isn't universally correct" or some such?

Jan

Re: [XEN PATCH 05/13] xen/common: fixed violations of MISRA C:2012 Rule 7.2

[Simone Ballarin]

Il giorno mar 20 giu 2023 alle ore 14:43 Jan Beulich  ha
scritto:

> On 20.06.2023 12:34, Simone Ballarin wrote:
> > From: Gianluca Luparini 
> >
> > The xen sources contains violations of MISRA C:2012 Rule 7.2 whose
> headline states:
> > "A "u" or "U" suffix shall be applied to all integer constants that are
> represented in an unsigned type".
> >
> > I propose to use "U" as a suffix to explicitly state when an integer
> constant is represented in an unsigned type.
> >
> > Signed-off-by: Simone Ballarin 
> > ---
> >  xen/common/device_tree.c| 4 ++--
> >  xen/include/xen/libfdt/fdt.h| 2 +-
> >  xen/include/xen/libfdt/libfdt.h | 2 +-
> >  3 files changed, 4 insertions(+), 4 deletions(-)
>
> I think me and a few other people being on Cc here is attributed to the
> (misleading) title. The set of touched files fully maps to "DEVICE TREE"
> in ./MAINTAINERS afaict, which the prefix in the title would then also
> be nice to express.
>

Yes, my bad. I will fix the commit name.


> That said I'm not sure whether libfdt code actually wants touching this
> way.
>
Yes, you are right. libfdt is out of scope. I will remove the changes in it.

>
> Jan
>


-- 
Simone Ballarin, M.Sc.

Field Application Engineer, BUGSENG (https://bugseng.com
)

Re: [QEMU PATCH 1/1] virtgpu: do not destroy resources when guest suspend

[Gerd Hoffmann]

On Tue, Jun 20, 2023 at 01:26:15PM +0100, Robert Beckett wrote:
> 
> On 20/06/2023 10:41, Gerd Hoffmann wrote:
> >Hi,
> > 
> > > > The guest driver should be able to restore resources after resume.
> > > Thank you for your suggestion!
> > > As far as I know, resources are created on host side and guest has no 
> > > backup, if resources are destroyed, guest can't restore them.
> > > Or do you mean guest driver need to send commands to re-create resources 
> > > after resume?
> > The later.  The guest driver knows which resources it has created,
> > it can restore them after suspend.
> 
> Are you sure that this is viable?
> 
> How would you propose that a guest kernel could reproduce a resource,
> including pixel data upload during a resume?
> 
> The kernel would not have any of the pixel data to transfer to host.

Depends on the of resource type.  For resources which are created by
uploading pixel data (using VIRTIO_GPU_CMD_TRANSFER_TO_HOST_*) a guest
mirror exists which can be used for re-upload.

For resources filled by gl rendering ops this is indeed not the case.

> Could you explain how you anticipate the guest being able to reproduce the
> resources please?

Same you do on physical hardware?  Suspend can poweroff your PCI
devices, so there must be some standard way to handle that situation
for resources stored in gpu device memory, which is very similar to
the problem we have here.

take care,
  Gerd

Re: [XEN PATCH 07/13] xen/x86: fixed violations of MISRA C:2012 Rule 7.2

[Simone Ballarin]

Il giorno mar 20 giu 2023 alle ore 14:51 Jan Beulich  ha
scritto:

> On 20.06.2023 12:34, Simone Ballarin wrote:
> > --- a/xen/arch/x86/cpu/vpmu_intel.c
> > +++ b/xen/arch/x86/cpu/vpmu_intel.c
> > @@ -950,10 +950,10 @@ const struct arch_vpmu_ops *__init
> core2_vpmu_init(void)
> > fixed_ctrl_mask |=
> > (FIXED_CTR_CTRL_ANYTHREAD_MASK << (FIXED_CTR_CTRL_BITS * i));
> >
> > -fixed_counters_mask = ~((1ull << core2_get_bitwidth_fix_count()) -
> 1);
> > +fixed_counters_mask = ~((1Ull << core2_get_bitwidth_fix_count()) -
> 1);
>
> What's the point of this adjustment? And if at all, why keep the l-s
> lowercase?
>

In the patch for Rule 7.3 I will propose to change all 'l' with 'L'.
I will move this type of change to the new patch for 7.3.


> >  global_ctrl_mask = ~1ULL << fixed_pmc_cnt) - 1) << 32) |
> >   ((1ULL << arch_pmc_cnt) - 1));
> > -global_ovf_ctrl_mask = ~(0xC000 |
> > +global_ovf_ctrl_mask = ~(0xC000U |
>
> If such constants gain a suffix, I think that ought to be UL or ULL.
>

Yes, I agree with using 'ULL'.


> > -#define INTR_INFO_VECTOR_MASK   0xff/* 7:0 */
> > -#define INTR_INFO_INTR_TYPE_MASK0x700   /* 10:8 */
> > -#define INTR_INFO_DELIVER_CODE_MASK 0x800   /* 11 */
> > -#define INTR_INFO_NMI_UNBLOCKED_BY_IRET 0x1000  /* 12 */
> > -#define INTR_INFO_VALID_MASK0x8000  /* 31 */
> > -#define INTR_INFO_RESVD_BITS_MASK   0x7000
> > +#define INTR_INFO_VECTOR_MASK   0xffU/* 7:0 */
> > +#define INTR_INFO_INTR_TYPE_MASK0x700U   /* 10:8 */
> > +#define INTR_INFO_DELIVER_CODE_MASK 0x800U   /* 11 */
> > +#define INTR_INFO_NMI_UNBLOCKED_BY_IRET 0x1000   /* 12 */
> > +#define INTR_INFO_VALID_MASK0x8000U  /* 31 */
> > +#define INTR_INFO_RESVD_BITS_MASK   0x7000U
>
> I think it would be nice if you took the opportunity and added
> zero-padding to these constants.
>

Ok, I can do that.


> >  #define X86_SEG_AR_SEG_TYPE 0xf/* 3:0, segment type */
> > -#define X86_SEG_AR_DESC_TYPE(1u << 4)  /* 4, descriptor type */
> > +#define X86_SEG_AR_DESC_TYPE(1U << 4)  /* 4, descriptor type */
> >  #define X86_SEG_AR_DPL  0x60   /* 6:5, descriptor privilege
> level */
> > -#define X86_SEG_AR_SEG_PRESENT  (1u << 7)  /* 7, segment present */
> > -#define X86_SEG_AR_AVL  (1u << 12) /* 12, available for system
> software */
> > -#define X86_SEG_AR_CS_LM_ACTIVE (1u << 13) /* 13, long mode active (CS
> only) */
> > -#define X86_SEG_AR_DEF_OP_SIZE  (1u << 14) /* 14, default operation
> size */
> > -#define X86_SEG_AR_GRANULARITY  (1u << 15) /* 15, granularity */
> > -#define X86_SEG_AR_SEG_UNUSABLE (1u << 16) /* 16, segment unusable */
> > +#define X86_SEG_AR_SEG_PRESENT  (1U << 7)  /* 7, segment present */
> > +#define X86_SEG_AR_AVL  (1U << 12) /* 12, available for system
> software */
> > +#define X86_SEG_AR_CS_LM_ACTIVE (1U << 13) /* 13, long mode active (CS
> only) */
> > +#define X86_SEG_AR_DEF_OP_SIZE  (1U << 14) /* 14, default operation
> size */
> > +#define X86_SEG_AR_GRANULARITY  (1U << 15) /* 15, granularity */
> > +#define X86_SEG_AR_SEG_UNUSABLE (1U << 16) /* 16, segment unusable */
>
> And all of these, when it's exactly the two numbers you don't touch
> which might want to gain a U (or u) suffix?
>

Ok.


> Jan
>


-- 
Simone Ballarin, M.Sc.

Field Application Engineer, BUGSENG (https://bugseng.com
)

Re: [XEN PATCH v2] xen/include: avoid undefined behavior.

[Jan Beulich]

On 21.06.2023 09:58, Nicola Vetrini wrote:
> Redefine BUILD_BUG_ON_ZERO to fully comply with C99 avoiding
> undefined behavior 58 ("A structure or union is defined as
> containing no named members (6.7.2.1)."

Here and in the title I'm not happy about you referencing undefined
behavior. What we do here is use a well-known compiler extension (and I'm
sure you're aware we do so elsewhere, where it's actually going to remain
as is from all I can tell right now).

> --- a/xen/include/xen/lib.h
> +++ b/xen/include/xen/lib.h
> @@ -51,9 +51,10 @@
> e.g. in a structure initializer (or where-ever else comma expressions
> aren't permitted). */
>  #define BUILD_BUG_ON_ZERO(cond) \
> -sizeof(struct { _Static_assert(!(cond), "!(" #cond ")"); })
> +(sizeof(struct { char c; _Static_assert(!(cond), "!(" #cond ")"); }) - 
> 1U)

To be compatible with whatever odd ABIs new ports may have, maybe better to
AND or multiply with 0? (I also don't think a U suffix is warranted here.)

>  #else
> -#define BUILD_BUG_ON_ZERO(cond) sizeof(struct { int:-!!(cond); })
> +#define BUILD_BUG_ON_ZERO(cond) \
> +(sizeof(struct { unsigned u : (cond) ? -1 : sizeof(unsigned) * 8; }) - 
> sizeof(unsigned))

What's wrong with just giving the bitfield a name:

#define BUILD_BUG_ON_ZERO(cond) sizeof(struct { int _:-!!(cond); })

Jan

[qemu-mainline test] 181530: regressions - trouble: blocked/broken/fail/pass

[osstest service owner]

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

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 build-armhf  broken
 build-armhf   4 host-install(4)broken REGR. vs. 180691
 build-amd64   6 xen-buildfail REGR. vs. 180691
 build-amd64-xsm   6 xen-buildfail REGR. vs. 180691
 build-i3866 xen-buildfail REGR. vs. 180691
 build-i386-xsm6 xen-buildfail REGR. vs. 180691
 build-arm64   6 xen-buildfail REGR. vs. 180691
 build-arm64-xsm   6 xen-buildfail REGR. vs. 180691

Tests which did not succeed, but are not blocking:
 test-amd64-i386-xl-qemuu-debianhvm-amd64-shadow  1 build-check(1)  blocked n/a
 test-amd64-i386-xl-qemuu-debianhvm-i386-xsm  1 build-check(1)  blocked n/a
 test-amd64-i386-xl-qemuu-dmrestrict-amd64-dmrestrict 1 build-check(1) blocked 
n/a
 test-amd64-i386-xl-qemuu-ovmf-amd64  1 build-check(1)  blocked n/a
 test-amd64-i386-xl-qemuu-win7-amd64  1 build-check(1)  blocked n/a
 test-amd64-i386-xl-qemuu-ws16-amd64  1 build-check(1)  blocked n/a
 test-amd64-i386-xl-shadow 1 build-check(1)   blocked  n/a
 test-amd64-i386-xl-vhd1 build-check(1)   blocked  n/a
 test-amd64-i386-xl-xsm1 build-check(1)   blocked  n/a
 test-arm64-arm64-libvirt-raw  1 build-check(1)   blocked  n/a
 test-arm64-arm64-libvirt-xsm  1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl   1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl-credit1   1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl-credit2   1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl-thunderx  1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl-vhd   1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl-xsm   1 build-check(1)   blocked  n/a
 test-armhf-armhf-libvirt  1 build-check(1)   blocked  n/a
 test-armhf-armhf-libvirt-qcow2  1 build-check(1)   blocked  n/a
 test-armhf-armhf-libvirt-raw  1 build-check(1)   blocked  n/a
 test-armhf-armhf-xl   1 build-check(1)   blocked  n/a
 test-armhf-armhf-xl-arndale   1 build-check(1)   blocked  n/a
 test-armhf-armhf-xl-multivcpu  1 build-check(1)   blocked  n/a
 test-armhf-armhf-xl-rtds  1 build-check(1)   blocked  n/a
 test-armhf-armhf-xl-vhd   1 build-check(1)   blocked  n/a
 test-amd64-i386-qemuu-rhel6hvm-amd  1 build-check(1)   blocked n/a
 test-amd64-i386-pair  1 build-check(1)   blocked  n/a
 test-amd64-i386-libvirt-xsm   1 build-check(1)   blocked  n/a
 test-amd64-i386-libvirt-raw   1 build-check(1)   blocked  n/a
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 1 build-check(1) blocked n/a
 test-amd64-i386-libvirt-pair  1 build-check(1)   blocked  n/a
 test-amd64-i386-libvirt   1 build-check(1)   blocked  n/a
 test-amd64-i386-freebsd10-i386  1 build-check(1)   blocked  n/a
 test-amd64-i386-freebsd10-amd64  1 build-check(1)   blocked  n/a
 test-amd64-coresched-i386-xl  1 build-check(1)   blocked  n/a
 test-amd64-coresched-amd64-xl  1 build-check(1)   blocked  n/a
 test-amd64-amd64-xl-xsm   1 build-check(1)   blocked  n/a
 build-amd64-libvirt   1 build-check(1)   blocked  n/a
 test-amd64-amd64-xl-shadow1 build-check(1)   blocked  n/a
 test-amd64-amd64-xl-rtds  1 build-check(1)   blocked  n/a
 build-arm64-libvirt   1 build-check(1)   blocked  n/a
 test-amd64-amd64-xl-qemuu-ws16-amd64  1 build-check(1) blocked n/a
 test-amd64-amd64-xl-qemuu-win7-amd64  1 build-check(1) blocked n/a
 test-amd64-amd64-xl-qemuu-ovmf-amd64  1 build-check(1) blocked n/a
 build-armhf-libvirt   1 build-check(1)   blocked  n/a
 test-amd64-amd64-xl-qemuu-dmrestrict-amd64-dmrestrict 1 build-check(1) blocked 
n/a
 build-i386-libvirt1 build-check(1)   blocked  n/a
 test-amd64-amd64-xl-qemuu-debianhvm-i386-xsm  1 build-check(1) blocked n/a
 test-amd64-amd64-dom0pvh-xl-amd  1 build-check(1)   blocked  n/a
 test-amd64-amd64-xl-qemuu-debianhvm-amd64-shadow  1 build-check(1) blocked n/a
 test-amd64-amd64-dom0pvh-xl-intel  1 build-check(1)   blocked  n/a
 test-amd64-amd64-xl-qemuu-debianhvm-amd64  1 build-check(1)blocked n/a
 test-amd64-amd64-libvirt  1 build-check(1)   blocked  n/a
 test-amd64-amd64-libvirt-pair  1 build-check(1) 

Re: [XEN PATCH 09/13] xen/public: fixed violations of MISRA C:2012 Rule 7.2

[Simone Ballarin]

Hi,

Il giorno mar 20 giu 2023 alle ore 15:00 Jan Beulich  ha
scritto:

> On 20.06.2023 12:35, Simone Ballarin wrote:
> > --- a/xen/include/public/io/ring.h
> > +++ b/xen/include/public/io/ring.h
> > @@ -36,11 +36,11 @@
> >  typedef unsigned int RING_IDX;
> >
> >  /* Round a 32-bit unsigned constant down to the nearest power of two. */
> > -#define __RD2(_x)  (((_x) & 0x0002) ? 0x2  : ((_x)
> & 0x1))
> > -#define __RD4(_x)  (((_x) & 0x000c) ? __RD2((_x)>>2)<<2:
> __RD2(_x))
> > -#define __RD8(_x)  (((_x) & 0x00f0) ? __RD4((_x)>>4)<<4:
> __RD4(_x))
> > -#define __RD16(_x) (((_x) & 0xff00) ? __RD8((_x)>>8)<<8:
> __RD8(_x))
> > -#define __RD32(_x) (((_x) & 0x) ? __RD16((_x)>>16)<<16 :
> __RD16(_x))
> > +#define __RD2(_x)  (((_x) & 0x0002U) ? 0x2  : ((_x)
> & 0x1))
> > +#define __RD4(_x)  (((_x) & 0x000cU) ? __RD2((_x)>>2)<<2:
> __RD2(_x))
> > +#define __RD8(_x)  (((_x) & 0x00f0U) ? __RD4((_x)>>4)<<4:
> __RD4(_x))
> > +#define __RD16(_x) (((_x) & 0xff00U) ? __RD8((_x)>>8)<<8:
> __RD8(_x))
> > +#define __RD32(_x) (((_x) & 0xU) ? __RD16((_x)>>16)<<16 :
> __RD16(_x))
>
> While I don't mind the suffixes being added, I'm wondering how
> the tool would have spotted the single violation here. Iirc we
> don't use this header anywhere in the hypervisor.
>

In the analyzed build it is used:
aarch64-linux-gnu-gcc-12 -MMD -MP -MF common/.vm_event.o.d -DBUILD_ID
-fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes
-Wdeclaration-after-statement -Wno-unused-but-set-variable
-Wno-unused-local-typedefs -O1 -fno-omit-frame-pointer -nostdinc
-fno-builtin -fno-common -Werror -Wredundant-decls -Wno-pointer-arith -Wvla
-pipe -D__XEN__ -include ./include/xen/config.h
"-Wa,--strip-local-absolute" -g -mcpu=generic -mgeneral-regs-only
-mno-outline-atomics -I./include -I./arch/arm/include -fno-pie
-fno-stack-protector -fno-exceptions -fno-asynchronous-unwind-tables
-Wnested-externs -fprofile-arcs -ftest-coverage -c common/vm_event.c -o
common/vm_event.o


xen/common/vm_event.c:
85 FRONT_RING_INIT(&ved->front_ring,
86 (vm_event_sring_t *)ved->ring_page,
87 PAGE_SIZE);

The expansions that lead to __RD32 are:
FRONT_RING_INIT -> FRONT_RING_ATTACH -> __RING_SIZE -> __RD32

>
> Furthermore, if I recall correctly Misra also mandates single
> evaluation of macro arguments. While I don't immediately see how
> to address that without resorting to compiler extensions, I don't
> think it makes sense to address one violation here but not he
> other (the more when the code in question doesn't affect the
> hypervisor build).
>

If not clearly connected I suggest discussing one rule at a time.

>
> Jan
>


-- 
Simone Ballarin, M.Sc.

Field Application Engineer, BUGSENG (https://bugseng.com
)

Re: [XEN PATCH 06/13] xen/efi: fixed violations of MISRA C:2012 Rule 7.2

[Simone Ballarin]

Il giorno mer 21 giu 2023 alle ore 09:06 Jan Beulich  ha
scritto:

> On 20.06.2023 22:56, Stefano Stabellini wrote:
> > On Tue, 20 Jun 2023, Jan Beulich wrote:
> >> On 20.06.2023 12:34, Simone Ballarin wrote:
> >>> From: Gianluca Luparini 
> >>>
> >>> The xen sources contains violations of MISRA C:2012 Rule 7.2 whose
> headline states:
> >>> "A "u" or "U" suffix shall be applied to all integer constants that
> are represented in an unsigned type".
> >>>
> >>> I propose to use "U" as a suffix to explicitly state when an integer
> constant is represented in an unsigned type.
> >>> For homogeneity, I also added the "U" suffix in some cases that the
> tool didn't report as violations.
> >>>
> >>> Signed-off-by: Simone Ballarin 
> >>> ---
> >>>  xen/arch/x86/include/asm/x86_64/efibind.h | 10 +-
> >>
> >> This file as well as ...
> >>
> >>>  xen/common/efi/boot.c |  8 
> >>>  xen/common/efi/runtime.c  |  2 +-
> >>>  xen/include/efi/efiapi.h  | 10 +-
> >>>  xen/include/efi/efidef.h  |  2 +-
> >>>  xen/include/efi/efiprot.h | 22 +++---
> >>
> >> ... the last three here are imported from the gnu-efi package. I'm wary
> >> of touching them, and thus getting them more out of sync with their
> >> original than strictly necessary. To allow the other changes to go in
> >> no matter what, I'd like to suggest splitting the patch.
> >
> > Should we add either those files individually or the directory
> > xen/include/efi (plus xen/arch/x86/include/asm/x86_64/efibind.h) to
> > docs/misra/exclude-list.json ?
>
> Probably, and in the former case imo the entire directory.
>
> Jan
>

Ok, I will remove all the changes in the "xen/include/efi" directory and
"xen/arch/x86/include/asm/x86_64/efibind.h".
-- 
Simone Ballarin, M.Sc.

Field Application Engineer, BUGSENG (https://bugseng.com
)

Re: [PATCH] x86/vpmu: Simplify is_pmc_quirk

[Andrew Cooper]

On 21/06/2023 9:16 am, Jan Beulich wrote:
> On 20.06.2023 19:45, Andrew Cooper wrote:
>> This should be static, and there's no need for a separate (non-init, even)
>> function to perform a simple equality test.  Drop the is_ prefix which is
>> gramatically questionable, and make it __ro_after_init.
>>
>> Leave a TODO, because the behaviour is definitely wrong to be applied to ~all
>> modern Intel CPUs, and has been raised on xen-devel previously without
>> conclusion.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper 
> Reviewed-by: Jan Beulich 

Thanks.

> with one request:
>
>> @@ -967,7 +960,8 @@ const struct arch_vpmu_ops *__init core2_vpmu_init(void)
>>sizeof(uint64_t) * fixed_pmc_cnt +
>>sizeof(struct xen_pmu_cntr_pair) * arch_pmc_cnt;
>>  
>> -check_pmc_quirk();
>> +/* TODO: This is surely wrong. */
>> +pmc_quirk = current_cpu_data.x86 == 6;
> In the description you say "~all modern Intel CPUs", which suggests it might
> be correct for old enough ones. Would you mind weakening the comment to
> "This surely isn't universally correct" or some such?

I'm happy to tweak the wording as appropriate, but Kyle (who is a
regular contributor to perf in Linux) questioned that there was an issue.

There's insufficient information to figure out why it was introduced in
the first place, and IIRC he wasn't aware of any errata that manifested
in this way.

It's possible it's entirely bogus, or it may be a misunderstood errata. 
It needs looking into by someone with some copious free time.

~Andrew

Re: [PATCH 3/7] xen/arm64: head: Add missing isb in setup_fixmap()

[Michal Orzel]

On 19/06/2023 19:01, Julien Grall wrote:
> 
> 
> From: Julien Grall 
> 
> On older version of the Arm Arm (ARM DDI 0487E.a, B2-125) there were
> the following paragraph:
> 
> "DMB and DSB instructions affect reads and writes to the memory system
> generated by Load/Store instructions and data or unified cache
> maintenance instructions being executed by the PE. Instruction fetches
> or accesses caused by a hardware translation table access are not
> explicit accesses."
> 
> Newer revision (e.g. ARM DDI 0487J.a) doesn't have the second sentence
> (it might be somewhere else in the Arm Arm). But the interpretation is
> not much different.
> 
> In setup_fixmap(), we write the fixmap area and may be used soon after,
> for instance, to write to the UART. IOW, there could be hardware
> translation table access. So we need to ensure the 'dsb' has completed
> before continuing. Therefore add an 'isb'.
> 
> Fixes: 2b11c3646105 ("xen/arm64: head: Remove 1:1 mapping as soon as it is 
> not used")
> Signed-off-by: Julien Grall 
Reviewed-by: Michal Orzel 

I'm happy with the whole series but I do not see a point in flooding each patch 
with my tag
since you already got two (from Henry and Luca).

When it comes to essential isb() after dsb() in arm64 head.S, I can see that we 
are missing one in enable_mmu()
after TLB invalidation. On HW without FEAT_ETS the TLB is "guaranteed to be 
complete after the execution of
DSB by that PE, followed by a Context synchronization event", so I view isb as 
necessary there. We could also
introduce (just like for arm32) flush_xen_tlb_local macro and use it there + 
remove opencoding it.

~Michal

Re: [XEN PATCH 02/13] AMD/IOMMU: fixed violations of MISRA C:2012 Rule 7.2

[Simone Ballarin]

Il giorno mar 20 giu 2023 alle ore 22:41 Stefano Stabellini <
sstabell...@kernel.org> ha scritto:

> On Tue, 20 Jun 2023, Luca Fancellu wrote:
> > > On 20 Jun 2023, at 13:39, Jan Beulich  wrote:
> > >
> > > On 20.06.2023 12:34, Simone Ballarin wrote:
> > >> --- a/xen/drivers/passthrough/amd/iommu-defs.h
> > >> +++ b/xen/drivers/passthrough/amd/iommu-defs.h
> > >> @@ -38,49 +38,49 @@
> > >> ((uint64_t)(offset) << (12 + (PTE_PER_TABLE_SHIFT * ((level)
> - 1
> > >>
> > >> /* IOMMU Capability */
> > >> -#define PCI_CAP_ID_MASK 0x00FF
> > >> +#define PCI_CAP_ID_MASK 0x00FFU
> > >
> > > Seeing this and similar uses further down, I think there's a purely
> > > stylistic question to be resolved first (and hence I'm widening the To:
> > > list here): Do we want to allow either case of U to be used
> > > interchangeably, leaving the choice up to the patch author?
> >
> > Because you ask to the list, my personal opinion is that we might want to
> > have only capital U, so that it’s consistent across the codebase and we
> don’t
> > need to create too many rules for the developer, it stands out anyway as
> it can’t
> > be an hex digit
>
> I agree with Luca


I also agree with Luca.
-- 
Simone Ballarin, M.Sc.

Field Application Engineer, BUGSENG (https://bugseng.com
)

Re: [PATCH 08/11] sysctl: Add size to register_sysctl_init

[Jiri Slaby]

On 21. 06. 23, 11:09, Joel Granados wrote:

In order to remove the end element from the ctl_table struct arrays, we
explicitly define the size when registering the targes. We add a size
argument to the register_sysctl_init call and pass an ARRAY_SIZE for all
the callers.


Hi, I am missing here (or in 00/00) _why_ you are doing that. Is it by a 
chance those saved 9k? I hope not.


thanks,
--
js
suse labs

Re: [PATCH 3/7] xen/arm64: head: Add missing isb in setup_fixmap()

[Julien Grall]

Hi,

On 21/06/2023 10:33, Michal Orzel wrote:



On 19/06/2023 19:01, Julien Grall wrote:



From: Julien Grall 

On older version of the Arm Arm (ARM DDI 0487E.a, B2-125) there were
the following paragraph:

"DMB and DSB instructions affect reads and writes to the memory system
generated by Load/Store instructions and data or unified cache
maintenance instructions being executed by the PE. Instruction fetches
or accesses caused by a hardware translation table access are not
explicit accesses."

Newer revision (e.g. ARM DDI 0487J.a) doesn't have the second sentence
(it might be somewhere else in the Arm Arm). But the interpretation is
not much different.

In setup_fixmap(), we write the fixmap area and may be used soon after,
for instance, to write to the UART. IOW, there could be hardware
translation table access. So we need to ensure the 'dsb' has completed
before continuing. Therefore add an 'isb'.

Fixes: 2b11c3646105 ("xen/arm64: head: Remove 1:1 mapping as soon as it is not 
used")
Signed-off-by: Julien Grall 

Reviewed-by: Michal Orzel 

I'm happy with the whole series but I do not see a point in flooding each patch 
with my tag
since you already got two (from Henry and Luca).


Thanks. To clarify, shall I add it in each patch or only this one?



When it comes to essential isb() after dsb() in arm64 head.S, I can see that we 
are missing one in enable_mmu()
after TLB invalidation. On HW without FEAT_ETS the TLB is "guaranteed to be 
complete after the execution of
DSB by that PE, followed by a Context synchronization event", so I view isb as 
necessary there.


While there is no ISB directly after DSB NSH, there are one right after 
MSR. I don't think we need one before because nothing will use the TLBs 
between before the ISB.


/*
 * The state of the TLBs is unknown before turning on the MMU.
 * Flush them to avoid stale one.
 */
tlbi  alle2  /* Flush hypervisor TLBs */
dsb   nsh

/* Write Xen's PT's paddr into TTBR0_EL2 */
load_paddr x0, boot_pgtable
msr   TTBR0_EL2, x0
isb



We could also
introduce (just like for arm32) flush_xen_tlb_local macro and use it there + 
remove opencoding it.


That would be good. But I don't think this is necessary here (see above).

Cheers,

--
Julien Grall

Re: [PATCH v2 13/16] xen-blkback: Implement diskseq checks

[Roger Pau Monné]

On Tue, Jun 20, 2023 at 09:14:25PM -0400, Demi Marie Obenour wrote:
> On Mon, Jun 12, 2023 at 10:09:39AM +0200, Roger Pau Monné wrote:
> > On Fri, Jun 09, 2023 at 12:55:39PM -0400, Demi Marie Obenour wrote:
> > > On Fri, Jun 09, 2023 at 05:13:45PM +0200, Roger Pau Monné wrote:
> > > > On Thu, Jun 08, 2023 at 11:33:26AM -0400, Demi Marie Obenour wrote:
> > > > > On Thu, Jun 08, 2023 at 10:29:18AM +0200, Roger Pau Monné wrote:
> > > > > > On Wed, Jun 07, 2023 at 12:14:46PM -0400, Demi Marie Obenour wrote:
> > > > > > > On Wed, Jun 07, 2023 at 10:20:08AM +0200, Roger Pau Monné wrote:
> > > > > > Then the block script will open the device by diskseq and pass the
> > > > > > major:minor numbers to blkback.
> > > > > 
> > > > > Alternatively, the toolstack could write both the diskseq and
> > > > > major:minor numbers and be confident that it is referring to the
> > > > > correct device, no matter how long ago it got that information.
> > > > > This could be quite useful for e.g. one VM exporting a device to
> > > > > another VM by calling losetup(8) and expecting a human to make a
> > > > > decision based on various properties about the device.  In this
> > > > > case there is no upper bound on the race window.
> > > > 
> > > > Instead of playing with xenstore nodes, it might be better to simply
> > > > have blkback export on sysfs the diskseq of the opened device, and let
> > > > the block script check whether that's correct or not.  That implies
> > > > less code in the kernel side, and doesn't pollute xenstore.
> > > 
> > > This would require that blkback delay exposing the device to the
> > > frontend until the block script has checked that the diskseq is correct.
> > 
> > This depends on your toolstack implementation.  libxl won't start the
> > domain until block scripts have finished execution, and hence the
> > block script waiting for the sysfs node to appear and check it against
> > the expected value would be enough.
> 
> True, but we cannot assume that everyone is using libxl.

Right, for the udev case this won't be good, since the domain could be
already running, and hence could potentially attach to the backend
before the hotplug script realized the opened device is wrong.
Likewise for hot add disks.

> > > Much simpler for the block script to provide the diskseq in xenstore.
> > > If you want to avoid an extra xenstore node, I can make the diskseq part
> > > of the physical-device node.
> > 
> > I'm thinking that we might want to introduce a "physical-device-uuid"
> > node and use that to provide the diskseq to the backened.  Toolstacks
> > (or block scripts) would need to be sure the "physical-device-uuid"
> > node is populated before setting "physical-device", as writes to
> > that node would still trigger blkback watch.  I think using two
> > distinct watches would just make the logic in blkback too
> > complicated.
> > 
> > My preference would be for the kernel to have a function for opening a
> > device identified by a diskseq (as fetched from
> > "physical-device-uuid"), so that we don't have to open using
> > major:minor and then check the diskseq.
> 
> In theory I agree, but in practice it would be a significantly more
> complex patch and given that it does not impact the uAPI I would prefer
> the less-invasive option.

>From a blkback point of view I don't see that option as more invasive,
it's actually the other way around IMO.  On blkback we would use
blkdev_get_by_diskseq() (or equivalent) instead of
blkdev_get_by_dev(), so it would result in an overall simpler
change (because the check against diskseq won't be needed anymore).

> Is there anything more that needs to be done
> here, other than replacing the "diskseq" name?

I think we also spoke about using sscanf to parse the option.

The patch to Xen blkif.h needs to be accepted before the Linux side
can progress.


> I prefer
> "physical-device-luid" because the ID is only valid in one particular
> VM.

"physical-device-uid" then maybe?

Thanks, Roger.

Re: [PATCH 3/7] xen/arm64: head: Add missing isb in setup_fixmap()

[Michal Orzel]

On 21/06/2023 12:02, Julien Grall wrote:
> 
> 
> Hi,
> 
> On 21/06/2023 10:33, Michal Orzel wrote:
>>
>>
>> On 19/06/2023 19:01, Julien Grall wrote:
>>>
>>>
>>> From: Julien Grall 
>>>
>>> On older version of the Arm Arm (ARM DDI 0487E.a, B2-125) there were
>>> the following paragraph:
>>>
>>> "DMB and DSB instructions affect reads and writes to the memory system
>>> generated by Load/Store instructions and data or unified cache
>>> maintenance instructions being executed by the PE. Instruction fetches
>>> or accesses caused by a hardware translation table access are not
>>> explicit accesses."
>>>
>>> Newer revision (e.g. ARM DDI 0487J.a) doesn't have the second sentence
>>> (it might be somewhere else in the Arm Arm). But the interpretation is
>>> not much different.
>>>
>>> In setup_fixmap(), we write the fixmap area and may be used soon after,
>>> for instance, to write to the UART. IOW, there could be hardware
>>> translation table access. So we need to ensure the 'dsb' has completed
>>> before continuing. Therefore add an 'isb'.
>>>
>>> Fixes: 2b11c3646105 ("xen/arm64: head: Remove 1:1 mapping as soon as it is 
>>> not used")
>>> Signed-off-by: Julien Grall 
>> Reviewed-by: Michal Orzel 
>>
>> I'm happy with the whole series but I do not see a point in flooding each 
>> patch with my tag
>> since you already got two (from Henry and Luca).
> 
> Thanks. To clarify, shall I add it in each patch or only this one?
Whatever you prefer. If you care about my tag and want to have more than two, 
feel free to add it to
all the patches.

> 
>>
>> When it comes to essential isb() after dsb() in arm64 head.S, I can see that 
>> we are missing one in enable_mmu()
>> after TLB invalidation. On HW without FEAT_ETS the TLB is "guaranteed to be 
>> complete after the execution of
>> DSB by that PE, followed by a Context synchronization event", so I view isb 
>> as necessary there.
> 
> While there is no ISB directly after DSB NSH, there are one right after
> MSR. I don't think we need one before because nothing will use the TLBs
> between before the ISB.
> 
>  /*
>   * The state of the TLBs is unknown before turning on the MMU.
>   * Flush them to avoid stale one.
>   */
>  tlbi  alle2  /* Flush hypervisor TLBs */
>  dsb   nsh
> 
>  /* Write Xen's PT's paddr into TTBR0_EL2 */
>  load_paddr x0, boot_pgtable
>  msr   TTBR0_EL2, x0
>  isb
> 
Although having isb after dsb would be clearer to the user and consistent with 
other TBL invalidations,
the one after msr can do the job for now, I agree.

~Michal

[OSSTEST PATCH] Use updated mirror for buster armhf, and update debian-installer

[Anthony PERARD]

This reverts commit b838a9daeb3b ("production-config: Use a snapshot
for buster armhf")

Installation now fails with "Invalid Release signature", while
downloading the "Release" file of a repo.

But, using the main mirror for armhf boxes seems to work fine now.

To use the live mirror, we need to update the debian-installer as
there's a newer version of the kernel. Otherwise, installation fails
with the installer not able to find the disk.

Signed-off-by: Anthony PERARD 
---

Notes:
I've tested this with just the *armhf* jobs, which works. So I'm going
to push that to osstest right away and let it do a full flight test.

 production-config | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/production-config b/production-config
index 78ad768e..2c44805c 100644
--- a/production-config
+++ b/production-config
@@ -91,9 +91,7 @@ TftpNetbootGroup osstest
 TftpDiVersion_wheezy 2016-06-08
 TftpDiVersion_jessie 2018-06-26
 TftpDiVersion_stretch 2020-09-24
-TftpDiVersion_buster 2022-03-28
-
-DebianMirror_buster_armhf 
http://snapshot.debian.org/archive/debian/20210124T203726Z/
+TftpDiVersion_buster 2023-06-20
 
 DebianSnapshotBackports_jessie 
http://snapshot.debian.org/archive/debian/20190206T211314Z/
 
-- 
Anthony PERARD

Re: [XEN PATCH v2] docs/misra: document the C dialect and translation toolchain assumptions.

[Jan Beulich]

On 20.06.2023 14:10, Roberto Bagnara wrote:
> +   * - static function is used in an inline function with external linkage
> + - ARM64, X86_64
> + - Non-documented GCC extension. An inline function with external linkage
> +   can be inlined everywhere. If that calls a static functions, which is
> +   not available everywhere, it is a constraint violation according to
> +   C99 6.7.4p3: "An inline definition of a function with external linkage
> +   shall not contain a definition of a modifiable object with static
> +   storage duration, and shall not contain a reference to an identifier
> +   with internal linkage."  A standard-compliant C compiler ought
> +   to diagnose all constraint violations: when it does not, as is the
> +   case for GCC, the behavior is implicitly undefined.

With _spin_lock_cb() taken care of, do we have any left? Or else can this
be dropped?

Jan

Re: [PATCH 08/11] sysctl: Add size to register_sysctl_init

[Greg Kroah-Hartman]

On Wed, Jun 21, 2023 at 11:09:57AM +0200, Joel Granados wrote:
>  static int __init random_sysctls_init(void)
>  {
> - register_sysctl_init("kernel/random", random_table);
> + register_sysctl_init("kernel/random", random_table,
> +  ARRAY_SIZE(random_table));

As mentioned before, why not just do:

#define register_sysctl_init(string, table) \
__register_sysctl_init(string, table, ARRAY_SIZE(table);

or something like that?

That way no callers need to change AND you prevent the size from ever
being incorrect?

thanks,

greg k-h

[ovmf test] 181531: all pass - PUSHED

[osstest service owner]

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

Perfect :-)
All tests in this flight passed as required
version targeted for testing:
 ovmf 4a0642ad27bfb566835ae86aedae0e18f9735cc2
baseline version:
 ovmf ea55bd8f66eeca5f4e80c3679bcf1b1007286b8a

Last test of basis   181438  2023-06-15 05:42:42 Z6 days
Testing same since   181531  2023-06-21 03:42:36 Z0 days1 attempts


People who touched revisions under test:
  Jian J Wang 

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
   ea55bd8f66..4a0642ad27  4a0642ad27bfb566835ae86aedae0e18f9735cc2 -> 
xen-tested-master

Re: [PATCH v1 1/1] Q35 Support

[David Hildenbrand]

On 20.06.23 19:24, Joel Upham wrote:

Inexpressive patch subject and non-existant patch desciption. I have no 
clue what this is supposed to do, except that it involes q35 and xen ()I 
guess ?.



---
  hw/acpi/ich9.c|   22 +-
  hw/acpi/pcihp.c   |6 +-
  hw/core/machine.c |   19 +
  hw/i386/pc_piix.c |3 +-
  hw/i386/pc_q35.c  |   39 +-
  hw/i386/xen/xen-hvm.c |7 +-
  hw/i386/xen/xen_platform.c|   19 +-
  hw/isa/lpc_ich9.c |   53 +-
  hw/isa/piix3.c|2 +-
  hw/pci-host/q35.c |   28 +-
  hw/pci/pci.c  |   17 +
  hw/xen/xen-host-pci-device.c  |  106 +++-
  hw/xen/xen-host-pci-device.h  |6 +-
  hw/xen/xen_pt.c   |   49 +-
  hw/xen/xen_pt.h   |   19 +-
  hw/xen/xen_pt_config_init.c   | 1103 ++---
  include/hw/acpi/ich9.h|1 +
  include/hw/acpi/pcihp.h   |2 +
  include/hw/boards.h   |1 +
  include/hw/i386/pc.h  |3 +
  include/hw/pci-host/q35.h |4 +-
  include/hw/pci/pci.h  |3 +
  include/hw/southbridge/ich9.h |1 +
  include/hw/xen/xen.h  |4 +-
  qemu-options.hx   |1 +
  softmmu/datadir.c |1 -
  softmmu/qdev-monitor.c|3 +-
  stubs/xen-hw-stub.c   |4 +-
  28 files changed, 1395 insertions(+), 131 deletions(-)



Usually people refrain from reviewing such massive patches. Most 
probably this can be broken up into reviewable pieces.


Was this supposed to be an RFC?

--
Cheers,

David / dhildenb

Re: [QEMU PATCH 1/1] virtgpu: do not destroy resources when guest suspend

[Robert Beckett]

On 21/06/2023 09:39, Gerd Hoffmann wrote:

On Tue, Jun 20, 2023 at 01:26:15PM +0100, Robert Beckett wrote:

On 20/06/2023 10:41, Gerd Hoffmann wrote:

Hi,


The guest driver should be able to restore resources after resume.

Thank you for your suggestion!
As far as I know, resources are created on host side and guest has no backup, 
if resources are destroyed, guest can't restore them.
Or do you mean guest driver need to send commands to re-create resources after 
resume?

The later.  The guest driver knows which resources it has created,
it can restore them after suspend.

Are you sure that this is viable?

How would you propose that a guest kernel could reproduce a resource,
including pixel data upload during a resume?

The kernel would not have any of the pixel data to transfer to host.

Depends on the of resource type.  For resources which are created by
uploading pixel data (using VIRTIO_GPU_CMD_TRANSFER_TO_HOST_*) a guest
mirror exists which can be used for re-upload.


unfortunately this is not always the case.

https://gitlab.freedesktop.org/mesa/mesa/-/blob/main/src/gallium/drivers/virgl/virgl_resource.c#L668

Often mesa will decide that it won't need to access a resource again 
after initial upload (textures etc). In this case, if it is able to copy 
back from host if needed, it will not maintain the guest shadow copy. 
Instead it will create a single page proxy object. The transfer to host 
will then over fill it to the correct size.


I think this was a fairly huge optimization for them.



For resources filled by gl rendering ops this is indeed not the case.


Could you explain how you anticipate the guest being able to reproduce the
resources please?

Same you do on physical hardware?  Suspend can poweroff your PCI
devices, so there must be some standard way to handle that situation
for resources stored in gpu device memory, which is very similar to
the problem we have here.


In traditional PCI gfx card setups, TTM is used as the memory manager in 
the kernel. This is used to migrate the buffers back from VRAM to system 
pages during a suspend.


This would be suitable for use to track host blob buffers that get 
mapped to guest via the PCI BAR, though would be a significant 
re-architecting of virtio gpu driver.


It would not help with the previously mentioned proxied resources. 
Though in theory the driver could read the resources back from host to 
guest pages during suspend, this would then be potentially complicated 
by suspend time alloc failures etc.



As virtio drivers are by design paravirt drivers ,I think it is 
reasonable to accept some knowledge with and cooperation with the host 
to manage suspend/resume.


It seems to me like a lot of effort and long term maintenance to add 
support for transparent suspend/resume that would otherwise be unneeded.


Perhaps others have alternative designs for this?



take care,
   Gerd

Re: [PATCH 08/11] sysctl: Add size to register_sysctl_init

[Petr Mladek]

On Wed 2023-06-21 11:09:57, Joel Granados wrote:
> In order to remove the end element from the ctl_table struct arrays, we
> explicitly define the size when registering the targes. We add a size
> argument to the register_sysctl_init call and pass an ARRAY_SIZE for all
> the callers.

This does not explain the motivatin why the end element is removed.

I agree with Jiri that saving 9k is a questionable gain. According to
the cover letter it saved 0,00%. It is because it saved 9k with allyes
config which produces huge kernel. IMHO, the 9k might be interesting
only for a tiny kernel. But I guess that it would safe much less
bytes there.

And the code with the added ARRAY_SIZE() parameter looks worse than before.

> diff --git a/kernel/printk/sysctl.c b/kernel/printk/sysctl.c
> index c228343eeb97..28f37b86414e 100644
> --- a/kernel/printk/sysctl.c
> +++ b/kernel/printk/sysctl.c
> @@ -81,5 +81,6 @@ static struct ctl_table printk_sysctls[] = {
>  
>  void __init printk_sysctl_init(void)
>  {
> - register_sysctl_init("kernel", printk_sysctls);
> + register_sysctl_init("kernel", printk_sysctls,
> +  ARRAY_SIZE(printk_sysctls));
>  }

Is register_sysctl_init() still ready to handle the last empty element,
please? I am not in Cc on the related patches.

Best Regards,
Petr

[xen-unstable-smoke test] 181532: trouble: blocked/broken/pass

[osstest service owner]

flight 181532 xen-unstable-smoke real [real]
http://logs.test-lab.xenproject.org/osstest/logs/181532/

Failures and problems with tests :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 build-armhf  broken
 build-armhf   4 host-install(4)broken REGR. vs. 181476

Tests which did not succeed, but are not blocking:
 test-armhf-armhf-xl   1 build-check(1)   blocked  n/a
 test-amd64-amd64-libvirt 15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  16 saverestore-support-checkfail   never pass

version targeted for testing:
 xen  a6120e3fc8fd90f3ed638c6f7a573bfb534af154
baseline version:
 xen  7a25a1501ca941c3e01b0c4e624ace05417f1587

Last test of basis   181476  2023-06-17 04:00:28 Z4 days
Failing since181504  2023-06-19 11:00:26 Z2 days6 attempts
Testing same since   181527  2023-06-20 21:00:31 Z0 days2 attempts


People who touched revisions under test:
  Alistair Francis 
  Andrew Cooper 
  Anthony PERARD 
  Henry Wang 
  Jan Beulich 
  Julien Grall 
  Michal Orzel 
  Oleksii Kurochko 
  Roger Pau Monné 
  Stefano Stabellini 

jobs:
 build-arm64-xsm  pass
 build-amd64  pass
 build-armhf  broken  
 build-amd64-libvirt  pass
 test-armhf-armhf-xl  blocked 
 test-arm64-arm64-xl-xsm  pass
 test-amd64-amd64-xl-qemuu-debianhvm-amd64pass
 test-amd64-amd64-libvirt pass



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

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

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

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

broken-job build-armhf broken
broken-step build-armhf host-install(4)

Not pushing.


commit a6120e3fc8fd90f3ed638c6f7a573bfb534af154
Author: Andrew Cooper 
Date:   Wed May 10 20:21:12 2023 +0100

x86: Use printk_once() instead of opencoding it

Technically our helper post-dates all of these examples, but it's good 
cleanup
nevertheless.  None of these examples should be using fully locked
test_and_set_bool() in the first place.

No functional change.

Signed-off-by: Andrew Cooper 
Reviewed-by: Jan Beulich 
Reviewed-by: Kevin Tian 

commit e5ba5165cae654f4bb5023b74008f57b5649979d
Author: Andrew Cooper 
Date:   Tue Jun 13 17:06:47 2023 +0100

xen/evtchn: Purge ERROR_EXIT{,_DOM}()

These interfere with code legibility by hiding control flow.  Expand and 
drop
them.

 * Rearrange the order of actions to write into rc, then render rc in the
   gdprintk().
 * Drop redundant "rc = rc" assignments
 * Switch to using %pd for rendering domains

As a side effect, this fixes several violations of MISRA rule 2.1 (dead 
code -
the while() following a goto).

No functional change.

Signed-off-by: Andrew Cooper 
Reviewed-by: Julien Grall 

commit 42473bae2394b6602372ab8b83a9ca294b1e40f4
Author: Michal Orzel 
Date:   Wed Jun 7 11:27:27 2023 +0200

xen/arm: pl011: Add SBSA UART device-tree support

We already have all the bits necessary in PL011 driver to support SBSA
UART thanks to commit 032ea8c736d10f02672863c6e369338f948f7ed8 that
enabled it for ACPI. Plumb in the remaining part for device-tree boot:
 - add arm,sbsa-uart compatible to pl011_dt_match (no need for a separate
   struct and DT_DEVICE_START as SBSA is a subset of PL011),
 - from pl011_dt_uart_init(), check for SBSA UART compatible to determine
   the UART type in use.

Signed-off-by: Michal Orzel 
Reviewed-by: Henry Wang 
Reviewed-by: Stefano Stabellini 
Tested-by: Henry Wang 

commit 47e3941d2eee347e9c41b311d19048c41e1b33e3
Author: Michal Orzel 
Date:   Wed Jun 7 11:27:26 2023 +0200

xen/arm: pl011: Use correct accessors

At the moment, we use 32-bit only accessors (i.e. readl/writel) to match
the SBSA v2.x requirement. This should not be the default case for normal
PL011 where accesses shall be 8/16-bit (max registe

Re: [QEMU PATCH 1/1] virtgpu: do not destroy resources when guest suspend

[Gerd Hoffmann]

  Hi,

> As virtio drivers are by design paravirt drivers ,I think it is reasonable
> to accept some knowledge with and cooperation with the host to manage
> suspend/resume.

Fair point.

In any case this needs a feature flag, so guest and host can negotiate
whenever this is supported or not.

virtio spec needs an update for that, describing the exact behavior.

take care,
  Gerd

Re: [PATCH v7 11/12] xen/arm: translate virtual PCI bus topology for guests

[Jan Beulich]

On 13.06.2023 12:32, Volodymyr Babchuk wrote:
> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -180,6 +180,44 @@ static void vpci_remove_virtual_device(const struct 
> pci_dev *pdev)
>  write_unlock(&pdev->domain->vpci_rwlock);
>  }
>  
> +/*
> + * Find the physical device which is mapped to the virtual device
> + * and translate virtual SBDF to the physical one.
> + */
> +bool vpci_translate_virtual_device(struct domain *d, pci_sbdf_t *sbdf)
> +{
> +struct pci_dev *pdev;
> +
> +ASSERT(!is_hardware_domain(d));
> +
> +read_lock(&d->vpci_rwlock);
> +pcidevs_lock();
> +for_each_pdev( d, pdev )
> +{
> +bool found;
> +
> +if ( !pdev->vpci )
> +continue;
> +
> +spin_lock(&pdev->vpci->lock);

Following the description of patch 1, why does this lock need acquiring here?
The r/w lock acquired above should already guard against modification.

> +found = pdev->vpci && (pdev->vpci->guest_sbdf.sbdf == sbdf->sbdf);

The LHS of the && is pointless here - when acquiring the lock you have
already de-referenced pdev->vpci.

> +spin_unlock(&pdev->vpci->lock);
> +
> +if ( found )
> +{
> +/* Replace guest SBDF with the physical one. */
> +*sbdf = pdev->sbdf;
> +pcidevs_unlock();
> +read_unlock(&d->vpci_rwlock);
> +return true;

What use is the result to the caller with all locks dropped, and hence
state possibly having changed before the caller gets a chance to look
at the result? If there are reason why this is okay, you want to (a)
spell out those reasons in the description and (b) document this
restriction in a comment, to warn people who may want to (re)use this
function. But really I think the caller needs to hold a suitable lock
until after the result from here was consumed.

Jan

Re: [PATCH v7 10/12] vpci: add initial support for virtual PCI bus topology

[Jan Beulich]

On 13.06.2023 12:32, Volodymyr Babchuk wrote:
> @@ -121,6 +124,62 @@ int vpci_add_handlers(struct pci_dev *pdev)
>  }
>  
>  #ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
> +static int add_virtual_device(struct pci_dev *pdev)
> +{
> +struct domain *d = pdev->domain;
> +pci_sbdf_t sbdf = { 0 };
> +unsigned long new_dev_number;
> +
> +if ( is_hardware_domain(d) )
> +return 0;
> +
> +ASSERT(pcidevs_locked());
> +
> +/*
> + * Each PCI bus supports 32 devices/slots at max or up to 256 when
> + * there are multi-function ones which are not yet supported.
> + */
> +if ( pdev->info.is_extfn )
> +{
> +gdprintk(XENLOG_ERR, "%pp: only function 0 passthrough supported\n",
> + &pdev->sbdf);
> +return -EOPNOTSUPP;
> +}
> +
> +new_dev_number = find_first_zero_bit(d->vpci_dev_assigned_map,
> + VPCI_MAX_VIRT_DEV);
> +if ( new_dev_number >= VPCI_MAX_VIRT_DEV )
> +return -ENOSPC;
> +
> +__set_bit(new_dev_number, &d->vpci_dev_assigned_map);

Since the find-and-set can't easily be atomic, the lock used here (
asserted to be held above) needs to be the same as ...

> +/*
> + * Both segment and bus number are 0:
> + *  - we emulate a single host bridge for the guest, e.g. segment 0
> + *  - with bus 0 the virtual devices are seen as embedded
> + *endpoints behind the root complex
> + *
> + * TODO: add support for multi-function devices.
> + */
> +sbdf.devfn = PCI_DEVFN(new_dev_number, 0);
> +pdev->vpci->guest_sbdf = sbdf;
> +
> +return 0;
> +
> +}
> +
> +static void vpci_remove_virtual_device(const struct pci_dev *pdev)
> +{
> +write_lock(&pdev->domain->vpci_rwlock);
> +if ( pdev->vpci )
> +{
> +__clear_bit(pdev->vpci->guest_sbdf.dev,
> +&pdev->domain->vpci_dev_assigned_map);
> +pdev->vpci->guest_sbdf.sbdf = ~0;
> +}
> +write_unlock(&pdev->domain->vpci_rwlock);

... the one used here.

Jan

[xen-unstable test] 181528: trouble: blocked/broken/fail/pass

[osstest service owner]

flight 181528 xen-unstable real [real]
http://logs.test-lab.xenproject.org/osstest/logs/181528/

Failures and problems with tests :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 build-armhf  broken
 build-armhf   4 host-install(4)broken REGR. vs. 181498

Tests which did not succeed, but are not blocking:
 build-armhf-libvirt   1 build-check(1)   blocked  n/a
 test-armhf-armhf-examine  1 build-check(1)   blocked  n/a
 test-armhf-armhf-libvirt  1 build-check(1)   blocked  n/a
 test-armhf-armhf-libvirt-qcow2  1 build-check(1)   blocked  n/a
 test-armhf-armhf-libvirt-raw  1 build-check(1)   blocked  n/a
 test-armhf-armhf-xl   1 build-check(1)   blocked  n/a
 test-armhf-armhf-xl-arndale   1 build-check(1)   blocked  n/a
 test-armhf-armhf-xl-credit1   1 build-check(1)   blocked  n/a
 test-armhf-armhf-xl-credit2   1 build-check(1)   blocked  n/a
 test-armhf-armhf-xl-multivcpu  1 build-check(1)   blocked  n/a
 test-armhf-armhf-xl-rtds  1 build-check(1)   blocked  n/a
 test-armhf-armhf-xl-vhd   1 build-check(1)   blocked  n/a
 test-amd64-amd64-libvirt-vhd 19 guest-start/debian.repeatfail  like 181498
 test-amd64-amd64-xl-qemuu-win7-amd64 12 windows-install   fail like 181498
 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 181513
 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 181513
 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 181513
 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 181513
 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 181513
 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 181513
 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 181513
 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 181513
 test-amd64-i386-libvirt-xsm  15 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-amd64-i386-xl-pvshim14 guest-start  fail   never pass
 test-amd64-amd64-libvirt 15 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  16 saverestore-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-arm64-arm64-xl  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  16 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail never pass
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail never pass
 test-amd64-i386-libvirt-raw  14 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-vhd 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

version targeted for testing:
 xen  7a25a1501ca941c3e01b0c4e624ace05417f1587
baseline version:
 xen  7a25a1501ca941c3e01b0c4e624ace05417f1587

Last test of basis   181528  2023-06-21 01:53:38 Z0 days
Testing same since  (not found) 0 attempts

jobs:
 build-amd64-xsm  pass
 build-arm64-xsm  pass
 build-i386-xsm   pass
 build-amd64-xtf  pass
 build-amd64  pass
 build-arm64  pass
 build-armhf  broken  
 build-i386

Re: [XEN PATCH v2] docs/misra: document the C dialect and translation toolchain assumptions.

[Roberto Bagnara]

On 20/06/23 16:52, Jan Beulich wrote:

On 20.06.2023 14:10, Roberto Bagnara wrote:

+   * - Non-standard tokens
+ - ARM64, X86_64
+ - _Static_assert:
+  see Section "2.1 C Language" of GCC_MANUAL.
+   asm, __asm__:
+  see Sections "6.48 Alternate Keywords" and "6.47 How to Use Inline 
Assembly Language in C Code" of GCC_MANUAL.
+   __volatile__:
+  see Sections "6.48 Alternate Keywords" and "6.47.2.1 Volatile" of 
GCC_MANUAL.
+   __const__, __inline__, __inline:
+  see Section "6.48 Alternate Keywords" of GCC_MANUAL.
+   typeof, __typeof__:
+  see Section "6.7 Referring to a Type with typeof" of GCC_MANUAL.
+   __alignof__, __alignof:
+  see Sections "6.48 Alternate Keywords" and "6.44 Determining the 
Alignment of Functions, Types or Variables" of GCC_MANUAL.
+   __attribute__:
+  see Section "6.39 Attribute Syntax" of GCC_MANUAL.
+   __builtin_types_compatible_p:
+  see Section "6.59 Other Built-in Functions Provided by GCC" of 
GCC_MANUAL.
+   __builtin_va_arg:
+  non-documented GCC extension.
+   __builtin_offsetof:
+  see Section "6.53 Support for offsetof" of GCC_MANUAL.
+   __signed__:
+  non-documented GCC extension.


I'd like to note that there is a patch [1] pending which removes all uses
of this extension. Hopefully in Prague we can settle on how to progress
this ...

Jan

[1] https://lists.xen.org/archives/html/xen-devel/2023-02/msg00445.html


Ok, removed from the document and from the tool configuration.

[PATCH 0/2] Fix Linux dom0 boot on a QEMU/KVM virtual machine

[Petr Pavlu]

Fix two problems that prevent booting Linux dom0 on a QEMU/KVM virtual
machine, which is sometimes useful for testing purposes.

Petr Pavlu (2):
  xen/virtio: Fix NULL deref when a bridge of PCI root bus has no parent
  xen/virtio: Avoid use of the dom0 backend in dom0

 drivers/xen/grant-dma-ops.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

-- 
2.35.3

[PATCH 2/2] xen/virtio: Avoid use of the dom0 backend in dom0

[Petr Pavlu]

When attempting to run Xen on a QEMU/KVM virtual machine with virtio
devices (all x86_64), dom0 tries to establish a grant for itself which
eventually results in a hang during the boot.

The backtrace looks as follows, the while loop in __send_control_msg()
makes no progress:

  #0  virtqueue_get_buf_ctx (_vq=_vq@entry=0x8880074a8400, 
len=len@entry=0xc9413c94, ctx=ctx@entry=0x0 ) at 
../drivers/virtio/virtio_ring.c:2326
  #1  0x817086b7 in virtqueue_get_buf 
(_vq=_vq@entry=0x8880074a8400, len=len@entry=0xc9413c94) at 
../drivers/virtio/virtio_ring.c:2333
  #2  0x8175f6b2 in __send_control_msg (portdev=, 
port_id=0x, event=0x0, value=0x1) at 
../drivers/char/virtio_console.c:562
  #3  0x8175f6ee in __send_control_msg (portdev=, 
port_id=, event=, value=) at 
../drivers/char/virtio_console.c:569
  #4  0x817618b1 in virtcons_probe (vdev=0x88800585e800) at 
../drivers/char/virtio_console.c:2098
  #5  0x81707117 in virtio_dev_probe (_d=0x88800585e810) at 
../drivers/virtio/virtio.c:305
  #6  0x8198e348 in call_driver_probe (drv=0x82be40c0 
, drv=0x82be40c0 , 
dev=0x88800585e810) at ../drivers/base/dd.c:579
  #7  really_probe (dev=dev@entry=0x88800585e810, 
drv=drv@entry=0x82be40c0 ) at ../drivers/base/dd.c:658
  #8  0x8198e58f in __driver_probe_device 
(drv=drv@entry=0x82be40c0 , 
dev=dev@entry=0x88800585e810) at ../drivers/base/dd.c:800
  #9  0x8198e65a in driver_probe_device 
(drv=drv@entry=0x82be40c0 , 
dev=dev@entry=0x88800585e810) at ../drivers/base/dd.c:830
  #10 0x8198e832 in __driver_attach (dev=0x88800585e810, 
data=0x82be40c0 ) at ../drivers/base/dd.c:1216
  #11 0x8198bfb2 in bus_for_each_dev (bus=, 
start=start@entry=0x0 , data=data@entry=0x82be40c0 
,
  fn=fn@entry=0x8198e7b0 <__driver_attach>) at 
../drivers/base/bus.c:368
  #12 0x8198db65 in driver_attach (drv=drv@entry=0x82be40c0 
) at ../drivers/base/dd.c:1233
  #13 0x8198d207 in bus_add_driver (drv=drv@entry=0x82be40c0 
) at ../drivers/base/bus.c:673
  #14 0x8198f550 in driver_register (drv=drv@entry=0x82be40c0 
) at ../drivers/base/driver.c:246
  #15 0x81706b47 in register_virtio_driver 
(driver=driver@entry=0x82be40c0 ) at 
../drivers/virtio/virtio.c:357
  #16 0x832cd34b in virtio_console_init () at 
../drivers/char/virtio_console.c:2258
  #17 0x8100105c in do_one_initcall (fn=0x832cd2e0 
) at ../init/main.c:1246
  #18 0x83277293 in do_initcall_level (command_line=0x888003e2f900 
"root", level=0x6) at ../init/main.c:1319
  #19 do_initcalls () at ../init/main.c:1335
  #20 do_basic_setup () at ../init/main.c:1354
  #21 kernel_init_freeable () at ../init/main.c:1571
  #22 0x81f64be1 in kernel_init (unused=) at 
../init/main.c:1462
  #23 0x81001f49 in ret_from_fork () at ../arch/x86/entry/entry_64.S:308
  #24 0x in ?? ()

Fix the problem by preventing xen_grant_init_backend_domid() from
setting dom0 as a backend when running in dom0.

Fixes: 035e3a4321f7 ("xen/virtio: Optimize the setup of "xen-grant-dma" 
devices")
Signed-off-by: Petr Pavlu 
---
 drivers/xen/grant-dma-ops.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
index 76f6f26265a3..29ed27ac450e 100644
--- a/drivers/xen/grant-dma-ops.c
+++ b/drivers/xen/grant-dma-ops.c
@@ -362,7 +362,9 @@ static int xen_grant_init_backend_domid(struct device *dev,
if (np) {
ret = xen_dt_grant_init_backend_domid(dev, np, backend_domid);
of_node_put(np);
-   } else if (IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT) || 
xen_pv_domain()) {
+   } else if ((IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT) ||
+   xen_pv_domain()) &&
+  !xen_initial_domain()) {
dev_info(dev, "Using dom0 as backend\n");
*backend_domid = 0;
ret = 0;
-- 
2.35.3

[PATCH 1/2] xen/virtio: Fix NULL deref when a bridge of PCI root bus has no parent

[Petr Pavlu]

When attempting to run Xen on a QEMU/KVM virtual machine with virtio
devices (all x86_64), function xen_dt_get_node() crashes on accessing
bus->bridge->parent->of_node because a bridge of the PCI root bus has no
parent set:

[1.694192][T1] BUG: kernel NULL pointer dereference, address: 
0288
[1.695688][T1] #PF: supervisor read access in kernel mode
[1.696297][T1] #PF: error_code(0x) - not-present page
[1.696297][T1] PGD 0 P4D 0
[1.696297][T1] Oops:  [#1] PREEMPT SMP NOPTI
[1.696297][T1] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 
6.3.7-1-default #1 openSUSE Tumbleweed a577eae57964bb7e83477b5a5645a1781df990f0
[1.696297][T1] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 
rel-1.15.0-0-g2dd4b9b-rebuilt.opensuse.org 04/01/2014
[1.696297][T1] RIP: e030:xen_virtio_restricted_mem_acc+0xd9/0x1c0
[1.696297][T1] Code: 45 0c 83 e8 c9 a3 ea ff 31 c0 eb d7 48 8b 87 40 ff 
ff ff 48 89 c2 48 8b 40 10 48 85 c0 75 f4 48 8b 82 10 01 00 00 48 8b 40 40 <48> 
83 b8 88 02 00 00 00 0f 84 45 ff ff ff 66 90 31 c0 eb a5 48 89
[1.696297][T1] RSP: e02b:c90040013cc8 EFLAGS: 00010246
[1.696297][T1] RAX:  RBX: 888006c75000 RCX: 
0029
[1.696297][T1] RDX: 888005ed1000 RSI: c900400f100c RDI: 
888005ee30d0
[1.696297][T1] RBP: 888006c75010 R08: 0001 R09: 
00033006
[1.696297][T1] R10: 888005850028 R11: 0002 R12: 
830439a0
[1.696297][T1] R13:  R14: 888005657900 R15: 
888006e3e1e8
[1.696297][T1] FS:  () GS:88804a00() 
knlGS:
[1.696297][T1] CS:  e030 DS:  ES:  CR0: 80050033
[1.696297][T1] CR2: 0288 CR3: 02e36000 CR4: 
00050660
[1.696297][T1] Call Trace:
[1.696297][T1]  
[1.696297][T1]  virtio_features_ok+0x1b/0xd0
[1.696297][T1]  virtio_dev_probe+0x19c/0x270
[1.696297][T1]  really_probe+0x19b/0x3e0
[1.696297][T1]  __driver_probe_device+0x78/0x160
[1.696297][T1]  driver_probe_device+0x1f/0x90
[1.696297][T1]  __driver_attach+0xd2/0x1c0
[1.696297][T1]  bus_for_each_dev+0x74/0xc0
[1.696297][T1]  bus_add_driver+0x116/0x220
[1.696297][T1]  driver_register+0x59/0x100
[1.696297][T1]  virtio_console_init+0x7f/0x110
[1.696297][T1]  do_one_initcall+0x47/0x220
[1.696297][T1]  kernel_init_freeable+0x328/0x480
[1.696297][T1]  kernel_init+0x1a/0x1c0
[1.696297][T1]  ret_from_fork+0x29/0x50
[1.696297][T1]  
[1.696297][T1] Modules linked in:
[1.696297][T1] CR2: 0288
[1.696297][T1] ---[ end trace  ]---

The PCI root bus is in this case created from ACPI description via
acpi_pci_root_add() -> pci_acpi_scan_root() -> acpi_pci_root_create() ->
pci_create_root_bus() where the last function is called with
parent=NULL. It indicates that no parent is present and then
bus->bridge->parent is NULL too.

Fix the problem by checking bus->bridge->parent in xen_dt_get_node() for
NULL first.

Fixes: ef8ae384b4c9 ("xen/virtio: Handle PCI devices which Host controller is 
described in DT")
Signed-off-by: Petr Pavlu 
---
 drivers/xen/grant-dma-ops.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
index 9784a77fa3c9..76f6f26265a3 100644
--- a/drivers/xen/grant-dma-ops.c
+++ b/drivers/xen/grant-dma-ops.c
@@ -303,6 +303,8 @@ static struct device_node *xen_dt_get_node(struct device 
*dev)
while (!pci_is_root_bus(bus))
bus = bus->parent;
 
+   if (!bus->bridge->parent)
+   return NULL;
return of_node_get(bus->bridge->parent->of_node);
}
 
-- 
2.35.3

Re: [PATCH 09/11] sysctl: Remove the end element in sysctl table arrays

[Joel Granados]

On Wed, Jun 21, 2023 at 02:16:55PM +0300, Jani Nikula wrote:
> On Wed, 21 Jun 2023, Joel Granados  wrote:
> > Remove the empty end element from all the arrays that are passed to the
> > register sysctl calls. In some files this means reducing the explicit
> > array size by one. Also make sure that we are using the size in
> > ctl_table_header instead of evaluating the .procname element.
> 
> Where's the harm in removing the end elements driver by driver? This is
> an unwieldy patch to handle.

I totally agree. Its a big one!!! but I'm concerned of breaking bisectibility:
* I could for example separate all the removes into separate commits and
  then have a final commit that removes the check for the empty element.
  But this will leave the tree in a state where the for loop will have
  undefined behavior when it looks for the empty end element. It might
  or might not work (probably not :) until the final commit where I fix
  that.

* I could also change the logic that looks for the final element,
  commit that first and then remove the empty element one commit per
  driver after that. But then for all the arrays that still have an
  empty element, there would again be undefined behavior as it would
  think that the last element is valid (when it is really the sentinel).

Any ideas on how to get around these?
> 
> > diff --git a/drivers/gpu/drm/i915/i915_perf.c 
> > b/drivers/gpu/drm/i915/i915_perf.c
> > index f43950219ffc..e4d7372afb10 100644
> > --- a/drivers/gpu/drm/i915/i915_perf.c
> > +++ b/drivers/gpu/drm/i915/i915_perf.c
> > @@ -4884,24 +4884,23 @@ int i915_perf_remove_config_ioctl(struct drm_device 
> > *dev, void *data,
> >  
> >  static struct ctl_table oa_table[] = {
> > {
> > -.procname = "perf_stream_paranoid",
> > -.data = &i915_perf_stream_paranoid,
> > -.maxlen = sizeof(i915_perf_stream_paranoid),
> > -.mode = 0644,
> > -.proc_handler = proc_dointvec_minmax,
> > -.extra1 = SYSCTL_ZERO,
> > -.extra2 = SYSCTL_ONE,
> > -},
> > +   .procname = "perf_stream_paranoid",
> > +   .data = &i915_perf_stream_paranoid,
> > +   .maxlen = sizeof(i915_perf_stream_paranoid),
> > +   .mode = 0644,
> > +   .proc_handler = proc_dointvec_minmax,
> > +   .extra1 = SYSCTL_ZERO,
> > +   .extra2 = SYSCTL_ONE,
> > +   },
> > {
> > -.procname = "oa_max_sample_rate",
> > -.data = &i915_oa_max_sample_rate,
> > -.maxlen = sizeof(i915_oa_max_sample_rate),
> > -.mode = 0644,
> > -.proc_handler = proc_dointvec_minmax,
> > -.extra1 = SYSCTL_ZERO,
> > -.extra2 = &oa_sample_rate_hard_limit,
> > -},
> > -   {}
> > +   .procname = "oa_max_sample_rate",
> > +   .data = &i915_oa_max_sample_rate,
> > +   .maxlen = sizeof(i915_oa_max_sample_rate),
> > +   .mode = 0644,
> > +   .proc_handler = proc_dointvec_minmax,
> > +   .extra1 = SYSCTL_ZERO,
> > +   .extra2 = &oa_sample_rate_hard_limit,
> > +   }
> >  };
> 
> The existing indentation is off, but fixing it doesn't really belong in
> this patch.

Agreed. But I actually was trying to fix something that checkpatch
flagged. I'll change these back (which will cause this patch to be
flagged).

An alternative solution would be to fix the indentation as part of the
preparation patches. Tell me what you think.

Thx

> 
> BR,
> Jani.
> 
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center

-- 

Joel Granados


signature.asc
Description: PGP signature

[PATCH 09/11] sysctl: Remove the end element in sysctl table arrays

[Joel Granados]

Remove the empty end element from all the arrays that are passed to the
register sysctl calls. In some files this means reducing the explicit
array size by one. Also make sure that we are using the size in
ctl_table_header instead of evaluating the .procname element.

Signed-off-by: Joel Granados 
---
 arch/arm/kernel/isa.c |  4 +-
 arch/arm64/kernel/armv8_deprecated.c  |  8 ++--
 arch/arm64/kernel/fpsimd.c|  6 +--
 arch/arm64/kernel/process.c   |  3 +-
 arch/ia64/kernel/crash.c  |  3 +-
 arch/powerpc/kernel/idle.c|  3 +-
 arch/powerpc/platforms/pseries/mobility.c |  3 +-
 arch/s390/appldata/appldata_base.c|  7 ++--
 arch/s390/kernel/debug.c  |  3 +-
 arch/s390/kernel/topology.c   |  3 +-
 arch/s390/mm/cmm.c|  3 +-
 arch/s390/mm/pgalloc.c|  3 +-
 arch/x86/entry/vdso/vdso32-setup.c|  3 +-
 arch/x86/kernel/cpu/intel.c   |  3 +-
 arch/x86/kernel/itmt.c|  3 +-
 crypto/fips.c |  3 +-
 drivers/base/firmware_loader/fallback_table.c |  3 +-
 drivers/cdrom/cdrom.c |  3 +-
 drivers/char/hpet.c   | 13 +++---
 drivers/char/ipmi/ipmi_poweroff.c |  3 +-
 drivers/char/random.c |  3 +-
 drivers/gpu/drm/i915/i915_perf.c  | 33 +++
 drivers/hv/hv_common.c|  3 +-
 drivers/infiniband/core/iwcm.c|  3 +-
 drivers/infiniband/core/ucma.c|  3 +-
 drivers/macintosh/mac_hid.c   |  3 +-
 drivers/md/md.c   |  3 +-
 drivers/misc/sgi-xp/xpc_main.c|  6 +--
 drivers/net/vrf.c |  3 +-
 drivers/parport/procfs.c  | 42 ---
 drivers/perf/arm_pmuv3.c  |  3 +-
 drivers/scsi/scsi_sysctl.c|  3 +-
 drivers/scsi/sg.c |  3 +-
 drivers/tty/tty_io.c  |  3 +-
 drivers/xen/balloon.c |  3 +-
 fs/aio.c  |  3 +-
 fs/cachefiles/error_inject.c  |  3 +-
 fs/coda/sysctl.c  |  3 +-
 fs/coredump.c |  3 +-
 fs/dcache.c   |  3 +-
 fs/devpts/inode.c |  3 +-
 fs/eventpoll.c|  3 +-
 fs/exec.c |  3 +-
 fs/file_table.c   |  3 +-
 fs/inode.c|  3 +-
 fs/lockd/svc.c|  3 +-
 fs/locks.c|  3 +-
 fs/namei.c|  3 +-
 fs/namespace.c|  3 +-
 fs/nfs/nfs4sysctl.c   |  3 +-
 fs/nfs/sysctl.c   |  3 +-
 fs/notify/dnotify/dnotify.c   |  3 +-
 fs/notify/fanotify/fanotify_user.c|  3 +-
 fs/notify/inotify/inotify_user.c  |  3 +-
 fs/ntfs/sysctl.c  |  3 +-
 fs/ocfs2/stackglue.c  |  3 +-
 fs/pipe.c |  3 +-
 fs/proc/proc_sysctl.c |  8 ++--
 fs/quota/dquot.c  |  3 +-
 fs/sysctls.c  |  3 +-
 fs/userfaultfd.c  |  3 +-
 fs/verity/signature.c |  3 +-
 fs/xfs/xfs_sysctl.c   |  4 +-
 init/do_mounts_initrd.c   |  3 +-
 ipc/ipc_sysctl.c  |  3 +-
 ipc/mq_sysctl.c   |  3 +-
 kernel/acct.c |  3 +-
 kernel/bpf/syscall.c  |  3 +-
 kernel/delayacct.c|  3 +-
 kernel/exit.c |  3 +-
 kernel/hung_task.c|  3 +-
 kernel/kexec_core.c   |  3 +-
 kernel/kprobes.c  |  3 +-
 kernel/latencytop.c   |  3 +-
 kernel/locking/lockdep.c  |  3 +-
 kernel/panic.c|  3 +-
 kernel/pid_namespace.c|  3 +-
 kernel/pid_sysctl.h   |  3 +-
 kernel/printk/sysctl.c|  3 +-
 kernel/reboot.c   |  3 +-
 kernel/sched/autogroup.c  |  3 +-
 kernel/sched/core.c   |  3 +-
 kernel/sched/deadline.c   |  3 +-
 kernel/sched/fair.c   |  3 +-
 kernel/sched/rt.c

Re: [PATCH 09/11] sysctl: Remove the end element in sysctl table arrays

[Jani Nikula]

On Wed, 21 Jun 2023, Joel Granados  wrote:
> Remove the empty end element from all the arrays that are passed to the
> register sysctl calls. In some files this means reducing the explicit
> array size by one. Also make sure that we are using the size in
> ctl_table_header instead of evaluating the .procname element.

Where's the harm in removing the end elements driver by driver? This is
an unwieldy patch to handle.

> diff --git a/drivers/gpu/drm/i915/i915_perf.c 
> b/drivers/gpu/drm/i915/i915_perf.c
> index f43950219ffc..e4d7372afb10 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -4884,24 +4884,23 @@ int i915_perf_remove_config_ioctl(struct drm_device 
> *dev, void *data,
>  
>  static struct ctl_table oa_table[] = {
>   {
> -  .procname = "perf_stream_paranoid",
> -  .data = &i915_perf_stream_paranoid,
> -  .maxlen = sizeof(i915_perf_stream_paranoid),
> -  .mode = 0644,
> -  .proc_handler = proc_dointvec_minmax,
> -  .extra1 = SYSCTL_ZERO,
> -  .extra2 = SYSCTL_ONE,
> -  },
> + .procname = "perf_stream_paranoid",
> + .data = &i915_perf_stream_paranoid,
> + .maxlen = sizeof(i915_perf_stream_paranoid),
> + .mode = 0644,
> + .proc_handler = proc_dointvec_minmax,
> + .extra1 = SYSCTL_ZERO,
> + .extra2 = SYSCTL_ONE,
> + },
>   {
> -  .procname = "oa_max_sample_rate",
> -  .data = &i915_oa_max_sample_rate,
> -  .maxlen = sizeof(i915_oa_max_sample_rate),
> -  .mode = 0644,
> -  .proc_handler = proc_dointvec_minmax,
> -  .extra1 = SYSCTL_ZERO,
> -  .extra2 = &oa_sample_rate_hard_limit,
> -  },
> - {}
> + .procname = "oa_max_sample_rate",
> + .data = &i915_oa_max_sample_rate,
> + .maxlen = sizeof(i915_oa_max_sample_rate),
> + .mode = 0644,
> + .proc_handler = proc_dointvec_minmax,
> + .extra1 = SYSCTL_ZERO,
> + .extra2 = &oa_sample_rate_hard_limit,
> + }
>  };

The existing indentation is off, but fixing it doesn't really belong in
this patch.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH 08/11] sysctl: Add size to register_sysctl_init

[Joel Granados]

On Wed, Jun 21, 2023 at 12:47:58PM +0200, Greg Kroah-Hartman wrote:
> On Wed, Jun 21, 2023 at 11:09:57AM +0200, Joel Granados wrote:
> >  static int __init random_sysctls_init(void)
> >  {
> > -   register_sysctl_init("kernel/random", random_table);
> > +   register_sysctl_init("kernel/random", random_table,
> > +ARRAY_SIZE(random_table));
> 
> As mentioned before, why not just do:
> 
> #define register_sysctl_init(string, table)   \
>   __register_sysctl_init(string, table, ARRAY_SIZE(table);
Answered you in the original mail where you suggested it.

Best
> 
> or something like that?
> 
> That way no callers need to change AND you prevent the size from ever
> being incorrect?
> 
> thanks,
> 
> greg k-h

-- 

Joel Granados


signature.asc
Description: PGP signature

[PATCH 08/11] sysctl: Add size to register_sysctl_init

[Joel Granados]

In order to remove the end element from the ctl_table struct arrays, we
explicitly define the size when registering the targes. We add a size
argument to the register_sysctl_init call and pass an ARRAY_SIZE for all
the callers.

Signed-off-by: Joel Granados 
---
 arch/x86/kernel/cpu/intel.c  |  2 +-
 drivers/char/random.c|  3 ++-
 drivers/tty/tty_io.c |  2 +-
 drivers/xen/balloon.c|  3 ++-
 fs/aio.c |  2 +-
 fs/coredump.c|  3 ++-
 fs/dcache.c  |  3 ++-
 fs/exec.c|  3 ++-
 fs/file_table.c  |  3 ++-
 fs/inode.c   |  2 +-
 fs/locks.c   |  2 +-
 fs/namei.c   |  2 +-
 fs/namespace.c   |  3 ++-
 fs/notify/dnotify/dnotify.c  |  3 ++-
 fs/pipe.c|  3 ++-
 fs/proc/proc_sysctl.c| 11 ++-
 fs/quota/dquot.c |  3 ++-
 fs/sysctls.c |  3 ++-
 fs/userfaultfd.c |  3 ++-
 include/linux/sysctl.h   |  8 +---
 init/do_mounts_initrd.c  |  3 ++-
 kernel/acct.c|  3 ++-
 kernel/bpf/syscall.c |  3 ++-
 kernel/delayacct.c   |  3 ++-
 kernel/exit.c|  3 ++-
 kernel/hung_task.c   |  3 ++-
 kernel/kexec_core.c  |  3 ++-
 kernel/kprobes.c |  3 ++-
 kernel/latencytop.c  |  3 ++-
 kernel/locking/lockdep.c |  3 ++-
 kernel/panic.c   |  3 ++-
 kernel/pid_namespace.c   |  3 ++-
 kernel/printk/sysctl.c   |  3 ++-
 kernel/reboot.c  |  3 ++-
 kernel/sched/autogroup.c |  3 ++-
 kernel/sched/core.c  |  3 ++-
 kernel/sched/deadline.c  |  3 ++-
 kernel/sched/fair.c  |  3 ++-
 kernel/sched/rt.c|  3 ++-
 kernel/sched/topology.c  |  3 ++-
 kernel/seccomp.c |  3 ++-
 kernel/signal.c  |  3 ++-
 kernel/stackleak.c   |  3 ++-
 kernel/sysctl.c  |  4 ++--
 kernel/trace/ftrace.c|  3 ++-
 kernel/trace/trace_events_user.c |  3 ++-
 kernel/umh.c |  3 ++-
 kernel/watchdog.c|  3 ++-
 mm/compaction.c  |  2 +-
 mm/hugetlb.c |  2 +-
 mm/hugetlb_vmemmap.c |  3 ++-
 mm/memory-failure.c  |  3 ++-
 mm/oom_kill.c|  3 ++-
 mm/page-writeback.c  |  3 ++-
 security/keys/sysctl.c   |  2 +-
 55 files changed, 104 insertions(+), 66 deletions(-)

diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index 1c4639588ff9..c77a3961443d 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -1195,7 +1195,7 @@ static struct ctl_table sld_sysctls[] = {
 
 static int __init sld_mitigate_sysctl_init(void)
 {
-   register_sysctl_init("kernel", sld_sysctls);
+   register_sysctl_init("kernel", sld_sysctls, ARRAY_SIZE(sld_sysctls));
return 0;
 }
 
diff --git a/drivers/char/random.c b/drivers/char/random.c
index 253f2ddb8913..8db2ea9e3d66 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -1692,7 +1692,8 @@ static struct ctl_table random_table[] = {
  */
 static int __init random_sysctls_init(void)
 {
-   register_sysctl_init("kernel/random", random_table);
+   register_sysctl_init("kernel/random", random_table,
+ARRAY_SIZE(random_table));
return 0;
 }
 device_initcall(random_sysctls_init);
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index c84be40fb8df..63fb3c543b94 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -3618,7 +3618,7 @@ static struct ctl_table tty_table[] = {
  */
 int __init tty_init(void)
 {
-   register_sysctl_init("dev/tty", tty_table);
+   register_sysctl_init("dev/tty", tty_table, ARRAY_SIZE(tty_table));
cdev_init(&tty_cdev, &tty_fops);
if (cdev_add(&tty_cdev, MKDEV(TTYAUX_MAJOR, 0), 1) ||
register_chrdev_region(MKDEV(TTYAUX_MAJOR, 0), 1, "/dev/tty") < 0)
diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
index 586a1673459e..e4544262a429 100644
--- a/drivers/xen/balloon.c
+++ b/drivers/xen/balloon.c
@@ -729,7 +729,8 @@ static int __init balloon_init(void)
 #ifdef CONFIG_XEN_BALLOON_MEMORY_HOTPLUG
set_online_page_callback(&xen_online_page);
register_memory_notifier(&xen_memory_nb);
-   register_sysctl_init("xen/balloon", balloon_table);
+   register_sysctl_init("xen/balloon", balloon_table,
+ARRAY_SIZE(balloon_table));
 #endif
 
balloon_add_regions();
diff --git a/fs/aio.c b/fs/aio.c
index b0b17bd098bb..b09abe7a14d3 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -244,7 +244,7 @@ static struct ctl_table aio_sysctls[] = {
 
 static void __init aio_sysctl_init(void)
 {
-  

Re: [PATCH 09/11] sysctl: Remove the end element in sysctl table arrays

[Jani Nikula]

On Wed, 21 Jun 2023, Joel Granados  wrote:
> On Wed, Jun 21, 2023 at 02:16:55PM +0300, Jani Nikula wrote:
>> On Wed, 21 Jun 2023, Joel Granados  wrote:
>> > Remove the empty end element from all the arrays that are passed to the
>> > register sysctl calls. In some files this means reducing the explicit
>> > array size by one. Also make sure that we are using the size in
>> > ctl_table_header instead of evaluating the .procname element.
>> 
>> Where's the harm in removing the end elements driver by driver? This is
>> an unwieldy patch to handle.
>
> I totally agree. Its a big one!!! but I'm concerned of breaking bisectibility:
> * I could for example separate all the removes into separate commits and
>   then have a final commit that removes the check for the empty element.
>   But this will leave the tree in a state where the for loop will have
>   undefined behavior when it looks for the empty end element. It might
>   or might not work (probably not :) until the final commit where I fix
>   that.
>
> * I could also change the logic that looks for the final element,
>   commit that first and then remove the empty element one commit per
>   driver after that. But then for all the arrays that still have an
>   empty element, there would again be undefined behavior as it would
>   think that the last element is valid (when it is really the sentinel).
>
> Any ideas on how to get around these?

First add size to the register calls, and allow the last one to be
sentinel but do not require the sentinel.

Start removing sentinels, adjusting the size passed in.

Once enough sentinels have been removed, add warning if the final entry
is a sentinel.

Never really remove the check? (But surely you can rework the logic to
not count the number of elements up front, only while iterating.)


BR,
Jani.

>> 
>> > diff --git a/drivers/gpu/drm/i915/i915_perf.c 
>> > b/drivers/gpu/drm/i915/i915_perf.c
>> > index f43950219ffc..e4d7372afb10 100644
>> > --- a/drivers/gpu/drm/i915/i915_perf.c
>> > +++ b/drivers/gpu/drm/i915/i915_perf.c
>> > @@ -4884,24 +4884,23 @@ int i915_perf_remove_config_ioctl(struct 
>> > drm_device *dev, void *data,
>> >  
>> >  static struct ctl_table oa_table[] = {
>> >{
>> > -   .procname = "perf_stream_paranoid",
>> > -   .data = &i915_perf_stream_paranoid,
>> > -   .maxlen = sizeof(i915_perf_stream_paranoid),
>> > -   .mode = 0644,
>> > -   .proc_handler = proc_dointvec_minmax,
>> > -   .extra1 = SYSCTL_ZERO,
>> > -   .extra2 = SYSCTL_ONE,
>> > -   },
>> > +  .procname = "perf_stream_paranoid",
>> > +  .data = &i915_perf_stream_paranoid,
>> > +  .maxlen = sizeof(i915_perf_stream_paranoid),
>> > +  .mode = 0644,
>> > +  .proc_handler = proc_dointvec_minmax,
>> > +  .extra1 = SYSCTL_ZERO,
>> > +  .extra2 = SYSCTL_ONE,
>> > +  },
>> >{
>> > -   .procname = "oa_max_sample_rate",
>> > -   .data = &i915_oa_max_sample_rate,
>> > -   .maxlen = sizeof(i915_oa_max_sample_rate),
>> > -   .mode = 0644,
>> > -   .proc_handler = proc_dointvec_minmax,
>> > -   .extra1 = SYSCTL_ZERO,
>> > -   .extra2 = &oa_sample_rate_hard_limit,
>> > -   },
>> > -  {}
>> > +  .procname = "oa_max_sample_rate",
>> > +  .data = &i915_oa_max_sample_rate,
>> > +  .maxlen = sizeof(i915_oa_max_sample_rate),
>> > +  .mode = 0644,
>> > +  .proc_handler = proc_dointvec_minmax,
>> > +  .extra1 = SYSCTL_ZERO,
>> > +  .extra2 = &oa_sample_rate_hard_limit,
>> > +  }
>> >  };
>> 
>> The existing indentation is off, but fixing it doesn't really belong in
>> this patch.
>
> Agreed. But I actually was trying to fix something that checkpatch
> flagged. I'll change these back (which will cause this patch to be
> flagged).
>
> An alternative solution would be to fix the indentation as part of the
> preparation patches. Tell me what you think.
>
> Thx
>
>> 
>> BR,
>> Jani.
>> 
>> 
>> -- 
>> Jani Nikula, Intel Open Source Graphics Center

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH 08/11] sysctl: Add size to register_sysctl_init

[Joel Granados]

On Wed, Jun 21, 2023 at 11:56:03AM +0200, Jiri Slaby wrote:
> On 21. 06. 23, 11:09, Joel Granados wrote:
> > In order to remove the end element from the ctl_table struct arrays, we
> > explicitly define the size when registering the targes. We add a size
> > argument to the register_sysctl_init call and pass an ARRAY_SIZE for all
> > the callers.
> 
> Hi, I am missing here (or in 00/00) _why_ you are doing that. Is it by a
Not sure what happened. I used the kernels get_maintainers.pl script
together with git-send-email. These are my settings:

"
tocmd ="`pwd`/scripts/get_maintainer.pl --nogit --nogit-fallback --norolestats 
--m --nol --nor"
cccmd ="`pwd`/scripts/get_maintainer.pl --nogit --nogit-fallback --norolestats 
--l --r --nom"
"

Could it be that there is an error in MAINTAINERS?

> chance those saved 9k? I hope not.
Not by chance. It is an expected consequence of removing the empty sentinel
element from ctl_table arrays

Best
> 
> thanks,
> -- 
> js
> suse labs
> 

-- 

Joel Granados


signature.asc
Description: PGP signature

Re: [XEN PATCH v2] docs/misra: document the C dialect and translation toolchain assumptions.

[Roberto Bagnara]

On 20/06/23 16:56, Jan Beulich wrote:

On 20.06.2023 14:10, Roberto Bagnara wrote:

+   * - Arithmetic operator on void type
+ - ARM64, X86_64
+ - See Section "6.24 Arithmetic on void- and Function-Pointers" of 
GCC_MANUAL."


The first line is misleading - we don't (and can't) do arithmetic on void.
What we do is arithmetic on pointers to void (like the gcc section title
properly says).


You are right: corrected now.
Thanks,

   Roberto

Re: [XEN PATCH v2] docs/misra: document the C dialect and translation toolchain assumptions.

[Roberto Bagnara]

On 21/06/23 12:27, Jan Beulich wrote:

On 20.06.2023 14:10, Roberto Bagnara wrote:

+   * - static function is used in an inline function with external linkage
+ - ARM64, X86_64
+ - Non-documented GCC extension. An inline function with external linkage
+   can be inlined everywhere. If that calls a static functions, which is
+   not available everywhere, it is a constraint violation according to
+   C99 6.7.4p3: "An inline definition of a function with external linkage
+   shall not contain a definition of a modifiable object with static
+   storage duration, and shall not contain a reference to an identifier
+   with internal linkage."  A standard-compliant C compiler ought
+   to diagnose all constraint violations: when it does not, as is the
+   case for GCC, the behavior is implicitly undefined.


With _spin_lock_cb() taken care of, do we have any left? Or else can this
be dropped?


Dropped both from the document and the tool configuration.

   Roberto

Re: [PATCH] mm/pdx: Add comments throughout the codebase for pdx

[Alejandro Vallejo]

On Mon, Jun 19, 2023 at 05:30:20PM +0200, Jan Beulich wrote:
> > + * ma_{top,bottom}_mask is simply a shifted pfn_{top,pdx_bottom}_mask 
> > where the
> > + * bottom one shifts in 1s rather than 0s.
> > + */
> 
> Nit: That 2nd bottom variable is ma_va_bottom_mask.
Sure

> 
> > @@ -57,9 +99,25 @@ uint64_t __init pdx_init_mask(uint64_t base_addr)
> >   (uint64_t)1 << (MAX_ORDER + PAGE_SHIFT)) - 1);
> >  }
> >  
> > -u64 __init pdx_region_mask(u64 base, u64 len)
> > +uint64_t __init pdx_region_mask(uint64_t base, uint64_t len)
> >  {
> > -return fill_mask(base ^ (base + len - 1));
> > +uint64_t last = base + len - 1;
> > +/*
> > + * The only bit that matters in base^last is the MSB. There are 2 
> > cases.
> > + *
> > + * case msb(base) < msb(last):
> > + * then fill_mask(base^last) == fill_mask(last). This is non
> > + * compressible.
> > + * case msb(base) == msb(last):
> > + * This means that there _may_ be a sequence of compressible zeroes
> > + * for all addresses between `base` and `last` iff `base` has 
> > enough
> > + * trailing zeroes. That is, it's compressible when
> > + * fill_mask(base^last) < fill_mask(last)
> > + *
> > + * The resulting mask is effectively the moving bits between `base` and
> > + * `last`
> > + */
> > +return fill_mask(base ^ last);
> >  }
> 
> I don't see a need for you to actually change the code here. You can
> as well introduce "last" as shorthand just for the comment.
I thought as you did initially and wrote it as such. In the end it felt
wrong to have an explanation in terms of a token not present in the code.
Furthermore, explaining what the shorthand is in the comment takes more
space than introducing `last` in the code itself.

```
   uint64_t last = base + len - 1;
  /*
   * The only bit that matters in base^last is the MSB. There are 2 cases.
```
  vs
```
  /*
   * Let `last = base + len -1` out of convenience.
   * The only bit that matters in base^last is the MSB. There are 2 cases.
```

TL;DR: I didn't factor out `last` due to aesthetics (I'd rather not touch
the code in this patch, in fact), but it seems warranted in order to reduce
the impedance mismatch between this big-ish comment and the call it
describes. I'll post v2 without that adjustment in case I managed to
convince you. Otherwise feel free to adjust it on commit.

> What I dislike in your way of putting it is the use of fill_mask(last) when
> such a call never occurs in code. Starting from the first sentence,
> can't you explain things just in terms of said MSB
I see. I can refer to the MSBs instead. Works equally well.

e.g:
  fill_mask(base^last) == fill_mask(last)
 |
 V
  msb(fill_mask(base^last)) == msb(last)

> (where the two cases are "set" and "clear")?
I'm not sure I understand what you mean here.

> 
> > --- a/xen/include/xen/mm.h
> > +++ b/xen/include/xen/mm.h
> > @@ -31,6 +31,22 @@
> >   *   (i.e. all devices assigned to) a guest share a single DMA address 
> > space
> >   *   and, by default, Xen will ensure dfn == pfn.
> >   *
> > + * pdx: Page InDeX
> > + *   Indices into the frame table holding the per-page's book-keeping
> > + *   metadata. A compression scheme is used and there's a non-identity
> > + *   mapping between valid(mfn) <-> valid(pdx) See the comments in pdx.c
> > + *   for an in-depth explanation of that mapping.
> 
> The mapping may very well be (and on x86 typically is) an identity
> one. IOW you want to describe not only the compression case, but also
> the "no compression possible" one.
Point taken. I'll rephrase it slightly as "possibly non-identity" and
explicitly state the "no compression is possible" case.

> 
> PDXes also aren't just indexes to the frame table, but also to the
> direct mapping.
I had something to that effect earlier on, but I removed it because it
doesn't seem to be the case on ARM. There's a directmap_base_pdx global
that states the first pdx to be mapped on the directmap.

> 
> > + * maddr: Machine Address
> > + *   The physical address that corresponds to an mfn
> > + *
> > + * vaddr: Xen Virtual Address
> > + *   A virtual address of memory accesible to Xen. It is typically either
> > + *   an address into the direct map or to Xen's own code/data. The direct
> > + *   map implements several compression tricks to save memory, so an offset
> > + *   into it does _not_ necessarily correspond to an maddr due to pdx
> > + *   compression.
> 
> We need to be careful here: If I'm not mistaken at least Arm uses vaddr
> also for guest addresses. In fact I'm not sure vaddr (and perhaps even
> maddr) need explaining here
I'd like to have at least maddr. It's sufficiently project-specific to be
otherwise confusing to find unexplained elsewhere. i.e: In other bare-metal
projects that would be a paddr instead.

vaddr might be trying too hard to boil the ocean as far as definitions

[ovmf test] 181536: all pass - PUSHED

[osstest service owner]

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

Perfect :-)
All tests in this flight passed as required
version targeted for testing:
 ovmf fcd71642df9a3a5932a2db116acc6fe23458d8f5
baseline version:
 ovmf 4a0642ad27bfb566835ae86aedae0e18f9735cc2

Last test of basis   181531  2023-06-21 03:42:36 Z0 days
Testing same since   181536  2023-06-21 11:12:23 Z0 days1 attempts


People who touched revisions under test:
  Dun Tan 
  duntan 
  Ray Ni 

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
   4a0642ad27..fcd71642df  fcd71642df9a3a5932a2db116acc6fe23458d8f5 -> 
xen-tested-master

Re: [XEN PATCH v2] docs/misra: document the C dialect and translation toolchain assumptions.

[Roberto Bagnara]

On 20/06/23 17:05, Jan Beulich wrote:

On 20.06.2023 14:10, Roberto Bagnara wrote:

+   * - Token pasting of ',' and __VA_ARGS__
+ - ARM64, X86_64
+ - See Section "6.21 Macros with a Variable Number of Arguments" of 
GCC_MANUAL.
+
+   * - No arguments for '...' parameter of variadic macro
+ - ARM64, X86_64
+ - See Section "6.21 Macros with a Variable Number of Arguments" of 
GCC_MANUAL.


Seeing these I think you want to also mention our extensive use of
the pre-C99 way of variadic macros, which also includes pasting of
',' with something.


Good catch.  Added

   * - Named variadic macro arguments
 - ARM64, X86_64
 - See Section "6.21 Macros with a Variable Number of Arguments" of 
GCC_MANUAL.

Thanks,

   Roberto

[PATCH] Updates to Xen hypercall preemption

[Per Bilse]

Some Xen hypercalls issued by dom0 guests may run for many 10s of
seconds, potentially causing watchdog timeouts and other problems.
It's rare for this to happen, but it does in extreme circumstances,
for instance when shutting down VMs with very large memory allocations
(> 0.5 - 1TB).  These hypercalls are preemptible, but the fixes in the
kernel to ensure preemption have fallen into a state of disrepair, and
are currently ineffective.  This patch brings things up to date by way of:

1) Update general feature selection from XEN_PV to XEN_DOM0.
The issue is unique to dom0 Xen guests, but isn't unique to PV dom0s,
and will occur in future PVH dom0s.  XEN_DOM0 depends on either PV or PVH,
as well as the appropriate details for dom0.

2) Update specific feature selection from !PREEMPTION to !PREEMPT.
The following table shows the relationship between different preemption
features and their indicators/selectors (Y = "=Y", N = "is not set",
. = absent):

| np-s | np-d | vp-s | vp-d | fp-s | fp-d
CONFIG_PREEMPT_DYNAMIC  N  Y  N  Y  N  Y
 CONFIG_PREEMPTION  .  Y  .  Y  Y  Y
CONFIG_PREEMPT  N  N  N  N  Y  Y
  CONFIG_PREEMPT_VOLUNTARY  N  N  Y  Y  N  N
   CONFIG_PREEMPT_NONE  Y  Y  N  N  N  N

Unless PREEMPT is set, we need to enable the fixes.

3) Update flag access from __this_cpu_XXX() to raw_cpu_XXX().
The long-running hypercalls are flagged by way of a per-cpu variable
which is set before and cleared after the relevant calls.  This elicits
a warning "BUG: using __this_cpu_write() in preemptible [] code",
but xen_pv_evtchn_do_upcall() deals specifically with this.  For
consistency, flag testing is also updated, and the code is simplified
and tidied accordingly.

4) Update irqentry_exit_cond_resched() to raw_irqentry_exit_cond_resched().
The code will call irqentry_exit_cond_resched() if the flag (as noted
above) is set, but the dynamic preemption feature will livepatch that
function to a no-op unless full preemption is selected.  The code is
therefore updated to call raw_irqentry_exit_cond_resched().

Signed-off-by: Per Bilse 
---
 arch/x86/entry/common.c | 34 +-
 drivers/xen/privcmd.c   | 12 ++--
 include/xen/xen-ops.h   | 14 +++---
 3 files changed, 26 insertions(+), 34 deletions(-)

diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
index 6c2826417b33..19e8609a7a5a 100644
--- a/arch/x86/entry/common.c
+++ b/arch/x86/entry/common.c
@@ -20,7 +20,7 @@
 #include 
 #include 
 
-#ifdef CONFIG_XEN_PV
+#ifdef CONFIG_XEN_DOM0
 #include 
 #include 
 #endif
@@ -252,17 +252,17 @@ SYSCALL_DEFINE0(ni_syscall)
return -ENOSYS;
 }
 
-#ifdef CONFIG_XEN_PV
-#ifndef CONFIG_PREEMPTION
+#ifdef CONFIG_XEN_DOM0
+#ifndef CONFIG_PREEMPT
 /*
  * Some hypercalls issued by the toolstack can take many 10s of
  * seconds. Allow tasks running hypercalls via the privcmd driver to
  * be voluntarily preempted even if full kernel preemption is
  * disabled.
  *
- * Such preemptible hypercalls are bracketed by
- * xen_preemptible_hcall_begin() and xen_preemptible_hcall_end()
- * calls.
+ * Such preemptible hypercalls are flagged by
+ * xen_preemptible_hcall_set(true/false), and status is
+ * returned by xen_preemptible_hcall_get().
  */
 DEFINE_PER_CPU(bool, xen_in_preemptible_hcall);
 EXPORT_SYMBOL_GPL(xen_in_preemptible_hcall);
@@ -271,21 +271,15 @@ EXPORT_SYMBOL_GPL(xen_in_preemptible_hcall);
  * In case of scheduling the flag must be cleared and restored after
  * returning from schedule as the task might move to a different CPU.
  */
-static __always_inline bool get_and_clear_inhcall(void)
+static __always_inline bool get_and_unset_hcall(void)
 {
-   bool inhcall = __this_cpu_read(xen_in_preemptible_hcall);
+   bool inhcall = xen_preemptible_hcall_get();
 
-   __this_cpu_write(xen_in_preemptible_hcall, false);
+   xen_preemptible_hcall_set(false);
return inhcall;
 }
-
-static __always_inline void restore_inhcall(bool inhcall)
-{
-   __this_cpu_write(xen_in_preemptible_hcall, inhcall);
-}
 #else
-static __always_inline bool get_and_clear_inhcall(void) { return false; }
-static __always_inline void restore_inhcall(bool inhcall) { }
+static __always_inline bool get_and_unset_hcall(void) { return false; }
 #endif
 
 static void __xen_pv_evtchn_do_upcall(struct pt_regs *regs)
@@ -302,16 +296,14 @@ static void __xen_pv_evtchn_do_upcall(struct pt_regs 
*regs)
 __visible noinstr void xen_pv_evtchn_do_upcall(struct pt_regs *regs)
 {
irqentry_state_t state = irqentry_enter(regs);
-   bool inhcall;
 
instrumentation_begin();
run_sysvec_on_irqstack_cond(__xen_pv_evtchn_do_upcall, regs);
 
-   inhcall = get_and_clear_inhcall();
-   if (inhcall && !WARN_ON_ONCE(state.exit_rcu)) {
-   irqentry_exit_cond_resched();
+   if (get_a

Re: [PATCH v4 12/15] xen: Add SET_CPUFREQ_HWP xen_sysctl_pm_op

[Jan Beulich]

(re-adding xen-devel@)

On 21.06.2023 16:16, Jason Andryuk wrote:
> On Mon, Jun 19, 2023 at 10:47 AM Jan Beulich  wrote:
>> On 14.06.2023 20:02, Jason Andryuk wrote:
>>> +if ( !(set_cppc->set_params & XEN_SYSCTL_CPPC_SET_ACT_WINDOW) &&
>>> + set_cppc->activity_window )
>>> +return -EINVAL;
> 
> There were a few aspects intended to be checked, but I have failed to
> implement them all properly and consistently.  The 32bit fields of the
> CPPC interface are larger than the 8 bit HWP fields (10 bits for
> activity window).  So the first aspect was supposed to ensure all
> those out-of-range bits are 0.
> 
> The second aspect, which wasn't implemented properly, was that fields
> would be 0 unless the corresponding bit was set in set_params.
> 
> The third aspect was to fail if a field was specified but hardware
> support isn't available.  That is now only activity window.
> 
> Do aspects #1 and #2 sound appropriate?  We can discuss #3 below.

Personally I'd prefer if inapplicable fields weren't checked, unless we
expect re-use of those fields with a different way of indicating that
the field holds an applicable value. But my primary desire is for
checking to be as consistent as possible.

>> Feels like I have wondered before what good this check does. I'm
>> inclined to suggest to ...
> 
> This check was supposed to enforce #2.
> 
>>> +if ( set_cppc->activity_window & ~XEN_SYSCTL_CPPC_ACT_WINDOW_MASK )
>>> +return -EINVAL;
>>
>> ... fold the two relevant checks, omitting the middle one:
>>
>> if ( (set_cppc->set_params & XEN_SYSCTL_CPPC_SET_ACT_WINDOW) &&
>>  (!feature_hwp_activity_window ||
>>   (set_cppc->activity_window & ~XEN_SYSCTL_CPPC_ACT_WINDOW_MASK))
>> return -EINVAL;
>>
>> Yet I'm also a little worried about the feature check, requiring the
>> caller to first figure out whether that feature is available. Would
>> it be an alternative to make such "best effort", preferably with
>> some indication that this aspect of the request was not carried out?
> 
> Yes, it would be nice to try and apply on a "best effort" basis as
> it's only activity window which may not be supported.
> 
> The SDM says, "Processors may support a subset of IA32_HWP_REQUEST
> fields as indicated by CPUID. Reads of non-supported fields will
> return 0. Writes to non-supported fields are ignored."
> 
> I'll have to test this, but potentially we just let the writes go
> through?  If the user checks xenpm, they will see that the activity
> window isn't supported?  Hmmm, I don't have a machine without activity
> window support, so I can't test it.  Skylake introduced HWP, but my
> skylake test system supports activity window.
> 
> Or do you want to make xen_set_cppc_para have an in/out and return the
> applied features?

Yes, that was what I meant with "indication of some sort". You could
e.g. simply clear the respective control bit in the request (and then
arrange for it to be copied back).

Jan

Re: [PATCH 08/11] sysctl: Add size to register_sysctl_init

[Joel Granados]

On Wed, Jun 21, 2023 at 01:36:46PM +0200, Petr Mladek wrote:
> On Wed 2023-06-21 11:09:57, Joel Granados wrote:
> > In order to remove the end element from the ctl_table struct arrays, we
> > explicitly define the size when registering the targes. We add a size
> > argument to the register_sysctl_init call and pass an ARRAY_SIZE for all
> > the callers.
> 
> This does not explain the motivatin why the end element is removed.
I also see that the cover letter also lacks this. Let me clarify in my
V2.

> 
> I agree with Jiri that saving 9k is a questionable gain. According to
> the cover letter it saved 0,00%. It is because it saved 9k with allyes
> config which produces huge kernel. IMHO, the 9k might be interesting
> only for a tiny kernel. But I guess that it would safe much less
> bytes there.
I put the 9K as a upper bound kind of value. To get an idea of exactly
how much we are talking about. A lower bound with tiny config and sysctl
enabled is a good idea to give a range.

> 
> And the code with the added ARRAY_SIZE() parameter looks worse than before.
This might not even be an issue in V2. After analysing Greg's feedback,
these might just go away.
> 
> > diff --git a/kernel/printk/sysctl.c b/kernel/printk/sysctl.c
> > index c228343eeb97..28f37b86414e 100644
> > --- a/kernel/printk/sysctl.c
> > +++ b/kernel/printk/sysctl.c
> > @@ -81,5 +81,6 @@ static struct ctl_table printk_sysctls[] = {
> >  
> >  void __init printk_sysctl_init(void)
> >  {
> > -   register_sysctl_init("kernel", printk_sysctls);
> > +   register_sysctl_init("kernel", printk_sysctls,
> > +ARRAY_SIZE(printk_sysctls));
> >  }
> 
> Is register_sysctl_init() still ready to handle the last empty element,
nope, after all the patch set, this functionality would be gone.

> please? I am not in Cc on the related patches.
Not sure what happened there. Should I just add you for the next batch?

> 
> Best Regards,
> Petr

Thx for the feedback

Best
-- 

Joel Granados


signature.asc
Description: PGP signature

[XEN PATCH v3] docs/misra: document the C dialect and translation toolchain assumptions.

[Roberto Bagnara]

This document specifies the C language dialect used by Xen and
the assumptions Xen makes on the translation toolchain.

Signed-off-by: Roberto Bagnara 

Changes in V2:
  - Clarified several entries.
  - Removed entry about the use of the undefined escape sequence \m.
Changes in V3:
  - Removed mention of the __signed__ non-standard keyword.
  - Fixed the entry about arithmetic operator on pointer to void.
  - Added entry for named variadic macro arguments.
  - Removed entry about static function used in an inline function
with external linkage.

---
 docs/misra/C-language-toolchain.rst | 468 
 1 file changed, 468 insertions(+)
 create mode 100644 docs/misra/C-language-toolchain.rst

diff --git a/docs/misra/C-language-toolchain.rst 
b/docs/misra/C-language-toolchain.rst
new file mode 100644
index 00..8651f59118
--- /dev/null
+++ b/docs/misra/C-language-toolchain.rst
@@ -0,0 +1,468 @@
+=
+C Dialect and Translation Assumptions for Xen
+=
+
+This document specifies the C language dialect used by Xen and
+the assumptions Xen makes on the translation toolchain.
+It covers, in particular:
+
+1. the used language extensions;
+2. the translation limits that the translation toolchains must be able
+   to accommodate;
+3. the implementation-defined behaviors upon which Xen may depend.
+
+All points are of course relevant for portability.  In addition,
+programming in C is impossible without a detailed knowledge of the
+implementation-defined behaviors.  For this reason, it is recommended
+that Xen developers have familiarity with this document and the
+documentation referenced therein.
+
+This document needs maintenance and adaptation in the following
+circumstances:
+
+- whenever the compiler is changed or updated;
+- whenever the use of a certain language extension is added or removed;
+- whenever code modifications cause exceeding the stated translation limits.
+
+
+Applicable C Language Standard
+__
+
+Xen is written in C99 with extensions.  The relevant ISO standard is
+
+*ISO/IEC 9899:1999/Cor 3:2007*: Programming Languages - C,
+Technical Corrigendum 3.
+ISO/IEC, Geneva, Switzerland, 2007.
+
+
+Reference Documentation
+___
+
+The following documents are referred to in the sequel:
+
+GCC_MANUAL:
+  https://gcc.gnu.org/onlinedocs/gcc-12.1.0/gcc.pdf
+CPP_MANUAL:
+  https://gcc.gnu.org/onlinedocs/gcc-12.1.0/cpp.pdf
+ARM64_ABI_MANUAL:
+  
https://github.com/ARM-software/abi-aa/blob/60a8eb8c55e999d74dac5e368fc9d7e36e38dda4/aapcs64/aapcs64.rst
+X86_64_ABI_MANUAL:
+  
https://gitlab.com/x86-psABIs/x86-64-ABI/-/jobs/artifacts/master/raw/x86-64-ABI/abi.pdf?job=build
+
+
+C Language Extensions
+_
+
+
+The following table lists the extensions currently used in Xen.
+The table columns are as follows:
+
+   Extension
+  a terse description of the extension;
+   Architectures
+  a set of Xen architectures making use of the extension;
+   References
+  when available, references to the documentation explaining
+  the syntax and semantics of (each instance of) the extension.
+
+
+.. list-table::
+   :widths: 30 15 55
+   :header-rows: 1
+
+   * - Extension
+ - Architectures
+ - References
+
+   * - Non-standard tokens
+ - ARM64, X86_64
+ - _Static_assert:
+  see Section "2.1 C Language" of GCC_MANUAL.
+   asm, __asm__:
+  see Sections "6.48 Alternate Keywords" and "6.47 How to Use Inline 
Assembly Language in C Code" of GCC_MANUAL.
+   __volatile__:
+  see Sections "6.48 Alternate Keywords" and "6.47.2.1 Volatile" of 
GCC_MANUAL.
+   __const__, __inline__, __inline:
+  see Section "6.48 Alternate Keywords" of GCC_MANUAL.
+   typeof, __typeof__:
+  see Section "6.7 Referring to a Type with typeof" of GCC_MANUAL.
+   __alignof__, __alignof:
+  see Sections "6.48 Alternate Keywords" and "6.44 Determining the 
Alignment of Functions, Types or Variables" of GCC_MANUAL.
+   __attribute__:
+  see Section "6.39 Attribute Syntax" of GCC_MANUAL.
+   __builtin_types_compatible_p:
+  see Section "6.59 Other Built-in Functions Provided by GCC" of 
GCC_MANUAL.
+   __builtin_va_arg:
+  non-documented GCC extension.
+   __builtin_offsetof:
+  see Section "6.53 Support for offsetof" of GCC_MANUAL.
+
+   * - Empty initialization list
+ - ARM64, X86_64
+ - Non-documented GCC extension.
+
+   * - Arithmetic operator on pointer to void
+ - ARM64, X86_64
+ - See Section "6.24 Arithmetic on void- and Function-Pointers" of 
GCC_MANUAL."
+
+   * - Statements and declarations in expressions
+ - ARM64, X86_64
+ - See Section "6.1 Statements and Declarations in Expressions" of 
GCC_MANUAL.
+
+   * - Structure or union definition with no members
+ - ARM64, X86_64
+ - See Sec

Re: [PATCH 09/11] sysctl: Remove the end element in sysctl table arrays

[Joel Granados]

On Wed, Jun 21, 2023 at 04:15:46PM +0300, Jani Nikula wrote:
> On Wed, 21 Jun 2023, Joel Granados  wrote:
> > On Wed, Jun 21, 2023 at 02:16:55PM +0300, Jani Nikula wrote:
> >> On Wed, 21 Jun 2023, Joel Granados  wrote:
> >> > Remove the empty end element from all the arrays that are passed to the
> >> > register sysctl calls. In some files this means reducing the explicit
> >> > array size by one. Also make sure that we are using the size in
> >> > ctl_table_header instead of evaluating the .procname element.
> >> 
> >> Where's the harm in removing the end elements driver by driver? This is
> >> an unwieldy patch to handle.
> >
> > I totally agree. Its a big one!!! but I'm concerned of breaking 
> > bisectibility:
> > * I could for example separate all the removes into separate commits and
> >   then have a final commit that removes the check for the empty element.
> >   But this will leave the tree in a state where the for loop will have
> >   undefined behavior when it looks for the empty end element. It might
> >   or might not work (probably not :) until the final commit where I fix
> >   that.
> >
> > * I could also change the logic that looks for the final element,
> >   commit that first and then remove the empty element one commit per
> >   driver after that. But then for all the arrays that still have an
> >   empty element, there would again be undefined behavior as it would
> >   think that the last element is valid (when it is really the sentinel).
> >
> > Any ideas on how to get around these?
> 
> First add size to the register calls, and allow the last one to be
> sentinel but do not require the sentinel.
> 
> Start removing sentinels, adjusting the size passed in.
This is a great idea! and I think I don't even have to adjust the size
because if I change the logic to stop on the sentinel or the size; so when
the sentinel is there, it will stop before the size. And when the
sentinel is not there, it will stop on the correct size.

There might be issues with indirection calls. And there might also be
lots of places where I need to adjust a for loop (as dan has pointed
out) but its worth a try for V2.

Best
> 
> Once enough sentinels have been removed, add warning if the final entry
> is a sentinel.
> 
> Never really remove the check? (But surely you can rework the logic to
> not count the number of elements up front, only while iterating.)
> 
> 
> BR,
> Jani.
> 
> >> 
> >> > diff --git a/drivers/gpu/drm/i915/i915_perf.c 
> >> > b/drivers/gpu/drm/i915/i915_perf.c
> >> > index f43950219ffc..e4d7372afb10 100644
> >> > --- a/drivers/gpu/drm/i915/i915_perf.c
> >> > +++ b/drivers/gpu/drm/i915/i915_perf.c
> >> > @@ -4884,24 +4884,23 @@ int i915_perf_remove_config_ioctl(struct 
> >> > drm_device *dev, void *data,
> >> >  
> >> >  static struct ctl_table oa_table[] = {
> >> >  {
> >> > - .procname = "perf_stream_paranoid",
> >> > - .data = &i915_perf_stream_paranoid,
> >> > - .maxlen = sizeof(i915_perf_stream_paranoid),
> >> > - .mode = 0644,
> >> > - .proc_handler = proc_dointvec_minmax,
> >> > - .extra1 = SYSCTL_ZERO,
> >> > - .extra2 = SYSCTL_ONE,
> >> > - },
> >> > +.procname = "perf_stream_paranoid",
> >> > +.data = &i915_perf_stream_paranoid,
> >> > +.maxlen = sizeof(i915_perf_stream_paranoid),
> >> > +.mode = 0644,
> >> > +.proc_handler = proc_dointvec_minmax,
> >> > +.extra1 = SYSCTL_ZERO,
> >> > +.extra2 = SYSCTL_ONE,
> >> > +},
> >> >  {
> >> > - .procname = "oa_max_sample_rate",
> >> > - .data = &i915_oa_max_sample_rate,
> >> > - .maxlen = sizeof(i915_oa_max_sample_rate),
> >> > - .mode = 0644,
> >> > - .proc_handler = proc_dointvec_minmax,
> >> > - .extra1 = SYSCTL_ZERO,
> >> > - .extra2 = &oa_sample_rate_hard_limit,
> >> > - },
> >> > -{}
> >> > +.procname = "oa_max_sample_rate",
> >> > +.data = &i915_oa_max_sample_rate,
> >> > +.maxlen = sizeof(i915_oa_max_sample_rate),
> >> > +.mode = 0644,
> >> > +.proc_handler = proc_dointvec_minmax,
> >> > +.extra1 = SYSCTL_ZERO,
> >> > +.extra2 = &oa_sample_rate_hard_limit,
> >> > +}
> >> >  };
> >> 
> >> The existing indentation is off, but fixing it doesn't really belong in
> >> this patch.
> >
> > Agreed. But I actually was trying to fix something that checkpatch
> > flagged. I'll change these back (which will cause this patch to be
> > flagged).
> >
> > An alternative solution would be to fix the indentation as part of the
> > preparation patches. Tell me what you think.
> >
> > Thx
> >
> >> 
> >> BR,
> >> Jani.
> >> 
> >> 
> >> -- 
> >> Jani Nikula, Intel Open Source Graphics Center
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center

-- 

Joel Granados

[XEN PATCH 1/3] build: define ARCH and SRCARCH later

[Anthony PERARD]

Defining ARCH and SRCARCH later in xen/Makefile allows to switch to
immediate evaluation variable type.

ARCH and SRCARCH depends on value defined in Config.mk and aren't used
TARGET_SUBARCH or TARGET_ARCH, and not before it's needed in a
sub-make or a rule.

This will help reduce the number of times the shell rune is been
run.

With GNU make 4.4, the number of execution of the command present in
these $(shell ) increased greatly. This is probably because as of make
4.4, exported variable are also added to the environment of $(shell )
construct.

Also, `make -d` shows a lot of these:
Makefile:39: not recursively expanding SRCARCH to export to shell function
Makefile:38: not recursively expanding ARCH to export to shell function

Reported-by: Jason Andryuk 
Signed-off-by: Anthony PERARD 
---
 xen/Makefile | 13 +++--
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/xen/Makefile b/xen/Makefile
index e89fc461fc4b..9631e45cfb9b 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -35,12 +35,6 @@ MAKEFLAGS += -rR
 
 EFI_MOUNTPOINT ?= $(BOOT_DIR)/efi
 
-ARCH=$(XEN_TARGET_ARCH)
-SRCARCH=$(shell echo $(ARCH) | \
-  sed -e 's/x86.*/x86/' -e s'/arm\(32\|64\)/arm/g' \
-  -e s'/riscv.*/riscv/g')
-export ARCH SRCARCH
-
 # Allow someone to change their config file
 export KCONFIG_CONFIG ?= .config
 
@@ -241,6 +235,13 @@ include scripts/Kbuild.include
 include $(XEN_ROOT)/Config.mk
 
 # Set ARCH/SUBARCH appropriately.
+
+ARCH := $(XEN_TARGET_ARCH)
+SRCARCH := $(shell echo $(ARCH) | \
+  sed -e 's/x86.*/x86/' -e s'/arm\(32\|64\)/arm/g' \
+  -e s'/riscv.*/riscv/g')
+export ARCH SRCARCH
+
 export TARGET_SUBARCH  := $(XEN_TARGET_ARCH)
 export TARGET_ARCH := $(shell echo $(XEN_TARGET_ARCH) | \
 sed -e 's/x86.*/x86/' -e s'/arm\(32\|64\)/arm/g' \
-- 
Anthony PERARD

[XEN PATCH 0/3] build: reduce number of $(shell) execution on make 4.4

[Anthony PERARD]

Patch series available in this git branch:
https://xenbits.xen.org/git-http/people/aperard/xen-unstable.git 
br.build-exported-shell-command-value-v1

With GNU make 4.4, the number of execution of the command present in $(shell )
in exported variables increased greatly. This is probably because as of make
4.4, exported variable are also added to the environment of $(shell )
construct.

>From the annoncement:

https://lists.gnu.org/archive/html/info-gnu/2022-10/msg8.html
> * WARNING: Backward-incompatibility!
>   Previously makefile variables marked as export were not exported to 
commands
>   started by the $(shell ...) function.  Now, all exported variables are
>   exported to $(shell ...).  If this leads to recursion during expansion, 
then
>   for backward-compatibility the value from the original environment is 
used.
>   To detect this change search for 'shell-export' in the .FEATURES 
variable.

Also, there's a new paragraph in the GNU make manual, but it's a warning about
exporting all variable, still it might be relevant to the new observed
behavior:

https://www.gnu.org/software/make/manual/make.html#Variables_002fRecursion
> Adding a variable’s value to the environment requires it to be expanded. 
If
> expanding a variable has side-effects (such as the info or eval or similar
> functions) then these side-effects will be seen every time a command is
> invoked.

The issue was reported on IRC by jandryuk.

So, I've locate a few obvious candidate to fix, maybe there's more $(shell) to
change?

Anthony PERARD (3):
  build: define ARCH and SRCARCH later
  build: evaluate XEN_BUILD_* and XEN_DOMAIN on first use
  Config.mk: evaluate XEN_COMPILE_ARCH and XEN_OS on first use

 Config.mk|  6 +++---
 xen/Makefile | 21 +++--
 2 files changed, 14 insertions(+), 13 deletions(-)

-- 
Anthony PERARD

[XEN PATCH 3/3] Config.mk: evaluate XEN_COMPILE_ARCH and XEN_OS on first use

[Anthony PERARD]

With GNU make 4.4, the number of execution of the command present in
these $(shell ) increased greatly. This is probably because as of make
4.4, exported variable are also added to the environment of $(shell )
construct.

So, to avoid having these command been run more than necessery, we
will use a construct to evaluate on first use.

Reported-by: Jason Andryuk 
Signed-off-by: Anthony PERARD 
---
 Config.mk | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Config.mk b/Config.mk
index c529b1ba19cd..5fbdbc4500d2 100644
--- a/Config.mk
+++ b/Config.mk
@@ -19,13 +19,13 @@ or   = $(if $(strip $(1)),$(1),$(if $(strip 
$(2)),$(2),$(if $(strip $(3)),$(
 
 -include $(XEN_ROOT)/.config
 
-XEN_COMPILE_ARCH?= $(shell uname -m | sed -e s/i.86/x86_32/ \
+XEN_COMPILE_ARCH?= $(eval XEN_COMPILE_ARCH := $(shell uname -m | sed -e 
s/i.86/x86_32/ \
  -e s/i86pc/x86_32/ -e s/amd64/x86_64/ \
  -e s/armv7.*/arm32/ -e s/armv8.*/arm64/ \
- -e s/aarch64/arm64/)
+ -e s/aarch64/arm64/))$(XEN_COMPILE_ARCH)
 
 XEN_TARGET_ARCH ?= $(XEN_COMPILE_ARCH)
-XEN_OS  ?= $(shell uname -s)
+XEN_OS  ?= $(eval XEN_OS := $(shell uname -s))$(XEN_OS)
 
 CONFIG_$(XEN_OS) := y
 
-- 
Anthony PERARD

[XEN PATCH 2/3] build: evaluate XEN_BUILD_* and XEN_DOMAIN on first use

[Anthony PERARD]

With GNU make 4.4, the number of execution of the command present in
these $(shell ) increased greatly. This is probably because as of make
4.4, exported variable are also added to the environment of $(shell )
construct.

Also, `make -d` shows a lot of these:
Makefile:15: not recursively expanding XEN_BUILD_DATE to export to shell 
function
Makefile:16: not recursively expanding XEN_BUILD_TIME to export to shell 
function
Makefile:17: not recursively expanding XEN_BUILD_HOST to export to shell 
function
Makefile:14: not recursively expanding XEN_DOMAIN to export to shell 
function

So, to avoid having these command been run more than necessery, we
will use a construct to evaluate on first use.

Reported-by: Jason Andryuk 
Signed-off-by: Anthony PERARD 
---
 xen/Makefile | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/Makefile b/xen/Makefile
index 9631e45cfb9b..b58c2a7f4539 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -11,10 +11,10 @@ export XEN_FULLVERSION   = 
$(XEN_VERSION).$(XEN_SUBVERSION)$(XEN_EXTRAVERSION)
 -include xen-version
 
 export XEN_WHOAMI  ?= $(USER)
-export XEN_DOMAIN  ?= $(shell ([ -x /bin/dnsdomainname ] && 
/bin/dnsdomainname) || ([ -x /bin/domainname ] && /bin/domainname || echo 
[unknown]))
-export XEN_BUILD_DATE  ?= $(shell LC_ALL=C date)
-export XEN_BUILD_TIME  ?= $(shell LC_ALL=C date +%T)
-export XEN_BUILD_HOST  ?= $(shell hostname)
+export XEN_DOMAIN  ?= $(eval XEN_DOMAIN := $(shell ([ -x 
/bin/dnsdomainname ] && /bin/dnsdomainname) || ([ -x /bin/domainname ] && 
/bin/domainname || echo [unknown])))$(XEN_DOMAIN)
+export XEN_BUILD_DATE  ?= $(eval XEN_BUILD_DATE := $(shell LC_ALL=C 
date))$(XEN_BUILD_DATE)
+export XEN_BUILD_TIME  ?= $(eval XEN_BUILD_TIME := $(shell LC_ALL=C date 
+%T))$(XEN_BUILD_TIME)
+export XEN_BUILD_HOST  ?= $(eval XEN_BUILD_HOST := $(shell 
hostname))$(XEN_BUILD_HOST)
 
 # Best effort attempt to find a python interpreter, defaulting to Python 3 if
 # available.  Fall back to just `python` if `which` is nowhere to be found.
-- 
Anthony PERARD

Re: [PATCH 1/2] xen/virtio: Fix NULL deref when a bridge of PCI root bus has no parent

[Oleksandr Tyshchenko]

On 21.06.23 16:12, Petr Pavlu wrote:


Hello Petr


> When attempting to run Xen on a QEMU/KVM virtual machine with virtio
> devices (all x86_64), function xen_dt_get_node() crashes on accessing
> bus->bridge->parent->of_node because a bridge of the PCI root bus has no
> parent set:
> 
> [1.694192][T1] BUG: kernel NULL pointer dereference, address: 
> 0288
> [1.695688][T1] #PF: supervisor read access in kernel mode
> [1.696297][T1] #PF: error_code(0x) - not-present page
> [1.696297][T1] PGD 0 P4D 0
> [1.696297][T1] Oops:  [#1] PREEMPT SMP NOPTI
> [1.696297][T1] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 
> 6.3.7-1-default #1 openSUSE Tumbleweed 
> a577eae57964bb7e83477b5a5645a1781df990f0
> [1.696297][T1] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), 
> BIOS rel-1.15.0-0-g2dd4b9b-rebuilt.opensuse.org 04/01/2014
> [1.696297][T1] RIP: e030:xen_virtio_restricted_mem_acc+0xd9/0x1c0
> [1.696297][T1] Code: 45 0c 83 e8 c9 a3 ea ff 31 c0 eb d7 48 8b 87 40 
> ff ff ff 48 89 c2 48 8b 40 10 48 85 c0 75 f4 48 8b 82 10 01 00 00 48 8b 40 40 
> <48> 83 b8 88 02 00 00 00 0f 84 45 ff ff ff 66 90 31 c0 eb a5 48 89
> [1.696297][T1] RSP: e02b:c90040013cc8 EFLAGS: 00010246
> [1.696297][T1] RAX:  RBX: 888006c75000 RCX: 
> 0029
> [1.696297][T1] RDX: 888005ed1000 RSI: c900400f100c RDI: 
> 888005ee30d0
> [1.696297][T1] RBP: 888006c75010 R08: 0001 R09: 
> 00033006
> [1.696297][T1] R10: 888005850028 R11: 0002 R12: 
> 830439a0
> [1.696297][T1] R13:  R14: 888005657900 R15: 
> 888006e3e1e8
> [1.696297][T1] FS:  () GS:88804a00() 
> knlGS:
> [1.696297][T1] CS:  e030 DS:  ES:  CR0: 80050033
> [1.696297][T1] CR2: 0288 CR3: 02e36000 CR4: 
> 00050660
> [1.696297][T1] Call Trace:
> [1.696297][T1]  
> [1.696297][T1]  virtio_features_ok+0x1b/0xd0
> [1.696297][T1]  virtio_dev_probe+0x19c/0x270
> [1.696297][T1]  really_probe+0x19b/0x3e0
> [1.696297][T1]  __driver_probe_device+0x78/0x160
> [1.696297][T1]  driver_probe_device+0x1f/0x90
> [1.696297][T1]  __driver_attach+0xd2/0x1c0
> [1.696297][T1]  bus_for_each_dev+0x74/0xc0
> [1.696297][T1]  bus_add_driver+0x116/0x220
> [1.696297][T1]  driver_register+0x59/0x100
> [1.696297][T1]  virtio_console_init+0x7f/0x110
> [1.696297][T1]  do_one_initcall+0x47/0x220
> [1.696297][T1]  kernel_init_freeable+0x328/0x480
> [1.696297][T1]  kernel_init+0x1a/0x1c0
> [1.696297][T1]  ret_from_fork+0x29/0x50
> [1.696297][T1]  
> [1.696297][T1] Modules linked in:
> [1.696297][T1] CR2: 0288
> [1.696297][T1] ---[ end trace  ]---
> 
> The PCI root bus is in this case created from ACPI description via
> acpi_pci_root_add() -> pci_acpi_scan_root() -> acpi_pci_root_create() ->
> pci_create_root_bus() where the last function is called with
> parent=NULL. It indicates that no parent is present and then
> bus->bridge->parent is NULL too.
> 
> Fix the problem by checking bus->bridge->parent in xen_dt_get_node() for
> NULL first >
> Fixes: ef8ae384b4c9 ("xen/virtio: Handle PCI devices which Host controller is 
> described in DT")

Oops, sorry. I have to admit I checked with DT only.


> Signed-off-by: Petr Pavlu 


Reviewed-by: Oleksandr Tyshchenko 



> ---
>   drivers/xen/grant-dma-ops.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
> index 9784a77fa3c9..76f6f26265a3 100644
> --- a/drivers/xen/grant-dma-ops.c
> +++ b/drivers/xen/grant-dma-ops.c
> @@ -303,6 +303,8 @@ static struct device_node *xen_dt_get_node(struct device 
> *dev)
>   while (!pci_is_root_bus(bus))
>   bus = bus->parent;
>   
> + if (!bus->bridge->parent)
> + return NULL;
>   return of_node_get(bus->bridge->parent->of_node);
>   }
>   

Re: [XEN PATCH 1/3] build: define ARCH and SRCARCH later

[Andrew Cooper]

On 21/06/2023 5:19 pm, Anthony PERARD wrote:
> Defining ARCH and SRCARCH later in xen/Makefile allows to switch to
> immediate evaluation variable type.
>
> ARCH and SRCARCH depends on value defined in Config.mk and aren't used
> TARGET_SUBARCH or TARGET_ARCH, and not before it's needed in a
> sub-make or a rule.
>
> This will help reduce the number of times the shell rune is been
> run.
>
> With GNU make 4.4, the number of execution of the command present in
> these $(shell ) increased greatly. This is probably because as of make
> 4.4, exported variable are also added to the environment of $(shell )
> construct.
>
> Also, `make -d` shows a lot of these:
> Makefile:39: not recursively expanding SRCARCH to export to shell function
> Makefile:38: not recursively expanding ARCH to export to shell function
>
> Reported-by: Jason Andryuk 
> Signed-off-by: Anthony PERARD 
> ---
>  xen/Makefile | 13 +++--
>  1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/xen/Makefile b/xen/Makefile
> index e89fc461fc4b..9631e45cfb9b 100644
> --- a/xen/Makefile
> +++ b/xen/Makefile
> @@ -35,12 +35,6 @@ MAKEFLAGS += -rR
>  
>  EFI_MOUNTPOINT ?= $(BOOT_DIR)/efi
>  
> -ARCH=$(XEN_TARGET_ARCH)
> -SRCARCH=$(shell echo $(ARCH) | \
> -  sed -e 's/x86.*/x86/' -e s'/arm\(32\|64\)/arm/g' \
> -  -e s'/riscv.*/riscv/g')
> -export ARCH SRCARCH
> -
>  # Allow someone to change their config file
>  export KCONFIG_CONFIG ?= .config
>  
> @@ -241,6 +235,13 @@ include scripts/Kbuild.include
>  include $(XEN_ROOT)/Config.mk
>  
>  # Set ARCH/SUBARCH appropriately.
> +
> +ARCH := $(XEN_TARGET_ARCH)
> +SRCARCH := $(shell echo $(ARCH) | \
> +  sed -e 's/x86.*/x86/' -e s'/arm\(32\|64\)/arm/g' \
> +  -e s'/riscv.*/riscv/g')
> +export ARCH SRCARCH
> +
>  export TARGET_SUBARCH  := $(XEN_TARGET_ARCH)
>  export TARGET_ARCH := $(shell echo $(XEN_TARGET_ARCH) | \
>  sed -e 's/x86.*/x86/' -e s'/arm\(32\|64\)/arm/g' 
> \

The change looks plausible to fix this issue, but could we take the
opportunity to dedup the sed expression into a $(call src-arch ...) or so ?

Except, given that ARCH := $(XEN_TARGET_ARCH) now, doesn't that mean
SRCARCH is always TARGET_ARCH ?

Can't we simplify this to just be plain aliases?

~Andrew

Re: [PATCH] Updates to Xen hypercall preemption

[Andy Lutomirski]

On Wed, Jun 21, 2023, at 8:14 AM, Per Bilse wrote:
> Some Xen hypercalls issued by dom0 guests may run for many 10s of
> seconds, potentially causing watchdog timeouts and other problems.
> It's rare for this to happen, but it does in extreme circumstances,
> for instance when shutting down VMs with very large memory allocations
> (> 0.5 - 1TB).  These hypercalls are preemptible, but the fixes in the
> kernel to ensure preemption have fallen into a state of disrepair, and
> are currently ineffective.  This patch brings things up to date by way of:
>
> 1) Update general feature selection from XEN_PV to XEN_DOM0.
> The issue is unique to dom0 Xen guests, but isn't unique to PV dom0s,
> and will occur in future PVH dom0s.  XEN_DOM0 depends on either PV or PVH,
> as well as the appropriate details for dom0.
>
> 2) Update specific feature selection from !PREEMPTION to !PREEMPT.
> The following table shows the relationship between different preemption
> features and their indicators/selectors (Y = "=Y", N = "is not set",
> . = absent):
>
> | np-s | np-d | vp-s | vp-d | fp-s | fp-d
> CONFIG_PREEMPT_DYNAMIC  N  Y  N  Y  N  Y
>  CONFIG_PREEMPTION  .  Y  .  Y  Y  Y
> CONFIG_PREEMPT  N  N  N  N  Y  Y
>   CONFIG_PREEMPT_VOLUNTARY  N  N  Y  Y  N  N
>CONFIG_PREEMPT_NONE  Y  Y  N  N  N  N
>
> Unless PREEMPT is set, we need to enable the fixes.

This code is a horrible mess, with and without your patches.  I think that, if 
this were new, there's no way it would make it in to the kernel.

I propose one of two rather radical changes:

1. (preferred) Just delete all of it and make support for dom0 require either 
full or dynamic preempt, and make a dynamic preempt kernel booting as dom0 run 
as full preempt.

2. Forget about trying to preempt a hypercall in the sense of scheduling from 
an interrupt.  Instead teach the interrupt code to detect that it's in a 
preemptible hypercall and change RIP to a landing pad that does a 
cond_resched() and then resumes the hypercall.

I don't think the entry code should have a whole special preempt implementation 
just for this nasty special case.

--Andy

Re: [XEN PATCH 1/3] build: define ARCH and SRCARCH later

[Jason Andryuk]

On Wed, Jun 21, 2023 at 12:20 PM Anthony PERARD
 wrote:
>
> Defining ARCH and SRCARCH later in xen/Makefile allows to switch to
> immediate evaluation variable type.
>
> ARCH and SRCARCH depends on value defined in Config.mk and aren't used
> TARGET_SUBARCH or TARGET_ARCH, and not before it's needed in a
> sub-make or a rule.
>
> This will help reduce the number of times the shell rune is been
> run.
>
> With GNU make 4.4, the number of execution of the command present in
> these $(shell ) increased greatly. This is probably because as of make
> 4.4, exported variable are also added to the environment of $(shell )
> construct.
>
> Also, `make -d` shows a lot of these:
> Makefile:39: not recursively expanding SRCARCH to export to shell function
> Makefile:38: not recursively expanding ARCH to export to shell function
>
> Reported-by: Jason Andryuk 
> Signed-off-by: Anthony PERARD 

Tested-by: Jason Andryuk 

Things are back to normal speed - Thanks a lot!

-Jason

Re: [XEN PATCH 1/3] build: define ARCH and SRCARCH later

[Jason Andryuk]

On Wed, Jun 21, 2023 at 12:27 PM Jason Andryuk  wrote:
>
> On Wed, Jun 21, 2023 at 12:20 PM Anthony PERARD
>  wrote:
> >
> > Defining ARCH and SRCARCH later in xen/Makefile allows to switch to
> > immediate evaluation variable type.
> >
> > ARCH and SRCARCH depends on value defined in Config.mk and aren't used
> > TARGET_SUBARCH or TARGET_ARCH, and not before it's needed in a
> > sub-make or a rule.
> >
> > This will help reduce the number of times the shell rune is been
> > run.
> >
> > With GNU make 4.4, the number of execution of the command present in
> > these $(shell ) increased greatly. This is probably because as of make
> > 4.4, exported variable are also added to the environment of $(shell )
> > construct.
> >
> > Also, `make -d` shows a lot of these:
> > Makefile:39: not recursively expanding SRCARCH to export to shell 
> > function
> > Makefile:38: not recursively expanding ARCH to export to shell function
> >
> > Reported-by: Jason Andryuk 
> > Signed-off-by: Anthony PERARD 
>
> Tested-by: Jason Andryuk 

Tested-by: for the whole series, FYI.

Thanks,
Jason

Re: [PATCH v1 1/1] Q35 Support

[Joel Upham]

Sorry, this was sent in error when I did the git send-email for the folder.
This was before I broke each patch down (after looking at the Qemu
submission guidance). This is my first time sending a patch in this way, so
thanks for the understanding. This patch can be ignored, as they are all
covered elsewhere.

-Joel Upham

On Wed, Jun 21, 2023 at 7:10 AM David Hildenbrand  wrote:

> On 20.06.23 19:24, Joel Upham wrote:
>
> Inexpressive patch subject and non-existant patch desciption. I have no
> clue what this is supposed to do, except that it involes q35 and xen ()I
> guess ?.
>
> > ---
> >   hw/acpi/ich9.c|   22 +-
> >   hw/acpi/pcihp.c   |6 +-
> >   hw/core/machine.c |   19 +
> >   hw/i386/pc_piix.c |3 +-
> >   hw/i386/pc_q35.c  |   39 +-
> >   hw/i386/xen/xen-hvm.c |7 +-
> >   hw/i386/xen/xen_platform.c|   19 +-
> >   hw/isa/lpc_ich9.c |   53 +-
> >   hw/isa/piix3.c|2 +-
> >   hw/pci-host/q35.c |   28 +-
> >   hw/pci/pci.c  |   17 +
> >   hw/xen/xen-host-pci-device.c  |  106 +++-
> >   hw/xen/xen-host-pci-device.h  |6 +-
> >   hw/xen/xen_pt.c   |   49 +-
> >   hw/xen/xen_pt.h   |   19 +-
> >   hw/xen/xen_pt_config_init.c   | 1103 ++---
> >   include/hw/acpi/ich9.h|1 +
> >   include/hw/acpi/pcihp.h   |2 +
> >   include/hw/boards.h   |1 +
> >   include/hw/i386/pc.h  |3 +
> >   include/hw/pci-host/q35.h |4 +-
> >   include/hw/pci/pci.h  |3 +
> >   include/hw/southbridge/ich9.h |1 +
> >   include/hw/xen/xen.h  |4 +-
> >   qemu-options.hx   |1 +
> >   softmmu/datadir.c |1 -
> >   softmmu/qdev-monitor.c|3 +-
> >   stubs/xen-hw-stub.c   |4 +-
> >   28 files changed, 1395 insertions(+), 131 deletions(-)
> >
>
> Usually people refrain from reviewing such massive patches. Most
> probably this can be broken up into reviewable pieces.
>
> Was this supposed to be an RFC?
>
> --
> Cheers,
>
> David / dhildenb
>
>

[xen-unstable-smoke test] 181537: trouble: blocked/broken/pass

[osstest service owner]

flight 181537 xen-unstable-smoke real [real]
http://logs.test-lab.xenproject.org/osstest/logs/181537/

Failures and problems with tests :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 build-armhf  broken
 build-armhf   4 host-install(4)broken REGR. vs. 181476

Tests which did not succeed, but are not blocking:
 test-armhf-armhf-xl   1 build-check(1)   blocked  n/a
 test-amd64-amd64-libvirt 15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  16 saverestore-support-checkfail   never pass

version targeted for testing:
 xen  14f42af3f52d56e769263dc414616be805bd6e2d
baseline version:
 xen  7a25a1501ca941c3e01b0c4e624ace05417f1587

Last test of basis   181476  2023-06-17 04:00:28 Z4 days
Failing since181504  2023-06-19 11:00:26 Z2 days7 attempts
Testing same since   181537  2023-06-21 12:02:07 Z0 days1 attempts


People who touched revisions under test:
  Alistair Francis 
  Andrew Cooper 
  Anthony PERARD 
  Henry Wang 
  Jan Beulich 
  Julien Grall 
  Michal Orzel 
  Oleksii Kurochko 
  Roger Pau Monné 
  Stefano Stabellini 

jobs:
 build-arm64-xsm  pass
 build-amd64  pass
 build-armhf  broken  
 build-amd64-libvirt  pass
 test-armhf-armhf-xl  blocked 
 test-arm64-arm64-xl-xsm  pass
 test-amd64-amd64-xl-qemuu-debianhvm-amd64pass
 test-amd64-amd64-libvirt pass



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

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

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

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

broken-job build-armhf broken
broken-step build-armhf host-install(4)

Not pushing.

(No revision log; it would be 308 lines long.)

Re: [PATCH v1 1/1] Q35 Support

[David Hildenbrand]

On 21.06.23 18:35, Joel Upham wrote:
Sorry, this was sent in error when I did the git send-email for the 
folder. This was before I broke each patch down (after looking at the 
Qemu submission guidance). This is my first time sending a patch in this 
way, so thanks for the understanding. This patch can be ignored, as they 
are all covered elsewhere.


We've all been there (messing with git send-email), no need to feel bad :)

--
Cheers,

David / dhildenb

Re: [PATCH] Updates to Xen hypercall preemption

[Peter Zijlstra]

On Wed, Jun 21, 2023 at 03:14:42PM +, Per Bilse wrote:
> Some Xen hypercalls issued by dom0 guests may run for many 10s of
> seconds, potentially causing watchdog timeouts and other problems.
> It's rare for this to happen, but it does in extreme circumstances,
> for instance when shutting down VMs with very large memory allocations
> (> 0.5 - 1TB).  These hypercalls are preemptible, but the fixes in the
> kernel to ensure preemption have fallen into a state of disrepair, and
> are currently ineffective.  This patch brings things up to date by way of:

I don't understand it -- fundamentally, how can linux schedule when the
guest isn't even running? Hypercall transfers control to the
host/hypervisor and leaves the guest suspended.

> 1) Update general feature selection from XEN_PV to XEN_DOM0.
> The issue is unique to dom0 Xen guests, but isn't unique to PV dom0s,
> and will occur in future PVH dom0s.  XEN_DOM0 depends on either PV or PVH,
> as well as the appropriate details for dom0.
> 
> 2) Update specific feature selection from !PREEMPTION to !PREEMPT.
> The following table shows the relationship between different preemption
> features and their indicators/selectors (Y = "=Y", N = "is not set",
> . = absent):
> 
> | np-s | np-d | vp-s | vp-d | fp-s | fp-d
> CONFIG_PREEMPT_DYNAMIC  N  Y  N  Y  N  Y
>  CONFIG_PREEMPTION  .  Y  .  Y  Y  Y
> CONFIG_PREEMPT  N  N  N  N  Y  Y
>   CONFIG_PREEMPT_VOLUNTARY  N  N  Y  Y  N  N
>CONFIG_PREEMPT_NONE  Y  Y  N  N  N  N
> 
> Unless PREEMPT is set, we need to enable the fixes.
> 
> 3) Update flag access from __this_cpu_XXX() to raw_cpu_XXX().
> The long-running hypercalls are flagged by way of a per-cpu variable
> which is set before and cleared after the relevant calls.  This elicits
> a warning "BUG: using __this_cpu_write() in preemptible [] code",
> but xen_pv_evtchn_do_upcall() deals specifically with this.  For
> consistency, flag testing is also updated, and the code is simplified
> and tidied accordingly.

This makes no sense; the race that warning warns about is:

CPU0CPU1
per-cpu write


do-hypercall

So you wrote the value on CPU0, got migrated to CPU1 because you had
preemptioned enabled, and then continue with the percpu value of CPU1
because that's where you're at now.

Simply making the warning go away doesn't help, CPU1 does hypercall
while store was on CPU0.

> 4) Update irqentry_exit_cond_resched() to raw_irqentry_exit_cond_resched().
> The code will call irqentry_exit_cond_resched() if the flag (as noted
> above) is set, but the dynamic preemption feature will livepatch that
> function to a no-op unless full preemption is selected.  The code is
> therefore updated to call raw_irqentry_exit_cond_resched().

That, again meeds more explanation. Why do you want this if not
preemptible?

You're doing 4 things, that should be 4 patches. Also, please give more
clues for how this is supposed to work at all.

Re: [PATCH v2 1/2] x86/boot: Clear XD_DISABLE from the early boot path

[Alejandro Vallejo]

Sure, to everything before this

> > diff --git a/xen/arch/x86/cpu/intel.c b/xen/arch/x86/cpu/intel.c
> > index 168cd58f36..46b0cd8dbb 100644
> > --- a/xen/arch/x86/cpu/intel.c
> > +++ b/xen/arch/x86/cpu/intel.c
> > @@ -305,23 +305,23 @@ static void cf_check early_init_intel(struct 
> > cpuinfo_x86 *c)
> > c->x86_cache_alignment = 128;
> >  
> > /* Unmask CPUID levels and NX if masked: */
> > -   rdmsrl(MSR_IA32_MISC_ENABLE, misc_enable);
> > -
> > -   disable = misc_enable & (MSR_IA32_MISC_ENABLE_LIMIT_CPUID |
> > -MSR_IA32_MISC_ENABLE_XD_DISABLE);
> > -   if (disable) {
> > -   wrmsrl(MSR_IA32_MISC_ENABLE, misc_enable & ~disable);
> > -   bootsym(trampoline_misc_enable_off) |= disable;
> > -   bootsym(trampoline_efer) |= EFER_NXE;
> > -   }
> > +   if (rdmsr_safe(MSR_IA32_MISC_ENABLE, misc_enable) == 0) {
> 
> There's no need to change rdmsrl() to rdmsr_safe(),
I thought we established before some hypervisors might not implement it. In
that case this function would crash. More gracefully than a triple fault,
sure, but still not a very friendly thing to do.

> and not doing so will shrink this diff a lot.
This particular one, yes. That said, looking more carefully I'd say I
should remove the XD_DISABLE parts of those lines altogether. It is
unconditionally cleared on boot now and will never ever have a non-zero
value at this point.

> The only thing you need to alter the re-enable NX printk(), which
> probably wants to move ahead of the "if (disable)" and source itself
> from bootsym(trampoline_misc_enable_off) instead.
Alternatively I could just get rid of the printk. As it is after this patch
even the BSP starts off with NX enabled.  EFER is updated as early as the
trampoline, which is as early as it would've happened before regardless of
XD_DISABLE.

Alejandro

Re: [PATCH v1 01/23] pc/xen: Xen Q35 support: provide IRQ handling for PCI devices

[Joel Upham]

Thank you, I was working off the Xen-devel and didn’t find his email. I
will update my qemu and xen patches for the next version.

-Joel

On Wed, Jun 21, 2023 at 3:17 AM Daniel P. Berrangé 
wrote:

> On Tue, Jun 20, 2023 at 01:24:34PM -0400, Joel Upham wrote:
> >
> > Signed-off-by: Alexey Gerasimenko 
>
> This isn't a valid email address for Alexey - I presume you grabbed
> these patches from the xen-devel mail archives, which have mangled
> the addresses for anti-spam reasons.
>
> Fortunately there are alternative archives which don't mangle the
> patches:
>
>
> https://lore.kernel.org/xen-devel/6067bc3c91c9ee629a35723dfb474ef168ff4ebf.1520867955.git.x19...@gmail.com/
>
>   Signed-off-by: Alexey Gerasimenko 
>
> This affects all patches in the series, but I won't repeat my
> comment on each one.
>
> > Signed-off-by: Joel Upham 
> > ---
> >  hw/i386/pc_piix.c |  3 +-
> >  hw/i386/xen/xen-hvm.c |  7 +++--
> >  hw/isa/lpc_ich9.c | 53 ---
> >  hw/isa/piix3.c|  2 +-
> >  include/hw/southbridge/ich9.h |  1 +
> >  include/hw/xen/xen.h  |  4 +--
> >  stubs/xen-hw-stub.c   |  4 +--
> >  7 files changed, 61 insertions(+), 13 deletions(-)
> >
> > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > index d5b0dcd1fe..8c1b20f3bc 100644
> > --- a/hw/i386/pc_piix.c
> > +++ b/hw/i386/pc_piix.c
> > @@ -62,6 +62,7 @@
> >  #endif
> >  #include "hw/xen/xen-x86.h"
> >  #include "hw/xen/xen.h"
> > +#include "sysemu/xen.h"
> >  #include "migration/global_state.h"
> >  #include "migration/misc.h"
> >  #include "sysemu/numa.h"
> > @@ -233,7 +234,7 @@ static void pc_init1(MachineState *machine,
> >x86ms->above_4g_mem_size,
> >pci_memory, ram_memory);
> >  pci_bus_map_irqs(pci_bus,
> > - xen_enabled() ? xen_pci_slot_get_pirq
> > + xen_enabled() ? xen_cmn_pci_slot_get_pirq
> > : pc_pci_slot_get_pirq);
> >  pcms->bus = pci_bus;
> >
> > diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
> > index 56641a550e..540ac46639 100644
> > --- a/hw/i386/xen/xen-hvm.c
> > +++ b/hw/i386/xen/xen-hvm.c
> > @@ -15,6 +15,7 @@
> >  #include "hw/pci/pci.h"
> >  #include "hw/pci/pci_host.h"
> >  #include "hw/i386/pc.h"
> > +#include "hw/southbridge/ich9.h"
> >  #include "hw/irq.h"
> >  #include "hw/hw.h"
> >  #include "hw/i386/apic-msidef.h"
> > @@ -136,14 +137,14 @@ typedef struct XenIOState {
> >  Notifier wakeup;
> >  } XenIOState;
> >
> > -/* Xen specific function for piix pci */
> > +/* Xen-specific functions for pci dev IRQ handling */
> >
> > -int xen_pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num)
> > +int xen_cmn_pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num)
> >  {
> >  return irq_num + (PCI_SLOT(pci_dev->devfn) << 2);
> >  }
> >
> > -void xen_piix3_set_irq(void *opaque, int irq_num, int level)
> > +void xen_cmn_set_irq(void *opaque, int irq_num, int level)
> >  {
> >  xen_set_pci_intx_level(xen_domid, 0, 0, irq_num >> 2,
> > irq_num & 3, level);
> > diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> > index 9c47a2f6c7..733a99d443 100644
> > --- a/hw/isa/lpc_ich9.c
> > +++ b/hw/isa/lpc_ich9.c
> > @@ -51,6 +51,9 @@
> >  #include "hw/core/cpu.h"
> >  #include "hw/nvram/fw_cfg.h"
> >  #include "qemu/cutils.h"
> > +#include "hw/xen/xen.h"
> > +#include "sysemu/xen.h"
> > +#include "hw/southbridge/piix.h"
> >  #include "hw/acpi/acpi_aml_interface.h"
> >  #include "trace.h"
> >
> > @@ -535,11 +538,49 @@ static int ich9_lpc_post_load(void *opaque, int
> version_id)
> >  return 0;
> >  }
> >
> > +static void ich9_lpc_config_write_xen(PCIDevice *d,
> > +  uint32_t addr, uint32_t val, int len)
> > +{
> > +static bool pirqe_f_warned = false;
> > +if (ranges_overlap(addr, len, ICH9_LPC_PIRQA_ROUT, 4)) {
> > +/* handle PIRQA..PIRQD routing */
> > +/* Scan for updates to PCI link routes (0x60-0x63). */
> > +int i;
> > +for (i = 0; i < len; i++) {
> > +uint8_t v = (val >> (8 * i)) & 0xff;
> > +if (v & 0x80) {
> > +v = 0;
> > +}
> > +v &= 0xf;
> > +if (((addr + i) >= PIIX_PIRQCA) && ((addr + i) <=
> PIIX_PIRQCD)) {
> > +xen_set_pci_link_route(addr + i - PIIX_PIRQCA, v);
> > +}
> > +}
> > +} else if (ranges_overlap(addr, len, ICH9_LPC_PIRQE_ROUT, 4)) {
> > +while (len--) {
> > +if (range_covers_byte(ICH9_LPC_PIRQE_ROUT, 4, addr) &&
> > +(val & 0x80) == 0) {
> > +/* print warning only once */
> > +if (!pirqe_f_warned) {
> > +pirqe_f_warned = true;
> > +fprintf(stderr, "WARNING: guest domain attempted to
> use PIRQ%c "
> > +   

Re: [patch 05/37] x86/topology: Remove CPU0 hotplug option

[Paul E. McKenney]

On Sat, Apr 15, 2023 at 01:44:21AM +0200, Thomas Gleixner wrote:
> This was introduced together with commit e1c467e69040 ("x86, hotplug: Wake
> up CPU0 via NMI instead of INIT, SIPI, SIPI") to eventually support
> physical hotplug of CPU0:
> 
>  "We'll change this code in the future to wake up hard offlined CPU0 if
>   real platform and request are available."
> 
> 11 years later this has not happened and physical hotplug is not officially
> supported. Remove the cruft.
> 
> Signed-off-by: Thomas Gleixner 
> ---
>  Documentation/admin-guide/kernel-parameters.txt |   14 ---
>  Documentation/core-api/cpu_hotplug.rst  |   13 ---
>  arch/x86/Kconfig|   43 --
>  arch/x86/include/asm/cpu.h  |3 
>  arch/x86/kernel/topology.c  |   98 
> 
>  arch/x86/power/cpu.c|   37 -
>  6 files changed, 6 insertions(+), 202 deletions(-)

[ . . . ]

> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -2294,49 +2294,6 @@ config HOTPLUG_CPU
>   def_bool y
>   depends on SMP
>  
> -config BOOTPARAM_HOTPLUG_CPU0

Removing this requires also removing its use in rcutorture.

I have therefore queued the commit below in -rcu, but please feel
free to take it along with the BOOTPARAM_HOTPLUG_CPU0-removal patch.
Just please let me know if you do.

(Yes, I finally got back to testing -next.  Why do you ask?)

Thanx, Paul



commit 95588de780c0e81004b72526aa3e3ef5ce054719
Author: Paul E. McKenney 
Date:   Wed Jun 21 09:44:52 2023 -0700

rcutorture: Remove obsolete BOOTPARAM_HOTPLUG_CPU0 Kconfig option

Now that the BOOTPARAM_HOTPLUG_CPU0 Kconfig option is in the process of
being removed, it is time to remove rcutorture's use of it.

Link: https://lore.kernel.org/lkml/20230414232309.510911...@linutronix.de/
Signed-off-by: Paul E. McKenney 
Cc: Thomas Gleixner 
Cc: 

diff --git a/tools/testing/selftests/rcutorture/configs/rcu/TREE01 
b/tools/testing/selftests/rcutorture/configs/rcu/TREE01
index 04831ef1f9b5..8ae41d5f81a3 100644
--- a/tools/testing/selftests/rcutorture/configs/rcu/TREE01
+++ b/tools/testing/selftests/rcutorture/configs/rcu/TREE01
@@ -15,4 +15,3 @@ CONFIG_DEBUG_LOCK_ALLOC=n
 CONFIG_RCU_BOOST=n
 CONFIG_DEBUG_OBJECTS_RCU_HEAD=n
 CONFIG_RCU_EXPERT=y
-CONFIG_BOOTPARAM_HOTPLUG_CPU0=y

[PATCH 2/4] automation: Fix KBUILD_DEFCONFIG for *ppc64le jobs

[Shawn Anastasio]

During an iteration of the initial ppc64le support patchset the default
defconfig was renamed but build.yaml wasn't updated to reflect this. Fix
it up.

Signed-off-by: Shawn Anastasio 
---
 automation/gitlab-ci/build.yaml | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/automation/gitlab-ci/build.yaml b/automation/gitlab-ci/build.yaml
index bd8c7332db..c401f62d61 100644
--- a/automation/gitlab-ci/build.yaml
+++ b/automation/gitlab-ci/build.yaml
@@ -548,21 +548,21 @@ debian-bullseye-gcc-ppc64le:
   extends: .gcc-ppc64le-cross-build
   variables:
 CONTAINER: debian:bullseye-ppc64le
-KBUILD_DEFCONFIG: openpower_defconfig
+KBUILD_DEFCONFIG: ppc64_defconfig
 HYPERVISOR_ONLY: y
 
 debian-bullseye-gcc-ppc64le-debug:
   extends: .gcc-ppc64le-cross-build-debug
   variables:
 CONTAINER: debian:bullseye-ppc64le
-KBUILD_DEFCONFIG: openpower_defconfig
+KBUILD_DEFCONFIG: ppc64_defconfig
 HYPERVISOR_ONLY: y
 
 debian-bullseye-gcc-ppc64le-randconfig:
   extends: .gcc-ppc64le-cross-build
   variables:
 CONTAINER: debian:bullseye-ppc64le
-KBUILD_DEFCONFIG: openpower_defconfig
+KBUILD_DEFCONFIG: ppc64_defconfig
 RANDCONFIG: y
 EXTRA_FIXED_RANDCONFIG:
   CONFIG_COVERAGE=n
@@ -571,7 +571,7 @@ debian-bullseye-gcc-ppc64le-debug-randconfig:
   extends: .gcc-ppc64le-cross-build-debug
   variables:
 CONTAINER: debian:bullseye-ppc64le
-KBUILD_DEFCONFIG: openpower_defconfig
+KBUILD_DEFCONFIG: ppc64_defconfig
 RANDCONFIG: y
 EXTRA_FIXED_RANDCONFIG:
   CONFIG_COVERAGE=n
-- 
2.30.2

[PATCH 4/4] automation: Add smoke test for ppc64le

[Shawn Anastasio]

Add an initial smoke test that boots xen on a ppc64/pseries machine and
checks for a magic string. Based on the riscv smoke test.

Eventually the powernv9 (POWER9 bare metal) machine type will want to be
tested as well, but for now we only boot on pseries.

Signed-off-by: Shawn Anastasio 
---
 automation/gitlab-ci/test.yaml   | 20 ++
 automation/scripts/qemu-smoke-ppc64le.sh | 27 
 2 files changed, 47 insertions(+)
 create mode 100755 automation/scripts/qemu-smoke-ppc64le.sh

diff --git a/automation/gitlab-ci/test.yaml b/automation/gitlab-ci/test.yaml
index d5cb238b0a..45e8ddb7a3 100644
--- a/automation/gitlab-ci/test.yaml
+++ b/automation/gitlab-ci/test.yaml
@@ -71,6 +71,19 @@
   tags:
 - x86_64
 
+.qemu-ppc64le:
+  extends: .test-jobs-common
+  variables:
+CONTAINER: debian:bullseye-ppc64le
+LOGFILE: qemu-smoke-ppc64le.log
+  artifacts:
+paths:
+  - smoke.serial
+  - '*.log'
+when: always
+  tags:
+- x86_64
+
 .xilinx-arm64:
   extends: .test-jobs-common
   variables:
@@ -444,3 +457,10 @@ qemu-smoke-riscv64-gcc:
 - ./automation/scripts/qemu-smoke-riscv64.sh 2>&1 | tee ${LOGFILE}
   needs:
 - archlinux-current-gcc-riscv64-debug
+
+qemu-smoke-ppc64le-pseries-gcc:
+  extends: .qemu-ppc64le
+  script:
+- ./automation/scripts/qemu-smoke-ppc64le.sh pseries-5.2 2>&1 | tee 
${LOGFILE}
+  needs:
+- debian-bullseye-gcc-ppc64le-debug
diff --git a/automation/scripts/qemu-smoke-ppc64le.sh 
b/automation/scripts/qemu-smoke-ppc64le.sh
new file mode 100755
index 00..eb55221221
--- /dev/null
+++ b/automation/scripts/qemu-smoke-ppc64le.sh
@@ -0,0 +1,27 @@
+#!/bin/bash
+
+set -ex
+
+# machine type from first arg passed directly to qemu -M
+machine=$1
+
+# Run the test
+rm -f smoke.serial
+set +e
+
+touch smoke.serial
+
+timeout -k 1 20 \
+qemu-system-ppc64 \
+-M $machine \
+-m 2g \
+-smp 1 \
+-vga none \
+-monitor none \
+-nographic \
+-serial file:smoke.serial \
+-kernel binaries/xen
+
+set -e
+(grep -q "Hello, ppc64le!" smoke.serial) || exit 1
+exit 0
-- 
2.30.2

[PATCH 1/4] automation: Add QEMU to bullseye-ppc64le

[Shawn Anastasio]

Add qemu-system-ppc package to the bullseye-ppc64le container to allow
running smoke tests in CI.

Signed-off-by: Shawn Anastasio 
---
 automation/build/debian/bullseye-ppc64le.dockerfile | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/automation/build/debian/bullseye-ppc64le.dockerfile 
b/automation/build/debian/bullseye-ppc64le.dockerfile
index 8a87631b52..8fad26e903 100644
--- a/automation/build/debian/bullseye-ppc64le.dockerfile
+++ b/automation/build/debian/bullseye-ppc64le.dockerfile
@@ -22,6 +22,8 @@ RUN apt-get update && \
 gcc-powerpc64le-linux-gnu \
 make \
 python3-minimal \
+# for test phase
+qemu-system-ppc \
 && \
 apt-get autoremove -y && \
 apt-get clean && \
-- 
2.30.2

[PATCH 0/4] Early serial on Power

[Shawn Anastasio]

Hello all,

This series adds support for early serial printing on Power, as well as
a simple CI smoke test modeled after the riscv one.

The third patch, which implements the serial console, also includes a
set of header additions required to get the build going when including
things like xen/lib.h. When applicable, headers from the old Xen 3.2.3
ppc tree were imported or otherwise empty stub headers were added to be
filled in later.

This will currently only run on QEMU pseries VMs, since the firmware
interface on bare metal differs significantly. Support for bare metal
will be added in a future series.

Thanks,
Shawn

Shawn Anastasio (4):
  automation: Add QEMU to bullseye-ppc64le
  automation: Fix KBUILD_DEFCONFIG for *ppc64le jobs
  xen/ppc: Implement early serial printk on pseries
  automation: Add smoke test for ppc64le

 .../build/debian/bullseye-ppc64le.dockerfile  |   2 +
 automation/gitlab-ci/build.yaml   |   8 +-
 automation/gitlab-ci/test.yaml|  20 ++
 automation/scripts/qemu-smoke-ppc64le.sh  |  27 +++
 xen/arch/ppc/Kconfig.debug|   5 +
 xen/arch/ppc/Makefile |   3 +
 xen/arch/ppc/boot-of.c| 116 ++
 xen/arch/ppc/configs/ppc64_defconfig  |   1 +
 xen/arch/ppc/early_printk.c   |  28 +++
 xen/arch/ppc/include/asm/boot.h   |  24 ++
 xen/arch/ppc/include/asm/bug.h|   6 +
 xen/arch/ppc/include/asm/byteorder.h  |  37 +++
 xen/arch/ppc/include/asm/cache.h  |   6 +
 xen/arch/ppc/include/asm/early_printk.h   |  15 ++
 xen/arch/ppc/include/asm/msr.h|  67 ++
 xen/arch/ppc/include/asm/processor.h  | 207 +
 xen/arch/ppc/include/asm/reg_defs.h   | 218 ++
 xen/arch/ppc/include/asm/string.h |   6 +
 xen/arch/ppc/include/asm/types.h  |  35 +++
 xen/arch/ppc/ppc64/Makefile   |   1 +
 xen/arch/ppc/ppc64/asm-offsets.c  |  55 +
 xen/arch/ppc/ppc64/head.S |  48 ++--
 xen/arch/ppc/ppc64/of-call.S  |  85 +++
 xen/arch/ppc/setup.c  |  31 +++
 24 files changed, 1025 insertions(+), 26 deletions(-)
 create mode 100755 automation/scripts/qemu-smoke-ppc64le.sh
 create mode 100644 xen/arch/ppc/boot-of.c
 create mode 100644 xen/arch/ppc/early_printk.c
 create mode 100644 xen/arch/ppc/include/asm/boot.h
 create mode 100644 xen/arch/ppc/include/asm/bug.h
 create mode 100644 xen/arch/ppc/include/asm/byteorder.h
 create mode 100644 xen/arch/ppc/include/asm/cache.h
 create mode 100644 xen/arch/ppc/include/asm/early_printk.h
 create mode 100644 xen/arch/ppc/include/asm/msr.h
 create mode 100644 xen/arch/ppc/include/asm/processor.h
 create mode 100644 xen/arch/ppc/include/asm/reg_defs.h
 create mode 100644 xen/arch/ppc/include/asm/string.h
 create mode 100644 xen/arch/ppc/include/asm/types.h
 create mode 100644 xen/arch/ppc/ppc64/of-call.S
 create mode 100644 xen/arch/ppc/setup.c

--
2.30.2

[PATCH 3/4] xen/ppc: Implement early serial printk on pseries

[Shawn Anastasio]

On typical Power VMs (e.g. QEMU's -M pseries), a variety of services
including an early serial console are provided by Open Firmware.
Implement the required interfaces to call into Open Firmware and write
to the serial console.

Since Open Firmware runs in 32-bit Big Endian mode and Xen runs in
64-bit Little Endian mode, a thunk is required to save/restore
any potentially-clobbered registers as well as to perform the
required endianness switch. Thankfully, linux already has such
a routine, which was imported into ppc64/of-call.S.

Support for bare metal (PowerNV) will be implemented in a future
patch.

Signed-off-by: Shawn Anastasio 
---
 xen/arch/ppc/Kconfig.debug  |   5 +
 xen/arch/ppc/Makefile   |   3 +
 xen/arch/ppc/boot-of.c  | 116 +
 xen/arch/ppc/configs/ppc64_defconfig|   1 +
 xen/arch/ppc/early_printk.c |  28 +++
 xen/arch/ppc/include/asm/boot.h |  24 +++
 xen/arch/ppc/include/asm/bug.h  |   6 +
 xen/arch/ppc/include/asm/byteorder.h|  37 
 xen/arch/ppc/include/asm/cache.h|   6 +
 xen/arch/ppc/include/asm/early_printk.h |  15 ++
 xen/arch/ppc/include/asm/msr.h  |  67 
 xen/arch/ppc/include/asm/processor.h| 207 ++
 xen/arch/ppc/include/asm/reg_defs.h | 218 
 xen/arch/ppc/include/asm/string.h   |   6 +
 xen/arch/ppc/include/asm/types.h|  35 
 xen/arch/ppc/ppc64/Makefile |   1 +
 xen/arch/ppc/ppc64/asm-offsets.c|  55 ++
 xen/arch/ppc/ppc64/head.S   |  48 +++---
 xen/arch/ppc/ppc64/of-call.S|  85 +
 xen/arch/ppc/setup.c|  31 
 20 files changed, 972 insertions(+), 22 deletions(-)
 create mode 100644 xen/arch/ppc/boot-of.c
 create mode 100644 xen/arch/ppc/early_printk.c
 create mode 100644 xen/arch/ppc/include/asm/boot.h
 create mode 100644 xen/arch/ppc/include/asm/bug.h
 create mode 100644 xen/arch/ppc/include/asm/byteorder.h
 create mode 100644 xen/arch/ppc/include/asm/cache.h
 create mode 100644 xen/arch/ppc/include/asm/early_printk.h
 create mode 100644 xen/arch/ppc/include/asm/msr.h
 create mode 100644 xen/arch/ppc/include/asm/processor.h
 create mode 100644 xen/arch/ppc/include/asm/reg_defs.h
 create mode 100644 xen/arch/ppc/include/asm/string.h
 create mode 100644 xen/arch/ppc/include/asm/types.h
 create mode 100644 xen/arch/ppc/ppc64/of-call.S
 create mode 100644 xen/arch/ppc/setup.c

diff --git a/xen/arch/ppc/Kconfig.debug b/xen/arch/ppc/Kconfig.debug
index e69de29bb2..608c9ff832 100644
--- a/xen/arch/ppc/Kconfig.debug
+++ b/xen/arch/ppc/Kconfig.debug
@@ -0,0 +1,5 @@
+config EARLY_PRINTK
+bool "Enable early printk"
+default DEBUG
+help
+  Enables early printk debug messages
diff --git a/xen/arch/ppc/Makefile b/xen/arch/ppc/Makefile
index 98220648af..4c4cdcb60e 100644
--- a/xen/arch/ppc/Makefile
+++ b/xen/arch/ppc/Makefile
@@ -1,4 +1,7 @@
 obj-$(CONFIG_PPC64) += ppc64/
+obj-y += setup.o
+obj-y += boot-of.o
+obj-$(CONFIG_EARLY_PRINTK) += early_printk.o
 
 $(TARGET): $(TARGET)-syms
cp -f $< $@
diff --git a/xen/arch/ppc/boot-of.c b/xen/arch/ppc/boot-of.c
new file mode 100644
index 00..1ceeaf1250
--- /dev/null
+++ b/xen/arch/ppc/boot-of.c
@@ -0,0 +1,116 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ *
+ * Copyright IBM Corp. 2005, 2006, 2007
+ * Copyright Raptor Engineering, LLC
+ *
+ * Authors: Jimi Xenidis 
+ *  Hollis Blanchard 
+ *  Shawn Anastasio 
+ */
+
+#include 
+#include 
+#include 
+#include 
+
+#define ADDR(x) (uint32_t)(unsigned long)(x)
+
+/* OF entrypoint*/
+static unsigned long of_vec;
+
+/* OF device handles*/
+static int bof_chosen;
+static int of_out;
+
+static int of_call(const char *service, uint32_t nargs, uint32_t nrets,
+   int32_t rets[], ...)
+{
+int rc;
+va_list args;
+int i;
+struct of_service s = { 0 };
+
+s.ofs_service = cpu_to_be32(ADDR(service));
+s.ofs_nargs = cpu_to_be32(nargs);
+s.ofs_nrets = cpu_to_be32(nrets);
+
+/* copy all the params into the args array */
+va_start(args, rets);
+
+for ( i = 0; i < nargs; i++ )
+{
+s.ofs_args[i] = cpu_to_be32(va_arg(args, uint32_t

[linux-linus test] 181529: regressions - trouble: blocked/broken/fail/pass

[osstest service owner]

flight 181529 linux-linus real [real]
http://logs.test-lab.xenproject.org/osstest/logs/181529/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 build-armhf  broken
 build-armhf   4 host-install(4)broken REGR. vs. 180278
 test-amd64-amd64-libvirt-qcow2 19 guest-start/debian.repeat fail REGR. vs. 
180278

Tests which did not succeed, but are not blocking:
 test-armhf-armhf-examine  1 build-check(1)   blocked  n/a
 test-armhf-armhf-libvirt  1 build-check(1)   blocked  n/a
 test-armhf-armhf-libvirt-qcow2  1 build-check(1)   blocked  n/a
 test-armhf-armhf-libvirt-raw  1 build-check(1)   blocked  n/a
 test-armhf-armhf-xl   1 build-check(1)   blocked  n/a
 test-armhf-armhf-xl-arndale   1 build-check(1)   blocked  n/a
 test-armhf-armhf-xl-credit1   1 build-check(1)   blocked  n/a
 test-armhf-armhf-xl-credit2   1 build-check(1)   blocked  n/a
 test-armhf-armhf-xl-multivcpu  1 build-check(1)   blocked  n/a
 test-armhf-armhf-xl-rtds  1 build-check(1)   blocked  n/a
 test-armhf-armhf-xl-vhd   1 build-check(1)   blocked  n/a
 build-armhf-libvirt   1 build-check(1)   blocked  n/a
 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 180278
 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 180278
 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 180278
 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 180278
 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 180278
 test-amd64-amd64-libvirt 15 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail never pass
 test-arm64-arm64-xl-xsm  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  16 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-raw 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-amd64-amd64-libvirt-qcow2 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

version targeted for testing:
 linuxe660abd551f1172e428b4e4003de887176a8a1fd
baseline version:
 linux6c538e1adbfc696ac4747fb10d63e704344f763d

Last test of basis   180278  2023-04-16 19:41:46 Z   65 days
Failing since180281  2023-04-17 06:24:36 Z   65 days  123 attempts
Testing same since   181529  2023-06-21 02:27:06 Z0 days1 attempts


2743 people touched revisions under test,
not listing them all

jobs:
 build-amd64-xsm  pass
 build-arm64-xsm  pass
 build-i386-xsm   pass
 build-amd64  pass
 build-arm64  pass
 build-armhf  broken  
 build-i386   pass
 build-amd64-libvirt  pass
 build-arm64-libvirt  pass
 build-armhf-libvirt  blocked 
 build-i386-libvirt   pass
 build-amd64-pvopspass
 build-arm64-pvopspass
 build-armhf-pvopspass
 build-i386-pvops

Re: [PATCH 3/4] xen/ppc: Implement early serial printk on pseries

[Andrew Cooper]

On 21/06/2023 5:59 pm, Shawn Anastasio wrote:
>  xen/arch/ppc/Kconfig.debug  |   5 +
>  xen/arch/ppc/Makefile   |   3 +
>  xen/arch/ppc/boot-of.c  | 116 +
>  xen/arch/ppc/configs/ppc64_defconfig|   1 +
>  xen/arch/ppc/early_printk.c |  28 +++
>  xen/arch/ppc/include/asm/boot.h |  24 +++
>  xen/arch/ppc/include/asm/bug.h  |   6 +
>  xen/arch/ppc/include/asm/byteorder.h|  37 
>  xen/arch/ppc/include/asm/cache.h|   6 +
>  xen/arch/ppc/include/asm/early_printk.h |  15 ++
>  xen/arch/ppc/include/asm/msr.h  |  67 
>  xen/arch/ppc/include/asm/processor.h| 207 ++
>  xen/arch/ppc/include/asm/reg_defs.h | 218 
>  xen/arch/ppc/include/asm/string.h   |   6 +
>  xen/arch/ppc/include/asm/types.h|  35 
>  xen/arch/ppc/ppc64/Makefile |   1 +
>  xen/arch/ppc/ppc64/asm-offsets.c|  55 ++
>  xen/arch/ppc/ppc64/head.S   |  48 +++---
>  xen/arch/ppc/ppc64/of-call.S|  85 +
>  xen/arch/ppc/setup.c|  31 
>  20 files changed, 972 insertions(+), 22 deletions(-)
>  create mode 100644 xen/arch/ppc/boot-of.c
>  create mode 100644 xen/arch/ppc/early_printk.c
>  create mode 100644 xen/arch/ppc/include/asm/boot.h
>  create mode 100644 xen/arch/ppc/include/asm/bug.h
>  create mode 100644 xen/arch/ppc/include/asm/byteorder.h
>  create mode 100644 xen/arch/ppc/include/asm/cache.h
>  create mode 100644 xen/arch/ppc/include/asm/early_printk.h
>  create mode 100644 xen/arch/ppc/include/asm/msr.h
>  create mode 100644 xen/arch/ppc/include/asm/processor.h
>  create mode 100644 xen/arch/ppc/include/asm/reg_defs.h
>  create mode 100644 xen/arch/ppc/include/asm/string.h
>  create mode 100644 xen/arch/ppc/include/asm/types.h
>  create mode 100644 xen/arch/ppc/ppc64/of-call.S
>  create mode 100644 xen/arch/ppc/setup.c

This is a surprising amount of infrastructure.  I'm guessing it's a
consequence of needing byteorder ?

There's a series still out deleting swathes of junk in byteorder.  I
guess I need to kick that thread again, but it mostly boils down to
using __builtin_bswap$N() (and on x86, reimplementing them on old enough
compilers).  Presumably all versions of GCC (and eventually Clang) we
care to support with ppc64 understand this builtin ?

I've noticed a couple of other things.  asm/types.h repeats some
antipatterns which we're trying to delete for MISRA reasons in other
architectures.  I was already planning to fix that up xen-wide, and I
guess now is the better time to do so.

Elsewhere, you've got a number of __inline__'s.  We think those are all
vestigial now, so should be switched to using a plain inline.

Also, there are a bunch of UL() or ULL() macros which encoding a
difference between asm and C.  In Xen, we use _AC() for that, which you
can find in .

Similarly, there are some functions which ought to be __init, so it
would be good to get them correct from the start.


Maybe as an intermediate step, just get the infinite loop moved from asm
up into C?  That gets the stack setup, and various of the asm helpers,
without all the rest of the C required to get early_printk() to work.

Something we've been trying very hard to do generally is declutter
processor.h, and on x86, we've got asm-defns.h as a more appropriate
place to have the stuff which is expected to be common to all asm code,
and never encountered in C.

A couple of questions before I dive further in.

Given:

#define r0  0

do the assemblers really not understand register names?  That seems mad.

Also, given the transformations to call into OpenFirmware, presumably
this limits Xen to running below the 4G boundary and on identity mappings?

~Andrew

[qemu-mainline test] 181534: regressions - trouble: blocked/broken/fail/pass

[osstest service owner]

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

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 build-armhf  broken
 build-armhf   4 host-install(4)broken REGR. vs. 180691
 build-amd64   6 xen-buildfail REGR. vs. 180691
 build-amd64-xsm   6 xen-buildfail REGR. vs. 180691
 build-i3866 xen-buildfail REGR. vs. 180691
 build-i386-xsm6 xen-buildfail REGR. vs. 180691
 build-arm64   6 xen-buildfail REGR. vs. 180691
 build-arm64-xsm   6 xen-buildfail REGR. vs. 180691

Tests which did not succeed, but are not blocking:
 test-amd64-i386-xl-qemuu-debianhvm-amd64-shadow  1 build-check(1)  blocked n/a
 test-amd64-i386-xl-qemuu-debianhvm-i386-xsm  1 build-check(1)  blocked n/a
 test-amd64-i386-xl-qemuu-dmrestrict-amd64-dmrestrict 1 build-check(1) blocked 
n/a
 test-amd64-i386-xl-qemuu-ovmf-amd64  1 build-check(1)  blocked n/a
 test-amd64-i386-xl-qemuu-win7-amd64  1 build-check(1)  blocked n/a
 test-amd64-i386-xl-qemuu-ws16-amd64  1 build-check(1)  blocked n/a
 test-amd64-i386-xl-shadow 1 build-check(1)   blocked  n/a
 test-amd64-i386-xl-vhd1 build-check(1)   blocked  n/a
 test-amd64-i386-xl-xsm1 build-check(1)   blocked  n/a
 test-arm64-arm64-libvirt-raw  1 build-check(1)   blocked  n/a
 test-arm64-arm64-libvirt-xsm  1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl   1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl-credit1   1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl-credit2   1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl-thunderx  1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl-vhd   1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl-xsm   1 build-check(1)   blocked  n/a
 test-armhf-armhf-libvirt  1 build-check(1)   blocked  n/a
 test-armhf-armhf-libvirt-qcow2  1 build-check(1)   blocked  n/a
 test-armhf-armhf-libvirt-raw  1 build-check(1)   blocked  n/a
 test-armhf-armhf-xl   1 build-check(1)   blocked  n/a
 test-armhf-armhf-xl-arndale   1 build-check(1)   blocked  n/a
 test-armhf-armhf-xl-multivcpu  1 build-check(1)   blocked  n/a
 test-armhf-armhf-xl-rtds  1 build-check(1)   blocked  n/a
 test-armhf-armhf-xl-vhd   1 build-check(1)   blocked  n/a
 test-amd64-i386-qemuu-rhel6hvm-amd  1 build-check(1)   blocked n/a
 test-amd64-i386-pair  1 build-check(1)   blocked  n/a
 test-amd64-i386-libvirt-xsm   1 build-check(1)   blocked  n/a
 test-amd64-i386-libvirt-raw   1 build-check(1)   blocked  n/a
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 1 build-check(1) blocked n/a
 test-amd64-i386-libvirt-pair  1 build-check(1)   blocked  n/a
 test-amd64-i386-libvirt   1 build-check(1)   blocked  n/a
 test-amd64-i386-freebsd10-i386  1 build-check(1)   blocked  n/a
 test-amd64-i386-freebsd10-amd64  1 build-check(1)   blocked  n/a
 test-amd64-coresched-i386-xl  1 build-check(1)   blocked  n/a
 test-amd64-coresched-amd64-xl  1 build-check(1)   blocked  n/a
 test-amd64-amd64-xl-xsm   1 build-check(1)   blocked  n/a
 build-amd64-libvirt   1 build-check(1)   blocked  n/a
 test-amd64-amd64-xl-shadow1 build-check(1)   blocked  n/a
 test-amd64-amd64-xl-rtds  1 build-check(1)   blocked  n/a
 build-arm64-libvirt   1 build-check(1)   blocked  n/a
 test-amd64-amd64-xl-qemuu-ws16-amd64  1 build-check(1) blocked n/a
 test-amd64-amd64-xl-qemuu-win7-amd64  1 build-check(1) blocked n/a
 test-amd64-amd64-xl-qemuu-ovmf-amd64  1 build-check(1) blocked n/a
 build-armhf-libvirt   1 build-check(1)   blocked  n/a
 test-amd64-amd64-xl-qemuu-dmrestrict-amd64-dmrestrict 1 build-check(1) blocked 
n/a
 build-i386-libvirt1 build-check(1)   blocked  n/a
 test-amd64-amd64-xl-qemuu-debianhvm-i386-xsm  1 build-check(1) blocked n/a
 test-amd64-amd64-dom0pvh-xl-amd  1 build-check(1)   blocked  n/a
 test-amd64-amd64-xl-qemuu-debianhvm-amd64-shadow  1 build-check(1) blocked n/a
 test-amd64-amd64-dom0pvh-xl-intel  1 build-check(1)   blocked  n/a
 test-amd64-amd64-xl-qemuu-debianhvm-amd64  1 build-check(1)blocked n/a
 test-amd64-amd64-libvirt  1 build-check(1)   blocked  n/a
 test-amd64-amd64-libvirt-pair  1 build-check(1) 

Re: [PATCH 2/2] xen/virtio: Avoid use of the dom0 backend in dom0

[Oleksandr Tyshchenko]

On 21.06.23 16:12, Petr Pavlu wrote:


Hello Petr


> When attempting to run Xen on a QEMU/KVM virtual machine with virtio
> devices (all x86_64), dom0 tries to establish a grant for itself which
> eventually results in a hang during the boot.
> 
> The backtrace looks as follows, the while loop in __send_control_msg()
> makes no progress:
> 
>#0  virtqueue_get_buf_ctx (_vq=_vq@entry=0x8880074a8400, 
> len=len@entry=0xc9413c94, ctx=ctx@entry=0x0 ) at 
> ../drivers/virtio/virtio_ring.c:2326
>#1  0x817086b7 in virtqueue_get_buf 
> (_vq=_vq@entry=0x8880074a8400, len=len@entry=0xc9413c94) at 
> ../drivers/virtio/virtio_ring.c:2333
>#2  0x8175f6b2 in __send_control_msg (portdev=, 
> port_id=0x, event=0x0, value=0x1) at 
> ../drivers/char/virtio_console.c:562
>#3  0x8175f6ee in __send_control_msg (portdev=, 
> port_id=, event=, value=) at 
> ../drivers/char/virtio_console.c:569
>#4  0x817618b1 in virtcons_probe (vdev=0x88800585e800) at 
> ../drivers/char/virtio_console.c:2098
>#5  0x81707117 in virtio_dev_probe (_d=0x88800585e810) at 
> ../drivers/virtio/virtio.c:305
>#6  0x8198e348 in call_driver_probe (drv=0x82be40c0 
> , drv=0x82be40c0 , 
> dev=0x88800585e810) at ../drivers/base/dd.c:579
>#7  really_probe (dev=dev@entry=0x88800585e810, 
> drv=drv@entry=0x82be40c0 ) at ../drivers/base/dd.c:658
>#8  0x8198e58f in __driver_probe_device 
> (drv=drv@entry=0x82be40c0 , 
> dev=dev@entry=0x88800585e810) at ../drivers/base/dd.c:800
>#9  0x8198e65a in driver_probe_device 
> (drv=drv@entry=0x82be40c0 , 
> dev=dev@entry=0x88800585e810) at ../drivers/base/dd.c:830
>#10 0x8198e832 in __driver_attach (dev=0x88800585e810, 
> data=0x82be40c0 ) at ../drivers/base/dd.c:1216
>#11 0x8198bfb2 in bus_for_each_dev (bus=, 
> start=start@entry=0x0 , data=data@entry=0x82be40c0 
> ,
>fn=fn@entry=0x8198e7b0 <__driver_attach>) at 
> ../drivers/base/bus.c:368
>#12 0x8198db65 in driver_attach (drv=drv@entry=0x82be40c0 
> ) at ../drivers/base/dd.c:1233
>#13 0x8198d207 in bus_add_driver (drv=drv@entry=0x82be40c0 
> ) at ../drivers/base/bus.c:673
>#14 0x8198f550 in driver_register 
> (drv=drv@entry=0x82be40c0 ) at 
> ../drivers/base/driver.c:246
>#15 0x81706b47 in register_virtio_driver 
> (driver=driver@entry=0x82be40c0 ) at 
> ../drivers/virtio/virtio.c:357
>#16 0x832cd34b in virtio_console_init () at 
> ../drivers/char/virtio_console.c:2258
>#17 0x8100105c in do_one_initcall (fn=0x832cd2e0 
> ) at ../init/main.c:1246
>#18 0x83277293 in do_initcall_level 
> (command_line=0x888003e2f900 "root", level=0x6) at ../init/main.c:1319
>#19 do_initcalls () at ../init/main.c:1335
>#20 do_basic_setup () at ../init/main.c:1354
>#21 kernel_init_freeable () at ../init/main.c:1571
>#22 0x81f64be1 in kernel_init (unused=) at 
> ../init/main.c:1462
>#23 0x81001f49 in ret_from_fork () at 
> ../arch/x86/entry/entry_64.S:308
>#24 0x in ?? ()
> 
> Fix the problem by preventing xen_grant_init_backend_domid() from
> setting dom0 as a backend when running in dom0.
> 
> Fixes: 035e3a4321f7 ("xen/virtio: Optimize the setup of "xen-grant-dma" 
> devices")


I am not 100% sure whether the Fixes tag points to precise commit. If I 
am not mistaken, the said commit just moves the code in the context 
without changing the logic of CONFIG_XEN_VIRTIO_FORCE_GRANT, this was 
introduced before.


> Signed-off-by: Petr Pavlu 
> ---
>   drivers/xen/grant-dma-ops.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
> index 76f6f26265a3..29ed27ac450e 100644
> --- a/drivers/xen/grant-dma-ops.c
> +++ b/drivers/xen/grant-dma-ops.c
> @@ -362,7 +362,9 @@ static int xen_grant_init_backend_domid(struct device 
> *dev,
>   if (np) {
>   ret = xen_dt_grant_init_backend_domid(dev, np, backend_domid);
>   of_node_put(np);
> - } else if (IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT) || 
> xen_pv_domain()) {
> + } else if ((IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT) ||
> + xen_pv_domain()) &&
> +!xen_initial_domain()) {

The commit lgtm, just one note:


I would even bail out early in xen_virtio_restricted_mem_acc() instead,
as I assume the same issue could happen on Arm with DT (although there 
we don't guess the backend's domid, we read it from DT and quite 
unlikely we get Dom0 being in Dom0 with correct DT).

Something like:

@@ -416,6 +421,10 @@ bool xen_virtio_restricted_mem_acc(struct 
virtio_device *dev)
  {
 domid_t backend_domid;

+   /* Xen grant DMA ops are not used when running as initial

Re: [PATCH 1/4] automation: Add QEMU to bullseye-ppc64le

[Andrew Cooper]

On 21/06/2023 5:59 pm, Shawn Anastasio wrote:
> Add qemu-system-ppc package to the bullseye-ppc64le container to allow
> running smoke tests in CI.
>
> Signed-off-by: Shawn Anastasio 

Acked-by: Andrew Cooper 

Sorry - should have remembered to check on the previous patch.

I'll commit and deploy this right away.

Re: [PATCH 2/4] automation: Fix KBUILD_DEFCONFIG for *ppc64le jobs

[Andrew Cooper]

On 21/06/2023 5:59 pm, Shawn Anastasio wrote:
> During an iteration of the initial ppc64le support patchset the default
> defconfig was renamed but build.yaml wasn't updated to reflect this. Fix
> it up.
>
> Signed-off-by: Shawn Anastasio 

Acked-by: Andrew Cooper 

I'll commit this too.  Clearly it's not fatal to build testing.

Re: [PATCH 3/4] xen/ppc: Implement early serial printk on pseries

[Shawn Anastasio]

On 6/21/23 12:54 PM, Andrew Cooper wrote:
> On 21/06/2023 5:59 pm, Shawn Anastasio wrote:
>>  xen/arch/ppc/Kconfig.debug  |   5 +
>>  xen/arch/ppc/Makefile   |   3 +
>>  xen/arch/ppc/boot-of.c  | 116 +
>>  xen/arch/ppc/configs/ppc64_defconfig|   1 +
>>  xen/arch/ppc/early_printk.c |  28 +++
>>  xen/arch/ppc/include/asm/boot.h |  24 +++
>>  xen/arch/ppc/include/asm/bug.h  |   6 +
>>  xen/arch/ppc/include/asm/byteorder.h|  37 
>>  xen/arch/ppc/include/asm/cache.h|   6 +
>>  xen/arch/ppc/include/asm/early_printk.h |  15 ++
>>  xen/arch/ppc/include/asm/msr.h  |  67 
>>  xen/arch/ppc/include/asm/processor.h| 207 ++
>>  xen/arch/ppc/include/asm/reg_defs.h | 218 
>>  xen/arch/ppc/include/asm/string.h   |   6 +
>>  xen/arch/ppc/include/asm/types.h|  35 
>>  xen/arch/ppc/ppc64/Makefile |   1 +
>>  xen/arch/ppc/ppc64/asm-offsets.c|  55 ++
>>  xen/arch/ppc/ppc64/head.S   |  48 +++---
>>  xen/arch/ppc/ppc64/of-call.S|  85 +
>>  xen/arch/ppc/setup.c|  31 
>>  20 files changed, 972 insertions(+), 22 deletions(-)
>>  create mode 100644 xen/arch/ppc/boot-of.c
>>  create mode 100644 xen/arch/ppc/early_printk.c
>>  create mode 100644 xen/arch/ppc/include/asm/boot.h
>>  create mode 100644 xen/arch/ppc/include/asm/bug.h
>>  create mode 100644 xen/arch/ppc/include/asm/byteorder.h
>>  create mode 100644 xen/arch/ppc/include/asm/cache.h
>>  create mode 100644 xen/arch/ppc/include/asm/early_printk.h
>>  create mode 100644 xen/arch/ppc/include/asm/msr.h
>>  create mode 100644 xen/arch/ppc/include/asm/processor.h
>>  create mode 100644 xen/arch/ppc/include/asm/reg_defs.h
>>  create mode 100644 xen/arch/ppc/include/asm/string.h
>>  create mode 100644 xen/arch/ppc/include/asm/types.h
>>  create mode 100644 xen/arch/ppc/ppc64/of-call.S
>>  create mode 100644 xen/arch/ppc/setup.c
> 
> This is a surprising amount of infrastructure.  I'm guessing it's a
> consequence of needing byteorder ?

Right, byteorder as well as va_{start,end,arg}. I could try to trim it
down further.

> There's a series still out deleting swathes of junk in byteorder.  I
> guess I need to kick that thread again, but it mostly boils down to
> using __builtin_bswap$N() (and on x86, reimplementing them on old enough
> compilers).  Presumably all versions of GCC (and eventually Clang) we
> care to support with ppc64 understand this builtin ?

Yes, those builtins should work just fine on any reasonably recent gcc
or clang toolchain. What would be the correct approach to integrating
these into byteorder.h? Would adding some `#define __arch_swab$N
__builtin_bswap$N` macros be the way to go?

> I've noticed a couple of other things.  asm/types.h repeats some
> antipatterns which we're trying to delete for MISRA reasons in other
> architectures.  I was already planning to fix that up xen-wide, and I
> guess now is the better time to do so.
> 
> Elsewhere, you've got a number of __inline__'s.  We think those are all
> vestigial now, so should be switched to using a plain inline.

Will do.

> Also, there are a bunch of UL() or ULL() macros which encoding a
> difference between asm and C.  In Xen, we use _AC() for that, which you
> can find in .

Thanks for the pointer, I'll update the usage of UL/ULL.

> Similarly, there are some functions which ought to be __init, so it
> would be good to get them correct from the start.

Good catch. This actually goes along with your subsequent observation
about Open Firmware residing in the bottom 4G of memory. See below.

> Maybe as an intermediate step, just get the infinite loop moved from asm
> up into C?  That gets the stack setup, and various of the asm helpers,
> without all the rest of the C required to get early_printk() to work.

Would you like that plus the serial patch in this same series, or would
you like me to just get the C infinite loop going for this series?

> Something we've been trying very hard to do generally is declutter
> processor.h, and on x86, we've got asm-defns.h as a more appropriate
> place to have the stuff which is expected to be common to all asm code,
> and never encountered in C.

Sounds good, I'll move the common asm-only stuff to an asm-defns.h.

> A couple of questions before I dive further in.
> 
> Given:
> 
> #define r0  0
> 
> do the assemblers really not understand register names?  That seems mad.

Yeah as surprising as it is, ppc64 assemblers don't handle register
names by default. I think at some point a flag was added to GAS (and
probably llvm? will have to check) to define them for you, but I'm not
sure which version introduced the flag.

I'll do some digging and if the flag is available a reasonable versions
of both toolchains (what exactly would constitute this? Are there
project-wide expectations of older toolchains working, 

Re: [PATCH 3/4] xen/ppc: Implement early serial printk on pseries

[Andrew Cooper]

On 21/06/2023 7:21 pm, Shawn Anastasio wrote:
> On 6/21/23 12:54 PM, Andrew Cooper wrote:
>> On 21/06/2023 5:59 pm, Shawn Anastasio wrote:
>>>  xen/arch/ppc/Kconfig.debug  |   5 +
>>>  xen/arch/ppc/Makefile   |   3 +
>>>  xen/arch/ppc/boot-of.c  | 116 +
>>>  xen/arch/ppc/configs/ppc64_defconfig|   1 +
>>>  xen/arch/ppc/early_printk.c |  28 +++
>>>  xen/arch/ppc/include/asm/boot.h |  24 +++
>>>  xen/arch/ppc/include/asm/bug.h  |   6 +
>>>  xen/arch/ppc/include/asm/byteorder.h|  37 
>>>  xen/arch/ppc/include/asm/cache.h|   6 +
>>>  xen/arch/ppc/include/asm/early_printk.h |  15 ++
>>>  xen/arch/ppc/include/asm/msr.h  |  67 
>>>  xen/arch/ppc/include/asm/processor.h| 207 ++
>>>  xen/arch/ppc/include/asm/reg_defs.h | 218 
>>>  xen/arch/ppc/include/asm/string.h   |   6 +
>>>  xen/arch/ppc/include/asm/types.h|  35 
>>>  xen/arch/ppc/ppc64/Makefile |   1 +
>>>  xen/arch/ppc/ppc64/asm-offsets.c|  55 ++
>>>  xen/arch/ppc/ppc64/head.S   |  48 +++---
>>>  xen/arch/ppc/ppc64/of-call.S|  85 +
>>>  xen/arch/ppc/setup.c|  31 
>>>  20 files changed, 972 insertions(+), 22 deletions(-)
>>>  create mode 100644 xen/arch/ppc/boot-of.c
>>>  create mode 100644 xen/arch/ppc/early_printk.c
>>>  create mode 100644 xen/arch/ppc/include/asm/boot.h
>>>  create mode 100644 xen/arch/ppc/include/asm/bug.h
>>>  create mode 100644 xen/arch/ppc/include/asm/byteorder.h
>>>  create mode 100644 xen/arch/ppc/include/asm/cache.h
>>>  create mode 100644 xen/arch/ppc/include/asm/early_printk.h
>>>  create mode 100644 xen/arch/ppc/include/asm/msr.h
>>>  create mode 100644 xen/arch/ppc/include/asm/processor.h
>>>  create mode 100644 xen/arch/ppc/include/asm/reg_defs.h
>>>  create mode 100644 xen/arch/ppc/include/asm/string.h
>>>  create mode 100644 xen/arch/ppc/include/asm/types.h
>>>  create mode 100644 xen/arch/ppc/ppc64/of-call.S
>>>  create mode 100644 xen/arch/ppc/setup.c
>> This is a surprising amount of infrastructure.  I'm guessing it's a
>> consequence of needing byteorder ?
> Right, byteorder as well as va_{start,end,arg}. I could try to trim it
> down further.

 should be entirely standalone.

But in general, it's nice to see if we can reduce the
inter-dependencies, and bringup of a new arch is the only time we get to
see them.

>
>> There's a series still out deleting swathes of junk in byteorder.  I
>> guess I need to kick that thread again, but it mostly boils down to
>> using __builtin_bswap$N() (and on x86, reimplementing them on old enough
>> compilers).  Presumably all versions of GCC (and eventually Clang) we
>> care to support with ppc64 understand this builtin ?
> Yes, those builtins should work just fine on any reasonably recent gcc
> or clang toolchain. What would be the correct approach to integrating
> these into byteorder.h? Would adding some `#define __arch_swab$N
> __builtin_bswap$N` macros be the way to go?

In the short term, that's perhaps best.

https://lore.kernel.org/xen-devel/cover.1653314499.git.lin@citrix.com/T/#u
is the full series

>> Similarly, there are some functions which ought to be __init, so it
>> would be good to get them correct from the start.
> Good catch. This actually goes along with your subsequent observation
> about Open Firmware residing in the bottom 4G of memory. See below.
>
>> Maybe as an intermediate step, just get the infinite loop moved from asm
>> up into C?  That gets the stack setup, and various of the asm helpers,
>> without all the rest of the C required to get early_printk() to work.
> Would you like that plus the serial patch in this same series, or would
> you like me to just get the C infinite loop going for this series?

Whatever is easier, although I expect splitting this patch into two will
make things simpler overall.

>> A couple of questions before I dive further in.
>>
>> Given:
>>
>> #define r0  0
>>
>> do the assemblers really not understand register names?  That seems mad.
> Yeah as surprising as it is, ppc64 assemblers don't handle register
> names by default. I think at some point a flag was added to GAS (and
> probably llvm? will have to check) to define them for you, but I'm not
> sure which version introduced the flag.
>
> I'll do some digging and if the flag is available a reasonable versions
> of both toolchains (what exactly would constitute this? Are there
> project-wide expectations of older toolchains working, and if so, how
> old?) then I can get rid of these.

You can pick a minimum version as you see fit, which is basically
anything goes prior to PPC64 being declared supported in Xen.

It needs to end up in the root README (but a patch for that probably
wants to wait until the port is a little more mature), but if I were you
I wouldn't care about supporting anything older than comes in cur

Re: [PATCH] Updates to Xen hypercall preemption

[Per Bilse]

On 6/21/2023 5:27 PM, Andy Lutomirski wrote:
> This code is a horrible mess, with and without your patches.  I think that, 
> if this were new, there's no way it would make it in to the kernel.

Hi Andy, and many thanks for your frank assessments.  Generally, this
is indeed somewhat old code, first introduced in 2015 by way of commit
fdfd811ddde3.  There's more information in the notes to that, and it's
maybe worth noting that we're not trying to introduce anything new,
merely fix what various commits since then have broken.

> I propose one of two rather radical changes:
> 
> 1. (preferred) Just delete all of it and make support for dom0 require either 
> full or dynamic preempt, and make a dynamic preempt kernel booting as dom0 
> run as full preempt.

Personally I think that's a good idea; a machine so limited in resources
that a fully preemptible dom0 kernel would be a problem wouldn't work as
a Xen server anyway.  Having said that, what to do about this isn't
really in my hands; the issues came to light because the kernel for
Citrix's XenServer product is being upgraded, and it was considered in
everybody's interest to upstream the fixes.  I'll see what I can do.

Best,

   -- Per

Re: [PATCH] Updates to Xen hypercall preemption

[Per Bilse]

On 6/21/2023 5:40 PM, Peter Zijlstra wrote:
> I don't understand it -- fundamentally, how can linux schedule when the
> guest isn't even running? Hypercall transfers control to the
> host/hypervisor and leaves the guest suspended.

Hi Peter, as noted in earlier note to Andy, this is essentially existing
code that other commits have rendered ineffective over time.  Hence,
the finer details of how or why it works haven't changed since it was
first introduced.

> This makes no sense; the race that warning warns about is:
> 
>   CPU0CPU1
>   per-cpu write
>   
>   
>   do-hypercall
> 
> So you wrote the value on CPU0, got migrated to CPU1 because you had
> preemptioned enabled, and then continue with the percpu value of CPU1
> because that's where you're at now.

This issue was raised internally, and it was noted that the only way
for the preemptible code to switch task is via an interrupt that goes
through xen_pv_evtchn_do_upcall(), which handles this.  I'm happy to
check with my sources, but it's holiday season right now.

>> 4) Update irqentry_exit_cond_resched() to raw_irqentry_exit_cond_resched().
>> The code will call irqentry_exit_cond_resched() if the flag (as noted
>> above) is set, but the dynamic preemption feature will livepatch that
>> function to a no-op unless full preemption is selected.  The code is
>> therefore updated to call raw_irqentry_exit_cond_resched().
> 
> That, again meeds more explanation. Why do you want this if not
> preemptible?

I'm not quite sure what you mean here.  Dynamic preemption
will livepatch irqentry_exit_cond_resched() to be a no-op, while
raw_irqentry_exit_cond_resched() remains functional.  This was 
introduced in commit 4624a14f4daa last year which was said to fix
the problem, but doesn't.  You may remember, it was signed off by 
yourself and Mark Rutland.

> You're doing 4 things, that should be 4 patches. Also, please give more
> clues for how this is supposed to work at all.

I respectfully have to disagree with that.  The fixes here are very
closely related, and we're not introducing anything new, we're merely
re-enabling code which has been rendered ineffective due to oversights
in commits made after the code was first introduced.  How the code is
supposed to work hasn't changed, and is beyond the scope of these fixes;
I'm sure it must have been discussed at great length at the time (commit 
fdfd811ddde3).

Best,

   -- Per

Re: [PATCH] Updates to Xen hypercall preemption

[Peter Zijlstra]

On Wed, Jun 21, 2023 at 07:19:21PM +, Per Bilse wrote:
> On 6/21/2023 5:40 PM, Peter Zijlstra wrote:
> > I don't understand it -- fundamentally, how can linux schedule when the
> > guest isn't even running? Hypercall transfers control to the
> > host/hypervisor and leaves the guest suspended.
> 
> Hi Peter, as noted in earlier note to Andy, this is essentially existing
> code that other commits have rendered ineffective over time.  Hence,
> the finer details of how or why it works haven't changed since it was
> first introduced.

That doesn't mean you don't have to explain how stuff works.

> > This makes no sense; the race that warning warns about is:
> > 
> > CPU0CPU1
> > per-cpu write
> > 
> > 
> > do-hypercall
> > 
> > So you wrote the value on CPU0, got migrated to CPU1 because you had
> > preemptioned enabled, and then continue with the percpu value of CPU1
> > because that's where you're at now.
> 
> This issue was raised internally, and it was noted that the only way
> for the preemptible code to switch task is via an interrupt that goes
> through xen_pv_evtchn_do_upcall(), which handles this.  I'm happy to
> check with my sources, but it's holiday season right now.

Then it should have all sorts of comments on and a comprehensive
changelog.

> >> 4) Update irqentry_exit_cond_resched() to raw_irqentry_exit_cond_resched().
> >> The code will call irqentry_exit_cond_resched() if the flag (as noted
> >> above) is set, but the dynamic preemption feature will livepatch that
> >> function to a no-op unless full preemption is selected.  The code is
> >> therefore updated to call raw_irqentry_exit_cond_resched().
> > 
> > That, again meeds more explanation. Why do you want this if not
> > preemptible?
> 
> I'm not quite sure what you mean here.  Dynamic preemption
> will livepatch irqentry_exit_cond_resched() to be a no-op, while
> raw_irqentry_exit_cond_resched() remains functional.  This was 
> introduced in commit 4624a14f4daa last year which was said to fix
> the problem, but doesn't.  You may remember, it was signed off by 
> yourself and Mark Rutland.

I don't see the relation; what you're doing is making dynamic preempt
that's not configured for full preempt do preemption. That's weird, and
again no comments.

I'm with Andy in that simply forcing full preemption would make far more
sense -- but I'm still missing something fundamental, see below.

> > You're doing 4 things, that should be 4 patches. Also, please give more
> > clues for how this is supposed to work at all.
> 
> I respectfully have to disagree with that.  The fixes here are very
> closely related, and we're not introducing anything new, we're merely
> re-enabling code which has been rendered ineffective due to oversights
> in commits made after the code was first introduced.  How the code is
> supposed to work hasn't changed, and is beyond the scope of these fixes;
> I'm sure it must have been discussed at great length at the time (commit 
> fdfd811ddde3).

You didn't even so much as reference that commit, nor provide any other
explanation. And having now read that commit, I'm not much enlightend.

*HOW* can a hypercall, something that exits the Guest and has the
Host/Hypervisor run get preempted in the Guest -- that isn't running.

Or are you calling apples pears?

Re: [PATCH 3/4] xen/ppc: Implement early serial printk on pseries

[Shawn Anastasio]

On 6/21/23 12:54 PM, Andrew Cooper wrote:
> I've noticed a couple of other things.  asm/types.h repeats some
> antipatterns which we're trying to delete for MISRA reasons in other
> architectures.  I was already planning to fix that up xen-wide, and I
> guess now is the better time to do so.

Could you elaborate on the changes you'd like to see in types.h?

Thanks,
Shawn

Re: [PATCH 3/4] xen/ppc: Implement early serial printk on pseries

[Julien Grall]

Hi Shawn,

Below some remark about the coding style. I will try to spot all of them 
so please go through your code and check if my comments applies in other 
places.


On 21/06/2023 17:59, Shawn Anastasio wrote:

On typical Power VMs (e.g. QEMU's -M pseries), a variety of services
including an early serial console are provided by Open Firmware.
Implement the required interfaces to call into Open Firmware and write
to the serial console.

Since Open Firmware runs in 32-bit Big Endian mode and Xen runs in
64-bit Little Endian mode, a thunk is required to save/restore
any potentially-clobbered registers as well as to perform the
required endianness switch. Thankfully, linux already has such
a routine, which was imported into ppc64/of-call.S.

Support for bare metal (PowerNV) will be implemented in a future
patch.

Signed-off-by: Shawn Anastasio 
---
  xen/arch/ppc/Kconfig.debug  |   5 +
  xen/arch/ppc/Makefile   |   3 +
  xen/arch/ppc/boot-of.c  | 116 +
  xen/arch/ppc/configs/ppc64_defconfig|   1 +
  xen/arch/ppc/early_printk.c |  28 +++
  xen/arch/ppc/include/asm/boot.h |  24 +++
  xen/arch/ppc/include/asm/bug.h  |   6 +
  xen/arch/ppc/include/asm/byteorder.h|  37 
  xen/arch/ppc/include/asm/cache.h|   6 +
  xen/arch/ppc/include/asm/early_printk.h |  15 ++
  xen/arch/ppc/include/asm/msr.h  |  67 
  xen/arch/ppc/include/asm/processor.h| 207 ++
  xen/arch/ppc/include/asm/reg_defs.h | 218 
  xen/arch/ppc/include/asm/string.h   |   6 +
  xen/arch/ppc/include/asm/types.h|  35 
  xen/arch/ppc/ppc64/Makefile |   1 +
  xen/arch/ppc/ppc64/asm-offsets.c|  55 ++
  xen/arch/ppc/ppc64/head.S   |  48 +++---
  xen/arch/ppc/ppc64/of-call.S|  85 +
  xen/arch/ppc/setup.c|  31 
  20 files changed, 972 insertions(+), 22 deletions(-)
  create mode 100644 xen/arch/ppc/boot-of.c
  create mode 100644 xen/arch/ppc/early_printk.c
  create mode 100644 xen/arch/ppc/include/asm/boot.h
  create mode 100644 xen/arch/ppc/include/asm/bug.h
  create mode 100644 xen/arch/ppc/include/asm/byteorder.h
  create mode 100644 xen/arch/ppc/include/asm/cache.h
  create mode 100644 xen/arch/ppc/include/asm/early_printk.h
  create mode 100644 xen/arch/ppc/include/asm/msr.h
  create mode 100644 xen/arch/ppc/include/asm/processor.h
  create mode 100644 xen/arch/ppc/include/asm/reg_defs.h
  create mode 100644 xen/arch/ppc/include/asm/string.h
  create mode 100644 xen/arch/ppc/include/asm/types.h
  create mode 100644 xen/arch/ppc/ppc64/of-call.S
  create mode 100644 xen/arch/ppc/setup.c

diff --git a/xen/arch/ppc/Kconfig.debug b/xen/arch/ppc/Kconfig.debug
index e69de29bb2..608c9ff832 100644
--- a/xen/arch/ppc/Kconfig.debug
+++ b/xen/arch/ppc/Kconfig.debug
@@ -0,0 +1,5 @@
+config EARLY_PRINTK
+bool "Enable early printk"
+default DEBUG
+help
+  Enables early printk debug messages
diff --git a/xen/arch/ppc/Makefile b/xen/arch/ppc/Makefile
index 98220648af..4c4cdcb60e 100644
--- a/xen/arch/ppc/Makefile
+++ b/xen/arch/ppc/Makefile
@@ -1,4 +1,7 @@
  obj-$(CONFIG_PPC64) += ppc64/
+obj-y += setup.o
+obj-y += boot-of.o
+obj-$(CONFIG_EARLY_PRINTK) += early_printk.o


We commonly order the file alphabetically. So setup.o should be last.

  
  $(TARGET): $(TARGET)-syms

cp -f $< $@
diff --git a/xen/arch/ppc/boot-of.c b/xen/arch/ppc/boot-of.c
new file mode 100644
index 00..1ceeaf1250
--- /dev/null
+++ b/xen/arch/ppc/boot-of.c
@@ -0,0 +1,116 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.


As you already have the SPDX license, the full license should be dropped.


+ *
+ * Copyright IBM Corp. 2005, 2006, 2007
+ * Copyright Raptor Engineering, LLC
+ *
+ * Authors: Jimi Xenidis 
+ *  Hollis Blanchard 
+ *  Shawn Anastasio 
+ */
+
+#include 
+#include 
+#include 
+#include 
+
+#define ADDR(x) (uint32_t)(unsigned long)(x)
+
+/* OF entrypoint*/
+static unsigned long of_vec;
+
+/* OF device handles*/
+static int bof_chosen;
+static int of_out;
+
+static int of_call(const char *service, uint32_t nargs, uint32_t nrets,
+   int32_t 

[PATCH 2/2] xen/types: Rework stdint vs __{u,s}$N types

[Andrew Cooper]

Xen uses the stdint types.  Rearrange the types headers to define the
compatibility __{u,s}$N types in terms of the stdint types, not the other way
around.

All all supported compilers on architectures other than x86 support the stdint
__*_TYPE__ macros.  Move these into the common types.h to avoid them being
duplicated in each architecture.

For x86 on obsolete compilers, synthesize appropriate types.

This cleanup has the side effect of removing all use of the undocumented
__signed__ GCC keyword.  This is a vestigial remnant of `gcc -traditional`
mode for dialetcs of C prior to the introduction of the signed keyword.

No functional change.

Signed-off-by: Andrew Cooper 
---
CC: Jan Beulich 
CC: Roger Pau Monné 
CC: Wei Liu 
CC: Stefano Stabellini 
CC: Julien Grall 
CC: Volodymyr Babchuk 
CC: Bertrand Marquis 
CC: Bob Eshleman 
CC: Alistair Francis 
CC: Connor Davis 
CC: Oleksii Kurochko 
CC: Shawn Anastasio 
CC: Timothy Pearson 
CC: Roberto Bagnara 

I've left the non __ types alone for now.  They're complicated mostly by ARM
having differing ideas of what a paddr_t is.

A different option would be to sort out the stdint types ahead of including
, which can either be done by introducing a  or
upping the minimum compiler version for x86; a task which is massively
overdue.
---
 xen/arch/arm/include/asm/types.h   | 19 ---
 xen/arch/riscv/include/asm/types.h | 19 ---
 xen/arch/x86/include/asm/types.h   | 21 +
 xen/include/xen/types.h| 28 +---
 4 files changed, 26 insertions(+), 61 deletions(-)

diff --git a/xen/arch/arm/include/asm/types.h b/xen/arch/arm/include/asm/types.h
index fb6618ef247f..545a5e9d1175 100644
--- a/xen/arch/arm/include/asm/types.h
+++ b/xen/arch/arm/include/asm/types.h
@@ -1,25 +1,6 @@
 #ifndef __ARM_TYPES_H__
 #define __ARM_TYPES_H__
 
-typedef __signed__ char __s8;
-typedef unsigned char __u8;
-
-typedef __signed__ short __s16;
-typedef unsigned short __u16;
-
-typedef __signed__ int __s32;
-typedef unsigned int __u32;
-
-#if defined(__GNUC__) && !defined(__STRICT_ANSI__)
-#if defined(CONFIG_ARM_32)
-typedef __signed__ long long __s64;
-typedef unsigned long long __u64;
-#elif defined (CONFIG_ARM_64)
-typedef __signed__ long __s64;
-typedef unsigned long __u64;
-#endif
-#endif
-
 typedef signed char s8;
 typedef unsigned char u8;
 
diff --git a/xen/arch/riscv/include/asm/types.h 
b/xen/arch/riscv/include/asm/types.h
index 0c0ce78c8f6e..93a680a8f323 100644
--- a/xen/arch/riscv/include/asm/types.h
+++ b/xen/arch/riscv/include/asm/types.h
@@ -1,25 +1,6 @@
 #ifndef __RISCV_TYPES_H__
 #define __RISCV_TYPES_H__
 
-typedef __signed__ char __s8;
-typedef unsigned char __u8;
-
-typedef __signed__ short __s16;
-typedef unsigned short __u16;
-
-typedef __signed__ int __s32;
-typedef unsigned int __u32;
-
-#if defined(__GNUC__) && !defined(__STRICT_ANSI__)
-#if defined(CONFIG_RISCV_32)
-typedef __signed__ long long __s64;
-typedef unsigned long long __u64;
-#elif defined (CONFIG_RISCV_64)
-typedef __signed__ long __s64;
-typedef unsigned long __u64;
-#endif
-#endif
-
 typedef signed char s8;
 typedef unsigned char u8;
 
diff --git a/xen/arch/x86/include/asm/types.h b/xen/arch/x86/include/asm/types.h
index 2d56aed66782..a5d4f6e9587a 100644
--- a/xen/arch/x86/include/asm/types.h
+++ b/xen/arch/x86/include/asm/types.h
@@ -1,18 +1,15 @@
 #ifndef __X86_TYPES_H__
 #define __X86_TYPES_H__
 
-typedef __signed__ char __s8;
-typedef unsigned char __u8;
-
-typedef __signed__ short __s16;
-typedef unsigned short __u16;
-
-typedef __signed__ int __s32;
-typedef unsigned int __u32;
-
-#if defined(__GNUC__) && !defined(__STRICT_ANSI__)
-typedef __signed__ long __s64;
-typedef unsigned long __u64;
+#ifndef __INT8_TYPE__ /* GCC <= 4.4 */
+# define __INT8_TYPE__ signed  char
+# define __UINT8_TYPE__  unsigned  char
+# define __INT16_TYPE__   short
+# define __UINT16_TYPE__ unsigned short
+# define __INT32_TYPE__ int
+# define __UINT32_TYPE__ unsigned   int
+# define __INT64_TYPE__long
+# define __UINT64_TYPE__ unsigned  long
 #endif
 
 typedef signed char s8;
diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h
index 8b22a02eeaa4..4083f1bf18b0 100644
--- a/xen/include/xen/types.h
+++ b/xen/include/xen/types.h
@@ -11,6 +11,15 @@ typedef signed long ssize_t;
 
 typedef __PTRDIFF_TYPE__ ptrdiff_t;
 
+typedef __INT8_TYPE__int8_t;
+typedef __UINT8_TYPE__  uint8_t;
+typedef __INT16_TYPE__  int16_t;
+typedef __UINT16_TYPE__uint16_t;
+typedef __INT32_TYPE__  int32_t;
+typedef __UINT32_TYPE__uint32_t;
+typedef __INT64_TYPE__  int64_t;
+typedef __UINT64_TYPE__uint64_t;
+
 #define BITS_TO_LONGS(bits) \
 (((bits)+BITS_PER_LONG-1)/BITS_PER_LONG)
 #define DECLARE_BITMAP(name,bits) \
@@ -39,17 +48,14 @@ typedef __PTRDIFF_TYPE__ ptrdiff_t;
 #define LONG_MIN(-LONG_MAX - 1)
 #define ULONG_MAX   (~0UL)
 
-typedef __u8uint8_t;
-type

[PATCH 0/2] xen/types: Cleanup

[Andrew Cooper]

Done both to avoid PPC64 bringing in yet another copy of some antipatterns,
and because it fixes the MISRA issue relating to the __signed__ keyword.

https://gitlab.com/xen-project/people/andyhhp/xen/-/pipelines/907638603

Andrew Cooper (2):
  xen/types: Drop #ifdefary for __{SIZE,PTRDIFF}_TYPE__
  xen/types: Rework stdint vs __{u,s}$N types

 xen/arch/arm/include/asm/types.h   | 19 ---
 xen/arch/riscv/include/asm/types.h | 19 ---
 xen/arch/x86/include/asm/types.h   | 21 -
 xen/include/xen/types.h| 37 +++---
 4 files changed, 27 insertions(+), 69 deletions(-)


base-commit: 5c84f1f636981dab5341e84aaba8d4dd00bbc2cb
-- 
2.30.2

[PATCH 1/2] xen/types: Drop #ifdefary for __{SIZE,PTRDIFF}_TYPE__

[Andrew Cooper]

All supported compilers have these types.

No functional change.

Signed-off-by: Andrew Cooper 
---
CC: Jan Beulich 
CC: Roger Pau Monné 
CC: Wei Liu 
CC: Stefano Stabellini 
CC: Julien Grall 
CC: Volodymyr Babchuk 
CC: Bertrand Marquis 
CC: Bob Eshleman 
CC: Alistair Francis 
CC: Connor Davis 
CC: Oleksii Kurochko 
CC: Shawn Anastasio 
CC: Timothy Pearson 

https://godbolt.org/z/Y6PWcd6js
---
 xen/include/xen/types.h | 9 +
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h
index 6aba80500aaf..8b22a02eeaa4 100644
--- a/xen/include/xen/types.h
+++ b/xen/include/xen/types.h
@@ -5,18 +5,11 @@
 
 #include 
 
-#if defined(__SIZE_TYPE__)
 typedef __SIZE_TYPE__ size_t;
-#else
-typedef unsigned long size_t;
-#endif
+
 typedef signed long ssize_t;
 
-#if defined(__PTRDIFF_TYPE__)
 typedef __PTRDIFF_TYPE__ ptrdiff_t;
-#else
-typedef signed long ptrdiff_t;
-#endif
 
 #define BITS_TO_LONGS(bits) \
 (((bits)+BITS_PER_LONG-1)/BITS_PER_LONG)
-- 
2.30.2

Re: [XEN PATCH 12/13] xen/x86: fixed violations of MISRA C:2012 Rule 7.2

[Stefano Stabellini]

On Wed, 21 Jun 2023, Jan Beulich wrote:
> First of all - please trim replies.
> 
> On 20.06.2023 23:23, Stefano Stabellini wrote:
> > On Tue, 20 Jun 2023, Simone Ballarin wrote:
> >> --- a/xen/arch/x86/percpu.c
> >> +++ b/xen/arch/x86/percpu.c
> >> @@ -12,7 +12,7 @@ unsigned long __per_cpu_offset[NR_CPUS];
> >>   * possible #PF at (NULL + a little) which has security implications in 
> >> the
> >>   * context of PV guests.
> >>   */
> >> -#define INVALID_PERCPU_AREA (0x8000L - (long)__per_cpu_start)
> >> +#define INVALID_PERCPU_AREA (0x8000UL - (long)__per_cpu_start)
> >>  #define PERCPU_ORDER get_order_from_bytes(__per_cpu_data_end - 
> >> __per_cpu_start)
> > 
> > Hi Jan, should this be ULL?
> 
> Not really, no - we're 64-bit only. Furthermore the expression is
> about addresses, which correspond to "unsigned long" in our world.

[...]

> >> --- a/xen/include/public/arch-x86/xen-x86_64.h
> >> +++ b/xen/include/public/arch-x86/xen-x86_64.h
> >> @@ -53,10 +53,10 @@
> >>  #define FLAT_USER_SS32 FLAT_RING3_SS32
> >>  #define FLAT_USER_SS   FLAT_USER_SS64
> >>  
> >> -#define __HYPERVISOR_VIRT_START 0x8000
> >> -#define __HYPERVISOR_VIRT_END   0x8800
> >> -#define __MACH2PHYS_VIRT_START  0x8000
> >> -#define __MACH2PHYS_VIRT_END0x8040
> >> +#define __HYPERVISOR_VIRT_START 0x8000U
> >> +#define __HYPERVISOR_VIRT_END   0x8800U
> >> +#define __MACH2PHYS_VIRT_START  0x8000U
> >> +#define __MACH2PHYS_VIRT_END0x8040U
> > 
> > Also here ULL?
> 
> UL yes, but as above I don't think ULL is good to use for addresses.
> That said, things are a little less clear in the public headers: In
> principle it could be a goal for them to be usable on foreign
> architectures (say e.g. a 32-bit tool stack, or an analysis tool
> which can be run without being on the original host architecture) as
> well.
> 
> Furthermore, open-coded use of ULL would make this no-longer-C89.
> If anything, it would then need to be UINT64_C(), yet even that would
> grow our public header dependencies on C99-like infrastructure being
> available (right now we only require certain stdint.h-like types;
> Arm alone, interestingly, also uses PRIx64 and PRIu64, which may be
> kind of unavoidable there).

UL seems to be the best option then.

Re: [PATCH v1 1/1] Q35 Support

[Bernhard Beschow]

Hi Joel,

Nice! I've been working on making the PIIX south bridge Xen agnostic, partly to 
show how Xen enablement in Q35 could look like. Not that I'd have any use case 
for it but great to see that you've actually done that!

I know you didn't intend to send this patch but I'll give you some early 
comments anyway.

Am 20. Juni 2023 17:24:35 UTC schrieb Joel Upham :
>---
> hw/acpi/ich9.c|   22 +-
> hw/acpi/pcihp.c   |6 +-
> hw/core/machine.c |   19 +
> hw/i386/pc_piix.c |3 +-
> hw/i386/pc_q35.c  |   39 +-
> hw/i386/xen/xen-hvm.c |7 +-
> hw/i386/xen/xen_platform.c|   19 +-
> hw/isa/lpc_ich9.c |   53 +-
> hw/isa/piix3.c|2 +-
> hw/pci-host/q35.c |   28 +-
> hw/pci/pci.c  |   17 +
> hw/xen/xen-host-pci-device.c  |  106 +++-
> hw/xen/xen-host-pci-device.h  |6 +-
> hw/xen/xen_pt.c   |   49 +-
> hw/xen/xen_pt.h   |   19 +-
> hw/xen/xen_pt_config_init.c   | 1103 ++---
> include/hw/acpi/ich9.h|1 +
> include/hw/acpi/pcihp.h   |2 +
> include/hw/boards.h   |1 +
> include/hw/i386/pc.h  |3 +
> include/hw/pci-host/q35.h |4 +-
> include/hw/pci/pci.h  |3 +
> include/hw/southbridge/ich9.h |1 +
> include/hw/xen/xen.h  |4 +-
> qemu-options.hx   |1 +
> softmmu/datadir.c |1 -
> softmmu/qdev-monitor.c|3 +-
> stubs/xen-hw-stub.c   |4 +-
> 28 files changed, 1395 insertions(+), 131 deletions(-)
>
>diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
>index 25e2c7243e..234706a191 100644
>--- a/hw/acpi/ich9.c
>+++ b/hw/acpi/ich9.c
>@@ -39,6 +39,8 @@
> #include "hw/southbridge/ich9.h"
> #include "hw/mem/pc-dimm.h"
> #include "hw/mem/nvdimm.h"
>+#include "hw/xen/xen.h"
>+#include "sysemu/xen.h"
> 
> //#define DEBUG
> 
>@@ -67,6 +69,10 @@ static void ich9_gpe_writeb(void *opaque, hwaddr addr, 
>uint64_t val,
> ICH9LPCPMRegs *pm = opaque;
> acpi_gpe_ioport_writeb(&pm->acpi_regs, addr, val);
> acpi_update_sci(&pm->acpi_regs, pm->irq);
>+
>+if (xen_enabled()) {
>+acpi_pcihp_reset(&pm->acpi_pci_hotplug);
>+}
> }
> 
> static const MemoryRegionOps ich9_gpe_ops = {
>@@ -137,7 +143,8 @@ static int ich9_pm_post_load(void *opaque, int version_id)
> {
> ICH9LPCPMRegs *pm = opaque;
> uint32_t pm_io_base = pm->pm_io_base;
>-pm->pm_io_base = 0;
>+if (!xen_enabled())
>+pm->pm_io_base = 0;
> ich9_pm_iospace_update(pm, pm_io_base);
> return 0;
> }
>@@ -268,7 +275,10 @@ static void pm_reset(void *opaque)
> acpi_pm1_evt_reset(&pm->acpi_regs);
> acpi_pm1_cnt_reset(&pm->acpi_regs);
> acpi_pm_tmr_reset(&pm->acpi_regs);
>-acpi_gpe_reset(&pm->acpi_regs);
>+/* Noticed guest freezing in xen when this was reset after S3. */
>+if (!xen_enabled()) {
>+acpi_gpe_reset(&pm->acpi_regs);
>+}

I wonder why this seems to work with PIIX?

I'd rather try to keep the Xen impact on the device model as low as possible, 
ideally as low as in the PIIX4 ACPI device model.

> 
> pm->smi_en = 0;
> if (!pm->smm_enabled) {
>@@ -316,7 +326,7 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm, 
>qemu_irq sci_irq)
> acpi_pm_tco_init(&pm->tco_regs, &pm->io);
> }
> 
>-if (pm->acpi_pci_hotplug.use_acpi_hotplug_bridge) {
>+if (pm->acpi_pci_hotplug.use_acpi_hotplug_bridge || xen_enabled()) {
> acpi_pcihp_init(OBJECT(lpc_pci),
> &pm->acpi_pci_hotplug,
> pci_get_bus(lpc_pci),
>@@ -332,10 +342,14 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm, 
>qemu_irq sci_irq)
> pm->powerdown_notifier.notify = pm_powerdown_req;
> qemu_register_powerdown_notifier(&pm->powerdown_notifier);
> 
>+if (xen_enabled()) {
>+acpi_set_pci_info(true);
>+}
>+
> legacy_acpi_cpu_hotplug_init(pci_address_space_io(lpc_pci),
> OBJECT(lpc_pci), &pm->gpe_cpu, ICH9_CPU_HOTPLUG_IO_BASE);
> 
>-if (pm->acpi_memory_hotplug.is_enabled) {
>+if (pm->acpi_memory_hotplug.is_enabled || xen_enabled()) {
> acpi_memory_hotplug_init(pci_address_space_io(lpc_pci), 
> OBJECT(lpc_pci),
>  &pm->acpi_memory_hotplug,
>  ACPI_MEMORY_HOTPLUG_BASE);
>diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
>index cdd6f775a1..5b065d670c 100644
>--- a/hw/acpi/pcihp.c
>+++ b/hw/acpi/pcihp.c
>@@ -40,6 +40,7 @@
> #include "qapi/error.h"
> #include "qom/qom-qobject.h"
> #include "trace.h"
>+#include "sysemu/xen.h"
> 
> #define ACPI_PCIHP_SIZE 0x0018
> #define PCI_UP_BASE 0x
>@@ -84,7 +85,8 @@ static void *acpi_set_bsel(PCIBus *bus, void *opaque)
> bool is_bridge = IS_PCI_BRIDGE(br);
> 
> /* hotplugged bridges can't be described in ACPI ignore them */
>-if (qbus_is_hotpluggable(BUS(bus))) {
>+/* Xen requ

Re: [PATCH] docs/misra: add Rules 8.2, 8.3, 8.14

[Stefano Stabellini]

On Wed, 21 Jun 2023, Jan Beulich wrote:
> On 21.06.2023 03:26, Stefano Stabellini wrote:
> > --- a/docs/misra/rules.rst
> > +++ b/docs/misra/rules.rst
> > @@ -213,6 +213,17 @@ maintainers if you want to suggest a change.
> >   - Types shall be explicitly specified
> >   -
> >  
> > +   * - `Rule 8.2 
> > `_
> > + - Required
> > + - Function types shall be in prototype form with named parameters
> > + -
> > +
> > +   * - `Rule 8.3 
> > `_
> > + - Required
> > + - All declarations of an object or function shall use the same
> > +   names and type qualifiers
> > + -
> 
> I think we want to deal with uses of const when not qualifying a pointed-to
> type: One approach is to simply say we don't use const like this (and the
> few uses there are should then go away). The other, if we deem this a
> valuable feature, would be to make a project-wide exception for this case,
> as having such const in declarations is meaningless and hence at the risk
> of being confusing or hampering readability.

I think they should go away (the first option you wrote).

If you are OK with it, I could add a note here, such as:

"The rule also applies to differences in const-ness."

[xen-unstable-smoke test] 181538: trouble: blocked/broken/pass

[osstest service owner]

flight 181538 xen-unstable-smoke real [real]
http://logs.test-lab.xenproject.org/osstest/logs/181538/

Failures and problems with tests :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 build-armhf  broken
 build-armhf   4 host-install(4)broken REGR. vs. 181476

Tests which did not succeed, but are not blocking:
 test-armhf-armhf-xl   1 build-check(1)   blocked  n/a
 test-amd64-amd64-libvirt 15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  16 saverestore-support-checkfail   never pass

version targeted for testing:
 xen  1ec2f5305b826367d2cbfcd24ca079448057e331
baseline version:
 xen  7a25a1501ca941c3e01b0c4e624ace05417f1587

Last test of basis   181476  2023-06-17 04:00:28 Z4 days
Failing since181504  2023-06-19 11:00:26 Z2 days8 attempts
Testing same since   181538  2023-06-21 17:03:40 Z0 days1 attempts


People who touched revisions under test:
  Alistair Francis 
  Andrew Cooper 
  Anthony PERARD 
  Henry Wang 
  Jan Beulich 
  Jiamei Xie 
  Julien Grall 
  Michal Orzel 
  Oleksii Kurochko 
  Roger Pau Monné 
  Shawn Anastasio 
  Stefano Stabellini 
  Stefano Stabellini 

jobs:
 build-arm64-xsm  pass
 build-amd64  pass
 build-armhf  broken  
 build-amd64-libvirt  pass
 test-armhf-armhf-xl  blocked 
 test-arm64-arm64-xl-xsm  pass
 test-amd64-amd64-xl-qemuu-debianhvm-amd64pass
 test-amd64-amd64-libvirt pass



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

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

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

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

broken-job build-armhf broken
broken-step build-armhf host-install(4)

Not pushing.

(No revision log; it would be 373 lines long.)

Re: [XEN PATCH v3] docs/misra: document the C dialect and translation toolchain assumptions.

[Stefano Stabellini]

On Wed, 21 Jun 2023, Roberto Bagnara wrote:
> This document specifies the C language dialect used by Xen and
> the assumptions Xen makes on the translation toolchain.
> 
> Signed-off-by: Roberto Bagnara 
> 
> Changes in V2:
>   - Clarified several entries.
>   - Removed entry about the use of the undefined escape sequence \m.
> Changes in V3:
>   - Removed mention of the __signed__ non-standard keyword.
>   - Fixed the entry about arithmetic operator on pointer to void.
>   - Added entry for named variadic macro arguments.
>   - Removed entry about static function used in an inline function
> with external linkage.

The list of changes should be after the "---" so that it gets
automatically removed by git on commit.

No worries, this time it can be manually done on commit. With that small
caveat:

Reviewed-by: Stefano Stabellini 


> ---
>  docs/misra/C-language-toolchain.rst | 468 
>  1 file changed, 468 insertions(+)
>  create mode 100644 docs/misra/C-language-toolchain.rst
> 
> diff --git a/docs/misra/C-language-toolchain.rst 
> b/docs/misra/C-language-toolchain.rst
> new file mode 100644
> index 00..8651f59118
> --- /dev/null
> +++ b/docs/misra/C-language-toolchain.rst
> @@ -0,0 +1,468 @@
> +=
> +C Dialect and Translation Assumptions for Xen
> +=
> +
> +This document specifies the C language dialect used by Xen and
> +the assumptions Xen makes on the translation toolchain.
> +It covers, in particular:
> +
> +1. the used language extensions;
> +2. the translation limits that the translation toolchains must be able
> +   to accommodate;
> +3. the implementation-defined behaviors upon which Xen may depend.
> +
> +All points are of course relevant for portability.  In addition,
> +programming in C is impossible without a detailed knowledge of the
> +implementation-defined behaviors.  For this reason, it is recommended
> +that Xen developers have familiarity with this document and the
> +documentation referenced therein.
> +
> +This document needs maintenance and adaptation in the following
> +circumstances:
> +
> +- whenever the compiler is changed or updated;
> +- whenever the use of a certain language extension is added or removed;
> +- whenever code modifications cause exceeding the stated translation limits.
> +
> +
> +Applicable C Language Standard
> +__
> +
> +Xen is written in C99 with extensions.  The relevant ISO standard is
> +
> +*ISO/IEC 9899:1999/Cor 3:2007*: Programming Languages - C,
> +Technical Corrigendum 3.
> +ISO/IEC, Geneva, Switzerland, 2007.
> +
> +
> +Reference Documentation
> +___
> +
> +The following documents are referred to in the sequel:
> +
> +GCC_MANUAL:
> +  https://gcc.gnu.org/onlinedocs/gcc-12.1.0/gcc.pdf
> +CPP_MANUAL:
> +  https://gcc.gnu.org/onlinedocs/gcc-12.1.0/cpp.pdf
> +ARM64_ABI_MANUAL:
> +  
> https://github.com/ARM-software/abi-aa/blob/60a8eb8c55e999d74dac5e368fc9d7e36e38dda4/aapcs64/aapcs64.rst
> +X86_64_ABI_MANUAL:
> +  
> https://gitlab.com/x86-psABIs/x86-64-ABI/-/jobs/artifacts/master/raw/x86-64-ABI/abi.pdf?job=build
> +
> +
> +C Language Extensions
> +_
> +
> +
> +The following table lists the extensions currently used in Xen.
> +The table columns are as follows:
> +
> +   Extension
> +  a terse description of the extension;
> +   Architectures
> +  a set of Xen architectures making use of the extension;
> +   References
> +  when available, references to the documentation explaining
> +  the syntax and semantics of (each instance of) the extension.
> +
> +
> +.. list-table::
> +   :widths: 30 15 55
> +   :header-rows: 1
> +
> +   * - Extension
> + - Architectures
> + - References
> +
> +   * - Non-standard tokens
> + - ARM64, X86_64
> + - _Static_assert:
> +  see Section "2.1 C Language" of GCC_MANUAL.
> +   asm, __asm__:
> +  see Sections "6.48 Alternate Keywords" and "6.47 How to Use Inline 
> Assembly Language in C Code" of GCC_MANUAL.
> +   __volatile__:
> +  see Sections "6.48 Alternate Keywords" and "6.47.2.1 Volatile" of 
> GCC_MANUAL.
> +   __const__, __inline__, __inline:
> +  see Section "6.48 Alternate Keywords" of GCC_MANUAL.
> +   typeof, __typeof__:
> +  see Section "6.7 Referring to a Type with typeof" of GCC_MANUAL.
> +   __alignof__, __alignof:
> +  see Sections "6.48 Alternate Keywords" and "6.44 Determining the 
> Alignment of Functions, Types or Variables" of GCC_MANUAL.
> +   __attribute__:
> +  see Section "6.39 Attribute Syntax" of GCC_MANUAL.
> +   __builtin_types_compatible_p:
> +  see Section "6.59 Other Built-in Functions Provided by GCC" of 
> GCC_MANUAL.
> +   __builtin_va_arg:
> +  non-documented GCC extension.
> +   __builtin_offsetof:
> +  see Section "6.53 Support for offseto