Re: [Xen-devel] [PATCH v4 4/8] ioreq-server: on-demand creation of ioreq server

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 02.04.14 at 17:11, <paul.durrant@xxxxxxxxxx> wrote:
> @@ -645,13 +643,68 @@ static void hvm_ioreq_server_remove_vcpu(struct 
> hvm_ioreq_server *s,
>      spin_unlock(&s->lock);
>  }
>  
> -static int hvm_create_ioreq_server(struct domain *d, domid_t domid)
> +static void hvm_ioreq_server_remove_all_vcpus(struct hvm_ioreq_server *s)
>  {
> -    struct hvm_ioreq_server *s;
> +    struct list_head *entry, *next;
>  
> -    s = xzalloc(struct hvm_ioreq_server);
> -    if ( !s )
> -        return -ENOMEM;
> +    spin_lock(&s->lock);
> +
> +    list_for_each_safe ( entry, next, &s->ioreq_vcpu_list )
> +    {
> +        struct hvm_ioreq_vcpu *sv = container_of(entry, 
> +                                                 struct hvm_ioreq_vcpu, 
> +                                                 list_entry);

list_for_each_entry_safe() avoids the need for the explicit use of
container_of(), making the code easier to read.

> +static int hvm_create_ioreq_server(struct domain *d, domid_t domid)
> +{
> +    struct hvm_ioreq_server *s;
> +    int rc;
> +
> +    spin_lock(&d->arch.hvm_domain.ioreq_server_lock);
> +
> +    rc = -EEXIST;
> +    if ( d->arch.hvm_domain.ioreq_server != NULL )
> +        goto fail1;
> + 
> +    rc = -ENOMEM;
> +    s = xzalloc(struct hvm_ioreq_server);

Similar comment as on an earlier patch: Please try to avoid allocations
with lock held.

> +    if ( !s )
> +        goto fail2;
> +
> +    domain_pause(d);

And with that adjusted I would then again wonder whether taking
the lock after pausing the domain wouldn't be the better model.

> @@ -4570,6 +4724,18 @@ long do_hvm_op(unsigned long op, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>              case HVM_PARAM_ACPI_S_STATE:
>                  a.value = d->arch.hvm_domain.is_s3_suspended ? 3 : 0;
>                  break;
> +            case HVM_PARAM_IOREQ_PFN:
> +            case HVM_PARAM_BUFIOREQ_PFN:
> +            case HVM_PARAM_BUFIOREQ_EVTCHN: {
> +                domid_t domid;
> +                
> +                /* May need to create server */
> +                domid = d->arch.hvm_domain.params[HVM_PARAM_DM_DOMAIN];
> +                rc = hvm_create_ioreq_server(d, domid);

Pretty odd that you do this on reads, but not on writes. What's the
rationale behind this?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel