Re: [PATCH V1 02/16] xen/ioreq: Make x86's IOREQ feature common

[Oleksandr <olekstysh@gmail.com>]

On 22.09.20 13:54, Jan Beulich wrote:

Hi Jan

> On 22.09.2020 11:58, Oleksandr wrote:
>> On 22.09.20 09:33, Jan Beulich wrote:
>>> On 21.09.2020 21:02, Oleksandr wrote:
>>>> On 14.09.20 17:17, Jan Beulich wrote:
>>>>> On 10.09.2020 22:21, Oleksandr Tyshchenko wrote:
>>>>>> +#define GET_IOREQ_SERVER(d, id) \
>>>>>> +    (d)->arch.hvm.ioreq_server.server[id]
>>>>> arch.hvm.* feels like a layering violation when used in this header.
>>>> Got it. The only reason why GET_IOREQ_SERVER is here is inline
>>>> get_ioreq_server(). I will make it non-inline and move both to
>>>> common/ioreq.c.
>>> Which won't make the layering violation go away. It's still
>>> common rather than per-arch code then. As suggested elsewhere,
>>> I think the whole ioreq_server struct wants to move into
>>> struct domain itself, perhaps inside a new #ifdef (iirc one of
>>> the patches introduces a suitable Kconfig option).
>> Well, your advise regarding ioreq_server sounds reasonable, but the
>> common ioreq.c
>> still will have other *arch.hvm.* for both vcpu and domain. So looks
>> like other involved structs should be moved
>> into *common* struct domain/vcpu itself, correct? Some of them could be
>> moved easily since contain the same fields (arch.hvm.ioreq_gfn),
>> but some of them couldn't and seems to require to pull a lot of changes
>> to the Xen code (arch.hvm.params, arch.hvm.hvm_io), I am afraid.
>> Or I missed something?
> arch.hvm.params, iirc, is an x86 concept, and hence would need
> abstracting away anyway. I expect this will be common pattern:
> Either you want things to become generic (structure fields
> living directly in struct domain, or at least not under arch.hvm),
> or things need abstracting for per-arch handling.
Got it.

Let me please clarify one more question.
In order to avoid the layering violation in current patch we could apply 
a complex approach.

1. *arch.hvm.ioreq_gfn* and *arch.hvm.ioreq_server*: Both structs go 
into common struct domain.

2. *arch.hvm.params*: Two functions that use it 
(hvm_alloc_legacy_ioreq_gfn/hvm_free_legacy_ioreq_gfn) either go into 
arch code completely or
     specific macro is used in common code:

    #define ioreq_get_params(d, i) ((d)->arch.hvm.params[i])

    I would prefer macro than moving functions to arch code (which are 
equal and should remain in sync).

3. *arch.hvm.hvm_io*: We could also use the following:

    #define ioreq_get_io_completion(v) ((v)->arch.hvm.hvm_io.io_completion)
    #define ioreq_get_io_req(v) ((v)->arch.hvm.hvm_io.io_req)

    This way struct hvm_vcpu_io won't be used in common code as well.

    Are #2, 3 appropriate to go with?


Dirty non-tested patch (which applied on top of the whole series and 
targets Arm only) shows how it could look like.


