Re: [RFC PATCH V1 04/12] xen/arm: Introduce arch specific bits for IOREQ/DM features

[Oleksandr <olekstysh@xxxxxxxxx>]

On 05.08.20 12:32, Julien Grall wrote:

Hi Julien

> > > @@ -2275,6 +2282,16 @@ static void check_for_vcpu_work(void)
> > >    */
> > >   void leave_hypervisor_to_guest(void)
> > >   {
> > > +#ifdef CONFIG_IOREQ_SERVER
> > > +    /*
> > > +     * XXX: Check the return. Shall we call that in
> > > +     * continue_running and context_switch instead?
> > > +     * The benefits would be to avoid calling
> > > +     * handle_hvm_io_completion on every return.
> > > +     */
> > Yeah, that could be a simple and good optimization
> Well, it is not simple as it is sounds :). handle_hvm_io_completion() 
> is the function in charge to mark the vCPU as waiting for I/O. So we 
> would at least need to split the function.
> I wrote this TODO because I wasn't sure about the complexity of 
> handle_hvm_io_completion(current). Looking at it again, the main 
> complexity is the looping over the IOREQ servers.
> I think it would be better to optimize handle_hvm_io_completion() 
> rather than trying to hack the context_switch() or continue_running().
Well, is the idea in proposed dirty test patch below close to what you 
expect? Patch optimizes handle_hvm_io_completion() to avoid extra 
actions if vcpu's domain doesn't have ioreq_server, alternatively
the check could be moved out of handle_hvm_io_completion() to avoid 
calling that function at all. BTW, TODO also suggests checking the 
return value of handle_hvm_io_completion(), but I am completely sure we 
can simply
just return from leave_hypervisor_to_guest() at this point. Could you 
please share your opinion?

---
 xen/common/hvm/ioreq.c       | 12 +++++++++++-
 xen/include/asm-arm/domain.h |  1 +
 xen/include/xen/hvm/ioreq.h  |  5 +++++
 3 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/xen/common/hvm/ioreq.c b/xen/common/hvm/ioreq.c
index 7e1fa23..dc6647a 100644
--- a/xen/common/hvm/ioreq.c
+++ b/xen/common/hvm/ioreq.c
@@ -38,9 +38,15 @@ static void set_ioreq_server(struct domain *d, 
unsigned int id,
                              struct hvm_ioreq_server *s)
 {
     ASSERT(id < MAX_NR_IOREQ_SERVERS);
-    ASSERT(!s || !d->arch.hvm.ioreq_server.server[id]);
+    ASSERT((!s && d->arch.hvm.ioreq_server.server[id]) ||
+           (s && !d->arch.hvm.ioreq_server.server[id]));

     d->arch.hvm.ioreq_server.server[id] = s;
+
+    if ( s )
+        d->arch.hvm.ioreq_server.nr_servers ++;
+    else
+        d->arch.hvm.ioreq_server.nr_servers --;
 }

 /*
@@ -169,6 +175,9 @@ bool handle_hvm_io_completion(struct vcpu *v)
         return false;
     }

+    if ( !hvm_domain_has_ioreq_server(d) )
+        return true;
+
     FOR_EACH_IOREQ_SERVER(d, id, s)
     {
         struct hvm_ioreq_vcpu *sv;
@@ -1415,6 +1424,7 @@ unsigned int hvm_broadcast_ioreq(ioreq_t *p, bool 
buffered)
 void hvm_ioreq_init(struct domain *d)
 {
     spin_lock_init(&d->arch.hvm.ioreq_server.lock);
+    d->arch.hvm.ioreq_server.nr_servers = 0;

     arch_hvm_ioreq_init(d);
 }
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 6a01d69..484bd1a 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -68,6 +68,7 @@ struct hvm_domain
     struct {
         spinlock_t              lock;
         struct hvm_ioreq_server *server[MAX_NR_IOREQ_SERVERS];
+        unsigned int            nr_servers;
     } ioreq_server;

     bool_t qemu_mapcache_invalidate;
diff --git a/xen/include/xen/hvm/ioreq.h b/xen/include/xen/hvm/ioreq.h
index 40b7b5e..8f78852 100644
--- a/xen/include/xen/hvm/ioreq.h
+++ b/xen/include/xen/hvm/ioreq.h
@@ -23,6 +23,11 @@

 #include <asm/hvm/ioreq.h>

+static inline bool hvm_domain_has_ioreq_server(const struct domain *d)
+{
+    return (d->arch.hvm.ioreq_server.nr_servers > 0);
+}
+
 #define GET_IOREQ_SERVER(d, id) \
     (d)->arch.hvm.ioreq_server.server[id]

--
2.7.4





--
Regards,

Oleksandr Tyshchenko