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

[Yu Zhang <yu.c.zhang@linux.intel.com>]

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