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

2016-09-09 Thread Jan Beulich 
>>> On 09.09.16 at 08:21,  wrote:
> On 9/9/2016 1:26 PM, Yu Zhang wrote:
>> >>> On 02.09.16 at 12:47,  wrote:
>> > +static const struct hvm_io_ops mem_ops = {
>> > +.read = mem_read,
>> > +.write = null_write
>> > +};
>> > +
>> > +static const struct hvm_io_handler mem_handler = {
>> > +.ops = _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.

SGTM.

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

Yes, thanks.

Jan


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

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

2016-09-09 Thread Yu Zhang 



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


>>> On 02.09.16 at 12:47,  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 = _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 = _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(_handler, );
> +/*
> + * 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(_handler, );
> +else
> +rc = hvm_process_io_intercept(_handler, );

... 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@lists.xen.org
https://lists.xen.org/xen-devel

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

2016-09-05 Thread Jan Beulich 
>>> On 02.09.16 at 12:47,  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 = _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 = _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(_handler, );
> +/*
> + * 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(_handler, );
> +else
> +rc = hvm_process_io_intercept(_handler, );

... 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@lists.xen.org
https://lists.xen.org/xen-devel