Re: [PATCH for-4.19] xen/arm: static-shmem: request host address to be specified for 1:1 domains
On Fri, 2024-06-21 at 11:22 +0200, Michal Orzel wrote:
> As a follow up to commit cb1ddafdc573 ("xen/arm/static-shmem: Static-
> shmem
> should be direct-mapped for direct-mapped domains") add a check to
> request that both host and guest physical address must be supplied
> for
> direct mapped domains. Otherwise return an error to prevent unwanted
> behavior.
>
> Signed-off-by: Michal Orzel
> ---
> Reasoning for 4.19:
> this is hardening the code to prevent a feature misuse and unwanted
> behavior.
> ---
Release-Acked-By: Oleksii Kurochko
~ Oleksii
> xen/arch/arm/static-shmem.c | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-
> shmem.c
> index cd48d2896b7e..aa80756c3ca5 100644
> --- a/xen/arch/arm/static-shmem.c
> +++ b/xen/arch/arm/static-shmem.c
> @@ -378,6 +378,13 @@ int __init process_shm(struct domain *d, struct
> kernel_info *kinfo,
> const struct membank *alloc_bank =
> find_shm_bank_by_id(get_shmem_heap_banks(), shm_id);
>
> + if ( is_domain_direct_mapped(d) )
> + {
> + printk("%pd: host and guest physical address must be
> supplied for direct-mapped domains\n",
> + d);
> + return -EINVAL;
> + }
> +
> /* guest phys address is right at the beginning */
> gbase = dt_read_paddr(cells, addr_cells);
>
Re: [PATCH for-4.19?] libelf: avoid UB in elf_xen_feature_{get,set}()
On Thu, 2024-06-20 at 17:07 +0100, Andrew Cooper wrote: > On 20/06/2024 4:34 pm, Jan Beulich wrote: > > When the left shift amount is up to 31, the shifted quantity wants > > to be > > of unsigned int (or wider) type. > > > > While there also adjust types: get doesn't alter the array and > > returns a > > boolean, while both don't really accept negative "nr". Drop a stray > > blank each as well. > > > > Signed-off-by: Jan Beulich > > Reviewed-by: Andrew Cooper > > +1 for 4.19. Release-Acked-By: Oleksii Kurochko ~ Oleksii > > > --- > > Really I wonder why these exist at all; they're effectively > > test_bit() > > and __set_bit() in hypervisor terms, and iirc something like that > > exists > > in the tool stack as well. > > The toolstack has tools/libs/ctrl/xc_bitops.h but they're not API > compatible with Xen. > > They're long-granular rather than int-granular, have swapped > arguments, > and are non-LOCKed. > > ~Andrew
Re: [XEN for-4.19 PATCH] x86/apic: Fix signing in left bitshift
On Thu, 2024-06-20 at 16:16 +0100, Andrew Cooper wrote: > On 20/06/2024 3:31 pm, Matthew Barnes wrote: > > There exists a bitshift in the IOAPIC code where a signed integer > > is > > shifted to the left by at most 31 bits. This is undefined > > behaviour, > > and can cause faults in xtf tests such as pv64-pv-iopl~hypercall. > > > > This patch fixes this by changing the integer from signed to > > unsigned. > > > > Signed-off-by: Matthew Barnes > > The code change itself is fine, but I'm going to recommend some > adjustments to the commit message. > > Its "x86/ioapic"; apic implies the Local APIC which is apic.c and > distinct from the IO-APIC. The subject would be clearer as "Fix > signed > shift in end_level_ioapic_irq_new()". > > The XTF test has nothing to do with this, so shouldn't be mentioned > like > this. The UBSAN failure was in an interrupt handler, and it was > complete chance that it triggered while pv64-pv-iopl~hypercall was > the > test being ran. > > I'm happy to fix all of that up on commit. > > CC Oleksii for 4.19. This is low risk, and found during testing with > UBSAN active. Release-Acked-By: Oleksii Kurochko ~ Oleksii
Re: [PATCH for-4.19?] tools/libs/light: Fix nic->vlan memory allocation
On Wed, 2024-06-19 at 13:57 +0200, Jan Beulich wrote:
> On 20.05.2024 19:08, Jason Andryuk wrote:
> > On 2024-05-20 12:44, Leigh Brown wrote:
> > > After the following commit:
> > > 3bc14e4fa4b9 ("tools/libs/light: Add vlan field to
> > > libxl_device_nic")
> > > xl list -l aborts with a double free error if a domain has at
> > > least
> > > one vif defined:
> > >
> > > $ sudo xl list -l
> > > free(): double free detected in tcache 2
> > > Aborted
> > >
> > > Orginally, the vlan field was called vid and was defined as an
> > > integer.
> > > It was appropriate to call libxl__xs_read_checked() with gc
> > > passed as
> > > the string data was copied to a different variable. However, the
> > > final
> > > version uses a string data type and the call should have been
> > > changed
> > > to use NOGC instead of gc to allow that data to live past the gc
> > > controlled lifetime, in line with the other string fields.
> > >
> > > This patch makes the change to pass NOGC instead of gc and moves
> > > the
> > > new code to be next to the other string fields (fixing a couple
> > > of
> > > errant tabs along the way), as recommended by Jason.
> > >
> > > Fixes: 3bc14e4fa4b9 ("tools/libs/light: Add vlan field to
> > > libxl_device_nic")
> > > Signed-off-by: Leigh Brown
> >
> > Reviewed-by: Jason Andryuk
>
> I notice this wasn't Cc-ed to the maintainer, which likely is the
> reason
> for there not having been an ack yet. Anthony, any thoughts?
>
> Further at this point, bug fix or not, it would likely also need a
> release
> ack. Oleksii, thoughts?
It seems to me it is bug fix, so it should be in release:
Release-acked-by: Oleksii Kurochko
~ Oleksii
Re: [PATCH for-4.19] hotplug: Restore block-tap phy compatibility
On Wed, 2024-06-19 at 14:33 +, Anthony PERARD wrote:
> On Wed, Jun 19, 2024 at 02:07:04PM +0200, Jan Beulich wrote:
> > On 16.05.2024 15:52, Jason Andryuk wrote:
> > > On 2024-05-16 03:41, Jan Beulich wrote:
> > > > On 16.05.2024 04:22, Jason Andryuk wrote:
> > > > > From: Jason Andryuk
> > > > >
> > > > > From: Jason Andryuk
> > > >
> > > > Two identical From: (also in another patch of yours, while in
> > > > yet another one
> > > > you have two _different_ ones, when only one will survive into
> > > > the eventual
> > > > commit anyway)?
> > >
> > > Sorry about that. Since I was sending from my gmail account, I
> > > thought
> > > I needed explicit From: lines to ensure the authorship was listed
> > > w/
> > > amd.com. I generated the patches with `git format-patch --from`,
> > > to get
> > > the explicit From: lines, and then sent with `git send-email`.
> > > The
> > > send-email step then inserted the additional lines. I guess it
> > > added
> > > From amd.com since I had changed to that address in .gitconfig.
> > >
> > > > > backendtype=phy using the blktap kernel module needs to use
> > > > > write_dev,
> > > > > but tapback can't support that. tapback should perform
> > > > > better, but make
> > > > > the script compatible with the old kernel module again.
> > > > >
> > > > > Signed-off-by: Jason Andryuk
> > > >
> > > > Should there be a Fixes: tag here?
> > >
> > > That makes sense.
> > >
> > > Fixes: 76a484193d ("hotplug: Update block-tap")
> >
> > Surely this wants going into 4.19? Thus - Anthony, Oleksii?
>
> Yes, I think so.
>
> Acked-by: Anthony PERARD
Release-Acked-by: Oleksii Kurochko
~ Oleksii
Re: [PATCH for-4.19] livepatch: use appropriate type for buffer offset variables
On Thu, 2024-06-20 at 09:04 +0100, Ross Lagerwall wrote:
> On Thu, Jun 20, 2024 at 8:16 AM Jan Beulich
> wrote:
> >
> > As was made noticeable by the last of the commits referenced below,
> > using a fixed-size type for such purposes is not only against
> > ./CODING_STYLE, but can lead to actual issues. Switch to using
> > size_t
> > instead, thus also allowing calculations to be lighter-weight in
> > 32-bit
> > builds.
> >
> > No functional change for 64-bit builds.
> >
> > Link: https://gitlab.com/xen-project/xen/-/jobs/7136417308
> > Fixes: b145b4a39c13 ("livepatch: Handle arbitrary size names with
> > the list operation")
> > Fixes: 5083e0ff939d ("livepatch: Add metadata runtime retrieval
> > mechanism")
> > Fixes: 43d5c5d5f70b ("xen: avoid UB in guest handle arithmetic")
> > Reported-by: Andrew Cooper
> > Signed-off-by: Jan Beulich
> >
>
> Reviewed-by: Ross Lagerwall
Release-Acked-by: Oleksii Kurochko
~ Oleksii
Re: [PATCH for-4.19 v4] x86/irq: forward pending interrupts to new destination in fixup_irqs()
On Wed, 2024-06-19 at 13:53 +0200, Jan Beulich wrote: > On 19.06.2024 11:58, Roger Pau Monne wrote: > > fixup_irqs() is used to evacuate interrupts from to be offlined > > CPUs. Given > > the CPU is to become offline, the normal migration logic used by > > Xen where the > > vector in the previous target(s) is left configured until the > > interrupt is > > received on the new destination is not suitable. > > > > Instead attempt to do as much as possible in order to prevent > > loosing > > interrupts. If fixup_irqs() is called from the CPU to be offlined > > (as is > > currently the case for CPU hot unplug) attempt to forward pending > > vectors when > > interrupts that target the current CPU are migrated to a different > > destination. > > > > Additionally, for interrupts that have already been moved from the > > current CPU > > prior to the call to fixup_irqs() but that haven't been delivered > > to the new > > destination (iow: interrupts with move_in_progress set and the > > current CPU set > > in ->arch.old_cpu_mask) also check whether the previous vector is > > pending and > > forward it to the new destination. > > > > This allows us to remove the window with interrupts enabled at the > > bottom of > > fixup_irqs(). Such window wasn't safe anyway: references to the > > CPU to become > > offline are removed from interrupts masks, but the per-CPU > > vector_irq[] array > > is not updated to reflect those changes (as the CPU is going > > offline anyway). > > > > Signed-off-by: Roger Pau Monné > > Reviewed-by: Jan Beulich Release-Acked-by: Oleksii Kurochko ~ Oleksii
Re: [PATCH for-4.19 v4 0/7] x86/xstate: Fixes to size calculations
On Mon, 2024-06-17 at 18:39 +0100, Andrew Cooper wrote:
> Only minor changes in v4 vs v3. See patches for details.
>
> The end result has been tested across the entire XenServer hardware
> lab. This
> found several false assupmtion about how the dynamic sizes behave.
Release-Acked-by: Oleksii Kurochko
~ Oleksii
>
> Patches 1 and 6 the main bugfixes from this series. There's still
> lots more
> work to do in order to get AMX and/or CET working, but this is at
> least a 4-yo
> series finally off my plate.
>
> Andrew Cooper (7):
> x86/xstate: Fix initialisation of XSS cache
> x86/xstate: Cross-check dynamic XSTATE sizes at boot
> x86/boot: Collect the Raw CPU Policy earlier on boot
> x86/xstate: Rework xstate_ctxt_size() as xstate_uncompressed_size()
> x86/cpu-policy: Simplify recalculate_xstate()
> x86/cpuid: Fix handling of XSAVE dynamic leaves
> x86/defns: Clean up X86_{XCR0,XSS}_* constants
>
> xen/arch/x86/cpu-policy.c | 56 ++--
> xen/arch/x86/cpuid.c | 24 +-
> xen/arch/x86/domctl.c | 2 +-
> xen/arch/x86/hvm/hvm.c | 2 +-
> xen/arch/x86/i387.c | 2 +-
> xen/arch/x86/include/asm/x86-defns.h | 55 ++--
> xen/arch/x86/include/asm/xstate.h | 8 +-
> xen/arch/x86/setup.c | 4 +-
> xen/arch/x86/xstate.c | 294 +-
> --
> xen/include/public/arch-x86/cpufeatureset.h | 3 +
> xen/include/xen/lib/x86/cpu-policy.h | 2 +-
> 11 files changed, 330 insertions(+), 122 deletions(-)
>
Re: [PATCH for-4.19] xen/arm: static-shmem: fix "gbase/pbase used uninitialized" build failure
Hi,
On Wed, 2024-06-19 at 09:02 +, Bertrand Marquis wrote:
> Hi,
>
> Adding Oleksii for Release ack.
>
> Cheers
> Bertrand
>
> > On 19 Jun 2024, at 08:46, Michal Orzel
> > wrote:
> >
> > Building Xen with CONFIG_STATIC_SHM=y results in a build failure:
> >
> > arch/arm/static-shmem.c: In function 'process_shm':
> > arch/arm/static-shmem.c:327:41: error: 'gbase' may be used
> > uninitialized [-Werror=maybe-uninitialized]
> > 327 | if ( is_domain_direct_mapped(d) && (pbase != gbase)
> > )
> > arch/arm/static-shmem.c:305:17: note: 'gbase' was declared here
> > 305 | paddr_t gbase, pbase, psize;
> >
> > This is because the commit cb1ddafdc573 adds a check referencing
> > gbase/pbase variables which were not yet assigned a value. Fix it.
> >
> > Fixes: cb1ddafdc573 ("xen/arm/static-shmem: Static-shmem should be
> > direct-mapped for direct-mapped domains")
> > Signed-off-by: Michal Orzel
Release-Acked-by: Oleksii Kurochko
~ Oleksii
> > ---
> > Rationale for 4.19: this patch fixes a build failure reported by
> > CI:
> > https://gitlab.com/xen-project/xen/-/jobs/7131807878
> > ---
> > xen/arch/arm/static-shmem.c | 13 +++--
> > 1 file changed, 7 insertions(+), 6 deletions(-)
> >
> > diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-
> > shmem.c
> > index c434b96e6204..cd48d2896b7e 100644
> > --- a/xen/arch/arm/static-shmem.c
> > +++ b/xen/arch/arm/static-shmem.c
> > @@ -324,12 +324,6 @@ int __init process_shm(struct domain *d,
> > struct kernel_info *kinfo,
> > printk("%pd: static shared memory bank not found:
> > '%s'", d, shm_id);
> > return -ENOENT;
> > }
> > - if ( is_domain_direct_mapped(d) && (pbase != gbase) )
> > - {
> > - printk("%pd: physical address 0x%"PRIpaddr" and guest
> > address 0x%"PRIpaddr" are not direct-mapped.\n",
> > - d, pbase, gbase);
> > - return -EINVAL;
> > - }
> >
> > pbase = boot_shm_bank->start;
> > psize = boot_shm_bank->size;
> > @@ -353,6 +347,13 @@ int __init process_shm(struct domain *d,
> > struct kernel_info *kinfo,
> > /* guest phys address is after host phys address */
> > gbase = dt_read_paddr(cells + addr_cells, addr_cells);
> >
> > + if ( is_domain_direct_mapped(d) && (pbase != gbase) )
> > + {
> > + printk("%pd: physical address 0x%"PRIpaddr" and
> > guest address 0x%"PRIpaddr" are not direct-mapped.\n",
> > + d, pbase, gbase);
> > + return -EINVAL;
> > + }
> > +
> > for ( i = 0; i < PFN_DOWN(psize); i++ )
> > if ( !mfn_valid(mfn_add(maddr_to_mfn(pbase), i)) )
> > {
> > --
> > 2.25.1
> >
>
Re: [PATCH for-4.19] xen/irq: Address MISRA Rule 8.3 violation
On Tue, 2024-06-18 at 14:00 +0100, Andrew Cooper wrote:
> When centralising irq_ack_none(), different architectures had
> different names
> for the parameter of irq_ack_none(). As it's type is struct irq_desc
> *, it
> should be named desc. Make this consistent.
>
> No functional change.
>
> Fixes: 8aeda4a241ab ("arch/irq: Make irq_ack_none() mandatory")
> Signed-off-by: Andrew Cooper
Release-Acked-by: Oleksii Kurochko
~ Oleksii
> ---
> CC: George Dunlap
> CC: Jan Beulich
> CC: Stefano Stabellini
> CC: Julien Grall
> CC: Volodymyr Babchuk
> CC: Bertrand Marquis
> CC: Michal Orzel
> CC: Oleksii Kurochko
> CC: Roberto Bagnara
> CC: Nicola Vetrini
> CC: consult...@bugseng.com
>
> Request for 4.19. This was an accidental regression in a recent
> cleanup
> patch, and the fix is just a rename - its no functional change.
> ---
> xen/arch/arm/irq.c | 4 ++--
> xen/include/xen/irq.h | 2 +-
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index c60502444ccf..6b89f64fd194 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -31,9 +31,9 @@ struct irq_guest
> unsigned int virq;
> };
>
> -void irq_ack_none(struct irq_desc *irq)
> +void irq_ack_none(struct irq_desc *desc)
> {
> - printk("unexpected IRQ trap at irq %02x\n", irq->irq);
> + printk("unexpected IRQ trap at irq %02x\n", desc->irq);
> }
>
> void irq_end_none(struct irq_desc *irq)
> diff --git a/xen/include/xen/irq.h b/xen/include/xen/irq.h
> index adf33547d25f..580ae37e7428 100644
> --- a/xen/include/xen/irq.h
> +++ b/xen/include/xen/irq.h
> @@ -134,7 +134,7 @@ void cf_check irq_actor_none(struct irq_desc
> *desc);
> * irq_ack_none() must be provided by the architecture.
> * irq_end_none() is optional, and opted into using a define.
> */
> -void cf_check irq_ack_none(struct irq_desc *irq);
> +void cf_check irq_ack_none(struct irq_desc *desc);
>
> /*
> * Per-cpu interrupted context register state - the inner-most
> interrupt frame
>
> base-commit: 8b4243a9b560c89bb259db5a27832c253d4bebc7
Re: [PATCH for-4.19] avoid UB in guest handle arithmetic
On Tue, 2024-06-18 at 14:24 +0100, Andrew Cooper wrote:
> On 19/03/2024 1:26 pm, Jan Beulich wrote:
> > At least XENMEM_memory_exchange can have huge values passed in the
> > nr_extents and nr_exchanged fields. Adding such values to pointers
> > can
> > overflow, resulting in UB. Cast respective pointers to "unsigned
> > long"
> > while at the same time making the necessary multiplication
> > explicit.
> > Remaining arithmetic is, despite there possibly being mathematical
> > overflow, okay as per the C99 spec: "A computation involving
> > unsigned
> > operands can never overflow, because a result that cannot be
> > represented
> > by the resulting unsigned integer type is reduced modulo the number
> > that
> > is one greater than the largest value that can be represented by
> > the
> > resulting type." The overflow that we need to guard against is
> > checked
> > for in array_access_ok().
> >
> > Note that in / down from array_access_ok() the address value is
> > only
> > ever cast to "unsigned long" anyway, which is why in the invocation
> > from
> > guest_handle_subrange_okay() the value doesn't need casting back to
> > pointer type.
> >
> > In compat grant table code change two guest_handle_add_offset() to
> > avoid
> > passing in negative offsets.
> >
> > Since {,__}clear_guest_offset() need touching anyway, also deal
> > with
> > another (latent) issue there: They were losing the handle type,
> > i.e. the
> > size of the individual objects accessed. Luckily the few users we
> > presently have all pass char or uint8 handles.
> >
> > Reported-by: Andrew Cooper
> > Signed-off-by: Jan Beulich
>
> There wants to be a xen: prefix in the subject.
>
> But as for the UB aspect, I've checked that this does resolve the
> failure identified by the XSA-212 XTF test.
>
> Acked-by: Andrew Cooper
> Tested-by: Andrew Cooper
>
> CC'ing Oleksii as this wants to go into 4.19.
Release-Acked-By: Oleksii Kurochko
~ Oleksii
Re: [PATCH v12 5/8] xen/riscv: add minimal stuff to mm.h to build full Xen
On Fri, 2024-06-14 at 10:47 +0100, Andrew Cooper wrote:
> On 11/06/2024 7:23 pm, Oleksii K. wrote:
> > On Tue, 2024-06-11 at 16:53 +0100, Andrew Cooper wrote:
> > > On 30/05/2024 7:22 pm, Oleksii K. wrote:
> > > > On Thu, 2024-05-30 at 18:23 +0100, Andrew Cooper wrote:
> > > > > On 29/05/2024 8:55 pm, Oleksii Kurochko wrote:
> > > > > > Signed-off-by: Oleksii Kurochko
> > > > > >
> > > > > > Acked-by: Jan Beulich
> > > > > This patch looks like it can go in independently? Or does it
> > > > > depend
> > > > > on
> > > > > having bitops.h working in practice?
> > > > >
> > > > > However, one very strong suggestion...
> > > > >
> > > > >
> > > > > > diff --git a/xen/arch/riscv/include/asm/mm.h
> > > > > > b/xen/arch/riscv/include/asm/mm.h
> > > > > > index 07c7a0abba..cc4a07a71c 100644
> > > > > > --- a/xen/arch/riscv/include/asm/mm.h
> > > > > > +++ b/xen/arch/riscv/include/asm/mm.h
> > > > > > @@ -3,11 +3,246 @@
> > > > > >
> > > > > > +/* PDX of the first page in the frame table. */
> > > > > > +extern unsigned long frametable_base_pdx;
> > > > > > +
> > > > > > +/* Convert between machine frame numbers and page-info
> > > > > > structures.
> > > > > > */
> > > > > > +#define
> > > > > > mfn_to_page(mfn)
> > > > > > \
> > > > > > + (frame_table + (mfn_to_pdx(mfn) -
> > > > > > frametable_base_pdx))
> > > > > > +#define
> > > > > > page_to_mfn(pg)
> > > > > > \
> > > > > > + pdx_to_mfn((unsigned long)((pg) - frame_table) +
> > > > > > frametable_base_pdx)
> > > > > Do yourself a favour and not introduce frametable_base_pdx to
> > > > > begin
> > > > > with.
> > > > >
> > > > > Every RISC-V board I can find has things starting from 0 in
> > > > > physical
> > > > > address space, with RAM starting immediately after.
> > > > I checked Linux kernel and grep there:
> > > > [ok@fedora linux-aia]$ grep -Rni "memory@"
> > > > arch/riscv/boot/dts/
> > > > --
> > > > exclude "*.tmp" -I
> > > > arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-
> > > > 2.dtsi:33:
> > > > memory@4000 {
> > > > arch/riscv/boot/dts/starfive/jh7100-common.dtsi:28:
> > > > memory@8000
> > > > {
> > > > arch/riscv/boot/dts/microchip/mpfs-sev-kit.dts:49:
> > > > ddrc_cache:
> > > > memory@10 {
> > > > arch/riscv/boot/dts/microchip/mpfs-m100pfsevp.dts:33:
> > > > ddrc_cache_lo:
> > > > memory@8000 {
> > > > arch/riscv/boot/dts/microchip/mpfs-m100pfsevp.dts:37:
> > > > ddrc_cache_hi:
> > > > memory@104000 {
> > > > arch/riscv/boot/dts/microchip/mpfs-tysom-m.dts:34:
> > > > ddrc_cache_lo:
> > > > memory@8000 {
> > > > arch/riscv/boot/dts/microchip/mpfs-tysom-m.dts:40:
> > > > ddrc_cache_hi:
> > > > memory@10 {
> > > > arch/riscv/boot/dts/microchip/mpfs-polarberry.dts:22:
> > > > ddrc_cache_lo:
> > > > memory@8000 {
> > > > arch/riscv/boot/dts/microchip/mpfs-polarberry.dts:27:
> > > > ddrc_cache_hi:
> > > > memory@10 {
> > > > arch/riscv/boot/dts/microchip/mpfs-icicle-kit.dts:57:
> > > > ddrc_cache_lo:
> > > > memory@8000 {
> > > > arch/riscv/boot/dts/microchip/mpfs-icicle-kit.dts:63:
> > > > ddrc_cache_hi:
> > > > memory@104000 {
> > > > arch/riscv/boot/dts/thead/th1520-beaglev-ahead.dts:32:
> > > > memory@0
> > > > {
> > > > arch/riscv/boot/dts/thead/th1520-lichee-module-
> > > > 4a.dtsi:14:
> > > > memory@0 {
> > > > arch/riscv/boot/dts/sophgo/cv1800b-milkv-duo.dts:26:
> > > > memory@8000
> > > > {
> > > > arch/riscv/boot/dts/sophgo/cv
Re: [PATCH v3 for-4.19 2/3] x86/irq: handle moving interrupts in _assign_irq_vector()
On Mon, 2024-06-17 at 15:31 +0200, Jan Beulich wrote:
> On 13.06.2024 18:56, Roger Pau Monne wrote:
> > Currently there's logic in fixup_irqs() that attempts to prevent
> > _assign_irq_vector() from failing, as fixup_irqs() is required to
> > evacuate all
> > interrupts from the CPUs not present in the input mask. The
> > current logic in
> > fixup_irqs() is incomplete, as it doesn't deal with interrupts that
> > have
> > move_cleanup_count > 0 and a non-empty ->arch.old_cpu_mask field.
> >
> > Instead of attempting to fixup the interrupt descriptor in
> > fixup_irqs() so that
> > _assign_irq_vector() cannot fail, introduce logic in
> > _assign_irq_vector()
> > to deal with interrupts that have either
> > move_{in_progress,cleanup_count} set
> > and no remaining online CPUs in ->arch.cpu_mask.
> >
> > If _assign_irq_vector() is requested to move an interrupt in the
> > state
> > described above, first attempt to see if ->arch.old_cpu_mask
> > contains any valid
> > CPUs that could be used as fallback, and if that's the case do move
> > the
> > interrupt back to the previous destination. Note this is easier
> > because the
> > vector hasn't been released yet, so there's no need to allocate and
> > setup a new
> > vector on the destination.
> >
> > Due to the logic in fixup_irqs() that clears offline CPUs from
> > ->arch.old_cpu_mask (and releases the old vector if the mask
> > becomes empty) it
> > shouldn't be possible to get into _assign_irq_vector() with
> > ->arch.move_{in_progress,cleanup_count} set but no online CPUs in
> > ->arch.old_cpu_mask.
> >
> > However if ->arch.move_{in_progress,cleanup_count} is set and the
> > interrupt has
> > also changed affinity, it's possible the members of -
> > >arch.old_cpu_mask are no
> > longer part of the affinity set, move the interrupt to a different
> > CPU part of
> > the provided mask and keep the current ->arch.old_{cpu_mask,vector}
> > for the
> > pending interrupt movement to be completed.
> >
> > Signed-off-by: Roger Pau Monné
>
> Reviewed-by: Jan Beulich
Release-acked-by: Oleksii Kurochko
~ Oleksii
>
> > --- a/xen/arch/x86/irq.c
> > +++ b/xen/arch/x86/irq.c
> > @@ -544,7 +544,58 @@ static int _assign_irq_vector(struct irq_desc
> > *desc, const cpumask_t *mask)
> > }
> >
> > if ( desc->arch.move_in_progress || desc-
> > >arch.move_cleanup_count )
> > - return -EAGAIN;
> > + {
> > + /*
> > + * If the current destination is online refuse to
> > shuffle. Retry after
> > + * the in-progress movement has finished.
> > + */
> > + if ( cpumask_intersects(desc->arch.cpu_mask,
> > _online_map) )
> > + return -EAGAIN;
> > +
> > + /*
> > + * Due to the logic in fixup_irqs() that clears offlined
> > CPUs from
> > + * ->arch.old_cpu_mask it shouldn't be possible to get
> > here with
> > + * ->arch.move_{in_progress,cleanup_count} set and no
> > online CPUs in
> > + * ->arch.old_cpu_mask.
> > + */
> > + ASSERT(valid_irq_vector(desc->arch.old_vector));
> > + ASSERT(cpumask_intersects(desc->arch.old_cpu_mask,
> > _online_map));
> > +
> > + if ( cpumask_intersects(desc->arch.old_cpu_mask, mask) )
> > + {
> > + /*
> > + * Fallback to the old destination if moving is in
> > progress and the
> > + * current destination is to be offlined. This is
> > only possible if
> > + * the CPUs in old_cpu_mask intersect with the
> > affinity mask passed
> > + * in the 'mask' parameter.
> > + */
> > + desc->arch.vector = desc->arch.old_vector;
>
> I'm a little puzzled that you use desc->arch.old_vector here, but ...
>
> > + cpumask_and(desc->arch.cpu_mask, desc-
> > >arch.old_cpu_mask, mask);
> > +
> > + /* Undo any possibly done cleanup. */
> > + for_each_cpu(cpu, desc->arch.cpu_mask)
> > + per_cpu(vector_irq, cpu)[desc->arch.vector] = irq;
> > +
> > + /* Cancel the pending move and release the current
> > vector. */
> > + desc->arch.old_vector = IRQ_VECTOR_UNASSIGNED;
> > + cpumask_clear(desc->arch.old_cpu_mask);
> > + desc->arch.move_in_progress = 0;
> > + desc->arch.move_cleanup_count = 0;
> > + if ( desc->arch.used_vectors )
> > + {
> > + ASSERT(test_bit(old_vector, desc-
> > >arch.used_vectors));
> > + clear_bit(old_vector, desc->arch.used_vectors);
>
> ... old_vector here. Since we have the latter, uniformly using it
> might
> be more consistent. I realize though that irq_to_vector() has cases
> where
> it wouldn't return desc->arch.old_vector; I think, however, that in
> those
> case we can't make it here. Still I'm not going to insist on making
> the
> adjustment. Happy to make it though while committing, should you
> agree.
>
> Also I'm not happy to see another instance of this
Re: [PATCH v3 for-4.19 1/3] x86/irq: deal with old_cpu_mask for interrupts in movement in fixup_irqs()
On Mon, 2024-06-17 at 15:18 +0200, Jan Beulich wrote: > On 13.06.2024 18:56, Roger Pau Monne wrote: > > Given the current logic it's possible for ->arch.old_cpu_mask to > > get out of > > sync: if a CPU set in old_cpu_mask is offlined and then onlined > > again without old_cpu_mask having been updated the data in the mask > > will no > > longer be accurate, as when brought back online the CPU will no > > longer have > > old_vector configured to handle the old interrupt source. > > > > If there's an interrupt movement in progress, and the to be > > offlined CPU (which > > is the call context) is in the old_cpu_mask clear it and update the > > mask, so it > > doesn't contain stale data. > > Perhaps a comma before "clear" might further help reading. Happy to > add while committing. > > > Note that when the system is going down fixup_irqs() will be called > > by > > smp_send_stop() from CPU 0 with a mask with only CPU 0 on it, > > effectively > > asking to move all interrupts to the current caller (CPU 0) which > > is the only > > CPU to remain online. In that case we don't care to migrate > > interrupts that > > are in the process of being moved, as it's likely we won't be able > > to move all > > interrupts to CPU 0 due to vector shortage anyway. > > > > Signed-off-by: Roger Pau Monné > > Reviewed-by: Jan Beulich Release-Acked-by: Oleksii Kurochko ~ Oleksii
Re: [PATCH] xen/ubsan: Fix UB in type_descriptor declaration
On Mon, 2024-06-17 at 18:55 +0100, Andrew Cooper wrote:
> struct type_descriptor is arranged with a NUL terminated string
Should it be NULL instead of NUL?
> following the
> kind/info fields.
>
> The only reason this doesn't trip UBSAN detection itself (on more
> modern
> compilers at least) is because struct type_descriptor is only
> referenced in
> suppressed regions.
>
> Switch the declaration to be a real flexible member. No functional
> change.
>
> Fixes: 00fcf4dd8eb4 ("xen/ubsan: Import ubsan implementation from
> Linux 4.13")
> Signed-off-by: Andrew Cooper
Release-Acked-by: Oleksii Kurochko
~ Oleksii
> ---
> CC: George Dunlap
> CC: Jan Beulich
> CC: Stefano Stabellini
> CC: Julien Grall
> CC: Oleksii Kurochko
>
> For 4.19, and for backport to all reasonable versions. This bug
> deserves some
> kind of irony award.
> ---
> xen/common/ubsan/ubsan.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/common/ubsan/ubsan.h b/xen/common/ubsan/ubsan.h
> index a3159040fefb..3db42e75b138 100644
> --- a/xen/common/ubsan/ubsan.h
> +++ b/xen/common/ubsan/ubsan.h
> @@ -10,7 +10,7 @@ enum {
> struct type_descriptor {
> u16 type_kind;
> u16 type_info;
> - char type_name[1];
> + char type_name[];
> };
>
> struct source_location {
>
> base-commit: 8b4243a9b560c89bb259db5a27832c253d4bebc7
Re: [PATCH for-4.19] xen/arch: Centralise __read_mostly and __ro_after_init
On Fri, 2024-06-14 at 13:49 +0100, Andrew Cooper wrote:
> These being in cache.h is inherited from Linux, but is an
> inappropriate
> location to live.
>
> __read_mostly is an optimisation related to data placement in order
> to avoid
> having shared data in cachelines that are likely to be written to,
> but it
> really is just a section of the linked image separating data by usage
> patterns; it has nothing to do with cache sizes or flushing logic.
>
> Worse, __ro_after_init was only in xen/cache.h because __read_mostly
> was in
> arch/cache.h, and has literally nothing whatsoever to do with caches.
>
> Move the definitions into xen/sections.h, which in paritcular means
> that
> RISC-V doesn't need to repeat the problematic pattern. Take the
> opportunity
> to provide a short descriptions of what these are used for.
>
> For now, leave TODO comments next to the other identical
> definitions. It
> turns out that unpicking cache.h is more complicated than it appears
> because a
> number of files use it for transitive dependencies.
>
> Signed-off-by: Andrew Cooper
Release-Acked-By: Oleksii Kurochko
> ---
> CC: Jan Beulich
> CC: Roger Pau Monné
> CC: Stefano Stabellini
> CC: Julien Grall
> CC: Volodymyr Babchuk
> CC: Bertrand Marquis
> CC: Michal Orzel
> CC: Oleksii Kurochko
> CC: Shawn Anastasio
>
> For 4.19. This is to help the RISC-V "full build of Xen" effort
> without
> introducing a pattern that's going to require extra effort to undo
> after the
> fact.
> ---
> xen/arch/arm/include/asm/cache.h | 1 +
> xen/arch/ppc/include/asm/cache.h | 1 +
> xen/arch/x86/include/asm/cache.h | 1 +
> xen/include/xen/cache.h | 1 +
> xen/include/xen/sections.h | 21 +
> 5 files changed, 25 insertions(+)
>
> diff --git a/xen/arch/arm/include/asm/cache.h
> b/xen/arch/arm/include/asm/cache.h
> index 240b6ae0eaa3..029b2896fb3e 100644
> --- a/xen/arch/arm/include/asm/cache.h
> +++ b/xen/arch/arm/include/asm/cache.h
> @@ -6,6 +6,7 @@
> #define L1_CACHE_SHIFT (CONFIG_ARM_L1_CACHE_SHIFT)
> #define L1_CACHE_BYTES (1 << L1_CACHE_SHIFT)
>
> +/* TODO: Phase out the use of this via cache.h */
> #define __read_mostly __section(".data.read_mostly")
>
> #endif
> diff --git a/xen/arch/ppc/include/asm/cache.h
> b/xen/arch/ppc/include/asm/cache.h
> index 0d7323d7892f..13c0bf3242c8 100644
> --- a/xen/arch/ppc/include/asm/cache.h
> +++ b/xen/arch/ppc/include/asm/cache.h
> @@ -3,6 +3,7 @@
> #ifndef _ASM_PPC_CACHE_H
> #define _ASM_PPC_CACHE_H
>
> +/* TODO: Phase out the use of this via cache.h */
> #define __read_mostly __section(".data.read_mostly")
>
> #endif /* _ASM_PPC_CACHE_H */
> diff --git a/xen/arch/x86/include/asm/cache.h
> b/xen/arch/x86/include/asm/cache.h
> index e4770efb22b9..956c05493e23 100644
> --- a/xen/arch/x86/include/asm/cache.h
> +++ b/xen/arch/x86/include/asm/cache.h
> @@ -9,6 +9,7 @@
> #define L1_CACHE_SHIFT (CONFIG_X86_L1_CACHE_SHIFT)
> #define L1_CACHE_BYTES (1 << L1_CACHE_SHIFT)
>
> +/* TODO: Phase out the use of this via cache.h */
> #define __read_mostly __section(".data.read_mostly")
>
> #ifndef __ASSEMBLY__
> diff --git a/xen/include/xen/cache.h b/xen/include/xen/cache.h
> index f52a0aedf768..55456823c543 100644
> --- a/xen/include/xen/cache.h
> +++ b/xen/include/xen/cache.h
> @@ -15,6 +15,7 @@
> #define __cacheline_aligned
> __attribute__((__aligned__(SMP_CACHE_BYTES)))
> #endif
>
> +/* TODO: Phase out the use of this via cache.h */
> #define __ro_after_init __section(".data.ro_after_init")
>
> #endif /* __LINUX_CACHE_H */
> diff --git a/xen/include/xen/sections.h b/xen/include/xen/sections.h
> index b6cb5604c285..6d4db2b38f0f 100644
> --- a/xen/include/xen/sections.h
> +++ b/xen/include/xen/sections.h
> @@ -3,9 +3,30 @@
> #ifndef __XEN_SECTIONS_H__
> #define __XEN_SECTIONS_H__
>
> +#include
> +
> /* SAF-0-safe */
> extern char __init_begin[], __init_end[];
>
> +/*
> + * Some data is expected to be written very rarely (if at all).
> + *
> + * For performance reasons is it helpful to group such data in the
> build, to
> + * avoid the linker placing it adjacent to often-written data.
> + */
> +#define __read_mostly __section(".data.read_mostly")
> +
> +/*
> + * Some data should be chosen during boot and be immutable
> thereafter.
> + *
> + * Variables annotated with __ro_after_init will become read-only
> after boot
> + * and suffer a runtime access fault if modified.
> + *
> + * For architectures/platforms which haven't implemented support,
> these
> + * variables will be treated as regular mutable data.
> + */
> +#define __ro_after_init __section(".data.ro_after_init")
> +
> #endif /* !__XEN_SECTIONS_H__ */
> /*
> * Local variables:
>
> base-commit: 8b4243a9b560c89bb259db5a27832c253d4bebc7
Re: [PATCH v3 0/3] x86/irq: fixes for CPU hot{,un}plug
On Fri, 2024-06-14 at 09:28 +0200, Roger Pau Monné wrote:
> Sorry, forgot to add the for-4.19 tag and Cc Oleksii.
>
> Since we have taken the start of the series, we might as well take
> the
> remaining patches (if other x86 maintainers agree) and attempt to
> hopefully fix all the interrupt issues with CPU hotplug/unplug.
>
> FTR: there are further issues when doing CPU hotplug/unplug from a
> PVH
> dom0, but those are out of the scope for 4.19, as I haven't even
> started to diagnose what's going on.
And this issues were before the current patch series was introduced?
~ Oleksii
>
> Thanks, Roger.
>
> On Thu, Jun 13, 2024 at 06:56:14PM +0200, Roger Pau Monne wrote:
> > Hello,
> >
> > The following series aim to fix interrupt handling when doing CPU
> > plug/unplug operations. Without this series running:
> >
> > cpus=`xl info max_cpu_id`
> > while [ 1 ]; do
> > for i in `seq 1 $cpus`; do
> > xen-hptool cpu-offline $i;
> > xen-hptool cpu-online $i;
> > done
> > done
> >
> > Quite quickly results in interrupts getting lost and "No irq
> > handler for
> > vector" messages on the Xen console. Drivers in dom0 also start
> > getting
> > interrupt timeouts and the system becomes unusable.
> >
> > After applying the series running the loop over night still result
> > in a
> > fully usable system, no "No irq handler for vector" messages at
> > all, no
> > interrupt loses reported by dom0. Test with x2apic-
> > mode={mixed,cluster}.
> >
> > I've attempted to document all code as good as I could, interrupt
> > handling has some unexpected corner cases that are hard to diagnose
> > and
> > reason about.
> >
> > Some XenRT testing is undergoing to ensure no breakages.
> >
> > Thanks, Roger.
> >
> > Roger Pau Monne (3):
> > x86/irq: deal with old_cpu_mask for interrupts in movement in
> > fixup_irqs()
> > x86/irq: handle moving interrupts in _assign_irq_vector()
> > x86/irq: forward pending interrupts to new destination in
> > fixup_irqs()
> >
> > xen/arch/x86/include/asm/apic.h | 5 +
> > xen/arch/x86/irq.c | 163 +---
> >
> > 2 files changed, 132 insertions(+), 36 deletions(-)
> >
> > --
> > 2.45.2
> >
Re: [PATCH v2 for-4.19 2/3] x86/EPT: avoid marking non-present entries for re-configuring
On Wed, 2024-06-12 at 15:16 +0200, Jan Beulich wrote:
> For non-present entries EMT, like most other fields, is meaningless
> to
> hardware. Make the logic in ept_set_entry() setting the field (and
> iPAT)
> conditional upon dealing with a present entry, leaving the value at 0
> otherwise. This has two effects for epte_get_entry_emt() which we'll
> want to leverage subsequently:
> 1) The call moved here now won't be issued with INVALID_MFN anymore
> (a
> respective BUG_ON() is being added).
> 2) Neither of the other two calls could now be issued with a
> truncated
> form of INVALID_MFN anymore (as long as there's no bug anywhere
> marking an entry present when that was populated using
> INVALID_MFN).
>
> Signed-off-by: Jan Beulich
Release-Acked-By: Oleksii Kurochko
~ Oleksii
> ---
> v2: New.
>
> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -650,6 +650,8 @@ static int cf_check resolve_misconfig(st
> if ( e.emt != MTRR_NUM_TYPES )
> break;
>
> + ASSERT(is_epte_present());
> +
> if ( level == 0 )
> {
> for ( gfn -= i, i = 0; i < EPT_PAGETABLE_ENTRIES;
> ++i )
> @@ -915,17 +917,6 @@ ept_set_entry(struct p2m_domain *p2m, gf
>
> if ( mfn_valid(mfn) || p2m_allows_invalid_mfn(p2mt) )
> {
> - bool ipat;
> - int emt = epte_get_entry_emt(p2m->domain, _gfn(gfn), mfn,
> - i * EPT_TABLE_ORDER, ,
> - p2mt);
> -
> - if ( emt >= 0 )
> - new_entry.emt = emt;
> - else /* ept_handle_misconfig() will need to take care of
> this. */
> - new_entry.emt = MTRR_NUM_TYPES;
> -
> - new_entry.ipat = ipat;
> new_entry.sp = !!i;
> new_entry.sa_p2mt = p2mt;
> new_entry.access = p2ma;
> @@ -941,6 +932,22 @@ ept_set_entry(struct p2m_domain *p2m, gf
> need_modify_vtd_table = 0;
>
> ept_p2m_type_to_flags(p2m, _entry);
> +
> + if ( is_epte_present(_entry) )
> + {
> + bool ipat;
> + int emt = epte_get_entry_emt(p2m->domain, _gfn(gfn),
> mfn,
> + i * EPT_TABLE_ORDER, ,
> + p2mt);
> +
> + BUG_ON(mfn_eq(mfn, INVALID_MFN));
> +
> + if ( emt >= 0 )
> + new_entry.emt = emt;
> + else /* ept_handle_misconfig() will need to take care of
> this. */
> + new_entry.emt = MTRR_NUM_TYPES;
> + new_entry.ipat = ipat;
> + }
> }
>
> if ( sve != -1 )
>
Re: [PATCH v2 for-4.19 1/3] x86/EPT: correct special page checking in epte_get_entry_emt()
On Wed, 2024-06-12 at 15:16 +0200, Jan Beulich wrote:
> mfn_valid() granularity is (currently) 256Mb. Therefore the start of
> a
> 1Gb page passing the test doesn't necessarily mean all parts of such
> a
> range would also pass. Yet using the result of mfn_to_page() on an
> MFN
> which doesn't pass mfn_valid() checking is liable to result in a
> crash
> (the invocation of mfn_to_page() alone is presumably "just" UB in
> such a
> case).
>
> Fixes: ca24b2ffdbd9 ("x86/hvm: set 'ipat' in EPT for special pages")
> Signed-off-by: Jan Beulich
Release-Acked-by: Oleksii Kurochko
~ Oleksii
> ---
> Of course we could leverage mfn_valid() granularity here to do an
> increment by more than 1 if mfn_valid() returned false. Yet doing so
> likely would want a suitable helper to be introduced first, rather
> than
> open-coding such logic here.
> ---
> v2: New.
>
> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -519,8 +519,12 @@ int epte_get_entry_emt(struct domain *d,
> }
>
> for ( special_pgs = i = 0; i < (1ul << order); i++ )
> - if ( is_special_page(mfn_to_page(mfn_add(mfn, i))) )
> + {
> + mfn_t cur = mfn_add(mfn, i);
> +
> + if ( mfn_valid(cur) && is_special_page(mfn_to_page(cur)) )
> special_pgs++;
> + }
>
> if ( special_pgs )
> {
>
Re: [PATCH for 4.19?] x86/Intel: unlock CPUID earlier for the BSP
On Thu, 2024-06-13 at 10:19 +0200, Jan Beulich wrote:
> Intel CPUs have a MSR bit to limit CPUID enumeration to leaf two. If
> this bit is set by the BIOS then CPUID evaluation does not work when
> data from any leaf greater than two is needed; early_cpu_init() in
> particular wants to collect leaf 7 data.
>
> Cure this by unlocking CPUID right before evaluating anything which
> depends on the maximum CPUID leaf being greater than two.
>
> Inspired by (and description cloned from) Linux commit 0c2f6d04619e
> ("x86/topology/intel: Unlock CPUID before evaluating anything").
>
> Signed-off-by: Jan Beulich
Release-Acked-by: Oleksii Kurochko
~ Oleksii
> ---
> While I couldn't spot anything, it kind of feels as if I'm
> overlooking
> further places where we might be inspecting in particular leaf 7 yet
> earlier.
>
> No Fixes: tag(s), as imo it would be too many that would want
> enumerating.
>
> --- a/xen/arch/x86/cpu/common.c
> +++ b/xen/arch/x86/cpu/common.c
> @@ -336,7 +336,8 @@ void __init early_cpu_init(bool verbose)
>
> c->x86_vendor = x86_cpuid_lookup_vendor(ebx, ecx, edx);
> switch (c->x86_vendor) {
> - case X86_VENDOR_INTEL: actual_cpu = intel_cpu_dev;
> break;
> + case X86_VENDOR_INTEL: intel_unlock_cpuid_leaves(c);
> + actual_cpu = intel_cpu_dev;
> break;
> case X86_VENDOR_AMD: actual_cpu = amd_cpu_dev;
> break;
> case X86_VENDOR_CENTAUR: actual_cpu = centaur_cpu_dev;
> break;
> case X86_VENDOR_SHANGHAI: actual_cpu = shanghai_cpu_dev;
> break;
> --- a/xen/arch/x86/cpu/cpu.h
> +++ b/xen/arch/x86/cpu/cpu.h
> @@ -24,3 +24,5 @@ void amd_init_lfence(struct cpuinfo_x86
> void amd_init_ssbd(const struct cpuinfo_x86 *c);
> void amd_init_spectral_chicken(void);
> void detect_zen2_null_seg_behaviour(void);
> +
> +void intel_unlock_cpuid_leaves(struct cpuinfo_x86 *c);
> --- a/xen/arch/x86/cpu/intel.c
> +++ b/xen/arch/x86/cpu/intel.c
> @@ -303,10 +303,24 @@ static void __init noinline intel_init_l
> ctxt_switch_masking = intel_ctxt_switch_masking;
> }
>
> -static void cf_check early_init_intel(struct cpuinfo_x86 *c)
> +/* Unmask CPUID levels if masked. */
> +void intel_unlock_cpuid_leaves(struct cpuinfo_x86 *c)
> {
> - u64 misc_enable, disable;
> + uint64_t misc_enable, disable;
> +
> + rdmsrl(MSR_IA32_MISC_ENABLE, misc_enable);
> +
> + disable = misc_enable & MSR_IA32_MISC_ENABLE_LIMIT_CPUID;
> + if (disable) {
> + wrmsrl(MSR_IA32_MISC_ENABLE, misc_enable &
> ~disable);
> + bootsym(trampoline_misc_enable_off) |= disable;
> + c->cpuid_level = cpuid_eax(0);
> + printk(KERN_INFO "revised cpuid level: %u\n", c-
> >cpuid_level);
> + }
> +}
>
> +static void cf_check early_init_intel(struct cpuinfo_x86 *c)
> +{
> /* Netburst reports 64 bytes clflush size, but does IO in
> 128 bytes */
> if (c->x86 == 15 && c->x86_cache_alignment == 64)
> c->x86_cache_alignment = 128;
> @@ -315,16 +329,7 @@ static void cf_check early_init_intel(st
> bootsym(trampoline_misc_enable_off) &
> MSR_IA32_MISC_ENABLE_XD_DISABLE)
> printk(KERN_INFO "re-enabled NX (Execute Disable)
> protection\n");
>
> - /* Unmask CPUID levels and NX if masked: */
> - rdmsrl(MSR_IA32_MISC_ENABLE, misc_enable);
> -
> - disable = misc_enable & MSR_IA32_MISC_ENABLE_LIMIT_CPUID;
> - if (disable) {
> - wrmsrl(MSR_IA32_MISC_ENABLE, misc_enable &
> ~disable);
> - bootsym(trampoline_misc_enable_off) |= disable;
> - printk(KERN_INFO "revised cpuid level: %d\n",
> - cpuid_eax(0));
> - }
> + intel_unlock_cpuid_leaves(c);
>
> /* CPUID workaround for Intel 0F33/0F34 CPU */
> if (boot_cpu_data.x86 == 0xF && boot_cpu_data.x86_model == 3
> &&
Re: [PATCH v2 3/7] x86/irq: limit interrupt movement done by fixup_irqs()
On Tue, 2024-06-11 at 11:59 +0200, Jan Beulich wrote: > On 10.06.2024 16:20, Roger Pau Monne wrote: > > The current check used in fixup_irqs() to decide whether to move > > around > > interrupts is based on the affinity mask, but such mask can have > > all bits set, > > and hence is unlikely to be a subset of the input mask. For > > example if an > > interrupt has an affinity mask of all 1s, any input to fixup_irqs() > > that's not > > an all set CPU mask would cause that interrupt to be shuffled > > around > > unconditionally. > > > > What fixup_irqs() care about is evacuating interrupts from CPUs not > > set on the > > input CPU mask, and for that purpose it should check whether the > > interrupt is > > assigned to a CPU not present in the input mask. Assume that - > > >arch.cpu_mask > > is a subset of the ->affinity mask, and keep the current logic that > > resets the > > ->affinity mask if the interrupt has to be shuffled around. > > > > Doing the affinity movement based on ->arch.cpu_mask requires > > removing the > > special handling to ->arch.cpu_mask done for high priority vectors, > > otherwise > > the adjustment done to cpu_mask makes them always skip the CPU > > interrupt > > movement. > > > > While there also adjust the comment as to the purpose of > > fixup_irqs(). > > > > Signed-off-by: Roger Pau Monné > > Reviewed-by: Jan Beulich Release-Acked-By: Oleksii Kurochko ~ Oleksii > > Aiui this is independent of patch 1, so could go in while we still > settle on > how to word things there? > > Jan >
Re: [PATCH v2 2/7] x86/irq: describe how the interrupt CPU movement works
On Tue, 2024-06-11 at 09:44 +0200, Jan Beulich wrote: > On 10.06.2024 16:20, Roger Pau Monne wrote: > > The logic to move interrupts across CPUs is complex, attempt to > > provide a > > comment that describes the expected behavior so users of the > > interrupt system > > have more context about the usage of the arch_irq_desc structure > > fields. > > > > Signed-off-by: Roger Pau Monné > > Reviewed-by: Jan Beulich Release-Acked-by: Oleksii Kurochko ~ Oleksii
Re: [PATCH v2 1/7] x86/smp: do not use shorthand IPI destinations in CPU hot{,un}plug contexts
On Wed, 2024-06-12 at 10:56 +0200, Jan Beulich wrote:
> On 12.06.2024 10:09, Roger Pau Monné wrote:
> > On Tue, Jun 11, 2024 at 09:42:39AM +0200, Jan Beulich wrote:
> > > On 10.06.2024 16:20, Roger Pau Monne wrote:
> > > > Due to the current rwlock logic, if the CPU calling
> > > > get_cpu_maps() does so from
> > > > a cpu_hotplug_{begin,done}() region the function will still
> > > > return success,
> > > > because a CPU taking the rwlock in read mode after having taken
> > > > it in write
> > > > mode is allowed. Such behavior however defeats the purpose of
> > > > get_cpu_maps(),
> > >
> > > I'm not happy to see you still have this claim here. The behavior
> > > may (appear
> > > to) defeat the purpose here, but as expressed previously I don't
> > > view that as
> > > being a general pattern.
> >
> > Right. What about replacing the paragraph with:
> >
> > "Due to the current rwlock logic, if the CPU calling get_cpu_maps()
> > does so from
> > a cpu_hotplug_{begin,done}() region the function will still return
> > success,
> > because a CPU taking the rwlock in read mode after having taken it
> > in write
> > mode is allowed. Such corner case makes using get_cpu_maps() alone
> > not enough to prevent using the shorthand in CPU hotplug regions."
>
> Thanks.
>
> > I think the rest is of the commit message is not controversial.
>
> Indeed.
>
> > > > --- a/xen/arch/x86/smp.c
> > > > +++ b/xen/arch/x86/smp.c
> > > > @@ -88,7 +88,7 @@ void send_IPI_mask(const cpumask_t *mask, int
> > > > vector)
> > > > * the system have been accounted for.
> > > > */
> > > > if ( system_state > SYS_STATE_smp_boot &&
> > > > - !unaccounted_cpus && !disabled_cpus &&
> > > > + !unaccounted_cpus && !disabled_cpus &&
> > > > !cpu_in_hotplug_context() &&
> > > > /* NB: get_cpu_maps lock requires enabled interrupts.
> > > > */
> > > > local_irq_is_enabled() && (cpus_locked =
> > > > get_cpu_maps()) &&
> > > > (park_offline_cpus ||
> > >
> > > Along with changing the condition you also want to update the
> > > comment to
> > > reflect the code adjustment.
> >
> > I've assumed the wording in the commet that says: "no CPU hotplug
> > or
> > unplug operations are taking place" would already cover the
> > addition
> > of the !cpu_in_hotplug_context() check.
>
> Hmm, yes, you're right. Just needs a release-ack then to go in (with
> the
> description adjustment).
Release-Acked-by: Oleksii Kurochko
~ Oleksii
Re: [PATCH for-4.19???] x86/physdev: replace physdev_{,un}map_pirq() checking against DOMID_SELF
On Wed, 2024-06-12 at 10:44 +0200, Jan Beulich wrote:
> It's hardly ever correct to check for just DOMID_SELF, as guests have
> ways to figure out their domain IDs and hence could instead use those
> as
> inputs to respective hypercalls. Note, however, that for ordinary
> DomU-s
> the adjustment is relaxing things rather than tightening them, since
> - as a result of XSA-237 - the respective XSM checks would have
> rejected
> self (un)mapping attempts for other than the control domain.
>
> Since in physdev_map_pirq() handling overall is a little easier this
> way, move obtaining of the domain pointer into the caller. Doing the
> same for physdev_unmap_pirq() is just to keep both consistent in this
> regard. For both this has the advantage that it is now provable (by
> the
> build not failing) that there are no DOMID_SELF checks left (and none
> could easily be re-added).
>
> Fixes: 0b469cd68708 ("Interrupt remapping to PIRQs in HVM guests")
> Fixes: 9e1a3415b773 ("x86: fixes after emuirq changes")
> Signed-off-by: Jan Beulich
Release-Acked-By: Oleksii Kurochko
~ Oleksii
> ---
> Note that the moving of rcu_lock_domain_by_any_id() is also going to
> help
> https://lists.xen.org/archives/html/xen-devel/2024-06/msg00206.html.
>
> --- a/xen/arch/x86/physdev.c
> +++ b/xen/arch/x86/physdev.c
> @@ -18,9 +18,9 @@
> #include
> #include
>
> -int physdev_map_pirq(domid_t domid, int type, int *index, int
> *pirq_p,
> +int physdev_map_pirq(struct domain *d, int type, int *index, int
> *pirq_p,
> struct msi_info *msi);
> -int physdev_unmap_pirq(domid_t domid, int pirq);
> +int physdev_unmap_pirq(struct domain *d, int pirq);
>
> #include "x86_64/mmconfig.h"
>
> @@ -88,13 +88,12 @@ static int physdev_hvm_map_pirq(
> return ret;
> }
>
> -int physdev_map_pirq(domid_t domid, int type, int *index, int
> *pirq_p,
> +int physdev_map_pirq(struct domain *d, int type, int *index, int
> *pirq_p,
> struct msi_info *msi)
> {
> - struct domain *d = current->domain;
> int ret;
>
> - if ( domid == DOMID_SELF && is_hvm_domain(d) && has_pirq(d) )
> + if ( d == current->domain && is_hvm_domain(d) && has_pirq(d) )
> {
> /*
> * Only makes sense for vector-based callback, else HVM-IRQ
> logic
> @@ -106,13 +105,9 @@ int physdev_map_pirq(domid_t domid, int
> return physdev_hvm_map_pirq(d, type, index, pirq_p);
> }
>
> - d = rcu_lock_domain_by_any_id(domid);
> - if ( d == NULL )
> - return -ESRCH;
> -
> ret = xsm_map_domain_pirq(XSM_DM_PRIV, d);
> if ( ret )
> - goto free_domain;
> + return ret;
>
> /* Verify or get irq. */
> switch ( type )
> @@ -135,24 +130,17 @@ int physdev_map_pirq(domid_t domid, int
> break;
> }
>
> - free_domain:
> - rcu_unlock_domain(d);
> return ret;
> }
>
> -int physdev_unmap_pirq(domid_t domid, int pirq)
> +int physdev_unmap_pirq(struct domain *d, int pirq)
> {
> - struct domain *d;
> int ret = 0;
>
> - d = rcu_lock_domain_by_any_id(domid);
> - if ( d == NULL )
> - return -ESRCH;
> -
> - if ( domid != DOMID_SELF || !is_hvm_domain(d) || !has_pirq(d) )
> + if ( d != current->domain || !is_hvm_domain(d) || !has_pirq(d) )
> ret = xsm_unmap_domain_pirq(XSM_DM_PRIV, d);
> if ( ret )
> - goto free_domain;
> + return ret;
>
> if ( is_hvm_domain(d) && has_pirq(d) )
> {
> @@ -160,8 +148,8 @@ int physdev_unmap_pirq(domid_t domid, in
> if ( domain_pirq_to_emuirq(d, pirq) != IRQ_UNBOUND )
> ret = unmap_domain_pirq_emuirq(d, pirq);
> write_unlock(>event_lock);
> - if ( domid == DOMID_SELF || ret )
> - goto free_domain;
> + if ( d == current->domain || ret )
> + return ret;
> }
>
> pcidevs_lock();
> @@ -170,8 +158,6 @@ int physdev_unmap_pirq(domid_t domid, in
> write_unlock(>event_lock);
> pcidevs_unlock();
>
> - free_domain:
> - rcu_unlock_domain(d);
> return ret;
> }
> #endif /* COMPAT */
> @@ -184,6 +170,8 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_H
>
> switch ( cmd )
> {
> + struct domain *d;
> +
> case PHYSDEVOP_eoi: {
> struct physdev_eoi eoi;
> struct pirq *pirq;
> @@ -331,8 +319,15 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_H
> msi.sbdf.devfn = map.devfn;
> msi.entry_nr = map.entry_nr;
> msi.table_base = map.table_base;
> - ret = physdev_map_pirq(map.domid, map.type, ,
> ,
> - );
> +
> + d = rcu_lock_domain_by_any_id(map.domid);
> + ret = -ESRCH;
> + if ( !d )
> + break;
> +
> + ret = physdev_map_pirq(d, map.type, , ,
> );
> +
> + rcu_unlock_domain(d);
>
> if ( map.type == MAP_PIRQ_TYPE_MULTI_MSI )
> map.entry_nr = msi.entry_nr;
> @@ -348,7 +343,15 @@ ret_t do_physdev_op(int cmd,
Re: [PATCH v12 5/8] xen/riscv: add minimal stuff to mm.h to build full Xen
On Tue, 2024-06-11 at 16:53 +0100, Andrew Cooper wrote:
> On 30/05/2024 7:22 pm, Oleksii K. wrote:
> > On Thu, 2024-05-30 at 18:23 +0100, Andrew Cooper wrote:
> > > On 29/05/2024 8:55 pm, Oleksii Kurochko wrote:
> > > > Signed-off-by: Oleksii Kurochko
> > > > Acked-by: Jan Beulich
> > > This patch looks like it can go in independently? Or does it
> > > depend
> > > on
> > > having bitops.h working in practice?
> > >
> > > However, one very strong suggestion...
> > >
> > >
> > > > diff --git a/xen/arch/riscv/include/asm/mm.h
> > > > b/xen/arch/riscv/include/asm/mm.h
> > > > index 07c7a0abba..cc4a07a71c 100644
> > > > --- a/xen/arch/riscv/include/asm/mm.h
> > > > +++ b/xen/arch/riscv/include/asm/mm.h
> > > > @@ -3,11 +3,246 @@
> > > >
> > > > +/* PDX of the first page in the frame table. */
> > > > +extern unsigned long frametable_base_pdx;
> > > > +
> > > > +/* Convert between machine frame numbers and page-info
> > > > structures.
> > > > */
> > > > +#define
> > > > mfn_to_page(mfn) \
> > > > + (frame_table + (mfn_to_pdx(mfn) - frametable_base_pdx))
> > > > +#define
> > > > page_to_mfn(pg) \
> > > > + pdx_to_mfn((unsigned long)((pg) - frame_table) +
> > > > frametable_base_pdx)
> > > Do yourself a favour and not introduce frametable_base_pdx to
> > > begin
> > > with.
> > >
> > > Every RISC-V board I can find has things starting from 0 in
> > > physical
> > > address space, with RAM starting immediately after.
> > I checked Linux kernel and grep there:
> > [ok@fedora linux-aia]$ grep -Rni "memory@" arch/riscv/boot/dts/
> > --
> > exclude "*.tmp" -I
> > arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-
> > 2.dtsi:33:
> > memory@4000 {
> > arch/riscv/boot/dts/starfive/jh7100-common.dtsi:28:
> > memory@8000
> > {
> > arch/riscv/boot/dts/microchip/mpfs-sev-kit.dts:49:
> > ddrc_cache:
> > memory@10 {
> > arch/riscv/boot/dts/microchip/mpfs-m100pfsevp.dts:33:
> > ddrc_cache_lo:
> > memory@8000 {
> > arch/riscv/boot/dts/microchip/mpfs-m100pfsevp.dts:37:
> > ddrc_cache_hi:
> > memory@104000 {
> > arch/riscv/boot/dts/microchip/mpfs-tysom-m.dts:34:
> > ddrc_cache_lo:
> > memory@8000 {
> > arch/riscv/boot/dts/microchip/mpfs-tysom-m.dts:40:
> > ddrc_cache_hi:
> > memory@10 {
> > arch/riscv/boot/dts/microchip/mpfs-polarberry.dts:22:
> > ddrc_cache_lo:
> > memory@8000 {
> > arch/riscv/boot/dts/microchip/mpfs-polarberry.dts:27:
> > ddrc_cache_hi:
> > memory@10 {
> > arch/riscv/boot/dts/microchip/mpfs-icicle-kit.dts:57:
> > ddrc_cache_lo:
> > memory@8000 {
> > arch/riscv/boot/dts/microchip/mpfs-icicle-kit.dts:63:
> > ddrc_cache_hi:
> > memory@104000 {
> > arch/riscv/boot/dts/thead/th1520-beaglev-ahead.dts:32: memory@0
> > {
> > arch/riscv/boot/dts/thead/th1520-lichee-module-4a.dtsi:14:
> > memory@0 {
> > arch/riscv/boot/dts/sophgo/cv1800b-milkv-duo.dts:26:
> > memory@8000
> > {
> > arch/riscv/boot/dts/sophgo/cv1812h.dtsi:12: memory@8000
> > {
> > arch/riscv/boot/dts/sifive/hifive-unmatched-a00.dts:26:
> > memory@8000
> > {
> > arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts:25:
> > memory@8000
> > {
> > arch/riscv/boot/dts/canaan/k210.dtsi:82: sram:
> > memory@8000 {
> >
> > And based on that majority of supported by Linux kernel boards has
> > RAM
> > starting not from 0 in physical address space. Am I confusing
> > something?
> >
> > > Taking the microchip board as an example, RAM actually starts at
> > > 0x800,
> > Today we had conversation with the guy from SiFive in xen-devel
> > channel
> > and he mentioned that they are using "starfive visionfive2 and
> > sifive
> > unleashed platforms." which based on the grep above has RAM not at
> > 0
> > address.
> >
> > Also, QEMU uses 0x800.
> >
> > > which means that having frametable_base_pdx and assuming it
&g
Re: [PATCH for-4.19] CI: Update FreeBSD to 13.3
On Tue, 2024-06-11 at 13:47 +0100, Andrew Cooper wrote: > Signed-off-by: Andrew Cooper Release-Acked-by: Oleksii Kurochko ~ Oleksii > --- > CC: Stefano Stabellini > CC: Michal Orzel > CC: Roger Pau Monné > CC: Oleksii Kurochko > CC: Jan Beulich > > Updated run: > https://cirrus-ci.com/task/4903594304995328 > > For 4.19, and for backporting to all trees including security trees. > FreeBSD-13.2 isn't available any more: > https://cirrus-ci.com/task/4554831417835520 > > causing build failures. > --- > .cirrus.yml | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/.cirrus.yml b/.cirrus.yml > index d0a9021a77e4..c431d8d2447d 100644 > --- a/.cirrus.yml > +++ b/.cirrus.yml > @@ -17,7 +17,7 @@ freebsd_template: _TEMPLATE > task: > name: 'FreeBSD 13' > freebsd_instance: > - image_family: freebsd-13-2 > + image_family: freebsd-13-3 > << : *FREEBSD_TEMPLATE > > task: > > base-commit: ea1cb444c28ce3ae7915d9c94c4344f4bf6d87d3
Re: [PATCH v4 0/7] Static shared memory followup v2 - pt2
On Tue, 2024-06-11 at 12:35 +, Luca Fancellu wrote: > + Oleksii > > > On 24 May 2024, at 13:40, Luca Fancellu > > wrote: > > > > This serie is a partial rework of this other serie: > > https://patchwork.kernel.org/project/xen-devel/cover/20231206090623.1932275-1-penny.zh...@arm.com/ > > > > The original serie is addressing an issue of the static shared > > memory feature > > that impacts the memory footprint of other component when the > > feature is > > enabled, another issue impacts the device tree generation for the > > guests when > > the feature is enabled and used and the last one is a missing > > feature that is > > the option to have a static shared memory region that is not from > > the host > > address space. > > > > This serie is handling some comment on the original serie and it is > > splitting > > the rework in two part, this first part is addressing the memory > > footprint issue > > and the device tree generation and currently is fully merged > > ( > > https://patchwork.kernel.org/project/xen-devel/cover/20240418073652.3622828-1-luca.fance...@arm.com > > /), > > this serie is addressing the static shared memory allocation from > > the Xen heap. > > > > Luca Fancellu (5): > > xen/arm: Lookup bootinfo shm bank during the mapping > > xen/arm: Wrap shared memory mapping code in one function > > xen/arm: Parse xen,shared-mem when host phys address is not > > provided > > xen/arm: Rework heap page allocation outside allocate_bank_memory > > xen/arm: Implement the logic for static shared memory from Xen > > heap > > > > Penny Zheng (2): > > xen/p2m: put reference for level 2 superpage > > xen/docs: Describe static shared memory when host address is not > > provided > > > > docs/misc/arm/device-tree/booting.txt | 52 ++- > > xen/arch/arm/arm32/mmu/mm.c | 11 +- > > xen/arch/arm/dom0less-build.c | 4 +- > > xen/arch/arm/domain_build.c | 84 +++-- > > xen/arch/arm/include/asm/domain_build.h | 9 +- > > xen/arch/arm/mmu/p2m.c | 82 +++-- > > xen/arch/arm/setup.c | 14 +- > > xen/arch/arm/static-shmem.c | 432 + > > --- > > 8 files changed, 502 insertions(+), 186 deletions(-) > > > > -- > > 2.34.1 > > > > > > Hi, > > We would like this serie to be in Xen 4.19, there was a > misunderstanding on our side because we thought > that since the serie was sent before the last posting date, it could > have been a candidate for merging in the > new release, instead after speaking with Julien and Oleksii we are > now aware that we need to provide a > justification for its presence. > > Pros to this serie is that we are closing the circle for static > shared memory, allowing it to use memory from > the host or from Xen, it is also a feature that is not enabled by > default, so it should not cause too much > disruption in case of any bugs that escaped the review, however we’ve > tested many configurations for that > with/without enabling the feature if that can be an additional value. > > Cons: we are touching some common code related to p2m, but also there > the impact should be minimal because > the new code is subject to l2 foreign mapping (to be confirmed maybe > from a p2m expert like Julien). > > The comments on patch 3 of this serie are addressed by this patch: > https://patchwork.kernel.org/project/xen-devel/patch/20240528125603.2467640-1-luca.fance...@arm.com/ > And the serie is fully reviewed. > > So our request is to allow this serie in 4.19, Oleksii, ARM > maintainers, do you agree on that? Considering that it is not enabled by default and affect on common code is minimal we should consider this patch series in 4.19: Release-Acked-by: Oleksii Kurochko ~ Oleksii
Re: [PATCH v12 5/8] xen/riscv: add minimal stuff to mm.h to build full Xen
On Thu, 2024-05-30 at 18:23 +0100, Andrew Cooper wrote:
> On 29/05/2024 8:55 pm, Oleksii Kurochko wrote:
> > Signed-off-by: Oleksii Kurochko
> > Acked-by: Jan Beulich
>
> This patch looks like it can go in independently? Or does it depend
> on
> having bitops.h working in practice?
>
> However, one very strong suggestion...
>
>
> > diff --git a/xen/arch/riscv/include/asm/mm.h
> > b/xen/arch/riscv/include/asm/mm.h
> > index 07c7a0abba..cc4a07a71c 100644
> > --- a/xen/arch/riscv/include/asm/mm.h
> > +++ b/xen/arch/riscv/include/asm/mm.h
> > @@ -3,11 +3,246 @@
> >
> > +/* PDX of the first page in the frame table. */
> > +extern unsigned long frametable_base_pdx;
> > +
> > +/* Convert between machine frame numbers and page-info structures.
> > */
> > +#define
> > mfn_to_page(mfn) \
> > + (frame_table + (mfn_to_pdx(mfn) - frametable_base_pdx))
> > +#define
> > page_to_mfn(pg) \
> > + pdx_to_mfn((unsigned long)((pg) - frame_table) +
> > frametable_base_pdx)
>
> Do yourself a favour and not introduce frametable_base_pdx to begin
> with.
To drop frametable_base_pdx if the following changes will be enough:
diff --git a/xen/arch/riscv/include/asm/mm.h
b/xen/arch/riscv/include/asm/mm.h
index cc4a07a71c..fdac7e0646 100644
--- a/xen/arch/riscv/include/asm/mm.h
+++ b/xen/arch/riscv/include/asm/mm.h
@@ -107,14 +107,11 @@ struct page_info
#define frame_table ((struct page_info *)FRAMETABLE_VIRT_START)
-/* PDX of the first page in the frame table. */
-extern unsigned long frametable_base_pdx;
-
/* Convert between machine frame numbers and page-info structures. */
#define mfn_to_page(mfn)\
-(frame_table + (mfn_to_pdx(mfn) - frametable_base_pdx))
+(frame_table + mfn - FRAMETABLE_BASE_OFFSET))
#define page_to_mfn(pg) \
-pdx_to_mfn((unsigned long)((pg) - frame_table) +
frametable_base_pdx)
+((unsigned long)((pg) - frame_table) + FRAMETABLE_BASE_OFFSET)
static inline void *page_to_virt(const struct page_info *pg)
{
diff --git a/xen/arch/riscv/mm.c b/xen/arch/riscv/mm.c
index 9c0fd80588..8f6dbdc699 100644
--- a/xen/arch/riscv/mm.c
+++ b/xen/arch/riscv/mm.c
@@ -15,7 +15,7 @@
#include
#include
-unsigned long __ro_after_init frametable_base_pdx;
unsigned long __ro_after_init frametable_virt_end;
struct mmu_desc {
>
> Every RISC-V board I can find has things starting from 0 in physical
> address space, with RAM starting immediately after.
>
> Taking the microchip board as an example, RAM actually starts at
> 0x800, which means that having frametable_base_pdx and assuming
> it
> does get set to 0x8000 (which isn't even a certainty, given that I
> think
> you'll need struct pages covering the PLICs), then what you are
> trading
> off is:
>
> * Saving 32k of virtual address space only (no need to even allocate
> memory for this range of the framtable), by
> * Having an extra memory load and add/sub in every page <-> mfn
> conversion, which is a screaming hotpath all over Xen.
Are you referring here to `mfn_to_pdx()` used in `mfn_to_page()` and
`pdx_to_mfn()` in `page_to_mfn()`?
My expectation was that when CONFIG_PDX_COMPRESSION is disabled then
this macros doesn't do anything:
/* pdx<->pfn == identity */
#define pdx_to_pfn(x) (x)
#define pfn_to_pdx(x) (x)
>
> It's a terribly short-sighted tradeoff.
>
> 32k of VA space might be worth saving in a 32bit build (I personally
> wouldn't - especially as there's no need to share Xen's VA space with
> guests, given no PV guests on ARM/RISC-V), but it's absolutely not at
> all in an a 64bit build with TB of VA space available.
Why 32k? If RAM is started at 0x8000_ then we have to cover 0x8
entries, and the size of it is 0x8_ = 524288 * 64 = 33554432, so it
is 32 Mb.
Am I confusing something?
P.S.: Should I map this start 32 mb? Or it is enough to have a slide (
FRAMETABLE_BASE_OFFSET ).
~ Oleksii
>
> Even if we do find a board with the first interesting thing in the
> frametable starting sufficiently away from 0 that it might be worth
> considering this slide, then it should still be Kconfig-able in a
> similar way to PDX_COMPRESSION.
>
> You don't want to be penalising everyone because of a
> theoretical/weird
> corner case.
>
> ~Andrew
Re: [PATCH for-4.19] x86/EPT: relax iPAT for "invalid" MFNs
On Mon, 2024-06-10 at 17:00 +0200, Jan Beulich wrote:
> On 10.06.2024 16:58, Jan Beulich wrote:
> > mfn_valid() is RAM-focused; it will often return false for MMIO.
> > Yet
> > access to actual MMIO space should not generally be restricted to
> > UC
> > only; especially video frame buffer accesses are unduly affected by
> > such
> > a restriction. Permit PAT use for directly assigned MMIO as long as
> > the
> > domain is known to have been granted some level of cache control.
> >
> > Signed-off-by: Jan Beulich
Release-Acked-by: Oleksii Kurochko
~ Oleksii
> > ---
> > Considering that we've just declared PVH Dom0 "supported", this may
> > well
> > qualify for 4.19. The issue was specifically very noticable there.
>
> Actually - meant to Cc Oleksii for this, and then forgot.
>
> Jan
>
> > The conditional may be more complex than really necessary, but it's
> > in
> > line with what we do elsewhere. And imo better continue to be a
> > little
> > too restrictive, than moving to too lax.
> >
> > --- a/xen/arch/x86/mm/p2m-ept.c
> > +++ b/xen/arch/x86/mm/p2m-ept.c
> > @@ -503,7 +503,8 @@ int epte_get_entry_emt(struct domain *d,
> >
> > if ( !mfn_valid(mfn) )
> > {
> > - *ipat = true;
> > + *ipat = type != p2m_mmio_direct ||
> > + (!is_iommu_enabled(d) &&
> > !cache_flush_permitted(d));
> > return X86_MT_UC;
> > }
> >
>
Re: [PATCH for-4.19 v1] automation: add a test for HVM domU on PVH dom0
On Mon, 2024-06-10 at 16:25 +0100, Andrew Cooper wrote: > On 10/06/2024 2:32 pm, Marek Marczykowski-Górecki wrote: > > This tests if QEMU works in PVH dom0. QEMU in dom0 requires > > enabling TUN > > in the kernel, so do that too. > > > > Add it to both x86 runners, similar to the PVH domU test. > > > > Signed-off-by: Marek Marczykowski-Górecki > > > > Acked-by: Andrew Cooper > > CC Oleksii. Release-Acked-By: Oleksii Kurochko ~ Oleksii > > > --- > > Requires rebuilding test-artifacts/kernel/6.1.19 > > Ok. > > But on a tangent, shouldn't that move forwards somewhat? > > > > > I'm actually not sure if there is a sense in testing HVM domU on > > both > > runners, when PVH domU variant is already tested on both. Are there > > any > > differences between Intel and AMD relevant for QEMU in dom0? > > It's not just Qemu, it's also HVMLoader, and the particulars of VT- > x/SVM > VMExit decode information in order to generate ioreqs. > > I'd firmly suggest having both. > > ~Andrew
Re: [XEN PATCH v6 0/7] FF-A notifications
Hi Bertrand and Julien, On Tue, 2024-06-11 at 07:09 +, Bertrand Marquis wrote: > Hi Julien and Oleksii, > > @Oleksii: Could we consider having this serie merged for next release > ? We can consider including it in Xen 4.19 as it has a low impact on existing systems and needs to be explicitly activated: Release-Acked-by: Oleksii Kurochko ~ Oleksii > > It is a feature that is in tech-preview at the moment and as such > should not have any > consequences on existing system unless it is activated explicitly in > the Xen configuration. > > There are some changes in the arm common code introducing support to > register SGI > interrupt handlers in drivers. As no drivers appart from FF-A is > trying to register such > handlers, the risk is minimal for existing systems. > > > > On 10 Jun 2024, at 22:38, Julien Grall wrote: > > > > Hi Bertrand, > > > > On 10/06/2024 16:54, Bertrand Marquis wrote: > > > Hi Jens, > > > > On 10 Jun 2024, at 08:53, Jens Wiklander > > > > wrote: > > > > > > > > Hi, > > > > > > > > This patch set adds support for FF-A notifications. We only > > > > support > > > > global notifications, per vCPU notifications remain > > > > unsupported. > > > > > > > > The first three patches are further cleanup and can be merged > > > > before the > > > > rest if desired. > > > > > > > > A physical SGI is used to make Xen aware of pending FF-A > > > > notifications. The > > > > physical SGI is selected by the SPMC in the secure world. Since > > > > it must not > > > > already be used by Xen the SPMC is in practice forced to donate > > > > one of the > > > > secure SGIs, but that's normally not a problem. The SGI > > > > handling in Xen is > > > > updated to support registration of handlers for SGIs that > > > > aren't statically > > > > assigned, that is, SGI IDs above GIC_SGI_MAX. > > > > > > > > The patch "xen/arm: add and call init_tee_secondary()" provides > > > > a hook for > > > > register the needed per-cpu interrupt handler in "xen/arm: ffa: > > > > support > > > > notification". > > > > > > > > The patch "xen/arm: add and call tee_free_domain_ctx()" > > > > provides a hook for > > > > later freeing of the TEE context. This hook is used in > > > > "xen/arm: ffa: > > > > support notification" and avoids the problem with TEE context > > > > being freed > > > > while we need to access it when handling a Schedule Receiver > > > > interrupt. It > > > > was suggested as an alternative in [1] that the TEE context > > > > could be freed > > > > from complete_domain_destroy(). > > > > > > > > [1] > > > > https://lore.kernel.org/all/cahua44h4ypoxyt7e6wnh5xjfpitzqjqp9ng4smty4ewhyn+...@mail.gmail.com/ > > > > > > > > Thanks, > > > > Jens > > > All patches are now reviewed and/or acked so I think they can get > > > in for the release. > > > > This would need a release-ack from Oleksii (I can't seem to find > > already one). > > You are right, i do not know why in my mind we already got one. > > > > > As we discussed last week, I am fine with the idea to merge the FFA > > patches as the feature is tech-preview. But there are some changes > > in the arm generic code. Do you (or Jens) have an assessment of the > > risk of the changes? > > Agree. > > In my view, the changes are changing the behaviour of some internal > functions if an interrupt handler is register for SGI but should not > have any impact for other kind of interrupts. > As there was nothing before the FF-A driver registering such > interrupts, the risk of the changes having any impact on existing > configurations not activating FF-A is fairly reduced. > > @Jens: do you agree with my analysis. > > I made a text for Oleksii at the beginning of the mail. > > Cheers > > Bertrand > > > > > Cheers, > > > > -- > > Julien Grall > >
Re: [PATCH v12 8/8] xen/README: add compiler and binutils versions for RISC-V64
On Thu, 2024-05-30 at 20:52 +0100, Andrew Cooper wrote:
> On 29/05/2024 8:55 pm, Oleksii Kurochko wrote:
> > diff --git a/README b/README
> > index c8a108449e..30da5ff9c0 100644
> > --- a/README
> > +++ b/README
> > @@ -48,6 +48,10 @@ provided by your OS distributor:
> > - For ARM 64-bit:
> > - GCC 5.1 or later
> > - GNU Binutils 2.24 or later
> > + - For RISC-V 64-bit:
> > + - GCC 12.2 or later
> > + - GNU Binutils 2.39 or later
>
> I would like to petition for GCC 10 and Binutils 2.35.
>
> These are the versions provided as cross-compilers by Debian, and
> therefore are the versions I would prefer to do smoke testing with.
We can consider that, but I prefer to make a separate patch series for
that.
~ Oleksii
>
> One issue is in cpu_relax(), needing this diff to fix:
>
> diff --git a/xen/arch/riscv/include/asm/processor.h
> b/xen/arch/riscv/include/asm/processor.h
> index 6846151717f7..830a05dd8e3a 100644
> --- a/xen/arch/riscv/include/asm/processor.h
> +++ b/xen/arch/riscv/include/asm/processor.h
> @@ -67,7 +67,7 @@ static inline void cpu_relax(void)
> __asm__ __volatile__ ( "pause" );
> #else
> /* Encoding of the pause instruction */
> - __asm__ __volatile__ ( ".insn 0x010F" );
> + __asm__ __volatile__ ( ".4byte 0x010F" );
> #endif
>
> barrier();
>
> The .insn directive appears to check that the byte pattern is a known
> extension, where .4byte doesn't. AFAICT, this makes .insn pretty
> useless for what I'd expect is it's main purpose...
>
>
> The other problem is a real issue in cmpxchg.h, already committed to
> staging (51dabd6312c).
>
> RISC-V does a conditional toolchain for the Zbb extension
> (xen/arch/riscv/rules.mk), but unconditionally uses the ANDN
> instruction
> in emulate_xchg_1_2().
>
> Nevertheless, this is also quite easy to fix:
>
> diff --git a/xen/arch/riscv/include/asm/cmpxchg.h
> b/xen/arch/riscv/include/asm/cmpxchg.h
> index d5e678c03678..12ecb0950701 100644
> --- a/xen/arch/riscv/include/asm/cmpxchg.h
> +++ b/xen/arch/riscv/include/asm/cmpxchg.h
> @@ -18,6 +18,20 @@
> : "r" (new) \
> : "memory" );
>
> +/*
> + * Binutils < 2.37 doesn't understand ANDN. If the toolchain is too
> old, form
> + * it of a NOT+AND pair
> + */
> +#ifdef __riscv_zbb
> +#define ANDN_INSN(rd, rs1, rs2) \
> + "andn " rd ", " rs1 ", " rs2 "\n"
> +#else
> +#define ANDN_INSN(rd, rs1, rs2) \
> + "not " rd ", " rs2 "\n" \
> + "and " rd ", " rs1 ", " rd "\n"
> +#endif
> +
> +
> /*
> * For LR and SC, the A extension requires that the address held in
> rs1 be
> * naturally aligned to the size of the operand (i.e., eight-byte
> aligned
> @@ -48,7 +62,7 @@
> \
> asm volatile ( \
> "0: lr.w" lr_sfx " %[old], %[ptr_]\n" \
> - " andn %[scratch], %[old], %[mask]\n" \
> + ANDN_INSN("%[scratch]", "%[old]", "%[mask]") \
> " or %[scratch], %[scratch], %z[new_]\n" \
> " sc.w" sc_sfx " %[scratch], %[scratch], %[ptr_]\n" \
> " bnez %[scratch], 0b\n" \
>
>
>
> And with that, everything builds... but doesn't link. I've got this:
>
> LDS arch/riscv/xen.lds
> riscv64-linux-gnu-ld -T arch/riscv/xen.lds -N prelink.o \
> ./common/symbols-dummy.o -o ./.xen-syms.0
> riscv64-linux-gnu-ld: prelink.o: in function
> `keyhandler_crash_action':
> /local/xen.git/xen/common/keyhandler.c:552: undefined reference to
> `guest_physmap_remove_page'
> riscv64-linux-gnu-ld: ./.xen-syms.0: hidden symbol
> `guest_physmap_remove_page' isn't defined
> riscv64-linux-gnu-ld: final link failed: bad value
>
> which is completely bizarre.
>
> keyhandler_crash_action() has no actual reference to
> guest_physmap_remove_page(), and keyhandler.o has no such relocation.
>
> I'm still investigating this one.
>
> ~Andrew
Re: [PATCH for-4.19] x86/pvh: declare PVH dom0 supported with caveats
On Fri, 2024-06-07 at 12:03 +0200, Roger Pau Monne wrote: > PVH dom0 is functionally very similar to PVH domU except for the > domain > builder and the added set of hypercalls available to it. > > The main concern with declaring it "Supported" is the lack of some > features > when compared to classic PV dom0, hence switch it's status to > supported with > caveats. List the known missing features, there might be more > features missing > or not working as expected apart from the ones listed. > > Note there's some (limited) PVH dom0 testing on both osstest and > gitlab. > > Signed-off-by: Roger Pau Monné Release-Acked-By: Oleksii Kurochko ~ Oleksii > --- > Hopefully this will attract more testing an resources to PVH dom0 in > order to > try to finish the missing features. > --- > CHANGELOG.md | 1 + > SUPPORT.md | 15 ++- > 2 files changed, 15 insertions(+), 1 deletion(-) > > diff --git a/CHANGELOG.md b/CHANGELOG.md > index 201478aa1c0e..1778419cae64 100644 > --- a/CHANGELOG.md > +++ b/CHANGELOG.md > @@ -14,6 +14,7 @@ The format is based on [Keep a > Changelog](https://keepachangelog.com/en/1.0.0/) > - HVM PIRQs are disabled by default. > - Reduce IOMMU setup time for hardware domain. > - Allow HVM/PVH domains to map foreign pages. > + - Declare PVH dom0 supported with caveats. > - xl/libxl configures vkb=[] for HVM domains with priority over > vkb_device. > - Increase the maximum number of CPUs Xen can be built for from > 4095 to > 16383. > diff --git a/SUPPORT.md b/SUPPORT.md > index d5d60c62ec11..711aacf34662 100644 > --- a/SUPPORT.md > +++ b/SUPPORT.md > @@ -161,7 +161,20 @@ Requires hardware virtualisation support (Intel > VMX / AMD SVM). > Dom0 support requires an IOMMU (Intel VT-d / AMD IOMMU). > > Status, domU: Supported > - Status, dom0: Experimental > + Status, dom0: Supported, with caveats > + > +PVH dom0 hasn't received the same test coverage as PV dom0, so it > can exhibit > +unexpected behavior or issues on some hardware. > + > +At least the following features are missing on a PVH dom0: > + > + * PCI SR-IOV and Resizable BARs. > + > + * Native NMI forwarding (nmi=dom0 command line option). > + > + * MCE handling. > + > + * PCI Passthrough to any kind of domUs. > > ### ARM >
Re: [PATCH] MAINTAINERS: alter EFI section
On Mon, 2024-06-10 at 08:38 +0200, Jan Beulich wrote: > To get past the recurring friction on the approach to take wrt > workarounds needed for various firmware flaws, I'm stepping down as > the > maintainer of our code interfacing with EFI firmware. Two new > maintainers are being introduced in my place. > > Signed-off-by: Jan Beulich Release-Acked-by: Oleksii Kurochko ~ Oleksii > --- > For the new maintainers, here's a 1st patch to consider right away: > https://lists.xen.org/archives/html/xen-devel/2024-03/msg00931.html. > > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -308,7 +308,9 @@ F:automation/eclair_analysis/ > F: automation/scripts/eclair > > EFI > -M: Jan Beulich > +M: Daniel P. Smith > +M: Marek Marczykowski-Górecki > +R: Jan Beulich > S: Supported > F: xen/arch/x86/efi/ > F: xen/arch/x86/include/asm/efi*.h
Re: [for-4.19] x86 emulator session outcome
On Thu, 2024-06-06 at 18:00 +0200, Jan Beulich wrote: > Oleksii, > > a decision of the session just finished was to deprecate support > for XeonPhi in 4.19, with the firm plan to remove support in 4.20. > This will want putting in the release notes. Thanks for notifing me. ~ Oleksii
Re: [XEN PATCH] automation/eclair_analysis: add more clean MISRA guidelines
On Tue, 2024-06-04 at 14:01 +0200, Nicola Vetrini wrote:
> On 2024-06-04 13:39, Oleksii K. wrote:
> > On Sat, 2024-06-01 at 21:13 +0200, Nicola Vetrini wrote:
> > > Rules 20.9, 20.12 and 14.4 are now clean on ARM and x86, so they
> > > are
> > > added
> > > to the list of clean guidelines.
> > >
> > > Some guidelines listed in the additional clean section for ARM
> > > are
> > > also
> > > clean on x86, so they can be removed from there.
> > >
> > > No functional change.
> > >
> > > Signed-off-by: Nicola Vetrini
> > > ---
> > > +Cc Oleksii for an opinion on the inclusion for 4.19
> > >
> > > This is a follow-up to series
> > > https://lore.kernel.org/xen-devel/cover.1717236930.git.nicola.vetr...@bugseng.com/
> > > and depends on it (otherwise the gitlab MISRA analysis would fail
> > > on
> > > violations of Rule 20.12).
> > > If it is decided that the dependent series should go in for 4.19,
> > > then
> > > my suggestion is to include this as well, to the gating on more
> > > guidelines.
> > > ---
> > I just want to clarify if I understand you correctly. Do you mean
> > taht
> > the current one patch will be merged without dependent series that
> > gitlab MISRA analysis would fail? IIUUC then I am not sure that we
> > have
> > an option to have this patch without the dependent patch series.
> >
>
> Exactly, that's why I specified the dependency. This patch should
> have
> been part of the series, but I forgot to include it.
I am okay to consider this patches in Xen 4.19, but only in case it the
dependencies will be resolved properly and necessary Acked will be
recieved.
~ Oleksii
>
> > ~ Oleksii
> > > automation/eclair_analysis/ECLAIR/tagging.ecl | 4 +++-
> > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/automation/eclair_analysis/ECLAIR/tagging.ecl
> > > b/automation/eclair_analysis/ECLAIR/tagging.ecl
> > > index a354ff322e03..b829655ca0bc 100644
> > > --- a/automation/eclair_analysis/ECLAIR/tagging.ecl
> > > +++ b/automation/eclair_analysis/ECLAIR/tagging.ecl
> > > @@ -60,6 +60,7 @@ MC3R1.R11.7||
> > > MC3R1.R11.9||
> > > MC3R1.R12.5||
> > > MC3R1.R14.1||
> > > +MC3R1.R14.4||
> > > MC3R1.R16.7||
> > > MC3R1.R17.1||
> > > MC3R1.R17.3||
> > > @@ -73,6 +74,7 @@ MC3R1.R20.4||
> > > MC3R1.R20.6||
> > > MC3R1.R20.9||
> > > MC3R1.R20.11||
> > > +MC3R1.R20.12||
> > > MC3R1.R20.13||
> > > MC3R1.R20.14||
> > > MC3R1.R21.3||
> > > @@ -105,7 +107,7 @@ if(string_equal(target,"x86_64"),
> > > )
> > >
> > > if(string_equal(target,"arm64"),
> > > -
> > > service_selector({"additional_clean_guidelines","MC3R1.R14.4||MC3
> > > R1.R
> > > 16.6||MC3R1.R20.12||MC3R1.R2.1||MC3R1.R5.3||MC3R1.R7.2||MC3R1.R7.
> > > 3||M
> > > C3R1.R8.6||MC3R1.R9.3"})
> > > +
> > > service_selector({"additional_clean_guidelines","MC3R1.R16.6||MC3
> > > R1.R
> > > 2.1||MC3R1.R5.3||MC3R1.R7.3"})
> > > )
> > >
> > > -
> > > reports+={clean:added,"service(clean_guidelines_common||additiona
> > > l_cl
> > > ean_guidelines)"}
>
Re: [XEN PATCH] automation/eclair_analysis: add more clean MISRA guidelines
On Sat, 2024-06-01 at 21:13 +0200, Nicola Vetrini wrote:
> Rules 20.9, 20.12 and 14.4 are now clean on ARM and x86, so they are
> added
> to the list of clean guidelines.
>
> Some guidelines listed in the additional clean section for ARM are
> also
> clean on x86, so they can be removed from there.
>
> No functional change.
>
> Signed-off-by: Nicola Vetrini
> ---
> +Cc Oleksii for an opinion on the inclusion for 4.19
>
> This is a follow-up to series
> https://lore.kernel.org/xen-devel/cover.1717236930.git.nicola.vetr...@bugseng.com/
> and depends on it (otherwise the gitlab MISRA analysis would fail on
> violations of Rule 20.12).
> If it is decided that the dependent series should go in for 4.19,
> then
> my suggestion is to include this as well, to the gating on more
> guidelines.
> ---
I just want to clarify if I understand you correctly. Do you mean taht
the current one patch will be merged without dependent series that
gitlab MISRA analysis would fail? IIUUC then I am not sure that we have
an option to have this patch without the dependent patch series.
~ Oleksii
> automation/eclair_analysis/ECLAIR/tagging.ecl | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/automation/eclair_analysis/ECLAIR/tagging.ecl
> b/automation/eclair_analysis/ECLAIR/tagging.ecl
> index a354ff322e03..b829655ca0bc 100644
> --- a/automation/eclair_analysis/ECLAIR/tagging.ecl
> +++ b/automation/eclair_analysis/ECLAIR/tagging.ecl
> @@ -60,6 +60,7 @@ MC3R1.R11.7||
> MC3R1.R11.9||
> MC3R1.R12.5||
> MC3R1.R14.1||
> +MC3R1.R14.4||
> MC3R1.R16.7||
> MC3R1.R17.1||
> MC3R1.R17.3||
> @@ -73,6 +74,7 @@ MC3R1.R20.4||
> MC3R1.R20.6||
> MC3R1.R20.9||
> MC3R1.R20.11||
> +MC3R1.R20.12||
> MC3R1.R20.13||
> MC3R1.R20.14||
> MC3R1.R21.3||
> @@ -105,7 +107,7 @@ if(string_equal(target,"x86_64"),
> )
>
> if(string_equal(target,"arm64"),
> -
> service_selector({"additional_clean_guidelines","MC3R1.R14.4||MC3R1.R
> 16.6||MC3R1.R20.12||MC3R1.R2.1||MC3R1.R5.3||MC3R1.R7.2||MC3R1.R7.3||M
> C3R1.R8.6||MC3R1.R9.3"})
> +
> service_selector({"additional_clean_guidelines","MC3R1.R16.6||MC3R1.R
> 2.1||MC3R1.R5.3||MC3R1.R7.3"})
> )
>
> -
> reports+={clean:added,"service(clean_guidelines_common||additional_cl
> ean_guidelines)"}
Re: [PATCH for-4.19 0/3] CI: Misc improvements
On Wed, 2024-05-29 at 15:19 +0100, Andrew Cooper wrote: > All found while making extensive use of Gitlab CI for the bitops boot > testing. > > For 4.19. It's all very low risk, and improves the > utility/useability of our > testing. Release-Acked-by: Oleksii Kurochko ~ Oleksii > > Andrew Cooper (3): > CI: Remove CI_COMMIT_REF_PROTECTED requirement for HW jobs > CI: Use a debug build of Xen for the Xilinx HW tests > CI: Improve serial handling in qemu-smoke-ppc64le.sh > > automation/gitlab-ci/test.yaml | 8 > automation/scripts/qemu-smoke-ppc64le.sh | 13 +++-- > 2 files changed, 11 insertions(+), 10 deletions(-) >
Re: [PATCH 2/2] arch/irq: Centralise no_irq_type
On Thu, 2024-05-30 at 19:40 +0100, Andrew Cooper wrote:
> Having no_irq_type defined per arch, but using common callbacks is a
> mess, and
> particualrly hard to bootstrap a new architecture with.
>
> Now that the ack()/end() hooks have been exported suitably, move the
> definition of no_irq_type into common/irq.c, and into .rodata for
> good
> measure.
>
> No functional change, but a whole lot less tangled.
>
> Signed-off-by: Andrew Cooper
LGTM: Reviewed-by: Oleksii Kurochko
> ---
> CC: Jan Beulich
> CC: Roger Pau Monné
> CC: Stefano Stabellini
> CC: Julien Grall
> CC: Volodymyr Babchuk
> CC: Bertrand Marquis
> CC: Oleksii Kurochko
> CC: Shawn Anastasio
>
> Oleksii: For RISC-V, you should only need to provide a irq_ack_none()
> stub now.
Sure, I will update my patch series during rebase.
~ Oleksii
> ---
> xen/arch/arm/irq.c | 10 --
> xen/arch/ppc/stubs.c | 9 -
> xen/arch/x86/irq.c | 9 -
> xen/common/irq.c | 13 +
> xen/include/xen/irq.h | 2 +-
> 5 files changed, 14 insertions(+), 29 deletions(-)
>
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index 7138f9e7c283..e5fb26a3de2d 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -45,16 +45,6 @@ void irq_end_none(struct irq_desc *irq)
> gic_hw_ops->gic_host_irq_type->end(irq);
> }
>
> -hw_irq_controller no_irq_type = {
> - .typename = "none",
> - .startup = irq_startup_none,
> - .shutdown = irq_shutdown_none,
> - .enable = irq_enable_none,
> - .disable = irq_disable_none,
> - .ack = irq_ack_none,
> - .end = irq_end_none
> -};
> -
> static irq_desc_t irq_desc[NR_IRQS];
> static DEFINE_PER_CPU(irq_desc_t[NR_LOCAL_IRQS], local_irq_desc);
>
> diff --git a/xen/arch/ppc/stubs.c b/xen/arch/ppc/stubs.c
> index 4e03428e071a..923f0e7b2095 100644
> --- a/xen/arch/ppc/stubs.c
> +++ b/xen/arch/ppc/stubs.c
> @@ -139,15 +139,6 @@ void irq_ack_none(struct irq_desc *desc)
> BUG_ON("unimplemented");
> }
>
> -hw_irq_controller no_irq_type = {
> - .typename = "none",
> - .startup = irq_startup_none,
> - .shutdown = irq_shutdown_none,
> - .enable = irq_enable_none,
> - .disable = irq_disable_none,
> - .ack = irq_ack_none,
> -};
> -
> int arch_init_one_irq_desc(struct irq_desc *desc)
> {
> BUG_ON("unimplemented");
> diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
> index cfd7a08479d2..e36e06deaa68 100644
> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -472,15 +472,6 @@ void cf_check irq_ack_none(struct irq_desc
> *desc)
> ack_bad_irq(desc->irq);
> }
>
> -hw_irq_controller no_irq_type = {
> - "none",
> - irq_startup_none,
> - irq_shutdown_none,
> - irq_enable_none,
> - irq_disable_none,
> - irq_ack_none,
> -};
> -
> static vmask_t *irq_get_used_vector_mask(int irq)
> {
> vmask_t *ret = NULL;
> diff --git a/xen/common/irq.c b/xen/common/irq.c
> index 7225b4637486..29729349a6f2 100644
> --- a/xen/common/irq.c
> +++ b/xen/common/irq.c
> @@ -3,6 +3,19 @@
>
> DEFINE_PER_CPU(const struct cpu_user_regs *, irq_regs);
>
> +const hw_irq_controller no_irq_type = {
> + .typename = "none",
> + .startup = irq_startup_none,
> + .shutdown = irq_shutdown_none,
> + .enable = irq_enable_none,
> + .disable = irq_disable_none,
> + .ack = irq_ack_none,
> +
> +#ifdef irq_end_none /* Hook is optional per arch */
> + .end = irq_end_none,
> +#endif
> +};
> +
> int init_one_irq_desc(struct irq_desc *desc)
> {
> int err;
> diff --git a/xen/include/xen/irq.h b/xen/include/xen/irq.h
> index b71f65db8621..327cd2217c7e 100644
> --- a/xen/include/xen/irq.h
> +++ b/xen/include/xen/irq.h
> @@ -122,7 +122,7 @@ extern int request_irq(unsigned int irq, unsigned
> int irqflags,
> void (*handler)(int irq, void *dev_id),
> const char *devname, void *dev_id);
>
> -extern hw_irq_controller no_irq_type;
> +extern const hw_irq_controller no_irq_type;
> void cf_check no_action(int cpl, void *dev_id);
> unsigned int cf_check irq_startup_none(struct irq_desc *desc);
> void cf_check irq_actor_none(struct irq_desc *desc);
Re: [PATCH 1/2] arch/irq: Make irq_ack_none() mandatory
On Thu, 2024-05-30 at 19:40 +0100, Andrew Cooper wrote:
> Any non-stub implementation of these is going to have to do something
> here.
>
> irq_end_none() is more complicated and has arch-specific interactions
> with
> irq_ack_none(), so make it optional.
>
> For PPC, introduce a stub irq_ack_none().
>
> For ARM and x86, export the existing {ack,end}_none() helpers,
> gaining an irq_
> prefix for consisntency with everything else in no_irq_type.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper
LGTM: Reviewed-by: Oleksii Kurochko
~ Oleksii
> ---
> CC: Jan Beulich
> CC: Roger Pau Monné
> CC: Stefano Stabellini
> CC: Julien Grall
> CC: Volodymyr Babchuk
> CC: Bertrand Marquis
> CC: Oleksii Kurochko
> CC: Shawn Anastasio
> ---
> xen/arch/arm/include/asm/irq.h | 3 +++
> xen/arch/arm/irq.c | 8
> xen/arch/ppc/stubs.c | 6 ++
> xen/arch/x86/irq.c | 4 ++--
> xen/include/xen/irq.h | 6 ++
> 5 files changed, 21 insertions(+), 6 deletions(-)
>
> diff --git a/xen/arch/arm/include/asm/irq.h
> b/xen/arch/arm/include/asm/irq.h
> index 1bae5388878e..ec437add0971 100644
> --- a/xen/arch/arm/include/asm/irq.h
> +++ b/xen/arch/arm/include/asm/irq.h
> @@ -98,6 +98,9 @@ void irq_set_affinity(struct irq_desc *desc, const
> cpumask_t *mask);
> */
> bool irq_type_set_by_domain(const struct domain *d);
>
> +void irq_end_none(struct irq_desc *irq);
> +#define irq_end_none irq_end_none
> +
> #endif /* _ASM_HW_IRQ_H */
> /*
> * Local variables:
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index bcce80a4d624..7138f9e7c283 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -31,12 +31,12 @@ struct irq_guest
> unsigned int virq;
> };
>
> -static void ack_none(struct irq_desc *irq)
> +void irq_ack_none(struct irq_desc *irq)
> {
> printk("unexpected IRQ trap at irq %02x\n", irq->irq);
> }
>
> -static void end_none(struct irq_desc *irq)
> +void irq_end_none(struct irq_desc *irq)
> {
> /*
> * Still allow a CPU to end an interrupt if we receive a
> spurious
> @@ -51,8 +51,8 @@ hw_irq_controller no_irq_type = {
> .shutdown = irq_shutdown_none,
> .enable = irq_enable_none,
> .disable = irq_disable_none,
> - .ack = ack_none,
> - .end = end_none
> + .ack = irq_ack_none,
> + .end = irq_end_none
> };
>
> static irq_desc_t irq_desc[NR_IRQS];
> diff --git a/xen/arch/ppc/stubs.c b/xen/arch/ppc/stubs.c
> index da193839bd09..4e03428e071a 100644
> --- a/xen/arch/ppc/stubs.c
> +++ b/xen/arch/ppc/stubs.c
> @@ -134,12 +134,18 @@ void pirq_set_affinity(struct domain *d, int
> pirq, const cpumask_t *mask)
> BUG_ON("unimplemented");
> }
>
> +void irq_ack_none(struct irq_desc *desc)
> +{
> + BUG_ON("unimplemented");
> +}
> +
> hw_irq_controller no_irq_type = {
> .typename = "none",
> .startup = irq_startup_none,
> .shutdown = irq_shutdown_none,
> .enable = irq_enable_none,
> .disable = irq_disable_none,
> + .ack = irq_ack_none,
> };
>
> int arch_init_one_irq_desc(struct irq_desc *desc)
> diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
> index c16205a9beb6..cfd7a08479d2 100644
> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -467,7 +467,7 @@ int __init init_irq_data(void)
> return 0;
> }
>
> -static void cf_check ack_none(struct irq_desc *desc)
> +void cf_check irq_ack_none(struct irq_desc *desc)
> {
> ack_bad_irq(desc->irq);
> }
> @@ -478,7 +478,7 @@ hw_irq_controller no_irq_type = {
> irq_shutdown_none,
> irq_enable_none,
> irq_disable_none,
> - ack_none,
> + irq_ack_none,
> };
>
> static vmask_t *irq_get_used_vector_mask(int irq)
> diff --git a/xen/include/xen/irq.h b/xen/include/xen/irq.h
> index 89f7a8317a87..b71f65db8621 100644
> --- a/xen/include/xen/irq.h
> +++ b/xen/include/xen/irq.h
> @@ -130,6 +130,12 @@ void cf_check irq_actor_none(struct irq_desc
> *desc);
> #define irq_disable_none irq_actor_none
> #define irq_enable_none irq_actor_none
>
> +/*
> + * irq_ack_none() must be provided by the architecture.
> + * irq_end_none() is optional, and opted into using a define.
> + */
> +void irq_ack_none(struct irq_desc *irq);
> +
> /*
> * Per-cpu interrupted context register state - the inner-most
> interrupt frame
> * on the stack.
Re: [PATCH for-4.19 0/2] arch/irq: Untangle no_irq_type
On Thu, 2024-05-30 at 19:40 +0100, Andrew Cooper wrote: > Found when reviewing Oleksii's series to enable the RISC-V build. > > The way no_irq_type works is horrifying. Make it less-so. > > Andrew Cooper (2): > arch/irq: Make irq_ack_none() mandatory > arch/irq: Centralise no_irq_type > > xen/arch/arm/include/asm/irq.h | 3 +++ > xen/arch/arm/irq.c | 14 ++ > xen/arch/ppc/stubs.c | 11 --- > xen/arch/x86/irq.c | 11 +-- > xen/common/irq.c | 13 + > xen/include/xen/irq.h | 8 +++- > 6 files changed, 30 insertions(+), 30 deletions(-) > > > base-commit: 9a905d7dc65883af082532b4dc91ce0131e54047 I am okay to have it in release if the necessary acks will be recieved. Release-Acked-by: Oleksii Kurochko ~ Oleksii
Re: [PATCH v12 7/8] xen/riscv: enable full Xen build
On Thu, 2024-05-30 at 18:45 +0100, Andrew Cooper wrote:
> On 30/05/2024 6:12 pm, Oleksii K. wrote:
> > On Thu, 2024-05-30 at 17:48 +0100, Andrew Cooper wrote:
> > > On 29/05/2024 8:55 pm, Oleksii Kurochko wrote:
> > > > diff --git a/xen/arch/riscv/stubs.c b/xen/arch/riscv/stubs.c
> > > > index 8285bcffef..bda35fc347 100644
> > > > --- a/xen/arch/riscv/stubs.c
> > > > +++ b/xen/arch/riscv/stubs.c
> > > > @@ -24,12 +24,6 @@ DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t,
> > > > cpu_core_mask);
> > > >
> > > > nodemask_t __read_mostly node_online_map = { { [0] = 1UL } };
> > > >
> > > > -/*
> > > > - * max_page is defined in page_alloc.c which isn't complied
> > > > for
> > > > now.
> > > > - * definition of max_page will be remove as soon as page_alloc
> > > > is
> > > > built.
> > > > - */
> > > > -unsigned long __read_mostly max_page;
> > > > -
> > > > /* time.c */
> > > >
> > > > unsigned long __ro_after_init cpu_khz; /* CPU clock frequency
> > > > in
> > > > kHz. */
> > > > @@ -419,21 +413,3 @@ void __cpu_die(unsigned int cpu)
> > > > {
> > > > BUG_ON("unimplemented");
> > > > }
> > > > -
> > > > -/*
> > > > - * The following functions are defined in common/irq.c, but
> > > > common/irq.c isn't
> > > > - * built for now. These changes will be removed there when
> > > > common/irq.c is
> > > > - * ready.
> > > > - */
> > > > -
> > > > -void cf_check irq_actor_none(struct irq_desc *desc)
> > > > -{
> > > > - BUG_ON("unimplemented");
> > > > -}
> > > > -
> > > > -unsigned int cf_check irq_startup_none(struct irq_desc *desc)
> > > > -{
> > > > - BUG_ON("unimplemented");
> > > > -
> > > > - return 0;
> > > > -}
> > > All 3 of these are introduced in the previous patch and deleted
> > > again
> > > here. Looks like a rebasing accident.
> > Not really.
> >
> > This was done to avoid build failures for RISC-V. If the HEAD is on
> > the
> > previous patch where these changes are introduced and then we just
> > drop
> > them, it will lead to a linkage error because these functions are
> > defined in common/irq.c, which isn't built for RISC-V if the HEAD
> > is on
> > the previous patch:
> > /build/xen/arch/riscv/entry.S:86: undefined reference to
> > `max_page'
>
> Nothing in the series touches entry.S, so I'm not sure what this is
> complaining about.
>
> The stub for get_upper_mfn_bound() references max_page, but it's only
> used in a SYSCTL so you can avoid the problem with a BUG_ON().
I didn't get how I can use BUG_ON() with declaration of variable to
avoid the compilation issue the undefined reference to max_page?
>
> BTW, you also don't need a return after a BUG_ON().
> __builtin_unreachable() takes care of everything properly for you.
>
>
> > riscv64-linux-gnu-ld: prelink.o:(.rodata+0x8): undefined
> > reference to
> > `irq_startup_none'
> > riscv64-linux-gnu-ld: prelink.o:(.rodata+0x10): undefined
> > reference to
> > `irq_actor_none'
> > riscv64-linux-gnu-ld: prelink.o:(.rodata+0x18): undefined
> > reference to
> > `irq_actor_none'
> > riscv64-linux-gnu-ld: prelink.o:(.rodata+0x20): undefined
> > reference to
> > `irq_actor_none'
> > riscv64-linux-gnu-ld: xen-syms: hidden symbol `irq_actor_none'
> > isn't
> > defined
> >
> > That is why these stubs were introduced in the previous patch
> > (because
> > common/irq.c isn't built at that moment) and are removed in this
> > patch
> > (since at the moment of this patch, common/irq.c is now being
> > built).
>
> These OTOH are a side effect of how no_irq_type works, which is
> horrifying, and not something I can unsee.
>
> I'll see about fixing it, because I really can't bare to leave it
> like
> this...
I am really sorry, but I don't understand should I deal with this
somehow now or the provided changes are okay?
~ Oleksii
Re: [PATCH v12 5/8] xen/riscv: add minimal stuff to mm.h to build full Xen
On Thu, 2024-05-30 at 18:23 +0100, Andrew Cooper wrote:
> On 29/05/2024 8:55 pm, Oleksii Kurochko wrote:
> > Signed-off-by: Oleksii Kurochko
> > Acked-by: Jan Beulich
>
> This patch looks like it can go in independently? Or does it depend
> on
> having bitops.h working in practice?
>
> However, one very strong suggestion...
>
>
> > diff --git a/xen/arch/riscv/include/asm/mm.h
> > b/xen/arch/riscv/include/asm/mm.h
> > index 07c7a0abba..cc4a07a71c 100644
> > --- a/xen/arch/riscv/include/asm/mm.h
> > +++ b/xen/arch/riscv/include/asm/mm.h
> > @@ -3,11 +3,246 @@
> >
> > +/* PDX of the first page in the frame table. */
> > +extern unsigned long frametable_base_pdx;
> > +
> > +/* Convert between machine frame numbers and page-info structures.
> > */
> > +#define
> > mfn_to_page(mfn) \
> > + (frame_table + (mfn_to_pdx(mfn) - frametable_base_pdx))
> > +#define
> > page_to_mfn(pg) \
> > + pdx_to_mfn((unsigned long)((pg) - frame_table) +
> > frametable_base_pdx)
>
> Do yourself a favour and not introduce frametable_base_pdx to begin
> with.
>
> Every RISC-V board I can find has things starting from 0 in physical
> address space, with RAM starting immediately after.
I checked Linux kernel and grep there:
[ok@fedora linux-aia]$ grep -Rni "memory@" arch/riscv/boot/dts/ --
exclude "*.tmp" -I
arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi:33:
memory@4000 {
arch/riscv/boot/dts/starfive/jh7100-common.dtsi:28: memory@8000
{
arch/riscv/boot/dts/microchip/mpfs-sev-kit.dts:49: ddrc_cache:
memory@10 {
arch/riscv/boot/dts/microchip/mpfs-m100pfsevp.dts:33: ddrc_cache_lo:
memory@8000 {
arch/riscv/boot/dts/microchip/mpfs-m100pfsevp.dts:37: ddrc_cache_hi:
memory@104000 {
arch/riscv/boot/dts/microchip/mpfs-tysom-m.dts:34: ddrc_cache_lo:
memory@8000 {
arch/riscv/boot/dts/microchip/mpfs-tysom-m.dts:40: ddrc_cache_hi:
memory@10 {
arch/riscv/boot/dts/microchip/mpfs-polarberry.dts:22: ddrc_cache_lo:
memory@8000 {
arch/riscv/boot/dts/microchip/mpfs-polarberry.dts:27: ddrc_cache_hi:
memory@10 {
arch/riscv/boot/dts/microchip/mpfs-icicle-kit.dts:57: ddrc_cache_lo:
memory@8000 {
arch/riscv/boot/dts/microchip/mpfs-icicle-kit.dts:63: ddrc_cache_hi:
memory@104000 {
arch/riscv/boot/dts/thead/th1520-beaglev-ahead.dts:32: memory@0 {
arch/riscv/boot/dts/thead/th1520-lichee-module-4a.dtsi:14:
memory@0 {
arch/riscv/boot/dts/sophgo/cv1800b-milkv-duo.dts:26:memory@8000
{
arch/riscv/boot/dts/sophgo/cv1812h.dtsi:12: memory@8000 {
arch/riscv/boot/dts/sifive/hifive-unmatched-a00.dts:26: memory@8000
{
arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts:25: memory@8000
{
arch/riscv/boot/dts/canaan/k210.dtsi:82:sram: memory@8000 {
And based on that majority of supported by Linux kernel boards has RAM
starting not from 0 in physical address space. Am I confusing
something?
>
> Taking the microchip board as an example, RAM actually starts at
> 0x800,
Today we had conversation with the guy from SiFive in xen-devel channel
and he mentioned that they are using "starfive visionfive2 and sifive
unleashed platforms." which based on the grep above has RAM not at 0
address.
Also, QEMU uses 0x800.
> which means that having frametable_base_pdx and assuming it
> does get set to 0x8000 (which isn't even a certainty, given that I
> think
> you'll need struct pages covering the PLICs), then what you are
> trading
> off is:
>
> * Saving 32k of virtual address space only (no need to even allocate
> memory for this range of the framtable), by
> * Having an extra memory load and add/sub in every page <-> mfn
> conversion, which is a screaming hotpath all over Xen.
>
> It's a terribly short-sighted tradeoff.
>
> 32k of VA space might be worth saving in a 32bit build (I personally
> wouldn't - especially as there's no need to share Xen's VA space with
> guests, given no PV guests on ARM/RISC-V), but it's absolutely not at
> all in an a 64bit build with TB of VA space available.
>
> Even if we do find a board with the first interesting thing in the
> frametable starting sufficiently away from 0 that it might be worth
> considering this slide, then it should still be Kconfig-able in a
> similar way to PDX_COMPRESSION.
I found your tradeoffs reasonable, but I don't understand how it will
work if RAM does not start from 0, as the frametable address and RAM
address are linked.
I tried to look at the PDX_COMPRESSION config and couldn't find any
"slide" there. Could you please clarify this for me?
If to use this "slide" would it help to avoid the mentioned above
tradeoffs?
One more question: if we decide to go without frametable_base_pdx,
would it be sufficient to simply remove mentions of it from the code (
at
Re: [PATCH v12 8/8] xen/README: add compiler and binutils versions for RISC-V64
On Thu, 2024-05-30 at 17:47 +0100, Andrew Cooper wrote: > On 29/05/2024 8:55 pm, Oleksii Kurochko wrote: > > diff --git a/README b/README > > index c8a108449e..30da5ff9c0 100644 > > --- a/README > > +++ b/README > > @@ -48,6 +48,10 @@ provided by your OS distributor: > > - For ARM 64-bit: > > - GCC 5.1 or later > > - GNU Binutils 2.24 or later > > + - For RISC-V 64-bit: > > + - GCC 12.2 or later > > + - GNU Binutils 2.39 or later > > + Older GCC and GNU Binutils would work, but this is not a > > guarantee. > > This sentence isn't appropriate to live here. > > The commit message saying "this is what we run in CI" is perfectly > good > enough. > > With this dropped, Reviewed-by: Andrew Cooper > . Can fix on commt. I am okay with dropping this sentence, but someone ( unfortunately I don't remember who Jan? Julien? ) requested it, and I think it would be nice to hear their opinion before doing so. ~ Oleksii
Re: [PATCH v12 7/8] xen/riscv: enable full Xen build
On Thu, 2024-05-30 at 17:48 +0100, Andrew Cooper wrote:
> On 29/05/2024 8:55 pm, Oleksii Kurochko wrote:
> > diff --git a/xen/arch/riscv/stubs.c b/xen/arch/riscv/stubs.c
> > index 8285bcffef..bda35fc347 100644
> > --- a/xen/arch/riscv/stubs.c
> > +++ b/xen/arch/riscv/stubs.c
> > @@ -24,12 +24,6 @@ DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t,
> > cpu_core_mask);
> >
> > nodemask_t __read_mostly node_online_map = { { [0] = 1UL } };
> >
> > -/*
> > - * max_page is defined in page_alloc.c which isn't complied for
> > now.
> > - * definition of max_page will be remove as soon as page_alloc is
> > built.
> > - */
> > -unsigned long __read_mostly max_page;
> > -
> > /* time.c */
> >
> > unsigned long __ro_after_init cpu_khz; /* CPU clock frequency in
> > kHz. */
> > @@ -419,21 +413,3 @@ void __cpu_die(unsigned int cpu)
> > {
> > BUG_ON("unimplemented");
> > }
> > -
> > -/*
> > - * The following functions are defined in common/irq.c, but
> > common/irq.c isn't
> > - * built for now. These changes will be removed there when
> > common/irq.c is
> > - * ready.
> > - */
> > -
> > -void cf_check irq_actor_none(struct irq_desc *desc)
> > -{
> > - BUG_ON("unimplemented");
> > -}
> > -
> > -unsigned int cf_check irq_startup_none(struct irq_desc *desc)
> > -{
> > - BUG_ON("unimplemented");
> > -
> > - return 0;
> > -}
>
> All 3 of these are introduced in the previous patch and deleted again
> here. Looks like a rebasing accident.
Not really.
This was done to avoid build failures for RISC-V. If the HEAD is on the
previous patch where these changes are introduced and then we just drop
them, it will lead to a linkage error because these functions are
defined in common/irq.c, which isn't built for RISC-V if the HEAD is on
the previous patch:
/build/xen/arch/riscv/entry.S:86: undefined reference to `max_page'
riscv64-linux-gnu-ld: prelink.o:(.rodata+0x8): undefined reference to
`irq_startup_none'
riscv64-linux-gnu-ld: prelink.o:(.rodata+0x10): undefined reference to
`irq_actor_none'
riscv64-linux-gnu-ld: prelink.o:(.rodata+0x18): undefined reference to
`irq_actor_none'
riscv64-linux-gnu-ld: prelink.o:(.rodata+0x20): undefined reference to
`irq_actor_none'
riscv64-linux-gnu-ld: xen-syms: hidden symbol `irq_actor_none' isn't
defined
That is why these stubs were introduced in the previous patch (because
common/irq.c isn't built at that moment) and are removed in this patch
(since at the moment of this patch, common/irq.c is now being built).
~ Oleksii
>
> (This patch has to be the final one touching build related things, so
> I
> can't simply fix it on commit)
>
> ~Andrew
Re: [PATCH] x86/hvm: allow XENMEM_machine_memory_map
On Thu, 2024-05-30 at 10:14 +0200, Roger Pau Monné wrote: > On Thu, May 30, 2024 at 09:04:08AM +0100, Andrew Cooper wrote: > > On 30/05/2024 8:53 am, Roger Pau Monne wrote: > > > For HVM based control domains XENMEM_machine_memory_map must be > > > available so > > > that the `e820_host` xl.cfg option can be used. > > > > > > Signed-off-by: Roger Pau Monné > > > > Seems safe enough to allow. > > > > Does this want a reported-by, or some further discussion about how > > it > > was found? > > I've found it while attempting to repro an issue with e820_host > reported by Marek, but the issue he reported is not related to this. > It's just that I have most of my test systems set as PVH dom0. > > > Also, as it's mostly PVH Dom0 bugfixing, shouldn't we want it in > > 4.19? > > Yeah, forgot to add the for-4.19 line and Oleksii, adding him now for > consideration for 4.19. Release-Acked-by: Oleksii Kurochko ~ Oleksii
Re: [PATCH v12 1/8] xen/riscv: disable unnecessary configs
On Thu, 2024-05-30 at 17:44 +0100, Andrew Cooper wrote: > > The subject should say "update Kconfig", because you're not (only) > disabling. > > I'd suggest "xen/riscv: Update Kconfig in preparation for a full Xen > build". > > On 29/05/2024 8:55 pm, Oleksii Kurochko wrote: > > Disables unnecessary configs for two cases: > > 1. By utilizing EXTRA_FIXED_RANDCONFIG for randconfig builds > > (GitLab CI jobs). > > 2. By using tiny64_defconfig for non-randconfig builds. > > > > Only configs which lead to compilation issues were disabled. > > > > Remove lines related to disablement of configs which aren't > > affected > > compilation: > > -# CONFIG_SCHED_CREDIT is not set > > -# CONFIG_SCHED_RTDS is not set > > -# CONFIG_SCHED_NULL is not set > > -# CONFIG_SCHED_ARINC653 is not set > > -# CONFIG_TRACEBUFFER is not set > > -# CONFIG_HYPFS is not set > > -# CONFIG_SPECULATIVE_HARDEN_ARRAY is not set > > > > To allow CONFIG_ARGO build happy it was included to > > > > as ARGO requires p2m_type_t ( p2m_ram_rw ) and declaration of > > check_get_page_from_gfn() from xen/p2m-common.h. > > > > Also, it was included to asm/p2m.h as after the > > latter was > > included to the compilation error that EINVAL, > > EOPNOTSUPP > > aren't declared started to occur. > > > > CONFIG_XSM=n as it requires an introduction of: > > * boot_module_find_by_kind() > > * BOOTMOD_XSM > > * struct bootmodule > > * copy_from_paddr() > > The mentioned things aren't introduced now. > > > > CPU_BOOT_TIME_CPUPOOLS requires an introduction of > > cpu_physical_id() and > > acpi_disabled, so it is disabled for now. > > CONFIG_BOOT_TIME_CPUPOOLS > > Also the "depends on DT" isn't good enough as a restriction IMO. > It's > very ARM-dom0less specific. > > > PERF_COUNTERS requires asm/perf.h and asm/perfc-defn.h, so it is > > also disabled for now, as RISC-V hasn't introduced this headers > > yet. > > LIVEPATCH isn't ready for RISC-V too and it can be overriden by > > randconfig, > > so to avoid compilation errors for randconfig it is disabled for > > now. > > PERF_COUNTERS is x86-only, and both LIVEPATCH really should be > guarded > by have HAVE_$FOO selected by ARCH. > > However, that's not work to get stuck into now. > > It's quite unreasonable how much stuff doesn't work in simple > builds... > > > Signed-off-by: Oleksii Kurochko > > Acked-by: Andrew Cooper > > I'm happy to fix up the two minor issues on commit. Thanks. I Would appreciate that. ~ Oleksii
Re: [PATCH v4 0/2] Clean the policy manipulation path in domain creation
On Thu, 2024-05-30 at 10:44 +0100, Andrew Cooper wrote: > On 29/05/2024 3:30 pm, Alejandro Vallejo wrote: > > Alejandro Vallejo (2): > > tools/xg: Streamline cpu policy serialise/deserialise calls > > tools/xg: Clean up xend-style overrides for CPU policies > > Oleksii: Please consider for 4.19. Release-Acked-by: Oleksii Kurochko ~ Oleksii > > This is internal clean-up to CPUID handling which has been trying > (one > way or another) to land for more than 3 years now. > > ~Andrew
Re: [PATCH] tools: (Actually) drop libsystemd as a dependency
On Thu, 2024-05-30 at 11:14 +0100, Andrew Cooper wrote:
> When reinstating some of systemd.m4 between v1 and v2, I reintroduced
> a little
> too much. While {c,o}xenstored are indeed no longer linked against
> libsystemd, ./configure still looks for it.
>
> Drop this too.
>
> Fixes: ae26101f6bfc ("tools: Drop libsystemd as a dependency")
> Signed-off-by: Andrew Cooper
Release-Acked-by: Oleksii Kurochko
~ Oleksii
> ---
> CC: Anthony PERARD
> CC: Juergen Gross
> CC: Oleksii Kurochko
> CC: George Dunlap
> CC: Jan Beulich
> CC: Stefano Stabellini
> CC: Julien Grall
> CC: Roger Pau Monné
>
> Found when trying to build Xen in XenServer with libsystemd absent
> from the
> chroot.
> ---
> m4/systemd.m4 | 8 --
> tools/configure | 229 +-
> --
> 2 files changed, 1 insertion(+), 236 deletions(-)
>
> diff --git a/m4/systemd.m4 b/m4/systemd.m4
> index e4fe51a8ba..ab12ea313d 100644
> --- a/m4/systemd.m4
> +++ b/m4/systemd.m4
> @@ -86,13 +86,6 @@ AC_DEFUN([AX_CHECK_SYSTEMD], [
> ],[systemd=n])
> ])
>
> -AC_DEFUN([AX_CHECK_SYSTEMD_ENABLE_AVAILABLE], [
> - PKG_CHECK_MODULES([SYSTEMD], [libsystemd-daemon],
> [systemd="y"],[
> - PKG_CHECK_MODULES([SYSTEMD], [libsystemd >= 209],
> - [systemd="y"],[systemd="n"])
> - ])
> -])
> -
> dnl Enables systemd by default and requires a --disable-systemd
> option flag
> dnl to configure if you want to disable.
> AC_DEFUN([AX_ENABLE_SYSTEMD], [
> @@ -112,6 +105,5 @@ dnl to have systemd build libraries it will be
> enabled. You can always force
> dnl disable with --disable-systemd
> AC_DEFUN([AX_AVAILABLE_SYSTEMD], [
> AX_ALLOW_SYSTEMD_OPTS()
> - AX_CHECK_SYSTEMD_ENABLE_AVAILABLE()
> AX_CHECK_SYSTEMD()
> ])
> diff --git a/tools/configure b/tools/configure
> index b8faa1d520..459bfb5652 100755
> --- a/tools/configure
> +++ b/tools/configure
> @@ -626,8 +626,6 @@ ac_subst_vars='LTLIBOBJS
> LIBOBJS
> pvshim
> ninepfs
> -SYSTEMD_LIBS
> -SYSTEMD_CFLAGS
> SYSTEMD_MODULES_LOAD
> SYSTEMD_DIR
> systemd
> @@ -864,9 +862,7 @@ pixman_LIBS
> libzstd_CFLAGS
> libzstd_LIBS
> LIBNL3_CFLAGS
> -LIBNL3_LIBS
> -SYSTEMD_CFLAGS
> -SYSTEMD_LIBS'
> +LIBNL3_LIBS'
>
>
> # Initialize some variables set by options.
> @@ -1621,10 +1617,6 @@ Some influential environment variables:
> LIBNL3_CFLAGS
> C compiler flags for LIBNL3, overriding pkg-config
> LIBNL3_LIBS linker flags for LIBNL3, overriding pkg-config
> - SYSTEMD_CFLAGS
> - C compiler flags for SYSTEMD, overriding pkg-config
> - SYSTEMD_LIBS
> - linker flags for SYSTEMD, overriding pkg-config
>
> Use these variables to override the choices made by `configure' or
> to help
> it to find libraries and programs with nonstandard names/locations.
> @@ -3889,8 +3881,6 @@ esac
>
>
>
> -
> -
>
>
>
> @@ -9540,223 +9530,6 @@ fi
>
>
>
> -
> -pkg_failed=no
> -{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for SYSTEMD" >&5
> -$as_echo_n "checking for SYSTEMD... " >&6; }
> -
> -if test -n "$SYSTEMD_CFLAGS"; then
> - pkg_cv_SYSTEMD_CFLAGS="$SYSTEMD_CFLAGS"
> - elif test -n "$PKG_CONFIG"; then
> - if test -n "$PKG_CONFIG" && \
> - { { $as_echo "$as_me:${as_lineno-$LINENO}: \$PKG_CONFIG --exists
> --print-errors \"libsystemd-daemon\""; } >&5
> - ($PKG_CONFIG --exists --print-errors "libsystemd-daemon") 2>&5
> - ac_status=$?
> - $as_echo "$as_me:${as_lineno-$LINENO}: \$? = $ac_status" >&5
> - test $ac_status = 0; }; then
> - pkg_cv_SYSTEMD_CFLAGS=`$PKG_CONFIG --cflags "libsystemd-daemon"
> 2>/dev/null`
> - test "x$?" != "x0" && pkg_failed=yes
> -else
> - pkg_failed=yes
> -fi
> - else
> - pkg_failed=untried
> -fi
> -if test -n "$SYSTEMD_LIBS"; then
> - pkg_cv_SYSTEMD_LIBS="$SYSTEMD_LIBS"
> - elif test -n "$PKG_CONFIG"; then
> - if test -n "$PKG_CONFIG" && \
> - { { $as_echo "$as_me:${as_lineno-$LINENO}: \$PKG_CONFIG --exists
> --print-errors \"libsystemd-daemon\""; } >&5
> - ($PKG_CONFIG --exists --print-errors "libsystemd-daemon") 2>&5
> - ac_status=$?
> - $as_echo "$as_me:${as_lineno-$LINENO}: \$? = $ac_status" >&5
> - test $ac_status = 0; }; then
> - pkg_cv_SYSTEMD_LIBS=`$PKG_CONFIG --libs "libsystemd-daemon"
> 2>/dev/null`
> - test "x$?" != "x0" && pkg_failed=yes
> -else
> - pkg_failed=yes
> -fi
> - else
> - pkg_failed=untried
> -fi
> -
> -
> -
> -if test $pkg_failed = yes; then
> - { $as_echo "$as_me:${as_lineno-$LINENO}: result: no" >&5
> -$as_echo "no" >&6; }
> -
> -if $PKG_CONFIG --atleast-pkgconfig-version 0.20; then
> - _pkg_short_errors_supported=yes
> -else
> - _pkg_short_errors_supported=no
> -fi
> - if test $_pkg_short_errors_supported = yes; then
> - SYSTEMD_PKG_ERRORS=`$PKG_CONFIG --short-errors --
> print-errors --cflags --libs "libsystemd-daemon" 2>&1`
> - else
> - SYSTEMD_PKG_ERRORS=`$PKG_CONFIG
Re: [PATCH v11 2/9] xen: introduce generic non-atomic test_*bit()
On Wed, 2024-05-29 at 18:29 +0200, Oleksii K. wrote: > On Wed, 2024-05-29 at 17:22 +0200, Jan Beulich wrote: > > On 29.05.2024 16:58, Oleksii K. wrote: > > > static always_inline bool test_bit(int nr, const volatile void > > > *addr)On > > > Wed, 2024-05-29 at 12:06 +0200, Jan Beulich wrote: > > > > On 29.05.2024 11:59, Julien Grall wrote: > > > > > I didn't realise this was an existing comment. I think the > > > > > suggestion is > > > > > a little bit odd because you could use the atomic version of > > > > > the > > > > > helper. > > > > > > > > > > Looking at Linux, the second sentence was dropped. But not > > > > > the > > > > > first > > > > > one. I would suggest to do the same. IOW keep: > > > > > > > > > > " > > > > > If two examples of this operation race, one can appear to > > > > > succeed > > > > > but > > > > > actually fail. > > > > > " > > > > > > > > As indicated, I'm okay with that being retained, but only in a > > > > form > > > > that > > > > actually makes sense. I've explained before (to Oleksii) what I > > > > consider > > > > wrong in this way of wording things. > > > > > > Would it be suitable to rephrase it in the following way: > > > * This operation is non-atomic and can be reordered. > > > - * If two examples of this operation race, one can appear to > > > succeed > > > - * but actually fail. You must protect multiple accesses > > > with > > > a > > > lock. > > > + * If two instances of this operation race, one may succeed > > > in > > > updating > > > + * the bit in memory, but actually fail. It should be > > > protected > > > from > > > + * potentially racy behavior. > > > */ > > > static always_inline bool > > > __test_and_set_bit(int nr, volatile void *addr) > > > > You've lost the "appear to" ahead of "succeed". Yet even with the > > adjustment > > I still don't see what the "appear to succeed" really is supposed > > to > > mean > > here. The issue (imo) isn't with success or failure, but with the > > write of > > one CPU potentially being immediately overwritten by another CPU, > > not > > observing the bit change that the first CPU did. IOW both will > > succeed (or > > appear to succeed), but one update may end up being lost. Maybe > > "..., > > both > > may update memory with their view of the new value, not taking into > > account > > the update the respectively other one did"? Or "..., both will > > appear > > to > > succeed, but one of the updates may be lost"? > For me, the first one is clearer. If then this part of the comment is needed for test_bit() as it is doing only reading? > Julien, would that be okay for you? ~ Oleksii
Re: [PATCH v11 2/9] xen: introduce generic non-atomic test_*bit()
On Wed, 2024-05-29 at 17:22 +0200, Jan Beulich wrote: > On 29.05.2024 16:58, Oleksii K. wrote: > > static always_inline bool test_bit(int nr, const volatile void > > *addr)On > > Wed, 2024-05-29 at 12:06 +0200, Jan Beulich wrote: > > > On 29.05.2024 11:59, Julien Grall wrote: > > > > I didn't realise this was an existing comment. I think the > > > > suggestion is > > > > a little bit odd because you could use the atomic version of > > > > the > > > > helper. > > > > > > > > Looking at Linux, the second sentence was dropped. But not the > > > > first > > > > one. I would suggest to do the same. IOW keep: > > > > > > > > " > > > > If two examples of this operation race, one can appear to > > > > succeed > > > > but > > > > actually fail. > > > > " > > > > > > As indicated, I'm okay with that being retained, but only in a > > > form > > > that > > > actually makes sense. I've explained before (to Oleksii) what I > > > consider > > > wrong in this way of wording things. > > > > Would it be suitable to rephrase it in the following way: > > * This operation is non-atomic and can be reordered. > > - * If two examples of this operation race, one can appear to > > succeed > > - * but actually fail. You must protect multiple accesses with > > a > > lock. > > + * If two instances of this operation race, one may succeed in > > updating > > + * the bit in memory, but actually fail. It should be protected > > from > > + * potentially racy behavior. > > */ > > static always_inline bool > > __test_and_set_bit(int nr, volatile void *addr) > > You've lost the "appear to" ahead of "succeed". Yet even with the > adjustment > I still don't see what the "appear to succeed" really is supposed to > mean > here. The issue (imo) isn't with success or failure, but with the > write of > one CPU potentially being immediately overwritten by another CPU, not > observing the bit change that the first CPU did. IOW both will > succeed (or > appear to succeed), but one update may end up being lost. Maybe "..., > both > may update memory with their view of the new value, not taking into > account > the update the respectively other one did"? Or "..., both will appear > to > succeed, but one of the updates may be lost"? For me, the first one is clearer. Julien, would that be okay for you? ~ Oleksii
Re: [PATCH v11 2/9] xen: introduce generic non-atomic test_*bit()
static always_inline bool test_bit(int nr, const volatile void *addr)On Wed, 2024-05-29 at 12:06 +0200, Jan Beulich wrote: > On 29.05.2024 11:59, Julien Grall wrote: > > Hi, > > > > On 29/05/2024 09:36, Jan Beulich wrote: > > > On 29.05.2024 09:50, Oleksii K. wrote: > > > > On Tue, 2024-05-28 at 09:53 +0100, Julien Grall wrote: > > > > > > > > +/** > > > > > > > > + * generic_test_bit - Determine whether a bit is set > > > > > > > > + * @nr: bit number to test > > > > > > > > + * @addr: Address to start counting from > > > > > > > > + * > > > > > > > > + * This operation is non-atomic and can be reordered. > > > > > > > > + * If two examples of this operation race, one can > > > > > > > > appear to > > > > > > > > succeed > > > > > > > > + * but actually fail. You must protect multiple > > > > > > > > accesses with > > > > > > > > a > > > > > > > > lock. > > > > > > > > + */ > > > > > > > > > > > > > > You got carried away updating comments - there's no > > > > > > > raciness for > > > > > > > simple test_bit(). As is also expressed by its name not > > > > > > > having > > > > > > > those > > > > > > > double underscores that the others have. > > > > > > Then it is true for every function in this header. Based on > > > > > > the > > > > > > naming > > > > > > the conclusion can be done if it is atomic/npn-atomic and > > > > > > can/can't > > > > > > be > > > > > > reordered. > > > > > > > > > > So let me start with that my only request is to keep the > > > > > existing > > > > > comments as you move it. It looks like there were none of > > > > > test_bit() > > > > > before. > > > > Just to clarify that I understand correctly. > > > > > > > > Do we need any comment above functions generic_*()? Based on > > > > that they > > > > are implemented in generic way they will be always "non-atomic > > > > and can > > > > be reordered.". > > > > > > I indicated before that I think reproducing the same comments > > > __test_and_* > > > already have also for generic_* isn't overly useful. If someone > > > insisted > > > on them being there as well, I could live with that, though. > > > > Would you be ok if the comment is only on top of the __test_and_* > > version? (So no comments on top of the generic_*) > > That's my preferred variant, actually. The alternative I would also > be > okay-ish with is to have the comments also ahead of generic_*. > > > > > Do you find the following comment useful? > > > > > > > > " * If two examples of this operation race, one can appear to > > > > succeed > > > > * but actually fail. You must protect multiple accesses with > > > > a lock." > > > > > > > > It seems to me that it can dropped as basically "non-atomic and > > > > can be > > > > reordered." means that. > > > > > > I agree, or else - as indicated before - the wording would need > > > to further > > > change. Yet iirc you've added that in response to a comment from > > > Julien, > > > so you'll primarily want his input as to the presence of > > > something along > > > these lines. > > > > I didn't realise this was an existing comment. I think the > > suggestion is > > a little bit odd because you could use the atomic version of the > > helper. > > > > Looking at Linux, the second sentence was dropped. But not the > > first > > one. I would suggest to do the same. IOW keep: > > > > " > > If two examples of this operation race, one can appear to succeed > > but > > actually fail. > > " > > As indicated, I'm okay with that being retained, but only in a form > that > actually makes sense. I've explained before (to Oleksii) what I > consider > wrong in this way of wording things. Would it be suitable to rephrase it in the following way: * This operation is non-atomic
Re: [PATCH] Partial revert of "x86/MCE: optional build of AMD/Intel MCE code"
On Wed, 2024-05-29 at 11:37 +0100, Andrew Cooper wrote:
> {cmci,lmce}_support are written during S3 resume, so cannot live in
> __ro_after_init. Move them back to being __read_mostly, as they were
> originally.
>
> Link: https://gitlab.com/xen-project/xen/-/jobs/6966698361
> Fixes: 19b6e9f9149f ("x86/MCE: optional build of AMD/Intel MCE code")
> Signed-off-by: Andrew Cooper
> ---
> CC: Jan Beulich
> CC: Roger Pau Monné
> CC: Sergiy Kibrik
> CC: Stefano Stabellini
> CC: Oleksii Kurochko
>
> We're past feature freeze and this was a silent change in a patch,
> which was
> also untested. A 30s look at mcheck_init() shows clearly that it's
> not a 30s
> job to fix.
> ---
Release-Acked-by: Oleksii Kurochko
~ Oleksii
> xen/arch/x86/cpu/mcheck/mce.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/x86/cpu/mcheck/mce.c
> b/xen/arch/x86/cpu/mcheck/mce.c
> index 1664ca6412ac..32c1b2756b90 100644
> --- a/xen/arch/x86/cpu/mcheck/mce.c
> +++ b/xen/arch/x86/cpu/mcheck/mce.c
> @@ -38,10 +38,10 @@ DEFINE_PER_CPU_READ_MOSTLY(unsigned int,
> nr_mce_banks);
> unsigned int __read_mostly firstbank;
> unsigned int __read_mostly ppin_msr;
> uint8_t __read_mostly cmci_apic_vector;
> -bool __ro_after_init cmci_support;
> +bool __read_mostly cmci_support;
>
> /* If mce_force_broadcast == 1, lmce_support will be disabled
> forcibly. */
> -bool __ro_after_init lmce_support;
> +bool __read_mostly lmce_support;
>
> DEFINE_PER_CPU_READ_MOSTLY(struct mca_banks *, poll_bankmask);
> DEFINE_PER_CPU_READ_MOSTLY(struct mca_banks *, no_cmci_banks);
>
> base-commit: 0840bc5ea114f536a4bdfb2ca095b79f2069aae6
Re: Updated Xen 4.19 schedule
On Wed, 2024-05-29 at 10:19 +0200, Jan Beulich wrote: > On 29.05.2024 10:03, Oleksii K. wrote: > > Hello everyone, > > > > I would like to announce that I have decided to update the Xen 4.19 > > schedule due to the extended feature freeze period and the upcoming > > Xen > > Summit next week. > > > > I propose the following updates: > > Code Freeze: from May 31 to June 7 > > This is almost fully the week of the summit. With ... > > > Hard Code Freeze: from June 21 to June 28 > > ... this, did you perhaps mean May 31 to June 20 there? > > > Final commits: from July 5 to July 12 > > Somewhat similarly, what's between June 28 and July 5? It should be RCx ( IIUC how it was done previously ) + bugfixes for serious bugs (including regressions), and probably low-risk fixes only Let me share the updated schedule as I made incorrect calculations: Code Freeze: Fri, June 14 (+2 weeks from Feature Freeze, considering the week of the summit) Hard Freeze: Fri, June 28 (+2 weeks from Code Freeze, usually it is +3 weeks, but I think we can go with +2 and extend ( to 3 weeks ) if necessary) Final Commits: Fri, July 12 (+2 weeks from Hard Freeze) Release: Wed, July 17 ~ Oleksii > > Jan > > > Release: July 17 > > The release date is shifted, but it still remains in July, which > > seems > > acceptable to me. > > > > One more thing: > > No release ack is needed before Rc1. Please commit bug fixes at > > will. > > > > Have a nice day. > > > > Best regards, > > Oleksii >
Re: [PATCH for-4.19 0/9] x86/irq: fixes for CPU hot{,un}plug
On Wed, 2024-05-29 at 11:01 +0200, Roger Pau Monne wrote:
> Hello,
>
> The following series aim to fix interrupt handling when doing CPU
> plug/unplug operations. Without this series running:
>
> cpus=`xl info max_cpu_id`
> while [ 1 ]; do
> for i in `seq 1 $cpus`; do
> xen-hptool cpu-offline $i;
> xen-hptool cpu-online $i;
> done
> done
>
> Quite quickly results in interrupts getting lost and "No irq handler
> for
> vector" messages on the Xen console. Drivers in dom0 also start
> getting
> interrupt timeouts and the system becomes unusable.
>
> After applying the series running the loop over night still result in
> a
> fully usable system, no "No irq handler for vector" messages at all,
> no
> interrupt loses reported by dom0. Test with
> x2apic-mode={mixed,cluster}.
>
> I'm tagging this for 4.19 as it's IMO bugfixes, but the series has
> grown
> quite bigger than expected, and hence we need to be careful to not
> introduce breakages late in the release cycle. I've attempted to
> document all code as good as I could, interrupt handling has some
> unexpected corner cases that are hard to diagnose and reason about.
Despite of the fact that it can be considered as bugfixes, it seems to
me that this patch series can be risky. Let's wait for maintainers
opinion...
~ Oleksii
>
> I'm currently also doing some extra testing with XenRT in case I've
> missed something.
>
> Thanks, Roger.
>
> Roger Pau Monne (9):
> x86/irq: remove offline CPUs from old CPU mask when adjusting
> move_cleanup_count
> xen/cpu: do not get the CPU map in stop_machine_run()
> xen/cpu: ensure get_cpu_maps() returns false if CPU operations are
> underway
> x86/irq: describe how the interrupt CPU movement works
> x86/irq: limit interrupt movement done by fixup_irqs()
> x86/irq: restrict CPU movement in set_desc_affinity()
> x86/irq: deal with old_cpu_mask for interrupts in movement in
> fixup_irqs()
> x86/irq: handle moving interrupts in _assign_irq_vector()
> x86/irq: forward pending interrupts to new destination in
> fixup_irqs()
>
> xen/arch/x86/apic.c | 5 +
> xen/arch/x86/include/asm/apic.h | 3 +
> xen/arch/x86/include/asm/irq.h | 28 +-
> xen/arch/x86/irq.c | 172 +-
> --
> xen/common/cpu.c | 8 +-
> xen/common/stop_machine.c | 15 +--
> xen/include/xen/cpu.h | 2 +
> xen/include/xen/rwlock.h | 2 +
> 8 files changed, 190 insertions(+), 45 deletions(-)
>
Updated Xen 4.19 schedule
Hello everyone, I would like to announce that I have decided to update the Xen 4.19 schedule due to the extended feature freeze period and the upcoming Xen Summit next week. I propose the following updates: Code Freeze: from May 31 to June 7 Hard Code Freeze: from June 21 to June 28 Final commits: from July 5 to July 12 Release: July 17 The release date is shifted, but it still remains in July, which seems acceptable to me. One more thing: No release ack is needed before Rc1. Please commit bug fixes at will. Have a nice day. Best regards, Oleksii
Re: [PATCH v11 2/9] xen: introduce generic non-atomic test_*bit()
On Tue, 2024-05-28 at 09:53 +0100, Julien Grall wrote: > > > > +/** > > > > + * generic_test_bit - Determine whether a bit is set > > > > + * @nr: bit number to test > > > > + * @addr: Address to start counting from > > > > + * > > > > + * This operation is non-atomic and can be reordered. > > > > + * If two examples of this operation race, one can appear to > > > > succeed > > > > + * but actually fail. You must protect multiple accesses with > > > > a > > > > lock. > > > > + */ > > > > > > You got carried away updating comments - there's no raciness for > > > simple test_bit(). As is also expressed by its name not having > > > those > > > double underscores that the others have. > > Then it is true for every function in this header. Based on the > > naming > > the conclusion can be done if it is atomic/npn-atomic and can/can't > > be > > reordered. > > So let me start with that my only request is to keep the existing > comments as you move it. It looks like there were none of test_bit() > before. Just to clarify that I understand correctly. Do we need any comment above functions generic_*()? Based on that they are implemented in generic way they will be always "non-atomic and can be reordered.". Do you find the following comment useful? " * If two examples of this operation race, one can appear to succeed * but actually fail. You must protect multiple accesses with a lock." It seems to me that it can dropped as basically "non-atomic and can be reordered." means that. ~ Oleksii
Re: [PATCH for-4.19 v3 0/3] xen/x86: support foreign mappings for HVM/PVH
On Wed, 2024-05-29 at 09:24 +0200, Jan Beulich wrote: > On 17.05.2024 15:33, Roger Pau Monne wrote: > > Hello, > > > > The following series attempts to solve a shortcoming of HVM/PVH > > guests > > with the lack of support for foreign mappings. Such lack of > > support > > prevents using PVH based guests as stubdomains for example. > > > > Add support in a way similar to how it's done on Arm, by iterating > > over > > the p2m based on the maximum gfn. > > > > Patch 2 is not strictly needed. Moving the enablement of altp2m > > from an > > HVM param to a create domctl flag avoids any possible race with the > > HVM > > param changing after it's been evaluated. Note the param can only > > be > > set by the control domain, and libxl currently sets it at domain > > create. Also altp2m enablement is different from activation, as > > activation does happen during runtime of the domain. > > > > Thanks, Roger. > > > > Roger Pau Monne (3): > > xen/x86: account number of foreign mappings in the p2m > > xen/x86: enable altp2m at create domain domctl > > xen/x86: remove foreign mappings from the p2m on teardown > > Here, too, I'd like to ask whether to keep this as a candidate for > 4.19, or > whether to postpone. Afaict what's still missing are Arm and tool > chain acks > on patch 2. We can consider to have this patch series in 4.19: Release-acked-by: Oleksii Kurochko ~ Oleksii > > Jan > > > CHANGELOG.md | 1 + > > tools/libs/light/libxl_create.c | 23 +- > > tools/libs/light/libxl_x86.c | 26 +-- > > tools/ocaml/libs/xc/xenctrl_stubs.c | 2 +- > > xen/arch/arm/domain.c | 6 +++ > > xen/arch/x86/domain.c | 28 > > xen/arch/x86/hvm/hvm.c | 23 +- > > xen/arch/x86/include/asm/p2m.h | 32 +- > > xen/arch/x86/mm/p2m-basic.c | 18 > > xen/arch/x86/mm/p2m.c | 68 > > +++-- > > xen/include/public/domctl.h | 20 - > > xen/include/public/hvm/params.h | 9 +--- > > 12 files changed, 215 insertions(+), 41 deletions(-) > > >
Re: [PATCH v2 for-4.19 0.5/13] xen: Introduce CONFIG_SELF_TESTS
On Tue, 2024-05-28 at 15:22 +0100, Andrew Cooper wrote:
> ... and move x86's stub_selftest() under this new option.
>
> There is value in having these tests included in release builds too.
>
> It will shortly be used to gate the bitops unit tests on all
> architectures.
>
> Signed-off-by: Andrew Cooper
Looks good to me.
We can consider it to be merged to 4.19:
Release-Acked-by: Oleksii Kurochko
~ Oleksii
> ---
> CC: Jan Beulich
> CC: Roger Pau Monné
> CC: Wei Liu
> CC: Stefano Stabellini
> CC: Julien Grall
> CC: Volodymyr Babchuk
> CC: Bertrand Marquis
> CC: Michal Orzel
> CC: Oleksii Kurochko
> CC: Shawn Anastasio
> CC: consult...@bugseng.com
> CC: Simone Ballarin
> CC: Federico Serafini
> CC: Nicola Vetrini
>
> v2.5:
> * As suggested in "[PATCH v2 05/13] xen/bitops: Implement
> generic_f?sl() in
> lib/"
>
> I've gone with SELF_TESTS rather than BOOT_TESTS, because already in
> bitops
> we've got compile time tests (which aren't strictly boot time), and
> the
> livepatching testing wants to be included here and is definitely not
> boot
> time.
> ---
> xen/Kconfig.debug | 6 ++
> xen/arch/x86/extable.c | 4 ++--
> xen/arch/x86/setup.c | 2 +-
> 3 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/xen/Kconfig.debug b/xen/Kconfig.debug
> index 61b24ac552cd..07ff7eb7ba83 100644
> --- a/xen/Kconfig.debug
> +++ b/xen/Kconfig.debug
> @@ -29,6 +29,12 @@ config FRAME_POINTER
> maybe slower, but it gives very useful debugging
> information
> in case of any Xen bugs.
>
> +config SELF_TESTS
> + bool "Extra self-testing"
> + default DEBUG
> + help
> + Enable extra unit and functional testing.
> +
> config COVERAGE
> bool "Code coverage support"
> depends on !LIVEPATCH
> diff --git a/xen/arch/x86/extable.c b/xen/arch/x86/extable.c
> index 8415cd1fa249..705cf9eb94ca 100644
> --- a/xen/arch/x86/extable.c
> +++ b/xen/arch/x86/extable.c
> @@ -144,7 +144,7 @@ search_exception_table(const struct cpu_user_regs
> *regs, unsigned long *stub_ra)
> return 0;
> }
>
> -#ifdef CONFIG_DEBUG
> +#ifdef CONFIG_SELF_TESTS
> #include
> #include
>
> @@ -214,7 +214,7 @@ int __init cf_check stub_selftest(void)
> return 0;
> }
> __initcall(stub_selftest);
> -#endif
> +#endif /* CONFIG_SELF_TESTS */
>
> unsigned long asmlinkage search_pre_exception_table(struct
> cpu_user_regs *regs)
> {
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index b50c9c84af6d..dd51e68dbe5b 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -742,7 +742,7 @@ static void noreturn init_done(void)
> system_state = SYS_STATE_active;
>
> /* Re-run stub recovery self-tests with CET-SS active. */
> - if ( IS_ENABLED(CONFIG_DEBUG) && cpu_has_xen_shstk )
> + if ( IS_ENABLED(CONFIG_SELF_TESTS) && cpu_has_xen_shstk )
> stub_selftest();
>
> domain_unpause_by_systemcontroller(dom0);
>
> base-commit: 2d93f78bfe25f695d8ffb61d110da9df293ed71b
Re: [PATCH for-4.19 0/3] xen: Misc MISRA changes
On Tue, 2024-05-21 at 18:15 +0100, Andrew Cooper wrote:
> Misc fixes collected during today's call.
Release-Acked-by: Oleksii Kurochko
>
> Andrew Cooper (3):
> xen/lzo: Implement COPY{4,8} using memcpy()
> xen/x86: Drop useless non-Kconfig CONFIG_* variables
> xen/x86: Address two misc MISRA 17.7 violations
>
> xen/arch/x86/alternative.c | 4 ++--
> xen/arch/x86/include/asm/config.h | 4
> xen/arch/x86/nmi.c | 5 ++---
> xen/common/lzo.c | 11 ++-
> xen/include/xen/acpi.h | 9 -
> xen/include/xen/watchdog.h | 13 +
> 6 files changed, 7 insertions(+), 39 deletions(-)
>
>
> base-commit: 26b122e3bf8f3921d87312fbf5e7e13872ae92b0
Re: [XEN PATCH v4 0/3] x86: make Intel/AMD vPMU & MCE support configurable
On Mon, 2024-05-27 at 17:47 +0200, Jan Beulich wrote: > Oleksii, > > On 22.05.2024 10:37, Sergiy Kibrik wrote: > > Three remaining patches to separate support of Intel & AMD CPUs in > > Xen build. > > Most of related patches from previous series had already been > > merged. > > Specific changes since v3 are provided per-patch. > > > > v3 series here: > > https://lore.kernel.org/xen-devel/cover.1715673586.git.sergiy_kib...@epam.com/ > > > > -Sergiy > > > > Sergiy Kibrik (3): > > x86/intel: move vmce_has_lmce() routine to header > > x86/MCE: add default switch case in init_nonfatal_mce_checker() > > x86/MCE: optional build of AMD/Intel MCE code > > As I'm apparently confused as to the state 4.19 is in, may I please > ask > whether this series is still okay to go in, or whether it should be > postponed until after branching. I am okay to go in this release. Sorry for the confusion with the 4.19 state. I'll send a proper schedule today. ~ Oleksii
Re: [PATCH] x86/svm: Rework VMCB_ACCESSORS() to use a plain type name
On Tue, 2024-05-28 at 17:37 +0200, Jan Beulich wrote: > On 28.05.2024 17:32, Andrew Cooper wrote: > > This avoids having a function call in a typeof() expression. > > > > No functional change. > > > > Signed-off-by: Andrew Cooper > > Acked-by: Jan Beulich > Release-Acked-by: Oleksii Kurochko ~ Oleksii
Re: [PATCH v11 2/9] xen: introduce generic non-atomic test_*bit()
On Tue, 2024-05-28 at 08:20 +0200, Jan Beulich wrote: > On 24.05.2024 13:08, Oleksii Kurochko wrote: > > The following generic functions were introduced: > > * test_bit > > * generic__test_and_set_bit > > * generic__test_and_clear_bit > > * generic__test_and_change_bit > > > > These functions and macros can be useful for architectures > > that don't have corresponding arch-specific instructions. > > > > Also, the patch introduces the following generics which are > > used by the functions mentioned above: > > * BITOP_BITS_PER_WORD > > * BITOP_MASK > > * BITOP_WORD > > * BITOP_TYPE > > > > BITS_PER_BYTE was moved to xen/bitops.h as BITS_PER_BYTE is the > > same > > across architectures. > > > > The following approach was chosen for generic*() and arch*() bit > > operation functions: > > If the bit operation function that is going to be generic starts > > with the prefix "__", then the corresponding generic/arch function > > will also contain the "__" prefix. For example: > > * test_bit() will be defined using arch_test_bit() and > > generic_test_bit(). > > * __test_and_set_bit() will be defined using > > arch__test_and_set_bit() and generic__test_and_set_bit(). > > > > Signed-off-by: Oleksii Kurochko > > --- > > Reviewed-by: Jan Beulich jbeul...@suse.com? Jan gave his R-by for > > the previous > > version of the patch, but some changes were done, so I wasn't sure > > if I could > > use the R-by in this patch version. > > That really depends on the nature of the changes. Seeing the pretty > long list below, I think it was appropriate to drop the R-b. > > > --- > > Changes in V11: > > - fix identation in generic_test_bit() function. > > - move definition of BITS_PER_BYTE from /bitops.h to > > xen/bitops.h > > I realize you were asked to do this, but I'm not overly happy with > it. > BITS_PER_BYTE is far more similar to BITS_PER_LONG than to > BITOP_BITS_PER_WORD. Plus in any event that change is entirely > unrelated > here. So where then it should be introduced? x86 introduces that in config.h, Arm in asm/bitops.h. I am okay to revert this change and make a separate patch. > > > - drop the changes in arm64/livepatch.c. > > - update the the comments on top of functions: > > generic__test_and_set_bit(), > > generic__test_and_clear_bit(), generic__test_and_change_bit(), > > generic_test_bit(). > > - Update footer after Signed-off section. > > - Rebase the patch on top of staging branch, so it can be merged > > when necessary > > approves will be given. > > - Update the commit message. > > --- > > xen/arch/arm/include/asm/bitops.h | 69 --- > > xen/arch/ppc/include/asm/bitops.h | 54 - > > xen/arch/ppc/include/asm/page.h | 2 +- > > xen/arch/ppc/mm-radix.c | 2 +- > > xen/arch/x86/include/asm/bitops.h | 31 ++--- > > xen/include/xen/bitops.h | 185 > > ++ > > 6 files changed, 196 insertions(+), 147 deletions(-) > > > > --- a/xen/include/xen/bitops.h > > +++ b/xen/include/xen/bitops.h > > @@ -103,8 +103,193 @@ static inline int generic_flsl(unsigned long > > x) > > * Include this here because some architectures need > > generic_ffs/fls in > > * scope > > */ > > + > > +#define BITOP_BITS_PER_WORD 32 > > +typedef uint32_t bitop_uint_t; > > + > > +#define BITOP_MASK(nr) ((bitop_uint_t)1 << ((nr) % > > BITOP_BITS_PER_WORD)) > > + > > +#define BITOP_WORD(nr) ((nr) / BITOP_BITS_PER_WORD) > > + > > +#define BITS_PER_BYTE 8 > > This, if to be moved at all, very clearly doesn't belong here. As > much > as it's unrelated to this change, it's also unrelated to bitops as a > whole. > > > +extern void __bitop_bad_size(void); > > + > > +#define bitop_bad_size(addr) (sizeof(*(addr)) < > > sizeof(bitop_uint_t)) > > + > > +/* - Please tidy above here -- > > --- */ > > + > > #include > > > > +/** > > + * generic__test_and_set_bit - Set a bit and return its old value > > + * @nr: Bit to set > > + * @addr: Address to count from > > + * > > + * This operation is non-atomic and can be reordered. > > + * If two examples of this operation race, one can appear to > > succeed > > Why "examples"? Do you mean "instances"? It's further unclear what > "succeed" and "fail" here mean. The function after all has two > purposes: Checking and setting the specified bit. Therefore I think > you mean "succeed in updating the bit in memory", yet then it's > still unclear what the "appear" here means: The caller has no > indication of success/failure. Therefore I think "appear to" also > wants dropping. > > > + * but actually fail. You must protect multiple accesses with a > > lock. > > That's not "must". An often better alternative is to use the atomic > forms instead. "Multiple" is imprecise, too: "Potentially racy" or > some such would come closer. I think we can go only with: " This operation is non-atomic and can be reordered." and drop: " If two examples of this operation race, one
Re: Code freeze date for Xen 4.19 is 31.05.2024
On Mon, 2024-05-27 at 17:55 +0200, Jan Beulich wrote: > On 27.05.2024 17:52, Oleksii K. wrote: > > On Mon, 2024-05-27 at 17:12 +0200, Jan Beulich wrote: > > > On 27.05.2024 15:58, Oleksii K. wrote: > > > > I would like to remind you that the code freeze date for Xen > > > > 4.19 > > > > is > > > > May 31, 2024. > > > > > > I may be confused: With feature freeze having been last Friday > > > aiui, > > > isn't > > > this a little too short a period? I was actually expecting a > > > longer- > > > than- > > > normal one to account for the Xen Summit week. > > It makes sense to move the last day to June 14. Consequently, we > > would > > need to adjust the schedule as follows: > > > > Hard Code Freeze: from June 21 to June 28 > > Final commits: from July 5 to July 12 > > Release: July 17 > > > > And this schedule also looks good to me. > > > > However, another option is to combine the Code Freeze and Hard Code > > Freeze periods since both phases are about accepting only bug > > fixes, > > differing only in the severity of the fixes. > > I'm okay with whichever way you want it. All I'm seeking is clarity. I realized that we have the Xen Summit coming up, which means we'll lose almost a week from the review process perspective. I would like to hear the other maintainers' thoughts on updating our release schedule to have the release on July 17. ~ Oleksii
Re: [PATCH v2 3/3] CHANGELOG: Mention libxl blktap/tapback support
On Sun, 2024-04-07 at 16:49 -0400, Jason Andryuk wrote: > From: Jason Andryuk > > Add entry for backendtype=tap support in libxl. blktap needs some > changes to work with libxl, which haven't been merged. They are > available from this PR: > https://github.com/xapi-project/blktap/pull/394 > > Signed-off-by: Jason Andryuk > --- > CHANGELOG.md | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/CHANGELOG.md b/CHANGELOG.md > index 8041cfb7d2..036328433d 100644 > --- a/CHANGELOG.md > +++ b/CHANGELOG.md > @@ -21,6 +21,7 @@ The format is based on [Keep a > Changelog](https://keepachangelog.com/en/1.0.0/) > for IPIs and Physical addressing mode for external interrupts. > - Add a new 9pfs backend running as a daemon in dom0. First user is > Xenstore-stubdom now being able to support full Xenstore trace > capability. > + - libxl support for backendtype=tap with tapback. > > ### Removed > - caml-stubdom. It hasn't built since 2014, was pinned to Ocaml > 4.02, and has Acked-by: Oleksii Kurochko ~ Oleksii
Re: Code freeze date for Xen 4.19 is 31.05.2024
On Mon, 2024-05-27 at 17:12 +0200, Jan Beulich wrote: > On 27.05.2024 15:58, Oleksii K. wrote: > > I would like to remind you that the code freeze date for Xen 4.19 > > is > > May 31, 2024. > > I may be confused: With feature freeze having been last Friday aiui, > isn't > this a little too short a period? I was actually expecting a longer- > than- > normal one to account for the Xen Summit week. It makes sense to move the last day to June 14. Consequently, we would need to adjust the schedule as follows: Hard Code Freeze: from June 21 to June 28 Final commits: from July 5 to July 12 Release: July 17 And this schedule also looks good to me. However, another option is to combine the Code Freeze and Hard Code Freeze periods since both phases are about accepting only bug fixes, differing only in the severity of the fixes. Thoughts? ~ Oleksii > > Jan > > > I'm okay with bug fixes being committed without my release ack > > (just CC > > me), except in cases where a one of maintainers gives a strong > > NACK. > > > > Have a nice week! > > > > Best regards, > > Oleksii >
Re: [PATCH v4 2/4] xen/arm: Alloc XenStore page for Dom0less DomUs from hypervisor
On Sat, 2024-05-25 at 00:06 +0100, Julien Grall wrote:
> Hi Stefano,
>
> You have sent a new version. But you didn't reply to Juergen's
> comment.
>
> While he is not the maintainer of the Arm code, he is the Xenstore
> maintainer. Even if I agree with this approach I don't feel this is
> right to even ack this patch without pending questions addressed.
>
> In this case, we are changing yet another time the sequence for
> initializing Xenstore dom0less domain. I would rather not want to
> have
> to change a 4th time...
>
> I don't think it is a big problem if this is not merged for the code
> freeze as this is technically a bug fix.
Agree, this is not a problem as it is still looks to me as a bug fix.
~ Oleksii
>
> Cheers,
>
> On 24/05/2024 23:55, Stefano Stabellini wrote:
> > From: Henry Wang
> >
> > There are use cases (for example using the PV driver) in Dom0less
> > setup that require Dom0less DomUs start immediately with Dom0, but
> > initialize XenStore later after Dom0's successful boot and call to
> > the init-dom0less application.
> >
> > An error message can seen from the init-dom0less application on
> > 1:1 direct-mapped domains:
> > ```
> > Allocating magic pages
> > memory.c:238:d0v0 mfn 0x39000 doesn't belong to d1
> > Error on alloc magic pages
> > ```
> >
> > The "magic page" is a terminology used in the toolstack as reserved
> > pages for the VM to have access to virtual platform capabilities.
> > Currently the magic pages for Dom0less DomUs are populated by the
> > init-dom0less app through populate_physmap(), and
> > populate_physmap()
> > automatically assumes gfn == mfn for 1:1 direct mapped domains.
> > This
> > cannot be true for the magic pages that are allocated later from
> > the
> > init-dom0less application executed in Dom0. For domain using
> > statically
> > allocated memory but not 1:1 direct-mapped, similar error "failed
> > to
> > retrieve a reserved page" can be seen as the reserved memory list
> > is
> > empty at that time.
> >
> > Since for init-dom0less, the magic page region is only for
> > XenStore.
> > To solve above issue, this commit allocates the XenStore page for
> > Dom0less DomUs at the domain construction time. The PFN will be
> > noted and communicated to the init-dom0less application executed
> > from Dom0. To keep the XenStore late init protocol, set the
> > connection
> > status to XENSTORE_RECONNECT.
> >
> > Reported-by: Alec Kwapis
> > Suggested-by: Daniel P. Smith
> > Signed-off-by: Henry Wang
> > Signed-off-by: Stefano Stabellini
> > ---
> > xen/arch/arm/dom0less-build.c | 55
> > ++-
> > 1 file changed, 54 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-
> > build.c
> > index 74f053c242..2963ecc0b3 100644
> > --- a/xen/arch/arm/dom0less-build.c
> > +++ b/xen/arch/arm/dom0less-build.c
> > @@ -1,5 +1,6 @@
> > /* SPDX-License-Identifier: GPL-2.0-only */
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -10,6 +11,8 @@
> > #include
> > #include
> >
> > +#include
> > +
> > #include
> > #include
> > #include
> > @@ -739,6 +742,53 @@ static int __init alloc_xenstore_evtchn(struct
> > domain *d)
> > return 0;
> > }
> >
> > +#define XENSTORE_PFN_OFFSET 1
> > +static int __init alloc_xenstore_page(struct domain *d)
> > +{
> > + struct page_info *xenstore_pg;
> > + struct xenstore_domain_interface *interface;
> > + mfn_t mfn;
> > + gfn_t gfn;
> > + int rc;
> > +
> > + if ( (UINT_MAX - d->max_pages) < 1 )
> > + {
> > + printk(XENLOG_ERR "%pd: Over-allocation for d->max_pages
> > by 1 page.\n",
> > + d);
> > + return -EINVAL;
> > + }
> > + d->max_pages += 1;
> > + xenstore_pg = alloc_domheap_page(d, MEMF_bits(32));
> > + if ( xenstore_pg == NULL && is_64bit_domain(d) )
> > + xenstore_pg = alloc_domheap_page(d, 0);
> > + if ( xenstore_pg == NULL )
> > + return -ENOMEM;
> > +
> > + mfn = page_to_mfn(xenstore_pg);
> > + if ( !mfn_x(mfn) )
> > + return -ENOMEM;
> > +
> > + if ( !is_domain_direct_mapped(d) )
> > + gfn = gaddr_to_gfn(GUEST_MAGIC_BASE +
> > + (XENSTORE_PFN_OFFSET << PAGE_SHIFT));
> > + else
> > + gfn = gaddr_to_gfn(mfn_to_maddr(mfn));
> > +
> > + rc = guest_physmap_add_page(d, gfn, mfn, 0);
> > + if ( rc )
> > + {
> > + free_domheap_page(xenstore_pg);
> > + return rc;
> > + }
> > +
> > + d->arch.hvm.params[HVM_PARAM_STORE_PFN] = gfn_x(gfn);
> > + interface = map_domain_page(mfn);
> > + interface->connection = XENSTORE_RECONNECT;
> > + unmap_domain_page(interface);
> > +
> > + return 0;
> > +}
> > +
> > static int __init construct_domU(struct domain *d,
> > const struct dt_device_node
> > *node)
> > {
> > @@ -839,7 +889,10 @@ static int __init
Code freeze date for Xen 4.19 is 31.05.2024
Hi all, I would like to remind you that the code freeze date for Xen 4.19 is May 31, 2024. I'm okay with bug fixes being committed without my release ack (just CC me), except in cases where a one of maintainers gives a strong NACK. Have a nice week! Best regards, Oleksii
Re: [PATCH v2 for-4.19 00/13] xen/bitops: Untangle ffs()/fls() infrastructure
I think we can consider to have this patch series in Xen 4.19 release: Release-acked-by: Oleksii Kurochko ~ Oleksii On Fri, 2024-05-24 at 21:03 +0100, Andrew Cooper wrote: > bitops.h is a mess. It has grown organtically over many years, and > forces > unreasonable repsonsibilities out into the per-arch stubs. > > Start cleaning it up with ffs() and friends. Across the board, this > adds: > > * Functioning bitops without arch-specific asm > * An option for arches to provide more optimal code generation > * Compile-time constant folding > * Testing at both compile time and during init that the basic > operations > behave according to spec. > > and the only reason this series isn't a net reduction in code alone > is the > because of the new unit testing. > > This form is superior in many ways, including getting RISC-V support > for free. > > v2: > * Many changes. See patches for details > * Include the fls() side of the infrastructure too. > > Testing: > > https://gitlab.com/xen-project/people/andyhhp/xen/-/pipelines/1304664544 > https://cirrus-ci.com/github/andyhhp/xen/ > > Series-wide net bloat-o-meter: > > x86: up/down: 51/-247 (-196) > ARM64: up/down: 40/-400 (-360) > > and PPC64 reproduced in full, just to demonstrate how absurd it was > to have > generic_f?s() as static inlines... > > add/remove: 1/0 grow/shrink: 1/11 up/down: 228/-4832 (-4604) > Function old new delta > init_constructors - 220 +220 > start_xen 92 100 +8 > alloc_heap_pages 1980 1744 -236 > xenheap_max_mfn 360 120 -240 > free_heap_pages 784 536 -248 > find_next_zero_bit 564 276 -288 > find_next_bit 548 260 -288 > find_first_zero_bit 444 148 -296 > find_first_bit 444 132 -312 > xmem_pool_free 1776 1440 -336 > __do_softirq 604 252 -352 > init_heap_pages 2328 1416 -912 > xmem_pool_alloc 2920 1596 -1324 > > > Andrew Cooper (12): > ppc/boot: Run constructors on boot > xen/bitops: Cleanup ahead of rearrangements > ARM/bitops: Change find_first_set_bit() to be a define > xen/page_alloc: Coerce min(flsl(), foo) expressions to being > unsigned > xen/bitops: Implement generic_f?sl() in lib/ > xen/bitops: Implement ffs() in common logic > x86/bitops: Improve arch_ffs() in the general case > xen/bitops: Implement ffsl() in common logic > xen/bitops: Replace find_first_set_bit() with ffsl() - 1 > xen/bitops: Delete find_first_set_bit() > xen/bitops: Clean up ffs64()/fls64() definitions > xen/bitops: Rearrange the top of xen/bitops.h > > Oleksii Kurochko (1): > xen/bitops: Implement fls()/flsl() in common logic > > xen/arch/arm/include/asm/arm32/bitops.h | 2 - > xen/arch/arm/include/asm/arm64/bitops.h | 12 -- > xen/arch/arm/include/asm/bitops.h | 35 +--- > xen/arch/ppc/include/asm/bitops.h | 17 +- > xen/arch/ppc/setup.c | 2 + > xen/arch/x86/guest/xen/xen.c | 4 +- > xen/arch/x86/hvm/dom0_build.c | 2 +- > xen/arch/x86/hvm/hpet.c | 8 +- > xen/arch/x86/include/asm/bitops.h | 114 +++-- > xen/arch/x86/include/asm/pt-contig-markers.h | 2 +- > xen/arch/x86/mm.c | 2 +- > xen/arch/x86/mm/p2m-pod.c | 4 +- > xen/common/Makefile | 1 + > xen/common/bitops.c | 89 +++ > xen/common/page_alloc.c | 6 +- > xen/common/softirq.c | 2 +- > xen/drivers/passthrough/amd/iommu_map.c | 2 +- > xen/drivers/passthrough/iommu.c | 4 +- > xen/drivers/passthrough/x86/iommu.c | 4 +- > xen/include/xen/bitops.h | 159 - > -- > xen/include/xen/boot-check.h | 60 +++ > xen/include/xen/compiler.h | 3 +- > xen/lib/Makefile | 2 + > xen/lib/generic-ffsl.c | 65 > xen/lib/generic-flsl.c | 68 > 25 files changed, 444 insertions(+), 225 deletions(-) > create mode 100644 xen/common/bitops.c > create mode 100644 xen/include/xen/boot-check.h > create mode 100644 xen/lib/generic-ffsl.c > create mode 100644 xen/lib/generic-flsl.c >
Re: [PATCH v10 02/14] xen: introduce generic non-atomic test_*bit()
On Thu, 2024-05-23 at 15:33 +0100, Julien Grall wrote: > > > > #include > > > > > > > > +/** > > > > + * generic__test_and_set_bit - Set a bit and return its old > > > > value > > > > + * @nr: Bit to set > > > > + * @addr: Address to count from > > > > + * > > > > + * This operation is non-atomic and can be reordered. > > > > + * If two examples of this operation race, one can appear to > > > > succeed > > > > + * but actually fail. You must protect multiple accesses with > > > > a > > > > lock. > > > > + */ > > > > > > Sorry for only mentioning this on v10. I think this comment > > > should be > > > duplicated (or moved to) on top of test_bit() because this is > > > what > > > everyone will use. This will avoid the developper to follow the > > > function > > > calls and only notice the x86 version which says "This function > > > is > > > atomic and may not be reordered." and would be wrong for all the > > > other arch. > > It makes sense to add this comment on top of test_bit(), but I am > > curious if it is needed to mention that for x86 arch_test_bit() "is > > atomic and may not be reordered": > > I would say no because any developper modifying common code can't > relying it. But won't then be confusion that if not generic implementation of test_bit() is chosen then test_bit() can be " atomic and cannot be reordered " ( as it is in case of x86 )? ~ Oleksii
Re: [PATCH v10 02/14] xen: introduce generic non-atomic test_*bit()
On Fri, 2024-05-24 at 09:35 +0200, Jan Beulich wrote: > On 24.05.2024 09:25, Oleksii K. wrote: > > On Fri, 2024-05-24 at 08:48 +0200, Jan Beulich wrote: > > > On 23.05.2024 18:40, Oleksii K. wrote: > > > > On Thu, 2024-05-23 at 15:33 +0100, Julien Grall wrote: > > > > > On 23/05/2024 15:11, Oleksii K. wrote: > > > > > > On Thu, 2024-05-23 at 14:00 +0100, Julien Grall wrote: > > > > > > > On 17/05/2024 14:54, Oleksii Kurochko wrote: > > > > > > > > diff --git a/xen/arch/arm/arm64/livepatch.c > > > > > > > > b/xen/arch/arm/arm64/livepatch.c > > > > > > > > index df2cebedde..4bc8ed9be5 100644 > > > > > > > > --- a/xen/arch/arm/arm64/livepatch.c > > > > > > > > +++ b/xen/arch/arm/arm64/livepatch.c > > > > > > > > @@ -10,7 +10,6 @@ > > > > > > > > #include > > > > > > > > #include > > > > > > > > > > > > > > > > -#include > > > > > > > > > > > > > > It is a bit unclear how this change is related to the > > > > > > > patch. > > > > > > > Can > > > > > > > you > > > > > > > explain in the commit message? > > > > > > Probably it doesn't need anymore. I will double check and > > > > > > if > > > > > > this > > > > > > change is not needed, I will just drop it in the next patch > > > > > > version. > > > > > > > > > > > > > > > > > > > > > #include > > > > > > > > #include > > > > > > > > #include > > > > > > > > diff --git a/xen/arch/arm/include/asm/bitops.h > > > > > > > > b/xen/arch/arm/include/asm/bitops.h > > > > > > > > index 5104334e48..8e16335e76 100644 > > > > > > > > --- a/xen/arch/arm/include/asm/bitops.h > > > > > > > > +++ b/xen/arch/arm/include/asm/bitops.h > > > > > > > > @@ -22,9 +22,6 @@ > > > > > > > > #define __set_bit(n,p) set_bit(n,p) > > > > > > > > #define __clear_bit(n,p) clear_bit(n,p) > > > > > > > > > > > > > > > > -#define BITOP_BITS_PER_WORD 32 > > > > > > > > -#define BITOP_MASK(nr) (1UL << ((nr) % > > > > > > > > BITOP_BITS_PER_WORD)) > > > > > > > > -#define BITOP_WORD(nr) ((nr) / > > > > > > > > BITOP_BITS_PER_WORD) > > > > > > > > #define BITS_PER_BYTE 8 > > > > > > > > > > > > > > OOI, any reason BITS_PER_BYTE has not been moved as well? > > > > > > > I > > > > > > > don't > > > > > > > expect > > > > > > > the value to change across arch. > > > > > > I can move it to generic one header too in the next patch > > > > > > version. > > > > > > > > > > > > > > > > > > > > [...] > > > > > > > > > > > > > > > diff --git a/xen/include/xen/bitops.h > > > > > > > > b/xen/include/xen/bitops.h > > > > > > > > index f14ad0d33a..6eeeff0117 100644 > > > > > > > > --- a/xen/include/xen/bitops.h > > > > > > > > +++ b/xen/include/xen/bitops.h > > > > > > > > @@ -65,10 +65,141 @@ static inline int > > > > > > > > generic_flsl(unsigned > > > > > > > > long > > > > > > > > x) > > > > > > > > * scope > > > > > > > > */ > > > > > > > > > > > > > > > > +#define BITOP_BITS_PER_WORD 32 > > > > > > > > +typedef uint32_t bitop_uint_t; > > > > > > > > + > > > > > > > > +#define BITOP_MASK(nr) ((bitop_uint_t)1 << ((nr) % > > > > > > > > BITOP_BITS_PER_WORD)) > > > > > > > > + > > > > > > > > +#define BITOP_WORD(nr) ((nr) / BITOP_BITS_PER_WORD) > > > > > > > > + > > > >
Re: [PATCH v10 02/14] xen: introduce generic non-atomic test_*bit()
On Fri, 2024-05-24 at 08:48 +0200, Jan Beulich wrote: > On 23.05.2024 18:40, Oleksii K. wrote: > > On Thu, 2024-05-23 at 15:33 +0100, Julien Grall wrote: > > > On 23/05/2024 15:11, Oleksii K. wrote: > > > > On Thu, 2024-05-23 at 14:00 +0100, Julien Grall wrote: > > > > > On 17/05/2024 14:54, Oleksii Kurochko wrote: > > > > > > diff --git a/xen/arch/arm/arm64/livepatch.c > > > > > > b/xen/arch/arm/arm64/livepatch.c > > > > > > index df2cebedde..4bc8ed9be5 100644 > > > > > > --- a/xen/arch/arm/arm64/livepatch.c > > > > > > +++ b/xen/arch/arm/arm64/livepatch.c > > > > > > @@ -10,7 +10,6 @@ > > > > > > #include > > > > > > #include > > > > > > > > > > > > -#include > > > > > > > > > > It is a bit unclear how this change is related to the patch. > > > > > Can > > > > > you > > > > > explain in the commit message? > > > > Probably it doesn't need anymore. I will double check and if > > > > this > > > > change is not needed, I will just drop it in the next patch > > > > version. > > > > > > > > > > > > > > > #include > > > > > > #include > > > > > > #include > > > > > > diff --git a/xen/arch/arm/include/asm/bitops.h > > > > > > b/xen/arch/arm/include/asm/bitops.h > > > > > > index 5104334e48..8e16335e76 100644 > > > > > > --- a/xen/arch/arm/include/asm/bitops.h > > > > > > +++ b/xen/arch/arm/include/asm/bitops.h > > > > > > @@ -22,9 +22,6 @@ > > > > > > #define __set_bit(n,p) set_bit(n,p) > > > > > > #define __clear_bit(n,p) clear_bit(n,p) > > > > > > > > > > > > -#define BITOP_BITS_PER_WORD 32 > > > > > > -#define BITOP_MASK(nr) (1UL << ((nr) % > > > > > > BITOP_BITS_PER_WORD)) > > > > > > -#define BITOP_WORD(nr) ((nr) / > > > > > > BITOP_BITS_PER_WORD) > > > > > > #define BITS_PER_BYTE 8 > > > > > > > > > > OOI, any reason BITS_PER_BYTE has not been moved as well? I > > > > > don't > > > > > expect > > > > > the value to change across arch. > > > > I can move it to generic one header too in the next patch > > > > version. > > > > > > > > > > > > > > [...] > > > > > > > > > > > diff --git a/xen/include/xen/bitops.h > > > > > > b/xen/include/xen/bitops.h > > > > > > index f14ad0d33a..6eeeff0117 100644 > > > > > > --- a/xen/include/xen/bitops.h > > > > > > +++ b/xen/include/xen/bitops.h > > > > > > @@ -65,10 +65,141 @@ static inline int > > > > > > generic_flsl(unsigned > > > > > > long > > > > > > x) > > > > > > * scope > > > > > > */ > > > > > > > > > > > > +#define BITOP_BITS_PER_WORD 32 > > > > > > +typedef uint32_t bitop_uint_t; > > > > > > + > > > > > > +#define BITOP_MASK(nr) ((bitop_uint_t)1 << ((nr) % > > > > > > BITOP_BITS_PER_WORD)) > > > > > > + > > > > > > +#define BITOP_WORD(nr) ((nr) / BITOP_BITS_PER_WORD) > > > > > > + > > > > > > +extern void __bitop_bad_size(void); > > > > > > + > > > > > > +#define bitop_bad_size(addr) (sizeof(*(addr)) < > > > > > > sizeof(bitop_uint_t)) > > > > > > + > > > > > > /* - Please tidy above here > > > > > > > > > > > > - > > > > > > */ > > > > > > > > > > > > #include > > > > > > > > > > > > +/** > > > > > > + * generic__test_and_set_bit - Set a bit and return its > > > > > > old > > > > > > value > > > > > > + * @nr: Bit to set > > > > > > + * @addr: Address to count from > > > > > > + * > > > > > > + * Thi
Re: [PATCH v10 02/14] xen: introduce generic non-atomic test_*bit()
On Thu, 2024-05-23 at 15:33 +0100, Julien Grall wrote: > > > On 23/05/2024 15:11, Oleksii K. wrote: > > On Thu, 2024-05-23 at 14:00 +0100, Julien Grall wrote: > > > Hi Oleksii, > > Hi Julien, > > > > > > > > On 17/05/2024 14:54, Oleksii Kurochko wrote: > > > > diff --git a/xen/arch/arm/arm64/livepatch.c > > > > b/xen/arch/arm/arm64/livepatch.c > > > > index df2cebedde..4bc8ed9be5 100644 > > > > --- a/xen/arch/arm/arm64/livepatch.c > > > > +++ b/xen/arch/arm/arm64/livepatch.c > > > > @@ -10,7 +10,6 @@ > > > > #include > > > > #include > > > > > > > > -#include > > > > > > It is a bit unclear how this change is related to the patch. Can > > > you > > > explain in the commit message? > > Probably it doesn't need anymore. I will double check and if this > > change is not needed, I will just drop it in the next patch > > version. > > > > > > > > > #include > > > > #include > > > > #include > > > > diff --git a/xen/arch/arm/include/asm/bitops.h > > > > b/xen/arch/arm/include/asm/bitops.h > > > > index 5104334e48..8e16335e76 100644 > > > > --- a/xen/arch/arm/include/asm/bitops.h > > > > +++ b/xen/arch/arm/include/asm/bitops.h > > > > @@ -22,9 +22,6 @@ > > > > #define __set_bit(n,p) set_bit(n,p) > > > > #define __clear_bit(n,p) clear_bit(n,p) > > > > > > > > -#define BITOP_BITS_PER_WORD 32 > > > > -#define BITOP_MASK(nr) (1UL << ((nr) % > > > > BITOP_BITS_PER_WORD)) > > > > -#define BITOP_WORD(nr) ((nr) / BITOP_BITS_PER_WORD) > > > > #define BITS_PER_BYTE 8 > > > > > > OOI, any reason BITS_PER_BYTE has not been moved as well? I don't > > > expect > > > the value to change across arch. > > I can move it to generic one header too in the next patch version. > > > > > > > > [...] > > > > > > > diff --git a/xen/include/xen/bitops.h > > > > b/xen/include/xen/bitops.h > > > > index f14ad0d33a..6eeeff0117 100644 > > > > --- a/xen/include/xen/bitops.h > > > > +++ b/xen/include/xen/bitops.h > > > > @@ -65,10 +65,141 @@ static inline int generic_flsl(unsigned > > > > long > > > > x) > > > > * scope > > > > */ > > > > > > > > +#define BITOP_BITS_PER_WORD 32 > > > > +typedef uint32_t bitop_uint_t; > > > > + > > > > +#define BITOP_MASK(nr) ((bitop_uint_t)1 << ((nr) % > > > > BITOP_BITS_PER_WORD)) > > > > + > > > > +#define BITOP_WORD(nr) ((nr) / BITOP_BITS_PER_WORD) > > > > + > > > > +extern void __bitop_bad_size(void); > > > > + > > > > +#define bitop_bad_size(addr) (sizeof(*(addr)) < > > > > sizeof(bitop_uint_t)) > > > > + > > > > /* - Please tidy above here > > > > - > > > > */ > > > > > > > > #include > > > > > > > > +/** > > > > + * generic__test_and_set_bit - Set a bit and return its old > > > > value > > > > + * @nr: Bit to set > > > > + * @addr: Address to count from > > > > + * > > > > + * This operation is non-atomic and can be reordered. > > > > + * If two examples of this operation race, one can appear to > > > > succeed > > > > + * but actually fail. You must protect multiple accesses with > > > > a > > > > lock. > > > > + */ > > > > > > Sorry for only mentioning this on v10. I think this comment > > > should be > > > duplicated (or moved to) on top of test_bit() because this is > > > what > > > everyone will use. This will avoid the developper to follow the > > > function > > > calls and only notice the x86 version which says "This function > > > is > > > atomic and may not be reordered." and would be wrong for all the > > > other arch. > > It makes sense to add this comment on top of test_bit(), but I am > > curious if it is needed to mention that for x86 arch_test_bit() "is > > atomic and may not be reordered": > > I would say no because any developp
Re: [PATCH v10 02/14] xen: introduce generic non-atomic test_*bit()
On Thu, 2024-05-23 at 14:00 +0100, Julien Grall wrote:
> Hi Oleksii,
Hi Julien,
>
> On 17/05/2024 14:54, Oleksii Kurochko wrote:
> > diff --git a/xen/arch/arm/arm64/livepatch.c
> > b/xen/arch/arm/arm64/livepatch.c
> > index df2cebedde..4bc8ed9be5 100644
> > --- a/xen/arch/arm/arm64/livepatch.c
> > +++ b/xen/arch/arm/arm64/livepatch.c
> > @@ -10,7 +10,6 @@
> > #include
> > #include
> >
> > -#include
>
> It is a bit unclear how this change is related to the patch. Can you
> explain in the commit message?
Probably it doesn't need anymore. I will double check and if this
change is not needed, I will just drop it in the next patch version.
>
> > #include
> > #include
> > #include
> > diff --git a/xen/arch/arm/include/asm/bitops.h
> > b/xen/arch/arm/include/asm/bitops.h
> > index 5104334e48..8e16335e76 100644
> > --- a/xen/arch/arm/include/asm/bitops.h
> > +++ b/xen/arch/arm/include/asm/bitops.h
> > @@ -22,9 +22,6 @@
> > #define __set_bit(n,p) set_bit(n,p)
> > #define __clear_bit(n,p) clear_bit(n,p)
> >
> > -#define BITOP_BITS_PER_WORD 32
> > -#define BITOP_MASK(nr) (1UL << ((nr) %
> > BITOP_BITS_PER_WORD))
> > -#define BITOP_WORD(nr) ((nr) / BITOP_BITS_PER_WORD)
> > #define BITS_PER_BYTE 8
>
> OOI, any reason BITS_PER_BYTE has not been moved as well? I don't
> expect
> the value to change across arch.
I can move it to generic one header too in the next patch version.
>
> [...]
>
> > diff --git a/xen/include/xen/bitops.h b/xen/include/xen/bitops.h
> > index f14ad0d33a..6eeeff0117 100644
> > --- a/xen/include/xen/bitops.h
> > +++ b/xen/include/xen/bitops.h
> > @@ -65,10 +65,141 @@ static inline int generic_flsl(unsigned long
> > x)
> > * scope
> > */
> >
> > +#define BITOP_BITS_PER_WORD 32
> > +typedef uint32_t bitop_uint_t;
> > +
> > +#define BITOP_MASK(nr) ((bitop_uint_t)1 << ((nr) %
> > BITOP_BITS_PER_WORD))
> > +
> > +#define BITOP_WORD(nr) ((nr) / BITOP_BITS_PER_WORD)
> > +
> > +extern void __bitop_bad_size(void);
> > +
> > +#define bitop_bad_size(addr) (sizeof(*(addr)) <
> > sizeof(bitop_uint_t))
> > +
> > /* - Please tidy above here -
> > */
> >
> > #include
> >
> > +/**
> > + * generic__test_and_set_bit - Set a bit and return its old value
> > + * @nr: Bit to set
> > + * @addr: Address to count from
> > + *
> > + * This operation is non-atomic and can be reordered.
> > + * If two examples of this operation race, one can appear to
> > succeed
> > + * but actually fail. You must protect multiple accesses with a
> > lock.
> > + */
>
> Sorry for only mentioning this on v10. I think this comment should be
> duplicated (or moved to) on top of test_bit() because this is what
> everyone will use. This will avoid the developper to follow the
> function
> calls and only notice the x86 version which says "This function is
> atomic and may not be reordered." and would be wrong for all the
> other arch.
It makes sense to add this comment on top of test_bit(), but I am
curious if it is needed to mention that for x86 arch_test_bit() "is
atomic and may not be reordered":
* This operation is non-atomic and can be reordered. ( Exception: for
* x86 arch_test_bit() is atomic and may not be reordered )
* If two examples of this operation race, one can appear to succeed
* but actually fail. You must protect multiple accesses with a lock.
*/
>
> > +static always_inline bool
> > +generic__test_and_set_bit(int nr, volatile void *addr)
> > +{
> > + bitop_uint_t mask = BITOP_MASK(nr);
> > + volatile bitop_uint_t *p = (volatile bitop_uint_t *)addr +
> > BITOP_WORD(nr);
> > + bitop_uint_t old = *p;
> > +
> > + *p = old | mask;
> > + return (old & mask);
> > +}
> > +
> > +/**
> > + * generic__test_and_clear_bit - Clear a bit and return its old
> > value
> > + * @nr: Bit to clear
> > + * @addr: Address to count from
> > + *
> > + * This operation is non-atomic and can be reordered.
> > + * If two examples of this operation race, one can appear to
> > succeed
> > + * but actually fail. You must protect multiple accesses with a
> > lock.
> > + */
>
> Same applies here and ...
>
> > +static always_inline bool
> > +generic__test_and_clear_bit(int nr, volatile void *addr)
> > +{
> > + bitop_uint_t mask = BITOP_MASK(nr);
> > + volatile bitop_uint_t *p = (volatile bitop_uint_t *)addr +
> > BITOP_WORD(nr);
> > + bitop_uint_t old = *p;
> > +
> > + *p = old & ~mask;
> > + return (old & mask);
> > +}
> > +
> > +/* WARNING: non atomic and it can be reordered! */
>
> ... here.
>
> > +static always_inline bool
> > +generic__test_and_change_bit(int nr, volatile void *addr)
> > +{
> > + bitop_uint_t mask = BITOP_MASK(nr);
> > + volatile bitop_uint_t *p = (volatile bitop_uint_t *)addr +
> > BITOP_WORD(nr);
> > + bitop_uint_t old = *p;
> > +
> > + *p = old ^ mask;
> > + return (old & mask);
> > +}
> > +/**
> > + * generic_test_bit -
Re: [PATCH] x86/shadow: don't leave trace record field uninitialized
On Wed, 2024-05-22 at 12:17 +0200, Jan Beulich wrote:
> The emulation_count field is set only conditionally right now.
> Convert
> all field setting to an initializer, thus guaranteeing that field to
> be
> set to 0 (default initialized) when GUEST_PAGING_LEVELS != 3.
>
> While there also drop the "event" local variable, thus eliminating an
> instance of the being phased out u32 type.
>
> Coverity ID: 1598430
> Fixes: 9a86ac1aa3d2 ("xentrace 5/7: Additional tracing for the shadow
> code")
> Signed-off-by: Jan Beulich
Release-acked-by: Oleksii Kurochko
~ Oleksii
>
> --- a/xen/arch/x86/mm/shadow/multi.c
> +++ b/xen/arch/x86/mm/shadow/multi.c
> @@ -2093,20 +2093,18 @@ static inline void trace_shadow_emulate(
> guest_l1e_t gl1e, write_val;
> guest_va_t va;
> uint32_t flags:29, emulation_count:3;
> - } d;
> - u32 event;
> -
> - event = TRC_SHADOW_EMULATE | ((GUEST_PAGING_LEVELS-2)<<8);
> -
> - d.gl1e = gl1e;
> - d.write_val.l1 = this_cpu(trace_emulate_write_val);
> - d.va = va;
> + } d = {
> + .gl1e = gl1e,
> + .write_val.l1 = this_cpu(trace_emulate_write_val),
> + .va = va,
> #if GUEST_PAGING_LEVELS == 3
> - d.emulation_count = this_cpu(trace_extra_emulation_count);
> + .emulation_count =
> this_cpu(trace_extra_emulation_count),
> #endif
> - d.flags = this_cpu(trace_shadow_path_flags);
> + .flags = this_cpu(trace_shadow_path_flags),
> + };
>
> - trace(event, sizeof(d), );
> + trace(TRC_SHADOW_EMULATE | ((GUEST_PAGING_LEVELS - 2) << 8),
> + sizeof(d), );
> }
> }
> #endif /* CONFIG_HVM */
Re: [for-4.19] Re: [XEN PATCH v3] arm/mem_access: add conditional build of mem_access.c
Hi Julien, On Wed, 2024-05-22 at 21:50 +0100, Julien Grall wrote: > Hi, > > Adding Oleksii as the release manager. > > On 22/05/2024 19:27, Tamas K Lengyel wrote: > > On Fri, May 10, 2024 at 8:32 AM Alessandro Zucchelli > > wrote: > > > > > > In order to comply to MISRA C:2012 Rule 8.4 for ARM the following > > > changes are done: > > > revert preprocessor conditional changes to xen/mem_access.h which > > > had it build unconditionally, add conditional build for > > > xen/mem_access.c > > > as well and provide stubs in asm/mem_access.h for the users of > > > this > > > header. > > > > > > Signed-off-by: Alessandro Zucchelli > > > > > > > Acked-by: Tamas K Lengyel > > Oleksii, would you be happy if this patch is committed for 4.19? Sure: Release-acked-by: Oleksii Kurochko > > BTW, do you want to be release-ack every bug until the hard code > freeze? > Or would you be fine to levea the decision to the maintainers? I would prefer to leave the decision to the maintainers. ~ Oleksii
Re: [PATCH v10 03/14] xen/bitops: implement fls{l}() in common logic
On Tue, 2024-05-21 at 13:18 +0200, Jan Beulich wrote:
> On 17.05.2024 15:54, Oleksii Kurochko wrote:
> > To avoid the compilation error below, it is needed to update to
> > places
> > in common/page_alloc.c where flsl() is used as now flsl() returns
> > unsigned int:
> >
> > ./include/xen/kernel.h:18:21: error: comparison of distinct pointer
> > types lacks a cast [-Werror]
> > 18 | (void) (&_x == &_y); \
> > | ^~
> > common/page_alloc.c:1843:34: note: in expansion of macro 'min'
> > 1843 | unsigned int inc_order = min(MAX_ORDER, flsl(e
> > - s) - 1);
> >
> > generic_fls{l} was used instead of __builtin_clz{l}(x) as if x is
> > 0,
> > the result in undefined.
> >
> > The prototype of the per-architecture fls{l}() functions was
> > changed to
> > return 'unsigned int' to align with the generic implementation of
> > these
> > functions and avoid introducing signed/unsigned mismatches.
> >
> > Signed-off-by: Oleksii Kurochko
> > ---
> > The patch is almost independent from Andrew's patch series
> > (
> > https://lore.kernel.org/xen-devel/20240313172716.2325427-1-andrew.coop...@citrix.com/T/#t
> > )
> > except test_fls() function which IMO can be merged as a separate
> > patch after Andrew's patch
> > will be fully ready.
>
> If there wasn't this dependency (I don't think it's "almost
> independent"),
> I'd be offering R-b with again one nit below.
Aren't all changes, except those in xen/common/bitops.c, independent? I
could move these changes in xen/common/bitops.c to a separate commit. I
think it is safe to commit them ( an introduction of common logic for
fls{l}() and tests ) separately since the CI tests have passed.
~ Oleksii
>
> > --- a/xen/arch/x86/include/asm/bitops.h
> > +++ b/xen/arch/x86/include/asm/bitops.h
> > @@ -425,20 +425,21 @@ static always_inline unsigned int
> > arch_ffsl(unsigned long x)
> > *
> > * This is defined the same way as ffs.
> > */
> > -static inline int flsl(unsigned long x)
> > +static always_inline unsigned int arch_flsl(unsigned long x)
> > {
> > - long r;
> > + unsigned long r;
> >
> > asm ( "bsr %1,%0\n\t"
> > "jnz 1f\n\t"
> > "mov $-1,%0\n"
> > "1:" : "=r" (r) : "rm" (x));
> > - return (int)r+1;
> > + return (unsigned int)r+1;
>
> Since you now touch this, you'd better tidy it at the same time:
>
> return r + 1;
>
> (i.e. style and no need for a cast).
>
> Jan
Re: [PATCH for-4.19 0/3] xen: Misc MISRA changes
Hi Andrew,
We can consider this patch series to be in Xen 4.19:
Release-acked-by: Oleksii Kurochko
~ Oleksii
On Tue, 2024-05-21 at 18:15 +0100, Andrew Cooper wrote:
> Misc fixes collected during today's call.
>
> Andrew Cooper (3):
> xen/lzo: Implement COPY{4,8} using memcpy()
> xen/x86: Drop useless non-Kconfig CONFIG_* variables
> xen/x86: Address two misc MISRA 17.7 violations
>
> xen/arch/x86/alternative.c | 4 ++--
> xen/arch/x86/include/asm/config.h | 4
> xen/arch/x86/nmi.c | 5 ++---
> xen/common/lzo.c | 11 ++-
> xen/include/xen/acpi.h | 9 -
> xen/include/xen/watchdog.h | 13 +
> 6 files changed, 7 insertions(+), 39 deletions(-)
>
>
> base-commit: 26b122e3bf8f3921d87312fbf5e7e13872ae92b0
Re: [PATCH for-4.19] x86/msi: prevent watchdog triggering when dumping MSI state
On Fri, 2024-05-17 at 15:56 +0200, Roger Pau Monne wrote: > Use the same check that's used in dump_irqs(). > > Signed-off-by: Roger Pau Monné > --- > xen/arch/x86/msi.c | 4 > 1 file changed, 4 insertions(+) > > diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c > index 19830528b65a..0c97fbb3fc03 100644 > --- a/xen/arch/x86/msi.c > +++ b/xen/arch/x86/msi.c > @@ -17,6 +17,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -1451,6 +1452,9 @@ static void cf_check dump_msi(unsigned char > key) > unsigned long flags; > const char *type = "???"; > > + if ( !(irq & 0x1f) ) > + process_pending_softirqs(); > + > if ( !irq_desc_initialized(desc) ) > continue; > Release-acked-by: Oleksii Kurochko ~ Oleksii
Re: [PATCH v9 03/15] xen/bitops: implement fls{l}() in common logic
On Wed, 2024-05-15 at 11:09 +0200, Jan Beulich wrote:
> But this then needs carrying through to ...
>
> > --- a/xen/arch/arm/include/asm/arm64/bitops.h
> > +++ b/xen/arch/arm/include/asm/arm64/bitops.h
> > @@ -22,17 +22,15 @@ static /*__*/always_inline unsigned long
> > __ffs(unsigned long word)
> > */
> > #define ffz(x) __ffs(~(x))
> >
> > -static inline int flsl(unsigned long x)
> > +static inline int arch_flsl(unsigned long x)
>
> ... e.g. here. You don't want to introduce signed/unsigned
> mismatches.
Is it critical for x86 to return int for flsl() and fls() or I can
update the return type for x86 too?
static always_inline int arch_flsl(unsigned long x)
{
long r;
asm ( "bsr %1,%0\n\t"
"jnz 1f\n\t"
"mov $-1,%0\n"
"1:" : "=r" (r) : "rm" (x));
return (int)r+1;
}
#define arch_flsl arch_flsl
static always_inline int arch_fls(unsigned int x)
{
int r;
asm ( "bsr %1,%0\n\t"
"jnz 1f\n\t"
"mov $-1,%0\n"
"1:" : "=r" (r) : "rm" (x));
return r + 1;
}
#define arch_fls arch_fls
Any specific reason why 'long' and 'int' types for r are used?
~ Oleksii
Re: [PATCH v9 02/15] xen: introduce generic non-atomic test_*bit()
On Thu, 2024-05-16 at 12:49 +0200, Jan Beulich wrote:
> > Anyway, I am not sure I understand which approach I should use in
> > this
> > patch. You mentioned that possibly test_and_() can't have a generic
> > form, meaning it won't be a set of arch_test_and_() functions.
> >
> > So, can I rename arch__test_() and generic__test_() to arch_test_()
> > and
> > generic_test_(), respectively, and use the renamed functions in
> > _test_and*() in xen/bitops.h? Is my understanding correct?
>
> You could. You could also stick to what you have now - as said, I can
> accept that with the worked out explanation. Or you could switch to
> using arch__test_bit() and generic__test_bit(), thus having the
> double
> inner underscores identify "internal to the implementation"
> functions.
> My preference would be in backwards order of what I have just
> enumerated
> as possible options. I wonder whether really no-one else has any
> opinion
> here ...
I see that __test_bit() doesn't exist now and perhaps won't exist at
all, but in this patch we are providing the generic for test_bit(), not
__test_bit(). Thereby according to provided by me naming for test_bit()
should be defined using {generic, arch}_test_bit().
~ Oleksii
Re: [XEN PATCH 0/4] address violations of MISRA C Rule 20.7
On Thu, 2024-05-16 at 18:08 +0200, Jan Beulich wrote: > > > for 4.18 we took a relaxed approach towards (simple) changes for > > > Misra purposes. > > > I wonder whether you mean to permit the same for 4.19, or whether > > > series like > > > this one rather want/need delaying until after branching. > > Lets follow the same approach for 4.19. > > Well, okay. But if you don't say now until when this is okay, you'll > need to announce the "stop" very prominently later on, so no-one > misses it. For me it is okay until we don't have Hard Code Release deadline. ~ Oleksii
Re: [PATCH] Revert "evtchn: refuse EVTCHNOP_status for Xen-bound event channels"
On Tue, 2024-05-14 at 12:13 +0100, Julien Grall wrote: > Hi, > > (+ Oleksii as the release manager) > > Chiming into the discussion as there seems there is disagreement. > > On 14/05/2024 11:03, Jan Beulich wrote: > > On 14.05.2024 11:51, Andrew Cooper wrote: > > > On 14/05/2024 10:25 am, Jan Beulich wrote: > > > > On 03.04.2024 08:16, Jan Beulich wrote: > > > > > On 02.04.2024 19:06, Andrew Cooper wrote: > > > > > > The commit makes a claim without any kind of justification. > > > > > Well, what does "have no business" leave open? > > > > > > > > > > > The claim is false, and the commit broke lsevtchn in dom0. > > > > > Or alternatively lsevtchn was doing something that was never > > > > > meant to work > > > > > (from Xen's perspective). > > > > > > > > > > > It is also quite > > > > > > obvious from XSM_TARGET that it has broken device model > > > > > > stubdoms too. > > > > > Why would that be "obvious"? What business would a stubdom > > > > > have to look at > > > > > Xen's side of an evtchn? > > > > > > > > > > > Whether to return information about a xen-owned evtchn is a > > > > > > matter of policy, > > > > > > and it's not acceptable to short circuit the XSM on the > > > > > > matter. > > > > > I can certainly accept this as one possible view point. As in > > > > > so many cases > > > > > I'm afraid I dislike you putting it as if it was the only > > > > > possible one. > > > > > > > > > > In summary: The supposed justification you claim is missing > > > > > in the original > > > > > change is imo also missing here then: What business would any > > > > > entity in the > > > > > system have to look at Xen's side of an event channel? Back > > > > > at the time, 3 > > > > > people agreed that it's "none". > > > > You've never responded to this reply of mine, or its follow-up. > > > > You also > > > > didn't chime in on the discussion Daniel and I were having. I > > > > consider my > > > > objections unaddressed, and in fact I continue to consider the > > > > change to > > > > be wrong. Therefore it was inappropriate for you to commit it; > > > > it needs > > > > reverting asap. If you're not going to do so, I will. > > > > > > You tried defending breaking a utility with "well it shouldn't > > > exist then". > > > > > > You don't have a leg to stand on, and two maintainers of relevant > > > subsystems here just got tired of bullshit being presented in > > > place of > > > any credible argument for having done the change in the way you > > > did. > > > > Please can you finally get into the habit of not sending rude > > replies? > > > > > The correct response was "Sorry I broke things. Lets revert this > > > for > > > now to unbreak, and I'll see about reworking it to not > > > intentionally > > > subvert Xen's security mechanism". > > > > I'm sorry, but I didn't break things. I made things more consistent > > with > > the earlier change, as pointed out before: With your revert, > > evtchn_status() is now (again) inconsistent with e.g. > > evtchn_send(). If > > you were serious about this being something that needs leaving to > > XSM, > > you'd have adjusted such further uses of consumer_is_xen() as well. > > But > > you aren't. You're merely insisting on lsevtchn needing to continue > > to > > work in a way it should never have worked, with a patch to improve > > the > > situation already pending. > > > > Just to state a very basic principle here again: Xen-internal event > > channels ought to either be fully under XSM control when it comes > > to > > domains attempting to access them (in whichever way), or they > > should > > truly be Xen-internal, with access uniformly prevented. To me the > > former option simply makes very little sense. > > I agree we need consistency on how we handle security policy event > channel. Although, I don't have a strong opinion on which way to go. > > For the commit message, it is not entirely clear what "broke > lseventch > in dom0" really mean. Is it lsevtchn would not stop or it will just > not > display the event channel? > > If the former, isn't a sign that the tool needs to be harden a bit > more? > If the latter, then I would argue that consistency for the XSM policy > is > more important than displaying the event channel for now (the patch > was > also committed 3 years ago...). > > So I would vote for a revert and, if desired, replacing with a patch > that would change the XSM policy consistently. Alternatively, the > consistency should be a blocker for Xen 4.19. Sorry for the delayed response. I am not deeply familiar with the technical details surrounding XSM, but if I understand Daniel's point correctly, the original change violates the access control framework. This suggests to me that the revert should be merged. However, I have a question: if we merge this revert, does it mean that using channels a user ( domain ) will be able to get information about certain events such as EVTCHNSTAT_unbound, EVTCHNSTAT_interdomain,
Re: [PATCH for-4.19] tools/xentop: fix cpu% sort order
On Tue, 2024-05-14 at 14:52 +0100, Andrew Cooper wrote:
> On 14/05/2024 1:36 pm, Leigh Brown wrote:
> > Hello,
> >
> > On 2024-05-14 13:07, Andrew Cooper wrote:
> > > On 14/05/2024 9:13 am, Leigh Brown wrote:
> > > > Although using integer comparison to compare doubles kind of
> > > > works, it's annoying to see domains slightly out of order when
> > > > sorting by cpu%.
> > > >
> > > > Add a compare_dbl() function and update compare_cpu_pct() to
> > > > call it.
> > > >
> > > > Signed-off-by: Leigh Brown
> > > > ---
> > > > tools/xentop/xentop.c | 13 -
> > > > 1 file changed, 12 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/tools/xentop/xentop.c b/tools/xentop/xentop.c
> > > > index 545bd5e96d..99199caec9 100644
> > > > --- a/tools/xentop/xentop.c
> > > > +++ b/tools/xentop/xentop.c
> > > > @@ -85,6 +85,7 @@ static void set_delay(const char *value);
> > > > static void set_prompt(const char *new_prompt, void
> > > > (*func)(const
> > > > char *));
> > > > static int handle_key(int);
> > > > static int compare(unsigned long long, unsigned long long);
> > > > +static int compare_dbl(double, double);
> > > > static int compare_domains(xenstat_domain **, xenstat_domain
> > > > **);
> > > > static unsigned long long tot_net_bytes( xenstat_domain *,
> > > > int);
> > > > static bool tot_vbd_reqs(xenstat_domain *, int, unsigned long
> > > > long *);
> > > > @@ -422,6 +423,16 @@ static int compare(unsigned long long i1,
> > > > unsigned long long i2)
> > > > return 0;
> > > > }
> > > >
> > > > +/* Compares two double precision numbers, returning -1,0,1 for
> > > > <,=,> */
> > > > +static int compare_dbl(double d1, double d2)
> > > > +{
> > > > + if(d1 < d2)
> > > > + return -1;
> > > > + if(d1 > d2)
> > > > + return 1;
> > > > + return 0;
> > > > +}
> > > > +
> > > > /* Comparison function for use with qsort. Compares two
> > > > domains
> > > > using the
> > > > * current sort field. */
> > > > static int compare_domains(xenstat_domain **domain1,
> > > > xenstat_domain
> > > > **domain2)
> > > > @@ -523,7 +534,7 @@ static double get_cpu_pct(xenstat_domain
> > > > *domain)
> > > >
> > > > static int compare_cpu_pct(xenstat_domain *domain1,
> > > > xenstat_domain
> > > > *domain2)
> > > > {
> > > > - return -compare(get_cpu_pct(domain1),
> > > > get_cpu_pct(domain2));
> > > > + return -compare_dbl(get_cpu_pct(domain1),
> > > > get_cpu_pct(domain2));
> > >
> > > Oh, we were doing an implicit double->unsigned long long
> > > conversion.
> > > Over the range 0.0 to 100.0, that ought to work as expected.
> > > What kind
> > > of out-of-order are you seeing?
> >
> > Without patch:
> >
> > xentop - 13:29:01 Xen 4.18.2
> > 13 domains: 1 running, 12 blocked, 0 paused, 0 crashed, 0 dying, 0
> > shutdown
> > Mem: 67030640k total, 33097800k used, 33932840k free CPUs: 24 @
> > 3693MHz
> > NAME STATE CPU(sec) CPU(%) MEM(k) MEM(%) MAXMEM(k)
> > MAXMEM(%)
> > icecream --b--- 2597 6.6 4194368 6.3
> > 4195328 6.3
> > xendd --b--- 4016 5.4 524268 0.8
> > 525312 0.8
> > Domain-0 -r 1059 1.7 1048576 1.6
> > 1048576 1.6
> > neon --b--- 826 1.1 2097216 3.1
> > 2098176 3.1
> > blender --b--- 121 0.2 1048640 1.6
> > 1049600 1.6
> > bread --b--- 69 0.1 524352 0.8
> > 525312 0.8
> > bob --b--- 502 0.3 16777284 25.0
> > 16778240 25.0
> > cheese --b--- 225 0.5 1048384 1.6
> > 1049600 1.6
> > cassini --b--- 489 0.4 3145792 4.7
> > 3146752 4.7
> > chickpea --b--- 67 0.1 524352 0.8
> > 525312 0.8
> > lentil --b--- 67 0.1 262208 0.4
> > 263168 0.4
> > fusilli --b--- 159 0.2 524352 0.8
> > 525312 0.8
> > pizza --b--- 359 0.5 524352 0.8
> > 525312 0.8
> >
> > With patch:
> >
> > xentop - 13:30:17 Xen 4.18.2
> > 13 domains: 1 running, 12 blocked, 0 paused, 0 crashed, 0 dying, 0
> > shutdown
> > Mem: 67030640k total, 33097788k used, 33932852k free CPUs: 24 @
> > 3693MHz
> > NAME STATE CPU(sec) CPU(%) MEM(k) MEM(%) MAXMEM(k)
> > MAXMEM(%)
> > xendd --b--- 4020 5.7 524268 0.8
> > 525312 0.8
> > icecream --b--- 2600 3.8 4194368 6.3
> > 4195328 6.3
> > Domain-0 -r 1060 1.5 1048576 1.6
> > 1048576 1.6
> > neon --b--- 827 1.1 2097216 3.1
> > 2098176 3.1
> > cheese --b--- 225 0.7 1048384 1.6
> > 1049600 1.6
> > pizza --b--- 359 0.5 524352 0.8
> > 525312 0.8
> > cassini --b--- 490 0.4 3145792 4.7
> > 3146752 4.7
> > fusilli --b--- 159
Re: [XEN PATCH v7 1/5] xen/vpci: Clear all vpci status of device
On Wed, 2024-05-15 at 10:27 -0400, Stewart Hildebrand wrote: > On 4/18/24 23:53, Jiqian Chen wrote: > > When a device has been reset on dom0 side, the vpci on Xen > > side won't get notification, so the cached state in vpci is > > all out of date compare with the real device state. > > To solve that problem, add a new hypercall to clear all vpci > > device state. When the state of device is reset on dom0 side, > > dom0 can call this hypercall to notify vpci. > > > > Signed-off-by: Huang Rui > > Signed-off-by: Jiqian Chen > > Reviewed-by: Stewart Hildebrand > > Reviewed-by: Stefano Stabellini > > Could we consider this patch for 4.19? It's independent of the rest > of > this series, and it fixes a real issue observed on both Arm and x86. > The > Linux counterpart has already been merged in linux-next [0]. > > [0] > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=next-20240515=b272722511d5e8ae580f01830687b8a6b2717f01 Sure! Fixes should be merged. Release-acked-by: Oleksii Kurochko ~ Oleksii
Re: [PATCH for-4.19?] xen/x86: pretty print interrupt CPU affinity masks
On Wed, 2024-05-15 at 16:30 +0100, Andrew Cooper wrote: > On 15/05/2024 4:29 pm, Roger Pau Monne wrote: > > Print the CPU affinity masks as numeric ranges instead of plain > > hexadecimal > > bitfields. > > > > Signed-off-by: Roger Pau Monné > > Ha - I was going to write exactly the same patch, but you beat me to > it. > > Acked-by: Andrew Cooper Looks good to me for having in Xen 4.19 release. Release-acked-by: Oleksii Kurochko ~ Oleksii
Re: [XEN PATCH 0/4] address violations of MISRA C Rule 20.7
On Wed, 2024-05-15 at 09:48 +0200, Jan Beulich wrote:
> Oleksii,
>
> On 15.05.2024 09:34, Nicola Vetrini wrote:
> > Hi all,
> >
> > this series aims to refactor some macros that cause violations of
> > MISRA C Rule
> > 20.7 ("Expressions resulting from the expansion of macro parameters
> > shall be
> > enclosed in parentheses"). All the macros touched by these patches
> > are in some
> > way involved in violations, and the strategy adopted to bring them
> > into
> > compliance is to add parentheses around macro arguments where
> > needed.
> >
> > Nicola Vetrini (4):
> > x86/vpmu: address violations of MISRA C Rule 20.7
> > x86/hvm: address violations of MISRA C Rule 20.7
> > x86_64/uaccess: address violations of MISRA C Rule 20.7
> > x86_64/cpu_idle: address violations of MISRA C Rule 20.7
>
> for 4.18 we took a relaxed approach towards (simple) changes for
> Misra purposes.
> I wonder whether you mean to permit the same for 4.19, or whether
> series like
> this one rather want/need delaying until after branching.
Lets follow the same approach for 4.19.
Sorry for delayed answer.
~ Oleksii
>
> Jan
>
> > xen/arch/x86/cpu/vpmu_amd.c | 4 ++--
> > xen/arch/x86/hvm/mtrr.c | 2 +-
> > xen/arch/x86/hvm/rtc.c | 2 +-
> > xen/arch/x86/include/asm/hvm/save.h | 2 +-
> > xen/arch/x86/include/asm/x86_64/uaccess.h | 7 ---
> > xen/arch/x86/x86_64/cpu_idle.c | 2 +-
> > 6 files changed, 10 insertions(+), 9 deletions(-)
> >
>
Feature Freeze Deadline Extended to May 24
Hello everyone, Since there were no objections to extending the Feature Freeze Deadline Proposal, I would like to inform you that the deadline has been extended to May 24. At the moment, I don't see any reason to extend other deadlines, so they will remain the same. Have a nice day! Best regards, Oleksii
Re: [PATCH v9 02/15] xen: introduce generic non-atomic test_*bit()
On Thu, 2024-05-16 at 09:04 +0200, Jan Beulich wrote:
> On 15.05.2024 19:03, Oleksii K. wrote:
> > On Wed, 2024-05-15 at 17:41 +0200, Jan Beulich wrote:
> > > On 15.05.2024 17:29, Oleksii K. wrote:
> > > > On Wed, 2024-05-15 at 10:52 +0200, Jan Beulich wrote:
> > > > > On 06.05.2024 12:15, Oleksii Kurochko wrote:
> > > > > > The following generic functions were introduced:
> > > > > > * test_bit
> > > > > > * generic__test_and_set_bit
> > > > > > * generic__test_and_clear_bit
> > > > > > * generic__test_and_change_bit
> > > > > >
> > > > > > Also, the patch introduces the following generics which are
> > > > > > used by the functions mentioned above:
> > > > > > * BITOP_BITS_PER_WORD
> > > > > > * BITOP_MASK
> > > > > > * BITOP_WORD
> > > > > > * BITOP_TYPE
> > > > > >
> > > > > > These functions and macros can be useful for architectures
> > > > > > that don't have corresponding arch-specific instructions.
> > > > >
> > > > > Logically this paragraph may better move ahead of the BITOP_*
> > > > > one.
> > > > >
> > > > > > Because of that x86 has the following check in the macros
> > > > > > test_bit(),
> > > > > > __test_and_set_bit(), __test_and_clear_bit(),
> > > > > > __test_and_change_bit():
> > > > > > if ( bitop_bad_size(addr) ) __bitop_bad_size();
> > > > > > It was necessary to make bitop bad size check generic too,
> > > > > > so
> > > > > > arch_check_bitop_size() was introduced.
> > > > >
> > > > > Not anymore, with the most recent adjustments? There's
> > > > > nothing
> > > > > arch-
> > > > > specific anymore in the checking.
> > > > >
> > > > > > @@ -183,7 +180,7 @@ static inline int test_and_set_bit(int
> > > > > > nr,
> > > > > > volatile void *addr)
> > > > > > * If two examples of this operation race, one can appear
> > > > > > to
> > > > > > succeed
> > > > > > * but actually fail. You must protect multiple accesses
> > > > > > with
> > > > > > a
> > > > > > lock.
> > > > > > */
> > > > > > -static inline int __test_and_set_bit(int nr, void *addr)
> > > > > > +static inline int arch__test_and_set_bit(int nr, volatile
> > > > > > void
> > > > > > *addr)
> > > > >
> > > > > I think I raised this point before: Why arch__ here, ...
> > > > >
> > > > > > @@ -232,7 +226,7 @@ static inline int
> > > > > > test_and_clear_bit(int
> > > > > > nr,
> > > > > > volatile void *addr)
> > > > > > * If two examples of this operation race, one can appear
> > > > > > to
> > > > > > succeed
> > > > > > * but actually fail. You must protect multiple accesses
> > > > > > with
> > > > > > a
> > > > > > lock.
> > > > > > */
> > > > > > -static inline int __test_and_clear_bit(int nr, void *addr)
> > > > > > +static inline int arch__test_and_clear_bit(int nr,
> > > > > > volatile
> > > > > > void
> > > > > > *addr)
> > > > >
> > > > > ... here, ...
> > > > >
> > > > > > @@ -243,13 +237,10 @@ static inline int
> > > > > > __test_and_clear_bit(int
> > > > > > nr, void *addr)
> > > > > >
> > > > > > return oldbit;
> > > > > > }
> > > > > > -#define __test_and_clear_bit(nr, addr) ({ \
> > > > > > - if ( bitop_bad_size(addr) ) __bitop_bad_size(); \
> > > > > > - __test_and_clear_bit(nr, addr); \
> > > > > > -})
> > > > > > +#define arch__test_and_clear_bit arch__test_and_clear_bit
> > > > > >
> > > > > > /* WARNING: non atomic and it can be reordered! */
> > > > > > -static inline int __test_and_change_bit(int nr, void
> &
Re: [PATCH v9 02/15] xen: introduce generic non-atomic test_*bit()
On Wed, 2024-05-15 at 17:41 +0200, Jan Beulich wrote:
> On 15.05.2024 17:29, Oleksii K. wrote:
> > On Wed, 2024-05-15 at 10:52 +0200, Jan Beulich wrote:
> > > On 06.05.2024 12:15, Oleksii Kurochko wrote:
> > > > The following generic functions were introduced:
> > > > * test_bit
> > > > * generic__test_and_set_bit
> > > > * generic__test_and_clear_bit
> > > > * generic__test_and_change_bit
> > > >
> > > > Also, the patch introduces the following generics which are
> > > > used by the functions mentioned above:
> > > > * BITOP_BITS_PER_WORD
> > > > * BITOP_MASK
> > > > * BITOP_WORD
> > > > * BITOP_TYPE
> > > >
> > > > These functions and macros can be useful for architectures
> > > > that don't have corresponding arch-specific instructions.
> > >
> > > Logically this paragraph may better move ahead of the BITOP_*
> > > one.
> > >
> > > > Because of that x86 has the following check in the macros
> > > > test_bit(),
> > > > __test_and_set_bit(), __test_and_clear_bit(),
> > > > __test_and_change_bit():
> > > > if ( bitop_bad_size(addr) ) __bitop_bad_size();
> > > > It was necessary to make bitop bad size check generic too, so
> > > > arch_check_bitop_size() was introduced.
> > >
> > > Not anymore, with the most recent adjustments? There's nothing
> > > arch-
> > > specific anymore in the checking.
> > >
> > > > @@ -183,7 +180,7 @@ static inline int test_and_set_bit(int nr,
> > > > volatile void *addr)
> > > > * If two examples of this operation race, one can appear to
> > > > succeed
> > > > * but actually fail. You must protect multiple accesses with
> > > > a
> > > > lock.
> > > > */
> > > > -static inline int __test_and_set_bit(int nr, void *addr)
> > > > +static inline int arch__test_and_set_bit(int nr, volatile void
> > > > *addr)
> > >
> > > I think I raised this point before: Why arch__ here, ...
> > >
> > > > @@ -232,7 +226,7 @@ static inline int test_and_clear_bit(int
> > > > nr,
> > > > volatile void *addr)
> > > > * If two examples of this operation race, one can appear to
> > > > succeed
> > > > * but actually fail. You must protect multiple accesses with
> > > > a
> > > > lock.
> > > > */
> > > > -static inline int __test_and_clear_bit(int nr, void *addr)
> > > > +static inline int arch__test_and_clear_bit(int nr, volatile
> > > > void
> > > > *addr)
> > >
> > > ... here, ...
> > >
> > > > @@ -243,13 +237,10 @@ static inline int
> > > > __test_and_clear_bit(int
> > > > nr, void *addr)
> > > >
> > > > return oldbit;
> > > > }
> > > > -#define __test_and_clear_bit(nr, addr) ({ \
> > > > - if ( bitop_bad_size(addr) ) __bitop_bad_size(); \
> > > > - __test_and_clear_bit(nr, addr); \
> > > > -})
> > > > +#define arch__test_and_clear_bit arch__test_and_clear_bit
> > > >
> > > > /* WARNING: non atomic and it can be reordered! */
> > > > -static inline int __test_and_change_bit(int nr, void *addr)
> > > > +static inline int arch__test_and_change_bit(int nr, volatile
> > > > void
> > > > *addr)
> > >
> > > ... and here, while ...
> > >
> > > > @@ -307,8 +295,7 @@ static inline int variable_test_bit(int nr,
> > > > const volatile void *addr)
> > > > return oldbit;
> > > > }
> > > >
> > > > -#define test_bit(nr, addr) ({ \
> > > > - if ( bitop_bad_size(addr) ) __bitop_bad_size(); \
> > > > +#define arch_test_bit(nr, addr) ({ \
> > >
> > > ... just arch_ here? I don't like the double underscore infixes
> > > very
> > > much, but I'm okay with them as long as they're applied
> > > consistently.
> >
> > Common code and x86 use __test_and_clear_bit(), and this patch
> > provides
> > a generic version of __test_and_clear_bit(). To emphasize that
> > generic__test_and_clear_bit() is a common implementation of
> > __test_and_clear_bit(), the double underscore was retained. Als
Re: [PATCH v9 03/15] xen/bitops: implement fls{l}() in common logic
On Wed, 2024-05-15 at 16:07 +0200, Jan Beulich wrote: > On 15.05.2024 15:55, Oleksii K. wrote: > > On Wed, 2024-05-15 at 11:09 +0200, Jan Beulich wrote: > > > On 06.05.2024 12:15, Oleksii Kurochko wrote: > > > > Changes in V9: > > > > - update return type of fls and flsl() to unsigned int to be > > > > aligned with other > > > > bit ops. > > > > > > But this then needs carrying through to ... > > > > > > > --- a/xen/arch/arm/include/asm/arm64/bitops.h > > > > +++ b/xen/arch/arm/include/asm/arm64/bitops.h > > > > @@ -22,17 +22,15 @@ static /*__*/always_inline unsigned long > > > > __ffs(unsigned long word) > > > > */ > > > > #define ffz(x) __ffs(~(x)) > > > > > > > > -static inline int flsl(unsigned long x) > > > > +static inline int arch_flsl(unsigned long x) > > > > > > ... e.g. here. You don't want to introduce signed/unsigned > > > mismatches. > > Do you mean that generic flsl() uses 'unsigned int' as a return > > type, > > but arch_flsl continue to use 'int'? > > Yes. > > > > Also why do you keep "inline" here, while ... > > > > > > > --- a/xen/arch/x86/include/asm/bitops.h > > > > +++ b/xen/arch/x86/include/asm/bitops.h > > > > @@ -425,7 +425,7 @@ static always_inline unsigned int > > > > arch_ffsl(unsigned long x) > > > > * > > > > * This is defined the same way as ffs. > > > > */ > > > > -static inline int flsl(unsigned long x) > > > > +static always_inline int arch_flsl(unsigned long x) > > > > > > ... you switch to always_inline here? > > Because Adnrew's patch with bitops.h for x86 changes to > > always_inline, > > so to be consistent, at least, for architecture. > > And why not extend this to Arm? No specific reason, just wanted to do minimal amount of necessary changes. I'll do that in the the next patch version. ~ Oleksii
Re: [PATCH v9 02/15] xen: introduce generic non-atomic test_*bit()
On Wed, 2024-05-15 at 10:52 +0200, Jan Beulich wrote:
> On 06.05.2024 12:15, Oleksii Kurochko wrote:
> > The following generic functions were introduced:
> > * test_bit
> > * generic__test_and_set_bit
> > * generic__test_and_clear_bit
> > * generic__test_and_change_bit
> >
> > Also, the patch introduces the following generics which are
> > used by the functions mentioned above:
> > * BITOP_BITS_PER_WORD
> > * BITOP_MASK
> > * BITOP_WORD
> > * BITOP_TYPE
> >
> > These functions and macros can be useful for architectures
> > that don't have corresponding arch-specific instructions.
>
> Logically this paragraph may better move ahead of the BITOP_* one.
>
> > Because of that x86 has the following check in the macros
> > test_bit(),
> > __test_and_set_bit(), __test_and_clear_bit(),
> > __test_and_change_bit():
> > if ( bitop_bad_size(addr) ) __bitop_bad_size();
> > It was necessary to make bitop bad size check generic too, so
> > arch_check_bitop_size() was introduced.
>
> Not anymore, with the most recent adjustments? There's nothing arch-
> specific anymore in the checking.
>
> > @@ -183,7 +180,7 @@ static inline int test_and_set_bit(int nr,
> > volatile void *addr)
> > * If two examples of this operation race, one can appear to
> > succeed
> > * but actually fail. You must protect multiple accesses with a
> > lock.
> > */
> > -static inline int __test_and_set_bit(int nr, void *addr)
> > +static inline int arch__test_and_set_bit(int nr, volatile void
> > *addr)
>
> I think I raised this point before: Why arch__ here, ...
>
> > @@ -232,7 +226,7 @@ static inline int test_and_clear_bit(int nr,
> > volatile void *addr)
> > * If two examples of this operation race, one can appear to
> > succeed
> > * but actually fail. You must protect multiple accesses with a
> > lock.
> > */
> > -static inline int __test_and_clear_bit(int nr, void *addr)
> > +static inline int arch__test_and_clear_bit(int nr, volatile void
> > *addr)
>
> ... here, ...
>
> > @@ -243,13 +237,10 @@ static inline int __test_and_clear_bit(int
> > nr, void *addr)
> >
> > return oldbit;
> > }
> > -#define __test_and_clear_bit(nr, addr) ({ \
> > - if ( bitop_bad_size(addr) ) __bitop_bad_size(); \
> > - __test_and_clear_bit(nr, addr); \
> > -})
> > +#define arch__test_and_clear_bit arch__test_and_clear_bit
> >
> > /* WARNING: non atomic and it can be reordered! */
> > -static inline int __test_and_change_bit(int nr, void *addr)
> > +static inline int arch__test_and_change_bit(int nr, volatile void
> > *addr)
>
> ... and here, while ...
>
> > @@ -307,8 +295,7 @@ static inline int variable_test_bit(int nr,
> > const volatile void *addr)
> > return oldbit;
> > }
> >
> > -#define test_bit(nr, addr) ({ \
> > - if ( bitop_bad_size(addr) ) __bitop_bad_size(); \
> > +#define arch_test_bit(nr, addr) ({ \
>
> ... just arch_ here? I don't like the double underscore infixes very
> much, but I'm okay with them as long as they're applied consistently.
Common code and x86 use __test_and_clear_bit(), and this patch provides
a generic version of __test_and_clear_bit(). To emphasize that
generic__test_and_clear_bit() is a common implementation of
__test_and_clear_bit(), the double underscore was retained. Also,
test_and_clear_bit() exists and if one day it will be needed to provide
a generic version of it, then it will be needed to have
generic__test_and_clear_bit() and generic_test_and_clear_bit()
A similar logic was chosen for test_bit.
>
> > --- a/xen/include/xen/bitops.h
> > +++ b/xen/include/xen/bitops.h
> > @@ -65,10 +65,144 @@ static inline int generic_flsl(unsigned long
> > x)
> > * scope
> > */
> >
> > +#define BITOP_MASK(nr) ((bitop_uint_t)1 << ((nr) %
> > BITOP_BITS_PER_WORD))
> > +
> > +#define BITOP_WORD(nr) ((nr) / BITOP_BITS_PER_WORD)
> > +
> > +extern void __bitop_bad_size(void);
> > +
> > /* - Please tidy above here --
> > --- */
> >
> > #include
> >
> > +#ifndef arch_check_bitop_size
> > +
> > +#define bitop_bad_size(addr) sizeof(*(addr)) <
> > sizeof(bitop_uint_t)
>
> Nit: Missing parentheses around the whole expression.
>
> > +#define arch_check_bitop_size(addr) \
> > + if ( bitop_bad_size(addr) ) __bitop_bad_size();
>
> Apart from the arch_ prefix that now wants dropping, this macro (if
> we
> want one in the first place)
What do you mean by 'want' here? I thought it is pretty necessary from
safety point of view to have this check.
Except arch_ prefix does it make sense to drop "#ifndef
arch_check_bitop_size" around this macros?
> also wants writing in a way such that it
> can be used as a normal expression, without double semicolons or
> other
> anomalies (potentially) resulting at use sites. E.g.
>
> #define check_bitop_size(addr) do { \
> if ( bitop_bad_size(addr) ) \
> __bitop_bad_size(); \
> } while ( 0 )
>
> >
Re: [PATCH v9 07/15] xen/riscv: introduce atomic.h
On Wed, 2024-05-15 at 11:49 +0200, Jan Beulich wrote: > On 06.05.2024 12:15, Oleksii Kurochko wrote: > > Changes in V9: > > - update the defintion of write_atomic macros: > > drop the return value as this macros isn't expeceted to return > > something > > drop unnessary parentheses around p. > > Yet then what about add_sized()? With that also tidied > Acked-by: Jan Beulich Sure, parentheses around p in add_sized() should be dropped too. I will update the patch in the next version of the patch series. ~ Oleksii
Re: [PATCH v9 03/15] xen/bitops: implement fls{l}() in common logic
On Wed, 2024-05-15 at 11:09 +0200, Jan Beulich wrote: > On 06.05.2024 12:15, Oleksii Kurochko wrote: > > Changes in V9: > > - update return type of fls and flsl() to unsigned int to be > > aligned with other > > bit ops. > > But this then needs carrying through to ... > > > --- a/xen/arch/arm/include/asm/arm64/bitops.h > > +++ b/xen/arch/arm/include/asm/arm64/bitops.h > > @@ -22,17 +22,15 @@ static /*__*/always_inline unsigned long > > __ffs(unsigned long word) > > */ > > #define ffz(x) __ffs(~(x)) > > > > -static inline int flsl(unsigned long x) > > +static inline int arch_flsl(unsigned long x) > > ... e.g. here. You don't want to introduce signed/unsigned > mismatches. Do you mean that generic flsl() uses 'unsigned int' as a return type, but arch_flsl continue to use 'int'? > > Also why do you keep "inline" here, while ... > > > --- a/xen/arch/x86/include/asm/bitops.h > > +++ b/xen/arch/x86/include/asm/bitops.h > > @@ -425,7 +425,7 @@ static always_inline unsigned int > > arch_ffsl(unsigned long x) > > * > > * This is defined the same way as ffs. > > */ > > -static inline int flsl(unsigned long x) > > +static always_inline int arch_flsl(unsigned long x) > > ... you switch to always_inline here? Because Adnrew's patch with bitops.h for x86 changes to always_inline, so to be consistent, at least, for architecture. ~ Oleksii > > (replying out of order) > > > --- a/xen/arch/arm/include/asm/arm32/bitops.h > > +++ b/xen/arch/arm/include/asm/arm32/bitops.h > > @@ -1,7 +1,7 @@ > > #ifndef _ARM_ARM32_BITOPS_H > > #define _ARM_ARM32_BITOPS_H > > > > -#define flsl fls > > +#define arch_flsl fls > > It's the Arm maintainers to ultimately judge, but I'd be inclined to > suggest > > #define arch_flsl arch_fls > > instead. That's not only behaviorally closer to what was there > before, but > also reduces (a tiny bit) the amount of work the compiler needs to > carry out. > > Jan
Re: Proposal to Extend Feature Freeze Deadline
Hi Kelly, On Wed, 2024-05-15 at 14:27 +0100, Kelly Choi wrote: > Hi Oleksii, > > If there are no objections by tomorrow, let's assume by lazy > consensus that we will extend the timeline by a week. > If anyone objects to this, please reply to this email. I will send a separate email tomorrow if no one objects. ~ Oleksii > > Many thanks, > Kelly Choi > > Community Manager > Xen Project > > > On Wed, May 15, 2024 at 4:13 AM Henry Wang wrote: > > Hi Oleksii, > > > > On 5/14/2024 11:43 PM, Andrew Cooper wrote: > > > On 14/05/2024 4:40 pm, Oleksii K. wrote: > > >> Hello everyone, > > >> > > >> We're observing fewer merged patches/series across several > > >> architectures for the current 4.19 release in comparison to > > previous > > >> release. > > >> > > >> For example: > > >> 1. For Arm, significant features like Cache Coloring and PCI > > >> Passthrough won't be fully merged. Thus, it would be beneficial > > to > > >> commit at least the following two patch series: > > >> > > [1]https://lore.kernel.org/xen-devel/20240511005611.83125-1-xin.wa...@amd.com/ > > >> > > >> > > [2]https://lore.kernel.org/xen-devel/20240424033449.168398-1-xin.wa...@amd.com/ > > >> > > >> 2. For RISC-V, having the following patch series [3], mostly > > reviewed > > >> with only one blocker [4], would be advantageous (As far as I > > know, > > >> Andrew is planning to update his patch series): > > >> > > [3]https://lore.kernel.org/xen-devel/cover.1713347222.git.oleksii.kuroc...@gmail.com/ > > >> [4] > > >> > > https://patchew.org/Xen/20240313172716.2325427-1-andrew.coop...@citrix.com/ > > >> > > >> 3. For PPC, it would be beneficial to have [5] merged: > > >> [5] > > >> > > https://lore.kernel.org/xen-devel/cover.1712893887.git.sanasta...@raptorengineering.com/ > > >> > > >> Extending the feature freeze deadline by one week, until May > > 24th, > > >> would provide additional time for merges mentioned above. This, > > in > > >> turn, would create space for more features and fixes for x86 and > > other > > >> common elements. If we agree to extend the feature freeze > > deadline, > > >> please feel free to outline what you would like to see in the > > 4.19 > > >> release. This will make it easier to track our final goals and > > >> determine if they are realistically achievable. > > >> > > >> I'd like to open the floor for discussion on this proposal. Does > > it > > >> make sense, and would it be useful? > > > Considering how many people are blocked on me, I'd welcome a > > little bit > > > longer to get the outstanding series/fixes to land. > > > > It would be great if we can extend the deadline for a week, thank > > you! I > > will try my best to make progress of the two above-mentioned Arm > > series. > > > > Kind regards, > > Henry
Proposal to Extend Feature Freeze Deadline
Hello everyone, We're observing fewer merged patches/series across several architectures for the current 4.19 release in comparison to previous release. For example: 1. For Arm, significant features like Cache Coloring and PCI Passthrough won't be fully merged. Thus, it would be beneficial to commit at least the following two patch series: [1]https://lore.kernel.org/xen-devel/20240511005611.83125-1-xin.wa...@amd.com/ [2]https://lore.kernel.org/xen-devel/20240424033449.168398-1-xin.wa...@amd.com/ 2. For RISC-V, having the following patch series [3], mostly reviewed with only one blocker [4], would be advantageous (As far as I know, Andrew is planning to update his patch series): [3]https://lore.kernel.org/xen-devel/cover.1713347222.git.oleksii.kuroc...@gmail.com/ [4] https://patchew.org/Xen/20240313172716.2325427-1-andrew.coop...@citrix.com/ 3. For PPC, it would be beneficial to have [5] merged: [5] https://lore.kernel.org/xen-devel/cover.1712893887.git.sanasta...@raptorengineering.com/ Extending the feature freeze deadline by one week, until May 24th, would provide additional time for merges mentioned above. This, in turn, would create space for more features and fixes for x86 and other common elements. If we agree to extend the feature freeze deadline, please feel free to outline what you would like to see in the 4.19 release. This will make it easier to track our final goals and determine if they are realistically achievable. I'd like to open the floor for discussion on this proposal. Does it make sense, and would it be useful? Have a great day! Best regards, Olesii
Re: [PATCH for-4.19 v2] tools/xen-cpuid: switch to use cpu-policy defined names
On Thu, 2024-05-09 at 14:13 +0200, Roger Pau Monné wrote: > On Thu, May 02, 2024 at 03:16:54PM +0200, Jan Beulich wrote: > > On 02.05.2024 13:49, Roger Pau Monne wrote: > > > Like it was done recently for libxl, switch to using the auto- > > > generated feature > > > names by the processing of cpufeatureset.h, this allows removing > > > the open-coded > > > feature names, and unifies the feature naming with libxl and the > > > hypervisor. > > > > > > Introduce a newly auto-generated array that contains the feature > > > names indexed > > > at featureset bit position, otherwise using the existing > > > INIT_FEATURE_NAMES > > > would require iterating over the array elements until a match > > > with the expected > > > bit position is found. > > > > > > Note that leaf names need to be kept, as the current auto- > > > generated data > > > doesn't contain the leaf names. > > > > > > Signed-off-by: Roger Pau Monné > > > > Reviewed-by: Jan Beulich > > Oleksii, now that Jan has provided a Reviewed-by, can you provide a > release-ack for this to go in? Based that it is reviewed, I will be happy to have this in 4.19: Release-acked-by: Oleksii Kurochko ~ Oleksii
Re: [PATCH for-4.19] x86/mtrr: avoid system wide rendezvous when setting AP MTRRs
On Mon, 2024-05-13 at 10:59 +0200, Roger Pau Monne wrote:
> There's no point in forcing a system wide update of the MTRRs on all
> processors
> when there are no changes to be propagated. On AP startup it's only
> the AP
> that needs to write the system wide MTRR values in order to match the
> rest of
> the already online CPUs.
>
> We have occasionally seen the watchdog trigger during `xen-hptool
> cpu-online`
> in one Intel Cascade Lake box with 448 CPUs due to the re-setting of
> the MTRRs
> on all the CPUs in the system.
>
> While there adjust the comment to clarify why the system-wide
> resetting of the
> MTRR registers is not needed for the purposes of mtrr_ap_init().
>
> Signed-off-by: Roger Pau Monné
> ---
> For consideration for 4.19: it's a bugfix of a rare instance of the
> watchdog
> triggering, but it's also a good performance improvement when
> performing
> cpu-online.
>
> Hopefully runtime changes to MTRR will affect a single MSR at a time,
> lowering
> the chance of the watchdog triggering due to the system-wide
> resetting of the
> range.
Considering it as a bugfix it will be good to have it in 4.19 release:
Release-acked-by: Oleksii Kurochko
~ Oleksii
> ---
> xen/arch/x86/cpu/mtrr/main.c | 15 ---
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/xen/arch/x86/cpu/mtrr/main.c
> b/xen/arch/x86/cpu/mtrr/main.c
> index 90b235f57e68..0a44ebbcb04f 100644
> --- a/xen/arch/x86/cpu/mtrr/main.c
> +++ b/xen/arch/x86/cpu/mtrr/main.c
> @@ -573,14 +573,15 @@ void mtrr_ap_init(void)
> if (!mtrr_if || hold_mtrr_updates_on_aps)
> return;
> /*
> - * Ideally we should hold mtrr_mutex here to avoid mtrr
> entries changed,
> - * but this routine will be called in cpu boot time, holding
> the lock
> - * breaks it. This routine is called in two cases: 1.very
> earily time
> - * of software resume, when there absolutely isn't mtrr
> entry changes;
> - * 2.cpu hotadd time. We let mtrr_add/del_page hold
> cpuhotplug lock to
> - * prevent mtrr entry changes
> + * hold_mtrr_updates_on_aps takes care of preventing
> unnecessary MTRR
> + * updates when batch starting the CPUs (see
> + * mtrr_aps_sync_{begin,end}()).
> + *
> + * Otherwise just apply the current system wide MTRR values
> to this AP.
> + * Note this doesn't require synchronization with the other
> CPUs, as
> + * there are strictly no modifications of the current MTRR
> values.
> */
> - set_mtrr(~0U, 0, 0, 0);
> + mtrr_set_all();
> }
>
> /**
Re: [PATCH for-4.19] libxl: fix population of the online vCPU bitmap for PVH
On Fri, 2024-05-10 at 16:20 +0100, Andrew Cooper wrote:
> On 10/05/2024 1:49 pm, Roger Pau Monne wrote:
> > libxl passes some information to libacpi to create the ACPI table
> > for a PVH
> > guest, and among that information it's a bitmap of which vCPUs are
> > online
> > which can be less than the maximum number of vCPUs assigned to the
> > domain.
> >
> > While the population of the bitmap is done correctly for HVM based
> > on the
> > number of online vCPUs, for PVH the population of the bitmap is
> > done based on
> > the number of maximum vCPUs allowed. This leads to all local APIC
> > entries in
> > the MADT being set as enabled, which contradicts the data in
> > xenstore if vCPUs
> > is different than maximum vCPUs.
> >
> > Fix by copying the internal libxl bitmap that's populated based on
> > the vCPUs
> > parameter.
> >
> > Reported-by: Arthur Borsboom
> > Link: https://gitlab.com/libvirt/libvirt/-/issues/399
> > Reported-by: Leigh Brown
> > Fixes: 14c0d328da2b ('libxl/acpi: Build ACPI tables for HVMlite
> > guests')
> > Signed-off-by: Roger Pau Monné
> > ---
> > Note that the setup of hvm_info_table could be shared between PVH
> > and HVM, as
> > the fields are very limited, see hvm_build_set_params() for the HVM
> > side.
> > However this late in the release it's safer to just adjust the PVH
> > path.
> >
> > Also note the checksum is not provided when hvm_info_table is built
> > for PVH.
> > This is fine so far because such checksum is only consumed by
> > hvmloader and not
> > libacpi itself.
> >
> > It's a bugfix, so should be considered for 4.19.
>
> Bugfixes are still completely fair game at this point.
Considering that this is bugfixes I will be happy to have that in 4.19
release:
Release-acked-by: Oleksii Kurochko
~ Oleksii
>
> Acked-by: Andrew Cooper
Re: [RFC PATCH v2 0/5] Add bridge VLAN support
On Thu, 2024-05-09 at 16:53 +0100, Andrew Cooper wrote: > On 08/05/2024 10:38 pm, Leigh Brown wrote: > > Hello all, > > > > I realised over the weekend that there is a valid use case for > > providing > > a VIF to a domain that has access to multiple VLANs, e.g. a router. > > Yes, > > you can create a VIF per VLAN, but if you start having several > > VLANs (as > > I do), it would be nicer to create a single interface that has > > access to > > all the relevant VLANs (e.g. enX0.10, enX0.20, etc.). > > > > So, version 2 changes the name and type of the parameter from an > > integer > > called `vid' to a string called `vlan'. The vlan parameter is then > > parsed by the vif-bridge script (actually, the functions called by > > it in > > xen-network-common.sh). > > > > As it quite a common practice to allocate VLANs in round numbers, I > > also > > implemented the ability to specify contiguous or non-contiguous > > ranges. > > You can specify whether a VLAN is tagged or untagged, and which > > VLAN is > > the PVID (only one PVID is allowed). For example, > > > > vif = [ 'mac=xx:xx:xx:xx:xx:xx, bridge=br0, vlan=10p/20-29' ] > > > > will setup the VIF so that 10 is the PVID and VLAN IDs 20 through > > 29 > > are permitted with tags. Another example: > > > > vif = [ 'mac=xx:xx:xx:xx:xx:xx, bridge=br0, vlan=1p/10+10x9' ] > > > > will setup the bridge to set 1 as the PVID and permit access with > > tags for VLAN IDs 10, 20, 30, 40, 50, 60, 70, 80 and 90. > > > > This patch set enables this capability as follows: > > > > 1. Adds `vlan' as a new member of the libxl_device_nic structure; > > 2. Adds support to read and write the vlan parameter from the > > xenstore; > > 3. Adds `vlan' as a new keyword for the vif configuration option; > > 4. Adds support to assign the bridge VLANs in the Linux hotplug > > scripts; > > 5. Updated xl-network-configuration(5) manpage and example configs. > > > > Original blurb below: > > > > For many years I have been configuring VLANs on my Linux Dom0 by > > creating VLAN interfaces for each VLAN I wanted to connect a domain > > to and then a corresponding bridge. So I would tend to have things > > like: > > > > enp0s0 -> br0 -> vif1, vif2 > > enp0s0.10 -> br0vl10 -> vif3, vif4 > > enp0s0.20 -> br0vl20 -> vif5 > > dummy0 -> br1 -> vif6 > > > > I recently discovered that iproute2 supports creating bridge VLANs > > that > > allows you to assign a VLAN to each of the interfaces associated to > > a > > bridge. This allows a greatly simplified configuration where a > > single > > bridge can support all the domains, and the iproute2 bridge command > > can > > assign each VIF to the required VLAN. This looks like this: > > > > # bridge vlan > > port vlan-id > > enp0s0 1 PVID Egress Untagged > > 10 > > 20 > > br0 1 PVID Egress Untagged > > vif1.0 1 PVID Egress Untagged > > vif2.0 1 PVID Egress Untagged > > vif3.0 10 PVID Egress Untagged > > vif4.0 10 PVID Egress Untagged > > vif5.0 20 PVID Egress Untagged > > vif6.0 30 PVID Egress Untagged > > > > This patch set enables this capability as follows: > > > > 1. Adds `vid' as a new member of the libxl_device_nic structure; > > 2. Adds support to read and write vid from the xenstore; > > 3. Adds `vid' as a new keyword for the vif configuration option; > > 4. Adds support for assign the bridge VLAN in the Linux hotplug > > scripts. > > > > I don't believe NetBSD or FreeBSD support this capability, but if > > they > > do please point me in the direction of some documentation and/or > > examples. > > > > NB: I'm not very familiar with Xen code base so may have missed > > something important, although I have tested it and it is working > > well > > for me. > > > > Cheers, > > > > Leigh. > > > > > > Leigh Brown (5): > > tools/libs/light: Add vlan field to libxl_device_nic > > tools/xl: add vlan keyword to vif option > > tools/hotplug/Linux: Add bridge VLAN support > > docs/man: document VIF vlan keyword > > tools/examples: Example Linux bridge VLAN config > > > > docs/man/xl-network-configuration.5.pod.in | 38 ++ > > tools/examples/linux-bridge-vlan/README | 68 +++ > > tools/examples/linux-bridge-vlan/br0.netdev | 7 ++ > > tools/examples/linux-bridge-vlan/br0.network | 8 ++ > > .../examples/linux-bridge-vlan/enp0s0.network | 16 +++ > > tools/hotplug/Linux/xen-network-common.sh | 111 > > ++ > > tools/libs/light/libxl_nic.c | 10 ++ > > tools/libs/light/libxl_types.idl | 1 + > > tools/xl/xl_parse.c | 2 + > > 9 files changed, 261 insertions(+) > > create mode 100644 tools/examples/linux-bridge-vlan/README > > create mode 100644 tools/examples/linux-bridge-vlan/br0.netdev > > create mode 100644 tools/examples/linux-bridge-vlan/br0.network > >
