Re: [Xen-devel] [PATCH v3 07/13] xen/arm: compile and initialize vmap

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2013-04-25 at 18:04 +0100, Stefano Stabellini wrote:
> On Thu, 25 Apr 2013, Ian Campbell wrote:
> > On Wed, 2013-04-24 at 20:07 +0100, Stefano Stabellini wrote:
> > > Rename EARLY_VMAP_VIRT_END and EARLY_VMAP_VIRT_START to
> > > VMAP_VIRT_END and VMAP_VIRT_START.
> > > 
> > > Defining VMAP_VIRT_START triggers the compilation of common/vmap.c.
> > > 
> > > Define PAGE_HYPERVISOR and MAP_SMALL_PAGES (unused on ARM).
> > 
> > So our vmap is 2MB mappings only? I suppose that's ok, at least for now.
> 
> No, I explained myself poorly, I meant that we only support 4K mappings
> so MAP_SMALL_PAGES is useless (always set).

Ah, that makes more sense, thanks. (I take it you'll adjust the
changelog?)

> > > +        BUG_ON(!xen_second[second_linear_offset(addr)].pt.valid);
> > > +
> > > +        third = 
> > > __va((paddr_t)xen_second[second_linear_offset(addr)].pt.base
> > > +                << PAGE_SHIFT);
> > > +        if ( third[third_table_offset(addr)].pt.valid )
> > > +            flush_tlb_local();
> > 
> > Why this flush? (I notice create_p2m_mapping does the same but with
> > _all_local())
> 
> I dropped the _all_local because we don't want to flush all VMIDs here,
> only the hypervisor mappings.
> However I am not sure that this flush is actually necessary as we are
> going to flush the mapping later on after the pte has been written.

Right, that was what I was really trying to ask: why is this flush
necessary at all (not why did you drop the all_local bit).

I can't see any reason why it would be needed on INSERT (really UPDATE
if it's already present) and on REMOVE the flush is obviously needed,
but not until after you've cleared the entry?

> > Isn't it a bug for the third to be already mapped? that suggests
> > something is overwriting the mapping, does vmap do that?
> 
> I don't know, but I thought that this function should be able to handle
> that case.

At the vmap layer I think it must always be a bug for it to be trying to
replace a mapping, the API simply doesn't allow for the possibility.

At the create_xen_mapping/map_pages_to_xen layer I think it would be
very uncommon to be replacing an existing Xen mapping, to the extent
that I think it should be an explicit separate UPDATE operation via an
update_xen_mapping() style API. But until we have a real usecase for
that I think we should just fail such attempts noisily -- they almost
certainly represent something gone awry.

Ian.


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