From: Raphael Ning <raphning@amazon.com>
Currently, evtchn_fifo_set_pending() will mark the event as PENDING even
if it fails to lock the FIFO event queue(s), or if the guest has not
initialized the FIFO control block for the target vCPU. A well-behaved
guest should never trigger either of these cases.
There is no good reason to set the PENDING bit (the guest should not
depend on this behaviour anyway) or check for pollers in such corner
cases, so skip that. In fact, both the comment above the for loop and
the commit message for
41a822c39263 xen/events: rework fifo queue locking
suggest that the bit should be set after the FIFO queue(s) are locked.
Take the opportunity to rename the was_pending variable (flipping its
sense) and switch to the standard bool type.
Suggested-by: David Vrabel <dvrabel@amazon.co.uk>
Signed-off-by: Raphael Ning <raphning@amazon.com>
---
xen/common/event_fifo.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/xen/common/event_fifo.c b/xen/common/event_fifo.c
index ed4d3beb10f3..6c74ccebebb7 100644
--- a/xen/common/event_fifo.c
+++ b/xen/common/event_fifo.c
@@ -165,7 +165,7 @@ static void cf_check evtchn_fifo_set_pending(
unsigned int port;
event_word_t *word;
unsigned long flags;
- bool_t was_pending;
+ bool_t check_pollers = false;
struct evtchn_fifo_queue *q, *old_q;
unsigned int try;
bool linked = true;
@@ -226,8 +226,6 @@ static void cf_check evtchn_fifo_set_pending(
spin_unlock_irqrestore(&q->lock, flags);
}
- was_pending = guest_test_and_set_bit(d, EVTCHN_FIFO_PENDING, word);
-
/* If we didn't get the lock bail out. */
if ( try == 3 )
{
@@ -249,6 +247,8 @@ static void cf_check evtchn_fifo_set_pending(
goto unlock;
}
+ check_pollers = !guest_test_and_set_bit(d, EVTCHN_FIFO_PENDING, word);
+
/*
* Link the event if it unmasked and not already linked.
*/
@@ -314,7 +314,7 @@ static void cf_check evtchn_fifo_set_pending(
&v->evtchn_fifo->control_block->ready) )
vcpu_mark_events_pending(v);
- if ( !was_pending )
+ if ( check_pollers )
evtchn_check_pollers(d, port);
}
--
2.32.0On 16/03/2022 18:38, Raphael Ning wrote: > Currently, evtchn_fifo_set_pending() will mark the event as PENDING even > if it fails to lock the FIFO event queue(s), or if the guest has not > initialized the FIFO control block for the target vCPU. A well-behaved > guest should never trigger either of these cases. Reviewed-by: David Vrabel <dvrabel@amazon.co.uk> David
On 16.03.22 19:38, Raphael Ning wrote: > From: Raphael Ning <raphning@amazon.com> > > Currently, evtchn_fifo_set_pending() will mark the event as PENDING even > if it fails to lock the FIFO event queue(s), or if the guest has not > initialized the FIFO control block for the target vCPU. A well-behaved > guest should never trigger either of these cases. Is this true even for the resume case e.g. after a migration? The guests starts on the new host with no FIFO control block for any vcpu registered, so couldn't an event get lost with your patch in case it was sent before the target vcpu's control block gets registered? Juergen
On 17/03/2022 06:28, Juergen Gross wrote: > On 16.03.22 19:38, Raphael Ning wrote: >> From: Raphael Ning <raphning@amazon.com> >> >> Currently, evtchn_fifo_set_pending() will mark the event as PENDING even >> if it fails to lock the FIFO event queue(s), or if the guest has not >> initialized the FIFO control block for the target vCPU. A well-behaved >> guest should never trigger either of these cases. > > Is this true even for the resume case e.g. after a migration? > > The guests starts on the new host with no FIFO control block for any > vcpu registered, so couldn't an event get lost with your patch in case > it was sent before the target vcpu's control block gets registered? An event that is PENDING but not LINKED is not reachable by the guest so it won't ever see such an event, so the event is lost whether the P bit is set or not. Guests ensure that event channels are not bound to VCPUs that don't (yet) have FIFO control blocks. For example, in Linux xen_irq_resume() reinitializes the control blocks (in xen_evtchn_resume()) before restoring any of the event channels. David
On 16/03/2022 18:38, Raphael Ning wrote: > From: Raphael Ning <raphning@amazon.com> > > Currently, evtchn_fifo_set_pending() will mark the event as PENDING even > if it fails to lock the FIFO event queue(s), or if the guest has not > initialized the FIFO control block for the target vCPU. A well-behaved > guest should never trigger either of these cases. > > There is no good reason to set the PENDING bit (the guest should not > depend on this behaviour anyway) or check for pollers in such corner > cases, so skip that. In fact, both the comment above the for loop and > the commit message for > > 41a822c39263 xen/events: rework fifo queue locking > > suggest that the bit should be set after the FIFO queue(s) are locked. > > Take the opportunity to rename the was_pending variable (flipping its > sense) and switch to the standard bool type. > > Suggested-by: David Vrabel <dvrabel@amazon.co.uk> > Signed-off-by: Raphael Ning <raphning@amazon.com> > --- > xen/common/event_fifo.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/xen/common/event_fifo.c b/xen/common/event_fifo.c > index ed4d3beb10f3..6c74ccebebb7 100644 > --- a/xen/common/event_fifo.c > +++ b/xen/common/event_fifo.c > @@ -165,7 +165,7 @@ static void cf_check evtchn_fifo_set_pending( > unsigned int port; > event_word_t *word; > unsigned long flags; > - bool_t was_pending; > + bool_t check_pollers = false; Considering your commit message, did you intend to change this to bool? Can be fixed on commit. Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> ~Andrew P.S. David - do you want your maintainership back? None of this code has undergone any major changes since you wrote it.
Hi Andrew,
On 16/03/2022 18:58, Andrew Cooper wrote:
>> diff --git a/xen/common/event_fifo.c b/xen/common/event_fifo.c
>> index ed4d3beb10f3..6c74ccebebb7 100644
>> --- a/xen/common/event_fifo.c
>> +++ b/xen/common/event_fifo.c
>> @@ -165,7 +165,7 @@ static void cf_check evtchn_fifo_set_pending(
>> unsigned int port;
>> event_word_t *word;
>> unsigned long flags;
>> - bool_t was_pending;
>> + bool_t check_pollers = false;
>
> Considering your commit message, did you intend to change this to bool?
>
> Can be fixed on commit. Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
So far, tools like b4 [1] are not able to find your tag on lore.kernel.org:
42sh> b4 am
6b84a20b2753130cc62406a0fd14d2708f6f504b.1647455219.git.raphning@amazon.com
Looking up
https://lore.kernel.org/r/6b84a20b2753130cc62406a0fd14d2708f6f504b.1647455219.git.raphning%40amazon.com
Analyzing 8 messages in the thread
Checking attestation on all messages, may take a moment...
---
✓ [PATCH] evtchn/fifo: Don't set PENDING bit if guest misbehaves
+ Reviewed-by: David Vrabel <dvrabel@amazon.co.uk>
+ Tested-by: Luca Fancellu <luca.fancellu@arm.com> (✓
DKIM/armh.onmicrosoft.com)
---
✓ Signed: DKIM/gmail.com
---
Total patches: 1
---
Link:
https://lore.kernel.org/r/6b84a20b2753130cc62406a0fd14d2708f6f504b.1647455219.git.raphning@amazon.com
Base: not specified
git am
./20220316_raphning_evtchn_fifo_don_t_set_pending_bit_if_guest_misbehaves.mbx
This is because they are not at the start of the line. In the future,
would you mind write the tag on a separate line?
Cheers,
[1]
https://people.kernel.org/monsieuricon/introducing-b4-and-patch-attestation
--
Julien Grall
> On 16 Mar 2022, at 18:58, Andrew Cooper <amc96@srcf.net> wrote: > > On 16/03/2022 18:38, Raphael Ning wrote: >> From: Raphael Ning <raphning@amazon.com> >> >> Currently, evtchn_fifo_set_pending() will mark the event as PENDING even >> if it fails to lock the FIFO event queue(s), or if the guest has not >> initialized the FIFO control block for the target vCPU. A well-behaved >> guest should never trigger either of these cases. >> >> There is no good reason to set the PENDING bit (the guest should not >> depend on this behaviour anyway) or check for pollers in such corner >> cases, so skip that. In fact, both the comment above the for loop and >> the commit message for >> >> 41a822c39263 xen/events: rework fifo queue locking >> >> suggest that the bit should be set after the FIFO queue(s) are locked. >> >> Take the opportunity to rename the was_pending variable (flipping its >> sense) and switch to the standard bool type. >> >> Suggested-by: David Vrabel <dvrabel@amazon.co.uk> >> Signed-off-by: Raphael Ning <raphning@amazon.com> >> --- >> xen/common/event_fifo.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/xen/common/event_fifo.c b/xen/common/event_fifo.c >> index ed4d3beb10f3..6c74ccebebb7 100644 >> --- a/xen/common/event_fifo.c >> +++ b/xen/common/event_fifo.c >> @@ -165,7 +165,7 @@ static void cf_check evtchn_fifo_set_pending( >> unsigned int port; >> event_word_t *word; >> unsigned long flags; >> - bool_t was_pending; >> + bool_t check_pollers = false; > > Considering your commit message, did you intend to change this to bool? > > Can be fixed on commit. Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> > I’ve tested on the ARM side, I’ve started/destroyed few guests from Dom0, connect to the console, run some network communications between guest and Dom0, everything works: Tested-by: Luca Fancellu <luca.fancellu@arm.com> Cheers, Luca > ~Andrew > > P.S. David - do you want your maintainership back? None of this code > has undergone any major changes since you wrote it. >
On 17/03/2022 14:26, Luca Fancellu wrote: > I’ve tested on the ARM side, I’ve started/destroyed few guests from Dom0, connect to the console, run > some network communications between guest and Dom0, everything works: > > Tested-by: Luca Fancellu <luca.fancellu@arm.com> Thanks! I tested on x86 (in a QEMU VM) by launching and destroying an HVM guest; both dom0 and the guest use FIFO event channels. > > Cheers, > Luca >
On 16/03/2022 18:58, Andrew Cooper wrote: > On 16/03/2022 18:38, Raphael Ning wrote: >> From: Raphael Ning <raphning@amazon.com> >> >> Currently, evtchn_fifo_set_pending() will mark the event as PENDING even >> if it fails to lock the FIFO event queue(s), or if the guest has not >> initialized the FIFO control block for the target vCPU. A well-behaved >> guest should never trigger either of these cases. >> >> There is no good reason to set the PENDING bit (the guest should not >> depend on this behaviour anyway) or check for pollers in such corner >> cases, so skip that. In fact, both the comment above the for loop and >> the commit message for >> >> 41a822c39263 xen/events: rework fifo queue locking >> >> suggest that the bit should be set after the FIFO queue(s) are locked. >> >> Take the opportunity to rename the was_pending variable (flipping its >> sense) and switch to the standard bool type. >> >> Suggested-by: David Vrabel <dvrabel@amazon.co.uk> >> Signed-off-by: Raphael Ning <raphning@amazon.com> >> --- >> xen/common/event_fifo.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/xen/common/event_fifo.c b/xen/common/event_fifo.c >> index ed4d3beb10f3..6c74ccebebb7 100644 >> --- a/xen/common/event_fifo.c >> +++ b/xen/common/event_fifo.c >> @@ -165,7 +165,7 @@ static void cf_check evtchn_fifo_set_pending( >> unsigned int port; >> event_word_t *word; >> unsigned long flags; >> - bool_t was_pending; >> + bool_t check_pollers = false; > Considering your commit message, did you intend to change this to bool? > > Can be fixed on commit. Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> My mistake, I missed that when rebasing the patch. > > ~Andrew > > P.S. David - do you want your maintainership back? None of this code > has undergone any major changes since you wrote it.
© 2016 - 2024 Red Hat, Inc.