Re: [Xen-devel] [PATCH v2 3/3] hvm_has_dm: Do a full check for backing DM

[Don Slutz <dslutz@xxxxxxxxxxx>]

On 02/03/15 09:58, Jan Beulich wrote:
>>>> On 02.02.15 at 16:22, <dslutz@xxxxxxxxxxx> wrote:
>>   Jan Beulich:
>>     The change on what to do when hvm_send_assist_req() fails is bad.
>>       That is correct.  Made hvm_has_dm() do full checking so
>>       that the extra VMEXIT and VMENTRY can be skipped.
>>     hvm_complete_assist_req() be a separate function yet having only
>>     a single caller ...
>>       Folded hvm_complete_assist_req() in to hvm_has_dm() which
>>       is the only place it is called.
> 
> This makes pretty little sense to me - previously the only caller was
> hvm_send_assist_req(), folding into which would make sense. But
> moving this code into hvm_has_dm() doesn't look right both from
> an abstract perspective (completely contrary to the function's name)
> and from what operations get carried out when:
> 

Ok, will be working on a much better commit message.  Do you want the
new commit message copied here (in the summary of the changes), or just
that fact that there is a new commit message?


>> --- a/xen/arch/x86/hvm/emulate.c
>> +++ b/xen/arch/x86/hvm/emulate.c
>> @@ -218,8 +218,11 @@ static int hvmemul_do_io(
>>              vio->io_state = HVMIO_handle_mmio_awaiting_completion;
>>          break;
>>      case X86EMUL_UNHANDLEABLE:
>> +    {
>> +        struct hvm_ioreq_server *s = hvm_has_dm(curr->domain, &p);
>> +
>>          /* If there is no backing DM, just ignore accesses */
>> -        if ( !hvm_has_dm(curr->domain) )
>> +        if ( !s )
>>          {
>>              rc = X86EMUL_OKAY;
>>              vio->io_state = HVMIO_none;
> 
> You alter the flow here, but leave the (now stale) comment
> untouched - !hvm_has_dm() no longer has the original meaning.
> 

I will say that the comment is not stale. Also this code flow change
is what I am trying to do in this patch.  Since I am talking about
Paul's code also, I might be off base.

It is true that hvm_has_dm() has changed.  However the change
is from "there is no backing DM" to "there is no backing DM
that will process this request".  To me they mean the same thing.

That is why I did not change the name and did not see a need
to change the comment.  However I can adjust the comment to be much longer.

How does:

/*
 * If there is no backing DM, or there is no backing DM to handle
 * this request, act just like a missing device.
 */

look as a new comment?

Would it help to s/hvm_has_dm/hvm_has_dm_for_req/?

Now I should have included that this patch has also changed for
PVH guests, the result of doing a inl (etc) is now to return 1's instead
of not changing eax.  This also includes the mmio cases just like
hvm_complete_assist_req() use to do.

Clearly I should have included this in the commit message.

Paul did say (in another thread):
"Actually one thing that's in your patch series does 'help' a bit. The
folding together of the has_dm check and the IO completion means that at
least hvmloader gets back f-s rather than 0-s for the emulation that
doesn't hit :-/ "


In my understanding, a proper PVH guest will not attempt to use
inl to QEMU devices.  The commit:

commit 68209bce0f551dcae991877ffd58211498bb0755
Author: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
Date:   Wed Nov 13 09:29:02 2013 +0100

    pvh: tolerate HVM guests having no ioreq page

    PVH guests don't have a backing device model emulator (qemu); just
    tolerate this situation explicitly, rather than special-casing PVH.
...

is where this comment comes from and leads me to thinking that having
inl be more like real hardware is not a problem.  However, if you want
to have no change to PVH, that code can be added.  It is a matter
of checking for list_empty() just like hvm_had_dm() use to do.


My understanding of what is going on here started with:

commit 434ae6517af2388572fa24c6c9904822cc75180b
Author: Paul Durrant <paul.durrant@xxxxxxxxxx>
Date:   Mon Jun 2 09:40:43 2014 +0200

    ioreq-server: add support for multiple servers
...


However instead of the statement:

    The previous single ioreq server that was created on demand now
    becomes the default server and an API is created to allow secondary
    servers, which handle specific IO ranges or PCI devices, to be
    added.

is not how QEMU was changed.  The change to QEMU:

commit 7665d6ba98e20fb05c420de947c1750fd47e5c07
Author: Paul Durrant <paul.durrant@xxxxxxxxxx>
Date:   Tue Jan 20 11:06:19 2015 +0000

    Xen: Use the ioreq-server API when available
...

causes QEMU to register a ioreq server that does not handle
all requests.  And no default ioreq server is created.

This lead me to (which did not get sent to xen-devel):

------------------------------------------------------------

References: <1417776605-36309-1-git-send-email-paul.durrant@xxxxxxxxxx>
        <1417776605-36309-3-git-send-email-paul.durrant@xxxxxxxxxx>
In-Reply-To: <1417776605-36309-3-git-send-email-paul.durrant@xxxxxxxxxx>
Subject: Re: [Qemu-devel] [PATCH v5 2/2] Xen: Use the ioreq-server API
when available
Date: Wed, 28 Jan 2015 14:32:04 -0500
From: Don Slutz <dslutz@xxxxxxxxxxx>
To: Paul Durrant <paul.durrant@xxxxxxxxxx>, qemu-devel@xxxxxxxxxx,
Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
CC: Peter Maydell <peter.maydell@xxxxxxxxxx>, Olaf Hering
<olaf@xxxxxxxxx>, Alexey Kardashevskiy <aik@xxxxxxxxx>, Stefan Weil
<sw@xxxxxxxxxxx>, Michael Tokarev <mjt@xxxxxxxxxx>, Alexander Graf
<agraf@xxxxxxx>, Gerd Hoffmann <kraxel@xxxxxxxxxx>, Stefan Hajnoczi
<stefanha@xxxxxxxxxx>, Paolo Bonzini <pbonzini@xxxxxxxxxx>

On 12/05/14 05:50, Paul Durrant wrote:
> The ioreq-server API added to Xen 4.5 offers better security than
> the existing Xen/QEMU interface because the shared pages that are
> used to pass emulation request/results back and forth are removed
> from the guest's memory space before any requests are serviced.
> This prevents the guest from mapping these pages (they are in a
> well known location) and attempting to attack QEMU by synthesizing
> its own request structures. Hence, this patch modifies configure
> to detect whether the API is available, and adds the necessary
> code to use the API if it is.

This patch (which is now on xenbits qemu staging) is causing me
issues.

So far I have tracked it back to hvm_select_ioreq_server()
which selects the "default_ioreq_server".  Since I have one 1
QEMU, it is both the "default_ioreq_server" and an enabled
2nd ioreq_server.  I am continuing to understand why my changes
are causing this.  More below.

...


Which did lead to:


Message-ID: <54CAEF19.1030205@xxxxxxxxxxxxx>
References: <1417776605-36309-1-git-send-email-paul.durrant@xxxxxxxxxx> 
 <1417776605-36309-3-git-send-email-paul.durrant@xxxxxxxxxx>
 <54C93934.2020707@xxxxxxxxxxxxx>
 <54C97947.5080908@xxxxxxxxxxxxx>
 <54C98586.3020509@xxxxxxxxxxxxx>
 <9AAE0902D5BC7E449B7C8E4E778ABCD0257DC103@xxxxxxxxxxxxxxxxxxxxxxxx>
 <54CA868A.6050407@xxxxxxxxxxxxx>
In-Reply-To: <54CA868A.6050407@xxxxxxxxxxxxx>
Subject: Re: [Qemu-devel] [PATCH v5 2/2] Xen: Use the ioreq-server API
when available
Date: Thu, 29 Jan 2015 21:40:25 -0500
From: Don Slutz <dslutz@xxxxxxxxxxx>
To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>, Don Slutz
<dslutz@xxxxxxxxxxx>, qemu-devel@xxxxxxxxxx <qemu-devel@xxxxxxxxxx>,
Stefano Stabellini <Stefano.Stabellini@xxxxxxxxxx>
CC: Peter Maydell <peter.maydell@xxxxxxxxxx>, Olaf Hering
<olaf@xxxxxxxxx>, Alexey Kardashevskiy <aik@xxxxxxxxx>, Stefan Weil
<sw@xxxxxxxxxxx>, Michael Tokarev <mjt@xxxxxxxxxx>, Alexander Graf
<agraf@xxxxxxx>, Gerd Hoffmann <kraxel@xxxxxxxxxx>, Stefan Hajnoczi
<stefanha@xxxxxxxxxx>, Paolo Bonzini <pbonzini@xxxxxxxxxx>,
xen-devel@xxxxxxxxxxxxx <xen-devel@xxxxxxxxxxxxx>
...
The one line patch:


From 5269b1fb947f207057ca69e320c79b397db3e8f5 Mon Sep 17 00:00:00 2001
From: Don Slutz <dslutz@xxxxxxxxxxx>
Date: Thu, 29 Jan 2015 21:24:05 -0500
Subject: [PATCH] Prevent hang if read of HVM_PARAM_IOREQ_PFN,
 HVM_PARAM_BUFIOREQ_PFN, HVM_PARAM_BUFIOREQ_EVTCHN is done
 before hvmloader starts.

Signed-off-by: Don Slutz <dslutz@xxxxxxxxxxx>
---
 xen/arch/x86/hvm/hvm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index bad410e..7ac4b45 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -993,7 +993,7 @@ static int hvm_create_ioreq_server(struct domain *d,
domid_t domid,
     spin_lock(&d->arch.hvm_domain.ioreq_server.lock);

     rc = -EEXIST;
-    if ( is_default && d->arch.hvm_domain.default_ioreq_server != NULL )
+    if ( is_default && !list_empty(&d->arch.hvm_domain.ioreq_server.list) )
         goto fail2;

     rc = hvm_ioreq_server_init(s, d, domid, is_default, handle_bufioreq,
-- 1.7.11.7

Does "fix this" but no idea if this is the way to go.

------------------------------------------------------------------

Which did end up on xen-devel -- so far no progress on that
bug (this patch set does not help here).


To sum this all up, there is no default_ioreq_server I.E.
d->arch.hvm_domain.default_ioreq_server is NULL in the normal case.

So hvm_select_ioreq_server() will either return a struct
hvm_ioreq_server* that will handle the request or NULL.

Based on this, currently hvm_send_assist_req() will either send the
request to QEMU or call on hvm_complete_assist_req().  Now currently
hvm_complete_assist_req() returns 1 and so hvm_send_assist_req()
will return 1 in the 2 cases:

1) The request was sent to QEMU

2) The request was completed by hvm_complete_assist_req

In either case, the code in hvmemul_do_io() will return X86EMUL_RETRY
and so you will get to:

  hvm_do_resume()
   -> hvm_wait_for_io() which checks the state of the ioreq.

For case 1, you will (most likely) call wait_on_xen_event_channel().
For case 2, you will just return.

Continuing with case 2, you get to VMENTRY which does a VMEXIT since
the rip has not changed.  This time around vio->io_state will be
HVMIO_completed, and so you will get to finish_access.

The whole point of this change is to avoid the extra VMENTRY and VMEXIT.

As you (correctly) pointed out:
"But looking at the description of the commit that
introduced this (bac0999325 "x86 hvm: Do not incorrectly retire an
instruction emulation...", almost immediately modified by f20f3c8ece
"x86 hvm: On failed hvm_send_assist_req(), io emulation...") I doubt
this is really what we want, "


Handling the 3 possible returns from hvm_send_assist_req() was the
"correct" way to go.


>> @@ -227,11 +230,12 @@ static int hvmemul_do_io(
>>          else
>>          {
>>              rc = X86EMUL_RETRY;
>> -            if ( !hvm_send_assist_req(&p) )
>> +            if ( !hvm_send_assist_req_to_ioreq_server(s, &p) )
>>                  vio->io_state = HVMIO_none;
>>              else if ( p_data == NULL )
>>                  rc = X86EMUL_OKAY;
>>          }
> 
> In particular, the returning of X86EMUL_RETRY vs X86EMUL_OKAY
> now change for the case where !s but also
> !list_empty(&d->arch.hvm_domain.ioreq_server.list). While this
> _may_ be intended, the description of the patch still doesn't make
> clear why this is so (again keeping in mind why the behavior prior to
> this patch was done in this particular way).
> 

This is the intended change.  Clearly I did not describe this.


> So in the end, considering that Paul approved of what you do, maybe
> all is well and it's just one step too many folded together for me to
> grok. In which case - please take the time to describe your changes
> suitably; this isn't the first time I have to ask you to do so.
> 

Sorry, I missed this this when adjusting to v2.  I have been having
the issue of "short", "cryptic", "un-readable", etc commit messages (or
the equivalent) for many years.  So please do not stop telling me to
fix them up.

Here is the proposed new commit message:

-----
Subject: [PATCH] hvmemul_do_io: Do not retry if no ioreq server exists for
 this I/O.

This saves a VMENTRY and a VMEXIT since we no longer retry the
ioport read on backing DM not handling a given ioreq.

There are 2 case about "no ioreq server exists for this I/O":

1) No ioreq servers (PVH case)
2) No ioreq servers for this I/O (non PVH case)

The routine hvm_has_dm() used to check for the empty list, the PVH
case (#1).

By changing hvm_has_dm() to check for both cases (the 1st thing
hvm_ioreq_server() does is check for an empty list), this code moves
the case of "no ioreq servers for this I/O" out of
hvm_complete_assist_req() and into hvm_has_dm().  Doing it this way
allows hvm_send_assist_req() to only have 2 possible return values.

The key part of skipping the retry is to do "rc = X86EMUL_OKAY"
which is what the error path on the call to hvm_has_dm() does in
hvmemul_do_io() (the only call on hvm_has_dm()).

Since this case is no longer handled in hvm_send_assist_req(), move
the call to hvm_complete_assist_req() into hvm_has_dm().

As part of this change, do the work of hvm_complete_assist_req() in
the PVH case.  Acting more like real hardware looks to be better.

Since there is only one caller of hvm_complete_assist_req(), fold
that routine into the changed hvm_has_dm().

Adding "rc = X86EMUL_OKAY" in the failing case of
hvm_send_assist_req() would unfix what was done in commit
bac0999325056a3b3a92f7622df7ffbc5388b1c3 and commit
f20f3c8ece5c10fa7626f253d28f570a43b23208.  We are currently doing
the succeeding case of hvm_send_assist_req() and retying the I/O.

To only call hvm_select_ioreq_server() once in this code path,
change hvm_has_dm() to return pointer to ioreq_server instead of a
bool.  This will now be passed on to hvm_send_assist_req(), which
was renamed from hvm_send_assist_req_to_ioreq_server().

Since hvm_send_assist_req() is an extern, add an ASSERT() on s.
-----


>> +    }
>>          break;
> 
> For indentation to be meaningful, the closing brace should follow
> the break statement.
> 

Ok, will fix.


>> --- a/xen/include/asm-x86/hvm/hvm.h
>> +++ b/xen/include/asm-x86/hvm/hvm.h
>> @@ -228,7 +228,9 @@ int hvm_vcpu_cacheattr_init(struct vcpu *v);
>>  void hvm_vcpu_cacheattr_destroy(struct vcpu *v);
>>  void hvm_vcpu_reset_state(struct vcpu *v, uint16_t cs, uint16_t ip);
>>  
>> -bool_t hvm_send_assist_req(ioreq_t *p);
>> +struct hvm_ioreq_server;
> 
> I think you could avoid this by simply moving up here the
> hvm_has_dm() declaration.
>

Will check that this works.


> 
>> +bool_t hvm_send_assist_req_to_ioreq_server(struct hvm_ioreq_server *s,
>> +                                           ioreq_t *p);
> 
> If, with the comments above in mind, you still intend to remove
> hvm_send_assist_req(), I'd much favor you renaming
> hvm_send_assist_req_to_ioreq_server() to hvm_send_assist_req().
> 

Happy to do this rename.

   -Don Slutz

> Jan
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel