diff mbox series

[V1,03/16] xen/ioreq: Make x86's hvm_ioreq_needs_completion() common

Message ID 1599769330-17656-4-git-send-email-olekstysh@gmail.com (mailing list archive)
State Superseded
Headers show
Series IOREQ feature (+ virtio-mmio) on Arm | expand

Commit Message

Oleksandr Tyshchenko Sept. 10, 2020, 8:21 p.m. UTC 
From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

The IOREQ is a common feature now and this helper will be used
on Arm as is. Move it to include/xen/ioreq.h

Although PIO handling on Arm is not introduced with the current series
(it will be implemented when we add support for vPCI), technically
the PIOs exist on Arm (however they are accessed the same way as MMIO)
and it would be better not to diverge now.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

---
Please note, this is a split/cleanup/hardening of Julien's PoC:
"Add support for Guest IO forwarding to a device emulator"

Changes RFC -> V1:
   - new patch, was split from:
     "[RFC PATCH V1 01/12] hvm/ioreq: Make x86's IOREQ feature common"
---
---
 xen/arch/x86/hvm/vmx/realmode.c | 1 +
 xen/include/asm-x86/hvm/vcpu.h  | 7 -------
 xen/include/xen/ioreq.h         | 7 +++++++
 3 files changed, 8 insertions(+), 7 deletions(-)

Comments

Jan Beulich Sept. 14, 2020, 2:59 p.m. UTC | #1 
On 10.09.2020 22:21, Oleksandr Tyshchenko wrote:
> --- a/xen/include/xen/ioreq.h
> +++ b/xen/include/xen/ioreq.h
> @@ -35,6 +35,13 @@ static inline struct hvm_ioreq_server *get_ioreq_server(const struct domain *d,
>      return GET_IOREQ_SERVER(d, id);
>  }
>  
> +static inline bool hvm_ioreq_needs_completion(const ioreq_t *ioreq)
> +{
> +    return ioreq->state == STATE_IOREQ_READY &&
> +           !ioreq->data_is_ptr &&
> +           (ioreq->type != IOREQ_TYPE_PIO || ioreq->dir != IOREQ_WRITE);
> +}

While the PIO aspect has been discussed to some length, what about
the data_is_ptr concept? I didn't think there were Arm insns fitting
this? Instead I thought some other Arm-specific adjustments to the
protocol might be needed. At which point the question of course would
be in how far ioreq_t as a whole really fits Arm in its current shape.

Jan
Oleksandr Tyshchenko Sept. 22, 2020, 4:16 p.m. UTC | #2 
On 14.09.20 17:59, Jan Beulich wrote:

Hi Jan

> On 10.09.2020 22:21, Oleksandr Tyshchenko wrote:
>> --- a/xen/include/xen/ioreq.h
>> +++ b/xen/include/xen/ioreq.h
>> @@ -35,6 +35,13 @@ static inline struct hvm_ioreq_server *get_ioreq_server(const struct domain *d,
>>       return GET_IOREQ_SERVER(d, id);
>>   }
>>   
>> +static inline bool hvm_ioreq_needs_completion(const ioreq_t *ioreq)
>> +{
>> +    return ioreq->state == STATE_IOREQ_READY &&
>> +           !ioreq->data_is_ptr &&
>> +           (ioreq->type != IOREQ_TYPE_PIO || ioreq->dir != IOREQ_WRITE);
>> +}
> While the PIO aspect has been discussed to some length, what about
> the data_is_ptr concept? I didn't think there were Arm insns fitting
> this? Instead I thought some other Arm-specific adjustments to the
> protocol might be needed. At which point the question of course would
> be in how far ioreq_t as a whole really fits Arm in its current shape.
I may mistake here but I don't think the "data_is_ptr" is supported.
It worth mentioning that on Arm, all the accesses to MMIO region will do 
a single memory access.
So we set "df" to 0 and "count" to 1. Other ioreq_t fields are in use.
Julien Grall Sept. 23, 2020, 5:27 p.m. UTC | #3 
Hi,

On 14/09/2020 15:59, Jan Beulich wrote:
> On 10.09.2020 22:21, Oleksandr Tyshchenko wrote:
>> --- a/xen/include/xen/ioreq.h
>> +++ b/xen/include/xen/ioreq.h
>> @@ -35,6 +35,13 @@ static inline struct hvm_ioreq_server *get_ioreq_server(const struct domain *d,
>>       return GET_IOREQ_SERVER(d, id);
>>   }
>>   
>> +static inline bool hvm_ioreq_needs_completion(const ioreq_t *ioreq)
>> +{
>> +    return ioreq->state == STATE_IOREQ_READY &&
>> +           !ioreq->data_is_ptr &&
>> +           (ioreq->type != IOREQ_TYPE_PIO || ioreq->dir != IOREQ_WRITE);
>> +}
> 
> While the PIO aspect has been discussed to some length, what about
> the data_is_ptr concept? I didn't think there were Arm insns fitting
> this? Instead I thought some other Arm-specific adjustments to the
> protocol might be needed. At which point the question of course would
> be in how far ioreq_t as a whole really fits Arm in its current shape.

I would rather not try to re-invent ioreq_t for Arm if we don't need to. 
This is only going to increase the amount of arch specific code in a 
device emulator that really ought to be agnostic.

At the moment, I think it is fine to have "unused" field on Arm as long 
as they contain the right value.

So I would rather keep the check in common code as well.

Cheers,
diff mbox series

Patch

diff --git a/xen/arch/x86/hvm/vmx/realmode.c b/xen/arch/x86/hvm/vmx/realmode.c
index bdbd9cb..292a7c3 100644
--- a/xen/arch/x86/hvm/vmx/realmode.c
+++ b/xen/arch/x86/hvm/vmx/realmode.c
@@ -10,6 +10,7 @@ 
  */
 
 #include <xen/init.h>
+#include <xen/ioreq.h>
 #include <xen/lib.h>
 #include <xen/sched.h>
 #include <xen/paging.h>
diff --git a/xen/include/asm-x86/hvm/vcpu.h b/xen/include/asm-x86/hvm/vcpu.h
index 5ccd075..6c1feda 100644
--- a/xen/include/asm-x86/hvm/vcpu.h
+++ b/xen/include/asm-x86/hvm/vcpu.h
@@ -91,13 +91,6 @@  struct hvm_vcpu_io {
     const struct g2m_ioport *g2m_ioport;
 };
 
-static inline bool hvm_ioreq_needs_completion(const ioreq_t *ioreq)
-{
-    return ioreq->state == STATE_IOREQ_READY &&
-           !ioreq->data_is_ptr &&
-           (ioreq->type != IOREQ_TYPE_PIO || ioreq->dir != IOREQ_WRITE);
-}
-
 struct nestedvcpu {
     bool_t nv_guestmode; /* vcpu in guestmode? */
     void *nv_vvmcx; /* l1 guest virtual VMCB/VMCS */
diff --git a/xen/include/xen/ioreq.h b/xen/include/xen/ioreq.h
index f846034..2240a73 100644
--- a/xen/include/xen/ioreq.h
+++ b/xen/include/xen/ioreq.h
@@ -35,6 +35,13 @@  static inline struct hvm_ioreq_server *get_ioreq_server(const struct domain *d,
     return GET_IOREQ_SERVER(d, id);
 }
 
+static inline bool hvm_ioreq_needs_completion(const ioreq_t *ioreq)
+{
+    return ioreq->state == STATE_IOREQ_READY &&
+           !ioreq->data_is_ptr &&
+           (ioreq->type != IOREQ_TYPE_PIO || ioreq->dir != IOREQ_WRITE);
+}
+
 bool hvm_io_pending(struct vcpu *v);
 bool handle_hvm_io_completion(struct vcpu *v);
 bool is_ioreq_server_page(struct domain *d, const struct page_info *page);