Xen project Mailing List

On Thu, Mar 12, 2015 at 4:13 PM, Julien Grall <julien.grall@xxxxxxxxxx> 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

Re: [Xen-devel] [PATCH V13 4/7] xen/arm: Data abort exception (R/W) mem_events.