diff --git a/xen/common/ioreq.c b/xen/common/ioreq.c
index 2e85ea7..5894bdab 100644
--- a/xen/common/ioreq.c
+++ b/xen/common/ioreq.c
@@ -194,7 +194,7 @@ static bool hvm_wait_for_io(struct hvm_ioreq_vcpu 
*sv, ioreq_t *p)
  bool handle_hvm_io_completion(struct vcpu *v)
  {
      struct domain *d = v->domain;
-    struct hvm_vcpu_io *vio = &v->arch.hvm.hvm_io;
+    ioreq_t io_req = ioreq_get_io_req(v);
      struct hvm_ioreq_server *s;
      struct hvm_ioreq_vcpu *sv;
      enum hvm_io_completion io_completion;
@@ -209,14 +209,14 @@ bool handle_hvm_io_completion(struct vcpu *v)
      if ( sv && !hvm_wait_for_io(sv, get_ioreq(s, v)) )
          return false;

-    vio->io_req.state = hvm_ioreq_needs_completion(&vio->io_req) ?
+    io_req.state = hvm_ioreq_needs_completion(&io_req) ?
          STATE_IORESP_READY : STATE_IOREQ_NONE;

      msix_write_completion(v);
      vcpu_end_shutdown_deferral(v);

-    io_completion = vio->io_completion;
-    vio->io_completion = HVMIO_no_completion;
+    io_completion = ioreq_get_io_completion(v);
+    ioreq_get_io_completion(v) = HVMIO_no_completion;

      switch ( io_completion )
      {
@@ -227,8 +227,8 @@ bool handle_hvm_io_completion(struct vcpu *v)
          return ioreq_handle_complete_mmio();

      case HVMIO_pio_completion:
-        return handle_pio(vio->io_req.addr, vio->io_req.size,
-                          vio->io_req.dir);
+        return handle_pio(io_req.addr, io_req.size,
+                          io_req.dir);

      default:
          return arch_handle_hvm_io_completion(io_completion);
@@ -247,7 +247,7 @@ static gfn_t hvm_alloc_legacy_ioreq_gfn(struct 
hvm_ioreq_server *s)
      for ( i = HVM_PARAM_IOREQ_PFN; i <= HVM_PARAM_BUFIOREQ_PFN; i++ )
      {
          if ( !test_and_clear_bit(i, &d->ioreq_gfn.legacy_mask) )
-            return _gfn(d->arch.hvm.params[i]);
+            return _gfn(ioreq_get_params(d, i));
      }

      return INVALID_GFN;
@@ -279,7 +279,7 @@ static bool hvm_free_legacy_ioreq_gfn(struct 
hvm_ioreq_server *s,

      for ( i = HVM_PARAM_IOREQ_PFN; i <= HVM_PARAM_BUFIOREQ_PFN; i++ )
      {
-        if ( gfn_eq(gfn, _gfn(d->arch.hvm.params[i])) )
+        if ( gfn_eq(gfn, _gfn(ioreq_get_params(d, i))) )
               break;
      }
      if ( i > HVM_PARAM_BUFIOREQ_PFN )
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 0e3ef20..ff761f5 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -21,6 +21,8 @@ struct hvm_domain
      uint64_t              params[HVM_NR_PARAMS];
  };

+#define ioreq_get_params(d, i) ((d)->arch.hvm.params[i])
+
  #ifdef CONFIG_ARM_64
  enum domain_type {
      DOMAIN_32BIT,
@@ -120,6 +122,9 @@ struct hvm_vcpu_io {
      unsigned long       mmio_gpfn;
  };

+#define ioreq_get_io_completion(v) ((v)->arch.hvm.hvm_io.io_completion)
+#define ioreq_get_io_req(v) ((v)->arch.hvm.hvm_io.io_req)
+
  struct arch_vcpu
  {
      struct {
(END)


>
>>> This goes
>>> alongside my suggestion to drop the "hvm" prefixes and infixes
>>> from involved function names.
>> Well, I assume this request as well as the request above should be
>> addressed in the follow-up patches, as we want to keep the code movement
>> in current patch as (almost) verbatim copy,
>> Am I correct?
> The renaming could imo be done before or after the move, but within
> a single series. Doing it (or some of it) during the move may be
> acceptable, but this primarily depends on the overall effect on the
> patch that this would have. I.e. the patch better wouldn't become
> gigantic just because all the renaming gets done in one go, and it's
> hundreds of places that need touching.

Got it as well.

Thank you for the explanation.

-- 
Regards,

Oleksandr Tyshchenko