Re: [XEN PATCH v2 00/10] address violations of MISRA C:2012 Directive 4.10

[Jan Beulich]

On 18.10.2023 02:48, Stefano Stabellini wrote:
> On Mon, 16 Oct 2023, Jan Beulich wrote:
>> On 29.09.2023 00:24, Stefano Stabellini wrote:
>>> If it is not a MISRA requirement, then I think we should go for the path
>>> of least resistance and try to make the smallest amount of changes
>>> overall, which seems to be:
>>
>> ... "least resistance" won't gain us much, as hardly any guards don't
>> start with an underscore.
>>
>>> - for xen/include/blah.h, __BLAH_H__
>>> - for xen/arch/arm/asm/include/blah.h, __ASM_ARM_BLAH_H__
>>> - for xen/arch/x86/asm/include/blah.h, it is far less consistent, maybe 
>>> __ASM_X86_BLAH_H__ ?
>>
>> There are no headers in xen/include/. For (e.g.) xen/include/xen/ we
>> may go with XEN_BLAH_H; whether ASM prefixes are needed I'm not sure;
>> we could go with just ARM_BLAH_H and X86_BLAH_H?
>>
>> The primary question though is (imo) how to deal with private headers,
>> such that the risk of name collisions is as small as possible.
> 
> Looking at concrete examples under xen/include/xen:
> xen/include/xen/mm.h __XEN_MM_H__
> xen/include/xen/dm.h __XEN_DM_H__
> xen/include/xen/hypfs.h __XEN_HYPFS_H__
> 
> So I think we should do for consistency:
> xen/include/xen/blah.h __XEN_BLAH_H__
> 
> Even if we know the leading underscore are undesirable, in this case I
> would prefer consistency.

I'm kind of okay with that. FTAOD - here and below you mean to make this
one explicit first exception from the "no new leading underscores" goal,
for newly added headers?

> On the other hand looking at ARM examples:
> xen/arch/arm/include/asm/traps.h __ASM_ARM_TRAPS__
> xen/arch/arm/include/asm/time.h __ARM_TIME_H__
> xen/arch/arm/include/asm/sysregs.h __ASM_ARM_SYSREGS_H
> xen/arch/arm/include/asm/io.h _ASM_IO_H
> 
> And also looking at x86 examples:
> xen/arch/x86/include/asm/paging.h _XEN_PAGING_H
> xen/arch/x86/include/asm/p2m.h _XEN_ASM_X86_P2M_H
> xen/arch/x86/include/asm/io.h _ASM_IO_H
> 
> Thet are very inconsistent.
> 
> 
> So for ARM and X86 headers I think we are free to pick anything we want,
> including your suggested ARM_BLAH_H and X86_BLAH_H. Those are fine by
> me.

To be honest, I'd prefer a global underlying pattern, i.e. if common
headers are "fine" to use leading underscores for guards, arch header
should, too.

> For private headers such as:
> xen/arch/arm/vuart.h __ARCH_ARM_VUART_H__
> xen/arch/arm/decode.h __ARCH_ARM_DECODE_H_
> xen/arch/x86/mm/p2m.h __ARCH_MM_P2M_H__
> xen/arch/x86/hvm/viridian/private.h X86_HVM_VIRIDIAN_PRIVATE_H
> 
> More similar but still inconsistent. I would go with ARCH_ARM_BLAH_H and
> ARCH_X86_BLAH_H for new headers.

I'm afraid I don't like this, as deeper paths would lead to unwieldy
guard names. If we continue to use double-underscore prefixed names
in common and arch headers, why don't we demand no leading underscores
and no path-derived prefixes in private headers? That'll avoid any
collisions between the two groups.

Jan

[xen-unstable test] 183403: tolerable FAIL - PUSHED

[osstest service owner]

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

Failures :-/ but no regressions.

Tests which are failing intermittently (not blocking):
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 7 xen-install fail pass in 
183406-retest
 test-amd64-amd64-xl-credit1 20 guest-localmigrate/x10 fail pass in 
183406-retest

Tests which did not succeed, but are not blocking:
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail in 183406 never pass
 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail  like 183392
 test-armhf-armhf-libvirt 16 saverestore-support-checkfail  like 183399
 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 183399
 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 183399
 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 183399
 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 183399
 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 183399
 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 183399
 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check   fail like 183399
 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 183399
 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 183399
 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 183399
 test-amd64-i386-xl-pvshim14 guest-start  fail   never pass
 test-amd64-i386-libvirt-xsm  15 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  16 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail never pass
 test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail  never pass
 test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail  never pass
 test-armhf-armhf-xl-rtds 15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt 15 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt 15 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt-raw  14 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-vhd  14 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-vhd  15 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt-raw 14 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  14 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  15 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-credit1  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit1  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass

version targeted for testing:
 xen  7114bbfc8424ee467c6c8c82f077764ca4fa799b
baseline version:
 xen 

Re: [PATCH for-4.18 v3] x86/pvh: fix identity mapping of low 1MB

[Henry Wang]

Hi Roger,

