[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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





On 9/9/2016 1:26 PM, Yu Zhang wrote:

>>> 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)


Thanks, I think we do not need this variable with hvm_copy_from_guest_phys().

> +    ASSERT(offset + size < PAGE_SIZE);

Surely <= ?


Yes. Thanks.

> +    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().


Agree.

> +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.

How about ioreq_server_read/ops? It is only for this special p2m type.


> @@ -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.

OK. I prefer to add the unlikely().

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


OK. And IIUC, you mean merge to the original comments above the "if (!s)"?
Like this:
        /*
         * For p2m_ioreq_server pages accessed with read-modify-write
         * instructions, we provide a read handler to copy the data to
         * the buffer. For other cases, if there is no suitable backing
         * DM, we just ignore accesses.
         */
        if ( !s )

Jan


Thanks
Yu

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.