[V2,08/23] xen/ioreq: Introduce ioreq_params to abstract accesses to arch.hvm.params

Commit Message

Oleksandr Tyshchenko Oct. 15, 2020, 4:44 p.m. UTC

From: Oleksandr Tyshchenko <[e-mail]>

We don't want to move HVM params field out of *arch.hvm* in this particular
case as although it stores a few IOREQ params, it is not a (completely)
IOREQ stuff and is specific to the architecture. Instead, abstract
accesses by the proposed macro.

This is a follow up action to reduce layering violation in the common code.

Signed-off-by: Oleksandr Tyshchenko <[e-mail]>
CC: Julien Grall <[e-mail]>

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

Changes V1 -> V2:
   - new patch
---
 xen/common/ioreq.c               | 4 ++--
 xen/include/asm-x86/hvm/domain.h | 2 ++
 2 files changed, 4 insertions(+), 2 deletions(-)

Comments

Durrant, Paul Oct. 20, 2020, 10:41 a.m. UTC | #1

> -----Original Message-----
> From: Oleksandr Tyshchenko <[e-mail]>
> Sent: 15 October 2020 17:44
> To: [e-mail]
> Cc: Oleksandr Tyshchenko <[e-mail]>; Paul Durrant <[e-mail]>; Jan Beulich
> <[e-mail]>; Andrew Cooper <[e-mail]>; Roger Pau Monné
> <[e-mail]>; Wei Liu <[e-mail]>; Julien Grall <[e-mail]>; Stefano Stabellini
> <[e-mail]>; Julien Grall <[e-mail]>
> Subject: [PATCH V2 08/23] xen/ioreq: Introduce ioreq_params to abstract accesses to arch.hvm.params
> 
> From: Oleksandr Tyshchenko <[e-mail]>
> 
> We don't want to move HVM params field out of *arch.hvm* in this particular
> case as although it stores a few IOREQ params, it is not a (completely)
> IOREQ stuff and is specific to the architecture. Instead, abstract
> accesses by the proposed macro.
> 
> This is a follow up action to reduce layering violation in the common code.
> 
> Signed-off-by: Oleksandr Tyshchenko <[e-mail]>
> CC: Julien Grall <[e-mail]>
> 

Keeping the 'legacy' magic page code under an x86 ioreq.c would avoid the need for this patch.

  Paul

> ---
> Please note, this is a split/cleanup/hardening of Julien's PoC:
> "Add support for Guest IO forwarding to a device emulator"
> 
> Changes V1 -> V2:
>    - new patch
> ---
>  xen/common/ioreq.c               | 4 ++--
>  xen/include/asm-x86/hvm/domain.h | 2 ++
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/common/ioreq.c b/xen/common/ioreq.c
> index 7f91bc2..a07f1d7 100644
> --- a/xen/common/ioreq.c
> +++ b/xen/common/ioreq.c
> @@ -223,7 +223,7 @@ static gfn_t hvm_alloc_legacy_ioreq_gfn(struct 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_params(d, i));
>      }
> 
>      return INVALID_GFN;
> @@ -255,7 +255,7 @@ static bool hvm_free_legacy_ioreq_gfn(struct 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_params(d, i))) )
>               break;
>      }
>      if ( i > HVM_PARAM_BUFIOREQ_PFN )
> diff --git a/xen/include/asm-x86/hvm/domain.h b/xen/include/asm-x86/hvm/domain.h
> index 5d60737..c3af339 100644
> --- a/xen/include/asm-x86/hvm/domain.h
> +++ b/xen/include/asm-x86/hvm/domain.h
> @@ -63,6 +63,8 @@ struct hvm_pi_ops {
>      void (*vcpu_block)(struct vcpu *);
>  };
> 
> +#define ioreq_params(d, i) ((d)->arch.hvm.params[i])
> +
>  struct hvm_domain {
>      /* Cached CF8 for guest PCI config cycles */
>      uint32_t                pci_cf8;
> --
> 2.7.4

Oleksandr Tyshchenko Nov. 10, 2020, 8 p.m. UTC | #2

On 20.10.20 13:41, Paul Durrant wrote:

Hi Paul

Sorry for the late response.


>> -----Original Message-----
>> From: Oleksandr Tyshchenko <[e-mail]>
>> Sent: 15 October 2020 17:44
>> To: [e-mail]
>> Cc: Oleksandr Tyshchenko <[e-mail]>; Paul Durrant <[e-mail]>; Jan Beulich
>> <[e-mail]>; Andrew Cooper <[e-mail]>; Roger Pau Monné
>> <[e-mail]>; Wei Liu <[e-mail]>; Julien Grall <[e-mail]>; Stefano Stabellini
>> <[e-mail]>; Julien Grall <[e-mail]>
>> Subject: [PATCH V2 08/23] xen/ioreq: Introduce ioreq_params to abstract accesses to arch.hvm.params
>>
>> From: Oleksandr Tyshchenko <[e-mail]>
>>
>> We don't want to move HVM params field out of *arch.hvm* in this particular
>> case as although it stores a few IOREQ params, it is not a (completely)
>> IOREQ stuff and is specific to the architecture. Instead, abstract
>> accesses by the proposed macro.
>>
>> This is a follow up action to reduce layering violation in the common code.
>>
>> Signed-off-by: Oleksandr Tyshchenko <[e-mail]>
>> CC: Julien Grall <[e-mail]>
>>
> Keeping the 'legacy' magic page code under an x86 ioreq.c would avoid the need for this patch.

In that case, yes, agree.

Patch

diff --git a/xen/common/ioreq.c b/xen/common/ioreq.c
index 7f91bc2..a07f1d7 100644
--- a/xen/common/ioreq.c
+++ b/xen/common/ioreq.c
@@ -223,7 +223,7 @@  static gfn_t hvm_alloc_legacy_ioreq_gfn(struct 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_params(d, i));
     }
 
     return INVALID_GFN;
@@ -255,7 +255,7 @@  static bool hvm_free_legacy_ioreq_gfn(struct 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_params(d, i))) )
              break;
     }
     if ( i > HVM_PARAM_BUFIOREQ_PFN )
diff --git a/xen/include/asm-x86/hvm/domain.h b/xen/include/asm-x86/hvm/domain.h
index 5d60737..c3af339 100644
--- a/xen/include/asm-x86/hvm/domain.h
+++ b/xen/include/asm-x86/hvm/domain.h
@@ -63,6 +63,8 @@  struct hvm_pi_ops {
     void (*vcpu_block)(struct vcpu *);
 };
 
+#define ioreq_params(d, i) ((d)->arch.hvm.params[i])
+
 struct hvm_domain {
     /* Cached CF8 for guest PCI config cycles */
     uint32_t                pci_cf8;