On 06.08.20 14:08, Julien Grall wrote:
Hi Julien
What is this function supposed to do?
Agree, sounds confusing a bit. I assume it is supposed to complete
Guest MMIO access after finishing emulation.
Shall I rename it to something appropriate (maybe by adding ioreq
prefix)?
How about ioreq_handle_complete_mmio()?
For me it sounds fine.
diff --git a/xen/common/memory.c b/xen/common/memory.c
index 9283e5e..0000477 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -8,6 +8,7 @@
*/
#include <xen/domain_page.h>
+#include <xen/hvm/ioreq.h>
#include <xen/types.h>
#include <xen/lib.h>
#include <xen/mm.h>
@@ -30,10 +31,6 @@
#include <public/memory.h>
#include <xsm/xsm.h>
-#ifdef CONFIG_IOREQ_SERVER
-#include <xen/hvm/ioreq.h>
-#endif
-
Why do you remove something your just introduced?
The reason I guarded that header is to make "xen/mm: Make x86's
XENMEM_resource_ioreq_server handling common" (previous) patch
buildable on Arm
without arch IOREQ header added yet. I tried to make sure that the
result after each patch was buildable to retain bisectability.
As current patch adds Arm IOREQ specific bits (including header),
that guard could be removed as not needed anymore.
I agree we want to have the build bisectable. However, I am still
puzzled why it is necessary to remove the #ifdef and move it earlier
in the list.
Do you mind to provide more details?
Previous patch "xen/mm: Make x86's XENMEM_resource_ioreq_server handling
common" breaks build on Arm as it includes xen/hvm/ioreq.h which
requires arch header
to be present (asm/hvm/ioreq.h). But the missing arch header together
with other arch specific bits are introduced here in current patch.
Probably I should have rearranged
changes in a way to not introduce #ifdef and then remove it...
[...]
+
+bool handle_mmio(void);
+
+static inline bool handle_pio(uint16_t port, unsigned int size,
int dir)
+{
+ /* XXX */
Can you expand this TODO? What do you expect to do?
I didn't expect this to be called on Arm. Sorry, I am not sure l have
an idea how to handle this properly. I would keep it unimplemented
until a real reason.
Will expand TODO.
Let see how the conversation on patch#1 goes about PIO vs MMIO.
ok
+ BUG();
+ return true;
+}
+
+static inline paddr_t hvm_mmio_first_byte(const ioreq_t *p)
+{
+ return p->addr;
+}
I understand that the x86 version is more complex as it check p->df.
However, aside reducing the complexity, I am not sure why we would
want to diverge it.
After all, IOREQ is now meant to be a common feature.
Well, no objections at all.
Could you please clarify how could 'df' (Direction Flag?) be
handled/used on Arm?
On x86, this is used by 'rep' instruction to tell the direction to
iterate (forward or backward).
On Arm, all the accesses to MMIO region will do a single memory
access. So for now, we can safely always set to 0.
I see that try_fwd_ioserv() always sets it 0. Or I need to just reuse
x86's helpers as is,
which (together with count = df = 0) will result in what we actually
have here?
AFAIU, both count and df should be 0 on Arm.
Thanks for the explanation. The only one question remains where to put
common helpers hvm_mmio_first_byte/hvm_mmio_last_byte (common io.h?)?
+
+static inline int p2m_set_ioreq_server(struct domain *d,
+ unsigned int flags,
+ struct hvm_ioreq_server *s)
+{
+ return -EOPNOTSUPP;
+}
This should be defined in p2m.h. But I am not even sure what it is
meant for. Can you expand it?
ok, will move.
In this series I tried to make as much IOREQ code common as possible
and avoid complicating things, in order to achieve that a few stubs
were added here. Please note,
that I also considered splitting into arch parts. But some functions
couldn't be split easily.
This one is called from common hvm_destroy_ioreq_server() with flag
being 0 (which will result in unmapping ioreq server from p2m type on
x86).
I could add a comment describing why this stub is present here.
Sorry if I wasn't clear. I wasn't asking why the stub is there but
what should be the expected implementation of the function.
In particular, you are returning -EOPNOTSUPP. The only reason you are
getting away from trouble is because the caller doesn't check the return.
True.
Would it make sense to have a stub arch_hvm_destroy_ioreq_server()?
With what has been said above, it make sense, will create.
+
+static inline void msix_write_completion(struct vcpu *v)
+{
+}
+
+static inline void handle_realmode_completion(void)
+{
+ ASSERT_UNREACHABLE();
+}
realmode is very x86 specific. So I don't think this function should
be called from common code. It might be worth considering to split
handle_hvm_io_completion() is 2 parts: common and arch specific.
I agree with you that realmode is x86 specific and looks not good in
Arm header.
It is not a problem of looking good or not. Instead, it is about
abstraction. A developper shouldn't need to understand all the other
architectures we support in order to follow the common code.
I was thinking how to split handle_hvm_io_completion() gracefully but
I failed find a good solution for that, so decided to add two stubs
(msix_write_completion and handle_realmode_completion) on Arm. I
could add a comment describing why they are here if appropriate. But
if you think they shouldn't be called from the common code in any
way, I will try to split it.
I am not entirely sure what msix_write_completion is meant to do on
x86. Is it dealing with virtual MSIx? Maybe Jan, Roger or Paul could
help?
Regarding handle_realmode_completion, I would add a new stub:
arch_ioreq_handle_io_completion() that is called from the default case
of the switch.
On x86 it would be implemented as:
switch (io_completion)
{
case HVMIO_realmode_completion:
...
default:
ASSERT_UNREACHABLE();
}
On Arm, it would be implemented as:
ASSERT_UNREACHABLE();
Good point, will update.
--
Regards,
Oleksandr Tyshchenko
