On Thu, Mar 12, 2015 at 4:13 PM, Julien Grall <julien.gr...@linaro.org>
wrote:
> Hi Tamas,
>
> On 06/03/15 21:24, Tamas K Lengyel wrote:
> > + /*
> > + * Preempt setting mem_access permissions as required by XSA-89,
> > + * if it's not the last iteration.
> > + */
> > + if ( op == MEMACCESS && count )
> > + {
> > + uint32_t progress = paddr_to_pfn(addr) - sgfn + 1;
> > +
> > + if ( (egfn - sgfn) > progress && !(progress & mask)
> > + && hypercall_preempt_check() )
> > + {
> > + rc = progress;
> > + goto out;
> > + }
> > + }
> > +
>
> Would it be possible to merge the memaccess prempt check with the
> relinquish one?
>
> That may require some change in the relinquish version but I think it
> would be cleaner.
>
Well, I don't really see an obvious way how the two could be combined.
>
> [..]
>
> > +bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct
> npfec npfec)
> > +{
> > + int rc;
> > + bool_t violation;
> > + xenmem_access_t xma;
> > + mem_event_request_t *req;
> > + struct vcpu *v = current;
> > + struct p2m_domain *p2m = p2m_get_hostp2m(v->domain);
> > +
> > + /* Mem_access is not in use. */
> > + if ( !p2m->mem_access_enabled )
> > + return true;
>
> true should be used with bool not boot_t.
>
> > +
> > + rc = p2m_get_mem_access(v->domain, paddr_to_pfn(gpa), &xma);
> > + if ( rc )
> > + return true;
>
> Ditto (I won't repeat for the few other place below)
>
There was just a discussion how there is no difference between 0/1 and
false/true other than maybe style. If one is preferred over the other, I'm
fine with either. Is this really an issue?
>
> > +
> > + /* Now check for mem_access violation. */
> > + switch ( xma )
> > + {
> > + case XENMEM_access_rwx:
> > + violation = false;
> > + break;
> > + case XENMEM_access_rw:
> > + violation = npfec.insn_fetch;
> > + break;
> > + case XENMEM_access_wx:
> > + violation = npfec.read_access;
> > + break;
> > + case XENMEM_access_rx:
> > + case XENMEM_access_rx2rw:
> > + violation = npfec.write_access;
> > + break;
> > + case XENMEM_access_x:
> > + violation = npfec.read_access || npfec.write_access;
> > + break;
> > + case XENMEM_access_w:
> > + violation = npfec.read_access || npfec.insn_fetch;
> > + break;
> > + case XENMEM_access_r:
> > + violation = npfec.write_access || npfec.insn_fetch;
> > + break;
> > + default:
> > + case XENMEM_access_n:
> > + case XENMEM_access_n2rwx:
> > + violation = true;
> > + break;
> > + }
> > +
> > + if ( !violation )
> > + return true;
>
> Ditto
>
> > +
> > + /* First, handle rx2rw and n2rwx conversion automatically. */
> > + if ( npfec.write_access && xma == XENMEM_access_rx2rw )
> > + {
> > + rc = p2m_set_mem_access(v->domain, paddr_to_pfn(gpa), 1,
> > + 0, ~0, XENMEM_access_rw);
> > + return false;
>
> Same as true.
>
> [..]
>
> > + if ( !i )
> > + {
> > + /*
> > + * No setting was found in the Radix tree. Check if the
> > + * entry exists in the page-tables.
> > + */
> > + paddr_t maddr = p2m_lookup(d, gpfn << PAGE_SHIFT, NULL);
> > + if ( INVALID_PADDR == maddr )
> > + return -ESRCH;
> > +
> > + /* If entry exists then its rwx. */
> > + *access = XENMEM_access_rwx;
> > + }
> > + else
> > + {
> > + /* Setting was found in the Radix tree. */
> > + index = radix_tree_ptr_to_int(i);
> > + if ( index >= ARRAY_SIZE(memaccess) )
> > + return -ERANGE;
>
> We trust the value stored in the radix tree. So I think this could be
> turned into an ASSERT/BUG_ON.
>
Sure.
>
> [..]
>
> > @@ -243,12 +245,17 @@ static inline bool_t
> p2m_mem_event_sanity_check(struct domain *d)
> >
> > /* Get access type for a pfn
> > * If pfn == -1ul, gets the default access type */
> > -static inline
> > int p2m_get_mem_access(struct domain *d, unsigned long pfn,
> > - xenmem_access_t *access)
> > -{
> > - return -ENOSYS;
> > -}
> > + xenmem_access_t *access);
> > +
>
> p2m_get_mem_access is called by the common code. So it should be moved
> in xen/include/xen/p2m-common.h
>
> In general, any function called by common code should be have the
> prototype declared in common header.
>
Ack, I'll move it into p2m-common.h once the function in ARM is actually
defined in the subsequent patch.
>
> Regards,
>
> --
> Julien Grall
>
Thanks,
Tamas
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
