Re: [Xen-devel] [PATCH 16/22] vixen: pass grant table operations through to the outer Xen

[Anthony Liguori <anthony@xxxxxxxxxxxxx>]

On Sun, Jan 7, 2018 at 8:45 AM, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
> On 07/01/2018 15:42, Anthony Liguori wrote:
>> On Sun, Jan 7, 2018 at 12:36 AM, Roger Pau Monné <roger.pau@xxxxxxxxxx> 
>> wrote:
>>> On Sat, Jan 06, 2018 at 02:54:31PM -0800, Anthony Liguori wrote:
>>>>  static long
>>>> +vixen_gnttab_setup_table(
>>>> +    XEN_GUEST_HANDLE_PARAM(gnttab_setup_table_t) uop, unsigned int count)
>>>> +{
>>>> +    long rc;
>>>> +
>>>> +    struct gnttab_setup_table op;
>>>> +    xen_pfn_t *frame_list = NULL;
>>>> +    static void *grant_table;
>>>> +    XEN_GUEST_HANDLE(xen_pfn_t) old_frame_list;
>>>> +
>>>> +    if ( count != 1 )
>>>> +        return -EINVAL;
>>>> +
>>>> +    if ( unlikely(copy_from_guest(&op, uop, 1) != 0) )
>>>> +    {
>>>> +        gdprintk(XENLOG_INFO, "Fault while reading 
>>>> gnttab_setup_table_t.\n");
>>>> +        return -EFAULT;
>>>> +    }
>>>> +
>>>> +    if ( grant_table == NULL ) {
>>>> +        struct xen_add_to_physmap xatp;
>>>> +        struct domain *d;
>>>> +        int i;
>>>> +
>>>> +        for ( i = 0; i < max_grant_frames; i++ )
>>>> +        {
>>>> +             grant_table = alloc_xenheap_page();
>>> This is wasting one memory page, grant table frames don't need to be
>>> populated.
>> Well they have to have a valid struct page_info in order for the guest
>> to map it within its address space.
>>
>> Or did you have something else in mind?
>
> Mapping of L0 frames into L1 is a giant mess.
>
> First of all, some technical facts:
> 1) Frames which we map from L0 into L1 do not need to replace existing
> RAM.  We can use any GFNs up to maxphysaddr.
> 2) Mapped frames should not replace RAM, and particularly not frames in
> .data or .bss, because of the performance hit from shattered host
> superpages.
> 3) Ideally, we'd want to map into entirely unused GFNs, because then we
> don't have to interfere with what was there before.
>
> In Xen, to allow a frame to be used by a guest, we need to set up domain
> ownership for it.  This requires a struct page_info to exist, which by
> default only occurs for pages L1 Xen things is RAM.
>
> There is a completely gross way of dealing with this by faking up L1's
> E820 map to include a range as RAM, and adding every entry in that range
> into the badpages list.  This causes L1 Xen to put together page_info's
> for them, but otherwise ignore their existence.

I'll look at this.  I know it's gross but it's pretty straight forward.

> Off the top of my head, frames needing special attention are:
> * The special pages, including Xenstore and Console rings.  These are
> real frames (as opposed to mappings), but live inside an E820 hole from
> L1's point of view.
> * Shared info
> * Grant table/status frames
> * Vcpuinfo frames

I think you mean the runstate area.  We punch this through in Vixen so
it doesn't need special handling.

> * Event_fifo (if we care to wire that up, but perhaps its not worth it).

I don't think it's worth it TBH.

> What I started doing in PV-shim (before switching to the SP2 side of
> things fully) was to hard code these mapping frames immediately after
> the special pages, which is a horrible but safe (as far as I can tell)
> way of doing things.

I'll take this path after checking myself.

> Ideally, L1 could work out a safe place to use for mappings (which
> ideally, would be a block of GFNs immediately above the last used
> frame), but this cannot be done with the toolstack-provided E820 alone,
> because it is insufficiently descriptive as it deliberately omits
> information which can be found in the DSDT (e.g. ACPI hotplug regions).
>
> The only reasonable option is for L0 to fully understand the guest
> physical address, and be able to report the details fully to L1,
> probably in an E820-like way but with our own type identifiers to cover
> the options which aren't in the E820 spec.
>
> This allows L1 to be positively told information such as "This range is
> safe for mapping into", without having to go and parse all the secondary
> layout information which is derived from this information in the first
> place.  Having said that, this will require hypervisor and toolstack
> changes, so isn't reasonable to retrofit.

Right.  With the current series, no changes are needed to the hypervisor or
toolstack which is pretty powerful.  Maintaining that property is pretty useful.

> Overall I want to ensure that, whatever plan we come up with for the
> shim, it doesn't further tangle things up and make them harder to untangle.

For sure.  I think perhaps codifying the HVM/PVH ABI to say that the special
pages region is, well, special and describing it a bit more is a nice way to
keep things simple but also make it less of a hack.

Regards,

Anthony Liguori

>
> ~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel