Re: [Xen-devel] [PATCH v2 1/6] ioreq-server: centralize access to ioreq structures

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.03.14 at 12:40, Paul Durrant <paul.durrant@xxxxxxxxxx> wrote:
> @@ -232,20 +211,15 @@ static int hvmemul_do_io(
>              vio->io_state = HVMIO_handle_mmio_awaiting_completion;
>          break;
>      case X86EMUL_UNHANDLEABLE:
> -        /* If there is no backing DM, just ignore accesses */
> -        if ( !has_dm )
> +        rc = X86EMUL_RETRY;
> +        if ( !hvm_send_assist_req(curr, p) )
>          {
>              rc = X86EMUL_OKAY;
>              vio->io_state = HVMIO_none;
>          }
> -        else
> -        {
> -            rc = X86EMUL_RETRY;
> -            if ( !hvm_send_assist_req(curr) )
> -                vio->io_state = HVMIO_none;
> -            else if ( p_data == NULL )
> -                rc = X86EMUL_OKAY;
> -        }
> +        else if ( p_data == NULL )
> +            rc = X86EMUL_OKAY;
> +
>          break;

Is the rc value change here really intentional? Previously, when
!hvm_send_assist_req(), rc would end up being X86EMUL_RETRY,
while now it gets set to X86EMUL_OKAY. After all, there were
three different paths originally (setting rc to X86EMUL_OKAY
and/or setting vio->io_state to HVMIO_none, and you can't
express this with the bool_t return type of hvm_send_assist_req().

> +bool_t hvm_io_pending(struct vcpu *v)
> +{
> +    ioreq_t *p;
> +
> +    if ( !(p = get_ioreq(v)) )

I'd prefer if you used the call to get_ioreq() as initializer instead of
in a parenthesized assignment inside a conditional. But yes, it's a
matter of taste...

> @@ -1407,7 +1425,86 @@ void hvm_vcpu_down(struct vcpu *v)
>      }
>  }
>  
> -bool_t hvm_send_assist_req(struct vcpu *v)
> +int hvm_buffered_io_send(ioreq_t *p)

const ioreq_t *?

> +{
> +    struct vcpu *v = current;

"curr" please.

> +    struct hvm_ioreq_page *iorp = &v->domain->arch.hvm_domain.buf_ioreq;
> +    buffered_iopage_t *pg = iorp->va;
> +    buf_ioreq_t bp;
> +    /* Timeoffset sends 64b data, but no address. Use two consecutive slots. 
> */
> +    int qw = 0;
> +
> +    /* Ensure buffered_iopage fits in a page */
> +    BUILD_BUG_ON(sizeof(buffered_iopage_t) > PAGE_SIZE);
> +
> +    /*
> +     * Return 0 for the cases we can't deal with:
> +     *  - 'addr' is only a 20-bit field, so we cannot address beyond 1MB
> +     *  - we cannot buffer accesses to guest memory buffers, as the guest
> +     *    may expect the memory buffer to be synchronously accessed
> +     *  - the count field is usually used with data_is_ptr and since we don't
> +     *    support data_is_ptr we do not waste space for the count field 
> either
> +     */
> +    if ( (p->addr > 0xffffful) || p->data_is_ptr || (p->count != 1) )
> +        return 0;
> +
> +    bp.type = p->type;
> +    bp.dir  = p->dir;
> +    switch ( p->size )
> +    {
> +    case 1:
> +        bp.size = 0;
> +        break;
> +    case 2:
> +        bp.size = 1;
> +        break;
> +    case 4:
> +        bp.size = 2;
> +        break;
> +    case 8:
> +        bp.size = 3;
> +        qw = 1;
> +        break;
> +    default:
> +        gdprintk(XENLOG_WARNING, "unexpected ioreq size: %u\n", p->size);
> +        return 0;
> +    }
> +    
> +    bp.data = p->data;
> +    bp.addr = p->addr;
> +    
> +    spin_lock(&iorp->lock);
> +
> +    if ( (pg->write_pointer - pg->read_pointer) >=
> +         (IOREQ_BUFFER_SLOT_NUM - qw) )
> +    {
> +        /* The queue is full: send the iopacket through the normal path. */
> +        spin_unlock(&iorp->lock);
> +        return 0;
> +    }
> +    
> +    memcpy(&pg->buf_ioreq[pg->write_pointer % IOREQ_BUFFER_SLOT_NUM],
> +           &bp, sizeof(bp));

Better be type safe using an assignment here?

> +    

Line of only spaces.

> +    if ( qw )
> +    {
> +        bp.data = p->data >> 32;
> +        memcpy(&pg->buf_ioreq[(pg->write_pointer+1) % IOREQ_BUFFER_SLOT_NUM],
> +               &bp, sizeof(bp));
> +    }
> +
> +    /* Make the ioreq_t visible /before/ write_pointer. */
> +    wmb();
> +    pg->write_pointer += qw ? 2 : 1;
> +
> +    notify_via_xen_event_channel(v->domain,
> +            v->domain->arch.hvm_domain.params[HVM_PARAM_BUFIOREQ_EVTCHN]);
> +    spin_unlock(&iorp->lock);

Perhaps worth caching v->domain into a local variable?

> +bool_t hvm_send_assist_req(struct vcpu *v, ioreq_t *proto_p)

const ioreq_t *

Jan

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