Re: [PATCH v3 1/2] x86/mm: add API for marking only part of a MMIO page read only
On 21.05.2024 17:33, Marek Marczykowski-Górecki wrote:
> On Tue, May 21, 2024 at 05:16:58PM +0200, Jan Beulich wrote:
>> On 21.05.2024 04:54, Marek Marczykowski-Górecki wrote:
>>> --- a/xen/arch/x86/include/asm/mm.h
>>> +++ b/xen/arch/x86/include/asm/mm.h
>>> @@ -522,9 +522,27 @@ extern struct rangeset *mmio_ro_ranges;
>>> void memguard_guard_stack(void *p);
>>> void memguard_unguard_stack(void *p);
>>>
>>> +/*
>>> + * Add more precise r/o marking for a MMIO page. Range specified here
>>> + * will still be R/O, but the rest of the page (not marked as R/O via
>>> another
>>> + * call) will have writes passed through.
>>> + * The start address and the size must be aligned to SUBPAGE_MMIO_RO_ALIGN.
>>> + *
>>> + * This API cannot be used for overlapping ranges, nor for pages already
>>> added
>>> + * to mmio_ro_ranges separately.
>>> + *
>>> + * Return values:
>>> + * - negative: error
>>> + * - 0: success
>>> + */
>>> +#define SUBPAGE_MMIO_RO_ALIGN 8
>>
>> This isn't just alignment, but also (and perhaps more importantly)
>> granularity.
>> I think the name wants to express this.
>
> SUBPAGE_MMIO_RO_GRANULARITY? Sounds a bit long...
..._GRAN? ..._CHUNK? ..._UNIT? (Perhaps also want to have MMIO_RO_ first.)
>>> +static int __init subpage_mmio_ro_add_page(
>>> +mfn_t mfn, unsigned int offset_s, unsigned int offset_e)
>>> +{
>>> +struct subpage_ro_range *entry = NULL, *iter;
>>> +unsigned int i;
>>> +
>>> +list_for_each_entry(iter, _ro_ranges, list)
>>> +{
>>> +if ( mfn_eq(iter->mfn, mfn) )
>>> +{
>>> +entry = iter;
>>> +break;
>>> +}
>>> +}
>>> +if ( !entry )
>>> +{
>>> +/* iter == NULL marks it was a newly allocated entry */
>>> +iter = NULL;
>>> +entry = xzalloc(struct subpage_ro_range);
>>> +if ( !entry )
>>> +return -ENOMEM;
>>> +entry->mfn = mfn;
>>> +}
>>> +
>>> +for ( i = offset_s; i <= offset_e; i += SUBPAGE_MMIO_RO_ALIGN )
>>> +{
>>> +int oldbit = __test_and_set_bit(i / SUBPAGE_MMIO_RO_ALIGN,
>>> +entry->ro_qwords);
>>
>> Why int, not bool?
>
> Because __test_and_set_bit returns int. But I can change to bool if you
> prefer.
test_bit() and the test-and set of functions should eventually all change
to be returning bool. One less use site to then also take care of.
Jan
[xen-4.17-testing test] 186063: regressions - FAIL
flight 186063 xen-4.17-testing real [real] http://logs.test-lab.xenproject.org/osstest/logs/186063/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-arm64-xsm 6 xen-buildfail REGR. vs. 185864 build-amd64-xsm 6 xen-buildfail REGR. vs. 185864 Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-qemuu-debianhvm-i386-xsm 1 build-check(1) blocked n/a test-amd64-amd64-xl-qemut-stubdom-debianhvm-amd64-xsm 1 build-check(1) blocked n/a test-amd64-amd64-xl-qemut-debianhvm-i386-xsm 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-xsm 1 build-check(1) blocked n/a test-amd64-amd64-xl-xsm 1 build-check(1) blocked n/a test-arm64-arm64-libvirt-xsm 1 build-check(1) blocked n/a test-arm64-arm64-xl-xsm 1 build-check(1) blocked n/a test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 185864 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 185864 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 185864 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 185864 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 185864 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 185864 test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-xl 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-qcow2 14 migrate-support-checkfail never pass test-amd64-amd64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit1 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-raw 14 migrate-support-checkfail never pass test-armhf-armhf-xl-raw 15 saverestore-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 15 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail never pass test-armhf-armhf-xl-qcow214 migrate-support-checkfail never pass test-armhf-armhf-xl-qcow215 saverestore-support-checkfail never pass test-arm64-arm64-xl-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-libvirt-vhd 15 saverestore-support-checkfail never pass version targeted for testing: xen afcce3cfc2458efbd3ff9320153c534a82a108c5 baseline version: xen effcf70f020ff12d34c80e2abde0ecb00ce92bda Last test of basis 185864 2024-04-29 08:08:55 Z 22 days Testing same since 186063 2024-05-21 10:06:36 Z0 days1 attempts People who touched revisions under test: Andrew Cooper Daniel P. Smith Demi Marie Obenour Jan Beulich Jason Andryuk Jason Andryuk Juergen Gross Leigh Brown Roger Pau Monné Ross Lagerwall jobs: build-amd64-xsm fail
Re: [PATCH] Revert "evtchn: refuse EVTCHNOP_status for Xen-bound event channels"
On Tue, 21 May 2024, Jan Beulich wrote: > On 17.05.2024 22:28, Stefano Stabellini wrote: > > On Fri, 17 May 2024, Jan Beulich wrote: > >> On 17.05.2024 03:21, Stefano Stabellini wrote: > >>> On Thu, 16 May 2024, Jan Beulich wrote: > 1) In the discussion George claimed that exposing status information in > an uncontrolled manner is okay. I'm afraid I have to disagree, seeing > how a similar assumption by CPU designers has led to a flood of > vulnerabilities over the last 6+ years. Information exposure imo is never > okay, unless it can be _proven_ that absolutely nothing "useful" can be > inferred from it. (I'm having difficulty seeing how such a proof might > look like.) > >>> > >>> Many would agree that it is better not to expose status information in > >>> an uncontrolled manner. Anyway, let's focus on the actionable. > >>> > >>> > 2) Me pointing out that the XSM hook might similarly get in the way of > debugging, Andrew suggested that this is not an issue because any > sensible > XSM policy used in such an environment would grant sufficient privilege > to > Dom0. Yet that then still doesn't cover why DomU-s also can obtain status > for Xen-internal event channels. The debugging argument then becomes > weak, > as in that case the XSM hook is possibly going to get in the way. > > 3) In the discussion Andrew further gave the impression that > evtchn_send() > had no XSM check. Yet it has; the difference to evtchn_status() is that > the latter uses XSM_TARGET while the former uses XSM_HOOK. (Much like > evtchn_status() may indeed be useful for debugging, evtchn_send() may be > similarly useful to allow getting a stuck channel unstuck.) > > In summary I continue to think that an outright revert was inappropriate. > DomU-s should continue to be denied status information on Xen-internal > event channels, unconditionally and independent of whether dummy, silo, > or > Flask is in use. > >>> > >>> I think DomU-s should continue to be denied status information on > >>> Xen-internal event channels *based on the default dummy, silo, or Flask > >>> policy*. It is not up to us to decide the security policy, only to > >>> enforce it and provide sensible defaults. > >>> > >>> In any case, the XSM_TARGET check in evtchn_status seems to do what we > >>> want? > >> > >> No. XSM_TARGET permits the "owning" (not really, but it's its table) domain > >> access. See xsm_default_action() in xsm/dummy.h. > > > > Sorry I still don't understand. Why is that a problem? It looks like the > > wanted default behavior? > > For ordinary event channels - yes. But not for Xen-internal ones, at least > from my pov. I understand your point of view, but in my opinion it is OK
Re: [PATCH 3/3] xen/x86: Address two misc MISRA 17.7 violations
On Tue, 21 May 2024, Andrew Cooper wrote:
> Neither text_poke() nor watchdog_setup() have return value consulted. Switch
> them to being void.
>
> Signed-off-by: Andrew Cooper
Reviewed-by: Stefano Stabellini
> ---
> CC: George Dunlap
> CC: Jan Beulich
> CC: Stefano Stabellini
> CC: Julien Grall
> CC: Roberto Bagnara
> CC: consult...@bugseng.com
> CC: Oleksii Kurochko
> ---
> xen/arch/x86/alternative.c | 4 ++--
> xen/arch/x86/nmi.c | 5 ++---
> xen/include/xen/watchdog.h | 2 +-
> 3 files changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
> index 2e7ba6e0b833..7824053c9d33 100644
> --- a/xen/arch/x86/alternative.c
> +++ b/xen/arch/x86/alternative.c
> @@ -155,10 +155,10 @@ void init_or_livepatch add_nops(void *insns, unsigned
> int len)
> * "noinline" to cause control flow change and thus invalidate I$ and
> * cause refetch after modification.
> */
> -static void *init_or_livepatch noinline
> +static void init_or_livepatch noinline
> text_poke(void *addr, const void *opcode, size_t len)
> {
> -return memcpy(addr, opcode, len);
> +memcpy(addr, opcode, len);
> }
>
> extern void *const __initdata_cf_clobber_start[];
> diff --git a/xen/arch/x86/nmi.c b/xen/arch/x86/nmi.c
> index f6329cb0270e..9793fa23168d 100644
> --- a/xen/arch/x86/nmi.c
> +++ b/xen/arch/x86/nmi.c
> @@ -464,12 +464,12 @@ bool watchdog_enabled(void)
> return !atomic_read(_disable_count);
> }
>
> -int __init watchdog_setup(void)
> +void __init watchdog_setup(void)
> {
> unsigned int cpu;
>
> /*
> - * Activate periodic heartbeats. We cannot do this earlier during
> + * Activate periodic heartbeats. We cannot do this earlier during
> * setup because the timer infrastructure is not available.
> */
> for_each_online_cpu ( cpu )
> @@ -477,7 +477,6 @@ int __init watchdog_setup(void)
> register_cpu_notifier(_nmi_nfb);
>
> watchdog_enable();
> -return 0;
> }
>
> /* Returns false if this was not a watchdog NMI, true otherwise */
> diff --git a/xen/include/xen/watchdog.h b/xen/include/xen/watchdog.h
> index 434fcbdd2b11..40c21bca823f 100644
> --- a/xen/include/xen/watchdog.h
> +++ b/xen/include/xen/watchdog.h
> @@ -10,7 +10,7 @@
> #include
>
> /* Try to set up a watchdog. */
> -int watchdog_setup(void);
> +void watchdog_setup(void);
>
> /* Enable the watchdog. */
> void watchdog_enable(void);
> --
> 2.30.2
>
Re: [PATCH 2/3] xen/x86: Drop useless non-Kconfig CONFIG_* variables
On Tue, 21 May 2024, Andrew Cooper wrote: > These are all either completely unused, or do nothing useful. > > Signed-off-by: Andrew Cooper Reviewed-by: Stefano Stabellini > --- > CC: George Dunlap > CC: Jan Beulich > CC: Stefano Stabellini > CC: Julien Grall > CC: Roberto Bagnara > CC: consult...@bugseng.com > CC: Oleksii Kurochko > --- > xen/arch/x86/include/asm/config.h | 4 > xen/include/xen/acpi.h| 9 - > xen/include/xen/watchdog.h| 11 --- > 3 files changed, 24 deletions(-) > > diff --git a/xen/arch/x86/include/asm/config.h > b/xen/arch/x86/include/asm/config.h > index ab7288cb3682..24b005ba1ff7 100644 > --- a/xen/arch/x86/include/asm/config.h > +++ b/xen/arch/x86/include/asm/config.h > @@ -20,7 +20,6 @@ > #define BITS_PER_XEN_ULONG BITS_PER_LONG > > #define CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS 1 > -#define CONFIG_DISCONTIGMEM 1 > #define CONFIG_NUMA_EMU 1 > > #define CONFIG_PAGEALLOC_MAX_ORDER (2 * PAGETABLE_ORDER) > @@ -30,11 +29,8 @@ > /* Intel P4 currently has largest cache line (L2 line size is 128 bytes). */ > #define CONFIG_X86_L1_CACHE_SHIFT 7 > > -#define CONFIG_ACPI_SRAT 1 > #define CONFIG_ACPI_CSTATE 1 > > -#define CONFIG_WATCHDOG 1 > - > #define CONFIG_MULTIBOOT 1 > > #define HZ 100 > diff --git a/xen/include/xen/acpi.h b/xen/include/xen/acpi.h > index e78e7e785252..bc4818c9430a 100644 > --- a/xen/include/xen/acpi.h > +++ b/xen/include/xen/acpi.h > @@ -140,15 +140,6 @@ int get_cpu_id(u32 acpi_id); > unsigned int acpi_register_gsi (u32 gsi, int edge_level, int > active_high_low); > int acpi_gsi_to_irq (u32 gsi, unsigned int *irq); > > -/* > - * This function undoes the effect of one call to acpi_register_gsi(). > - * If this matches the last registration, any IRQ resources for gsi > - * are freed. > - */ > -#ifdef CONFIG_ACPI_DEALLOCATE_IRQ > -void acpi_unregister_gsi (u32 gsi); > -#endif > - > #ifdef CONFIG_ACPI_CSTATE > /* > * max_cstate sets the highest legal C-state. > diff --git a/xen/include/xen/watchdog.h b/xen/include/xen/watchdog.h > index 86fde0884ae7..434fcbdd2b11 100644 > --- a/xen/include/xen/watchdog.h > +++ b/xen/include/xen/watchdog.h > @@ -9,8 +9,6 @@ > > #include > > -#ifdef CONFIG_WATCHDOG > - > /* Try to set up a watchdog. */ > int watchdog_setup(void); > > @@ -23,13 +21,4 @@ void watchdog_disable(void); > /* Is the watchdog currently enabled. */ > bool watchdog_enabled(void); > > -#else > - > -#define watchdog_setup() ((void)0) > -#define watchdog_enable() ((void)0) > -#define watchdog_disable() ((void)0) > -#define watchdog_enabled() ((void)0) > - > -#endif > - > #endif /* __XEN_WATCHDOG_H__ */ > -- > 2.30.2 >
Re: [PATCH v3 5/8] xen/arm/gic: Allow routing/removing interrupt to running VMs
Hi Stefano,
On 5/22/2024 9:16 AM, Stefano Stabellini wrote:
On Wed, 22 May 2024, Henry Wang wrote:
Hi Julien,
On 5/21/2024 8:30 PM, Julien Grall wrote:
Hi,
On 21/05/2024 05:35, Henry Wang wrote:
diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
index 56490dbc43..956c11ba13 100644
--- a/xen/arch/arm/gic-vgic.c
+++ b/xen/arch/arm/gic-vgic.c
@@ -439,24 +439,33 @@ int vgic_connect_hw_irq(struct domain *d, struct
vcpu *v, unsigned int virq,
/* We are taking to rank lock to prevent parallel connections. */
vgic_lock_rank(v_target, rank, flags);
+ spin_lock(_target->arch.vgic.lock);
I know this is what Stefano suggested, but v_target would point to the
current affinity whereas the interrupt may be pending/active on the
"previous" vCPU. So it is a little unclear whether v_target is the correct
lock. Do you have more pointer to show this is correct?
No I think you are correct, we have discussed this in the initial version of
this patch. Sorry.
I followed the way from that discussion to note down the vcpu ID and retrieve
here, below is the diff, would this make sense to you?
diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
index 956c11ba13..134ed4e107 100644
--- a/xen/arch/arm/gic-vgic.c
+++ b/xen/arch/arm/gic-vgic.c
@@ -439,7 +439,7 @@ int vgic_connect_hw_irq(struct domain *d, struct vcpu *v,
unsigned int virq,
/* We are taking to rank lock to prevent parallel connections. */
vgic_lock_rank(v_target, rank, flags);
- spin_lock(_target->arch.vgic.lock);
+ spin_lock(>vcpu[p->spi_vcpu_id]->arch.vgic.lock);
if ( connect )
{
@@ -465,7 +465,7 @@ int vgic_connect_hw_irq(struct domain *d, struct vcpu *v,
unsigned int virq,
p->desc = NULL;
}
- spin_unlock(_target->arch.vgic.lock);
+ spin_unlock(>vcpu[p->spi_vcpu_id]->arch.vgic.lock);
vgic_unlock_rank(v_target, rank, flags);
return ret;
diff --git a/xen/arch/arm/include/asm/vgic.h b/xen/arch/arm/include/asm/vgic.h
index 79b73a0dbb..f4075d3e75 100644
--- a/xen/arch/arm/include/asm/vgic.h
+++ b/xen/arch/arm/include/asm/vgic.h
@@ -85,6 +85,7 @@ struct pending_irq
uint8_t priority;
uint8_t lpi_priority; /* Caches the priority if this is an LPI. */
uint8_t lpi_vcpu_id; /* The VCPU for an LPI. */
+ uint8_t spi_vcpu_id; /* The VCPU for an SPI. */
/* inflight is used to append instances of pending_irq to
* vgic.inflight_irqs */
struct list_head inflight;
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index c04fc4f83f..e852479f13 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -632,6 +632,7 @@ void vgic_inject_irq(struct domain *d, struct vcpu *v,
unsigned int virq,
}
list_add_tail(>inflight, >arch.vgic.inflight_irqs);
out:
+ n->spi_vcpu_id = v->vcpu_id;
spin_unlock_irqrestore(>arch.vgic.lock, flags);
/* we have a new higher priority irq, inject it into the guest */
vcpu_kick(v);
Also, while looking at the locking, I noticed that we are not doing anything
with GIC_IRQ_GUEST_MIGRATING. In gic_update_one_lr(), we seem to assume that
if the flag is set, then p->desc cannot be NULL.
Can we reach vgic_connect_hw_irq() with the flag set?
I think even from the perspective of making the code extra safe, we should
also check GIC_IRQ_GUEST_MIGRATING as the LR is allocated for this case. I
will also add the check of GIC_IRQ_GUEST_MIGRATING here.
Yes. I think it might be easier to check for GIC_IRQ_GUEST_MIGRATING
early and return error immediately in that case. Otherwise, we can
continue and take spin_lock(_target->arch.vgic.lock) because no
migration is in progress
Ok, this makes sense to me, I will add
if( test_bit(GIC_IRQ_GUEST_MIGRATING, >status) )
{
vgic_unlock_rank(v_target, rank, flags);
return -EBUSY;
}
right after taking the vgic rank lock.
Kind regards,
Henry
Re: [PATCH 1/3] xen/lzo: Implement COPY{4,8} using memcpy()
On Tue, 21 May 2024, Andrew Cooper wrote:
> This is simpler and easier for both humans and compilers to read.
>
> It also addresses 6 instances of MISRA R5.3 violation (shadowing of the ptr_
> local variable inside both {put,get}_unaligned()).
>
> No change, not even in the compiled binary.
>
> Signed-off-by: Andrew Cooper
Reviewed-by: Stefano Stabellini
> ---
> CC: George Dunlap
> CC: Jan Beulich
> CC: Stefano Stabellini
> CC: Julien Grall
> CC: Roberto Bagnara
> CC: consult...@bugseng.com
> CC: Oleksii Kurochko
> ---
> xen/common/lzo.c | 11 ++-
> 1 file changed, 2 insertions(+), 9 deletions(-)
>
> diff --git a/xen/common/lzo.c b/xen/common/lzo.c
> index cc03f0f5547f..3454ce4a7e24 100644
> --- a/xen/common/lzo.c
> +++ b/xen/common/lzo.c
> @@ -25,15 +25,8 @@
> */
>
>
> -#define COPY4(dst, src)\
> -put_unaligned(get_unaligned((const u32 *)(src)), (u32 *)(dst))
> -#if defined(__x86_64__)
> -#define COPY8(dst, src)\
> -put_unaligned(get_unaligned((const u64 *)(src)), (u64 *)(dst))
> -#else
> -#define COPY8(dst, src)\
> -COPY4(dst, src); COPY4((dst) + 4, (src) + 4)
> -#endif
> +#define COPY4(dst, src) memcpy(dst, src, 4)
> +#define COPY8(dst, src) memcpy(dst, src, 8)
>
> #ifdef __MINIOS__
> # include
> --
> 2.30.2
>
Re: [PATCH v3 5/8] xen/arm/gic: Allow routing/removing interrupt to running VMs
On Wed, 22 May 2024, Henry Wang wrote:
> Hi Julien,
>
> On 5/21/2024 8:30 PM, Julien Grall wrote:
> > Hi,
> >
> > On 21/05/2024 05:35, Henry Wang wrote:
> > > diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
> > > index 56490dbc43..956c11ba13 100644
> > > --- a/xen/arch/arm/gic-vgic.c
> > > +++ b/xen/arch/arm/gic-vgic.c
> > > @@ -439,24 +439,33 @@ int vgic_connect_hw_irq(struct domain *d, struct
> > > vcpu *v, unsigned int virq,
> > > /* We are taking to rank lock to prevent parallel connections. */
> > > vgic_lock_rank(v_target, rank, flags);
> > > + spin_lock(_target->arch.vgic.lock);
> >
> > I know this is what Stefano suggested, but v_target would point to the
> > current affinity whereas the interrupt may be pending/active on the
> > "previous" vCPU. So it is a little unclear whether v_target is the correct
> > lock. Do you have more pointer to show this is correct?
>
> No I think you are correct, we have discussed this in the initial version of
> this patch. Sorry.
>
> I followed the way from that discussion to note down the vcpu ID and retrieve
> here, below is the diff, would this make sense to you?
>
> diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
> index 956c11ba13..134ed4e107 100644
> --- a/xen/arch/arm/gic-vgic.c
> +++ b/xen/arch/arm/gic-vgic.c
> @@ -439,7 +439,7 @@ int vgic_connect_hw_irq(struct domain *d, struct vcpu *v,
> unsigned int virq,
>
> /* We are taking to rank lock to prevent parallel connections. */
> vgic_lock_rank(v_target, rank, flags);
> - spin_lock(_target->arch.vgic.lock);
> + spin_lock(>vcpu[p->spi_vcpu_id]->arch.vgic.lock);
>
> if ( connect )
> {
> @@ -465,7 +465,7 @@ int vgic_connect_hw_irq(struct domain *d, struct vcpu *v,
> unsigned int virq,
> p->desc = NULL;
> }
>
> - spin_unlock(_target->arch.vgic.lock);
> + spin_unlock(>vcpu[p->spi_vcpu_id]->arch.vgic.lock);
> vgic_unlock_rank(v_target, rank, flags);
>
> return ret;
> diff --git a/xen/arch/arm/include/asm/vgic.h b/xen/arch/arm/include/asm/vgic.h
> index 79b73a0dbb..f4075d3e75 100644
> --- a/xen/arch/arm/include/asm/vgic.h
> +++ b/xen/arch/arm/include/asm/vgic.h
> @@ -85,6 +85,7 @@ struct pending_irq
> uint8_t priority;
> uint8_t lpi_priority; /* Caches the priority if this is an LPI. */
> uint8_t lpi_vcpu_id; /* The VCPU for an LPI. */
> + uint8_t spi_vcpu_id; /* The VCPU for an SPI. */
> /* inflight is used to append instances of pending_irq to
> * vgic.inflight_irqs */
> struct list_head inflight;
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index c04fc4f83f..e852479f13 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -632,6 +632,7 @@ void vgic_inject_irq(struct domain *d, struct vcpu *v,
> unsigned int virq,
> }
> list_add_tail(>inflight, >arch.vgic.inflight_irqs);
> out:
> + n->spi_vcpu_id = v->vcpu_id;
> spin_unlock_irqrestore(>arch.vgic.lock, flags);
>
> /* we have a new higher priority irq, inject it into the guest */
> vcpu_kick(v);
>
>
> > Also, while looking at the locking, I noticed that we are not doing anything
> > with GIC_IRQ_GUEST_MIGRATING. In gic_update_one_lr(), we seem to assume that
> > if the flag is set, then p->desc cannot be NULL.
> >
> > Can we reach vgic_connect_hw_irq() with the flag set?
>
> I think even from the perspective of making the code extra safe, we should
> also check GIC_IRQ_GUEST_MIGRATING as the LR is allocated for this case. I
> will also add the check of GIC_IRQ_GUEST_MIGRATING here.
Yes. I think it might be easier to check for GIC_IRQ_GUEST_MIGRATING
early and return error immediately in that case. Otherwise, we can
continue and take spin_lock(_target->arch.vgic.lock) because no
migration is in progress
> > What about the other flags? Is this going to be a concern if we don't reset
> > them?
>
> I don't think so, if I am not mistaken, no LR will be allocated with other
> flags set.
>
> Kind regards,
> Henry
Re: [PATCH v3 5/8] xen/arm/gic: Allow routing/removing interrupt to running VMs
Hi Julien,
On 5/21/2024 8:30 PM, Julien Grall wrote:
Hi,
On 21/05/2024 05:35, Henry Wang wrote:
diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
index 56490dbc43..956c11ba13 100644
--- a/xen/arch/arm/gic-vgic.c
+++ b/xen/arch/arm/gic-vgic.c
@@ -439,24 +439,33 @@ int vgic_connect_hw_irq(struct domain *d,
struct vcpu *v, unsigned int virq,
/* We are taking to rank lock to prevent parallel
connections. */
vgic_lock_rank(v_target, rank, flags);
+ spin_lock(_target->arch.vgic.lock);
I know this is what Stefano suggested, but v_target would point to the
current affinity whereas the interrupt may be pending/active on the
"previous" vCPU. So it is a little unclear whether v_target is the
correct lock. Do you have more pointer to show this is correct?
No I think you are correct, we have discussed this in the initial
version of this patch. Sorry.
I followed the way from that discussion to note down the vcpu ID and
retrieve here, below is the diff, would this make sense to you?
diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
index 956c11ba13..134ed4e107 100644
--- a/xen/arch/arm/gic-vgic.c
+++ b/xen/arch/arm/gic-vgic.c
@@ -439,7 +439,7 @@ int vgic_connect_hw_irq(struct domain *d, struct
vcpu *v, unsigned int virq,
/* We are taking to rank lock to prevent parallel connections. */
vgic_lock_rank(v_target, rank, flags);
- spin_lock(_target->arch.vgic.lock);
+ spin_lock(>vcpu[p->spi_vcpu_id]->arch.vgic.lock);
if ( connect )
{
@@ -465,7 +465,7 @@ int vgic_connect_hw_irq(struct domain *d, struct
vcpu *v, unsigned int virq,
p->desc = NULL;
}
- spin_unlock(_target->arch.vgic.lock);
+ spin_unlock(>vcpu[p->spi_vcpu_id]->arch.vgic.lock);
vgic_unlock_rank(v_target, rank, flags);
return ret;
diff --git a/xen/arch/arm/include/asm/vgic.h
b/xen/arch/arm/include/asm/vgic.h
index 79b73a0dbb..f4075d3e75 100644
--- a/xen/arch/arm/include/asm/vgic.h
+++ b/xen/arch/arm/include/asm/vgic.h
@@ -85,6 +85,7 @@ struct pending_irq
uint8_t priority;
uint8_t lpi_priority; /* Caches the priority if this is an
LPI. */
uint8_t lpi_vcpu_id; /* The VCPU for an LPI. */
+ uint8_t spi_vcpu_id; /* The VCPU for an SPI. */
/* inflight is used to append instances of pending_irq to
* vgic.inflight_irqs */
struct list_head inflight;
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index c04fc4f83f..e852479f13 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -632,6 +632,7 @@ void vgic_inject_irq(struct domain *d, struct vcpu
*v, unsigned int virq,
}
list_add_tail(>inflight, >arch.vgic.inflight_irqs);
out:
+ n->spi_vcpu_id = v->vcpu_id;
spin_unlock_irqrestore(>arch.vgic.lock, flags);
/* we have a new higher priority irq, inject it into the guest */
vcpu_kick(v);
Also, while looking at the locking, I noticed that we are not doing
anything with GIC_IRQ_GUEST_MIGRATING. In gic_update_one_lr(), we seem
to assume that if the flag is set, then p->desc cannot be NULL.
Can we reach vgic_connect_hw_irq() with the flag set?
I think even from the perspective of making the code extra safe, we
should also check GIC_IRQ_GUEST_MIGRATING as the LR is allocated for
this case. I will also add the check of GIC_IRQ_GUEST_MIGRATING here.
What about the other flags? Is this going to be a concern if we don't
reset them?
I don't think so, if I am not mistaken, no LR will be allocated with
other flags set.
Kind regards,
Henry
Cheers,
Re: [XEN PATCH] automation/eclair_analysis: add already clean rules to the analysis
On Tue, 21 May 2024, Nicola Vetrini wrote:
> Some MISRA C rules already have no violations in Xen, so they can be
> set as clean.
>
> Reorder the rules in tagging.ecl according to version ordering
> (i.e. sort -V) and split the configuration on multiple lines for
> readability.
>
> Signed-off-by: Nicola Vetrini
Acked-by: Stefano Stabellini
> ---
> .../eclair_analysis/ECLAIR/monitored.ecl | 17
> automation/eclair_analysis/ECLAIR/tagging.ecl | 78 ++-
> 2 files changed, 94 insertions(+), 1 deletion(-)
>
> diff --git a/automation/eclair_analysis/ECLAIR/monitored.ecl
> b/automation/eclair_analysis/ECLAIR/monitored.ecl
> index 9da709dc889c..4daecb0c838f 100644
> --- a/automation/eclair_analysis/ECLAIR/monitored.ecl
> +++ b/automation/eclair_analysis/ECLAIR/monitored.ecl
> @@ -79,4 +79,21 @@
> -enable=MC3R1.R9.3
> -enable=MC3R1.R9.4
> -enable=MC3R1.R9.5
> +-enable=MC3R1.R18.8
> +-enable=MC3R1.R20.2
> +-enable=MC3R1.R20.3
> +-enable=MC3R1.R20.6
> +-enable=MC3R1.R20.11
> +-enable=MC3R1.R21.3
> +-enable=MC3R1.R21.4
> +-enable=MC3R1.R21.5
> +-enable=MC3R1.R21.7
> +-enable=MC3R1.R21.8
> +-enable=MC3R1.R21.12
> +-enable=MC3R1.R22.1
> +-enable=MC3R1.R22.3
> +-enable=MC3R1.R22.7
> +-enable=MC3R1.R22.8
> +-enable=MC3R1.R22.9
> +-enable=MC3R1.R22.10
> -doc_end
> diff --git a/automation/eclair_analysis/ECLAIR/tagging.ecl
> b/automation/eclair_analysis/ECLAIR/tagging.ecl
> index b7a9f75b275b..a354ff322e03 100644
> --- a/automation/eclair_analysis/ECLAIR/tagging.ecl
> +++ b/automation/eclair_analysis/ECLAIR/tagging.ecl
> @@ -19,7 +19,83 @@
>
> -doc_begin="Clean guidelines: new violations for these guidelines are not
> accepted."
>
> --service_selector={clean_guidelines_common,
> "MC3R1.D1.1||MC3R1.D2.1||MC3R1.D4.1||MC3R1.D4.11||MC3R1.D4.14||MC3R1.R10.2||MC3R1.R11.7||MC3R1.R11.9||MC3R1.R12.5||MC3R1.R1.1||MC3R1.R1.3||MC3R1.R1.4||MC3R1.R14.1||MC3R1.R16.7||MC3R1.R17.1||MC3R1.R17.3||MC3R1.R17.4||MC3R1.R17.5||MC3R1.R17.6||MC3R1.R20.13||MC3R1.R20.14||MC3R1.R20.4||MC3R1.R20.9||MC3R1.R21.10||MC3R1.R21.13||MC3R1.R21.19||MC3R1.R21.21||MC3R1.R21.9||MC3R1.R2.2||MC3R1.R22.2||MC3R1.R22.4||MC3R1.R22.5||MC3R1.R22.6||MC3R1.R2.6||MC3R1.R3.1||MC3R1.R3.2||MC3R1.R4.1||MC3R1.R4.2||MC3R1.R5.1||MC3R1.R5.2||MC3R1.R5.4||MC3R1.R5.6||MC3R1.R6.1||MC3R1.R6.2||MC3R1.R7.1||MC3R1.R7.2||MC3R1.R7.4||MC3R1.R8.1||MC3R1.R8.10||MC3R1.R8.12||MC3R1.R8.14||MC3R1.R8.2||MC3R1.R8.5||MC3R1.R8.6||MC3R1.R8.8||MC3R1.R9.2||MC3R1.R9.3||MC3R1.R9.4||MC3R1.R9.5"
> +-service_selector={clean_guidelines_common,
> +"MC3R1.D1.1||
> +MC3R1.D2.1||
> +MC3R1.D4.1||
> +MC3R1.D4.11||
> +MC3R1.D4.14||
> +MC3R1.R1.1||
> +MC3R1.R1.3||
> +MC3R1.R1.4||
> +MC3R1.R2.2||
> +MC3R1.R2.6||
> +MC3R1.R3.1||
> +MC3R1.R3.2||
> +MC3R1.R4.1||
> +MC3R1.R4.2||
> +MC3R1.R5.1||
> +MC3R1.R5.2||
> +MC3R1.R5.4||
> +MC3R1.R5.6||
> +MC3R1.R6.1||
> +MC3R1.R6.2||
> +MC3R1.R7.1||
> +MC3R1.R7.2||
> +MC3R1.R7.4||
> +MC3R1.R8.1||
> +MC3R1.R8.2||
> +MC3R1.R8.5||
> +MC3R1.R8.6||
> +MC3R1.R8.8||
> +MC3R1.R8.10||
> +MC3R1.R8.12||
> +MC3R1.R8.14||
> +MC3R1.R9.2||
> +MC3R1.R9.3||
> +MC3R1.R9.4||
> +MC3R1.R9.5||
> +MC3R1.R10.2||
> +MC3R1.R11.7||
> +MC3R1.R11.9||
> +MC3R1.R12.5||
> +MC3R1.R14.1||
> +MC3R1.R16.7||
> +MC3R1.R17.1||
> +MC3R1.R17.3||
> +MC3R1.R17.4||
> +MC3R1.R17.5||
> +MC3R1.R17.6||
> +MC3R1.R18.8||
> +MC3R1.R20.2||
> +MC3R1.R20.3||
> +MC3R1.R20.4||
> +MC3R1.R20.6||
> +MC3R1.R20.9||
> +MC3R1.R20.11||
> +MC3R1.R20.13||
> +MC3R1.R20.14||
> +MC3R1.R21.3||
> +MC3R1.R21.4||
> +MC3R1.R21.5||
> +MC3R1.R21.7||
> +MC3R1.R21.8||
> +MC3R1.R21.9||
> +MC3R1.R21.10||
> +MC3R1.R21.12||
> +MC3R1.R21.13||
> +MC3R1.R21.19||
> +MC3R1.R21.21||
> +MC3R1.R22.1||
> +MC3R1.R22.2||
> +MC3R1.R22.3||
> +MC3R1.R22.4||
> +MC3R1.R22.5||
> +MC3R1.R22.6||
> +MC3R1.R22.7||
> +MC3R1.R22.8||
> +MC3R1.R22.9||
> +MC3R1.R22.10"
> }
>
> -setq=target,getenv("XEN_TARGET_ARCH")
> --
> 2.34.1
>
[xen-4.18-testing test] 186060: tolerable FAIL - PUSHED
flight 186060 xen-4.18-testing real [real] http://logs.test-lab.xenproject.org/osstest/logs/186060/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 185865 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 185865 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 185865 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 185865 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 185865 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 185865 test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-amd64-libvirt-qcow2 14 migrate-support-checkfail never pass test-amd64-amd64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-xl 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 16 saverestore-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 15 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 15 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit1 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail never pass test-armhf-armhf-xl-qcow214 migrate-support-checkfail never pass test-armhf-armhf-xl-qcow215 saverestore-support-checkfail never pass test-arm64-arm64-xl-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-xl-raw 14 migrate-support-checkfail never pass test-armhf-armhf-xl-raw 15 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-libvirt-vhd 15 saverestore-support-checkfail never pass version targeted for testing: xen 7cdb1fa2ab0b5e11f66cada0370770404153c824 baseline version: xen f0ff1d9cb96041a84a24857a6464628240deed4f Last test of basis 185865 2024-04-29 08:08:54 Z 22 days Testing same since 186060 2024-05-21 08:38:31 Z0 days1 attempts People who touched revisions under test: Andrew Cooper Daniel P. Smith Demi Marie Obenour Jan Beulich Jason Andryuk Juergen Gross Leigh Brown Roger Pau Monné jobs: build-amd64-xsm pass build-arm64-xsm pass build-i386-xsm pass build-amd64-xtf pass build-amd64 pass build-arm64 pass build-armhf
[XEN PATCH] automation/eclair_analysis: add already clean rules to the analysis
Some MISRA C rules already have no violations in Xen, so they can be
set as clean.
Reorder the rules in tagging.ecl according to version ordering
(i.e. sort -V) and split the configuration on multiple lines for
readability.
Signed-off-by: Nicola Vetrini
---
.../eclair_analysis/ECLAIR/monitored.ecl | 17
automation/eclair_analysis/ECLAIR/tagging.ecl | 78 ++-
2 files changed, 94 insertions(+), 1 deletion(-)
diff --git a/automation/eclair_analysis/ECLAIR/monitored.ecl
b/automation/eclair_analysis/ECLAIR/monitored.ecl
index 9da709dc889c..4daecb0c838f 100644
--- a/automation/eclair_analysis/ECLAIR/monitored.ecl
+++ b/automation/eclair_analysis/ECLAIR/monitored.ecl
@@ -79,4 +79,21 @@
-enable=MC3R1.R9.3
-enable=MC3R1.R9.4
-enable=MC3R1.R9.5
+-enable=MC3R1.R18.8
+-enable=MC3R1.R20.2
+-enable=MC3R1.R20.3
+-enable=MC3R1.R20.6
+-enable=MC3R1.R20.11
+-enable=MC3R1.R21.3
+-enable=MC3R1.R21.4
+-enable=MC3R1.R21.5
+-enable=MC3R1.R21.7
+-enable=MC3R1.R21.8
+-enable=MC3R1.R21.12
+-enable=MC3R1.R22.1
+-enable=MC3R1.R22.3
+-enable=MC3R1.R22.7
+-enable=MC3R1.R22.8
+-enable=MC3R1.R22.9
+-enable=MC3R1.R22.10
-doc_end
diff --git a/automation/eclair_analysis/ECLAIR/tagging.ecl
b/automation/eclair_analysis/ECLAIR/tagging.ecl
index b7a9f75b275b..a354ff322e03 100644
--- a/automation/eclair_analysis/ECLAIR/tagging.ecl
+++ b/automation/eclair_analysis/ECLAIR/tagging.ecl
@@ -19,7 +19,83 @@
-doc_begin="Clean guidelines: new violations for these guidelines are not
accepted."
--service_selector={clean_guidelines_common,
"MC3R1.D1.1||MC3R1.D2.1||MC3R1.D4.1||MC3R1.D4.11||MC3R1.D4.14||MC3R1.R10.2||MC3R1.R11.7||MC3R1.R11.9||MC3R1.R12.5||MC3R1.R1.1||MC3R1.R1.3||MC3R1.R1.4||MC3R1.R14.1||MC3R1.R16.7||MC3R1.R17.1||MC3R1.R17.3||MC3R1.R17.4||MC3R1.R17.5||MC3R1.R17.6||MC3R1.R20.13||MC3R1.R20.14||MC3R1.R20.4||MC3R1.R20.9||MC3R1.R21.10||MC3R1.R21.13||MC3R1.R21.19||MC3R1.R21.21||MC3R1.R21.9||MC3R1.R2.2||MC3R1.R22.2||MC3R1.R22.4||MC3R1.R22.5||MC3R1.R22.6||MC3R1.R2.6||MC3R1.R3.1||MC3R1.R3.2||MC3R1.R4.1||MC3R1.R4.2||MC3R1.R5.1||MC3R1.R5.2||MC3R1.R5.4||MC3R1.R5.6||MC3R1.R6.1||MC3R1.R6.2||MC3R1.R7.1||MC3R1.R7.2||MC3R1.R7.4||MC3R1.R8.1||MC3R1.R8.10||MC3R1.R8.12||MC3R1.R8.14||MC3R1.R8.2||MC3R1.R8.5||MC3R1.R8.6||MC3R1.R8.8||MC3R1.R9.2||MC3R1.R9.3||MC3R1.R9.4||MC3R1.R9.5"
+-service_selector={clean_guidelines_common,
+"MC3R1.D1.1||
+MC3R1.D2.1||
+MC3R1.D4.1||
+MC3R1.D4.11||
+MC3R1.D4.14||
+MC3R1.R1.1||
+MC3R1.R1.3||
+MC3R1.R1.4||
+MC3R1.R2.2||
+MC3R1.R2.6||
+MC3R1.R3.1||
+MC3R1.R3.2||
+MC3R1.R4.1||
+MC3R1.R4.2||
+MC3R1.R5.1||
+MC3R1.R5.2||
+MC3R1.R5.4||
+MC3R1.R5.6||
+MC3R1.R6.1||
+MC3R1.R6.2||
+MC3R1.R7.1||
+MC3R1.R7.2||
+MC3R1.R7.4||
+MC3R1.R8.1||
+MC3R1.R8.2||
+MC3R1.R8.5||
+MC3R1.R8.6||
+MC3R1.R8.8||
+MC3R1.R8.10||
+MC3R1.R8.12||
+MC3R1.R8.14||
+MC3R1.R9.2||
+MC3R1.R9.3||
+MC3R1.R9.4||
+MC3R1.R9.5||
+MC3R1.R10.2||
+MC3R1.R11.7||
+MC3R1.R11.9||
+MC3R1.R12.5||
+MC3R1.R14.1||
+MC3R1.R16.7||
+MC3R1.R17.1||
+MC3R1.R17.3||
+MC3R1.R17.4||
+MC3R1.R17.5||
+MC3R1.R17.6||
+MC3R1.R18.8||
+MC3R1.R20.2||
+MC3R1.R20.3||
+MC3R1.R20.4||
+MC3R1.R20.6||
+MC3R1.R20.9||
+MC3R1.R20.11||
+MC3R1.R20.13||
+MC3R1.R20.14||
+MC3R1.R21.3||
+MC3R1.R21.4||
+MC3R1.R21.5||
+MC3R1.R21.7||
+MC3R1.R21.8||
+MC3R1.R21.9||
+MC3R1.R21.10||
+MC3R1.R21.12||
+MC3R1.R21.13||
+MC3R1.R21.19||
+MC3R1.R21.21||
+MC3R1.R22.1||
+MC3R1.R22.2||
+MC3R1.R22.3||
+MC3R1.R22.4||
+MC3R1.R22.5||
+MC3R1.R22.6||
+MC3R1.R22.7||
+MC3R1.R22.8||
+MC3R1.R22.9||
+MC3R1.R22.10"
}
-setq=target,getenv("XEN_TARGET_ARCH")
--
2.34.1
[xen-unstable test] 186058: tolerable FAIL - PUSHED
flight 186058 xen-unstable real [real] http://logs.test-lab.xenproject.org/osstest/logs/186058/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 186049 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 186049 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 186049 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 186049 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 186049 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 186049 test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-xl 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-qcow2 14 migrate-support-checkfail never pass test-amd64-amd64-libvirt-raw 14 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 15 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail never pass test-arm64-arm64-xl-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-raw 14 migrate-support-checkfail never pass test-armhf-armhf-xl-raw 15 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-libvirt-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit1 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 16 saverestore-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-qcow214 migrate-support-checkfail never pass test-armhf-armhf-xl-qcow215 saverestore-support-checkfail never pass version targeted for testing: xen 26b122e3bf8f3921d87312fbf5e7e13872ae92b0 baseline version: xen 54aa34fc89151c943550541e0af47664fb6e8676 Last test of basis 186049 2024-05-20 18:07:01 Z1 days Testing same since 186058 2024-05-21 05:10:55 Z0 days1 attempts People who touched revisions under test: Andrew Cooper Roger Pau Monné jobs: build-amd64-xsm pass build-arm64-xsm pass build-i386-xsm pass build-amd64-xtf pass build-amd64 pass build-arm64 pass build-armhf pass build-i386 pass
[xen-unstable-smoke test] 186064: tolerable all pass - PUSHED
flight 186064 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/186064/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass version targeted for testing: xen ced21fbb2842ac4655048bdee56232974ff9ff9c baseline version: xen 24dbf5bd03ff11dbcebb4d15e34fbba8eb34936b Last test of basis 186062 2024-05-21 10:00:22 Z0 days Testing same since 186064 2024-05-21 15:04:02 Z0 days1 attempts People who touched revisions under test: Henry Wang Jan Beulich Nicola Vetrini jobs: build-arm64-xsm pass build-amd64 pass build-armhf pass build-amd64-libvirt pass test-armhf-armhf-xl pass test-arm64-arm64-xl-xsm pass test-amd64-amd64-xl-qemuu-debianhvm-amd64pass test-amd64-amd64-libvirt pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Pushing revision : To xenbits.xen.org:/home/xen/git/xen.git 24dbf5bd03..ced21fbb28 ced21fbb2842ac4655048bdee56232974ff9ff9c -> smoke
[PATCH 3/3] xen/x86: Address two misc MISRA 17.7 violations
Neither text_poke() nor watchdog_setup() have return value consulted. Switch
them to being void.
Signed-off-by: Andrew Cooper
---
CC: George Dunlap
CC: Jan Beulich
CC: Stefano Stabellini
CC: Julien Grall
CC: Roberto Bagnara
CC: consult...@bugseng.com
CC: Oleksii Kurochko
---
xen/arch/x86/alternative.c | 4 ++--
xen/arch/x86/nmi.c | 5 ++---
xen/include/xen/watchdog.h | 2 +-
3 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
index 2e7ba6e0b833..7824053c9d33 100644
--- a/xen/arch/x86/alternative.c
+++ b/xen/arch/x86/alternative.c
@@ -155,10 +155,10 @@ void init_or_livepatch add_nops(void *insns, unsigned int
len)
* "noinline" to cause control flow change and thus invalidate I$ and
* cause refetch after modification.
*/
-static void *init_or_livepatch noinline
+static void init_or_livepatch noinline
text_poke(void *addr, const void *opcode, size_t len)
{
-return memcpy(addr, opcode, len);
+memcpy(addr, opcode, len);
}
extern void *const __initdata_cf_clobber_start[];
diff --git a/xen/arch/x86/nmi.c b/xen/arch/x86/nmi.c
index f6329cb0270e..9793fa23168d 100644
--- a/xen/arch/x86/nmi.c
+++ b/xen/arch/x86/nmi.c
@@ -464,12 +464,12 @@ bool watchdog_enabled(void)
return !atomic_read(_disable_count);
}
-int __init watchdog_setup(void)
+void __init watchdog_setup(void)
{
unsigned int cpu;
/*
- * Activate periodic heartbeats. We cannot do this earlier during
+ * Activate periodic heartbeats. We cannot do this earlier during
* setup because the timer infrastructure is not available.
*/
for_each_online_cpu ( cpu )
@@ -477,7 +477,6 @@ int __init watchdog_setup(void)
register_cpu_notifier(_nmi_nfb);
watchdog_enable();
-return 0;
}
/* Returns false if this was not a watchdog NMI, true otherwise */
diff --git a/xen/include/xen/watchdog.h b/xen/include/xen/watchdog.h
index 434fcbdd2b11..40c21bca823f 100644
--- a/xen/include/xen/watchdog.h
+++ b/xen/include/xen/watchdog.h
@@ -10,7 +10,7 @@
#include
/* Try to set up a watchdog. */
-int watchdog_setup(void);
+void watchdog_setup(void);
/* Enable the watchdog. */
void watchdog_enable(void);
--
2.30.2
[PATCH 1/3] xen/lzo: Implement COPY{4,8} using memcpy()
This is simpler and easier for both humans and compilers to read.
It also addresses 6 instances of MISRA R5.3 violation (shadowing of the ptr_
local variable inside both {put,get}_unaligned()).
No change, not even in the compiled binary.
Signed-off-by: Andrew Cooper
---
CC: George Dunlap
CC: Jan Beulich
CC: Stefano Stabellini
CC: Julien Grall
CC: Roberto Bagnara
CC: consult...@bugseng.com
CC: Oleksii Kurochko
---
xen/common/lzo.c | 11 ++-
1 file changed, 2 insertions(+), 9 deletions(-)
diff --git a/xen/common/lzo.c b/xen/common/lzo.c
index cc03f0f5547f..3454ce4a7e24 100644
--- a/xen/common/lzo.c
+++ b/xen/common/lzo.c
@@ -25,15 +25,8 @@
*/
-#define COPY4(dst, src)\
-put_unaligned(get_unaligned((const u32 *)(src)), (u32 *)(dst))
-#if defined(__x86_64__)
-#define COPY8(dst, src)\
-put_unaligned(get_unaligned((const u64 *)(src)), (u64 *)(dst))
-#else
-#define COPY8(dst, src)\
-COPY4(dst, src); COPY4((dst) + 4, (src) + 4)
-#endif
+#define COPY4(dst, src) memcpy(dst, src, 4)
+#define COPY8(dst, src) memcpy(dst, src, 8)
#ifdef __MINIOS__
# include
--
2.30.2
[PATCH 2/3] xen/x86: Drop useless non-Kconfig CONFIG_* variables
These are all either completely unused, or do nothing useful. Signed-off-by: Andrew Cooper --- CC: George Dunlap CC: Jan Beulich CC: Stefano Stabellini CC: Julien Grall CC: Roberto Bagnara CC: consult...@bugseng.com CC: Oleksii Kurochko --- xen/arch/x86/include/asm/config.h | 4 xen/include/xen/acpi.h| 9 - xen/include/xen/watchdog.h| 11 --- 3 files changed, 24 deletions(-) diff --git a/xen/arch/x86/include/asm/config.h b/xen/arch/x86/include/asm/config.h index ab7288cb3682..24b005ba1ff7 100644 --- a/xen/arch/x86/include/asm/config.h +++ b/xen/arch/x86/include/asm/config.h @@ -20,7 +20,6 @@ #define BITS_PER_XEN_ULONG BITS_PER_LONG #define CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS 1 -#define CONFIG_DISCONTIGMEM 1 #define CONFIG_NUMA_EMU 1 #define CONFIG_PAGEALLOC_MAX_ORDER (2 * PAGETABLE_ORDER) @@ -30,11 +29,8 @@ /* Intel P4 currently has largest cache line (L2 line size is 128 bytes). */ #define CONFIG_X86_L1_CACHE_SHIFT 7 -#define CONFIG_ACPI_SRAT 1 #define CONFIG_ACPI_CSTATE 1 -#define CONFIG_WATCHDOG 1 - #define CONFIG_MULTIBOOT 1 #define HZ 100 diff --git a/xen/include/xen/acpi.h b/xen/include/xen/acpi.h index e78e7e785252..bc4818c9430a 100644 --- a/xen/include/xen/acpi.h +++ b/xen/include/xen/acpi.h @@ -140,15 +140,6 @@ int get_cpu_id(u32 acpi_id); unsigned int acpi_register_gsi (u32 gsi, int edge_level, int active_high_low); int acpi_gsi_to_irq (u32 gsi, unsigned int *irq); -/* - * This function undoes the effect of one call to acpi_register_gsi(). - * If this matches the last registration, any IRQ resources for gsi - * are freed. - */ -#ifdef CONFIG_ACPI_DEALLOCATE_IRQ -void acpi_unregister_gsi (u32 gsi); -#endif - #ifdef CONFIG_ACPI_CSTATE /* * max_cstate sets the highest legal C-state. diff --git a/xen/include/xen/watchdog.h b/xen/include/xen/watchdog.h index 86fde0884ae7..434fcbdd2b11 100644 --- a/xen/include/xen/watchdog.h +++ b/xen/include/xen/watchdog.h @@ -9,8 +9,6 @@ #include -#ifdef CONFIG_WATCHDOG - /* Try to set up a watchdog. */ int watchdog_setup(void); @@ -23,13 +21,4 @@ void watchdog_disable(void); /* Is the watchdog currently enabled. */ bool watchdog_enabled(void); -#else - -#define watchdog_setup() ((void)0) -#define watchdog_enable() ((void)0) -#define watchdog_disable() ((void)0) -#define watchdog_enabled() ((void)0) - -#endif - #endif /* __XEN_WATCHDOG_H__ */ -- 2.30.2
[PATCH for-4.19 0/3] xen: Misc MISRA changes
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
--
2.30.2
Re: [PATCH v3 1/2] x86/mm: add API for marking only part of a MMIO page read only
On Tue, May 21, 2024 at 05:16:58PM +0200, Jan Beulich wrote:
> On 21.05.2024 04:54, Marek Marczykowski-Górecki wrote:
> > --- a/xen/arch/x86/include/asm/mm.h
> > +++ b/xen/arch/x86/include/asm/mm.h
> > @@ -522,9 +522,27 @@ extern struct rangeset *mmio_ro_ranges;
> > void memguard_guard_stack(void *p);
> > void memguard_unguard_stack(void *p);
> >
> > +/*
> > + * Add more precise r/o marking for a MMIO page. Range specified here
> > + * will still be R/O, but the rest of the page (not marked as R/O via
> > another
> > + * call) will have writes passed through.
> > + * The start address and the size must be aligned to SUBPAGE_MMIO_RO_ALIGN.
> > + *
> > + * This API cannot be used for overlapping ranges, nor for pages already
> > added
> > + * to mmio_ro_ranges separately.
> > + *
> > + * Return values:
> > + * - negative: error
> > + * - 0: success
> > + */
> > +#define SUBPAGE_MMIO_RO_ALIGN 8
>
> This isn't just alignment, but also (and perhaps more importantly)
> granularity.
> I think the name wants to express this.
SUBPAGE_MMIO_RO_GRANULARITY? Sounds a bit long...
>
> > @@ -4910,6 +4921,260 @@ long arch_memory_op(unsigned long cmd,
> > XEN_GUEST_HANDLE_PARAM(void) arg)
> > return rc;
> > }
> >
> > +/*
> > + * Mark part of the page as R/O.
> > + * Returns:
> > + * - 0 on success - first range in the page
> > + * - 1 on success - subsequent range in the page
> > + * - <0 on error
> > + *
> > + * This needs subpage_ro_lock already taken */
>
> Nit: Comment style (full stop and */ on its own line).
>
> > +static int __init subpage_mmio_ro_add_page(
> > +mfn_t mfn, unsigned int offset_s, unsigned int offset_e)
> > +{
> > +struct subpage_ro_range *entry = NULL, *iter;
> > +unsigned int i;
> > +
> > +list_for_each_entry(iter, _ro_ranges, list)
> > +{
> > +if ( mfn_eq(iter->mfn, mfn) )
> > +{
> > +entry = iter;
> > +break;
> > +}
> > +}
> > +if ( !entry )
> > +{
> > +/* iter == NULL marks it was a newly allocated entry */
> > +iter = NULL;
> > +entry = xzalloc(struct subpage_ro_range);
> > +if ( !entry )
> > +return -ENOMEM;
> > +entry->mfn = mfn;
> > +}
> > +
> > +for ( i = offset_s; i <= offset_e; i += SUBPAGE_MMIO_RO_ALIGN )
> > +{
> > +int oldbit = __test_and_set_bit(i / SUBPAGE_MMIO_RO_ALIGN,
> > +entry->ro_qwords);
>
> Why int, not bool?
Because __test_and_set_bit returns int. But I can change to bool if you
prefer.
> > +ASSERT(!oldbit);
> > +}
> > +
> > +if ( !iter )
> > +list_add(>list, _ro_ranges);
> > +
> > +return iter ? 1 : 0;
> > +}
> > +
> > +/* This needs subpage_ro_lock already taken */
> > +static void __init subpage_mmio_ro_remove_page(
> > +mfn_t mfn,
> > +int offset_s,
> > +int offset_e)
>
> Can either of these be negative? The more that ...
Right, I can change them to unsigned. They are unsigned already in
subpage_mmio_ro_add_page.
> > +{
> > +struct subpage_ro_range *entry = NULL, *iter;
> > +unsigned int i;
>
> ... this is used ...
>
> > +list_for_each_entry(iter, _ro_ranges, list)
> > +{
> > +if ( mfn_eq(iter->mfn, mfn) )
> > +{
> > +entry = iter;
> > +break;
> > +}
> > +}
> > +if ( !entry )
> > +return;
> > +
> > +for ( i = offset_s; i <= offset_e; i += SUBPAGE_MMIO_RO_ALIGN )
>
> ... with both of them?
>
> > +__clear_bit(i / SUBPAGE_MMIO_RO_ALIGN, entry->ro_qwords);
> > +
> > +if ( !bitmap_empty(entry->ro_qwords, PAGE_SIZE /
> > SUBPAGE_MMIO_RO_ALIGN) )
> > +return;
> > +
> > +list_del(>list);
> > +if ( entry->mapped )
> > +iounmap(entry->mapped);
> > +xfree(entry);
> > +}
> > +
> > +int __init subpage_mmio_ro_add(
> > +paddr_t start,
> > +size_t size)
> > +{
> > +mfn_t mfn_start = maddr_to_mfn(start);
> > +paddr_t end = start + size - 1;
> > +mfn_t mfn_end = maddr_to_mfn(end);
> > +unsigned int offset_end = 0;
> > +int rc;
> > +bool subpage_start, subpage_end;
> > +
> > +ASSERT(IS_ALIGNED(start, SUBPAGE_MMIO_RO_ALIGN));
> > +ASSERT(IS_ALIGNED(size, SUBPAGE_MMIO_RO_ALIGN));
> > +if ( !IS_ALIGNED(size, SUBPAGE_MMIO_RO_ALIGN) )
> > +size = ROUNDUP(size, SUBPAGE_MMIO_RO_ALIGN);
> > +
> > +if ( !size )
> > +return 0;
> > +
> > +if ( mfn_eq(mfn_start, mfn_end) )
> > +{
> > +/* Both starting and ending parts handled at once */
> > +subpage_start = PAGE_OFFSET(start) || PAGE_OFFSET(end) !=
> > PAGE_SIZE - 1;
> > +subpage_end = false;
> > +}
> > +else
> > +{
> > +subpage_start = PAGE_OFFSET(start);
> > +subpage_end = PAGE_OFFSET(end) != PAGE_SIZE - 1;
> > +}
> > +
> > +spin_lock(_ro_lock);
> > +
> > +if ( subpage_start )
> > +{
> > +offset_end = mfn_eq(mfn_start,
Re: [PATCH v3 1/2] x86/mm: add API for marking only part of a MMIO page read only
On 21.05.2024 04:54, Marek Marczykowski-Górecki wrote:
> --- a/xen/arch/x86/include/asm/mm.h
> +++ b/xen/arch/x86/include/asm/mm.h
> @@ -522,9 +522,27 @@ extern struct rangeset *mmio_ro_ranges;
> void memguard_guard_stack(void *p);
> void memguard_unguard_stack(void *p);
>
> +/*
> + * Add more precise r/o marking for a MMIO page. Range specified here
> + * will still be R/O, but the rest of the page (not marked as R/O via another
> + * call) will have writes passed through.
> + * The start address and the size must be aligned to SUBPAGE_MMIO_RO_ALIGN.
> + *
> + * This API cannot be used for overlapping ranges, nor for pages already
> added
> + * to mmio_ro_ranges separately.
> + *
> + * Return values:
> + * - negative: error
> + * - 0: success
> + */
> +#define SUBPAGE_MMIO_RO_ALIGN 8
This isn't just alignment, but also (and perhaps more importantly) granularity.
I think the name wants to express this.
> @@ -4910,6 +4921,260 @@ long arch_memory_op(unsigned long cmd,
> XEN_GUEST_HANDLE_PARAM(void) arg)
> return rc;
> }
>
> +/*
> + * Mark part of the page as R/O.
> + * Returns:
> + * - 0 on success - first range in the page
> + * - 1 on success - subsequent range in the page
> + * - <0 on error
> + *
> + * This needs subpage_ro_lock already taken */
Nit: Comment style (full stop and */ on its own line).
> +static int __init subpage_mmio_ro_add_page(
> +mfn_t mfn, unsigned int offset_s, unsigned int offset_e)
> +{
> +struct subpage_ro_range *entry = NULL, *iter;
> +unsigned int i;
> +
> +list_for_each_entry(iter, _ro_ranges, list)
> +{
> +if ( mfn_eq(iter->mfn, mfn) )
> +{
> +entry = iter;
> +break;
> +}
> +}
> +if ( !entry )
> +{
> +/* iter == NULL marks it was a newly allocated entry */
> +iter = NULL;
> +entry = xzalloc(struct subpage_ro_range);
> +if ( !entry )
> +return -ENOMEM;
> +entry->mfn = mfn;
> +}
> +
> +for ( i = offset_s; i <= offset_e; i += SUBPAGE_MMIO_RO_ALIGN )
> +{
> +int oldbit = __test_and_set_bit(i / SUBPAGE_MMIO_RO_ALIGN,
> +entry->ro_qwords);
Why int, not bool?
> +ASSERT(!oldbit);
> +}
> +
> +if ( !iter )
> +list_add(>list, _ro_ranges);
> +
> +return iter ? 1 : 0;
> +}
> +
> +/* This needs subpage_ro_lock already taken */
> +static void __init subpage_mmio_ro_remove_page(
> +mfn_t mfn,
> +int offset_s,
> +int offset_e)
Can either of these be negative? The more that ...
> +{
> +struct subpage_ro_range *entry = NULL, *iter;
> +unsigned int i;
... this is used ...
> +list_for_each_entry(iter, _ro_ranges, list)
> +{
> +if ( mfn_eq(iter->mfn, mfn) )
> +{
> +entry = iter;
> +break;
> +}
> +}
> +if ( !entry )
> +return;
> +
> +for ( i = offset_s; i <= offset_e; i += SUBPAGE_MMIO_RO_ALIGN )
... with both of them?
> +__clear_bit(i / SUBPAGE_MMIO_RO_ALIGN, entry->ro_qwords);
> +
> +if ( !bitmap_empty(entry->ro_qwords, PAGE_SIZE / SUBPAGE_MMIO_RO_ALIGN) )
> +return;
> +
> +list_del(>list);
> +if ( entry->mapped )
> +iounmap(entry->mapped);
> +xfree(entry);
> +}
> +
> +int __init subpage_mmio_ro_add(
> +paddr_t start,
> +size_t size)
> +{
> +mfn_t mfn_start = maddr_to_mfn(start);
> +paddr_t end = start + size - 1;
> +mfn_t mfn_end = maddr_to_mfn(end);
> +unsigned int offset_end = 0;
> +int rc;
> +bool subpage_start, subpage_end;
> +
> +ASSERT(IS_ALIGNED(start, SUBPAGE_MMIO_RO_ALIGN));
> +ASSERT(IS_ALIGNED(size, SUBPAGE_MMIO_RO_ALIGN));
> +if ( !IS_ALIGNED(size, SUBPAGE_MMIO_RO_ALIGN) )
> +size = ROUNDUP(size, SUBPAGE_MMIO_RO_ALIGN);
> +
> +if ( !size )
> +return 0;
> +
> +if ( mfn_eq(mfn_start, mfn_end) )
> +{
> +/* Both starting and ending parts handled at once */
> +subpage_start = PAGE_OFFSET(start) || PAGE_OFFSET(end) != PAGE_SIZE
> - 1;
> +subpage_end = false;
> +}
> +else
> +{
> +subpage_start = PAGE_OFFSET(start);
> +subpage_end = PAGE_OFFSET(end) != PAGE_SIZE - 1;
> +}
> +
> +spin_lock(_ro_lock);
> +
> +if ( subpage_start )
> +{
> +offset_end = mfn_eq(mfn_start, mfn_end) ?
> + PAGE_OFFSET(end) :
> + (PAGE_SIZE - 1);
> +rc = subpage_mmio_ro_add_page(mfn_start,
> + PAGE_OFFSET(start),
> + offset_end);
> +if ( rc < 0 )
> +goto err_unlock;
> +/* Check if not marking R/W part of a page intended to be fully R/O
> */
> +ASSERT(rc || !rangeset_contains_singleton(mmio_ro_ranges,
> + mfn_x(mfn_start)));
> +}
> +
> +if ( subpage_end )
> +{
> +rc =
[xen-unstable-smoke test] 186062: tolerable all pass - PUSHED
flight 186062 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/186062/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass version targeted for testing: xen 24dbf5bd03ff11dbcebb4d15e34fbba8eb34936b baseline version: xen 9c5444b01ad51369bc09197a442a93d87b4b76f2 Last test of basis 186056 2024-05-21 04:09:17 Z0 days Testing same since 186062 2024-05-21 10:00:22 Z0 days1 attempts People who touched revisions under test: Andrew Cooper Oleksii Kurochko Petr Beneš Roger Pau Monné Tamas K Lengyel jobs: build-arm64-xsm pass build-amd64 pass build-armhf pass build-amd64-libvirt pass test-armhf-armhf-xl pass test-arm64-arm64-xl-xsm pass test-amd64-amd64-xl-qemuu-debianhvm-amd64pass test-amd64-amd64-libvirt pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Pushing revision : To xenbits.xen.org:/home/xen/git/xen.git 9c5444b01a..24dbf5bd03 24dbf5bd03ff11dbcebb4d15e34fbba8eb34936b -> smoke
[libvirt test] 186057: tolerable all pass - PUSHED
flight 186057 libvirt real [real] http://logs.test-lab.xenproject.org/osstest/logs/186057/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 186025 test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-arm64-arm64-libvirt 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qcow2 14 migrate-support-checkfail never pass test-amd64-amd64-libvirt-raw 14 migrate-support-checkfail never pass test-armhf-armhf-libvirt 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-qcow2 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-qcow2 15 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-libvirt-vhd 15 saverestore-support-checkfail never pass version targeted for testing: libvirt 7c8e606b64c73ca56d7134cb16d01257f39c53ef baseline version: libvirt 34f52aec286af47066484e8a96ecba0ef8e79451 Last test of basis 186025 2024-05-17 04:20:41 Z4 days Testing same since 186057 2024-05-21 04:18:53 Z0 days1 attempts People who touched revisions under test: Göran Uddeborg Jonathon Jongsma jobs: build-amd64-xsm pass build-arm64-xsm pass build-i386-xsm pass build-amd64 pass build-arm64 pass build-armhf pass build-i386 pass build-amd64-libvirt pass build-arm64-libvirt pass build-armhf-libvirt pass build-i386-libvirt pass build-amd64-pvopspass build-arm64-pvopspass build-armhf-pvopspass build-i386-pvops pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm pass test-amd64-amd64-libvirt-xsm pass test-arm64-arm64-libvirt-xsm pass test-amd64-amd64-libvirt pass test-arm64-arm64-libvirt pass test-armhf-armhf-libvirt pass test-amd64-amd64-libvirt-pairpass test-amd64-amd64-libvirt-qcow2 pass test-arm64-arm64-libvirt-qcow2 pass test-amd64-amd64-libvirt-raw pass test-arm64-arm64-libvirt-raw pass test-amd64-amd64-libvirt-vhd pass test-armhf-armhf-libvirt-vhd pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Pushing revision : To xenbits.xen.org:/home/xen/git/libvirt.git 34f52aec28..7c8e606b64 7c8e606b64c73ca56d7134cb16d01257f39c53ef -> xen-tested-master
Re: [PATCH v15 1/5] arm/vpci: honor access size when returning an error
Hi Stewart, On 17/05/2024 18:06, Stewart Hildebrand wrote: From: Volodymyr Babchuk Guest can try to read config space using different access sizes: 8, 16, 32, 64 bits. We need to take this into account when we are returning an error back to MMIO handler, otherwise it is possible to provide more data than requested: i.e. guest issues LDRB instruction to read one byte, but we are writing 0x in the target register. Signed-off-by: Volodymyr Babchuk Signed-off-by: Stewart Hildebrand Acked-by: Julien Grall Cheers, -- Julien Grall
Re: [PATCH v3 7/8] tools: Introduce the "xl dt-overlay {attach,detach}" commands
On 2024-05-21 00:35, Henry Wang wrote:
With the XEN_DOMCTL_dt_overlay DOMCTL added, users should be able to
attach/detach devices from the provided DT overlay to domains.
Support this by introducing a new set of "xl dt-overlay" commands and
related documentation, i.e. "xl dt-overlay {attach,detach}". Slightly
rework the command option parsing logic.
Signed-off-by: Henry Wang
Reviewed-by: Jason Andryuk
Thanks,
Jason
[linux-linus test] 186052: tolerable FAIL - PUSHED
flight 186052 linux-linus real [real] flight 186061 linux-linus real-retest [real] http://logs.test-lab.xenproject.org/osstest/logs/186052/ http://logs.test-lab.xenproject.org/osstest/logs/186061/ Failures :-/ but no regressions. Tests which are failing intermittently (not blocking): test-arm64-arm64-xl-thunderx 8 xen-bootfail pass in 186061-retest test-armhf-armhf-xl-credit2 8 xen-bootfail pass in 186061-retest Tests which did not succeed, but are not blocking: test-arm64-arm64-xl-thunderx 15 migrate-support-check fail in 186061 never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-check fail in 186061 never pass test-armhf-armhf-xl-credit2 15 migrate-support-check fail in 186061 never pass test-armhf-armhf-xl-credit2 16 saverestore-support-check fail in 186061 never pass test-armhf-armhf-xl-qcow2 8 xen-boot fail like 186044 test-armhf-armhf-libvirt 8 xen-boot fail like 186044 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 186047 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 186047 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 186047 test-armhf-armhf-xl-credit1 8 xen-boot fail like 186047 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 186047 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 186047 test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-xl 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-rtds 15 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qcow2 14 migrate-support-checkfail never pass test-amd64-amd64-libvirt-raw 14 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-libvirt-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail never pass test-armhf-armhf-xl-raw 14 migrate-support-checkfail never pass test-armhf-armhf-xl-raw 15 saverestore-support-checkfail never pass version targeted for testing: linux8f6a15f095a63a83b096d9b29aaff4f0fbe6f6e6 baseline version: linuxeb6a9339efeb6f3d2b5c86fdf2382cdc293eca2c Last test of basis 186047 2024-05-20 15:13:33 Z0 days Testing same since 186052 2024-05-21 01:42:42 Z0 days1 attempts People who touched revisions under test: "Darrick J. Wong" Abhinav Jain Alex Williamson Alexander Lobakin Alexander Stein Allison Henderson Amir Goldstein Andi Shyti Andrew Geissler Andrew Jeffery André Draszik Andy Shevchenko Andy Shevchenko Animesh Agarwal Anjelique Melendez Ard Biesheuvel Arnd Bergmann Arnd Bergmann Baruch Siach Bryan O'Donoghue Chandan Babu R Chao Yu Chen Ni Chen-Yu Tsai Christoph Hellwig Christophe JAILLET Colin Ian King Conor Dooley Daeho Jeong Dan Carpenter
Re: [PATCH v3 4/8] tools/arm: Introduce the "nr_spis" xl config entry
On 2024-05-21 00:35, Henry Wang wrote: Currently, the number of SPIs allocated to the domain is only configurable for Dom0less DomUs. Xen domains are supposed to be platform agnostics and therefore the numbers of SPIs for libxl guests should not be based on the hardware. Introduce a new xl config entry for Arm to provide a method for user to decide the number of SPIs. This would help to avoid bumping the `config->arch.nr_spis` in libxl everytime there is a new platform with increased SPI numbers. Update the doc and the golang bindings accordingly. Signed-off-by: Henry Wang Reviewed-by: Jason Andryuk Thanks, Jason
Re: [PATCH v3 5/8] xen/arm/gic: Allow routing/removing interrupt to running VMs
Hi, On 21/05/2024 05:35, Henry Wang wrote: From: Vikram Garhwal Currently, routing/removing physical interrupts are only allowed at the domain creation/destroy time. For use cases such as dynamic device tree overlay adding/removing, the routing/removing of physical IRQ to running domains should be allowed. Removing the above-mentioned domain creation/dying check. Since this will introduce interrupt state unsync issues for cases when the interrupt is active or pending in the guest, therefore for these cases we simply reject the operation. Do it for both new and old vGIC implementations. Signed-off-by: Vikram Garhwal Signed-off-by: Stefano Stabellini Signed-off-by: Henry Wang --- v3: - Update in-code comments. - Correct the if conditions. - Add taking/releasing the vgic lock of the vcpu. v2: - Reject the case where the IRQ is active or pending in guest. --- xen/arch/arm/gic-vgic.c | 15 --- xen/arch/arm/gic.c | 15 --- xen/arch/arm/vgic/vgic.c | 10 +++--- 3 files changed, 19 insertions(+), 21 deletions(-) diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c index 56490dbc43..956c11ba13 100644 --- a/xen/arch/arm/gic-vgic.c +++ b/xen/arch/arm/gic-vgic.c @@ -439,24 +439,33 @@ int vgic_connect_hw_irq(struct domain *d, struct vcpu *v, unsigned int virq, /* We are taking to rank lock to prevent parallel connections. */ vgic_lock_rank(v_target, rank, flags); +spin_lock(_target->arch.vgic.lock); I know this is what Stefano suggested, but v_target would point to the current affinity whereas the interrupt may be pending/active on the "previous" vCPU. So it is a little unclear whether v_target is the correct lock. Do you have more pointer to show this is correct? Also, while looking at the locking, I noticed that we are not doing anything with GIC_IRQ_GUEST_MIGRATING. In gic_update_one_lr(), we seem to assume that if the flag is set, then p->desc cannot be NULL. Can we reach vgic_connect_hw_irq() with the flag set? What about the other flags? Is this going to be a concern if we don't reset them? Cheers, -- Julien Grall
Re: [XEN PATCH 4/4] x86_64/cpu_idle: address violations of MISRA C Rule 20.7
On 16.05.2024 01:19, Stefano Stabellini wrote: > On Wed, 15 May 2024, Nicola Vetrini wrote: >> MISRA C Rule 20.7 states: "Expressions resulting from the expansion >> of macro parameters shall be enclosed in parentheses". Therefore, some >> macro definitions should gain additional parentheses to ensure that all >> current and future users will be safe with respect to expansions that >> can possibly alter the semantics of the passed-in macro parameter. >> >> No functional change. >> >> Signed-off-by: Nicola Vetrini > > Reviewed-by: Stefano Stabellini Acked-by: Jan Beulich
Re: [XEN PATCH 3/4] x86_64/uaccess: address violations of MISRA C Rule 20.7
On 16.05.2024 01:19, Stefano Stabellini wrote: > On Wed, 15 May 2024, Nicola Vetrini wrote: >> MISRA C Rule 20.7 states: "Expressions resulting from the expansion >> of macro parameters shall be enclosed in parentheses". Therefore, some >> macro definitions should gain additional parentheses to ensure that all >> current and future users will be safe with respect to expansions that >> can possibly alter the semantics of the passed-in macro parameter. >> >> xlat_malloc_init is touched for consistency, despite the construct >> being already deviated. >> >> No functional change. >> >> Signed-off-by: Nicola Vetrini > > Reviewed-by: Stefano Stabellini Acked-by: Jan Beulich
Re: [XEN PATCH 2/4] x86/hvm: address violations of MISRA C Rule 20.7
On 16.05.2024 01:18, Stefano Stabellini wrote: > On Wed, 15 May 2024, Nicola Vetrini wrote: >> MISRA C Rule 20.7 states: "Expressions resulting from the expansion >> of macro parameters shall be enclosed in parentheses". Therefore, some >> macro definitions should gain additional parentheses to ensure that all >> current and future users will be safe with respect to expansions that >> can possibly alter the semantics of the passed-in macro parameter. >> >> No functional change. >> >> Signed-off-by: Nicola Vetrini > > Reviewed-by: Stefano Stabellini > > >> --- >> xen/arch/x86/hvm/mtrr.c | 2 +- >> xen/arch/x86/hvm/rtc.c | 2 +- >> xen/arch/x86/include/asm/hvm/save.h | 2 +- >> 3 files changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/xen/arch/x86/hvm/mtrr.c b/xen/arch/x86/hvm/mtrr.c >> index 32f74c1db03b..1079851f70ed 100644 >> --- a/xen/arch/x86/hvm/mtrr.c >> +++ b/xen/arch/x86/hvm/mtrr.c >> @@ -16,7 +16,7 @@ >> #include >> >> /* Get page attribute fields (PAn) from PAT MSR. */ >> -#define pat_cr_2_paf(pat_cr,n) uint64_t)pat_cr) >> ((n)<<3)) & 0xff) >> +#define pat_cr_2_paf(pat_cr, n) uint64_t)(pat_cr)) >> ((n) << 3)) & >> 0xff) > > just a cosmetic change Not really, as Nicola already pointed out. However, there's an excess pair of parentheses which better would have been re-arranged rather than adding yet another pair: #define pat_cr_2_paf(pat_cr, n) (((uint64_t)(pat_cr) >> ((n) << 3)) & 0xff) Preferably with that (which I'll likely take the liberty of adjusting while committing) Acked-by: Jan Beulich Jan
Re: [XEN PATCH 1/4] x86/vpmu: address violations of MISRA C Rule 20.7
On 16.05.2024 01:17, Stefano Stabellini wrote: > On Wed, 15 May 2024, Nicola Vetrini wrote: >> MISRA C Rule 20.7 states: "Expressions resulting from the expansion >> of macro parameters shall be enclosed in parentheses". Therefore, some >> macro definitions should gain additional parentheses to ensure that all >> current and future users will be safe with respect to expansions that >> can possibly alter the semantics of the passed-in macro parameter. >> >> No functional change. >> >> Signed-off-by: Nicola Vetrini > > Reviewed-by: Stefano Stabellini Acked-by: Jan Beulich
Re: [PATCH v10 03/14] xen/bitops: implement fls{l}() in common logic
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.
> --- 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 v10 02/14] xen: introduce generic non-atomic test_*bit()
On 17.05.2024 15:54, 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
>
> 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
with one remaining nit (can be adjusted while committing, provided other
necessary acks arrive):
> --- 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.
> + */
> +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.
> + */
> +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! */
> +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 - Determine whether a bit is set
> + * @nr: bit number to test
> + * @addr: Address to start counting from
> + */
> +static always_inline bool generic_test_bit(int nr, const volatile void *addr)
> +{
> +bitop_uint_t mask = BITOP_MASK(nr);
> +const volatile bitop_uint_t *p =
> +(const volatile bitop_uint_t *)addr + BITOP_WORD(nr);
Indentation is odd here. Seeing that the line needs wrapping here, it would
imo want to be
const volatile bitop_uint_t *p =
(const volatile bitop_uint_t *)addr + BITOP_WORD(nr);
(i.e. one indentation level further from what the start of the decl has).
Jan
Re: [PATCH for-4.19? v4 4/6] x86: Make the maximum number of altp2m views configurable
On 18.05.2024 13:02, Petr Beneš wrote:
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -685,6 +685,18 @@ int arch_sanitise_domain_config(struct
> xen_domctl_createdomain *config)
> return -EINVAL;
> }
>
> +if ( config->nr_altp2m && !hvm_altp2m_supported() )
> +{
> +dprintk(XENLOG_INFO, "altp2m requested but not available\n");
> +return -EINVAL;
> +}
> +
> +if ( config->nr_altp2m > MAX_EPTP )
The compared entities don't really fit together. I think we want a new
MAX_NR_ALTP2M, which - for the time being - could simply be
#define MAX_NR_ALTP2M MAX_EPTP
in the header. That would then be a suitable replacement for the
min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) that you're adjusting
elsewhere. Which however raises the question whether in EPT-specific
code the min() wouldn't better survive, as min(d->nr_altp2m, MAX_EPTP).
> @@ -5228,7 +5234,7 @@ void hvm_fast_singlestep(struct vcpu *v, uint16_t
> p2midx)
> if ( !hvm_is_singlestep_supported() )
> return;
>
> -if ( p2midx >= MAX_ALTP2M )
> +if ( p2midx >= v->domain->nr_altp2m )
> return;
You don't introduce a new local variable here. I'd like to ask that you also
don't ...
> @@ -403,12 +403,12 @@ long p2m_set_mem_access_multi(struct domain *d,
> /* altp2m view 0 is treated as the hostp2m */
> if ( altp2m_idx )
> {
> -if ( altp2m_idx >= min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) ||
> - d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)] ==
> - mfn_x(INVALID_MFN) )
> +if ( altp2m_idx >= d->nr_altp2m ||
> + d->arch.altp2m_eptp[array_index_nospec(altp2m_idx,
> d->nr_altp2m)]
> + == mfn_x(INVALID_MFN) )
Please don't break previously correct style: Binary operators (here: == )
belong onto the end of the earlier line. That'll render the line too long
again, but you want to deal with that e.g. thus:
d->arch.altp2m_eptp[array_index_nospec(altp2m_idx,
d->nr_altp2m)] ==
mfn_x(INVALID_MFN) )
Jan
Re: [PATCH for-4.19 v3 2/3] xen: enable altp2m at create domain domctl
On 17.05.2024 15:33, Roger Pau Monne wrote:
> Enabling it using an HVM param is fragile, and complicates the logic when
> deciding whether options that interact with altp2m can also be enabled.
>
> Leave the HVM param value for consumption by the guest, but prevent it from
> being set. Enabling is now done using and additional altp2m specific field in
> xen_domctl_createdomain.
>
> Note that albeit only currently implemented in x86, altp2m could be
> implemented
> in other architectures, hence why the field is added to
> xen_domctl_createdomain
> instead of xen_arch_domainconfig.
>
> Signed-off-by: Roger Pau Monné
Reviewed-by: Jan Beulich # hypervisor
albeit with one question:
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -637,6 +637,8 @@ int arch_sanitise_domain_config(struct
> xen_domctl_createdomain *config)
> bool hap = config->flags & XEN_DOMCTL_CDF_hap;
> bool nested_virt = config->flags & XEN_DOMCTL_CDF_nested_virt;
> unsigned int max_vcpus;
> +unsigned int altp2m_mode = MASK_EXTR(config->altp2m_opts,
> + XEN_DOMCTL_ALTP2M_mode_mask);
>
> if ( hvm ? !hvm_enabled : !IS_ENABLED(CONFIG_PV) )
> {
> @@ -715,6 +717,26 @@ int arch_sanitise_domain_config(struct
> xen_domctl_createdomain *config)
> return -EINVAL;
> }
>
> +if ( config->altp2m_opts & ~XEN_DOMCTL_ALTP2M_mode_mask )
> +{
> +dprintk(XENLOG_INFO, "Invalid altp2m options selected: %#x\n",
> +config->flags);
> +return -EINVAL;
> +}
> +
> +if ( altp2m_mode && nested_virt )
> +{
> +dprintk(XENLOG_INFO,
> +"Nested virt and altp2m are not supported together\n");
> +return -EINVAL;
> +}
> +
> +if ( altp2m_mode && !hap )
> +{
> +dprintk(XENLOG_INFO, "altp2m is only supported with HAP\n");
> +return -EINVAL;
> +}
Should this last one perhaps be further extended to permit altp2m with EPT
only?
Jan
Re: [PATCH v2 05/12] IOMMU: rename and re-type ats_enabled
On 21.05.2024 12:03, Roger Pau Monné wrote: > On Tue, May 21, 2024 at 08:21:35AM +0200, Jan Beulich wrote: >> On 20.05.2024 12:29, Roger Pau Monné wrote: >>> On Wed, May 15, 2024 at 12:07:50PM +0200, Jan Beulich wrote: On 06.05.2024 15:53, Roger Pau Monné wrote: > On Mon, May 06, 2024 at 03:20:38PM +0200, Jan Beulich wrote: >> On 06.05.2024 14:42, Roger Pau Monné wrote: >>> On Thu, Feb 15, 2024 at 11:15:39AM +0100, Jan Beulich wrote: @@ -196,7 +196,7 @@ static int __must_check amd_iommu_setup_ dte->sys_mgt = MASK_EXTR(ivrs_dev->device_flags, ACPI_IVHD_SYSTEM_MGMT); if ( use_ats(pdev, iommu, ivrs_dev) ) -dte->i = ats_enabled; +dte->i = true; >>> >>> Might be easier to just use: >>> >>> dte->i = use_ats(pdev, iommu, ivrs_dev); >> >> I'm hesitant here, as in principle we might be overwriting a "true" by >> "false" then. > > Hm, but that would be fine, what's the point in enabling the IOMMU to > reply to ATS requests if ATS is not enabled on the device? > > IOW: overwriting a "true" with a "false" seem like the correct > behavior if it's based on the output of use_ats(). I don't think so, unless there were flow guarantees excluding the possibility of taking this path twice without intermediately disabling the device again. Down from here the enabling of ATS is gated on use_ats(). Hence if, in an earlier invocation, we enabled ATS (and set dte->i), we wouldn't turn off ATS below (there's only code to turn it on), yet with what you suggest we'd clear dte->i. >>> >>> Please bear with me, I think I'm confused, why would use_ats(), and if >>> that's the case, don't we want to update dte->i so that it matches the >>> ATS state? >> >> I'm afraid I can't parse this. Maybe a result of incomplete editing? The >> topic is complex enough that I don't want to even try to guess what you >> may have meant to ask ... > > Oh, indeed, sorry, the full sentences should have been: > > Please bear with me, I think I'm confused, why would use_ats() return > different values for the same device? Right now it can't, yet in principle opt_ats could change value when wired up accordingly via hypfs. > And if that's the case, don't we want to update dte->i so that it > matches the ATS state signaled by use_ats()? That depends on what adjustment would be done besides changing the variable value, if that was to become runtime changeable. >>> Otherwise we would fail to disable IOMMU device address translation >>> support if ATS was disabled? >> >> I think the answer here is "no", but with the above I'm not really sure >> here, either. > > Given the current logic in use_ats() AFAICT the return value of that > function should not change for a given device? Right now it shouldn't, yes. I'm still a little hesitant to have callers make implications from that. Jan
Re: [XEN PATCH v3 2/6] x86/intel: move vmce_has_lmce() routine to header
On 21.05.2024 12:00, Sergiy Kibrik wrote: > 21.05.24 09:05, Jan Beulich: >>> This is because MCG_LMCE_P is defined in arch/x86/cpu/mcheck/x86_mca.h >>> -- so either MCG_LMCE_P (+ a bunch of MCG_* declarations) has to be >>> moved to common header to be accessible, or local x86_mca.h got to be >>> included from common arch/x86/include/asm/mce.h. >>> >>> As for the MCG_* declarations movement I didn't think there's a good >>> enough reason to do it; as for the inclusion of x86_mca.h it didn't look >>> nice at all. >> I'm afraid I don't follow the latter: Why's including x86_mca.h any worse >> than what you do right now? > > To include x86_mca.h from asm/mce.h something like this line would be > needed: > > #include "../../cpu/mcheck/x86_mca.h" > > I've found only two include-s of such kind, so I presume they're not common. Indeed, and I have to apologize for not reading your earlier reply quite right. Jan > Besides xen/sched.h includes asm/mce.h before declaration of struct > vcpu, so body of vmce_has_lmce(const struct vcpu *v) can't really be > compiled in asm/mce.h > >-Sergiy
Re: [PATCH v2 05/12] IOMMU: rename and re-type ats_enabled
On Tue, May 21, 2024 at 08:21:35AM +0200, Jan Beulich wrote: > On 20.05.2024 12:29, Roger Pau Monné wrote: > > On Wed, May 15, 2024 at 12:07:50PM +0200, Jan Beulich wrote: > >> On 06.05.2024 15:53, Roger Pau Monné wrote: > >>> On Mon, May 06, 2024 at 03:20:38PM +0200, Jan Beulich wrote: > On 06.05.2024 14:42, Roger Pau Monné wrote: > > On Thu, Feb 15, 2024 at 11:15:39AM +0100, Jan Beulich wrote: > >> @@ -196,7 +196,7 @@ static int __must_check amd_iommu_setup_ > >> dte->sys_mgt = MASK_EXTR(ivrs_dev->device_flags, > >> ACPI_IVHD_SYSTEM_MGMT); > >> > >> if ( use_ats(pdev, iommu, ivrs_dev) ) > >> -dte->i = ats_enabled; > >> +dte->i = true; > > > > Might be easier to just use: > > > > dte->i = use_ats(pdev, iommu, ivrs_dev); > > I'm hesitant here, as in principle we might be overwriting a "true" by > "false" then. > >>> > >>> Hm, but that would be fine, what's the point in enabling the IOMMU to > >>> reply to ATS requests if ATS is not enabled on the device? > >>> > >>> IOW: overwriting a "true" with a "false" seem like the correct > >>> behavior if it's based on the output of use_ats(). > >> > >> I don't think so, unless there were flow guarantees excluding the > >> possibility > >> of taking this path twice without intermediately disabling the device > >> again. > >> Down from here the enabling of ATS is gated on use_ats(). Hence if, in an > >> earlier invocation, we enabled ATS (and set dte->i), we wouldn't turn off > >> ATS > >> below (there's only code to turn it on), yet with what you suggest we'd > >> clear > >> dte->i. > > > > Please bear with me, I think I'm confused, why would use_ats(), and if > > that's the case, don't we want to update dte->i so that it matches the > > ATS state? > > I'm afraid I can't parse this. Maybe a result of incomplete editing? The > topic is complex enough that I don't want to even try to guess what you > may have meant to ask ... Oh, indeed, sorry, the full sentences should have been: Please bear with me, I think I'm confused, why would use_ats() return different values for the same device? And if that's the case, don't we want to update dte->i so that it matches the ATS state signaled by use_ats()? > > Otherwise we would fail to disable IOMMU device address translation > > support if ATS was disabled? > > I think the answer here is "no", but with the above I'm not really sure > here, either. Given the current logic in use_ats() AFAICT the return value of that function should not change for a given device? Thanks, Roger.
Re: [XEN PATCH v3 2/6] x86/intel: move vmce_has_lmce() routine to header
21.05.24 09:05, Jan Beulich: This is because MCG_LMCE_P is defined in arch/x86/cpu/mcheck/x86_mca.h -- so either MCG_LMCE_P (+ a bunch of MCG_* declarations) has to be moved to common header to be accessible, or local x86_mca.h got to be included from common arch/x86/include/asm/mce.h. As for the MCG_* declarations movement I didn't think there's a good enough reason to do it; as for the inclusion of x86_mca.h it didn't look nice at all. I'm afraid I don't follow the latter: Why's including x86_mca.h any worse than what you do right now? To include x86_mca.h from asm/mce.h something like this line would be needed: #include "../../cpu/mcheck/x86_mca.h" I've found only two include-s of such kind, so I presume they're not common. Besides xen/sched.h includes asm/mce.h before declaration of struct vcpu, so body of vmce_has_lmce(const struct vcpu *v) can't really be compiled in asm/mce.h -Sergiy
[xen-unstable-smoke test] 186056: tolerable all pass - PUSHED
flight 186056 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/186056/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass version targeted for testing: xen 9c5444b01ad51369bc09197a442a93d87b4b76f2 baseline version: xen 26b122e3bf8f3921d87312fbf5e7e13872ae92b0 Last test of basis 186048 2024-05-20 18:02:09 Z0 days Testing same since 186050 2024-05-20 22:02:07 Z0 days2 attempts People who touched revisions under test: Andrew Cooper Jan Beulich jobs: build-arm64-xsm pass build-amd64 pass build-armhf pass build-amd64-libvirt pass test-armhf-armhf-xl pass test-arm64-arm64-xl-xsm pass test-amd64-amd64-xl-qemuu-debianhvm-amd64pass test-amd64-amd64-libvirt pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Pushing revision : To xenbits.xen.org:/home/xen/git/xen.git 26b122e3bf..9c5444b01a 9c5444b01ad51369bc09197a442a93d87b4b76f2 -> smoke
Re: [PATCH for-4.19] xen/x86: limit interrupt movement done by fixup_irqs()
On 16.05.2024 17:56, Roger Pau Monné wrote:
> On Thu, May 16, 2024 at 05:00:54PM +0200, Jan Beulich wrote:
>> On 16.05.2024 15:22, Roger Pau Monne wrote:
>>> --- a/xen/arch/x86/irq.c
>>> +++ b/xen/arch/x86/irq.c
>>> @@ -2527,7 +2527,7 @@ static int __init cf_check setup_dump_irqs(void)
>>> }
>>> __initcall(setup_dump_irqs);
>>>
>>> -/* Reset irq affinities to match the given CPU mask. */
>>> +/* Evacuate interrupts assigned to CPUs not present in the input CPU mask.
>>> */
>>> void fixup_irqs(const cpumask_t *mask, bool verbose)
>>> {
>>
>> Evacuating is one purpose. Updating affinity, if need be, is another. I've
>> been wondering more than once though whether it is actually correct /
>> necessary for ->affinity to be updated by the function. As it stands you
>> don't remove the respective code, though.
>
> Yeah, I didn't want to get into updating ->affinity in this patch, so
> decided to leave that as-is.
>
> Note however that if we shuffle the interrupt around we should update
> ->affinity, so that the new destination is part of ->affinity?
I would put it differently: If we shuffle the IRQ around, we want to
respect ->affinity if at all possible. Only if that's impossible (all CPUs
in ->affinity offline) we may need to update ->affinity as well. Issue is
that ...
> Otherwise we could end up with the interrupt assigned to CPU(s) that
> are not part of the ->affinity mask. Maybe that's OK, TBH I'm not
> sure I understand the purpose of the ->affinity mask, hence why I've
> decided to leave it alone in this patch.
..., as you say, it's not entirely clear what ->affinity's purpose is, and
hence whether it might be okay(ish) to leave it without any intersection
with online CPUs. If we were to permit that, I'm relatively sure though
that then other code may need updating (it'll at least need auditing).
>>> @@ -2576,7 +2576,12 @@ void fixup_irqs(const cpumask_t *mask, bool verbose)
>>> release_old_vec(desc);
>>> }
>>>
>>> -if ( !desc->action || cpumask_subset(desc->affinity, mask) )
>>> +/*
>>> + * Avoid shuffling the interrupt around if it's assigned to a CPU
>>> set
>>> + * that's all covered by the requested affinity mask.
>>> + */
>>> +cpumask_and(affinity, desc->arch.cpu_mask, _online_map);
>>> +if ( !desc->action || cpumask_subset(affinity, mask) )
>>> {
>>> spin_unlock(>lock);
>>> continue;
>>
>> First my understanding of how the two CPU sets are used: ->affinity is
>> merely a representation of where the IRQ is permitted to be handled.
>> ->arch.cpu_mask describes all CPUs where the assigned vector is valid
>> (and would thus need cleaning up when a new vector is assigned). Neither
>> of the two needs to be a strict subset of the other.
>
> Oh, so it's allowed to have the interrupt target a CPU
> (->arch.cpu_mask) that's not set in the affinity mask?
To be honest I'm not quite sure whether it's "allowed" or merely "happens
to".
Jan
Re: [PATCH for-4.19] xen/x86: limit interrupt movement done by fixup_irqs()
On 16.05.2024 18:23, Roger Pau Monné wrote:
> On Thu, May 16, 2024 at 06:04:22PM +0200, Jan Beulich wrote:
>> On 16.05.2024 17:56, Roger Pau Monné wrote:
>>> On Thu, May 16, 2024 at 05:00:54PM +0200, Jan Beulich wrote:
On 16.05.2024 15:22, Roger Pau Monne wrote:
> @@ -2576,7 +2576,12 @@ void fixup_irqs(const cpumask_t *mask, bool
> verbose)
> release_old_vec(desc);
> }
>
> -if ( !desc->action || cpumask_subset(desc->affinity, mask) )
> +/*
> + * Avoid shuffling the interrupt around if it's assigned to a
> CPU set
> + * that's all covered by the requested affinity mask.
> + */
> +cpumask_and(affinity, desc->arch.cpu_mask, _online_map);
> +if ( !desc->action || cpumask_subset(affinity, mask) )
> {
> spin_unlock(>lock);
> continue;
[...]
In
which case cpumask_subset() is going to always return true with your
change in place, if I'm not mistaken. That seems to make your change
questionable. Yet with that I guess I'm overlooking something.)
>>>
>>> I might we wrong, but I think you are missing that the to be offlined
>>> CPU has been removed from cpu_online_map by the time it gets passed
>>> to fixup_irqs().
>>
>> Just on this part (I'll need to take more time to reply to other parts):
>> No, I've specifically paid attention to that fact. Yet for this particular
>> observation of mine is doesn't matter. If mask == _online_map, then
>> no matter what is in cpu_online_map
>>
>> cpumask_and(affinity, desc->arch.cpu_mask, _online_map);
>>
>> will mask things down to a subset of cpu_online_map, and hence
>>
>> if ( !desc->action || cpumask_subset(affinity, mask) )
>>
>> (effectively being
>>
>> if ( !desc->action || cpumask_subset(affinity, _online_map) )
>>
>> ) is nothing else than
>>
>> if ( !desc->action || true )
>>
>> . Yet that doesn't feel quite right.
>
> Oh, I get it now. Ideally we would use cpu_online_map with the to be
> removed CPU set, but that's complicated in this context.
>
> For the purposes here we might as well avoid the AND of
> ->arch.cpu_mask with cpu_online_map and just check:
>
> if ( !desc->action || cpumask_subset(desc->arch.cpu_mask, mask) )
Right, just that I wouldn't say "as well" - we simply may not mask with
cpu_online_map, for the reason stated in the earlier reply.
However, I remain unconvinced that we can outright drop the check of
->affinity. While I doubt cpumask_subset() was correct before, if there's
no intersection with cpu_online_map we still need to update ->affinity
too, to avoid it becoming an "impossible" setting. So I continue to think
that the logic as we have it right now may need splitting into two parts,
one dealing with IRQ movement and the other with ->affinity.
> As even if ->arch.cpu_mask has non-online CPUs set aside from the to
> be offlined CPU, it would just mean that we might be shuffling more
> than strictly necessary.
Limiting the overall benefit of your change, but yes.
> Note this will only be an issue with cluster
> mode, physical mode must always have a single online CPU set in
> ->arch.cpu_mask.
Sure.
Jan
Re: [PATCH v3] xen/arm: Set correct per-cpu cpu_core_mask
On 21/05/2024 09:51, Henry Wang wrote: > Hi Michal, > > On 5/21/2024 3:47 PM, Michal Orzel wrote: >> Hi Henry. >> >> On 3/21/2024 11:57 AM, Henry Wang wrote: In the common sysctl command XEN_SYSCTL_physinfo, the value of cores_per_socket is calculated based on the cpu_core_mask of CPU0. Currently on Arm this is a fixed value 1 (can be checked via xl info), which is not correct. This is because during the Arm CPU online process at boot time, setup_cpu_sibling_map() only sets the per-cpu cpu_core_mask for itself. cores_per_socket refers to the number of cores that belong to the same socket (NUMA node). Currently Xen on Arm does not support physical CPU hotplug and NUMA, also we assume there is no multithread. Therefore cores_per_socket means all possible CPUs detected from the device tree. Setting the per-cpu cpu_core_mask in setup_cpu_sibling_map() accordingly. Modify the in-code comment which seems to be outdated. Add a warning to users if Xen is running on processors with multithread support. Signed-off-by: Henry Wang Signed-off-by: Henry Wang >> Reviewed-by: Michal Orzel > > Thanks. > /* ID of the PCPU we're running on */ DEFINE_PER_CPU(unsigned int, cpu_id); -/* XXX these seem awfully x86ish... */ +/* + * Although multithread is part of the Arm spec, there are not many + * processors support multithread and current Xen on Arm assumes there >> NIT: s/support/supporting > > Sorry, it should have been spotted locally before sending. Anyway, I > will correct this in v4 with your Reviewed-by tag taken. Thanks for > pointing this out. I don't think there is a need to resend a patch just for fixing this typo. It can be done on commit. ~Michal
Re: [PATCH v3] xen/arm: Set correct per-cpu cpu_core_mask
Hi Michal, On 5/21/2024 3:47 PM, Michal Orzel wrote: Hi Henry. On 3/21/2024 11:57 AM, Henry Wang wrote: In the common sysctl command XEN_SYSCTL_physinfo, the value of cores_per_socket is calculated based on the cpu_core_mask of CPU0. Currently on Arm this is a fixed value 1 (can be checked via xl info), which is not correct. This is because during the Arm CPU online process at boot time, setup_cpu_sibling_map() only sets the per-cpu cpu_core_mask for itself. cores_per_socket refers to the number of cores that belong to the same socket (NUMA node). Currently Xen on Arm does not support physical CPU hotplug and NUMA, also we assume there is no multithread. Therefore cores_per_socket means all possible CPUs detected from the device tree. Setting the per-cpu cpu_core_mask in setup_cpu_sibling_map() accordingly. Modify the in-code comment which seems to be outdated. Add a warning to users if Xen is running on processors with multithread support. Signed-off-by: Henry Wang Signed-off-by: Henry Wang Reviewed-by: Michal Orzel Thanks. /* ID of the PCPU we're running on */ DEFINE_PER_CPU(unsigned int, cpu_id); -/* XXX these seem awfully x86ish... */ +/* + * Although multithread is part of the Arm spec, there are not many + * processors support multithread and current Xen on Arm assumes there NIT: s/support/supporting Sorry, it should have been spotted locally before sending. Anyway, I will correct this in v4 with your Reviewed-by tag taken. Thanks for pointing this out. Kind regards, Henry __init smp_get_max_cpus(void) ~Michal
Re: [PATCH v3] xen/arm: Set correct per-cpu cpu_core_mask
Hi Henry.
On 20/05/2024 04:57, Henry Wang wrote:
> Hi All,
>
> Gentle ping since it has been a couple of months, any comments on this
> updated patch? Thanks!
Sorry for the late reply.
>
> Kind regards,
> Henry
>
> On 3/21/2024 11:57 AM, Henry Wang wrote:
>> In the common sysctl command XEN_SYSCTL_physinfo, the value of
>> cores_per_socket is calculated based on the cpu_core_mask of CPU0.
>> Currently on Arm this is a fixed value 1 (can be checked via xl info),
>> which is not correct. This is because during the Arm CPU online
>> process at boot time, setup_cpu_sibling_map() only sets the per-cpu
>> cpu_core_mask for itself.
>>
>> cores_per_socket refers to the number of cores that belong to the same
>> socket (NUMA node). Currently Xen on Arm does not support physical
>> CPU hotplug and NUMA, also we assume there is no multithread. Therefore
>> cores_per_socket means all possible CPUs detected from the device
>> tree. Setting the per-cpu cpu_core_mask in setup_cpu_sibling_map()
>> accordingly. Modify the in-code comment which seems to be outdated. Add
>> a warning to users if Xen is running on processors with multithread
>> support.
>>
>> Signed-off-by: Henry Wang
>> Signed-off-by: Henry Wang
Reviewed-by: Michal Orzel
>> ---
>> v3:
>> - Use cpumask_copy() to set cpu_core_mask and drop the unnecessary
>>cpumask_set_cpu(cpu, per_cpu(cpu_core_mask, cpu)).
>> - In-code comment adjustments.
>> - Add a warning for multithread.
>> v2:
>> - Do not do the multithread check.
>> ---
>> xen/arch/arm/smpboot.c | 18 +++---
>> 1 file changed, 15 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
>> index a84e706d77..b6268be27a 100644
>> --- a/xen/arch/arm/smpboot.c
>> +++ b/xen/arch/arm/smpboot.c
>> @@ -66,7 +66,11 @@ static bool cpu_is_dead;
>>
>> /* ID of the PCPU we're running on */
>> DEFINE_PER_CPU(unsigned int, cpu_id);
>> -/* XXX these seem awfully x86ish... */
>> +/*
>> + * Although multithread is part of the Arm spec, there are not many
>> + * processors support multithread and current Xen on Arm assumes there
NIT: s/support/supporting
>> + * is no multithread.
>> + */
>> /* representing HT siblings of each logical CPU */
>> DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, cpu_sibling_mask);
>> /* representing HT and core siblings of each logical CPU */
>> @@ -85,9 +89,13 @@ static int setup_cpu_sibling_map(int cpu)
>>!zalloc_cpumask_var(_cpu(cpu_core_mask, cpu)) )
>> return -ENOMEM;
>>
>> -/* A CPU is a sibling with itself and is always on its own core. */
>> +/*
>> + * Currently we assume there is no multithread and NUMA, so
>> + * a CPU is a sibling with itself, and the all possible CPUs
>> + * are supposed to belong to the same socket (NUMA node).
>> + */
>> cpumask_set_cpu(cpu, per_cpu(cpu_sibling_mask, cpu));
>> -cpumask_set_cpu(cpu, per_cpu(cpu_core_mask, cpu));
>> +cpumask_copy(per_cpu(cpu_core_mask, cpu), _possible_map);
>>
>> return 0;
>> }
>> @@ -277,6 +285,10 @@ void __init smp_init_cpus(void)
>> warning_add("WARNING: HMP COMPUTING HAS BEEN ENABLED.\n"
>> "It has implications on the security and stability of
>> the system,\n"
>> "unless the cpu affinity of all domains is
>> specified.\n");
>> +
>> +if ( system_cpuinfo.mpidr.mt == 1 )
>> +warning_add("WARNING: MULTITHREADING HAS BEEN DETECTED ON THE
>> PROCESSOR.\n"
>> +"It might impact the security of the system.\n");
>> }
>>
>> unsigned int __init smp_get_max_cpus(void)
>
~Michal
Re: [PATCH v2 06/12] VT-d: respect ACPI SATC's ATC_REQUIRED flag
On 20.05.2024 13:36, Roger Pau Monné wrote: > On Wed, May 15, 2024 at 12:42:40PM +0200, Jan Beulich wrote: >> On 06.05.2024 15:38, Roger Pau Monné wrote: >>> On Thu, Feb 15, 2024 at 11:16:11AM +0100, Jan Beulich wrote: When the flag is set, permit Dom0 to control the device (no worse than what we had before and in line with other "best effort" behavior we use when it comes to Dom0), >>> >>> I think we should somehow be able to signal dom0 that this device >>> might not operate as expected, otherwise dom0 might use it and the >>> device could silently malfunction due to ATS not being enabled. >> >> Whatever signaling we invented, no Dom0 would be required to respect it, >> and for (perhaps quite) some time no Dom0 kernel would even exist to query >> that property. >> >>> Otherwise we should just hide the device from dom0. >> >> This would feel wrong to me, almost like a regression from what we had >> before. > > Exposing a device to dom0 that won't be functional doesn't seem like a > very wise choice from Xen TBH. Yes but. That's what we're doing right now, after all. >>> I assume setting the IOMMU context entry to passthrough mode would >>> also be fine for such devices that require ATS? >> >> I'm afraid I'm lacking the connection of the question to what is being >> done here. Can you perhaps provide some more context? To provide some >> context from my side: Using pass-through mode would be excluded when Dom0 >> is PVH. Hence why I'm not getting why we would want to even just consider >> doing so. >> >> Yet, looking at the spec, in pass-through mode translation requests are >> treated as UR. So maybe your question was towards there needing to be >> handling (whichever way) for the case where pass-through mode was >> requested for PV Dom0? The only half-way sensible thing to do in that case >> that I can think of right now would be to ignore that command line option, > > Hm, maybe I'm confused, but if the IOMMU device context entry is set > in pass-through mode ATS won't be enabled and hence no translation > requests would be send from the device? > > IOW, devices listed in the SATC can only mandate ATS enabled when the > IOMMU is enforcing translation. IF the IOMMU is not enabled or if > the device is in passthrough mode then the requirement for having ATS > enabled no longer applies. Oh, I think I now get what your original question was about: Instead of enabling ATS on such devices, we might run them in pass-through mode. For PV that would appear to be an option, yes. But with PVH (presumably) being the future I'd be rather hesitant to go that route. Jan
Re: [PATCH v2 05/12] IOMMU: rename and re-type ats_enabled
On 20.05.2024 12:29, Roger Pau Monné wrote: > On Wed, May 15, 2024 at 12:07:50PM +0200, Jan Beulich wrote: >> On 06.05.2024 15:53, Roger Pau Monné wrote: >>> On Mon, May 06, 2024 at 03:20:38PM +0200, Jan Beulich wrote: On 06.05.2024 14:42, Roger Pau Monné wrote: > On Thu, Feb 15, 2024 at 11:15:39AM +0100, Jan Beulich wrote: >> @@ -196,7 +196,7 @@ static int __must_check amd_iommu_setup_ >> dte->sys_mgt = MASK_EXTR(ivrs_dev->device_flags, >> ACPI_IVHD_SYSTEM_MGMT); >> >> if ( use_ats(pdev, iommu, ivrs_dev) ) >> -dte->i = ats_enabled; >> +dte->i = true; > > Might be easier to just use: > > dte->i = use_ats(pdev, iommu, ivrs_dev); I'm hesitant here, as in principle we might be overwriting a "true" by "false" then. >>> >>> Hm, but that would be fine, what's the point in enabling the IOMMU to >>> reply to ATS requests if ATS is not enabled on the device? >>> >>> IOW: overwriting a "true" with a "false" seem like the correct >>> behavior if it's based on the output of use_ats(). >> >> I don't think so, unless there were flow guarantees excluding the possibility >> of taking this path twice without intermediately disabling the device again. >> Down from here the enabling of ATS is gated on use_ats(). Hence if, in an >> earlier invocation, we enabled ATS (and set dte->i), we wouldn't turn off ATS >> below (there's only code to turn it on), yet with what you suggest we'd clear >> dte->i. > > Please bear with me, I think I'm confused, why would use_ats(), and if > that's the case, don't we want to update dte->i so that it matches the > ATS state? I'm afraid I can't parse this. Maybe a result of incomplete editing? The topic is complex enough that I don't want to even try to guess what you may have meant to ask ... > Otherwise we would fail to disable IOMMU device address translation > support if ATS was disabled? I think the answer here is "no", but with the above I'm not really sure here, either. Jan
Re: [PATCH] Revert "evtchn: refuse EVTCHNOP_status for Xen-bound event channels"
On 17.05.2024 22:28, Stefano Stabellini wrote: > On Fri, 17 May 2024, Jan Beulich wrote: >> On 17.05.2024 03:21, Stefano Stabellini wrote: >>> On Thu, 16 May 2024, Jan Beulich wrote: 1) In the discussion George claimed that exposing status information in an uncontrolled manner is okay. I'm afraid I have to disagree, seeing how a similar assumption by CPU designers has led to a flood of vulnerabilities over the last 6+ years. Information exposure imo is never okay, unless it can be _proven_ that absolutely nothing "useful" can be inferred from it. (I'm having difficulty seeing how such a proof might look like.) >>> >>> Many would agree that it is better not to expose status information in >>> an uncontrolled manner. Anyway, let's focus on the actionable. >>> >>> 2) Me pointing out that the XSM hook might similarly get in the way of debugging, Andrew suggested that this is not an issue because any sensible XSM policy used in such an environment would grant sufficient privilege to Dom0. Yet that then still doesn't cover why DomU-s also can obtain status for Xen-internal event channels. The debugging argument then becomes weak, as in that case the XSM hook is possibly going to get in the way. 3) In the discussion Andrew further gave the impression that evtchn_send() had no XSM check. Yet it has; the difference to evtchn_status() is that the latter uses XSM_TARGET while the former uses XSM_HOOK. (Much like evtchn_status() may indeed be useful for debugging, evtchn_send() may be similarly useful to allow getting a stuck channel unstuck.) In summary I continue to think that an outright revert was inappropriate. DomU-s should continue to be denied status information on Xen-internal event channels, unconditionally and independent of whether dummy, silo, or Flask is in use. >>> >>> I think DomU-s should continue to be denied status information on >>> Xen-internal event channels *based on the default dummy, silo, or Flask >>> policy*. It is not up to us to decide the security policy, only to >>> enforce it and provide sensible defaults. >>> >>> In any case, the XSM_TARGET check in evtchn_status seems to do what we >>> want? >> >> No. XSM_TARGET permits the "owning" (not really, but it's its table) domain >> access. See xsm_default_action() in xsm/dummy.h. > > Sorry I still don't understand. Why is that a problem? It looks like the > wanted default behavior? For ordinary event channels - yes. But not for Xen-internal ones, at least from my pov. Jan
Re: [PATCH] tools: Add install/uninstall targets to tests/x86_emulator
On 16.05.2024 16:46, Alejandro Vallejo wrote: > Hi, > > On 16/05/2024 13:37, Jan Beulich wrote: >> On 16.05.2024 14:29, Alejandro Vallejo wrote: >>> On 16/05/2024 12:35, Roger Pau Monné wrote: On Thu, May 16, 2024 at 12:07:10PM +0100, Alejandro Vallejo wrote: > Bring test_x86_emulator in line with other tests by adding > install/uninstall rules. > > Signed-off-by: Alejandro Vallejo > --- > tools/tests/x86_emulator/Makefile | 11 +-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/tools/tests/x86_emulator/Makefile > b/tools/tests/x86_emulator/Makefile > index 834b2112e7fe..30edf7e0185d 100644 > --- a/tools/tests/x86_emulator/Makefile > +++ b/tools/tests/x86_emulator/Makefile > @@ -269,8 +269,15 @@ clean: > .PHONY: distclean > distclean: clean > > -.PHONY: install uninstall > -install uninstall: > +.PHONY: install > +install: all > + $(INSTALL_DIR) $(DESTDIR)$(LIBEXEC_BIN) > + $(if $(TARGET-y),$(INSTALL_PROG) $(TARGET-y) $(DESTDIR)$(LIBEXEC_BIN)) > + > +.PHONY: uninstall > +uninstall: > + $(RM) -- $(addprefix $(DESTDIR)$(LIBEXEC_BIN)/,$(TARGET-y)) > + FWIW, should you check that HOSTCC == CC before installing? Otherwise I'm unsure of the result in cross-compiled builds, as the x86_emulator is built with HOSTCC, not CC. Thanks, Roger. >>> >>> Right... >>> >>> More generally, should we do s/CC/HOSTCC/ on all compiler checks? I see >>> no particular reason to do them on $(CC) rather than the actual compiler >>> used during build. >> >> No. There really is a mix here, intentionally. Anything built through >> testcase.mk >> is using CC, and hence respective checking needs to use CC, too. That said, I >> don't think the split is done quite correctly just yet, which may raise the >> question of whether having the split is actually worth it. > > I'm a bit puzzled by this. Why do we compile pieces of the test binary > with different toolchains? > > At a glance it seems inconsequential in the native case and > fully broken on the cross-compiled case (which I guess is what Roger was > hinting at and I failed to notice). > > Why the distinction? What am I missing? It's convoluted and not fully consistent, yes. Consider for a moment that the emulator truly was what its name says, i.e. not merely re-playing insns. In such a case it could be run on non-x86, while still emulating x86 code. Thus code being emulated and code doing the emulation would necessarily need to be built with different compilers. It being (in most cases) merely replaying, the boundary has been fuzzy for a very long time: While for most parts it's clear what group they fall into, test_x86_emulator.c itself is (has become? even 3.2.3 already has at least one instance) a hybrid. Yet in principle this file should also be buildable with the (x86 or non-x86) host compiler. Jan
Re: [XEN PATCH v3 2/6] x86/intel: move vmce_has_lmce() routine to header
On 20.05.2024 11:32, Sergiy Kibrik wrote:
> 16.05.24 12:39, Jan Beulich:
>> On 14.05.2024 10:20, Sergiy Kibrik wrote:
>>> Moving this function out of mce_intel.c would make it possible to disable
>>> build of Intel MCE code later on, because the function gets called from
>>> common x86 code.
>>
>> Why "would"? "Will" or "is going to" would seem more to the point to me.
>
> yes, sure
>
>> But anyway.
>>
>>> --- a/xen/arch/x86/cpu/mcheck/mce.h
>>> +++ b/xen/arch/x86/cpu/mcheck/mce.h
>>> @@ -170,6 +170,11 @@ static inline int mce_bank_msr(const struct vcpu *v,
>>> uint32_t msr)
>>> return 0;
>>> }
>>>
>>> +static inline bool vmce_has_lmce(const struct vcpu *v)
>>> +{
>>> +return v->arch.vmce.mcg_cap & MCG_LMCE_P;
>>> +}
>>
>> Is there a particular reason this is placed here, rather than ...
>>
>>> --- a/xen/arch/x86/include/asm/mce.h
>>> +++ b/xen/arch/x86/include/asm/mce.h
>>> @@ -41,7 +41,6 @@ extern void vmce_init_vcpu(struct vcpu *v);
>>> extern int vmce_restore_vcpu(struct vcpu *v, const struct hvm_vmce_vcpu
>>> *ctxt);
>>> extern int vmce_wrmsr(uint32_t msr, uint64_t val);
>>> extern int vmce_rdmsr(uint32_t msr, uint64_t *val);
>>> -extern bool vmce_has_lmce(const struct vcpu *v);
>>> extern int vmce_enable_mca_cap(struct domain *d, uint64_t cap);
>>
>> ... in the file the declaration was in, thus avoiding ...
>>
>>> --- a/xen/arch/x86/msr.c
>>> +++ b/xen/arch/x86/msr.c
>>> @@ -24,6 +24,8 @@
>>>
>>> #include
>>>
>>> +#include "cpu/mcheck/mce.h"
>>
>> ... the need for such a non-standard, cross-directory #include?
>>
>
>
> This is because MCG_LMCE_P is defined in arch/x86/cpu/mcheck/x86_mca.h
> -- so either MCG_LMCE_P (+ a bunch of MCG_* declarations) has to be
> moved to common header to be accessible, or local x86_mca.h got to be
> included from common arch/x86/include/asm/mce.h.
>
> As for the MCG_* declarations movement I didn't think there's a good
> enough reason to do it; as for the inclusion of x86_mca.h it didn't look
> nice at all.
I'm afraid I don't follow the latter: Why's including x86_mca.h any worse
than what you do right now?
Jan