> On Oct 17, 2023, at 16:29, Roger Pau Monne  wrote:
> 
> The mapping of memory regions below the 1MB mark was all done by the PVH dom0
> builder code, causing the region to be avoided by the arch specific IOMMU
> hardware domain initialization code.  That lead to the IOMMU being enabled
> without reserved regions in the low 1MB identity mapped in the p2m for PVH
> hardware domains.  Firmware which happens to be missing RMRR/IVMD ranges
> describing E820 reserved regions in the low 1MB would transiently trigger 
> IOMMU
> faults until the p2m is populated by the PVH dom0 builder:
> 
> AMD-Vi: IO_PAGE_FAULT: :00:13.1 d0 addr 000eb380 flags 0x20 RW
> AMD-Vi: IO_PAGE_FAULT: :00:13.1 d0 addr 000eb340 flags 0
> AMD-Vi: IO_PAGE_FAULT: :00:13.2 d0 addr 000ea1c0 flags 0
> AMD-Vi: IO_PAGE_FAULT: :00:14.5 d0 addr 000eb480 flags 0x20 RW
> AMD-Vi: IO_PAGE_FAULT: :00:12.0 d0 addr 000eb080 flags 0x20 RW
> AMD-Vi: IO_PAGE_FAULT: :00:14.5 d0 addr 000eb400 flags 0
> AMD-Vi: IO_PAGE_FAULT: :00:12.0 d0 addr 000eb040 flags 0
> 
> Those errors have been observed on the osstest pinot{0,1} boxes (AMD Fam15h
> Opteron(tm) Processor 3350 HE).
> 
> Rely on the IOMMU arch init code to create any identity mappings for reserved
> regions in the low 1MB range (like it already does for reserved regions
> elsewhere), and leave the mapping of any holes to be performed by the dom0
> builder code.
> 
> Fixes: 6b4f6a31ace1 ('x86/PVH: de-duplicate mappings for first Mb of Dom0 
> memory')
> Signed-off-by: Roger Pau Monné 

Release-acked-by: Henry Wang 

Kind regards,
Henry

Re: [PATCH v7 8/8] xen/arm: mmu: move MMU specific P2M code to mmu/p2m.{c,h}

[Henry Wang]

Hi Julien,

> On Oct 18, 2023, at 02:13, Julien Grall  wrote:
> 
> Hi Henry,
> 
> On 17/10/2023 06:55, Henry Wang wrote:
>>> On Oct 14, 2023, at 02:22, Julien Grall  wrote:
>>> 
>>> Hi Henry,
>>> 
>>> On 09/10/2023 02:03, Henry Wang wrote:
 
 diff --git a/xen/arch/arm/include/asm/p2m.h 
 b/xen/arch/arm/include/asm/p2m.h
 index 940495d42b..a9622dac9a 100644
 --- a/xen/arch/arm/include/asm/p2m.h
 +++ b/xen/arch/arm/include/asm/p2m.h
 @@ -19,6 +19,22 @@ extern unsigned int p2m_root_level;
  #define P2M_ROOT_ORDERp2m_root_order
>>> 
>>> You seem to use P2M_ROOT_ORDER to allocate p2m->root in arm/p2m.c. However, 
>>> as I mentioned before, I don't think the defintion of p2m->root is suitable 
>>> for the MPU. I think the two functions using p2m->root should be moved in 
>>> mmu/p2m.c and P2M_ROOT_ORDER should be moved in mmu/p2m.h.
>> While working on this, I noticed that if we move p2m_final_teardown() (one 
>> of the two
>> functions you mentioned that consuming p2m->root), we also need to move the 
>> static
>> function p2m_free_vmid(). This seems reasonable as on MPU we only have 
>> dom0less
>> domUs so we don’t really need to free vmid.
> 
> Regardless on whether you need to free VMID on MPU, the allocation and free 
> functions should live in the same unit. So if you plan to move 
> p2m_final_teardown() in mmu/p2m.c, then p2m_free_vmid should be exported.

Sounds good, I will follow this way. Thanks.

Kind regards,
Henry

> 
> Cheers,
> 
> -- 
> Julien Grall

Re: [PATCH for-4.18] iommu/vt-d: use max supported AGAW

[Henry Wang]

Hi Roger,

> On Oct 17, 2023, at 21:09, Roger Pau Monne  wrote:
> 
> SAGAW is a bitmap field, with bits 1 and 2 signaling support for AGAW 1 and
> AGAW 2 respectively.  According to the Intel VT-d specification, an IOMMU 
> might
> support multiple AGAW values.
> 
> The AGAW value for each device is set in the device context entry, however
> there's a caveat related to the value the field supports depending on the
> translation type:
> 
> "When the Translation-type (T) field indicates pass-through (010b) or
> guest-mode (100b or 101b), this field must be programmed to indicate the
> largest AGAW value supported by hardware."
> 
> Of the translation types listed above Xen only uses pass-through (010b), and
> hence we need to make sure the context entry AGAW field is set appropriately,
> or else the IOMMU will report invalid context entry errors.
> 
> To do so calculate the IOMMU supported page table levels based on the last bit
> set in the SAGAW field, instead of the first one.  This also allows making use
> of the widest address width supported by the IOMMU, in case multiple AGAWs are
> supported.
> 
> Note that 859d11b27912 claims to replace the open-coded find_first_set_bit(),
> but it's actually replacing an open coded implementation to find the last set
> bit.
> 
> Fixes: 859d11b27912 ('VT-d: prune SAGAW recognition')
> Signed-off-by: Roger Pau Monné 

Release-acked-by: Henry Wang 

Kind regards,
Henry

[xen-unstable-smoke test] 183404: tolerable all pass - PUSHED

[osstest service owner]

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

Failures :-/ but no regressions.

Tests which did not succeed, but are not blocking:
 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
 test-armhf-armhf-xl  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  16 saverestore-support-checkfail   never pass

version targeted for testing:
 xen  f51c92383b8dc76233481e2814aa2e905fb9b501
baseline version:
 xen  7114bbfc8424ee467c6c8c82f077764ca4fa799b

Last test of basis   183396  2023-10-17 08:05:24 Z0 days
Testing same since   183404  2023-10-17 22:02:08 Z0 days1 attempts


People who touched revisions under test:
  George Dunlap 
  Michal Orzel 
  Stefano Stabellini 

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



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

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

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

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


Pushing revision :

To xenbits.xen.org:/home/xen/git/xen.git
   7114bbfc84..f51c92383b  f51c92383b8dc76233481e2814aa2e905fb9b501 -> smoke

Re: [XEN PATCH 10/10] arm/smmu: address violation of MISRA C:2012 Rule 8.2

[Stefano Stabellini]

On Tue, 17 Oct 2023, Julien Grall wrote:
> Hi Stefano,
> 
> On 16/10/2023 21:47, Stefano Stabellini wrote:
> > On Mon, 16 Oct 2023, Bertrand Marquis wrote:
> > > > On 16 Oct 2023, at 15:38, Julien Grall  wrote:
> > > > 
> > > > 
> > > > 
> > > > On 16/10/2023 14:31, Bertrand Marquis wrote:
> > > > > Hi Julien,
> > > > 
> > > > Hi Bertrand,
> > > > 
> > > > > > On 16 Oct 2023, at 11:07, Julien Grall  wrote:
> > > > > > 
> > > > > > Hi,
> > > > > > 
> > > > > > On 13/10/2023 16:24, Federico Serafini wrote:
> > > > > > > Add missing parameter names, no functional change.
> > > > > > > Signed-off-by: Federico Serafini 
> > > > > > > ---
> > > > > > >   xen/drivers/passthrough/arm/smmu.c | 6 +++---
> > > > > > 
> > > > > > This file is using the Linux coding style because it is imported
> > > > > > from Linux. I was under the impression we would exclude such file
> > > > > > for now.
> > > > > > 
> > > > > > Looking at exclude-list.json, it doesn't seem to be present. I think
> > > > > > this patch should be replaced with adding a line in
> > > > > > execlude-list.json.
> > > > > I think that during one of the discussions we said that this file
> > > > > already deviated quite a lot from the status in Linux and we wanted to
> > > > > turn it to Xen coding style in the future hence it is not listed in
> > > > > the exclude file.
> > > > AFAIK the SMMUv{1, 2} code didn't deviate very much from Linux. I can't
> > > > tell about the SMMUv3.
> > > 
> > > True that i had the v3 code in mind, we might not want to do that for v1/2
> > > you are right.
> > > 
> > > > 
> > > > > At the end having a working smmu might be critical in a safety context
> > > > > so it will make sense to also check this part of xen.
> > > > I don't buy this argument right now. We have files in exclude-lists.json
> > > > that I would consider critical for Xen to run (e.g. common/bitmap.c,
> > > > common/libfdt). So if you want to use this argument then we need to move
> > > > critical components of Xen out of the exclusion list.
> > > 
> > > I am sure we will at some point for runtime code but we cannot do
> > > everything on the first shot.
> > > I was not meaning to do that now but that it is something to consider.
> > 
> > Things that are in exclude-lists.json are source files that come from
> > other projects and also change very rarely. The argument that we don't
> > do MISRA C on the files in exclude-lists.json, it is not because those
> > files are unimportant, but because they change only once every many
> > years.
> 
> Interesting. I would have thought this would be a condition to do MISRA as the
> cost to port a patch would increase a bit but this is one time cost every many
> years. Whereas code like the SMMU are still actively developped. And in
> particular for SMMUv2 we tried to stick close to Linux to help backport. So
> this would be a reason to initially exclude it from MISRA.
> 
> > 
> > Of course the least we rely on exclude-lists.json the better.
> > 
> > For smmu.c, looking at the git history I think it is more actively
> > worked on than other files such as lib/rbtree.c or common/bitmap.c.
> > Given that backports from Linux to smmu.c are not straightforward anyway
> > (please correct me if I am wrong) then I think we should not add smmu.c
> > to exclude-lists.json and do MISRA for smmu.c.
> 
> I haven't done any recently. But if they are already not straightforward, then
> adding MISRA on top is not really to make it better. So I think if you want to
> do MISRA for the SMMU, then we need to fully convert it to Xen and abandon the
> idea to backport from Linux.
> 
> This would also make the code looks nicer as at the moment this contains
> wrapper just to stay as close as possible to Linux.

You have a good point. If we do MISRA for the SMMU then we might as well
fully convert the file to Xen. As a clarification, we can still look at
the fixes on the Linux driver and "port" security fixes and other key
patches such as workarounds for broken specific SMMU versions, but for
sure we wouldn't want to backport a new feature of the driver or code
refactoring / code improvements of the driver. But that probably is
already the case today?


> As a side note, the change here looks fairly self-contained. So I don't expect
> a major impact and therefore would not block this. This may not be the case
> for more complex one. Hence why I wanted to exclude it.

Thanks!


> Do you expect larger MISRA changes in the SMMU driver?

I'll let Federico answer this one.

Re: [XEN PATCH v2 00/10] address violations of MISRA C:2012 Directive 4.10

[Stefano Stabellini]

On Mon, 16 Oct 2023, Jan Beulich wrote:
> On 29.09.2023 00:24, Stefano Stabellini wrote:
> > On Thu, 28 Sep 2023, Jan Beulich wrote:
> >> On 28.09.2023 15:17, Simone Ballarin wrote:
> >>> On 28/09/23 14:51, Jan Beulich wrote:
>  On 28.09.2023 14:46, Simone Ballarin wrote:
> > On 13/09/23 10:02, Jan Beulich wrote:
> >> On 12.09.2023 11:36, Simone Ballarin wrote:
> >>> Add or move inclusion guards to address violations of
> >>> MISRA C:2012 Directive 4.10 ("Precautions shall be taken in order
> >>> to prevent the contents of a header file being included more than
> >>> once").
> >>>
> >>> Inclusion guards must appear at the beginning of the headers
> >>> (comments are permitted anywhere) and the #if directive cannot
> >>> be used for other checks.
> >>>
> >>> Simone Ballarin (10):
> >>> misra: add deviation for headers that explicitly avoid guards
> >>> misra: modify deviations for empty and generated headers
> >>> misra: add deviations for direct inclusion guards
> >>> xen/arm: address violations of MISRA C:2012 Directive 4.10
> >>> xen/x86: address violations of MISRA C:2012 Directive 4.10
> >>> x86/EFI: address violations of MISRA C:2012 Directive 4.10
> >>> xen/common: address violations of MISRA C:2012 Directive 4.10
> >>> xen/efi: address violations of MISRA C:2012 Directive 4.10
> >>> xen: address violations of MISRA C:2012 Directive 4.10
> >>> x86/asm: address violations of MISRA C:2012 Directive 4.10
> >>
> >> Just to mention it here again for the entire series, seeing that 
> >> despite
> >> my earlier comments to this effect a few R-b have arrived: If private
> >> headers need to gain guards (for, imo, no real reason), we first need 
> >> to
> >> settle on a naming scheme for these guards, such that guards used in
> >> private headers aren't at risk of colliding with ones used headers
> >> living in one of the usual include directories. IOW imo fair parts of
> >> this series may need redoing.
> >>
> >> Jan
> >>
> >
> > My proposal is:
> > - the relative path from "xen/arch" for files in this directory
> > (i.e. X86_X86_X86_MMCONFIG_H for "xen/arch/x86/x86_64/mmconfig.h";
> 
>  X86_X86_64_MMCONFIG_H that is?
> 
>  Yet then this scheme won't hold for xen/arch/include/asm/... ? It's also
>  not clear whether you're deliberately omitting leading/trailing 
>  underscores
>  here, which may be a way to distinguish private from global headers.
> >>>
> >>> Each name that begins with a double or single underscore (__, _) 
> >>> followed by an uppercase letter is reserved. Using a reserved identifier 
> >>> is an undefined-b.
> >>>
> >>> I would be better to avoid them.
> >>
> >> I'm with you about avoiding them, except that we use such all over the
> >> place. Taking this together with ...
> >>
> > - for the others, the entire path.
> 
>  What exactly is "entire" here?
> >>>
> >>> Let me try again.
> >>>
> >>> If we are inside xen/arch the relative path starting from this directory:
> >>>| xen/arch/x86/include/asm/compat.h
> >>>X86_INCLUDE_ASM_COMPAT_H
> >>>
> >>> For xen/include, the current convention.
> >>> Maybe, in a future patch, we can consider removing the leading _.
> >>>
> >>> For the others, the relative path after xen:
> >>>| xen/common/efi/efi.h
> >>>COMMON_EFI_EFI_H
> >>
> >> ... this you're effectively suggesting to change all existing guards.
> >> That's an option, but likely not a preferred one. Personally I'd prefer
> >> if in particular the headers in xen/include/ and in xen/arch/*include/
> >> didn't needlessly include _INCLUDE_ in their guard names.
> >>
> >> I'm really curious what others think.
> > 
> > If it is a MISRA requirement to avoid names that begin with single or
> > double underscore, then I think we should bite the bullet and change all
> > guard names, taking the opportunity to make them consistent.
> 
> Note how below you still suggest names with two leading underscores. I'm
> afraid ...

Sorry for the confusion. My example was an example of "path of least
resistance" trying to extrapolate the existing patterns. I was not
trying also to comply with the other MISRA rule about leading
underscores.

By "path of least resistance" I meant the smallest amount of work to
address Directive 4.10. As opposed to the "bite the bullet" approach
where we are willing to change everything to be consistent and also
address Directive 4.10.

So in my example of "path of least resistance" I kept the leading
underscores to minimize changes to the existing codebase.

The result of the MISRA C discussion this morning was that in general we
are *not* going to remove leading underscores everywhere, although
in general we would also like to avoid them when it can be done without
harming readability.

Now what does it mean for this patch seri

[PATCH] hvc/xen: fix event channel handling for secondary consoles

[David Woodhouse]

From: David Woodhouse 

The xencons_connect_backend() function allocates a local interdomain
event channel with xenbus_alloc_evtchn(), then calls
bind_interdomain_evtchn_to_irq_lateeoi() to bind to that port# on the
*remote* domain.

That doesn't work very well:

(qemu) device_add xen-console,id=con1,chardev=pty0
[   44.323872] xenconsole console-1: 2 xenbus_dev_probe on device/console/1
[   44.323995] xenconsole: probe of console-1 failed with error -2

Fix it to use bind_evtchn_to_irq_lateeoi(), which does the right thing
by just binding that *local* event channel to an irq. The backend will
do the interdomain binding.

This didn't affect the primary console because the setup for that is
special — the toolstack allocates the guest event channel and the guest
discovers it with HVMOP_get_param.

Once that's fixed, there's also a warning on hot-unplug because
xencons_disconnect_backend() unconditionally calls free_irq() via
unbind_from_irqhandler():

(qemu) device_del con1
[   32.050919] [ cut here ]
[   32.050942] Trying to free already-free IRQ 33
[   32.050990] WARNING: CPU: 0 PID: 51 at kernel/irq/manage.c:1895 
__free_irq+0x1d4/0x330

Fix that by calling notifier_del_irq() first, which only calls
free_irq() if the irq was requested in the first place. Then use
evtchn_put() to release the irq and event channel. Avoid calling
xenbus_free_evtchn() in the normal case, as evtchn_put() will do that
too. The only time xenbus_free_evtchn() needs to be called is for the
cleanup when bind_evtchn_to_irq_lateeoi() fails.

Finally, fix the error path in xen_hvc_init() when there's no primary
console. It should still register the frontend driver, as there may be
secondary consoles. (Qemu can always add secondary consoles, but only
the toolstack can add the primary because it's special.)

Fixes: fe415186b4 ("xen/console: harden hvc_xen against event channel storms")
Signed-off-by: David Woodhouse 
Cc: sta...@vger.kernel.org
---

Untested on actual Xen.

 drivers/tty/hvc/hvc_xen.c | 17 +++--
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c
index 98764e740c07..f0376612b267 100644
--- a/drivers/tty/hvc/hvc_xen.c
+++ b/drivers/tty/hvc/hvc_xen.c
@@ -377,9 +377,13 @@ void xen_console_resume(void)
 #ifdef CONFIG_HVC_XEN_FRONTEND
 static void xencons_disconnect_backend(struct xencons_info *info)
 {
-   if (info->irq > 0)
-   unbind_from_irqhandler(info->irq, NULL);
-   info->irq = 0;
+   if (info->irq > 0) {
+   notifier_del_irq(info->hvc, info->irq);
+   evtchn_put(info->evtchn);
+   info->irq = 0;
+   info->evtchn = 0;
+   }
+   /* evtchn_put() will also close it so this is only an error path */
if (info->evtchn > 0)
xenbus_free_evtchn(info->xbdev, info->evtchn);
info->evtchn = 0;
@@ -433,7 +437,7 @@ static int xencons_connect_backend(struct xenbus_device 
*dev,
if (ret)
return ret;
info->evtchn = evtchn;
-   irq = bind_interdomain_evtchn_to_irq_lateeoi(dev, evtchn);
+   irq = bind_evtchn_to_irq_lateeoi(evtchn);
if (irq < 0)
return irq;
info->irq = irq;
@@ -597,7 +601,7 @@ static int __init xen_hvc_init(void)
else
r = xen_pv_console_init();
if (r < 0)
-   return r;
+   goto register_fe;
 
info = vtermno_to_xencons(HVC_COOKIE);
info->irq = bind_evtchn_to_irq_lateeoi(info->evtchn);
@@ -616,12 +620,13 @@ static int __init xen_hvc_init(void)
list_del(&info->list);
spin_unlock_irqrestore(&xencons_lock, flags);
if (info->irq)
-   unbind_from_irqhandler(info->irq, NULL);
+   evtchn_put(info->evtchn);
kfree(info);
return r;
}
 
r = 0;
+ register_fe:
 #ifdef CONFIG_HVC_XEN_FRONTEND
r = xenbus_register_frontend(&xencons_driver);
 #endif
-- 
2.40.1




smime.p7s
Description: S/MIME cryptographic signature

[xen-unstable test] 183399: tolerable FAIL

[osstest service owner]

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

Failures :-/ but no regressions.

Tests which are failing intermittently (not blocking):
 test-amd64-i386-xl-qemuu-debianhvm-i386-xsm 12 debian-hvm-install fail in 
183392 pass in 183399
 test-amd64-amd64-xl-qemuu-win7-amd64 12 windows-install fail in 183392 pass in 
183399
 test-armhf-armhf-libvirt-raw 13 guest-startfail pass in 183392

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stop  fail blocked in 183392
 test-armhf-armhf-libvirt-raw 15 saverestore-support-check fail in 183392 
blocked in 183399
 test-armhf-armhf-libvirt-raw 14 migrate-support-check fail in 183392 never pass
 test-armhf-armhf-libvirt 16 saverestore-support-checkfail  like 183392
 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 183392
 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 183392
 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 183392
 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 183392
 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 183392
 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 183392
 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check   fail like 183392
 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 183392
 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 183392
 test-amd64-i386-xl-pvshim14 guest-start  fail   never pass
 test-amd64-i386-libvirt-xsm  15 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  16 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail never pass
 test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail  never pass
 test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail  never pass
 test-armhf-armhf-libvirt 15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  16 saverestore-support-checkfail   never pass
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail never pass
 test-amd64-amd64-libvirt 15 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt-raw  14 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-vhd  14 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-vhd  15 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  14 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  15 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-credit1  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit1  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass

version targeted for testing:
 xen  0ce2ee7a16f2886c32b3f070bba3172f4a577aa4
baseline version:
 xen  

Re: [PATCH] ui/input: Constify QemuInputHandler structure

[Mark Cave-Ayland]

On 17/10/2023 14:12, Philippe Mathieu-Daudé wrote:


Access to QemuInputHandlerState::handler are read-only.

Signed-off-by: Philippe Mathieu-Daudé 
---
  include/hw/virtio/virtio-input.h | 2 +-
  include/ui/input.h   | 2 +-
  chardev/msmouse.c| 2 +-
  chardev/wctablet.c   | 2 +-
  hw/char/escc.c   | 2 +-
  hw/display/xenfb.c   | 6 +++---
  hw/input/adb-kbd.c   | 2 +-
  hw/input/hid.c   | 6 +++---
  hw/input/ps2.c   | 4 ++--
  hw/input/virtio-input-hid.c  | 8 
  ui/input-legacy.c| 2 +-
  ui/input.c   | 4 ++--
  ui/vdagent.c | 2 +-
  13 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/include/hw/virtio/virtio-input.h b/include/hw/virtio/virtio-input.h
index 08f1591424..a6c9703644 100644
--- a/include/hw/virtio/virtio-input.h
+++ b/include/hw/virtio/virtio-input.h
@@ -84,7 +84,7 @@ struct VirtIOInputHID {
  VirtIOInput   parent_obj;
  char  *display;
  uint32_t  head;
-QemuInputHandler  *handler;
+const QemuInputHandler*handler;
  QemuInputHandlerState *hs;
  int   ledstate;
  bool  wheel_axis;
diff --git a/include/ui/input.h b/include/ui/input.h
index 24d8e4579e..8f9aac562e 100644
--- a/include/ui/input.h
+++ b/include/ui/input.h
@@ -30,7 +30,7 @@ struct QemuInputHandler {
  };
  
  QemuInputHandlerState *qemu_input_handler_register(DeviceState *dev,

-   QemuInputHandler *handler);
+const QemuInputHandler *handler);
  void qemu_input_handler_activate(QemuInputHandlerState *s);
  void qemu_input_handler_deactivate(QemuInputHandlerState *s);
  void qemu_input_handler_unregister(QemuInputHandlerState *s);
diff --git a/chardev/msmouse.c b/chardev/msmouse.c
index ab8fe981d6..a774c397b4 100644
--- a/chardev/msmouse.c
+++ b/chardev/msmouse.c
@@ -171,7 +171,7 @@ static int msmouse_chr_write(struct Chardev *s, const 
uint8_t *buf, int len)
  return len;
  }
  
-static QemuInputHandler msmouse_handler = {

+static const QemuInputHandler msmouse_handler = {
  .name  = "QEMU Microsoft Mouse",
  .mask  = INPUT_EVENT_MASK_BTN | INPUT_EVENT_MASK_REL,
  .event = msmouse_input_event,
diff --git a/chardev/wctablet.c b/chardev/wctablet.c
index 43bdf6b608..f4008bf35b 100644
--- a/chardev/wctablet.c
+++ b/chardev/wctablet.c
@@ -178,7 +178,7 @@ static void wctablet_input_sync(DeviceState *dev)
  }
  }
  
-static QemuInputHandler wctablet_handler = {

+static const QemuInputHandler wctablet_handler = {
  .name  = "QEMU Wacom Pen Tablet",
  .mask  = INPUT_EVENT_MASK_BTN | INPUT_EVENT_MASK_ABS,
  .event = wctablet_input_event,
diff --git a/hw/char/escc.c b/hw/char/escc.c
index 4be66053c1..48b30ee760 100644
--- a/hw/char/escc.c
+++ b/hw/char/escc.c
@@ -845,7 +845,7 @@ static void sunkbd_handle_event(DeviceState *dev, 
QemuConsole *src,
  put_queue(s, keycode);
  }
  
-static QemuInputHandler sunkbd_handler = {

+static const QemuInputHandler sunkbd_handler = {
  .name  = "sun keyboard",
  .mask  = INPUT_EVENT_MASK_KEY,
  .event = sunkbd_handle_event,
diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
index 0074a9b6f8..b2130a0d70 100644
--- a/hw/display/xenfb.c
+++ b/hw/display/xenfb.c
@@ -321,20 +321,20 @@ static void xenfb_mouse_sync(DeviceState *dev)
  xenfb->wheel = 0;
  }
  
-static QemuInputHandler xenfb_keyboard = {

+static const QemuInputHandler xenfb_keyboard = {
  .name  = "Xen PV Keyboard",
  .mask  = INPUT_EVENT_MASK_KEY,
  .event = xenfb_key_event,
  };
  
-static QemuInputHandler xenfb_abs_mouse = {

+static const QemuInputHandler xenfb_abs_mouse = {
  .name  = "Xen PV Mouse",
  .mask  = INPUT_EVENT_MASK_BTN | INPUT_EVENT_MASK_ABS,
  .event = xenfb_mouse_event,
  .sync  = xenfb_mouse_sync,
  };
  
-static QemuInputHandler xenfb_rel_mouse = {

+static const QemuInputHandler xenfb_rel_mouse = {
  .name  = "Xen PV Mouse",
  .mask  = INPUT_EVENT_MASK_BTN | INPUT_EVENT_MASK_REL,
  .event = xenfb_mouse_event,
diff --git a/hw/input/adb-kbd.c b/hw/input/adb-kbd.c
index a9088c910c..e21edf9acd 100644
--- a/hw/input/adb-kbd.c
+++ b/hw/input/adb-kbd.c
@@ -355,7 +355,7 @@ static void adb_kbd_reset(DeviceState *dev)
  s->count = 0;
  }
  
-static QemuInputHandler adb_keyboard_handler = {

+static const QemuInputHandler adb_keyboard_handler = {
  .name  = "QEMU ADB Keyboard",
  .mask  = INPUT_EVENT_MASK_KEY,
  .event = adb_keyboard_event,
diff --git a/hw/input/hid.c b/hw/input/hid.c
index a9c7dd1ce1..b8e85374ca 100644
--- a/hw/input/hid.c
+++ b/hw/input/hid.c
@@ -510,20 +510,20 @@ void hid_free(HIDState *hs)
  hid_del_idle_timer(hs);
  }
  
-s

Re: [PATCH v4 2/2] xen/arm: Enlarge identity map space to 10TB

[Stefano Stabellini]

On Mon, 15 Oct 2023, Julien Grall wrote:
> On 16/10/2023 16:07, Bertrand Marquis wrote:
> > > On 16 Oct 2023, at 16:46, Julien Grall  wrote:
> > > CC Henry
> > > 
> > > Hi Alexey,
> > > 
> > > On 16/10/2023 15:24, Alexey Klimov wrote:
> > > > On Mon, 16 Oct 2023 at 15:13, Luca Fancellu 
> > > > wrote:
> > > > > 
> > > > > 
> > > > > 
> > > > > > On 16 Oct 2023, at 15:00, Bertrand Marquis
> > > > > >  wrote:
> > > > > > 
> > > > > > Hi
> > > > > > 
> > > > > > +Luca and Rahul
> > > > > > 
> > > > > > > On 16 Oct 2023, at 15:54, Julien Grall  wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > On 16/10/2023 09:44, Michal Orzel wrote:
> > > > > > > > Hi,
> > > > > > > 
> > > > > > > Hi,
> > > > > > > 
> > > > > > > > On 13/10/2023 14:26, Leo Yan wrote:
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > On ADLink AVA platform (Ampere Altra SoC with 32 Arm Neoverse
> > > > > > > > > N1 cores),
> > > > > > > > > the physical memory regions are:
> > > > > > > > > 
> > > > > > > > >   DRAM memory regions:
> > > > > > > > > Node[0] Region[0]: 0x8000 - 0x
> > > > > > > > > Node[0] Region[1]: 0x0800 - 0x08007fff
> > > > > > > > > Node[0] Region[2]: 0x0801 - 0x0807
> > > > > > > > > 
> > > > > > > > > The UEFI loads Xen hypervisor and DTB into the high memory,
> > > > > > > > > the kernel
> > > > > > > > > and ramdisk images are loaded into the low memory space:
> > > > > > > > > 
> > > > > > > > >   (XEN) MODULE[0]: 0807f6df - 0807f6f3e000 Xen
> > > > > > > > >   (XEN) MODULE[1]: 0807f8054000 - 0807f8056000 Device
> > > > > > > > > Tree
> > > > > > > > >   (XEN) MODULE[2]: fa834000 - fc5de1d5 Ramdisk
> > > > > > > > >   (XEN) MODULE[3]: fc5df000 - ffb3f810 Kernel
> > > > > > > > > 
> > > > > > > > > In this case, the Xen binary is loaded above 8TB, which
> > > > > > > > > exceeds the
> > > > > > > > > maximum supported identity map space of 2TB in Xen.
> > > > > > > > > Consequently, the
> > > > > > > > > system fails to boot.
> > > > > > > > > 
> > > > > > > > > This patch enlarges identity map space to 10TB, allowing
> > > > > > > > > module loading
> > > > > > > > > within the range of [0x0 .. 0x09ff__].
> > > > > > > > > 
> > > > > > > > > Fixes: 1c78d76b67 ("xen/arm64: mm: Introduce helpers to
> > > > > > > > > prepare/enable/disable")
> > > > > > > > I don't think a fixes tag applies here given that 2TB was just a
> > > > > > > > number we believed is enough
> > > > > > > > and all of this is platform dependent.
> > > > > > > > This can be dropped on commit if committer agrees
> > > > > > > Xen may have booted on that platform before hand. So this would be
> > > > > > > considered a regression and therefore a tag would be warrant.
> > > > > > > 
> > > > > > > AFAICT, the commit is only present on the upcoming 4.18. So the
> > > > > > > question is whether Xen 4.17 booted out-of-the-box on ADLink? If
> > > > > > > the answer is yes, then we need to add a Fixes tag. But the
> > > > > > > correct one would be
> > > > > > > 
> > > > > > 
> > > > > > @Rahul or Luca: could you give an answer here ?
> > > > > > I know you used Xen on an AVA platform but was it booting out of the
> > > > > > box ?
> > > > > 
> > > > > I can’t say for Xen 4.17, but our nightly job has run successfully on
> > > > > AVA for the commit 730406ab81094115d9fb5ca00ba8d53cec1279b3
> > > > > (docs/misra: add deviations.rst to document additional deviations.)
> > > > > 
> > > > > We are not applying any patch for it to run on AVA.
> > > > Most likely it is because your UEFI/BIOS firmware is 2.x, for instance
> > > > 2.04.100.07.
> > > > This fix if for AVA machine with older UEFI firmware 1.07.300.03.
> > > 
> > > OOI, why not updating your firmware? I was expecting that it would also
> > > contain other critical fixes.
> > > 
> > > With this in mind, I am now more in two mind to ask to merge this patch in
> > > Xen 4.18. On one hand, I understand it will help to boot on AVA machine
> > > with an older firmware. But on the other hand this is changing the memory
> > > layout quite late in the release. The risk seems limited because Xen is
> > > not loaded at the top of the virtual address space (there is the directmap
> > > afterwards).
> > > 
> > > Henry (as the release manager) and others, any opinions?
> > 
> > With the new information, I think it should be stated that it is fixing an
> > issue on AVA platforms using an old firmware and platforms with and
> > up-to-date firmware are not impacted.
> > It is an important information to keep in mind that this is not a fix to be
> > backported for example (and for me the fixes tag should not be kept).
> 
> IMHO, the fixes tag should not be based solely on the AVA platform. It should
> be based on whether this could sensibly affect others. Right now, there is
> nothing that would indicate either way.
> 
> And therefore a Fixes tag is sensible. This doesn't me

Re: [PATCH 3/4] [WTF] avoid qemu_del_nic() in xen_netdev_unrealize() on shutdown

[David Woodhouse]

On Tue, 2023-10-17 at 19:25 +0100, David Woodhouse wrote:
> From: David Woodhouse 
> 
> When QEMU is exiting, qemu_cleanup() calls net_cleanup(), which deletes
> the NIC from underneath the xen-net-device. When xen_netdev_unrealize()
> is later called via the xenbus exit notifier, it crashes.
> 
> Signed-off-by: David Woodhouse 
> ---
>  hw/net/xen_nic.c | 8 +++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/net/xen_nic.c b/hw/net/xen_nic.c
> index 84914c329c..8d25fb3101 100644
> --- a/hw/net/xen_nic.c
> +++ b/hw/net/xen_nic.c
> @@ -25,6 +25,8 @@
>  #include "qapi/qmp/qdict.h"
>  #include "qapi/error.h"
>  
> +#include "sysemu/runstate.h"
> +
>  #include 
>  #include 
>  #include 
> @@ -530,7 +532,11 @@ static void xen_netdev_unrealize(XenDevice *xendev)
>  /* Disconnect from the frontend in case this has not already happened */
>  xen_netdev_disconnect(xendev, NULL);
>  
> -    if (netdev->nic) {
> +    /*
> + * WTF? In RUN_STATE_SHUTDOWN, qemu_cleanup()→net_cleanup() already 
> deleted
> + * our NIC from underneath us!
> + */
> +    if (netdev->nic && !runstate_check(RUN_STATE_SHUTDOWN)) {
>  qemu_del_nic(netdev->nic);
>  }
>  }

I wonder if this is the better answer? There's no point deleting the
*NICs*, is there? It's the other net clients we really want to clean
up?

--- a/net/net.c
+++ b/net/net.c
@@ -1499,18 +1499,22 @@ static void net_vm_change_state_handler(void *opaque, 
bool running,
 
 void net_cleanup(void)
 {
-NetClientState *nc;
+NetClientState *nc, **p = &net_clients.tqh_first;
 
 /*cleanup colo compare module for COLO*/
 colo_compare_cleanup();
 
-/* We may del multiple entries during qemu_del_net_client(),
- * so QTAILQ_FOREACH_SAFE() is also not safe here.
+/*
+ * We may del multiple entries during qemu_del_net_client(), so
+ * QTAILQ_FOREACH_SAFE() is not safe here. The only safe pointer
+ * to keep is a NET_CLIENT_DRIVER_NIC entry, as we don't want
+ * to delete those (we'd upset the devices which own them, if we
+ * did).
  */
-while (!QTAILQ_EMPTY(&net_clients)) {
-nc = QTAILQ_FIRST(&net_clients);
+while (*p) {
+nc = *p;
 if (nc->info->type == NET_CLIENT_DRIVER_NIC) {
-qemu_del_nic(qemu_get_nic(nc));
+p = &nc->next.tqe_next;
 } else {
 qemu_del_net_client(nc);
 }



smime.p7s
Description: S/MIME cryptographic signature

[PATCH 0/4] Update QEMU qnic driver to "new" XenDevice model

[David Woodhouse]

This has been on my TODO list for a while, and Paul's since 2019. Having 
converted the console driver just to get PV guests booting, I figured I 
should do this one while I still remember how.

The fact that net_cleanup() frees my NIC from underneath me confused
me for a while. Not entirely sure what's going on there. Other devices
seem to survive just because they aren't cleaned up at exit. But XenBus
devices really should be properly cleaned up on exit, because in some
cases they leave detritus in XenStore, which outlives QEMU. So "Don't
Do That Then" doesn't seem like it's the answer.

The default NIC handling is horrid (I mean, before I even looked at it)
but that isn't today's yak to shave...

David Woodhouse (4):
  hw/xen: only remove peers of PCI NICs on unplug
  hw/xen: update Xen PV NIC to XenDevice model
  [WTF] avoid qemu_del_nic() in xen_netdev_unrealize() on shutdown
  hw/i386/pc: support '-nic' for xen-net-device

 hw/i386/pc.c   |  11 ++-
 hw/i386/pc_piix.c  |   2 +-
 hw/i386/pc_q35.c   |   2 +-
 hw/i386/xen/xen_platform.c |   9 ++-
 hw/net/meson.build |   2 +-
 hw/net/trace-events|   9 +++
 hw/net/xen_nic.c   | 434 
+++
 hw/xen/xen-bus.c   |   4 +-
 hw/xenpv/xen_machine_pv.c  |   1 -
 include/hw/i386/pc.h   |   4 +-
 include/hw/xen/xen-bus.h   |   2 +-
 11 files changed, 373 insertions(+), 107 deletions(-)

[PATCH 2/4] hw/xen: update Xen PV NIC to XenDevice model

[David Woodhouse]

From: David Woodhouse 

Signed-off-by: David Woodhouse 
---
 hw/net/meson.build|   2 +-
 hw/net/trace-events   |   9 +
 hw/net/xen_nic.c  | 426 +-
 hw/xenpv/xen_machine_pv.c |   1 -
 4 files changed, 342 insertions(+), 96 deletions(-)

diff --git a/hw/net/meson.build b/hw/net/meson.build
index 2632634df3..f64651c467 100644
--- a/hw/net/meson.build
+++ b/hw/net/meson.build
@@ -1,5 +1,5 @@
 system_ss.add(when: 'CONFIG_DP8393X', if_true: files('dp8393x.c'))
-system_ss.add(when: 'CONFIG_XEN', if_true: files('xen_nic.c'))
+system_ss.add(when: 'CONFIG_XEN_BUS', if_true: files('xen_nic.c'))
 system_ss.add(when: 'CONFIG_NE2000_COMMON', if_true: files('ne2000.c'))
 
 # PCI network cards
diff --git a/hw/net/trace-events b/hw/net/trace-events
index 3abfd65e5b..ee56acfbce 100644
--- a/hw/net/trace-events
+++ b/hw/net/trace-events
@@ -482,3 +482,12 @@ dp8393x_receive_oversize(int size) "oversize packet, 
pkt_size is %d"
 dp8393x_receive_not_netcard(void) "packet not for netcard"
 dp8393x_receive_packet(int crba) "Receive packet at 0x%"PRIx32
 dp8393x_receive_write_status(int crba) "Write status at 0x%"PRIx32
+
+# xen_nic.c
+xen_netdev_realize(int idx, const char *info) "idx %u info '%s'"
+xen_netdev_unrealize(int idx) "idx %u"
+xen_netdev_create(int idx) "idx %u"
+xen_netdev_destroy(int idx) "idx %u"
+xen_netdev_disconnect(int idx) "idx %u"
+xen_netdev_connect(int idx, unsigned int tx, unsigned int rx, int port) "idx 
%u tx %u rx %u port %u"
+xen_netdev_frontend_changed(const char *idx, int state) "idx %s state %d"
diff --git a/hw/net/xen_nic.c b/hw/net/xen_nic.c
index 9bbf6599fc..84914c329c 100644
--- a/hw/net/xen_nic.c
+++ b/hw/net/xen_nic.c
@@ -20,6 +20,11 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/cutils.h"
+#include "qemu/qemu-print.h"
+#include "qapi/qmp/qdict.h"
+#include "qapi/error.h"
+
 #include 
 #include 
 #include 
@@ -27,18 +32,26 @@
 #include "net/net.h"
 #include "net/checksum.h"
 #include "net/util.h"
-#include "hw/xen/xen-legacy-backend.h"
+
+#include "hw/xen/xen-backend.h"
+#include "hw/xen/xen-bus-helper.h"
+#include "hw/qdev-properties.h"
+#include "hw/qdev-properties-system.h"
 
 #include "hw/xen/interface/io/netif.h"
+#include "hw/xen/interface/io/xs_wire.h"
+
+#include "trace.h"
 
 /* - */
 
 struct XenNetDev {
-struct XenLegacyDevice  xendev;  /* must be first */
-char  *mac;
+struct XenDevice  xendev;  /* must be first */
+XenEventChannel   *event_channel;
+int   dev;
 int   tx_work;
-int   tx_ring_ref;
-int   rx_ring_ref;
+unsigned int  tx_ring_ref;
+unsigned int  rx_ring_ref;
 struct netif_tx_sring *txs;
 struct netif_rx_sring *rxs;
 netif_tx_back_ring_t  tx_ring;
@@ -47,6 +60,13 @@ struct XenNetDev {
 NICState  *nic;
 };
 
+typedef struct XenNetDev XenNetDev;
+
+#define TYPE_XEN_NET_DEVICE "xen-net-device"
+OBJECT_DECLARE_SIMPLE_TYPE(XenNetDev, XEN_NET_DEVICE)
+
+#define xen_pv_printf(a, n, ...) qemu_printf(__VA_ARGS__)
+
 /* - */
 
 static void net_tx_response(struct XenNetDev *netdev, netif_tx_request_t *txp, 
int8_t st)
@@ -68,7 +88,8 @@ static void net_tx_response(struct XenNetDev *netdev, 
netif_tx_request_t *txp, i
 netdev->tx_ring.rsp_prod_pvt = ++i;
 RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(&netdev->tx_ring, notify);
 if (notify) {
-xen_pv_send_notify(&netdev->xendev);
+xen_device_notify_event_channel(XEN_DEVICE(netdev),
+netdev->event_channel, NULL);
 }
 
 if (i == netdev->tx_ring.req_cons) {
@@ -104,8 +125,9 @@ static void net_tx_error(struct XenNetDev *netdev, 
netif_tx_request_t *txp, RING
 #endif
 }
 
-static void net_tx_packets(struct XenNetDev *netdev)
+static bool net_tx_packets(struct XenNetDev *netdev)
 {
+bool done_something = false;
 netif_tx_request_t txreq;
 RING_IDX rc, rp;
 void *page;
@@ -122,6 +144,7 @@ static void net_tx_packets(struct XenNetDev *netdev)
 }
 memcpy(&txreq, RING_GET_REQUEST(&netdev->tx_ring, rc), 
sizeof(txreq));
 netdev->tx_ring.req_cons = ++rc;
+done_something = true;
 
 #if 1
 /* should not happen in theory, we don't announce the *
@@ -151,7 +174,7 @@ static void net_tx_packets(struct XenNetDev *netdev)
 continue;
 }
 
-xen_pv_printf(&netdev->xendev, 3,
+if (0) qemu_printf(//&netdev->xendev, 3,
   "tx packet ref %d, off %d, len %d, flags 
0x%x%s%s%s%s\n",
   txreq.gref, txreq.offset, txreq.size, txreq.flags,
   (txreq.flags & NETTXF_csum_blank) ? " 
csum_blank" : "",
@@ -159,8 +182,8 @@ static void net_tx_packets(stru

[PATCH 1/4] hw/xen: only remove peers of PCI NICs on unplug

[David Woodhouse]

From: David Woodhouse 

When the Xen guest asks to unplug *emulated* NICs, it's kind of unhelpful
also to unplug the peer of the *Xen* PV NIC.

Signed-off-by: David Woodhouse 
---
 hw/i386/xen/xen_platform.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/hw/i386/xen/xen_platform.c b/hw/i386/xen/xen_platform.c
index 17457ff3de..e2dd1b536a 100644
--- a/hw/i386/xen/xen_platform.c
+++ b/hw/i386/xen/xen_platform.c
@@ -140,9 +140,14 @@ static void unplug_nic(PCIBus *b, PCIDevice *d, void *o)
 /* Remove the peer of the NIC device. Normally, this would be a tap device. */
 static void del_nic_peer(NICState *nic, void *opaque)
 {
-NetClientState *nc;
+NetClientState *nc = qemu_get_queue(nic);
+ObjectClass *klass = module_object_class_by_name(nc->model);
+
+/* Only delete peers of PCI NICs that we're about to delete */
+if (!klass || !object_class_dynamic_cast(klass, TYPE_PCI_DEVICE)) {
+return;
+}
 
-nc = qemu_get_queue(nic);
 if (nc->peer)
 qemu_del_net_client(nc->peer);
 }
-- 
2.40.1

[PATCH 4/4] hw/i386/pc: support '-nic' for xen-net-device

[David Woodhouse]

From: David Woodhouse 

The default NIC creation seems a bit hackish to me. I don't understand
why each platform ha to call pci_nic_init_nofail() from a point in the
code where it actually has a pointer to the PCI bus, and then we have
the special cases for things like ne2k_isa.

If qmp_device_add() can *find* the appropriate bus and instantiate
the device on it, why can't we just do that from generic code for
creating the default NICs too?

But that isn't a yak I want to shave today. Add a xenbus field to the
PCMachineState so that it can make its way from pc_basic_device_init()
to pc_nic_init() and be handled as a special case like ne2k_isa is.

Now we can launch emulated Xen guests with '-nic user'.

Signed-off-by: David Woodhouse 
---
 hw/i386/pc.c | 11 ---
 hw/i386/pc_piix.c|  2 +-
 hw/i386/pc_q35.c |  2 +-
 hw/xen/xen-bus.c |  4 +++-
 include/hw/i386/pc.h |  4 +++-
 include/hw/xen/xen-bus.h |  2 +-
 6 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index bb3854d1d0..7413ca50c8 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1265,7 +1265,7 @@ void pc_basic_device_init(struct PCMachineState *pcms,
 if (pcms->bus) {
 pci_create_simple(pcms->bus, -1, "xen-platform");
 }
-xen_bus_init();
+pcms->xenbus = xen_bus_init();
 xen_be_init();
 }
 #endif
@@ -1291,7 +1291,8 @@ void pc_basic_device_init(struct PCMachineState *pcms,
 pcms->vmport != ON_OFF_AUTO_ON);
 }
 
-void pc_nic_init(PCMachineClass *pcmc, ISABus *isa_bus, PCIBus *pci_bus)
+void pc_nic_init(PCMachineClass *pcmc, ISABus *isa_bus, PCIBus *pci_bus,
+ BusState *xen_bus)
 {
 MachineClass *mc = MACHINE_CLASS(pcmc);
 int i;
@@ -1301,7 +1302,11 @@ void pc_nic_init(PCMachineClass *pcmc, ISABus *isa_bus, 
PCIBus *pci_bus)
 NICInfo *nd = &nd_table[i];
 const char *model = nd->model ? nd->model : mc->default_nic;
 
-if (g_str_equal(model, "ne2k_isa")) {
+if (xen_bus && (!nd->model || g_str_equal(model, "xen-net-device"))) {
+DeviceState *dev = qdev_new("xen-net-device");
+qdev_set_nic_properties(dev, nd);
+qdev_realize_and_unref(dev, xen_bus, &error_fatal);
+} else if (g_str_equal(model, "ne2k_isa")) {
 pc_init_ne2k_isa(isa_bus, nd);
 } else {
 pci_nic_init_nofail(nd, pci_bus, model, NULL);
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index e36a3262b2..90b5ae7258 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -322,7 +322,7 @@ static void pc_init1(MachineState *machine,
 pc_basic_device_init(pcms, isa_bus, x86ms->gsi, rtc_state, true,
  0x4);
 
-pc_nic_init(pcmc, isa_bus, pci_bus);
+pc_nic_init(pcmc, isa_bus, pci_bus, pcms->xenbus);
 
 if (pcmc->pci_enabled) {
 PCIDevice *dev;
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index a7386f2ca2..2ed0aab2a7 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -340,7 +340,7 @@ static void pc_q35_init(MachineState *machine)
 
 /* the rest devices to which pci devfn is automatically assigned */
 pc_vga_init(isa_bus, host_bus);
-pc_nic_init(pcmc, isa_bus, host_bus);
+pc_nic_init(pcmc, isa_bus, host_bus, pcms->xenbus);
 
 if (machine->nvdimms_state->is_enabled) {
 nvdimm_init_acpi_state(machine->nvdimms_state, system_io,
diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
index 0da2aa219a..d7823964f8 100644
--- a/hw/xen/xen-bus.c
+++ b/hw/xen/xen-bus.c
@@ -1127,11 +1127,13 @@ static void xen_register_types(void)
 
 type_init(xen_register_types)
 
-void xen_bus_init(void)
+BusState *xen_bus_init(void)
 {
 DeviceState *dev = qdev_new(TYPE_XEN_BRIDGE);
 BusState *bus = qbus_new(TYPE_XEN_BUS, dev, NULL);
 
 sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
 qbus_set_bus_hotplug_handler(bus);
+
+return bus;
 }
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index bec38cb92c..feabf3d195 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -33,6 +33,7 @@ typedef struct PCMachineState {
 
 /* Pointers to devices and objects: */
 PCIBus *bus;
+BusState *xenbus;
 I2CBus *smbus;
 PFlashCFI01 *flash[2];
 ISADevice *pcspk;
@@ -182,7 +183,8 @@ void pc_basic_device_init(struct PCMachineState *pcms,
 void pc_cmos_init(PCMachineState *pcms,
   BusState *ide0, BusState *ide1,
   ISADevice *s);
-void pc_nic_init(PCMachineClass *pcmc, ISABus *isa_bus, PCIBus *pci_bus);
+void pc_nic_init(PCMachineClass *pcmc, ISABus *isa_bus, PCIBus *pci_bus,
+ BusState *xen_bus);
 
 void pc_i8259_create(ISABus *isa_bus, qemu_irq *i8259_irqs);
 
diff --git a/include/hw/xen/xen-bus.h b/include/hw/xen/xen-bus.h
index eb440880b5..acad871b80 100644
--- a/include/hw/xen/xen-bus.h
+++ b/include/hw/xen/xen-bus.h
@@ -75,7 +75,7 @@ struct XenBusClass {
 OBJEC

[PATCH 3/4] [WTF] avoid qemu_del_nic() in xen_netdev_unrealize() on shutdown

[David Woodhouse]

From: David Woodhouse 

When QEMU is exiting, qemu_cleanup() calls net_cleanup(), which deletes
the NIC from underneath the xen-net-device. When xen_netdev_unrealize()
is later called via the xenbus exit notifier, it crashes.

Signed-off-by: David Woodhouse 
---
 hw/net/xen_nic.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/net/xen_nic.c b/hw/net/xen_nic.c
index 84914c329c..8d25fb3101 100644
--- a/hw/net/xen_nic.c
+++ b/hw/net/xen_nic.c
@@ -25,6 +25,8 @@
 #include "qapi/qmp/qdict.h"
 #include "qapi/error.h"
 
+#include "sysemu/runstate.h"
+
 #include 
 #include 
 #include 
@@ -530,7 +532,11 @@ static void xen_netdev_unrealize(XenDevice *xendev)
 /* Disconnect from the frontend in case this has not already happened */
 xen_netdev_disconnect(xendev, NULL);
 
-if (netdev->nic) {
+/*
+ * WTF? In RUN_STATE_SHUTDOWN, qemu_cleanup()→net_cleanup() already deleted
+ * our NIC from underneath us!
+ */
+if (netdev->nic && !runstate_check(RUN_STATE_SHUTDOWN)) {
 qemu_del_nic(netdev->nic);
 }
 }
-- 
2.40.1

Re: [PATCH v7 8/8] xen/arm: mmu: move MMU specific P2M code to mmu/p2m.{c,h}

[Julien Grall]

Hi Henry,

On 17/10/2023 06:55, Henry Wang wrote:

On Oct 14, 2023, at 02:22, Julien Grall  wrote:

Hi Henry,

On 09/10/2023 02:03, Henry Wang wrote:


diff --git a/xen/arch/arm/include/asm/p2m.h b/xen/arch/arm/include/asm/p2m.h
index 940495d42b..a9622dac9a 100644
--- a/xen/arch/arm/include/asm/p2m.h
+++ b/xen/arch/arm/include/asm/p2m.h
@@ -19,6 +19,22 @@ extern unsigned int p2m_root_level;
  #define P2M_ROOT_ORDERp2m_root_order


You seem to use P2M_ROOT_ORDER to allocate p2m->root in arm/p2m.c. However, as I 
mentioned before, I don't think the defintion of p2m->root is suitable for the MPU. I 
think the two functions using p2m->root should be moved in mmu/p2m.c and 
P2M_ROOT_ORDER should be moved in mmu/p2m.h.


While working on this, I noticed that if we move p2m_final_teardown() (one of 
the two
functions you mentioned that consuming p2m->root), we also need to move the 
static
function p2m_free_vmid(). This seems reasonable as on MPU we only have dom0less
domUs so we don’t really need to free vmid.


Regardless on whether you need to free VMID on MPU, the allocation and 
free functions should live in the same unit. So if you plan to move 
p2m_final_teardown() in mmu/p2m.c, then p2m_free_vmid should be exported.


Cheers,

--
Julien Grall

Re: [PATCH 11/12] hw/xen: automatically assign device index to block devices

[David Woodhouse]

On Tue, 2023-10-17 at 12:21 +0200, Kevin Wolf wrote:
> Am 16.10.2023 um 17:19 hat David Woodhouse geschrieben:
> > From: David Woodhouse 
> > 
> > There's no need to force the user to assign a vdev. We can automatically
> > assign one, starting at xvda and searching until we find the first disk
> > name that's unused.
> > 
> > This means we can now allow '-drive if=xen,file=xxx' to work without an
> > explicit separate -driver argument, just like if=virtio.
> > 
> > Signed-off-by: David Woodhouse 
> 
> > @@ -34,6 +34,31 @@ static char *xen_block_get_name(XenDevice *xendev, Error 
> > **errp)
> >  XenBlockDevice *blockdev = XEN_BLOCK_DEVICE(xendev);
> >  XenBlockVdev *vdev = &blockdev->props.vdev;
> >  
> > +    if (blockdev->props.vdev.type == XEN_BLOCK_VDEV_TYPE_INVALID) {
> > +    char name[11];
> > +    int disk = 0;
> > +    unsigned long idx;
> > +
> > +    /* Find an unoccupied device name */
> > +    while (disk < (1 << 20)) {
> 
> I like your optimism that we can handle a million disks. :-)

Heh, yeah. Given that there *is* a limit, setting it lower seemed a bit
arbitrary.

For consoles I picked 100 instead of letting it go all the way to
INT_MAX, and in a patch set soon to be posted I'll do the same for the
xen-net-device as I convert it.

Even with a limit of 100, having that many devices *WITHOUT SPECIFYING
WHICH ONE IS WHICH* seems a bit many!

FWIW I've changed it to check for the existence of the *frontend* nodes
(the ones which are visible to the guest). Currently it checks for the
backend nodes, but those might be in different places.

> I haven't reviewed the Xen part in detail, but the patch looks fine on
> the block layer side.
> 
> Acked-by: Kevin Wolf 

Thanks.


smime.p7s
Description: S/MIME cryptographic signature

Re: [QEMU PATCH v5 05/13] virtio-gpu: Configure context init for virglrenderer

[Dmitry Osipenko]

On 10/10/23 14:51, Daniel P. Berrangé wrote:
> On Tue, Oct 10, 2023 at 02:41:22PM +0300, Dmitry Osipenko wrote:
>> On 9/15/23 14:11, Huang Rui wrote:
>>> Configure context init feature flag for virglrenderer.
>>>
>>> Originally-by: Antonio Caggiano 
>>> Signed-off-by: Huang Rui 
>>> ---
>>>
>>> V4 -> V5:
>>> - Inverted patch 5 and 6 because we should configure
>>>   HAVE_VIRGL_CONTEXT_INIT firstly. (Philippe)
>>>
>>>  meson.build | 4 
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/meson.build b/meson.build
>>> index 98e68ef0b1..ff20d3c249 100644
>>> --- a/meson.build
>>> +++ b/meson.build
>>> @@ -1068,6 +1068,10 @@ if not get_option('virglrenderer').auto() or 
>>> have_system or have_vhost_user_gpu
>>> prefix: '#include 
>>> ',
>>> dependencies: virgl))
>>>endif
>>> +  config_host_data.set('HAVE_VIRGL_CONTEXT_INIT',
>>> +   
>>> cc.has_function('virgl_renderer_context_create_with_flags',
>>> +   prefix: '#include 
>>> ',
>>> +   dependencies: virgl))
>> The "cc.has_function" doesn't work properly with PKG_CONFIG_PATH. It ignores 
>> the the given pkg and uses system includes. Antonio was aware about that 
>> problem [1].
>>
>> [1] 
>> https://gitlab.freedesktop.org/Fahien/qemu/-/commit/ea1c252a707940983ccce71e92a292b49496bfcd
>>
>> Given that virglrenderer 1.0 has been released couple weeks ago,
>> can we make the v1.0 a mandatory requirement for qemu and remove
>> all the ifdefs? I doubt that anyone is going to test newer qemu
>> using older libviglrenderer, all that ifdef code will be bitrotting.
> 
> We cannot do that. If is is only weeks old, no distro will
> have virglrenderer 1.0 present. QEMU has a defined set of
> platforms that it targets compatibility with:
> 
>   https://www.qemu.org/docs/master/about/build-platforms.html
> 
> For newly added functionality we can set the min version to
> something newer than the oldest QEMU target platform.
> 
> For existing functionality though, we must not regress wrt
> the currently supported set of target platforms. So we can
> only bump the min version to that present in the oldest
> platform we target.

Well, then alternatively we could specify supported libvirglrender
features based on the lib version to avoid those pkgconfig+meson troubles.

-- 
Best regards,
Dmitry

Re: [XEN PATCH][for-4.19 v2 6/7] xen/console: make function static inline

[Nicola Vetrini]

On 17/10/2023 18:26, Jan Beulich wrote:

On 17.10.2023 17:24, Nicola Vetrini wrote:

On 16/10/2023 16:52, Jan Beulich wrote:

On 09.10.2023 08:54, Nicola Vetrini wrote:

--- a/xen/include/xen/consoled.h
+++ b/xen/include/xen/consoled.h
@@ -12,7 +12,7 @@ size_t consoled_guest_tx(char c);

 #else

-size_t consoled_guest_tx(char c) { return 0; }
+static inline size_t consoled_guest_tx(char c) { return 0; }


Why inline? We do so in headers, but we generally avoid "inline" in
.c files.


Yes. The file modified is in fact an header.


Hmm, how did I not pay attention? Yet then a different question arises:
Without the "static inline" I'd expect this to result in a build error
from any two .c files including this header. Yet we aren't aware of
such a build issue, so I wonder whether the stub is needed in the first
place.

Jan


This is a good observation. Right now I see only one caller, that is 
conditioned on
both CONFIG_X86 and pv_shim and pv_console, making this stub unused as 
far as I can tell.

It might indeed be a good idea to drop it.

--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

[PATCH -tip v3 1/3] x86/percpu: Correct PER_CPU_VAR usage to include symbol and its addend

[Uros Bizjak]

PER_CPU_VAR macro should be applied to a symbol and its addend.
Inconsistent usage is currently harmless, but needs to be corrected
before %rip-relative addressing is introduced to PER_CPU_VAR macro.

No functional changes intended.

Cc: Thomas Gleixner 
Cc: Ingo Molnar 
Cc: Borislav Petkov 
Cc: Dave Hansen 
Cc: "H. Peter Anvin" 
Signed-off-by: Uros Bizjak 
---
 arch/x86/entry/calling.h  | 2 +-
 arch/x86/entry/entry_32.S | 2 +-
 arch/x86/entry/entry_64.S | 2 +-
 arch/x86/kernel/head_64.S | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/entry/calling.h b/arch/x86/entry/calling.h
index f6907627172b..47368ab0bda0 100644
--- a/arch/x86/entry/calling.h
+++ b/arch/x86/entry/calling.h
@@ -173,7 +173,7 @@ For 32-bit we have the following conventions - kernel is 
built with
 .endm
 
 #define THIS_CPU_user_pcid_flush_mask   \
-   PER_CPU_VAR(cpu_tlbstate) + TLB_STATE_user_pcid_flush_mask
+   PER_CPU_VAR(cpu_tlbstate + TLB_STATE_user_pcid_flush_mask)
 
 .macro SWITCH_TO_USER_CR3_NOSTACK scratch_reg:req scratch_reg2:req
ALTERNATIVE "jmp .Lend_\@", "", X86_FEATURE_PTI
diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
index c73047bf9f4b..4e295798638b 100644
--- a/arch/x86/entry/entry_32.S
+++ b/arch/x86/entry/entry_32.S
@@ -305,7 +305,7 @@
 .macro CHECK_AND_APPLY_ESPFIX
 #ifdef CONFIG_X86_ESPFIX32
 #define GDT_ESPFIX_OFFSET (GDT_ENTRY_ESPFIX_SS * 8)
-#define GDT_ESPFIX_SS PER_CPU_VAR(gdt_page) + GDT_ESPFIX_OFFSET
+#define GDT_ESPFIX_SS PER_CPU_VAR(gdt_page + GDT_ESPFIX_OFFSET)
 
ALTERNATIVE "jmp .Lend_\@", "", X86_BUG_ESPFIX
 
diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index de6469dffe3a..1a88ad8a7b48 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -190,7 +190,7 @@ SYM_FUNC_START(__switch_to_asm)
 
 #ifdef CONFIG_STACKPROTECTOR
movqTASK_stack_canary(%rsi), %rbx
-   movq%rbx, PER_CPU_VAR(fixed_percpu_data) + FIXED_stack_canary
+   movq%rbx, PER_CPU_VAR(fixed_percpu_data + FIXED_stack_canary)
 #endif
 
/*
diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index 086a2c30..3dcabbc49149 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -448,7 +448,7 @@ SYM_CODE_START(soft_restart_cpu)
UNWIND_HINT_END_OF_STACK
 
/* Find the idle task stack */
-   movqPER_CPU_VAR(pcpu_hot) + X86_current_task, %rcx
+   movqPER_CPU_VAR(pcpu_hot + X86_current_task), %rcx
movqTASK_threadsp(%rcx), %rsp
 
jmp .Ljump_to_C_code
-- 
2.41.0

[PATCH -tip v3 3/3] x86/percpu: Introduce %rip-relative addressing to PER_CPU_VAR

[Uros Bizjak]

Introduce x86_64 %rip-relative addressing to PER_CPU_VAR macro.
Instructions using %rip-relative address operand are one byte shorter
than their absolute address counterparts and are also compatible with
position independent executable (-fpie) build. The patch reduces
code size of a test kernel build by 150 bytes.

PER_CPU_VAR macro is intended to be applied to a symbol and should
not be used with register operands. Introduce new __percpu macro and
use it in cmpxchg{8,16}b_emu.S instead.

Also add a missing function comment to this_cpu_cmpxchg8b_emu.

No functional changes intended.

Cc: Thomas Gleixner 
Cc: Ingo Molnar 
Cc: Borislav Petkov 
Cc: Dave Hansen 
Cc: "H. Peter Anvin" 
Cc: Peter Zijlstra 
Signed-off-by: Uros Bizjak 
--
v2: Introduce PER_CPU_ARG macro to conditionally enable
segment registers in cmpxchg{8,16}b_emu.S for CONFIG_SMP.
v3: Introduce __percpu macro instead of PER_CPU_ARG (hpa).
---
 arch/x86/include/asm/percpu.h | 12 
 arch/x86/lib/cmpxchg16b_emu.S | 12 ++--
 arch/x86/lib/cmpxchg8b_emu.S  | 30 +-
 3 files changed, 35 insertions(+), 19 deletions(-)

diff --git a/arch/x86/include/asm/percpu.h b/arch/x86/include/asm/percpu.h
index 54746903b8c3..02f1780f02f5 100644
--- a/arch/x86/include/asm/percpu.h
+++ b/arch/x86/include/asm/percpu.h
@@ -4,17 +4,21 @@
 
 #ifdef CONFIG_X86_64
 #define __percpu_seg   gs
+#define __percpu_rel   (%rip)
 #else
 #define __percpu_seg   fs
+#define __percpu_rel
 #endif
 
 #ifdef __ASSEMBLY__
 
 #ifdef CONFIG_SMP
-#define PER_CPU_VAR(var)   %__percpu_seg:var
-#else /* ! SMP */
-#define PER_CPU_VAR(var)   var
-#endif /* SMP */
+#define __percpu   %__percpu_seg:
+#else
+#define __percpu
+#endif
+
+#define PER_CPU_VAR(var)   __percpu(var)__percpu_rel
 
 #ifdef CONFIG_X86_64_SMP
 #define INIT_PER_CPU_VAR(var)  init_per_cpu__##var
diff --git a/arch/x86/lib/cmpxchg16b_emu.S b/arch/x86/lib/cmpxchg16b_emu.S
index 6962df315793..4fb44894ad87 100644
--- a/arch/x86/lib/cmpxchg16b_emu.S
+++ b/arch/x86/lib/cmpxchg16b_emu.S
@@ -23,14 +23,14 @@ SYM_FUNC_START(this_cpu_cmpxchg16b_emu)
cli
 
/* if (*ptr == old) */
-   cmpqPER_CPU_VAR(0(%rsi)), %rax
+   cmpq__percpu (%rsi), %rax
jne .Lnot_same
-   cmpqPER_CPU_VAR(8(%rsi)), %rdx
+   cmpq__percpu 8(%rsi), %rdx
jne .Lnot_same
 
/* *ptr = new */
-   movq%rbx, PER_CPU_VAR(0(%rsi))
-   movq%rcx, PER_CPU_VAR(8(%rsi))
+   movq%rbx, __percpu (%rsi)
+   movq%rcx, __percpu 8(%rsi)
 
/* set ZF in EFLAGS to indicate success */
orl $X86_EFLAGS_ZF, (%rsp)
@@ -42,8 +42,8 @@ SYM_FUNC_START(this_cpu_cmpxchg16b_emu)
/* *ptr != old */
 
/* old = *ptr */
-   movqPER_CPU_VAR(0(%rsi)), %rax
-   movqPER_CPU_VAR(8(%rsi)), %rdx
+   movq__percpu (%rsi), %rax
+   movq__percpu 8(%rsi), %rdx
 
/* clear ZF in EFLAGS to indicate failure */
andl$(~X86_EFLAGS_ZF), (%rsp)
diff --git a/arch/x86/lib/cmpxchg8b_emu.S b/arch/x86/lib/cmpxchg8b_emu.S
index 873e4ef23e49..1c96be769adc 100644
--- a/arch/x86/lib/cmpxchg8b_emu.S
+++ b/arch/x86/lib/cmpxchg8b_emu.S
@@ -24,12 +24,12 @@ SYM_FUNC_START(cmpxchg8b_emu)
pushfl
cli
 
-   cmpl0(%esi), %eax
+   cmpl(%esi), %eax
jne .Lnot_same
cmpl4(%esi), %edx
jne .Lnot_same
 
-   movl%ebx, 0(%esi)
+   movl%ebx, (%esi)
movl%ecx, 4(%esi)
 
orl $X86_EFLAGS_ZF, (%esp)
@@ -38,7 +38,7 @@ SYM_FUNC_START(cmpxchg8b_emu)
RET
 
 .Lnot_same:
-   movl0(%esi), %eax
+   movl(%esi), %eax
movl4(%esi), %edx
 
andl$(~X86_EFLAGS_ZF), (%esp)
@@ -53,18 +53,30 @@ EXPORT_SYMBOL(cmpxchg8b_emu)
 
 #ifndef CONFIG_UML
 
+/*
+ * Emulate 'cmpxchg8b %fs:(%rsi)'
+ *
+ * Inputs:
+ * %esi : memory location to compare
+ * %eax : low 32 bits of old value
+ * %edx : high 32 bits of old value
+ * %ebx : low 32 bits of new value
+ * %ecx : high 32 bits of new value
+ *
+ * Notably this is not LOCK prefixed and is not safe against NMIs
+ */
 SYM_FUNC_START(this_cpu_cmpxchg8b_emu)
 
pushfl
cli
 
-   cmplPER_CPU_VAR(0(%esi)), %eax
+   cmpl__percpu (%esi), %eax
jne .Lnot_same2
-   cmplPER_CPU_VAR(4(%esi)), %edx
+   cmpl__percpu 4(%esi), %edx
jne .Lnot_same2
 
-   movl%ebx, PER_CPU_VAR(0(%esi))
-   movl%ecx, PER_CPU_VAR(4(%esi))
+   movl%ebx, __percpu (%esi)
+   movl%ecx, __percpu 4(%esi)
 
orl $X86_EFLAGS_ZF, (%esp)
 
@@ -72,8 +84,8 @@ SYM_FUNC_START(this_cpu_cmpxchg8b_emu)
RET
 
 .Lnot_same2:
-   movlPER_CPU_VAR(0(%esi)), %eax
-   movlPER_CPU_VAR(4(%esi)), %edx
+   movl__percpu (%esi), %eax
+   movl__percpu 4(%esi), %edx
 
andl$(~X86_EFLAGS_ZF), (%esp)
 
-- 
2.41.0

[PATCH -tip v3 2/3] x86/percpu, xen: Correct PER_CPU_VAR usage to include symbol and its addend

[Uros Bizjak]

PER_CPU_VAR macro should be applied to a symbol and its addend.
Inconsistent usage is currently harmless, but needs to be corrected
before %rip-relative addressing is introduced to PER_CPU_VAR macro.

No functional changes intended.

Cc: Juergen Gross 
Cc: Thomas Gleixner 
Cc: Ingo Molnar 
Cc: Borislav Petkov 
Cc: Dave Hansen 
Cc: "H. Peter Anvin" 
Reviewed-by: Boris Ostrovsky 
Signed-off-by: Uros Bizjak 
---
 arch/x86/xen/xen-asm.S | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/xen/xen-asm.S b/arch/x86/xen/xen-asm.S
index 9e5e68008785..448958ddbaf8 100644
--- a/arch/x86/xen/xen-asm.S
+++ b/arch/x86/xen/xen-asm.S
@@ -28,7 +28,7 @@
  * non-zero.
  */
 SYM_FUNC_START(xen_irq_disable_direct)
-   movb $1, PER_CPU_VAR(xen_vcpu_info) + XEN_vcpu_info_mask
+   movb $1, PER_CPU_VAR(xen_vcpu_info + XEN_vcpu_info_mask)
RET
 SYM_FUNC_END(xen_irq_disable_direct)
 
@@ -69,7 +69,7 @@ SYM_FUNC_END(check_events)
 SYM_FUNC_START(xen_irq_enable_direct)
FRAME_BEGIN
/* Unmask events */
-   movb $0, PER_CPU_VAR(xen_vcpu_info) + XEN_vcpu_info_mask
+   movb $0, PER_CPU_VAR(xen_vcpu_info + XEN_vcpu_info_mask)
 
/*
 * Preempt here doesn't matter because that will deal with any
@@ -78,7 +78,7 @@ SYM_FUNC_START(xen_irq_enable_direct)
 */
 
/* Test for pending */
-   testb $0xff, PER_CPU_VAR(xen_vcpu_info) + XEN_vcpu_info_pending
+   testb $0xff, PER_CPU_VAR(xen_vcpu_info + XEN_vcpu_info_pending)
jz 1f
 
call check_events
@@ -97,7 +97,7 @@ SYM_FUNC_END(xen_irq_enable_direct)
  * x86 use opposite senses (mask vs enable).
  */
 SYM_FUNC_START(xen_save_fl_direct)
-   testb $0xff, PER_CPU_VAR(xen_vcpu_info) + XEN_vcpu_info_mask
+   testb $0xff, PER_CPU_VAR(xen_vcpu_info + XEN_vcpu_info_mask)
setz %ah
addb %ah, %ah
RET
@@ -113,7 +113,7 @@ SYM_FUNC_END(xen_read_cr2);
 
 SYM_FUNC_START(xen_read_cr2_direct)
FRAME_BEGIN
-   _ASM_MOV PER_CPU_VAR(xen_vcpu_info) + XEN_vcpu_info_arch_cr2, %_ASM_AX
+   _ASM_MOV PER_CPU_VAR(xen_vcpu_info + XEN_vcpu_info_arch_cr2), %_ASM_AX
FRAME_END
RET
 SYM_FUNC_END(xen_read_cr2_direct);
-- 
2.41.0

Re: [XEN PATCH][for-4.19 v2 6/7] xen/console: make function static inline

[Jan Beulich]

On 17.10.2023 17:24, Nicola Vetrini wrote:
> On 16/10/2023 16:52, Jan Beulich wrote:
>> On 09.10.2023 08:54, Nicola Vetrini wrote:
>>> --- a/xen/include/xen/consoled.h
>>> +++ b/xen/include/xen/consoled.h
>>> @@ -12,7 +12,7 @@ size_t consoled_guest_tx(char c);
>>>
>>>  #else
>>>
>>> -size_t consoled_guest_tx(char c) { return 0; }
>>> +static inline size_t consoled_guest_tx(char c) { return 0; }
>>
>> Why inline? We do so in headers, but we generally avoid "inline" in
>> .c files.
> 
> Yes. The file modified is in fact an header.

Hmm, how did I not pay attention? Yet then a different question arises:
Without the "static inline" I'd expect this to result in a build error
from any two .c files including this header. Yet we aren't aware of
such a build issue, so I wonder whether the stub is needed in the first
place.

Jan

Re: [XEN PATCH v2 3/7] x86: add deviation comments for asm-only functions

[Nicola Vetrini]

Now, concrete action items. Nicola, I think we should look into having 
a

larger-than-usual ECLAIR scan every week which includes all of Intel
files and in general as much as possible of the codebase.



This can be arranged. I'll keep you posted about this.

--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH][for-4.19 v2 6/7] xen/console: make function static inline

[Nicola Vetrini]

On 16/10/2023 16:52, Jan Beulich wrote:

On 09.10.2023 08:54, Nicola Vetrini wrote:

--- a/xen/include/xen/consoled.h
+++ b/xen/include/xen/consoled.h
@@ -12,7 +12,7 @@ size_t consoled_guest_tx(char c);

 #else

-size_t consoled_guest_tx(char c) { return 0; }
+static inline size_t consoled_guest_tx(char c) { return 0; }


Why inline? We do so in headers, but we generally avoid "inline" in
.c files.

Jan


Yes. The file modified is in fact an header.

--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [PATCH for-4.18] docs/sphinx: Lifecycle of a domid

[Alejandro Vallejo]

The page is pretty nice overall and I quite liked it. Just a few extra
questions below, as I'm not familiar with this side of things,

On Mon, Oct 16, 2023 at 05:24:50PM +0100, Andrew Cooper wrote:
> Signed-off-by: Andrew Cooper 
> ---
> CC: George Dunlap 
> CC: Jan Beulich 
> CC: Stefano Stabellini 
> CC: Wei Liu 
> CC: Julien Grall 
> CC: Roger Pau Monné 
> CC: Juergen Gross 
> CC: Henry Wang 
> 
> Rendered form:
>   
> https://andrewcoop-xen.readthedocs.io/en/docs-devel/hypervisor-guide/domid-lifecycle.html
> 
> I'm not sure why it's using the alibaster theme and not RTD theme, but I
> don't have time to debug that further at this point.
> 
> This was written mostly while sat waiting for flights in Nanjing and Beijing.
> 
> If while reading this you spot a hole, congratulations.  There are holes which
> need fixing...
> ---
>  docs/glossary.rst |   9 ++
>  docs/hypervisor-guide/domid-lifecycle.rst | 164 ++
>  docs/hypervisor-guide/index.rst   |   1 +
>  3 files changed, 174 insertions(+)
>  create mode 100644 docs/hypervisor-guide/domid-lifecycle.rst
> 
> diff --git a/docs/glossary.rst b/docs/glossary.rst
> index 8ddbdab160a1..1fd1de0f0e97 100644
> --- a/docs/glossary.rst
> +++ b/docs/glossary.rst
> @@ -50,3 +50,12 @@ Glossary
>  
>   By default it gets all devices, including all disks and network cards, 
> so
>   is responsible for multiplexing guest I/O.
> +
> +   system domain
> + Abstractions within Xen that are modelled in a similar way to regular
> + :term:`domains`.  E.g. When there's no work to do, Xen schedules
> + ``DOMID_IDLE`` to put the CPU into a lower power state.
> +
> + System domains have :term:`domids` and are referenced by
> + privileged software for certain control operations, but they do not run
> + guest code.
> diff --git a/docs/hypervisor-guide/domid-lifecycle.rst 
> b/docs/hypervisor-guide/domid-lifecycle.rst
> new file mode 100644
> index ..d405a321f3c7
> --- /dev/null
> +++ b/docs/hypervisor-guide/domid-lifecycle.rst
> @@ -0,0 +1,164 @@
> +.. SPDX-License-Identifier: CC-BY-4.0
> +
> +Lifecycle of a domid
> +
> +
> +Overview
> +
> +
> +A :term:`domid` is Xen's numeric identifier for a :term:`domain`.  In any
> +operational Xen system, there are one or more domains running.
> +
> +Domids are 16-bit integers.  Regular domids start from 0, but there are some
> +special identifiers, e.g. ``DOMID_SELF``, and :term:`system domains +domain>`, e.g. ``DOMID_IDLE`` starting from 0x7ff0.  Therefore, a Xen system
> +can run a maximum of 32k domains concurrently.
> +
> +.. note::
> +
> +   Despite being exposed in the domid ABI, the system domains are internal to
> +   Xen and do not have lifecycles like regular domains.  Therefore, they are
> +   not discussed further in this document.
> +
> +At system boot, Xen will construct one or more domains.  Kernels and
> +configuration for these domains must be provided by the bootloader, or at
> +Xen's compile time for more highly integrated solutions.
> +
> +Correct functioning of the domain lifecycle involves ``xenstored``, and some
> +privileged entity which has bound the ``VIRQ_DOM_EXC`` global event channel.
> +
> +.. note::
> +
> +   While not a strict requirement for these to be the same entity, it is
> +   ``xenstored`` which typically has ``VIRQ_DOM_EXC`` bound.  This document 
> is
> +   written assuming the common case.
> +
> +Creation
> +
> +
> +Within Xen, the ``domain_create()`` function is used to allocate and perform
> +bare minimum construction of a domain.  The :term:`control domain` accesses
> +this functionality via the ``DOMCTL_createdomain`` hypercall.
> +
> +The final action that ``domain_create()`` performs before returning
> +successfully is to enter the new domain into the domlist.  This makes the
> +domain "visible" within Xen, allowing the new domid to be successfully
> +referenced by other hypercalls.
> +
> +At this point, the domain exists as far as Xen is concerned, but not usefully
> +as a VM yet.  The toolstack performs further construction activities;
> +allocating vCPUs, RAM, copying in the initial executable code, etc.  Domains
> +are automatically created with one "pause" reference count held, meaning that
> +it is not eligible for scheduling.
> +
> +When the toolstack has finished VM construction, it send an ``XS_INTRODUCE``
> +command to ``xenstored``.  This instructs ``xenstored`` to connect to the
> +guest's xenstore ring, and fire the ``@introduceDomain`` watch.  The firing 
> of
> +this watch is the signal to all other components which care that a new VM has
> +appeared and is about to start running.
Presumably the xenstore ring is memory shared between xenstore and the
newly created domain. Who establishes that connection? For the case where
xenstore lives in dom0 things are _simpler_ because it lives in the same VM
as the toolstack, but I suspect things are hairier when xe

[libvirt test] 183395: tolerable all pass - PUSHED

[osstest service owner]

flight 183395 libvirt real [real]
http://logs.test-lab.xenproject.org/osstest/logs/183395/

Failures :-/ but no regressions.

Tests which did not succeed, but are not blocking:
 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check   fail like 183351
 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail  like 183351
 test-armhf-armhf-libvirt 16 saverestore-support-checkfail  like 183351
 test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt-xsm  15 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt  15 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt 15 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt 15 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt 16 saverestore-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-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail never pass
 test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-qcow2 14 migrate-support-checkfail never pass
 test-arm64-arm64-libvirt-qcow2 15 saverestore-support-checkfail never pass
 test-amd64-i386-libvirt-raw  14 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass
 test-armhf-armhf-libvirt-raw 14 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt 15 migrate-support-checkfail   never pass

version targeted for testing:
 libvirt  0c5f37364ffcbdd58ef34ec35d081843d2e144b4
baseline version:
 libvirt  8eb09a2bb984f3550cc87074e57841b86be39601

Last test of basis   183351  2023-10-12 04:18:45 Z5 days
Testing same since   183395  2023-10-17 04:20:33 Z0 days1 attempts


People who touched revisions under test:
  Michal Privoznik 

jobs:
 build-amd64-xsm  pass
 build-arm64-xsm  pass
 build-i386-xsm   pass
 build-amd64  pass
 build-arm64  pass
 build-armhf  pass
 build-i386   pass
 build-amd64-libvirt  pass
 build-arm64-libvirt  pass
 build-armhf-libvirt  pass
 build-i386-libvirt   pass
 build-amd64-pvopspass
 build-arm64-pvopspass
 build-armhf-pvopspass
 build-i386-pvops pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm   pass
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsmpass
 test-amd64-amd64-libvirt-xsm pass
 test-arm64-arm64-libvirt-xsm pass
 test-amd64-i386-libvirt-xsm  pass
 test-amd64-amd64-libvirt pass
 test-arm64-arm64-libvirt pass
 test-armhf-armhf-libvirt pass
 test-amd64-i386-libvirt  pass
 test-amd64-amd64-libvirt-pairpass
 test-amd64-i386-libvirt-pair pass
 test-arm64-arm64-libvirt-qcow2   pass
 test-armhf-armhf-libvirt-qcow2   pass
 test-arm64-arm64-libvirt-raw pass
 test-armhf-armhf-libvirt-raw pass
 test-amd64-i386-libvirt-raw  pass
 test-amd64-amd64-libvirt-vhd pass



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 a

Re: [for-4.19] xen/arm64: head: only use the macro load_paddr() in the MMU code

[Julien Grall]

Hi,

On 17/10/2023 15:07, Michal Orzel wrote:

On 17/10/2023 14:52, Julien Grall wrote:



The macro load_paddr() requires to know the offset between the
physical location of Xen and the virtual location.

When using the MPU, x20 will always be 0. Rather than wasting
a register for a compile-time constant value, it would be best if
we can avoid using load_paddr() altogher in the common head.S code.

s/altogher/altogether/


I will fix it.





The current use of load_paddr() are equivalent to adr_l() because
the MMU is off.

All the use of load_paddr() in arm64/head.S are now replaced with
adr_l(). With that, load_paddr() can now be moved in arm64/mmu/head.S.

For now, x20 is still unconditionally set. But this could change
in the future if needed.

Signed-off-by: Julien Grall 

Reviewed-by: Michal Orzel 

Side note:
Looking at all the uses of load_paddr(), none of them takes place after 
enabling MMU
which would indicate that we actually don't need this macro at all. Do you 
agree?


I agree they are none today called after enabling the MMU. But, 
create_table_entry() used to be called after and I can't rule out this 
will not happen again.


So I don't think we should replace the use in create_table_entry() with 
adr_l. We could consider to open-code code though.


Cheers,

--
Julien Grall

[linux-linus test] 183393: tolerable FAIL - PUSHED

[osstest service owner]

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

Failures :-/ but no regressions.

Tests which are failing intermittently (not blocking):
 test-amd64-amd64-libvirt-pair 22 guests-nbd-mirror/debian fail pass in 
183401-retest

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 183380
 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 183380
 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 183380
 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail  like 183380
 test-armhf-armhf-libvirt 16 saverestore-support-checkfail  like 183380
 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 183380
 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check   fail like 183380
 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 183380
 test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt 15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  15 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  16 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail never pass
 test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail  never pass
 test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail  never pass
 test-armhf-armhf-xl-arndale  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  16 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-qcow2 14 migrate-support-checkfail never pass
 test-amd64-amd64-libvirt-raw 14 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-vhd  14 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-vhd  15 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt-raw 14 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  14 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  15 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt 15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit1  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit1  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass

version targeted for testing:
 linux213f891525c222e8ed145ce1ce7ae1f47921cb9c
baseline version:
 linux58720809f52779dc0f08e53e54b014209d13eebb

Last test of basis   183380  2023-10-16 02:44:29 Z1 days
Testing same since   183393  2023-10-17 02:12:22 Z0 days1 attempts


People who touched revisions under test:
  Anshuman Khandual 
  Claudio Imbrenda 
  Dapeng Mi 
  Dave Hansen 
  Florent Revest 
  Ganapatrao Kulkarni 
  Jim Mattson 
  Joey Gouly 
  Like Xu 
  Linus Torvalds 
  Marc Zyngier 
  Masami Hiramatsu (Google) 
  Maxim Levitsky 
  Michael Mueller 
  Mingwei Zhang 
  Paolo Bonzini 
  Roman Kagan 
  Sean Christopherson 
  Suravee Suthikulpanit 
  Tom Lendacky 

jobs:
 build-amd64-xsm  pass
 build-arm64-xsm   

Re: [for-4.19] xen/arm64: head: only use the macro load_paddr() in the MMU code

[Michal Orzel]

Hi Julien,

On 17/10/2023 14:52, Julien Grall wrote:
> 
> 
> The macro load_paddr() requires to know the offset between the
> physical location of Xen and the virtual location.
> 
> When using the MPU, x20 will always be 0. Rather than wasting
> a register for a compile-time constant value, it would be best if
> we can avoid using load_paddr() altogher in the common head.S code.
s/altogher/altogether/

> 
> The current use of load_paddr() are equivalent to adr_l() because
> the MMU is off.
> 
> All the use of load_paddr() in arm64/head.S are now replaced with
> adr_l(). With that, load_paddr() can now be moved in arm64/mmu/head.S.
> 
> For now, x20 is still unconditionally set. But this could change
> in the future if needed.
> 
> Signed-off-by: Julien Grall 
Reviewed-by: Michal Orzel 

Side note:
Looking at all the uses of load_paddr(), none of them takes place after 
enabling MMU
which would indicate that we actually don't need this macro at all. Do you 
agree?

~Michal
 

Re: [XEN PATCH 01/10] arm/gic: address violations of MISRA C:2012 Rule 8.2

[Federico Serafini]

On 16/10/23 10:53, Julien Grall wrote:

Hi,

On 13/10/2023 16:24, Federico Serafini wrote:

Add missing parameter names, no functional change.

Signed-off-by: Federico Serafini 
---
  xen/arch/arm/include/asm/gic.h | 12 ++--
  1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/include/asm/gic.h 
b/xen/arch/arm/include/asm/gic.h

index f1ef347edc..03f209529b 100644
--- a/xen/arch/arm/include/asm/gic.h
+++ b/xen/arch/arm/include/asm/gic.h
@@ -246,7 +246,7 @@ void gic_set_irq_type(struct irq_desc *desc, 
unsigned int type);

  /* Program the GIC to route an interrupt */
  extern void gic_route_irq_to_xen(struct irq_desc *desc, unsigned int 
priority);

-extern int gic_route_irq_to_guest(struct domain *, unsigned int virq,
+extern int gic_route_irq_to_guest(struct domain *d, unsigned int virq,
    struct irq_desc *desc,
    unsigned int priority);
@@ -330,11 +330,11 @@ struct gic_hw_operations {
  /* Initialize the GIC and the boot CPU */
  int (*init)(void);
  /* Save GIC registers */
-    void (*save_state)(struct vcpu *);
+    void (*save_state)(struct vcpu *v);
  /* Restore GIC registers */
-    void (*restore_state)(const struct vcpu *);
+    void (*restore_state)(const struct vcpu *v);
  /* Dump GIC LR register information */
-    void (*dump_state)(const struct vcpu *);
+    void (*dump_state)(const struct vcpu *v);
  /* hw_irq_controller to enable/disable/eoi host irq */
  hw_irq_controller *gic_host_irq_type;
@@ -369,9 +369,9 @@ struct gic_hw_operations {
  /* Clear LR register */
  void (*clear_lr)(int lr);
  /* Read LR register and populate gic_lr structure */
-    void (*read_lr)(int lr, struct gic_lr *);
+    void (*read_lr)(int lr, struct gic_lr *lr_reg);
  /* Write LR register from gic_lr structure */
-    void (*write_lr)(int lr, const struct gic_lr *);
+    void (*write_lr)(int lr, const struct gic_lr *lr_reg);


Are the parameters name meant to match the declaration of the callbacks?
I am asking it because I see there are some inconsistencies between the 
declaration in GICv3 and GICv2. So this may want to be harmonized.


Cheers,



I can propose another patch that also changes parameter names
of gicv3_write_lr() as well as adding the missing ones in gic.h.

--
Federico Serafini, M.Sc.

Software Engineer, BUGSENG (http://bugseng.com)

Re: [PATCH] ui/input: Constify QemuInputHandler structure

[Marc-André Lureau]

On Tue, Oct 17, 2023 at 5:13 PM Philippe Mathieu-Daudé
 wrote:
>
> Access to QemuInputHandlerState::handler are read-only.
>
> Signed-off-by: Philippe Mathieu-Daudé 

Reviewed-by: Marc-André Lureau 

> ---
>  include/hw/virtio/virtio-input.h | 2 +-
>  include/ui/input.h   | 2 +-
>  chardev/msmouse.c| 2 +-
>  chardev/wctablet.c   | 2 +-
>  hw/char/escc.c   | 2 +-
>  hw/display/xenfb.c   | 6 +++---
>  hw/input/adb-kbd.c   | 2 +-
>  hw/input/hid.c   | 6 +++---
>  hw/input/ps2.c   | 4 ++--
>  hw/input/virtio-input-hid.c  | 8 
>  ui/input-legacy.c| 2 +-
>  ui/input.c   | 4 ++--
>  ui/vdagent.c | 2 +-
>  13 files changed, 22 insertions(+), 22 deletions(-)
>
> diff --git a/include/hw/virtio/virtio-input.h 
> b/include/hw/virtio/virtio-input.h
> index 08f1591424..a6c9703644 100644
> --- a/include/hw/virtio/virtio-input.h
> +++ b/include/hw/virtio/virtio-input.h
> @@ -84,7 +84,7 @@ struct VirtIOInputHID {
>  VirtIOInput   parent_obj;
>  char  *display;
>  uint32_t  head;
> -QemuInputHandler  *handler;
> +const QemuInputHandler*handler;
>  QemuInputHandlerState *hs;
>  int   ledstate;
>  bool  wheel_axis;
> diff --git a/include/ui/input.h b/include/ui/input.h
> index 24d8e4579e..8f9aac562e 100644
> --- a/include/ui/input.h
> +++ b/include/ui/input.h
> @@ -30,7 +30,7 @@ struct QemuInputHandler {
>  };
>
>  QemuInputHandlerState *qemu_input_handler_register(DeviceState *dev,
> -   QemuInputHandler 
> *handler);
> +const QemuInputHandler *handler);
>  void qemu_input_handler_activate(QemuInputHandlerState *s);
>  void qemu_input_handler_deactivate(QemuInputHandlerState *s);
>  void qemu_input_handler_unregister(QemuInputHandlerState *s);
> diff --git a/chardev/msmouse.c b/chardev/msmouse.c
> index ab8fe981d6..a774c397b4 100644
> --- a/chardev/msmouse.c
> +++ b/chardev/msmouse.c
> @@ -171,7 +171,7 @@ static int msmouse_chr_write(struct Chardev *s, const 
> uint8_t *buf, int len)
>  return len;
>  }
>
> -static QemuInputHandler msmouse_handler = {
> +static const QemuInputHandler msmouse_handler = {
>  .name  = "QEMU Microsoft Mouse",
>  .mask  = INPUT_EVENT_MASK_BTN | INPUT_EVENT_MASK_REL,
>  .event = msmouse_input_event,
> diff --git a/chardev/wctablet.c b/chardev/wctablet.c
> index 43bdf6b608..f4008bf35b 100644
> --- a/chardev/wctablet.c
> +++ b/chardev/wctablet.c
> @@ -178,7 +178,7 @@ static void wctablet_input_sync(DeviceState *dev)
>  }
>  }
>
> -static QemuInputHandler wctablet_handler = {
> +static const QemuInputHandler wctablet_handler = {
>  .name  = "QEMU Wacom Pen Tablet",
>  .mask  = INPUT_EVENT_MASK_BTN | INPUT_EVENT_MASK_ABS,
>  .event = wctablet_input_event,
> diff --git a/hw/char/escc.c b/hw/char/escc.c
> index 4be66053c1..48b30ee760 100644
> --- a/hw/char/escc.c
> +++ b/hw/char/escc.c
> @@ -845,7 +845,7 @@ static void sunkbd_handle_event(DeviceState *dev, 
> QemuConsole *src,
>  put_queue(s, keycode);
>  }
>
> -static QemuInputHandler sunkbd_handler = {
> +static const QemuInputHandler sunkbd_handler = {
>  .name  = "sun keyboard",
>  .mask  = INPUT_EVENT_MASK_KEY,
>  .event = sunkbd_handle_event,
> diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
> index 0074a9b6f8..b2130a0d70 100644
> --- a/hw/display/xenfb.c
> +++ b/hw/display/xenfb.c
> @@ -321,20 +321,20 @@ static void xenfb_mouse_sync(DeviceState *dev)
>  xenfb->wheel = 0;
>  }
>
> -static QemuInputHandler xenfb_keyboard = {
> +static const QemuInputHandler xenfb_keyboard = {
>  .name  = "Xen PV Keyboard",
>  .mask  = INPUT_EVENT_MASK_KEY,
>  .event = xenfb_key_event,
>  };
>
> -static QemuInputHandler xenfb_abs_mouse = {
> +static const QemuInputHandler xenfb_abs_mouse = {
>  .name  = "Xen PV Mouse",
>  .mask  = INPUT_EVENT_MASK_BTN | INPUT_EVENT_MASK_ABS,
>  .event = xenfb_mouse_event,
>  .sync  = xenfb_mouse_sync,
>  };
>
> -static QemuInputHandler xenfb_rel_mouse = {
> +static const QemuInputHandler xenfb_rel_mouse = {
>  .name  = "Xen PV Mouse",
>  .mask  = INPUT_EVENT_MASK_BTN | INPUT_EVENT_MASK_REL,
>  .event = xenfb_mouse_event,
> diff --git a/hw/input/adb-kbd.c b/hw/input/adb-kbd.c
> index a9088c910c..e21edf9acd 100644
> --- a/hw/input/adb-kbd.c
> +++ b/hw/input/adb-kbd.c
> @@ -355,7 +355,7 @@ static void adb_kbd_reset(DeviceState *dev)
>  s->count = 0;
>  }
>
> -static QemuInputHandler adb_keyboard_handler = {
> +static const QemuInputHandler adb_keyboard_handler = {
>  .name  = "QEMU ADB Keyboard",
>  .mask  = INPUT_EVENT_MASK_KEY,
>  .event = adb_keyboard_event,

Re: MISRA C:2012 D4.11 caution on staging

[Julien Grall]

Hi Jan,

On 17/10/2023 07:11, Jan Beulich wrote:

On 16.10.2023 20:06, Julien Grall wrote:

Instead, it would be best to find a way to help Eclair to detect this is
not an issue and also improve readability. Would the following help Eclair?

diff --git a/xen/common/domain.c b/xen/common/domain.c
index 30c227967345..ab16124eabd6 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -671,6 +671,8 @@ struct domain *domain_create(domid_t domid,

   if ( !is_idle_domain(d) )
   {
+ASSERT(config);
+
   watchdog_domain_init(d);
   init_status |= INIT_watchdog;


Just to mention it: Even if right now it turned out to help, it wouldn't
once release builds are also checked.


Indeed. I thought about it when writing the e-mail yesterday. I have the 
feeling that we are not getting many similar report today thanks to the 
various ASSERT().


This may mean that the ASSERT() will have to be kept during static 
analysis or we deviate/add proper error checking.


Cheers,

--
Julien Grall

[PATCH] ui/input: Constify QemuInputHandler structure

[Philippe Mathieu-Daudé]

Access to QemuInputHandlerState::handler are read-only.

Signed-off-by: Philippe Mathieu-Daudé 
---
 include/hw/virtio/virtio-input.h | 2 +-
 include/ui/input.h   | 2 +-
 chardev/msmouse.c| 2 +-
 chardev/wctablet.c   | 2 +-
 hw/char/escc.c   | 2 +-
 hw/display/xenfb.c   | 6 +++---
 hw/input/adb-kbd.c   | 2 +-
 hw/input/hid.c   | 6 +++---
 hw/input/ps2.c   | 4 ++--
 hw/input/virtio-input-hid.c  | 8 
 ui/input-legacy.c| 2 +-
 ui/input.c   | 4 ++--
 ui/vdagent.c | 2 +-
 13 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/include/hw/virtio/virtio-input.h b/include/hw/virtio/virtio-input.h
index 08f1591424..a6c9703644 100644
--- a/include/hw/virtio/virtio-input.h
+++ b/include/hw/virtio/virtio-input.h
@@ -84,7 +84,7 @@ struct VirtIOInputHID {
 VirtIOInput   parent_obj;
 char  *display;
 uint32_t  head;
-QemuInputHandler  *handler;
+const QemuInputHandler*handler;
 QemuInputHandlerState *hs;
 int   ledstate;
 bool  wheel_axis;
diff --git a/include/ui/input.h b/include/ui/input.h
index 24d8e4579e..8f9aac562e 100644
--- a/include/ui/input.h
+++ b/include/ui/input.h
@@ -30,7 +30,7 @@ struct QemuInputHandler {
 };
 
 QemuInputHandlerState *qemu_input_handler_register(DeviceState *dev,
-   QemuInputHandler *handler);
+const QemuInputHandler *handler);
 void qemu_input_handler_activate(QemuInputHandlerState *s);
 void qemu_input_handler_deactivate(QemuInputHandlerState *s);
 void qemu_input_handler_unregister(QemuInputHandlerState *s);
diff --git a/chardev/msmouse.c b/chardev/msmouse.c
index ab8fe981d6..a774c397b4 100644
--- a/chardev/msmouse.c
+++ b/chardev/msmouse.c
@@ -171,7 +171,7 @@ static int msmouse_chr_write(struct Chardev *s, const 
uint8_t *buf, int len)
 return len;
 }
 
-static QemuInputHandler msmouse_handler = {
+static const QemuInputHandler msmouse_handler = {
 .name  = "QEMU Microsoft Mouse",
 .mask  = INPUT_EVENT_MASK_BTN | INPUT_EVENT_MASK_REL,
 .event = msmouse_input_event,
diff --git a/chardev/wctablet.c b/chardev/wctablet.c
index 43bdf6b608..f4008bf35b 100644
--- a/chardev/wctablet.c
+++ b/chardev/wctablet.c
@@ -178,7 +178,7 @@ static void wctablet_input_sync(DeviceState *dev)
 }
 }
 
-static QemuInputHandler wctablet_handler = {
+static const QemuInputHandler wctablet_handler = {
 .name  = "QEMU Wacom Pen Tablet",
 .mask  = INPUT_EVENT_MASK_BTN | INPUT_EVENT_MASK_ABS,
 .event = wctablet_input_event,
diff --git a/hw/char/escc.c b/hw/char/escc.c
index 4be66053c1..48b30ee760 100644
--- a/hw/char/escc.c
+++ b/hw/char/escc.c
@@ -845,7 +845,7 @@ static void sunkbd_handle_event(DeviceState *dev, 
QemuConsole *src,
 put_queue(s, keycode);
 }
 
-static QemuInputHandler sunkbd_handler = {
+static const QemuInputHandler sunkbd_handler = {
 .name  = "sun keyboard",
 .mask  = INPUT_EVENT_MASK_KEY,
 .event = sunkbd_handle_event,
diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
index 0074a9b6f8..b2130a0d70 100644
--- a/hw/display/xenfb.c
+++ b/hw/display/xenfb.c
@@ -321,20 +321,20 @@ static void xenfb_mouse_sync(DeviceState *dev)
 xenfb->wheel = 0;
 }
 
-static QemuInputHandler xenfb_keyboard = {
+static const QemuInputHandler xenfb_keyboard = {
 .name  = "Xen PV Keyboard",
 .mask  = INPUT_EVENT_MASK_KEY,
 .event = xenfb_key_event,
 };
 
-static QemuInputHandler xenfb_abs_mouse = {
+static const QemuInputHandler xenfb_abs_mouse = {
 .name  = "Xen PV Mouse",
 .mask  = INPUT_EVENT_MASK_BTN | INPUT_EVENT_MASK_ABS,
 .event = xenfb_mouse_event,
 .sync  = xenfb_mouse_sync,
 };
 
-static QemuInputHandler xenfb_rel_mouse = {
+static const QemuInputHandler xenfb_rel_mouse = {
 .name  = "Xen PV Mouse",
 .mask  = INPUT_EVENT_MASK_BTN | INPUT_EVENT_MASK_REL,
 .event = xenfb_mouse_event,
diff --git a/hw/input/adb-kbd.c b/hw/input/adb-kbd.c
index a9088c910c..e21edf9acd 100644
--- a/hw/input/adb-kbd.c
+++ b/hw/input/adb-kbd.c
@@ -355,7 +355,7 @@ static void adb_kbd_reset(DeviceState *dev)
 s->count = 0;
 }
 
-static QemuInputHandler adb_keyboard_handler = {
+static const QemuInputHandler adb_keyboard_handler = {
 .name  = "QEMU ADB Keyboard",
 .mask  = INPUT_EVENT_MASK_KEY,
 .event = adb_keyboard_event,
diff --git a/hw/input/hid.c b/hw/input/hid.c
index a9c7dd1ce1..b8e85374ca 100644
--- a/hw/input/hid.c
+++ b/hw/input/hid.c
@@ -510,20 +510,20 @@ void hid_free(HIDState *hs)
 hid_del_idle_timer(hs);
 }
 
-static QemuInputHandler hid_keyboard_handler = {
+static const QemuInputHandler hid_keyboard_handler = {
 .name  = "QEMU HID Keyb

[PATCH for-4.18] iommu/vt-d: use max supported AGAW

[Roger Pau Monne]

SAGAW is a bitmap field, with bits 1 and 2 signaling support for AGAW 1 and
AGAW 2 respectively.  According to the Intel VT-d specification, an IOMMU might
support multiple AGAW values.

The AGAW value for each device is set in the device context entry, however
there's a caveat related to the value the field supports depending on the
translation type:

"When the Translation-type (T) field indicates pass-through (010b) or
guest-mode (100b or 101b), this field must be programmed to indicate the
largest AGAW value supported by hardware."

Of the translation types listed above Xen only uses pass-through (010b), and
hence we need to make sure the context entry AGAW field is set appropriately,
or else the IOMMU will report invalid context entry errors.

To do so calculate the IOMMU supported page table levels based on the last bit
set in the SAGAW field, instead of the first one.  This also allows making use
of the widest address width supported by the IOMMU, in case multiple AGAWs are
supported.

Note that 859d11b27912 claims to replace the open-coded find_first_set_bit(),
but it's actually replacing an open coded implementation to find the last set
bit.

Fixes: 859d11b27912 ('VT-d: prune SAGAW recognition')
Signed-off-by: Roger Pau Monné 
---
 xen/drivers/passthrough/vtd/iommu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/drivers/passthrough/vtd/iommu.c 
b/xen/drivers/passthrough/vtd/iommu.c
index ceef7359e553..be60d7573dae 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1328,7 +1328,7 @@ int __init iommu_alloc(struct acpi_drhd_unit *drhd)
 /* Calculate number of pagetable levels: 3 or 4. */
 sagaw = cap_sagaw(iommu->cap);
 if ( sagaw & 6 )
-agaw = find_first_set_bit(sagaw & 6);
+agaw = fls(sagaw & 6) - 1;
 if ( !agaw )
 {
 printk(XENLOG_ERR VTDPREFIX "IOMMU: unsupported sagaw %x\n", sagaw);
-- 
2.42.0

[for-4.19] xen/arm64: head: only use the macro load_paddr() in the MMU code

[Julien Grall]

The macro load_paddr() requires to know the offset between the
physical location of Xen and the virtual location.

When using the MPU, x20 will always be 0. Rather than wasting
a register for a compile-time constant value, it would be best if
we can avoid using load_paddr() altogher in the common head.S code.

The current use of load_paddr() are equivalent to adr_l() because
the MMU is off.

All the use of load_paddr() in arm64/head.S are now replaced with
adr_l(). With that, load_paddr() can now be moved in arm64/mmu/head.S.

For now, x20 is still unconditionally set. But this could change
in the future if needed.

Signed-off-by: Julien Grall 
---
 xen/arch/arm/arm64/head.S   | 4 ++--
 xen/arch/arm/arm64/mmu/head.S   | 6 ++
 xen/arch/arm/include/asm/arm64/macros.h | 6 --
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 4ad85dcf58d2..8dbd3300d89f 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -259,7 +259,7 @@ real_start_efi:
 
 /* Using the DTB in the .dtb section? */
 .ifnes CONFIG_DTB_FILE,""
-load_paddr x21, _sdtb
+adr_l x21, _sdtb
 .endif
 
 /* Initialize the UART if earlyprintk has been enabled. */
@@ -301,7 +301,7 @@ GLOBAL(init_secondary)
 bic   x24, x0, x13   /* Mask out flags to get CPU ID */
 
 /* Wait here until __cpu_up is ready to handle the CPU */
-load_paddr x0, smp_up_cpu
+adr_l x0, smp_up_cpu
 dsb   sy
 2:  ldr   x1, [x0]
 cmp   x1, x24
diff --git a/xen/arch/arm/arm64/mmu/head.S b/xen/arch/arm/arm64/mmu/head.S
index 88075ef08334..412b28e649a4 100644
--- a/xen/arch/arm/arm64/mmu/head.S
+++ b/xen/arch/arm/arm64/mmu/head.S
@@ -19,6 +19,12 @@
 #define XEN_FIRST_SLOT  first_table_offset(XEN_VIRT_START)
 #define XEN_SECOND_SLOT second_table_offset(XEN_VIRT_START)
 
+/* Load the physical address of a symbol into xb */
+.macro load_paddr xb, sym
+ldr \xb, =\sym
+add \xb, \xb, x20
+.endm
+
 /*
  * Flush local TLBs
  *
diff --git a/xen/arch/arm/include/asm/arm64/macros.h 
b/xen/arch/arm/include/asm/arm64/macros.h
index 99c401fcafa9..fb9a0602494d 100644
--- a/xen/arch/arm/include/asm/arm64/macros.h
+++ b/xen/arch/arm/include/asm/arm64/macros.h
@@ -62,12 +62,6 @@
 add  \dst, \dst, :lo12:\sym
 .endm
 
-/* Load the physical address of a symbol into xb */
-.macro load_paddr xb, sym
-ldr \xb, =\sym
-add \xb, \xb, x20
-.endm
-
 /*
  * Register aliases.
  */
-- 
2.40.1

[xen-unstable-smoke test] 183396: tolerable all pass - PUSHED

[osstest service owner]

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

Failures :-/ but no regressions.

Tests which did not succeed, but are not blocking:
 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
 test-armhf-armhf-xl  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  16 saverestore-support-checkfail   never pass

version targeted for testing:
 xen  7114bbfc8424ee467c6c8c82f077764ca4fa799b
baseline version:
 xen  0ce2ee7a16f2886c32b3f070bba3172f4a577aa4

Last test of basis   183389  2023-10-16 18:03:55 Z0 days
Testing same since   183396  2023-10-17 08:05:24 Z0 days1 attempts


People who touched revisions under test:
  Anthony PERARD 
  Federico Serafini 
  Jan Beulich 
  Tamas K Lengyel 

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



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

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

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

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


Pushing revision :

To xenbits.xen.org:/home/xen/git/xen.git
   0ce2ee7a16..7114bbfc84  7114bbfc8424ee467c6c8c82f077764ca4fa799b -> smoke

[xen-unstable test] 183392: tolerable FAIL - PUSHED

[osstest service owner]

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

Failures :-/ but no regressions.

Tests which are failing intermittently (not blocking):
 test-amd64-amd64-xl-qemuu-win7-amd64 12 windows-install fail pass in 
183398-retest

Tests which did not succeed, but are not blocking:
 test-amd64-i386-xl-qemuu-debianhvm-i386-xsm 12 debian-hvm-install fail blocked 
in 183383
 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stop  fail in 183398 like 183383
 test-armhf-armhf-libvirt 16 saverestore-support-checkfail  like 183383
 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 183383
 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 183383
 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 183383
 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail  like 183383
 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 183383
 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 183383
 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 183383
 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check   fail like 183383
 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 183383
 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 183383
 test-amd64-i386-xl-pvshim14 guest-start  fail   never pass
 test-amd64-i386-libvirt-xsm  15 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  16 saverestore-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  15 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  16 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail never pass
 test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail  never pass
 test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail  never pass
 test-armhf-armhf-xl-credit2  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt 15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  16 saverestore-support-checkfail   never pass
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check 
fail never pass
 test-amd64-amd64-libvirt 15 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt-raw  14 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-vhd  14 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-vhd  15 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt-raw 14 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  14 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  15 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-credit1  15 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit1  16 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass

version targeted for testing:
 xen  0ce2ee7a16f2886c32b3f070bba3172f4a577aa4
baseline version:
 xen  6

Re: [PATCH for-4.18] docs/sphinx: Lifecycle of a domid

[Juergen Gross]

On 17.10.23 12:09, Andrew Cooper wrote:

On 17/10/2023 6:24 am, Juergen Gross wrote:

On 16.10.23 18:24, Andrew Cooper wrote:

+command to ``xenstored``.  This instructs ``xenstored`` to connect to
the
+guest's xenstore ring, and fire the ``@introduceDomain`` watch.  The
firing of
+this watch is the signal to all other components which care that a
new VM has
+appeared and is about to start running.


A note should be added that the control domain is introduced implicitly by
xenstored, so no XS_INTRODUCE command is needed and no @introduceDomain
watch is being sent for the control domain.


How does this work for a stub xenstored?  It can't know that dom0 is
alive, and is the control domain, and mustn't assume that this is true.


A stub xenstored gets the control domain's domid via a boot parameter.


I admit that I've been a bit vague in the areas where I think there are
pre-existing bugs.  This is one area.

I'm planning a separate document on "how to connect to xenstore" seeing
as it is buggy in multiple ways in Linux (causing a deadlock on boot
with a stub xenstored), and made worse by dom0less creating memory
corruption from a 3rd entity into the xenstored<->kernel comms channel.

(And as I've said multiple times already, shuffling code in one of the
two xenstored's doesn't fix the root of the dom0less bug.  It simply
shuffles it around for someone else to trip over.)


All components interested in the @introduceDomain watch have to find out for
themselves which new domain has appeared, as the watch event doesn't contain
the domid of the new domain.


Yes, but we're intending to change that, and it is diverting focus from
the domain's lifecycle.

I suppose I could put in a footnote discussing the single-bit-ness of
the three signals.


Fine with me. I just wanted to mention this detail.




+ceased to exist.  It fires the ``@releaseDomain`` watch a second time to
+signal to any components which care that the domain has gone away.
+
+E.g. The second ``@releaseDomain`` is commonly used by paravirtual
driver
+backends to shut themselves down.


There is no guarantee that @releaseDomain will always be fired twice for a
domain ceasing to exist,


Are you sure?


Yes. Identical pending watch events are allowed to be merged into one.


Because the toolstack needs to listen to @releaseDomain in order to
start cleanup, there will be two distinct @releaseDomain's for an
individual domain.
> But an individual @releaseDomain can be relevant for a state change in
more than one domain, so there are not necessary 2*nr_doms worth of
@releaseDomain's fired.


Correct.




and multiple domains disappearing might result in
only one @releaseDomain watch being fired. This means that any component
receiving this watch event have not only to find out the domid(s) of the
domains changing state, but whether they have been shutting down only, or
are completely gone, too.


All entities holding a reference on the domain will block the second
notification until they have performed their own unmap action.


You are aware that backends normally don't register for @releaseDomain, but
set a watch on their backend specific Xenstore node in order to react on
the tool stack removing the backend device nodes?


But for entities which don't hold a reference on the domain, there is a
race condition where it's @releaseDomain notification is delivered
sufficiently late that the domid has already disappeared.


Exactly.


It's certainly good coding practice to cope with the domain disappearing
entirely underfoot, but entities without held references don't watch
@releaseDomain in the first place, so I don't think this case occurs in
practice.


I could easily see use cases where this assumptions isn't true, like a daemon
supervising domains in order to respawn them in case they have died.


Juergen


OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key


OpenPGP_signature.asc
Description: OpenPGP digital signature

Re: [PATCH 11/12] hw/xen: automatically assign device index to block devices

[Kevin Wolf]

Am 16.10.2023 um 17:19 hat David Woodhouse geschrieben:
> From: David Woodhouse 
> 
> There's no need to force the user to assign a vdev. We can automatically
> assign one, starting at xvda and searching until we find the first disk
> name that's unused.
> 
> This means we can now allow '-drive if=xen,file=xxx' to work without an
> explicit separate -driver argument, just like if=virtio.
> 
> Signed-off-by: David Woodhouse 

> @@ -34,6 +34,31 @@ static char *xen_block_get_name(XenDevice *xendev, Error 
> **errp)
>  XenBlockDevice *blockdev = XEN_BLOCK_DEVICE(xendev);
>  XenBlockVdev *vdev = &blockdev->props.vdev;
>  
> +if (blockdev->props.vdev.type == XEN_BLOCK_VDEV_TYPE_INVALID) {
> +char name[11];
> +int disk = 0;
> +unsigned long idx;
> +
> +/* Find an unoccupied device name */
> +while (disk < (1 << 20)) {

I like your optimism that we can handle a million disks. :-)

I haven't reviewed the Xen part in detail, but the patch looks fine on
the block layer side.

Acked-by: Kevin Wolf 

Re: [PATCH for-4.18] docs/sphinx: Lifecycle of a domid

[Jan Beulich]

On 17.10.2023 12:15, Andrew Cooper wrote:
> On 17/10/2023 7:34 am, Jan Beulich wrote:
>> On 16.10.2023 18:24, Andrew Cooper wrote:
>>> +Termination
>>> +---
>>> +
>>> +The VM runs for a period of time, but eventually stops.  It can stop for a
>>> +number of reasons, including:
>>> +
>>> + * Directly at the guest kernel's request, via the ``SCHEDOP_shutdown``
>>> +   hypercall.  The hypercall also includes the reason for the shutdown,
>>> +   e.g. ``poweroff``, ``reboot`` or ``crash``.
>>> +
>>> + * Indirectly from certain states.  E.g. executing a ``HLT`` instruction 
>>> with
>>> +   interrupts disabled is interpreted as a shutdown request as it is a 
>>> common
>>> +   code pattern for fatal error handling when no better options are 
>>> available.
>> HLT (note btw that this is x86 and HVM specific and hence may want mentioning
>> as such) is interpreted this way only if all other vCPU-s are also "down"
>> already.
>>
>>> + * Indirectly from fatal exceptions.  In some states, execution is unable 
>>> to
>>> +   continue, e.g. Triple Fault on x86.
>> Nit: This again is HVM specific.
> 
> Triple fault, maybe.  fatal exceptions terminating the VM, no.
> 
> For both, these details are not important for the document.  This is an
> list of examples, not an exhaustive list.

Of course. But I would recommend to avoid having examples which don't
describe technical details correctly.

>>> +Destruction
>>> +---
>>> +
>>> +The domain object in Xen is reference counted, and survives until all
>>> +references are dropped.
>>> +
>>> +The ``@releaseDomain`` watch is to inform all entities that hold a 
>>> reference
>>> +on the domain to clean up.  This may include:
>>> +
>>> + * Paravirtual driver backends having a grant map of the shared ring with 
>>> the
>>> +   frontend.
>> Beyond the shared ring(s), other (data) pages may also still have mappings.
> 
> Yes, but again, this is just a examples.  Other data pages should only
> be mapped while data is in flight.

Hmm, blkif's persistent grants are explicitly not like that. All I'm asking
for here is to insert another "e.g."

Jan

Re: [PATCH for-4.18] docs/sphinx: Lifecycle of a domid

[Andrew Cooper]

On 17/10/2023 7:34 am, Jan Beulich wrote:
> On 16.10.2023 18:24, Andrew Cooper wrote:
>> +Termination
>> +---
>> +
>> +The VM runs for a period of time, but eventually stops.  It can stop for a
>> +number of reasons, including:
>> +
>> + * Directly at the guest kernel's request, via the ``SCHEDOP_shutdown``
>> +   hypercall.  The hypercall also includes the reason for the shutdown,
>> +   e.g. ``poweroff``, ``reboot`` or ``crash``.
>> +
>> + * Indirectly from certain states.  E.g. executing a ``HLT`` instruction 
>> with
>> +   interrupts disabled is interpreted as a shutdown request as it is a 
>> common
>> +   code pattern for fatal error handling when no better options are 
>> available.
> HLT (note btw that this is x86 and HVM specific and hence may want mentioning
> as such) is interpreted this way only if all other vCPU-s are also "down"
> already.
>
>> + * Indirectly from fatal exceptions.  In some states, execution is unable to
>> +   continue, e.g. Triple Fault on x86.
> Nit: This again is HVM specific.

Triple fault, maybe.  fatal exceptions terminating the VM, no.

For both, these details are not important for the document.  This is an
list of examples, not an exhaustive list.
>> +Destruction
>> +---
>> +
>> +The domain object in Xen is reference counted, and survives until all
>> +references are dropped.
>> +
>> +The ``@releaseDomain`` watch is to inform all entities that hold a reference
>> +on the domain to clean up.  This may include:
>> +
>> + * Paravirtual driver backends having a grant map of the shared ring with 
>> the
>> +   frontend.
> Beyond the shared ring(s), other (data) pages may also still have mappings.

Yes, but again, this is just a examples.  Other data pages should only
be mapped while data is in flight.

~Andrew

[ovmf test] 183397: all pass - PUSHED

[osstest service owner]

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

Perfect :-)
All tests in this flight passed as required
version targeted for testing:
 ovmf 772ec92577a8c786b6c9f8643fa60f1cf893bcd9
baseline version:
 ovmf a445e1a42ccf3cb9f70537c7cd80ece689bf4d9a

Last test of basis   183394  2023-10-17 03:11:04 Z0 days
Testing same since   183397  2023-10-17 08:10:41 Z0 days1 attempts


People who touched revisions under test:
  Tuan Phan 

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
   a445e1a42c..772ec92577  772ec92577a8c786b6c9f8643fa60f1cf893bcd9 -> 
xen-tested-master

Re: [PATCH for-4.18] docs/sphinx: Lifecycle of a domid

[Andrew Cooper]

On 17/10/2023 6:24 am, Juergen Gross wrote:
> On 16.10.23 18:24, Andrew Cooper wrote:
>> +command to ``xenstored``.  This instructs ``xenstored`` to connect to
>> the
>> +guest's xenstore ring, and fire the ``@introduceDomain`` watch.  The
>> firing of
>> +this watch is the signal to all other components which care that a
>> new VM has
>> +appeared and is about to start running.
> 
> A note should be added that the control domain is introduced implicitly by
> xenstored, so no XS_INTRODUCE command is needed and no @introduceDomain
> watch is being sent for the control domain.

How does this work for a stub xenstored?  It can't know that dom0 is
alive, and is the control domain, and mustn't assume that this is true.

I admit that I've been a bit vague in the areas where I think there are
pre-existing bugs.  This is one area.

I'm planning a separate document on "how to connect to xenstore" seeing
as it is buggy in multiple ways in Linux (causing a deadlock on boot
with a stub xenstored), and made worse by dom0less creating memory
corruption from a 3rd entity into the xenstored<->kernel comms channel.

(And as I've said multiple times already, shuffling code in one of the
two xenstored's doesn't fix the root of the dom0less bug.  It simply
shuffles it around for someone else to trip over.)

> All components interested in the @introduceDomain watch have to find out for
> themselves which new domain has appeared, as the watch event doesn't contain
> the domid of the new domain.

Yes, but we're intending to change that, and it is diverting focus from
the domain's lifecycle.

I suppose I could put in a footnote discussing the single-bit-ness of
the three signals.

>> +ceased to exist.  It fires the ``@releaseDomain`` watch a second time to
>> +signal to any components which care that the domain has gone away.
>> +
>> +E.g. The second ``@releaseDomain`` is commonly used by paravirtual
>> driver
>> +backends to shut themselves down.
> 
> There is no guarantee that @releaseDomain will always be fired twice for a
> domain ceasing to exist,

Are you sure?

Because the toolstack needs to listen to @releaseDomain in order to
start cleanup, there will be two distinct @releaseDomain's for an
individual domain.

But an individual @releaseDomain can be relevant for a state change in
more than one domain, so there are not necessary 2*nr_doms worth of
@releaseDomain's fired.

> and multiple domains disappearing might result in
> only one @releaseDomain watch being fired. This means that any component
> receiving this watch event have not only to find out the domid(s) of the
> domains changing state, but whether they have been shutting down only, or
> are completely gone, too.

All entities holding a reference on the domain will block the second
notification until they have performed their own unmap action.

But for entities which don't hold a reference on the domain, there is a
race condition where it's @releaseDomain notification is delivered
sufficiently late that the domid has already disappeared.

It's certainly good coding practice to cope with the domain disappearing
entirely underfoot, but entities without held references don't watch
@releaseDomain in the first place, so I don't think this case occurs in
practice.

~Andrew

Re: [XEN PATCH][for-next v2 6/8] x86/mce: Move MC_NCLASSES into the enum mctelem_class

[Jan Beulich]

On 17.10.2023 11:43, Nicola Vetrini wrote:
> On 17/10/2023 10:26, Jan Beulich wrote:
>> On 17.10.2023 10:12, Nicola Vetrini wrote:
>>> On 17/10/2023 09:02, Jan Beulich wrote:
 On 16.10.2023 18:05, Nicola Vetrini wrote:
> On 16/10/2023 17:45, Jan Beulich wrote:
>> On 12.10.2023 17:28, Nicola Vetrini wrote:
>>> The definition of MC_NCLASSES contained a violation of MISRA 
>>> C:2012
>>> Rule 10.1, therefore by moving it as an enumeration constant
>>> resolves
>>> the
>>> violation and makes it more resilient to possible additions to 
>>> that
>>> enum.
>>
>> And using an enumerator as array dimension specifier is okay for
>> Misra?
>> That would be odd when elsewhere named enums are treated specially.
>
> Yes, the array subscript operator is one of the few places where an
> enum
> can be used as
> an operand (also because negative values wouldn't compile), as 
> opposed
> to mixing them
> with ordinary integers.

 When saying "odd" I didn't even think of negative values. May I
 therefore
 ask for the reasoning of why this specific case is deemed non-risky? 
 To
 me there looks to be a fair risk of creating undersized arrays, 
 leading
 to out-of-bounds accesses.
>>>
>>> Two reasons: MC_NCLASSES is the last value of the enum, and a pattern
>>> I've spot in various
>>> other places in Xen, so I assumed it was a fairly common pattern for 
>>> the
>>> community.
>>> The other reason is that since the value of an enum constant can be
>>> derived statically, there
>>> is little risk of it being the wrong value, because no arithmetic is
>>> done on it in its use
>>> as an array's size (and besides, the enum changed the last time ~15
>>> years ago, so I think
>>> it's unlikely to change much in the near future).
>>
>> You focus on the specific instance, yet my question was on the general
>> principle.
> 
> A couple of reasons why this is allowed:
> - associating values to set of symbols is typical and makes sense in 
> some contexts
> - out-of-bounds operations on arrays are dealt with by a host of other 
> guidelines
>(Series 18, mainly)
> - this rule is about which kinds of operands makes sense to use with 
> certain operators.
>It was deemed unlikely by MISRA that risky behaviour may arise by 
> using symbolic indices
>as subscripts, given the rest of the other guidelines and the 
> unspecified and undefined
>associated with Rule 10.1. It's not impossible that problems will 
> arise, but far less
>likely than using enums with no restrictions at all (such as those 
> caused by an enum of
>and implementation-defined type used in an arithmetic operation, that 
> could give
>unexpected results).

Now you appear to focus on uses of arrays, not their definition. Yet even
there I wonder: array[idx] is nothing else than *(array + idx). Adding
integer types and enums is disallowed. Nothing is said about arithmetic
on pointers throughout the description of the type model and its rules.
So despite the restriction on integer types, adding enums to pointers is
permitted?

Jan

Re: [XEN PATCH 10/10] arm/smmu: address violation of MISRA C:2012 Rule 8.2

[Julien Grall]

Hi Stefano,

On 16/10/2023 21:47, Stefano Stabellini wrote:

On Mon, 16 Oct 2023, Bertrand Marquis wrote:

On 16 Oct 2023, at 15:38, Julien Grall  wrote:



On 16/10/2023 14:31, Bertrand Marquis wrote:

Hi Julien,


Hi Bertrand,


On 16 Oct 2023, at 11:07, Julien Grall  wrote:

Hi,

On 13/10/2023 16:24, Federico Serafini wrote:

Add missing parameter names, no functional change.
Signed-off-by: Federico Serafini 
---
  xen/drivers/passthrough/arm/smmu.c | 6 +++---


This file is using the Linux coding style because it is imported from Linux. I 
was under the impression we would exclude such file for now.

Looking at exclude-list.json, it doesn't seem to be present. I think this patch 
should be replaced with adding a line in execlude-list.json.

I think that during one of the discussions we said that this file already 
deviated quite a lot from the status in Linux and we wanted to turn it to Xen 
coding style in the future hence it is not listed in the exclude file.

AFAIK the SMMUv{1, 2} code didn't deviate very much from Linux. I can't tell 
about the SMMUv3.


True that i had the v3 code in mind, we might not want to do that for v1/2 you 
are right.




At the end having a working smmu might be critical in a safety context so it 
will make sense to also check this part of xen.

I don't buy this argument right now. We have files in exclude-lists.json that I 
would consider critical for Xen to run (e.g. common/bitmap.c, common/libfdt). 
So if you want to use this argument then we need to move critical components of 
Xen out of the exclusion list.


I am sure we will at some point for runtime code but we cannot do everything on 
the first shot.
I was not meaning to do that now but that it is something to consider.


Things that are in exclude-lists.json are source files that come from
other projects and also change very rarely. The argument that we don't
do MISRA C on the files in exclude-lists.json, it is not because those
files are unimportant, but because they change only once every many
years.


Interesting. I would have thought this would be a condition to do MISRA 
as the cost to port a patch would increase a bit but this is one time 
cost every many years. Whereas code like the SMMU are still actively 
developped. And in particular for SMMUv2 we tried to stick close to 
Linux to help backport. So this would be a reason to initially exclude 
it from MISRA.




Of course the least we rely on exclude-lists.json the better.

For smmu.c, looking at the git history I think it is more actively
worked on than other files such as lib/rbtree.c or common/bitmap.c.
Given that backports from Linux to smmu.c are not straightforward anyway
(please correct me if I am wrong) then I think we should not add smmu.c
to exclude-lists.json and do MISRA for smmu.c.


I haven't done any recently. But if they are already not 
straightforward, then adding MISRA on top is not really to make it 
better. So I think if you want to do MISRA for the SMMU, then we need to 
fully convert it to Xen and abandon the idea to backport from Linux.


This would also make the code looks nicer as at the moment this contains 
wrapper just to stay as close as possible to Linux.


As a side note, the change here looks fairly self-contained. So I don't 
expect a major impact and therefore would not block this. This may not 
be the case for more complex one. Hence why I wanted to exclude it.


Do you expect larger MISRA changes in the SMMU driver?

Cheers,

--
Julien Grall

Re: [XEN PATCH][for-next v2 7/7] x86/mem_access: make function static

[Nicola Vetrini]

On 16/10/2023 16:29, Jan Beulich wrote:

On 09.10.2023 08:54, Nicola Vetrini wrote:

--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -249,7 +249,7 @@ bool p2m_mem_access_check(paddr_t gpa, unsigned 
long gla,

 return (p2ma != p2m_access_n2rwx);
 }

-int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain 
*hp2m,
+static int p2m_set_altp2m_mem_access(struct domain *d, struct 
p2m_domain *hp2m,
   struct p2m_domain *ap2m, p2m_access_t 
a,

   gfn_t gfn)


As mentioned before, when adding static (or whatever else) like this,
the next line(s) need re-indenting.

Jan


Ok.

--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH][for-next v2 6/8] x86/mce: Move MC_NCLASSES into the enum mctelem_class

[Nicola Vetrini]

On 17/10/2023 10:26, Jan Beulich wrote:

On 17.10.2023 10:12, Nicola Vetrini wrote:

On 17/10/2023 09:02, Jan Beulich wrote:

On 16.10.2023 18:05, Nicola Vetrini wrote:

On 16/10/2023 17:45, Jan Beulich wrote:

On 12.10.2023 17:28, Nicola Vetrini wrote:
The definition of MC_NCLASSES contained a violation of MISRA 
C:2012

Rule 10.1, therefore by moving it as an enumeration constant
resolves
the
violation and makes it more resilient to possible additions to 
that

enum.


And using an enumerator as array dimension specifier is okay for
Misra?
That would be odd when elsewhere named enums are treated specially.


Yes, the array subscript operator is one of the few places where an
enum
can be used as
an operand (also because negative values wouldn't compile), as 
opposed

to mixing them
with ordinary integers.


When saying "odd" I didn't even think of negative values. May I
therefore
ask for the reasoning of why this specific case is deemed non-risky? 
To
me there looks to be a fair risk of creating undersized arrays, 
leading

to out-of-bounds accesses.


Two reasons: MC_NCLASSES is the last value of the enum, and a pattern
I've spot in various
other places in Xen, so I assumed it was a fairly common pattern for 
the

community.
The other reason is that since the value of an enum constant can be
derived statically, there
is little risk of it being the wrong value, because no arithmetic is
done on it in its use
as an array's size (and besides, the enum changed the last time ~15
years ago, so I think
it's unlikely to change much in the near future).


You focus on the specific instance, yet my question was on the general
principle.

Jan


A couple of reasons why this is allowed:
- associating values to set of symbols is typical and makes sense in 
some contexts
- out-of-bounds operations on arrays are dealt with by a host of other 
guidelines

  (Series 18, mainly)
- this rule is about which kinds of operands makes sense to use with 
certain operators.
  It was deemed unlikely by MISRA that risky behaviour may arise by 
using symbolic indices
  as subscripts, given the rest of the other guidelines and the 
unspecified and undefined
  associated with Rule 10.1. It's not impossible that problems will 
arise, but far less
  likely than using enums with no restrictions at all (such as those 
caused by an enum of
  and implementation-defined type used in an arithmetic operation, that 
could give

  unexpected results).

--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [PATCH v3] x86/amd: Address AMD erratum #1485

[Roger Pau Monné]

On Tue, Oct 17, 2023 at 11:21:47AM +0200, Jan Beulich wrote:
> On 17.10.2023 11:13, Roger Pau Monné wrote:
> > On Tue, Oct 17, 2023 at 08:50:45AM +0100, Andrew Cooper wrote:
> >> On 17/10/2023 8:44 am, Jan Beulich wrote:
> >>> On 13.10.2023 17:38, Alejandro Vallejo wrote:
>  Fix adapted off Linux's mailing list:
>    
>  https://lore.kernel.org/lkml/d99589f4-bc5d-430b-87b2-72c20370c...@exactcode.com/T/#u
> >>> Why reference the bug report when there's a proper commit (f454b18e07f5) 
> >>> now?
> >>> Plus in any event a short summary of the erratum would help if put right 
> >>> here
> >>> (without needing to look up any documents or follow any links).
> >>
> >> That is not public information yet.  The erratum number alone is the
> >> best we can do at this juncture.
>  --- a/xen/arch/x86/cpu/amd.c
>  +++ b/xen/arch/x86/cpu/amd.c
>  @@ -1004,6 +1004,28 @@ static void cf_check zen2_disable_c6(void *arg)
>   wrmsrl(MSR_AMD_CSTATE_CFG, val & mask);
>   }
>   
>  +static void amd_check_erratum_1485(void)
>  +{
>  +uint64_t val, chickenbit = (1 << 5);
> >>> Linux gives the bit a name. Any reason you don't?
> >>
> >> There are multiple different names depending on where you look, and none
> >> are particularly relevant here.
> > 
> > Could we make chickenbit const static?
> > 
> > I would also use ULL just to be on the safe side, because we then copy
> > this for a different bit and it explodes.
> 
> I guess the way it is resembles what we already have in amd_check_zenbleed().
> Also it's not clear to me why besides "const" you also ask for "static".

Yes, makes no sense to put in .rodata, sorry, just const.

Roger.

Re: [PATCH v3] x86/amd: Address AMD erratum #1485

[Andrew Cooper]

On 17/10/2023 10:13 am, Roger Pau Monné wrote:
> On Tue, Oct 17, 2023 at 08:50:45AM +0100, Andrew Cooper wrote:
>> On 17/10/2023 8:44 am, Jan Beulich wrote:
>>> On 13.10.2023 17:38, Alejandro Vallejo wrote:
 Fix adapted off Linux's mailing list:
   
 https://lore.kernel.org/lkml/d99589f4-bc5d-430b-87b2-72c20370c...@exactcode.com/T/#u
>>> Why reference the bug report when there's a proper commit (f454b18e07f5) 
>>> now?
>>> Plus in any event a short summary of the erratum would help if put right 
>>> here
>>> (without needing to look up any documents or follow any links).
>> That is not public information yet.  The erratum number alone is the
>> best we can do at this juncture.
 --- a/xen/arch/x86/cpu/amd.c
 +++ b/xen/arch/x86/cpu/amd.c
 @@ -1004,6 +1004,28 @@ static void cf_check zen2_disable_c6(void *arg)
wrmsrl(MSR_AMD_CSTATE_CFG, val & mask);
  }
  
 +static void amd_check_erratum_1485(void)
 +{
 +  uint64_t val, chickenbit = (1 << 5);
>>> Linux gives the bit a name. Any reason you don't?
>> There are multiple different names depending on where you look, and none
>> are particularly relevant here.
> Could we make chickenbit const static?

Why would we want to force something that's optimised to an instruction
immediate into a .data variable?

~Andrew

Re: [PATCH v3] x86/amd: Address AMD erratum #1485

[Jan Beulich]

On 17.10.2023 11:13, Roger Pau Monné wrote:
> On Tue, Oct 17, 2023 at 08:50:45AM +0100, Andrew Cooper wrote:
>> On 17/10/2023 8:44 am, Jan Beulich wrote:
>>> On 13.10.2023 17:38, Alejandro Vallejo wrote:
 Fix adapted off Linux's mailing list:
   
 https://lore.kernel.org/lkml/d99589f4-bc5d-430b-87b2-72c20370c...@exactcode.com/T/#u
>>> Why reference the bug report when there's a proper commit (f454b18e07f5) 
>>> now?
>>> Plus in any event a short summary of the erratum would help if put right 
>>> here
>>> (without needing to look up any documents or follow any links).
>>
>> That is not public information yet.  The erratum number alone is the
>> best we can do at this juncture.
 --- a/xen/arch/x86/cpu/amd.c
 +++ b/xen/arch/x86/cpu/amd.c
 @@ -1004,6 +1004,28 @@ static void cf_check zen2_disable_c6(void *arg)
wrmsrl(MSR_AMD_CSTATE_CFG, val & mask);
  }
  
 +static void amd_check_erratum_1485(void)
 +{
 +  uint64_t val, chickenbit = (1 << 5);
>>> Linux gives the bit a name. Any reason you don't?
>>
>> There are multiple different names depending on where you look, and none
>> are particularly relevant here.
> 
> Could we make chickenbit const static?
> 
> I would also use ULL just to be on the safe side, because we then copy
> this for a different bit and it explodes.

I guess the way it is resembles what we already have in amd_check_zenbleed().
Also it's not clear to me why besides "const" you also ask for "static".

Jan

Re: [PATCH v3] x86/amd: Address AMD erratum #1485

[Roger Pau Monné]

On Tue, Oct 17, 2023 at 08:50:45AM +0100, Andrew Cooper wrote:
> On 17/10/2023 8:44 am, Jan Beulich wrote:
> > On 13.10.2023 17:38, Alejandro Vallejo wrote:
> >> Fix adapted off Linux's mailing list:
> >>   
> >> https://lore.kernel.org/lkml/d99589f4-bc5d-430b-87b2-72c20370c...@exactcode.com/T/#u
> > Why reference the bug report when there's a proper commit (f454b18e07f5) 
> > now?
> > Plus in any event a short summary of the erratum would help if put right 
> > here
> > (without needing to look up any documents or follow any links).
> 
> That is not public information yet.  The erratum number alone is the
> best we can do at this juncture.
> >> --- a/xen/arch/x86/cpu/amd.c
> >> +++ b/xen/arch/x86/cpu/amd.c
> >> @@ -1004,6 +1004,28 @@ static void cf_check zen2_disable_c6(void *arg)
> >>wrmsrl(MSR_AMD_CSTATE_CFG, val & mask);
> >>  }
> >>  
> >> +static void amd_check_erratum_1485(void)
> >> +{
> >> +  uint64_t val, chickenbit = (1 << 5);
> > Linux gives the bit a name. Any reason you don't?
> 
> There are multiple different names depending on where you look, and none
> are particularly relevant here.

Could we make chickenbit const static?

I would also use ULL just to be on the safe side, because we then copy
this for a different bit and it explodes.

(not strong requirements, but if a resend is needed it would be nice
to have).

Thanks, Roger.

Re: [XEN PATCH][for-4.19 v2 7/8] xen/types: address Rule 10.1 for DECLARE_BITMAP use

[Nicola Vetrini]

On 16/10/2023 17:49, Jan Beulich wrote:

On 12.10.2023 17:28, Nicola Vetrini wrote:

--- a/xen/include/xen/types.h
+++ b/xen/include/xen/types.h
@@ -22,6 +22,14 @@ typedef signed long ssize_t;

 typedef __PTRDIFF_TYPE__ ptrdiff_t;

+/*
+ * Users of this macro are expected to pass a positive value.


Is passing 0 going to cause any issues?



I don't think so, even if that wouldn't make much sense. Given that the 
usage of the
zero lenght array extension is documented, that shouldn't be a concern 
either.



+ * Eventually, this should become an unsigned quantity, but this
+ * requires fixing various uses of this macro and BITS_PER_LONG in 
signed
+ * contexts, such as type-safe 'min' macro uses, which give rise to 
build errors
+ * when the arguments have differing signedness, due to the build 
flags used.

+ */


I'm not convinced of the usefulness of this part of the comment.

Jan



Isn't it useful to record why it was left as-is, and what should be done 
about it?

If it's not, this can be dropped on commit, in my opinion.

--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

[PATCH for-4.18 v3] x86/pvh: fix identity mapping of low 1MB

[Roger Pau Monne]

The mapping of memory regions below the 1MB mark was all done by the PVH dom0
builder code, causing the region to be avoided by the arch specific IOMMU
hardware domain initialization code.  That lead to the IOMMU being enabled
without reserved regions in the low 1MB identity mapped in the p2m for PVH
hardware domains.  Firmware which happens to be missing RMRR/IVMD ranges
describing E820 reserved regions in the low 1MB would transiently trigger IOMMU
faults until the p2m is populated by the PVH dom0 builder:

AMD-Vi: IO_PAGE_FAULT: :00:13.1 d0 addr 000eb380 flags 0x20 RW
AMD-Vi: IO_PAGE_FAULT: :00:13.1 d0 addr 000eb340 flags 0
AMD-Vi: IO_PAGE_FAULT: :00:13.2 d0 addr 000ea1c0 flags 0
AMD-Vi: IO_PAGE_FAULT: :00:14.5 d0 addr 000eb480 flags 0x20 RW
AMD-Vi: IO_PAGE_FAULT: :00:12.0 d0 addr 000eb080 flags 0x20 RW
AMD-Vi: IO_PAGE_FAULT: :00:14.5 d0 addr 000eb400 flags 0
AMD-Vi: IO_PAGE_FAULT: :00:12.0 d0 addr 000eb040 flags 0

Those errors have been observed on the osstest pinot{0,1} boxes (AMD Fam15h
Opteron(tm) Processor 3350 HE).

Rely on the IOMMU arch init code to create any identity mappings for reserved
regions in the low 1MB range (like it already does for reserved regions
elsewhere), and leave the mapping of any holes to be performed by the dom0
builder code.

Fixes: 6b4f6a31ace1 ('x86/PVH: de-duplicate mappings for first Mb of Dom0 
memory')
Signed-off-by: Roger Pau Monné 
---
Changes since v2:
 - Leave the identity mapping of holes in the low 1MB.

Changes since v1:
 - Reword commit message.
---
 xen/arch/x86/hvm/dom0_build.c   | 6 +++---
 xen/drivers/passthrough/x86/iommu.c | 8 +---
 2 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
index bc0e290db612..b8c27c1b1646 100644
--- a/xen/arch/x86/hvm/dom0_build.c
+++ b/xen/arch/x86/hvm/dom0_build.c
@@ -449,7 +449,7 @@ static int __init pvh_populate_p2m(struct domain *d)
 }
 }
 
-/* Non-RAM regions of space below 1MB get identity mapped. */
+/* Identity map everything below 1MB that's not already mapped. */
 for ( i = rc = 0; i < MB1_PAGES; ++i )
 {
 p2m_type_t p2mt;
@@ -459,8 +459,8 @@ static int __init pvh_populate_p2m(struct domain *d)
 rc = set_mmio_p2m_entry(d, _gfn(i), _mfn(i), PAGE_ORDER_4K);
 else
 /*
- * If the p2m entry is already set it must belong to a RMRR and
- * already be identity mapped, or be a RAM region.
+ * If the p2m entry is already set it must belong to a RMRR/IVMD or
+ * reserved region and be identity mapped, or else be a RAM region.
  */
 ASSERT(p2mt == p2m_ram_rw || mfn_eq(mfn, _mfn(i)));
 put_gfn(d, i);
diff --git a/xen/drivers/passthrough/x86/iommu.c 
b/xen/drivers/passthrough/x86/iommu.c
index c85549ccad6e..857dccb6a465 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -400,13 +400,7 @@ void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
 max_pfn = (GB(4) >> PAGE_SHIFT) - 1;
 top = max(max_pdx, pfn_to_pdx(max_pfn) + 1);
 
-/*
- * First Mb will get mapped in one go by pvh_populate_p2m(). Avoid
- * setting up potentially conflicting mappings here.
- */
-start = paging_mode_translate(d) ? PFN_DOWN(MB(1)) : 0;
-
-for ( i = pfn_to_pdx(start), count = 0; i < top; )
+for ( i = 0, start = 0, count = 0; i < top; )
 {
 unsigned long pfn = pdx_to_pfn(i);
 unsigned int perms = hwdom_iommu_map(d, pfn, max_pfn);
-- 
2.42.0

Re: [PATCH for-4.18 v2] x86/pvh: fix identity mapping of low 1MB

[Roger Pau Monné]

On Mon, Oct 16, 2023 at 04:55:30PM +0200, Jan Beulich wrote:
> On 16.10.2023 16:51, Roger Pau Monné wrote:
> > On Mon, Oct 16, 2023 at 04:07:22PM +0200, Jan Beulich wrote:
> >> On 16.10.2023 15:51, Roger Pau Monné wrote:
> >>> On Mon, Oct 16, 2023 at 03:32:54PM +0200, Jan Beulich wrote:
>  On 13.10.2023 10:56, Roger Pau Monne wrote:
> > The mapping of memory regions below the 1MB mark was all done by the 
> > PVH dom0
> > builder code, causing the region to be avoided by the arch specific 
> > IOMMU
> > hardware domain initialization code.  That lead to the IOMMU being 
> > enabled
> > without reserved regions in the low 1MB identity mapped in the p2m for 
> > PVH
> > hardware domains.  Firmware which happens to be missing RMRR/IVMD ranges
> > describing E820 reserved regions in the low 1MB would transiently 
> > trigger IOMMU
> > faults until the p2m is populated by the PVH dom0 builder:
> >
> > AMD-Vi: IO_PAGE_FAULT: :00:13.1 d0 addr 000eb380 flags 0x20 
> > RW
> > AMD-Vi: IO_PAGE_FAULT: :00:13.1 d0 addr 000eb340 flags 0
> > AMD-Vi: IO_PAGE_FAULT: :00:13.2 d0 addr 000ea1c0 flags 0
> > AMD-Vi: IO_PAGE_FAULT: :00:14.5 d0 addr 000eb480 flags 0x20 
> > RW
> > AMD-Vi: IO_PAGE_FAULT: :00:12.0 d0 addr 000eb080 flags 0x20 
> > RW
> > AMD-Vi: IO_PAGE_FAULT: :00:14.5 d0 addr 000eb400 flags 0
> > AMD-Vi: IO_PAGE_FAULT: :00:12.0 d0 addr 000eb040 flags 0
> >
> > Those errors have been observed on the osstest pinot{0,1} boxes (AMD 
> > Fam15h
> > Opteron(tm) Processor 3350 HE).
> >
> > Mostly remove the special handling of the low 1MB done by the PVH dom0 
> > builder,
> > leaving just the data copy between RAM regions.  Otherwise rely on the 
> > IOMMU
> > arch init code to create any identity mappings for reserved regions in 
> > that
> > range (like it already does for reserved regions elsewhere).
> >
> > Note there's a small difference in behavior, as holes in the low 1MB 
> > will no
> > longer be identity mapped to the p2m.
> 
>  I certainly like the simplification, but I'm concerned by this: The BDA
>  is not normally reserved, yet may want accessing by Dom0 (to see the real
>  machine contents). We do access that first page of memory ourselves, so
>  I expect OSes may do so as well (even if the specific aspect I'm thinking
>  of - the warm/cold reboot field - is under Xen's control).
> >>>
> >>> The BDA on the systems I've checked falls into a RAM area on the
> >>> memory map, but if you think it can be problematic I could arrange for
> >>> arch_iommu_hwdom_init() to also identity map holes in the low 1MB.
> >>
> >> Hmm, this again is a case where I'd wish CPU and IOMMU mappings could
> >> be different. I don't see reasons to try I/O to such holes, but I can
> >> see reasons for CPU accesses (of more or less probing kind).
> > 
> > Hm, while I agree devices have likely no reason to access holes (there
> > or elsewhere) I don't see much benefit of having this differentiation,
> > it's easier to just map everything for accesses from both device and
> > CPU rather than us having to decide (and maybe get wrong) whether
> > ranges should only be accessed by the CPU.
> 
> I understand that, and I also follow Andrew's arguments towards not
> making such a distinction. The consequence though is that we need
> to map more than possibly necessary, and never too little.
> 
> >>> Keep in mind this is only for PVH, it won't affect PV.
> >>
> >> Of course.
> > 
> > Would you be willing to Ack it?
> 
> If "it" is the present version, then me doing so would be stretch.
> How averse are you to re-adding the hole mappings?

Given the point we are regarding the release I guess it's safer to
leave the mapping of the holes in the low 1MB as-is, and consider
removing it for 4.19?  That would give us a full release cycle to
check whether it causes issues on systems.

I will send the updated patch.

Thanks, Roger.

Re: [XEN PATCH][for-next v2 6/8] x86/mce: Move MC_NCLASSES into the enum mctelem_class

[Jan Beulich]

On 17.10.2023 10:12, Nicola Vetrini wrote:
> On 17/10/2023 09:02, Jan Beulich wrote:
>> On 16.10.2023 18:05, Nicola Vetrini wrote:
>>> On 16/10/2023 17:45, Jan Beulich wrote:
 On 12.10.2023 17:28, Nicola Vetrini wrote:
> The definition of MC_NCLASSES contained a violation of MISRA C:2012
> Rule 10.1, therefore by moving it as an enumeration constant 
> resolves
> the
> violation and makes it more resilient to possible additions to that
> enum.

 And using an enumerator as array dimension specifier is okay for 
 Misra?
 That would be odd when elsewhere named enums are treated specially.
>>>
>>> Yes, the array subscript operator is one of the few places where an 
>>> enum
>>> can be used as
>>> an operand (also because negative values wouldn't compile), as opposed
>>> to mixing them
>>> with ordinary integers.
>>
>> When saying "odd" I didn't even think of negative values. May I 
>> therefore
>> ask for the reasoning of why this specific case is deemed non-risky? To
>> me there looks to be a fair risk of creating undersized arrays, leading
>> to out-of-bounds accesses.
> 
> Two reasons: MC_NCLASSES is the last value of the enum, and a pattern 
> I've spot in various
> other places in Xen, so I assumed it was a fairly common pattern for the 
> community.
> The other reason is that since the value of an enum constant can be 
> derived statically, there
> is little risk of it being the wrong value, because no arithmetic is 
> done on it in its use
> as an array's size (and besides, the enum changed the last time ~15 
> years ago, so I think
> it's unlikely to change much in the near future).

You focus on the specific instance, yet my question was on the general
principle.

Jan

Re: [XEN PATCH][for-4.19 v2 1/1] xen: introduce a deviation for Rule 11.9

[Nicola Vetrini]

On 17/10/2023 08:51, Jan Beulich wrote:

On 16.10.2023 18:49, Nicola Vetrini wrote:

On 16/10/2023 15:43, Jan Beulich wrote:

On 11.10.2023 14:46, Nicola Vetrini wrote:

--- a/xen/include/xen/compiler.h
+++ b/xen/include/xen/compiler.h
@@ -109,13 +109,16 @@

 #define offsetof(a,b) __builtin_offsetof(a,b)

+/* Access the field of structure type, without defining a local
variable */
+#define access_field(type, member) (((type *)NULL)->member)


This is not a field access, so I consider the macro misnamed. 
Question

is
whether such a helper macro is needed in the first place.

+#define typeof_field(type, member) typeof(access_field(type, 
member))


If this needs adding, it wants to come ...


 /**
  * sizeof_field(TYPE, MEMBER)
  *
  * @TYPE: The structure containing the field of interest
  * @MEMBER: The field to return the size of
  */
-#define sizeof_field(TYPE, MEMBER) sizeofTYPE *)0)->MEMBER))
+#define sizeof_field(TYPE, MEMBER) sizeof(access_field(TYPE, 
MEMBER))


... with a commend similar as this one has. (Or the commend could be
slightly altered to cover both).



I added access_field since it's possibly useful on its own in the
future,
but that may not be the case. Not a real field access, perhaps a
fake_access_field?


I don't like this either, I'm afraid: This isn't a fake access. Maybe
field_of() or field_of_type()? Yet at this point I'd still prefer for
this to not be a separate macro in the first place.

Jan


Ok, no problem.

--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [XEN PATCH][for-next v2 6/8] x86/mce: Move MC_NCLASSES into the enum mctelem_class

[Nicola Vetrini]

On 17/10/2023 09:02, Jan Beulich wrote:

On 16.10.2023 18:05, Nicola Vetrini wrote:

On 16/10/2023 17:45, Jan Beulich wrote:

On 12.10.2023 17:28, Nicola Vetrini wrote:

The definition of MC_NCLASSES contained a violation of MISRA C:2012
Rule 10.1, therefore by moving it as an enumeration constant 
resolves

the
violation and makes it more resilient to possible additions to that
enum.


And using an enumerator as array dimension specifier is okay for 
Misra?

That would be odd when elsewhere named enums are treated specially.


Yes, the array subscript operator is one of the few places where an 
enum

can be used as
an operand (also because negative values wouldn't compile), as opposed
to mixing them
with ordinary integers.


When saying "odd" I didn't even think of negative values. May I 
therefore

ask for the reasoning of why this specific case is deemed non-risky? To
me there looks to be a fair risk of creating undersized arrays, leading
to out-of-bounds accesses.

Jan


Two reasons: MC_NCLASSES is the last value of the enum, and a pattern 
I've spot in various
other places in Xen, so I assumed it was a fairly common pattern for the 
community.
The other reason is that since the value of an enum constant can be 
derived statically, there
is little risk of it being the wrong value, because no arithmetic is 
done on it in its use
as an array's size (and besides, the enum changed the last time ~15 
years ago, so I think

it's unlikely to change much in the near future).

--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [PATCH v8 1/4] x86/microcode: WARN->INFO for the "no ucode loading" log message

[Jan Beulich]

On 30.08.2023 17:53, Alejandro Vallejo wrote:
> Currently there's a printk statement triggered when no ucode loading
> facilities are discovered. This statement should have severity INFO rather
> than WARNING because it's not reporting anything wrong. Warnings ought
> to be reserved for recoverable system errors.
> 
> Signed-off-by: Alejandro Vallejo 

Acked-by: Jan Beulich 

Re: [PATCH v3] x86/amd: Address AMD erratum #1485

[Andrew Cooper]

On 17/10/2023 8:44 am, Jan Beulich wrote:
> On 13.10.2023 17:38, Alejandro Vallejo wrote:
>> Fix adapted off Linux's mailing list:
>>   
>> https://lore.kernel.org/lkml/d99589f4-bc5d-430b-87b2-72c20370c...@exactcode.com/T/#u
> Why reference the bug report when there's a proper commit (f454b18e07f5) now?
> Plus in any event a short summary of the erratum would help if put right here
> (without needing to look up any documents or follow any links).

That is not public information yet.  The erratum number alone is the
best we can do at this juncture.
>> --- a/xen/arch/x86/cpu/amd.c
>> +++ b/xen/arch/x86/cpu/amd.c
>> @@ -1004,6 +1004,28 @@ static void cf_check zen2_disable_c6(void *arg)
>>  wrmsrl(MSR_AMD_CSTATE_CFG, val & mask);
>>  }
>>  
>> +static void amd_check_erratum_1485(void)
>> +{
>> +uint64_t val, chickenbit = (1 << 5);
> Linux gives the bit a name. Any reason you don't?

There are multiple different names depending on where you look, and none
are particularly relevant here.

~Andrew

Re: [PATCH v3] x86/amd: Address AMD erratum #1485

[Jan Beulich]

On 13.10.2023 17:38, Alejandro Vallejo wrote:
> Fix adapted off Linux's mailing list:
>   
> https://lore.kernel.org/lkml/d99589f4-bc5d-430b-87b2-72c20370c...@exactcode.com/T/#u

Why reference the bug report when there's a proper commit (f454b18e07f5) now?
Plus in any event a short summary of the erratum would help if put right here
(without needing to look up any documents or follow any links).

> --- a/xen/arch/x86/cpu/amd.c
> +++ b/xen/arch/x86/cpu/amd.c
> @@ -1004,6 +1004,28 @@ static void cf_check zen2_disable_c6(void *arg)
>   wrmsrl(MSR_AMD_CSTATE_CFG, val & mask);
>  }
>  
> +static void amd_check_erratum_1485(void)
> +{
> + uint64_t val, chickenbit = (1 << 5);

Linux gives the bit a name. Any reason you don't?

Everything else lgtm.

Jan

Re: [XEN PATCH][for-next v2 6/8] x86/mce: Move MC_NCLASSES into the enum mctelem_class

[Jan Beulich]

On 16.10.2023 18:05, Nicola Vetrini wrote:
> On 16/10/2023 17:45, Jan Beulich wrote:
>> On 12.10.2023 17:28, Nicola Vetrini wrote:
>>> The definition of MC_NCLASSES contained a violation of MISRA C:2012
>>> Rule 10.1, therefore by moving it as an enumeration constant resolves 
>>> the
>>> violation and makes it more resilient to possible additions to that 
>>> enum.
>>
>> And using an enumerator as array dimension specifier is okay for Misra?
>> That would be odd when elsewhere named enums are treated specially.
> 
> Yes, the array subscript operator is one of the few places where an enum 
> can be used as
> an operand (also because negative values wouldn't compile), as opposed 
> to mixing them
> with ordinary integers.

When saying "odd" I didn't even think of negative values. May I therefore
ask for the reasoning of why this specific case is deemed non-risky? To
me there looks to be a fair risk of creating undersized arrays, leading
to out-of-bounds accesses.

Jan