Re: [Xen-devel] [PATCH v3 04/10] ioreq: add internal ioreq initialization support

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.09.2019 15:32, Roger Pau Monne wrote:
> @@ -855,6 +884,8 @@ int hvm_destroy_ioreq_server(struct domain *d, ioservid_t 
> id)
>      struct hvm_ioreq_server *s;
>      int rc;
>  
> +    ASSERT(!hvm_ioreq_is_internal(id));

With this, ...

> @@ -871,13 +903,13 @@ int hvm_destroy_ioreq_server(struct domain *d, 
> ioservid_t id)
>  
>      p2m_set_ioreq_server(d, 0, id);
>  
> -    hvm_ioreq_server_disable(s);
> +    hvm_ioreq_server_disable(s, hvm_ioreq_is_internal(id));

... why not simply "false" here?

>      /*
>       * It is safe to call hvm_ioreq_server_deinit() prior to
>       * set_ioreq_server() since the target domain is paused.
>       */
> -    hvm_ioreq_server_deinit(s);
> +    hvm_ioreq_server_deinit(s, false);

The more that here you do so.

> @@ -900,6 +932,8 @@ int hvm_get_ioreq_server_info(struct domain *d, 
> ioservid_t id,
>      struct hvm_ioreq_server *s;
>      int rc;
>  
> +    ASSERT(!hvm_ioreq_is_internal(id));
> +
>      spin_lock_recursive(&d->arch.hvm.ioreq_server.lock);
>  
>      s = get_ioreq_server(d, id);
> @@ -909,6 +943,7 @@ int hvm_get_ioreq_server_info(struct domain *d, 
> ioservid_t id,
>          goto out;
>  
>      rc = -EPERM;
> +    /* NB: don't allow fetching information from internal ioreq servers. */
>      if ( s->emulator != current->domain )
>          goto out;

The comment doesn't really seem to be applicable to the code here:
->emulator lives in the "external" part of the union, and hence if
anywhere I think the comment should go next to the ASSERT() above.

> @@ -1010,7 +1045,7 @@ int hvm_map_io_range_to_ioreq_server(struct domain *d, 
> ioservid_t id,
>          goto out;
>  
>      rc = -EPERM;
> -    if ( s->emulator != current->domain )
> +    if ( !hvm_ioreq_is_internal(id) && s->emulator != current->domain )
>          goto out;
>  
>      switch ( type )
> @@ -1062,7 +1097,7 @@ int hvm_unmap_io_range_from_ioreq_server(struct domain 
> *d, ioservid_t id,
>          goto out;
>  
>      rc = -EPERM;
> -    if ( s->emulator != current->domain )
> +    if ( !hvm_ioreq_is_internal(id) && s->emulator != current->domain )
>          goto out;
>  
>      switch ( type )
> @@ -1108,6 +1143,8 @@ int hvm_map_mem_type_to_ioreq_server(struct domain *d, 
> ioservid_t id,
>      struct hvm_ioreq_server *s;
>      int rc;
>  
> +    ASSERT(!hvm_ioreq_is_internal(id));
> +
>      if ( type != HVMMEM_ioreq_server )
>          return -EINVAL;

Taking just these three, things seem pretty inconsistent: Why ASSERT()
here but if() above? I think it would be better if dm.c was left
unchanged (not sure if I'm in opposition with this to prior review
comments by someone else), in particular making it unnecessary (as it
seems) to expose hvm_ioreq_is_internal() outside of this CU.

> @@ -1184,7 +1221,7 @@ int hvm_all_ioreq_servers_add_vcpu(struct domain *d, 
> struct vcpu *v)
>  
>      spin_lock_recursive(&d->arch.hvm.ioreq_server.lock);
>  
> -    FOR_EACH_IOREQ_SERVER(d, id, s)
> +    FOR_EACH_EXTERNAL_IOREQ_SERVER(d, id, s)

Still remembering the error path fix you likely spotted as necessary
while doing this work (commit 215f2576b0): Don't you need to again
adjust this same error path here (MAX_NR_IOREQ_SERVERS ->
MAX_NR_EXTERNAL_IOREQ_SERVERS)?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel