Re: [Xen-devel] [PATCH v6 3/4] x86/ioreq server: Handle read-modify-write cases for p2m_ioreq_server pages.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 02.09.16 at 12:47, <yu.c.zhang@xxxxxxxxxxxxxxx> wrote:
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -95,6 +95,41 @@ static const struct hvm_io_handler null_handler = {
>      .ops = &null_ops
>  };
>  
> +static int mem_read(const struct hvm_io_handler *io_handler,
> +                    uint64_t addr,
> +                    uint32_t size,
> +                    uint64_t *data)
> +{
> +    struct domain *currd = current->domain;
> +    unsigned long gmfn = paddr_to_pfn(addr);
> +    unsigned long offset = addr & ~PAGE_MASK;
> +    struct page_info *page = get_page_from_gfn(currd, gmfn, NULL, 
> P2M_UNSHARE);
> +    uint8_t *p;

const (and preferably also void)

> +    ASSERT(offset + size < PAGE_SIZE);

Surely <= ?

> +    if ( !page )
> +        return X86EMUL_UNHANDLEABLE;
> +
> +    p = __map_domain_page(page);
> +    p += offset;
> +    memcpy(data, p, size);
> +
> +    unmap_domain_page(p);
> +    put_page(page);

But anyway - I think rather than all this open coding you would
better call hvm_copy_from_guest_phys().

> +static const struct hvm_io_ops mem_ops = {
> +    .read = mem_read,
> +    .write = null_write
> +};
> +
> +static const struct hvm_io_handler mem_handler = {
> +    .ops = &mem_ops
> +};

I think the mem_ prefix for both objects is a bad one, considering
that this isn't suitable for general memory handling.

> @@ -204,7 +239,15 @@ static int hvmemul_do_io(
>          /* If there is no suitable backing DM, just ignore accesses */
>          if ( !s )
>          {
> -            rc = hvm_process_io_intercept(&null_handler, &p);
> +            /*
> +             * For p2m_ioreq_server pages accessed with read-modify-write
> +             * instructions, we provide a read handler to copy the data to
> +             * the buffer.
> +             */
> +            if ( p2mt == p2m_ioreq_server )

Please add unlikely() here, or aid the compiler in avoiding any
branch by ...

> +                rc = hvm_process_io_intercept(&mem_handler, &p);
> +            else
> +                rc = hvm_process_io_intercept(&null_handler, &p);

... using a conditional expression for the first function argument.

And the comment ahead of the if() now also needs adjustment
(perhaps you want to merge the one you add into that one).

Jan


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