Re: [Xen-devel] [PATCH v8 1/3] ioreq-server: add support for multiple servers

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.05.14 at 15:47, <paul.durrant@xxxxxxxxxx> wrote:
> +static int hvm_access_cf8(
> +    int dir, uint32_t port, uint32_t bytes, uint32_t *val)
> +{
> +    struct domain *d = current->domain;
> +
> +    if ( dir == IOREQ_WRITE &&
> +         bytes == 4 )

Don't you also need to check port == 0xcf8 here?

> +static ioservid_t next_ioservid(struct domain *d)
> +{
> +    struct hvm_ioreq_server *s;
> +    ioservid_t id;
> +    
> +    ASSERT(spin_is_locked(&d->arch.hvm_domain.ioreq_server.lock));
> +
> +    id = d->arch.hvm_domain.ioreq_server.id;
> +
> + again:
> +    id++;
> +
> +    /* Check for uniqueness */
> +    list_for_each_entry ( s,
> +                          &d->arch.hvm_domain.ioreq_server.list,
> +                          list_entry )
> +    {
> +        if (id == s->id)

The beloved coding style remark again.

> @@ -757,25 +937,34 @@ static int hvm_create_ioreq_server(struct domain *d, 
> domid_t domid)
>          goto fail1;
>  
>      domain_pause(d);
> -    spin_lock(&d->arch.hvm_domain.ioreq_server_lock);
> +    spin_lock(&d->arch.hvm_domain.ioreq_server.lock);
>  
>      rc = -EEXIST;
> -    if ( d->arch.hvm_domain.ioreq_server != NULL )
> +    if ( is_default && d->arch.hvm_domain.default_ioreq_server != NULL )
>          goto fail2;
>  
> -    rc = hvm_ioreq_server_init(s, d, domid);
> +    rc = hvm_ioreq_server_init(s, d, domid, is_default,
> +                               next_ioservid(d));
>      if ( rc )
> -        goto fail2;
> +        goto fail3;
> +
> +    list_add(&s->list_entry,
> +             &d->arch.hvm_domain.ioreq_server.list);
>  
> -    d->arch.hvm_domain.ioreq_server = s;
> +    if ( is_default )
> +        d->arch.hvm_domain.default_ioreq_server = s;
>  
> -    spin_unlock(&d->arch.hvm_domain.ioreq_server_lock);
> +    if (id != NULL)

And again.

> +static struct hvm_ioreq_server *hvm_select_ioreq_server(struct domain *d,
> +                                                        ioreq_t *p)
> +{
> +#define CF8_BDF(cf8)     (((cf8) & 0x00ffff00) >> 8)
> +#define CF8_ADDR_LO(cf8) ((cf8) & 0x000000fc)
> +#define CF8_ADDR_HI(cf8) (((cf8) & 0x0f000000) >> 16)
> +#define CF8_ENABLED(cf8) (!!((cf8) & 0x80000000))
> +
> +    struct hvm_ioreq_server *s;
> +    uint32_t cf8;
> +    uint8_t type;
> +    uint64_t addr;
> +
> +    if ( list_empty(&d->arch.hvm_domain.ioreq_server.list) )
> +        return NULL;
> +
> +    if ( list_is_singular(&d->arch.hvm_domain.ioreq_server.list) ||
> +         (p->type != IOREQ_TYPE_COPY && p->type != IOREQ_TYPE_PIO) )
> +        return d->arch.hvm_domain.default_ioreq_server;
> +
> +    cf8 = d->arch.hvm_domain.pci_cf8;
> +
> +    /*
> +     * NOTE: Only types IOREQ_TYPE_PIO and IOREQ_TYPE_COPY are valid
> +     *       on entry.
> +     */

I realize this got added in response to my comments on the previous
version, but just now I realized that you had already then added a
respective check in the second if() above (which I missed). With that
the comment seems rather useless.

> +        

Line with only blanks.

>  struct hvm_domain {
> -    spinlock_t              ioreq_server_lock;
> -    struct hvm_ioreq_server *ioreq_server;
> +    /* Guest page range used for non-default ioreq servers */
> +    struct {
> +        unsigned long base;
> +        unsigned long mask;
> +        unsigned int  count;

Do you really need count and mask here? I suppose that partly
depends on whether HVM_PARAM_IOREQ_SERVER_PFN can be
changed more than once for a domain. If it can't/shouldn't, I
suppose just mask would suffice (by simply setting all unavailable
bits). If it can, the question arises what the right action is upon
a request to lower the count below the number of currently
registered servers.

Jan


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